feat(ux): error hints + opaque exception catches (Phase 9 PR 3/3)#223
Merged
BANANASJIM merged 11 commits intomasterfrom Apr 18, 2026
Merged
feat(ux): error hints + opaque exception catches (Phase 9 PR 3/3)#223BANANASJIM merged 11 commits intomasterfrom
BANANASJIM merged 11 commits intomasterfrom
Conversation
…e_push to AUR git job
Curly braces in jq examples inside <pre><code> were parsed as Astro JSX
expressions, causing esbuild to fail with "triangles is not defined" and
"{}" parse errors. Wrap affected jq object literals in {"..."} string
expressions. Also add force_push: true to aur-git job to fix non-fast-forward
push rejections from AUR.
pixi run rdc previously fell back to /usr/bin/rdc (stale system install) because no explicit task existed. Adding `rdc = "uv run rdc"` ensures the development version is always used.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds targeted remediation hints and explicit step-labeling to capture, remote-capture, remote-replay, and session-start failure messages; introduces OS-specific and OS-neutral injection hint helpers; tightens exception scopes for remote operations; and updates unit tests to assert presence of hints and step labels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Daemon as Daemon/rdc
participant Remote as RemoteServer
participant Target as TargetApp
participant Storage as RemoteStorage
Client->>Daemon: request remote capture
Daemon->>Remote: connect / stage capture (step: upload capture)
Remote->>Target: inject & run (step: inject/transfer)
Target-->>Remote: capture produced / ident
Remote->>Storage: copy capture to remote (step: transfer)
Storage-->>Daemon: download capture (step: download capture)
Daemon->>Daemon: open local capture
alt failure at any step
Note right of Daemon: append step label and hint to error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rdc/daemon_server.py (1)
399-422:⚠️ Potential issue | 🟠 MajorClose partially opened capture handles on metadata/adapter failures.
After
cap = rd.OpenCaptureFile(), theOpenFilefailure path returns withoutcap.Shutdown(). The outer catch also skipscap.Shutdown()andremote.CloseCapture(controller)when failures happen afterremote.OpenCapturesucceeds, such as during adapter initialization.Suggested cleanup pattern
+ controller = None + cap = None + step = "open remote capture" result, controller = remote.OpenCapture( rd.RemoteServer.NoPreference, remote_path, remote_opts, @@ step = "open local metadata" cap = rd.OpenCaptureFile() open_result = cap.OpenFile(state.local_capture_path, "", None) if open_result != rd.ResultCode.Succeeded: + cap.Shutdown() _cleanup_temp_capture(state) remote.CloseCapture(controller) remote.ShutdownConnection() return f"local OpenFile (metadata) failed: {open_result}" @@ except Exception as exc: # noqa: BLE001 _stop_ping_thread(state) + if cap is not None: + try: + cap.Shutdown() + except Exception: # noqa: BLE001 + pass + if controller is not None: + try: + remote.CloseCapture(controller) + except Exception: # noqa: BLE001 + pass _cleanup_temp_capture(state) remote.ShutdownConnection() return f"remote replay setup failed at step '{step}' ({type(exc).__name__}): {exc}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rdc/daemon_server.py` around lines 399 - 422, The failure paths leave capture handles and remote captures open; ensure any partially opened OpenCaptureFile() is shutdown and remote.OpenCapture(controller) is closed on errors: after cap = rd.OpenCaptureFile() if cap.OpenFile(...) fails call cap.Shutdown() before returning; when remote.OpenCapture(controller) succeeds make sure the outer except handler (and all early returns) calls cap.Shutdown() (or state.cap.Shutdown()) and remote.CloseCapture(controller) in addition to _stop_ping_thread(state) and _cleanup_temp_capture(state) so no handle or remote capture is leaked; update references around OpenCaptureFile, cap.OpenFile, remote.OpenCapture, remote.CloseCapture, cap.Shutdown, controller, _init_adapter_state, and _start_ping_thread to ensure shutdown ordering is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rdc/daemon_server.py`:
- Around line 363-366: The except block that currently returns
f"CopyCaptureFromRemote failed: {exc}" should instead return the new
step-labeled failure string so remote download errors follow the same format as
uploads; update the except handler that cleans local_tmp and calls
remote.ShutdownConnection() to return a message like "failed at step
'remote-path staging': {exc}" (use str(exc) or repr(exc) for the error text) so
callers see the standardized "failed at step ..." wording for the
CopyCaptureFromRemote failure.
In `@src/rdc/remote_core.py`:
- Around line 161-166: The code uses a local-platform hint via
_inject_failure_hint() when handling remote injection errors (_inj_hint =
_inject_failure_hint()), which can give incorrect guidance for remote targets;
replace the call and any uses of _inj_hint in the CaptureResult error messages
with a remote-safe generic hint (e.g., a constant string or a new function like
_remote_inject_failure_hint()) that does not inspect sys.platform and instead
offers neutral guidance about remote privilege/LSM (AppArmor/SELinux) or
connection/permission checks; update the error messages returned from the
exec_result.result != 0 and exec_result.ident == 0 branches (where CaptureResult
is constructed) to include the new generic hint symbol instead of
_inject_failure_hint().
---
Outside diff comments:
In `@src/rdc/daemon_server.py`:
- Around line 399-422: The failure paths leave capture handles and remote
captures open; ensure any partially opened OpenCaptureFile() is shutdown and
remote.OpenCapture(controller) is closed on errors: after cap =
rd.OpenCaptureFile() if cap.OpenFile(...) fails call cap.Shutdown() before
returning; when remote.OpenCapture(controller) succeeds make sure the outer
except handler (and all early returns) calls cap.Shutdown() (or
state.cap.Shutdown()) and remote.CloseCapture(controller) in addition to
_stop_ping_thread(state) and _cleanup_temp_capture(state) so no handle or remote
capture is leaked; update references around OpenCaptureFile, cap.OpenFile,
remote.OpenCapture, remote.CloseCapture, cap.Shutdown, controller,
_init_adapter_state, and _start_ping_thread to ensure shutdown ordering is
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 694bc161-b405-4426-a8ac-57b4c68a5765
📒 Files selected for processing (11)
src/rdc/capture_core.pysrc/rdc/daemon_server.pysrc/rdc/handlers/capture.pysrc/rdc/remote_core.pysrc/rdc/services/session_service.pytests/unit/test_capture_core.pytests/unit/test_capture_handlers.pytests/unit/test_daemon_server_unit.pytests/unit/test_remote_core.pytests/unit/test_remote_replay.pytests/unit/test_session_service.py
BANANASJIM
added a commit
that referenced
this pull request
Apr 18, 2026
Three issues flagged on PR #223: 1. Remote inject hint used local sys.platform, giving wrong guidance when target OS differs from host. Add `_remote_inject_failure_hint()` (OS-neutral) for remote_capture; keep `_inject_failure_hint()` for local execute_and_capture. 2. Download-branch error message didn't match upload-sibling step-label format. Unified to `"at step 'download capture': {exc}"`. 3. `_load_remote_replay` outer except leaked `cap` and `controller` when late-stage steps failed. Initialize both to None at try-block top; cleanup in outer except with try/except pass guards; explicit cap.Shutdown() on open-local-metadata early-return path.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rdc/daemon_server.py (1)
341-436: Step labeling + guarded cleanup look solid; one optional consistency nit.The refactor correctly:
- Narrows
except Exception:around upload/download to(RuntimeError, OSError)with step-labeled returns (addresses the prior review on the download path — now matches the"at step 'download capture': {exc}"format).- Guards outer-except cleanup of
cap/controllerwithis not Noneplus inner try/except so a failure in one shutdown doesn't skip the others beforeShutdownConnection().- Nests the GPU probe so
tmp_cap.Shutdown()runs infinallyregardless ofOpenFile'sResultCode.Optional nit: non-exception result-code failures at Lines 396-399 (
remote OpenCapture failed: {result}) and Lines 404-409 (local OpenFile (metadata) failed: {open_result}) do not use the"remote replay setup failed at step '...'"prefix that the exception paths and outer catchall use. Callers parsing/grouping errors by the new step-label prefix won't see these. Consider normalizing for consistency:♻️ Optional consistency tweak
- if result != rd.ResultCode.Succeeded: - _cleanup_temp_capture(state) - remote.ShutdownConnection() - return f"remote OpenCapture failed: {result}" + if result != rd.ResultCode.Succeeded: + _cleanup_temp_capture(state) + remote.ShutdownConnection() + return ( + f"remote replay setup failed at step '{step}': " + f"remote OpenCapture failed: {result}" + ) @@ - if open_result != rd.ResultCode.Succeeded: + if open_result != rd.ResultCode.Succeeded: _cleanup_temp_capture(state) remote.CloseCapture(controller) cap.Shutdown() remote.ShutdownConnection() - return f"local OpenFile (metadata) failed: {open_result}" + return ( + f"remote replay setup failed at step '{step}': " + f"local OpenFile (metadata) failed: {open_result}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rdc/daemon_server.py` around lines 341 - 436, The non-exception result-code failure returns in the remote.OpenCapture branch (variable result after calling remote.OpenCapture) and the local metadata open branch (open_result after cap.OpenFile) don't use the standardized prefix; update those two return statements to use the same "remote replay setup failed at step '...': {result}" / "remote replay setup failed at step '...': {open_result}" format (matching steps "open remote capture" and "open local metadata") so error messages are consistent with the other failure paths; locate the checks after remote.OpenCapture(...) and after cap.OpenFile(...) and replace their return strings accordingly.
🤖 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/daemon_server.py`:
- Around line 341-436: The non-exception result-code failure returns in the
remote.OpenCapture branch (variable result after calling remote.OpenCapture) and
the local metadata open branch (open_result after cap.OpenFile) don't use the
standardized prefix; update those two return statements to use the same "remote
replay setup failed at step '...': {result}" / "remote replay setup failed at
step '...': {open_result}" format (matching steps "open remote capture" and
"open local metadata") so error messages are consistent with the other failure
paths; locate the checks after remote.OpenCapture(...) and after
cap.OpenFile(...) and replace their return strings accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 480ccbe5-80d9-429b-be81-fbc805825da9
📒 Files selected for processing (5)
src/rdc/capture_core.pysrc/rdc/daemon_server.pysrc/rdc/remote_core.pytests/unit/test_remote_core.pytests/unit/test_remote_replay.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rdc/remote_core.py
- tests/unit/test_remote_replay.py
…atch 1) Add `-- hint: ...` suffix to 5 common error throws so users see what to do next instead of having to grep docs: - remote_core.connect_remote_server: connection failed - capture_core.execute_and_capture: inject failed / zero ident - remote_core.remote_capture: same inject hints via shared helper - daemon_server: OpenCapture failed - session_service: daemon failed to start Platform branching extracted into `_inject_failure_hint()` helper in capture_core to avoid duplicating decision logic across files. Addresses issue #212 P3 top-5 hint injection.
…atch 2) Split two opaque `except Exception:` catches in `_load_remote_replay` and `_handle_remote_capture` into per-step try blocks that name which step failed, while preserving the outer `except Exception` as a safety net. Steps labeled in `_load_remote_replay`: stage capture, match gpu, open remote capture, open local metadata, init adapter state, start ping thread. Inner catches target concrete types: - `CopyCaptureToRemote` / `CopyCaptureFromRemote`: `(RuntimeError, OSError)` - GPU probe: `Exception` (optional, logged as warning) Also fixes a pre-existing `tmp_cap` leak when `OpenFile` returned a non-Succeeded result code (now wrapped in try/finally). Addresses issue #212 P4 opaque exception refactoring.
Add 20 tests (17 new + 3 extended) covering batch 1 and batch 2: - `_inject_failure_hint()` platform branches (darwin/win32/linux) - hint assertions on inject/connect/OpenCapture/daemon-start error paths - 6 step labels in `_load_remote_replay` (upload RuntimeError+OSError, GPU probe non-fatal, outer catchall with exception type) - `tmp_cap.Shutdown` called in 3 paths via try/finally - `_handle_remote_capture` inner vs outer catch distinction - `remote.ShutdownConnection()` accounting via finally Coverage: daemon_server.py 92% (350-352, 384-385, 418-422 now covered), capture_core.py 95%, remote_core.py 97%, handlers/capture.py new lines 188-190 covered. Total 92.68%.
Three issues flagged on PR #223: 1. Remote inject hint used local sys.platform, giving wrong guidance when target OS differs from host. Add `_remote_inject_failure_hint()` (OS-neutral) for remote_capture; keep `_inject_failure_hint()` for local execute_and_capture. 2. Download-branch error message didn't match upload-sibling step-label format. Unified to `"at step 'download capture': {exc}"`. 3. `_load_remote_replay` outer except leaked `cap` and `controller` when late-stage steps failed. Initialize both to None at try-block top; cleanup in outer except with try/except pass guards; explicit cap.Shutdown() on open-local-metadata early-return path.
BANANASJIM
force-pushed
the
phase9/pr3-error-hints
branch
from
d28cb60 to
8ab3f79
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes Phase 9 of the remote UX polish initiative — 6 of 7 pain points from #212 resolved (the 7th,
--start-via ssh, is explicitly deferred to Phase 9.1 per the plan).Batch 1 — hint injection (commit c03b1d4)
Add
-- hint: <suggestion>suffix to 5 common error throws so users immediately know what to do:remote_core.connect_remote_server: connection failed →verify 'rdc serve' is running on <url>capture_core.execute_and_capture+remote_core.remote_capture: inject failed / zero ident → platform-specific hint (SIP / Administrator / AppArmor-SELinux /--hook-children)daemon_server._load_replay: OpenCapture failed →try 'rdc open --proxy HOST:PORT'services/session_service: daemon failed to start (both open_session and listen_open_session) →run 'rdc doctor'Platform branching extracted into
_inject_failure_hint()helper incapture_core.pyto avoid cross-file duplication.Batch 2 — step labels (commit 89b7d5f)
Split two opaque
except Exception:catches into per-step try blocks that name which step failed, while preserving the outerexcept Exceptionas a safety net:_load_remote_replay: 6 labeled steps (stage capture / match gpu / open remote capture / open local metadata / init adapter state / start ping thread)_handle_remote_capture: inner(RuntimeError, OSError)→ step'inject/transfer'; outerexcept Exception→'unknown step ({TypeName})'tmp_cap.Shutdown()now runs infinallyso it's called even whenOpenFilereturns a non-SucceededResultCode.Batch 3 — test coverage (commit 20a0e8f)
20 tests (17 new + 3 extended) covering every new path:
_inject_failure_hint()3 platform branchesremote.ShutdownConnection()accountingCoverage:
daemon_server.py92% (350-352, 384-385, 418-422 now covered),capture_core.py95%,remote_core.py97%,handlers/capture.pynew lines 188-190 covered. Total 92.68%.Issue #212 coverage
rdc remote setup/status/disconnectrun_target_control_loop5s tick--keep-remote+ PR 3 proxy hint--start-via sshEnd-to-end verification of actual CLI output vs. issue text confirms no regressions and no exaggerated claims. All hints are reproduced verbatim from source; failing commands produce genuinely actionable guidance (e.g.
rdc remote setup localhost:1now printshint: rdc serve does not appear to be running on localhost:1. On the target, run: rdc serve --daemon --port 1).Test plan
pixi run e2e) all pass with 26/26 okrdc remote setup localhost:1/rdc remote --helpoutput--start-via sshFixes #212 (partial — remaining work tracked as Phase 9.1).
Summary by CodeRabbit
Bug Fixes
Tests