feat(handlers): add callstack_resolve and section_write (Phase 5C)

[BANANASJIM]

User description

Summary

• Add callstack_resolve daemon handler: resolves CPU callstacks via InitResolver() + GetResolve(), with proper guards for missing callstacks and unavailable symbols
• Add section_write daemon handler: writes custom sections into .rdc files via base64-encoded data, with protection against overwriting system sections (FrameCapture, ResolveDatabase, etc.)
• Add rdc callstacks [--eid N] [--json] CLI command with 60s timeout for symbol loading latency
• Extend rdc section <name> --write <file> CLI command supporting file and stdin (-) input
• Add timeout parameter to call() helper for per-command timeout control
• 22 handler tests + 17 CLI tests + 5 E2E tests

Test plan

• pixi run lint — zero errors
• pixi run test — 2488 passed, 93.80% coverage
• pixi run test-e2e — GPU e2e tests pass
• Manual: rdc open fixture.rdc → rdc callstacks → exits 1 "no callstacks"
• Manual: rdc section MyNote --write /tmp/note.txt → write + readback

---

PR Type

Enhancement

---

Description

• Add callstack_resolve daemon handler resolving CPU callstacks via InitResolver/GetResolve

• Add section_write daemon handler writing custom sections with built-in section protection

• Add rdc callstacks [--eid N] [--json] CLI command with 60s timeout

• Extend rdc section <name> --write <file> supporting file and stdin input

• Add timeout parameter to call() helper for per-command timeout control

• 44 handler tests + 17 CLI tests + 5 E2E tests covering all error paths

---

Diagram Walkthrough

flowchart LR
  CLI["CLI Commands<br/>callstacks, section --write"]
  CALL["call() helper<br/>timeout param"]
  HANDLERS["Daemon Handlers<br/>callstack_resolve, section_write"]
  CAP["Capture API<br/>InitResolver, GetResolve, WriteSection"]
  
  CLI -- "invoke" --> CALL
  CALL -- "timeout=60s" --> HANDLERS
  HANDLERS -- "resolve/write" --> CAP

Loading

File Walkthrough

	Relevant files
Enhancement	4 files cli.py Register callstacks_cmd in main CLI                                            +2/-0      _helpers.py Add timeout parameter to call() helper                                      +3/-2      capturefile.py Add callstacks_cmd and section_write support                          +45/-3    capturefile.py Implement callstack_resolve and section_write handlers      +135/-1
Tests	16 files test_capturefile.py Add E2E tests for callstacks and section write                      +93/-0    mock_renderdoc.py Add mock support for callstack and section APIs                    +19/-1    conftest.py Update send_request mock to accept timeout kwarg                  +1/-1      test_capturefile_commands.py Add 17 CLI tests for callstacks and section write                +224/-0  test_capturefile_handlers.py Add 44 handler tests for callstack and section logic          +351/-0  test_debug_commands.py Update send_request mock signature for timeout                      +1/-1      test_info_commands.py Update send_request mock signature for timeout                      +4/-2      test_pipeline_cli_phase27.py Update send_request mock signature for timeout                      +2/-2      test_pipeline_commands.py Update send_request mock signature for timeout                      +4/-4      test_pipeline_shader.py Update send_request mock signature for timeout                      +3/-3      test_resources_commands.py Update send_request mock signature for timeout                      +1/-1      test_resources_filter.py Update send_request mock signature for timeout                      +5/-5      test_script_command.py Update send_request mock signature for timeout                      +5/-5      test_search.py Update send_request mock signature for timeout                      +13/-3    test_shader_preload.py Update send_request mock signature for timeout                      +2/-2      test_unix_helpers_commands.py Update send_request mock signature for timeout                      +1/-1
Documentation	1 files commands-quick-ref.md Document callstacks command and section --write option      +13/-1

---

Summary by CodeRabbit

• New Features

  • Added an rdc callstacks command to resolve and display CPU call stacks for capture events (supports event ID filtering and JSON output).

• Improvements

  • Enhanced rdc section command with a --write option to write named section contents to files or stdin.

• Quality

  • Expanded test coverage and improved test robustness for capturefile, section write, and callstacks behaviors.

• Documentation

  • Updated command reference documentation.

[coderabbitai]

Warning

Rate limit exceeded

@BANANASJIM has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 27 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4845800 and a3e3bfa.

📒 Files selected for processing (2)

• src/rdc/handlers/capturefile.py
• tests/unit/test_capturefile_handlers.py

📝 Walkthrough

Walkthrough

Added a new rdc callstacks CLI command and server handler to resolve CPU callstacks for events; extended rdc section to support writing section contents via a --write option; RPC helper gained a timeout parameter. Tests and mocks updated to cover new flows.

Changes

Cohort / File(s)	Summary
Documentation src/rdc/_skills/references/commands-quick-ref.md	Documented new rdc callstacks command (--eid, --json) and enhanced rdc section docs with --write option and updated description.
CLI Integration src/rdc/cli.py	Exposed new callstacks subcommand by importing callstacks_cmd from capturefile.
Command Implementation src/rdc/commands/capturefile.py	Added callstacks_cmd(eid, use_json) to request/resolved callstacks (uses longer timeout) and extended section_cmd(...) to accept --write / write_file for sending base64 payloads to the daemon.
RPC Helper src/rdc/commands/_helpers.py	Changed call() signature to accept keyword-only timeout: float = 30.0 and pass it to send_request, allowing per-call timeouts.
Daemon Handlers src/rdc/handlers/capturefile.py	Added _handle_callstack_resolve and _handle_section_write, _parse_resolve_string, _PROTECTED_SECTION_TYPES and registered handlers (callstack_resolve, section_write) with validation, bounds checks, base64 handling and error paths.
Mocks tests/mocks/mock_renderdoc.py	Extended mock types and behavior: SectionType add EmbeddedLogfile, MockReplayController.GetCallstack(), and MockCaptureFile add HasCallstacks/InitResolver/GetResolve/WriteSection plus written-section tracking.
Unit tests — commands tests/unit/test_capturefile_commands.py	Added tests for callstacks_cmd (TSV/JSON, EID filtering, error cases) and section_cmd --write (text, binary, zero-byte, error paths).
Unit tests — handlers tests/unit/test_capturefile_handlers.py	Extensive handler unit tests covering callstack resolution states, EID bounds, resolver errors, and section write validation/error cases.
End-to-end tests tests/e2e/test_capturefile.py	Added TestCallstacks and TestSectionWrite E2E tests exercising CLI flows, round-trip section write/readback, and built-in-section rejection.
Test harness adjustments tests/unit/... (many files)	Updated numerous test mocks to accept arbitrary keyword args (**_kw) in patched send_request and capture helpers to accommodate added timeout/options without failing.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant CLI as "rdc CLI"
participant Cmd as "capturefile.cmd (callstacks/section)"
participant RPC as "commands._helpers.call (RPC)"
participant Daemon as "rdc daemon (HANDLERS)"
participant Capture as "CaptureFile / ReplayController"
CLI->>Cmd: user invokes rdc callstacks / rdc section --write
Cmd->>RPC: call(method, params..., timeout=...)
RPC->>Daemon: send_request(payload)
Daemon->>Daemon: route to handler (callstack_resolve / section_write)
Daemon->>Capture: query HasCallstacks / InitResolver / WriteSection / Resolve
Capture-->>Daemon: resolve result or write ack
Daemon-->>RPC: reply(result or error)
RPC-->>Cmd: return result
Cmd-->>CLI: print output (JSON or text)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• PR #156: Touches src/rdc/commands/_helpers.py and modifies call() behavior/signature — closely related to the timeout addition.
• PR #108: Adds capturefile CLI/handler foundations that this change extends (additional HANDLERS and CLI surface).
• PR #134: Also modifies error-handling and helper utilities in _helpers.py, overlapping the same file and ecosystem.

Poem

🐰 I hopped through stacks and sections bright,

I stitched base64 by moonlit night,
Timeouts set to calm the sway,
Frames resolved to guide the way—
A tiny rabbit cheers: Hooray!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 48.78% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the primary changes: adding two new daemon handlers (callstack_resolve and section_write) with a phase identifier for context.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/phase5c-callstack-section-write

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Resource exhaustion Description: The new section_write handler accepts arbitrarily large base64 payloads (data) and decodes/writes them via base64.b64decode() and state.cap.WriteSection() without any size limits or streaming, which could allow an authenticated client to trigger memory/disk exhaustion or long blocking writes by sending a very large section body. capturefile.py [183-239] Referred Code def _handle_section_write( request_id: int, params: dict[str, Any], state: DaemonState ) -> tuple[dict[str, Any], bool]: """Write a named section to the capture file.""" if state.cap is None: return _error_response(request_id, -32002, "no capture file open"), True name = params.get("name") if not name or not isinstance(name, str): return _error_response(request_id, -32602, "missing or empty 'name'"), True data_b64 = params.get("data") if data_b64 is None or not isinstance(data_b64, str): return _error_response(request_id, -32602, "missing 'data' parameter"), True idx = state.cap.FindSectionByName(name) if idx >= 0: props = state.cap.GetSectionProperties(idx) type_name = getattr(getattr(props, "type", None), "name", "") if type_name in _PROTECTED_SECTION_TYPES: return _error_response( ... (clipped 36 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing write audit: The new section_write handler performs a capture-file write but does not emit any audit log entry identifying who/what performed the write and its outcome. Referred Code def _handle_section_write( request_id: int, params: dict[str, Any], state: DaemonState ) -> tuple[dict[str, Any], bool]: """Write a named section to the capture file.""" if state.cap is None: return _error_response(request_id, -32002, "no capture file open"), True name = params.get("name") if not name or not isinstance(name, str): return _error_response(request_id, -32602, "missing or empty 'name'"), True data_b64 = params.get("data") if data_b64 is None or not isinstance(data_b64, str): return _error_response(request_id, -32602, "missing 'data' parameter"), True idx = state.cap.FindSectionByName(name) if idx >= 0: props = state.cap.GetSectionProperties(idx) type_name = getattr(getattr(props, "type", None), "name", "") if type_name in _PROTECTED_SECTION_TYPES: return _error_response( ... (clipped 37 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unhandled write failure: The new section_write path calls state.cap.WriteSection(...) without catching exceptions, which can crash request processing instead of returning a structured JSON-RPC error. Referred Code def _handle_section_write( request_id: int, params: dict[str, Any], state: DaemonState ) -> tuple[dict[str, Any], bool]: """Write a named section to the capture file.""" if state.cap is None: return _error_response(request_id, -32002, "no capture file open"), True name = params.get("name") if not name or not isinstance(name, str): return _error_response(request_id, -32602, "missing or empty 'name'"), True data_b64 = params.get("data") if data_b64 is None or not isinstance(data_b64, str): return _error_response(request_id, -32602, "missing 'data' parameter"), True idx = state.cap.FindSectionByName(name) if idx >= 0: props = state.cap.GetSectionProperties(idx) type_name = getattr(getattr(props, "type", None), "name", "") if type_name in _PROTECTED_SECTION_TYPES: return _error_response( ... (clipped 37 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Leaks exception details: The new callstack_resolve handler returns the raw exception string in symbols not available: {exc}, potentially exposing internal environment details to the end-user. Referred Code def _handle_callstack_resolve( request_id: int, params: dict[str, Any], state: DaemonState ) -> tuple[dict[str, Any], bool]: """Resolve CPU callstack for an event ID.""" if state.cap is None: return _error_response(request_id, -32002, "no capture file open"), True if not state.cap.HasCallstacks(): return _error_response(request_id, -32002, "no callstacks recorded in this capture"), True try: ok = state.cap.InitResolver(interactive=False, progress=None) except Exception as exc: # noqa: BLE001 return _error_response(request_id, -32002, f"symbols not available: {exc}"), True if not ok: return _error_response(request_id, -32002, "symbols not available"), True Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Write authorization unclear: The new section_write handler accepts arbitrary base64 payloads and writes them into the open capture, but the diff does not show any explicit authorization/permission check beyond the RPC token mechanism, requiring verification that access is properly constrained. Referred Code def _handle_section_write( request_id: int, params: dict[str, Any], state: DaemonState ) -> tuple[dict[str, Any], bool]: """Write a named section to the capture file.""" if state.cap is None: return _error_response(request_id, -32002, "no capture file open"), True name = params.get("name") if not name or not isinstance(name, str): return _error_response(request_id, -32602, "missing or empty 'name'"), True data_b64 = params.get("data") if data_b64 is None or not isinstance(data_b64, str): return _error_response(request_id, -32602, "missing 'data' parameter"), True idx = state.cap.FindSectionByName(name) if idx >= 0: props = state.cap.GetSectionProperties(idx) type_name = getattr(getattr(props, "type", None), "name", "") if type_name in _PROTECTED_SECTION_TYPES: return _error_response( ... (clipped 37 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Stream file to avoid high memory usage Refactor the file writing logic to use a streaming approach for base64 encoding. This avoids reading the entire file into memory, which improves performance and reduces memory consumption for large files. src/rdc/commands/capturefile.py [73-87] if write_file is not None: + # Use a streaming approach for base64 encoding to handle large files + # without reading them entirely into memory. + import io + if str(write_file) == "-": - data = click.get_binary_stream("stdin").read() + input_stream = click.get_binary_stream("stdin") else: try: - data = write_file.read_bytes() + input_stream = write_file.open("rb") except OSError as exc: click.echo(f"error: {exc}", err=True) raise SystemExit(1) from exc - encoded = base64.b64encode(data).decode() + + with input_stream: + output_buffer = io.StringIO() + base64.encode(input_stream, output_buffer) + encoded = output_buffer.getvalue() + result = call("section_write", {"name": name, "data": encoded}) click.echo(f"section '{name}' written ({result['bytes']} bytes)", err=True) if use_json: click.echo(json.dumps(result)) return Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential memory issue when reading large files and proposes a streaming approach, which is a significant improvement for robustness and scalability.	Medium
	error when resolver missing Return a clear JSON-RPC error if the GetCallstack method is not available on the controller. Currently, it fails silently by returning an empty list of frames. src/rdc/handlers/capturefile.py [167-173] ctrl = getattr(state.adapter, "controller", None) if state.adapter else None get_cs = getattr(ctrl, "GetCallstack", None) if ctrl else None -if callable(get_cs): - try: - callstack = list(get_cs(eid)) - except Exception: # noqa: BLE001 - _log.debug("GetCallstack(%d) failed, falling back", eid) +if not callable(get_cs): + return _error_response(request_id, -32002, "callstack resolver unavailable"), True +try: + callstack = list(get_cs(eid)) +except Exception: # noqa: BLE001 + _log.debug("GetCallstack(%d) failed, falling back", eid) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the current implementation silently fails if GetCallstack is not available. Returning an explicit error provides clearer feedback to the user.	Low
	validate non-negative eid Add validation to the callstacks command to ensure the --eid argument is a non-negative integer. This provides earlier and more specific error feedback to the user. src/rdc/commands/capturefile.py [101-102] if eid is not None: + if eid < 0: + raise click.BadParameter("eid must be non-negative") params["eid"] = eid Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly proposes adding client-side validation for the eid parameter. This provides faster and clearer feedback to the user for invalid input.	Low
Possible issue	Improve parsing of callstack strings Improve the parsing of callstack strings to correctly handle file paths that contain spaces. The current implementation may fail in such cases. src/rdc/handlers/capturefile.py [124-140] def _parse_resolve_string(s: str) -> dict[str, Any]: """Parse a resolve string like ``'func file.c:42'`` into a frame dict.""" - parts = s.rsplit(" ", 1) - func = parts[0] if len(parts) > 1 else s - file_line = parts[1] if len(parts) > 1 else "" - file_name = "" - line = 0 - if ":" in file_line: - fl_parts = file_line.rsplit(":", 1) - file_name = fl_parts[0] + func, file_name, line = s, "", 0 + + # Find the last colon, which typically separates file path and line number. + # This is more robust than splitting by space, which fails for paths with spaces. + last_colon_idx = s.rfind(":") + if last_colon_idx > 0: try: - line = int(fl_parts[1]) - except ValueError: - file_name = file_line - else: - file_name = file_line + # Check if the part after the colon is a valid line number + line_num_str = s[last_colon_idx + 1 :] + if line_num_str.isdecimal(): + line = int(line_num_str) + file_part = s[:last_colon_idx] + # The function is everything before the last space in the file part + last_space_idx = file_part.rfind(" ") + if last_space_idx > 0: + func = file_part[:last_space_idx] + file_name = file_part[last_space_idx + 1 :] + except (ValueError, IndexError): + # Parsing failed, fall back to original string as function + pass + return {"function": func, "file": file_name, "line": line} Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential parsing bug with file paths containing spaces and proposes a more robust parsing logic, which improves the reliability of the new callstacks feature.	Low
	check for missing result key Add a check to ensure the result key exists in the daemon response before accessing it. This prevents a KeyError if the response is malformed. src/rdc/commands/_helpers.py [120-122] if "error" in response: _emit_error(response["error"]["message"]) +if "result" not in response: + _emit_error("malformed response from daemon") return cast(dict[str, Any], response["result"]) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion improves robustness by adding a check for the result key in the daemon response, preventing a potential KeyError from a malformed response.	Low
Update

[coderabbitai]

🧹 Nitpick comments (3)

src/rdc/commands/_helpers.py (1)

125-142: Consider adding timeout parameter to try_call for consistency.

try_call currently doesn't support the timeout parameter while call does. If callers need longer timeouts for optional features, they'd be unable to configure it.

♻️ Optional: Add timeout to try_call

-def try_call(method: str, params: dict[str, Any]) -> dict[str, Any] | None:
+def try_call(method: str, params: dict[str, Any], *, timeout: float = 30.0) -> dict[str, Any] | None:
     """Send a JSON-RPC request, returning None on failure.

     Unlike call(), this never exits -- failures are silent.
     Use for optional features where partial success is acceptable.
     """
     try:
         host, port, token = require_session()
     except SystemExit:
         return None
     payload = _request(method, 1, {"_token": token, **params}).to_dict()
     try:
-        response = send_request(host, port, payload)
+        response = send_request(host, port, payload, timeout=timeout)
     except (OSError, ValueError):
         return None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/rdc/commands/_helpers.py` around lines 125 - 142, The try_call function
lacks a timeout parameter while call supports one; add an optional timeout:
float | None = None to try_call's signature and docstring (mirror call's
default), pass it through to send_request(host, port, payload, timeout=timeout),
and update the return/typing accordingly; ensure require_session(),
_request(method, 1, ...), and any callers of try_call are compatible with the
new optional parameter (update usages if they need to set a timeout).

tests/unit/test_capturefile_handlers.py (1)

302-321: Thorough multi-frame test with minor style suggestion.

The test correctly validates frame ordering and content parsing. Minor suggestion: line 311's [i for i in range(5)] can be simplified to list(range(5)).

♻️ Suggested simplification

     resolve = [f"func{i} file{i}.c:{i * 10}" for i in range(5)]
     state = _make_callstack_state(
         tmp_path,
         monkeypatch,
-        callstack_addrs=[i for i in range(5)],
+        callstack_addrs=list(range(5)),
         resolve_strings=resolve,
     )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_capturefile_handlers.py` around lines 302 - 321, In
test_callstack_resolve_multi_frame replace the explicit comprehension used for
callstack_addrs with the simpler built-in conversion: change the argument
callstack_addrs=[i for i in range(5)] to callstack_addrs=list(range(5)) in the
test_callstack_resolve_multi_frame function to improve readability and style.

tests/unit/test_capturefile_commands.py (1)

162-176: Consider using the top-level json import.

The test imports json as _json locally, but json is already imported at the module level in src/rdc/commands/capturefile.py. If this test file needs JSON parsing, consider adding import json to the top-level imports instead of the local alias.

♻️ Suggested refactor

Add to top-level imports:

import json

Then in the test:

 def test_section_write_json(monkeypatch, tmp_path: Path) -> None:
     """T-5C-28: --json on write emits JSON."""
-    import json as _json
-
     f = tmp_path / "notes.txt"
     f.write_bytes(b"hello")
     patch_cli_session(monkeypatch, {"name": "MyNotes", "bytes": 5})
     result = CliRunner().invoke(section_cmd, ["MyNotes", "--write", str(f), "--json"])
     assert result.exit_code == 0
     # stdout has confirmation on stderr + JSON; extract JSON line
     json_lines = [ln for ln in result.output.splitlines() if ln.startswith("{")]
     assert json_lines, "no JSON line in output"
-    data = _json.loads(json_lines[0])
+    data = json.loads(json_lines[0])
     assert data["name"] == "MyNotes"
     assert data["bytes"] == 5

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_capturefile_commands.py` around lines 162 - 176, The test
test_section_write_json is locally importing json as _json; instead add a
top-level import json to the test module and switch the test to use that
top-level json (remove the local `import json as _json`), so data =
json.loads(...) and other json references use the module-level import; this
keeps imports consistent with the rest of the codebase and avoids the local
alias.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/rdc/commands/_helpers.py`:
- Around line 125-142: The try_call function lacks a timeout parameter while
call supports one; add an optional timeout: float | None = None to try_call's
signature and docstring (mirror call's default), pass it through to
send_request(host, port, payload, timeout=timeout), and update the return/typing
accordingly; ensure require_session(), _request(method, 1, ...), and any callers
of try_call are compatible with the new optional parameter (update usages if
they need to set a timeout).

In `@tests/unit/test_capturefile_commands.py`:
- Around line 162-176: The test test_section_write_json is locally importing
json as _json; instead add a top-level import json to the test module and switch
the test to use that top-level json (remove the local `import json as _json`),
so data = json.loads(...) and other json references use the module-level import;
this keeps imports consistent with the rest of the codebase and avoids the local
alias.

In `@tests/unit/test_capturefile_handlers.py`:
- Around line 302-321: In test_callstack_resolve_multi_frame replace the
explicit comprehension used for callstack_addrs with the simpler built-in
conversion: change the argument callstack_addrs=[i for i in range(5)] to
callstack_addrs=list(range(5)) in the test_callstack_resolve_multi_frame
function to improve readability and style.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fca28ce and 22d2e5f.

📒 Files selected for processing (21)

• src/rdc/_skills/references/commands-quick-ref.md
• src/rdc/cli.py
• src/rdc/commands/_helpers.py
• src/rdc/commands/capturefile.py
• src/rdc/handlers/capturefile.py
• tests/e2e/test_capturefile.py
• tests/mocks/mock_renderdoc.py
• tests/unit/conftest.py
• tests/unit/test_capturefile_commands.py
• tests/unit/test_capturefile_handlers.py
• tests/unit/test_debug_commands.py
• tests/unit/test_info_commands.py
• tests/unit/test_pipeline_cli_phase27.py
• tests/unit/test_pipeline_commands.py
• tests/unit/test_pipeline_shader.py
• tests/unit/test_resources_commands.py
• tests/unit/test_resources_filter.py
• tests/unit/test_script_command.py
• tests/unit/test_search.py
• tests/unit/test_shader_preload.py
• tests/unit/test_unix_helpers_commands.py

[greptile-apps]

Greptile Summary

This PR successfully implements Phase 5C by adding CPU callstack resolution and custom section writing capabilities to the RenderDoc CLI. The implementation is well-designed with proper error handling, input validation, and security protections.

Key Changes:

• Added callstack_resolve daemon handler that resolves CPU callstacks via InitResolver() and GetResolve(), with guards for missing callstacks and unavailable symbols
• Added section_write daemon handler that writes custom sections with base64-encoded data and protects against overwriting system sections (FrameCapture, ResolveDatabase, etc.)
• Added rdc callstacks [--eid N] [--json] CLI command with 60s timeout to accommodate symbol loading latency
• Extended rdc section <name> --write <file> to support both file and stdin (-) input
• Added timeout parameter to call() helper for per-command timeout control (default 30s)
• Comprehensive test coverage: 44 handler tests + 17 CLI tests + 5 E2E tests (93.80% coverage)

Code Quality Highlights:

• Proper error sanitization: exception details logged but not exposed to users
• Defensive programming: graceful fallback to SimpleNamespace when SectionProperties unavailable
• Robust string parsing in _parse_resolve_string() handling edge cases
• Section protection mechanism correctly checks section TYPE (not just name) against protected list
• All test mocks updated to accept timeout parameter consistently

Confidence Score: 5/5

• This PR is safe to merge with high confidence
• The implementation demonstrates excellent code quality with comprehensive error handling, proper input validation, security-conscious error message sanitization, protection against overwriting system sections, and extensive test coverage (93.80%). The changes follow established patterns in the codebase, include both unit and E2E tests covering all error paths, and add useful functionality without introducing breaking changes.
• No files require special attention

Important Files Changed

Filename	Overview
src/rdc/handlers/capturefile.py	Added callstack_resolve and section_write handlers with proper error handling, input validation, and protection against overwriting system sections
src/rdc/commands/capturefile.py	Added callstacks command with 60s timeout and extended section command with --write option supporting file and stdin input
src/rdc/commands/_helpers.py	Added timeout parameter to call() helper with default 30s, enabling per-command timeout control
tests/unit/test_capturefile_handlers.py	Added 44 comprehensive handler tests covering callstack resolution and section write with edge cases and error paths
tests/unit/test_capturefile_commands.py	Added 17 CLI tests for callstacks command and section write functionality with various input scenarios
tests/e2e/test_capturefile.py	Added 5 E2E tests verifying callstacks behavior on real captures and section write/read round-trips with protection enforcement

Sequence Diagram

sequenceDiagram
    participant CLI as CLI Command
    participant Helper as call() helper
    participant Daemon as Daemon Handler
    participant RDC as RenderDoc API

    Note over CLI,RDC: Callstack Resolution Flow
    CLI->>Helper: callstacks_cmd(eid, json)<br/>timeout=60s
    Helper->>Daemon: callstack_resolve(eid)
    Daemon->>RDC: HasCallstacks()
    RDC-->>Daemon: true
    Daemon->>RDC: InitResolver(interactive=False)
    RDC-->>Daemon: ok
    Daemon->>RDC: controller.GetCallstack(eid)
    RDC-->>Daemon: [addresses]
    Daemon->>RDC: GetResolve(addresses)
    RDC-->>Daemon: ["func file.c:line"]
    Daemon->>Daemon: _parse_resolve_string()
    Daemon-->>Helper: {eid, frames}
    Helper-->>CLI: formatted output

    Note over CLI,RDC: Section Write Flow
    CLI->>Helper: section_cmd(name, write_file)
    CLI->>CLI: read file or stdin
    CLI->>CLI: base64.encode(data)
    Helper->>Daemon: section_write(name, data_b64)
    Daemon->>RDC: FindSectionByName(name)
    RDC-->>Daemon: idx
    alt section exists
        Daemon->>RDC: GetSectionProperties(idx)
        RDC-->>Daemon: props
        Daemon->>Daemon: check if type in PROTECTED_SECTION_TYPES
        alt protected
            Daemon-->>Helper: error: cannot overwrite
        end
    end
    Daemon->>Daemon: base64.decode(data_b64)
    Daemon->>RDC: WriteSection(props, bytes)
    RDC-->>Daemon: success
    Daemon-->>Helper: {name, bytes}
    Helper-->>CLI: confirmation message

Loading

_{Last reviewed commit: a3e3bfa}