iter-56: dogfooding session 41 fixes (exit codes, defaults, schema docs)#59
Merged
iter-56: dogfooding session 41 fixes (exit codes, defaults, schema docs)#59
Conversation
- A1: Map AppError variants to documented exit codes (3=connection, 124=timeout) - A2: Align responsive/click/wait/inspect/sources/page-text --help schemas with actual JSON output - B1: Bump daemon-drain timeout floor to 15s and reclassify as Timeout (exit 124) - B2: geometry/responsive default to visible-only; add --include-hidden to opt in - B3: eval --stringify no longer double-encodes string results - B4: Gate "debug:" stderr lines behind --verbose / RUST_LOG A3 (screenshot fallback on FF150) and C1 (navigate --wait-text noSuchActor) deferred to backlog — both need live recordings against Firefox 150 to capture exact failure shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR consolidates fixes from dogfooding session 41, introducing dedicated error types with documented exit codes (3 for connection, 124 for timeout), inverting visibility defaults for geometry/responsive to ChangesDogfooding Session 41 Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Bugfix iteration to make ff-rdp CLI behavior match its published contract (exit codes + --help JSON schemas) while improving defaults for real-world usage (timeouts, visible-only geometry/responsive, eval --stringify string handling, and quieter stderr via --verbose).
Changes:
- Implemented explicit exit code mapping (3 = connection failure, 124 = timeout) and added an e2e exit-code contract suite.
- Updated multiple subcommand
--helpJSON schemas to match actual output; introduced global--verboseto gate fallback/debug stderr. - Adjusted defaults/behavior: daemon network drain timeout floor, geometry/responsive visible-only-by-default with
--include-hidden, andeval --stringifyavoiding double-encoded strings.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/iterations/iteration-56-dogfood-41-fixes.md | New iteration writeup capturing tasks, rationale, acceptance criteria. |
| kb/dogfooding/dogfooding-session-41.md | New dogfooding session log driving the fixes and priorities. |
| kb/backlog/future-features.md | Backlog carryover + deferred items recorded for follow-up. |
| crates/ff-rdp-cli/tests/fixtures/eval_result_stringify_string.json | Fixture for eval --stringify string passthrough case. |
| crates/ff-rdp-cli/tests/fixtures/eval_result_stringify_number.json | Fixture for eval --stringify number-to-string case. |
| crates/ff-rdp-cli/tests/e2e/wait.rs | Updated wait-timeout test to assert exit code 124. |
| crates/ff-rdp-cli/tests/e2e/tabs.rs | Updated connection-refused test to assert exit code 3. |
| crates/ff-rdp-cli/tests/e2e/sources.rs | Added e2e tests asserting fallback stderr is gated behind --verbose. |
| crates/ff-rdp-cli/tests/e2e/main.rs | Registered new exit_codes e2e module. |
| crates/ff-rdp-cli/tests/e2e/geometry.rs | Updated tests for visible-only default + --include-hidden opt-in. |
| crates/ff-rdp-cli/tests/e2e/exit_codes.rs | New e2e suite asserting documented exit code contract (0/1/2/3/124). |
| crates/ff-rdp-cli/tests/e2e/eval.rs | Added e2e coverage for eval --stringify string/number/object behavior. |
| crates/ff-rdp-cli/src/main.rs | Added top-level exit handling for AppError::Connection (3) and AppError::Timeout (124). |
| crates/ff-rdp-cli/src/error.rs | Introduced new AppError variants and protocol-error classification into connection/timeout. |
| crates/ff-rdp-cli/src/dispatch.rs | Wired --include-hidden through dispatch to geometry/responsive commands. |
| crates/ff-rdp-cli/src/daemon/client.rs | Reclassified daemon connect failures as AppError::Connection. |
| crates/ff-rdp-cli/src/commands/tabs.rs | Reclassified connect failures into AppError::Connection for exit code 3. |
| crates/ff-rdp-cli/src/commands/sources.rs | Gated fallback debug stderr behind cli.is_verbose(). |
| crates/ff-rdp-cli/src/commands/responsive.rs | Added visible-only filtering control in JS + --include-hidden plumbing and tests. |
| crates/ff-rdp-cli/src/commands/network.rs | Raised daemon drain read-timeout floor and reclassified drain timeouts to exit 124. |
| crates/ff-rdp-cli/src/commands/js_helpers.rs | Converted poll timeout path to AppError::Timeout (exit 124). |
| crates/ff-rdp-cli/src/commands/geometry.rs | Default visible-only behavior; --include-hidden opt-in. |
| crates/ff-rdp-cli/src/commands/eval.rs | Updated --stringify page-side helper to avoid double-encoding string results. |
| crates/ff-rdp-cli/src/commands/console.rs | Gated debug stderr behind cli.is_verbose(). |
| crates/ff-rdp-cli/src/commands/connect_tab.rs | Reclassified connect failures into AppError::Connection. |
| crates/ff-rdp-cli/src/commands/a11y.rs | Gated debug stderr behind cli.is_verbose(). |
| crates/ff-rdp-cli/src/cli/args.rs | Added global --verbose, updated help schemas, and flipped geometry/responsive default flag semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+78
to
+80
| // BigInt/Symbol cannot be JSON.stringify'd either, so we fall back to | ||
| // String() coercion for those — consistent with the pre-existing circular | ||
| // reference fallback that returns a plain string. |
Comment on lines
+48
to
+57
| drain_result.map_err(|e| { | ||
| if let AppError::Internal(ref inner) = e { | ||
| let msg = format!("{inner:#}"); | ||
| if msg.contains("timed out") || msg.contains("WouldBlock") { | ||
| return AppError::Timeout(format!( | ||
| "network drain timed out — try --timeout {drain_timeout_ms}" | ||
| )); | ||
| } | ||
| } | ||
| e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ff-rdp-cli/tests/e2e/tabs.rs (1)
156-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docstring to reflect exit code 3.
The function documentation states "must exit 1" but the test now asserts exit code 3 (line 177). The comment should be updated to match the new exit-code contract for connection errors.
📝 Proposed fix
-/// Connecting to a port with nothing listening must exit 1 and print a +/// Connecting to a port with nothing listening must exit 3 and print a /// user-friendly message that mentions whether Firefox is running.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ff-rdp-cli/tests/e2e/tabs.rs` around lines 156 - 157, Update the test docstring to match the asserted exit code: change the sentence "must exit 1" to "must exit 3" in the comment above the test in crates/ff-rdp-cli/tests/e2e/tabs.rs so the documentation reflects the current expectation asserted by the test (the assertion that the process exits with code 3).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ff-rdp-cli/src/commands/js_helpers.rs`:
- Line 131: Update the stale rustdoc for poll_js_condition to reflect the new
timeout behavior: change the docs that claim a timeout returns AppError::Exit(1)
and is printed to stderr so they instead state that poll_js_condition returns
AppError::Timeout(timeout_context) on timeout (and note any change in printing
behavior if applicable). Locate the doc comment above the poll_js_condition
function in js_helpers.rs and replace the outdated description/examples to
reference AppError::Timeout and the timeout_context payload.
In `@crates/ff-rdp-cli/src/commands/network.rs`:
- Around line 49-53: Update drain_daemon_events to preserve typed timeout
errors: change its signature from returning anyhow::Result to Result<Vec<Value>,
ProtocolError> and stop wrapping transport.recv() errors with .context() so
ProtocolError::Timeout is propagated; this lets the existing From<ProtocolError>
-> AppError conversion (see error.rs) produce AppError::Timeout in network.rs
instead of doing string matching on the formatted anyhow error, and removes the
brittle checks for "timed out" or "WouldBlock" in the code handling
drain_daemon_events.
In `@crates/ff-rdp-cli/src/commands/responsive.rs`:
- Around line 138-140: The template substitution can collide if a selector
string contains "__VISIBLE_ONLY__"; to avoid this, swap the replacement order so
you replace "__VISIBLE_ONLY__" before "__SELECTORS__" (i.e., call
RESPONSIVE_JS_TEMPLATE.replace("__VISIBLE_ONLY__",
visible_only_str).replace("__SELECTORS__", &selectors_json)), or otherwise
perform the two substitutions with a method that avoids mutating serialized
selector JSON first; update the code that builds the final JS from
RESPONSIVE_JS_TEMPLATE, referencing RESPONSIVE_JS_TEMPLATE, selectors_json, and
visible_only_str.
In `@kb/backlog/future-features.md`:
- Line 73: The wikilink [[`#ultrareview`]] is dangling because there is no
corresponding "ultrareview" heading in the document; update the note to point to
an existing heading or create a matching heading (e.g., add a heading like "##
ultrareview" or "### ultrareview") so the anchor resolves, and ensure you use
Obsidian-compatible wikilinks (keep the double-bracket syntax) when editing the
reference or heading.
---
Outside diff comments:
In `@crates/ff-rdp-cli/tests/e2e/tabs.rs`:
- Around line 156-157: Update the test docstring to match the asserted exit
code: change the sentence "must exit 1" to "must exit 3" in the comment above
the test in crates/ff-rdp-cli/tests/e2e/tabs.rs so the documentation reflects
the current expectation asserted by the test (the assertion that the process
exits with code 3).
🪄 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: 529bea9c-83f6-4e86-a357-fea047804a71
📒 Files selected for processing (27)
crates/ff-rdp-cli/src/cli/args.rscrates/ff-rdp-cli/src/commands/a11y.rscrates/ff-rdp-cli/src/commands/connect_tab.rscrates/ff-rdp-cli/src/commands/console.rscrates/ff-rdp-cli/src/commands/eval.rscrates/ff-rdp-cli/src/commands/geometry.rscrates/ff-rdp-cli/src/commands/js_helpers.rscrates/ff-rdp-cli/src/commands/network.rscrates/ff-rdp-cli/src/commands/responsive.rscrates/ff-rdp-cli/src/commands/sources.rscrates/ff-rdp-cli/src/commands/tabs.rscrates/ff-rdp-cli/src/daemon/client.rscrates/ff-rdp-cli/src/dispatch.rscrates/ff-rdp-cli/src/error.rscrates/ff-rdp-cli/src/main.rscrates/ff-rdp-cli/tests/e2e/eval.rscrates/ff-rdp-cli/tests/e2e/exit_codes.rscrates/ff-rdp-cli/tests/e2e/geometry.rscrates/ff-rdp-cli/tests/e2e/main.rscrates/ff-rdp-cli/tests/e2e/sources.rscrates/ff-rdp-cli/tests/e2e/tabs.rscrates/ff-rdp-cli/tests/e2e/wait.rscrates/ff-rdp-cli/tests/fixtures/eval_result_stringify_number.jsoncrates/ff-rdp-cli/tests/fixtures/eval_result_stringify_string.jsonkb/backlog/future-features.mdkb/dogfooding/dogfooding-session-41.mdkb/iterations/iteration-56-dogfood-41-fixes.md
- Clarify --stringify helper comment to match actual TypeError handling
(only circular references get the sentinel; BigInt/Symbol rethrow).
- Reword poll_js_condition rustdoc: timeout now returns AppError::Timeout.
- Replace brittle substring matching in network drain timeout
classification with typed downcast through the anyhow chain
(ProtocolError::Timeout + io::ErrorKind::{WouldBlock,TimedOut}).
- Swap responsive JS template substitution order so a selector literal
containing the visible-only placeholder can't collide.
- Drop dangling wikilink anchor in backlog/future-features.
- Tick iteration-56 scope checkboxes for the work that landed; mark A3
and C1 explicitly deferred with reasons.
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
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
Bugfix iteration driven by dogfooding session 41. Two themes: trust the contract (exit codes, help text schemas match reality) and sensible defaults (timeouts, visible-only filtering, no double-encoded strings, quiet stderr).
AppError::Connection→ 3,AppError::Timeout→ 124. Reclassified daemon/connect/wait timeout sites. Newexit_codes.rse2e suite asserts 0/1/2/3/124 per failure class.responsive,click,wait,inspect,sources,page-text--helpJSON schemas to match actual output. Other iter-55 C2 commands had no drift.networknon-follow path; timeout reclassified to user-actionableTimeout(exit 124) with hint message.geometryandresponsiveskip hidden / zero-rect elements by default. Replaces--visible-onlywith--include-hidden. Behavior change — scripts relying on the old default need--include-hidden.eval --stringifystrings: Page-side helper skipsJSON.stringifywhen the result is already a string, sodocument.titlereturns"Example Domain"not"\"Example Domain\"". Objects/arrays still stringified.eprintln!("debug: …")insources,console,a11ybehind--verbose/RUST_LOG. Added--verboseglobal flag.navigate --wait-textnoSuchActor) both need live recordings against Firefox 150 to capture exact failure shape — moved to backlog.Test plan
cargo fmtcargo clippy --workspace --all-targets -- -D warningscargo test --workspace -qeval --stringifystring/number/object cases passsourcesfallback passgeometry/responsivedefault-changed tests updatedff-rdp networkagainst a heavy real-world page (~100 requests) succeeds with default timeout — verify post-merge against Comparis or similar.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--verboseflag to emit debug messages to stderr--include-hiddenflag forgeometryandresponsivecommands to include hidden/zero-sized elementsBug Fixes
--verboseis enabledExit Codes
3124