iter-61h: fix headless screenshot on Firefox 149+ via chrome-scope fallback#73
Merged
Merged
Conversation
…llback
Two-part fix for a regression deferred across dogfooding sessions 44–47:
1. Version detection — parse_app_version in actors/device.rs now also
reads the lowercase getDescription fields ("version", "platformversion")
that Firefox 150 introduced alongside the previous camelCase names. The
doctor warning "Firefox version not advertised" goes away and the
screenshot error message reports the real version instead of "unknown".
2. Chrome-scope screenshot fallback — when screenshotActor.capture fails
with Firefox 149+'s "global option is required in DevTools distinct
global" regression in capture-screenshot.js, fall back to:
listProcesses → parent process → getTarget → chrome-privileged
consoleActor → evaluateJSAsync that requires capture-screenshot in
chrome scope (where the ESM loader works), writes the PNG via nsIFile,
and is polled back to disk. New helpers: RootActor::list_processes /
ProcessInfo, TabActor::get_process_target, is_esm_global_option_error,
try_chrome_scope_screenshot.
Live-verified on headless Firefox 150 against example.com (32 KB) and
news.ycombinator.com (212 KB). cargo fmt / clippy -D warnings / cargo test
-q all green; no fixtures touched (chrome-scope path is exercised by live
runs, not the mock server).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the validation pass that confirmed iter-61g's three fixes (navigate blocking, network nav-scoped buffer, sources walker fallback) and surfaced the still-broken headless screenshot that this branch fixes. 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 (2)
📝 WalkthroughWalkthroughAdds root process enumeration and process-target parsing in core, extends device version parsing, implements a Firefox 149+ screenshot ESM-loader fallback in the CLI (chrome-scope eval + temp-file polling), updates version-handling flows to remember rather than warn, tweaks recorder locking, and adds tests and dogfooding docs. ChangesFirefox 149+ ESM Screenshot Fallback Implementation
Version handling and CLI UX changes
Docs, recorder tweak, and dogfooding reports
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
Fixes a Firefox 149+/150 headless screenshot regression by adding a chrome-scope loader fallback that bypasses the broken ChromeUtils.importESModule call in capture-screenshot.js, and updates parse_app_version to recognise Firefox 150's renamed (lowercase) version/platformversion fields.
Changes:
- New chrome-scope screenshot fallback path in
screenshot.rsthat walkslistProcesses→ parent process → chromeconsoleActor→evaluateJSAsync→ temp-file round-trip - New
RootActor::list_processesandTabActor::get_process_targethelpers (plusProcessInfore-export) to support the fallback - Version parser now also accepts Firefox 150's lowercase
version/platformversionfields, with corresponding unit tests - New dogfooding session 47 KB note
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ff-rdp-cli/src/commands/screenshot.rs | Adds try_chrome_scope_screenshot fallback, new is_esm_global_option_error detector, and unit tests |
| crates/ff-rdp-core/src/actors/root.rs | Adds ProcessInfo struct and list_processes method with a hand-rolled recv loop |
| crates/ff-rdp-core/src/actors/tab.rs | Adds get_process_target and parse_process_target_response to handle process-descriptor getTarget responses |
| crates/ff-rdp-core/src/actors/device.rs | Extends parse_app_version to try lowercase version/platformversion fields |
| crates/ff-rdp-core/src/lib.rs | Re-exports new ProcessInfo type |
| kb/dogfooding/dogfooding-session-47.md | New dogfooding session note documenting the verification pass that surfaced the screenshot fix needed here |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+402
to
+414
| // Step 3: build temp file paths. | ||
| let ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis(); | ||
| let tmp_png = std::env::temp_dir().join(format!("ff_rdp_chrome_cap_{ts}.png")); | ||
| let tmp_err = std::env::temp_dir().join(format!("ff_rdp_chrome_cap_{ts}.err")); | ||
|
|
||
| // Build a JSON-safe path string (forward slashes work on all platforms | ||
| // inside Firefox's nsIFile on macOS/Linux; on Windows the JS receives a | ||
| // backslash-escaped string from serde_json). | ||
| let out_path_js = serde_json::to_string(&tmp_png.to_string_lossy().as_ref()).unwrap(); | ||
| let err_path_js = serde_json::to_string(&tmp_err.to_string_lossy().as_ref()).unwrap(); |
Comment on lines
+494
to
+504
| if tmp_png.exists() | ||
| && let Ok(meta) = std::fs::metadata(&tmp_png) | ||
| && meta.len() > 8 | ||
| { | ||
| // Read the PNG bytes. | ||
| let png_bytes = std::fs::read(&tmp_png).map_err(|e| { | ||
| AppError::from(anyhow::anyhow!( | ||
| "screenshot: could not read chrome-scope temp PNG: {e}" | ||
| )) | ||
| })?; | ||
| let _ = std::fs::remove_file(&tmp_png); |
Comment on lines
+498
to
+510
| // Read the PNG bytes. | ||
| let png_bytes = std::fs::read(&tmp_png).map_err(|e| { | ||
| AppError::from(anyhow::anyhow!( | ||
| "screenshot: could not read chrome-scope temp PNG: {e}" | ||
| )) | ||
| })?; | ||
| let _ = std::fs::remove_file(&tmp_png); | ||
|
|
||
| // Encode as a data URL so the caller can use the same | ||
| // prefix-stripping path as the other code paths. | ||
| let b64 = base64::engine::general_purpose::STANDARD.encode(&png_bytes); | ||
| return Ok(format!("{PNG_DATA_URL_PREFIX}{b64}")); | ||
| } |
Comment on lines
+92
to
+94
| fn is_esm_global_option_error(err: &ProtocolError) -> bool { | ||
| err.to_string() | ||
| .contains("global option is required in DevTools distinct global") |
Comment on lines
+87
to
+121
| pub fn list_processes(transport: &mut RdpTransport) -> Result<Vec<ProcessInfo>, ProtocolError> { | ||
| let request = json!({"to": "root", "type": "listProcesses"}); | ||
| transport.send(&request)?; | ||
|
|
||
| let response = loop { | ||
| let msg = transport.recv()?; | ||
| let from = msg.get("from").and_then(Value::as_str).unwrap_or_default(); | ||
| if from == "root" { | ||
| if msg.get("type").is_some() { | ||
| continue; | ||
| } | ||
| if let Some(error) = msg.get("error").and_then(Value::as_str) { | ||
| return Err(ProtocolError::ActorError { | ||
| actor: "root".to_owned(), | ||
| kind: crate::error::ActorErrorKind::from_code(error), | ||
| error: error.to_owned(), | ||
| message: msg | ||
| .get("message") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("") | ||
| .to_owned(), | ||
| }); | ||
| } | ||
| break msg; | ||
| } | ||
| }; | ||
|
|
||
| let processes = response | ||
| .get("processes") | ||
| .and_then(Value::as_array) | ||
| .ok_or_else(|| { | ||
| ProtocolError::InvalidPacket( | ||
| "listProcesses response missing 'processes' field".into(), | ||
| ) | ||
| })?; |
Comment on lines
+484
to
+523
| let poll_deadline = Instant::now() + Duration::from_secs(10); | ||
| loop { | ||
| if tmp_err.exists() { | ||
| let msg = std::fs::read_to_string(&tmp_err).unwrap_or_default(); | ||
| let _ = std::fs::remove_file(&tmp_err); | ||
| return Err(AppError::User(format!( | ||
| "screenshot: chrome-scope capture failed: {msg}" | ||
| ))); | ||
| } | ||
| })?; | ||
|
|
||
| Ok(data) | ||
| if tmp_png.exists() | ||
| && let Ok(meta) = std::fs::metadata(&tmp_png) | ||
| && meta.len() > 8 | ||
| { | ||
| // Read the PNG bytes. | ||
| let png_bytes = std::fs::read(&tmp_png).map_err(|e| { | ||
| AppError::from(anyhow::anyhow!( | ||
| "screenshot: could not read chrome-scope temp PNG: {e}" | ||
| )) | ||
| })?; | ||
| let _ = std::fs::remove_file(&tmp_png); | ||
|
|
||
| // Encode as a data URL so the caller can use the same | ||
| // prefix-stripping path as the other code paths. | ||
| let b64 = base64::engine::general_purpose::STANDARD.encode(&png_bytes); | ||
| return Ok(format!("{PNG_DATA_URL_PREFIX}{b64}")); | ||
| } | ||
|
|
||
| if Instant::now() >= poll_deadline { | ||
| let _ = std::fs::remove_file(&tmp_png); | ||
| let _ = std::fs::remove_file(&tmp_err); | ||
| return Err(AppError::User( | ||
| "screenshot: chrome-scope capture timed out after 10s — \ | ||
| the Firefox async capture promise did not resolve in time" | ||
| .to_owned(), | ||
| )); | ||
| } | ||
|
|
||
| std::thread::sleep(Duration::from_millis(50)); | ||
| } |
Comment on lines
+505
to
+510
|
|
||
| // Encode as a data URL so the caller can use the same | ||
| // prefix-stripping path as the other code paths. | ||
| let b64 = base64::engine::general_purpose::STANDARD.encode(&png_bytes); | ||
| return Ok(format!("{PNG_DATA_URL_PREFIX}{b64}")); | ||
| } |
Comment on lines
+410
to
+421
| // Build a JSON-safe path string (forward slashes work on all platforms | ||
| // inside Firefox's nsIFile on macOS/Linux; on Windows the JS receives a | ||
| // backslash-escaped string from serde_json). | ||
| let out_path_js = serde_json::to_string(&tmp_png.to_string_lossy().as_ref()).unwrap(); | ||
| let err_path_js = serde_json::to_string(&tmp_err.to_string_lossy().as_ref()).unwrap(); | ||
|
|
||
| let js = format!( | ||
| r#"(function() {{ | ||
| var outPath = {out_path_js}; | ||
| var errPath = {err_path_js}; | ||
| var bcId = {browsing_context_id}; | ||
| var full = {full_page}; |
Comment on lines
+423
to
+438
| function writeBytes(path, arr) {{ | ||
| var f = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile); | ||
| f.initWithPath(path); | ||
| var s = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream); | ||
| s.init(f, 0x04|0x08|0x20, 0o644, 0); | ||
| var b = Cc["@mozilla.org/binaryoutputstream;1"].createInstance(Ci.nsIBinaryOutputStream); | ||
| b.setOutputStream(s); b.writeByteArray(arr); b.close(); s.close(); | ||
| }} | ||
| function writeText(path, text) {{ | ||
| var f = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile); | ||
| f.initWithPath(path); | ||
| var s = Cc["@mozilla.org/network/file-output-stream;1"].createInstance(Ci.nsIFileOutputStream); | ||
| s.init(f, 0x04|0x08|0x20, 0o644, 0); | ||
| var o = Cc["@mozilla.org/intl/converter-output-stream;1"].createInstance(Ci.nsIConverterOutputStream); | ||
| o.init(s, "UTF-8"); o.writeString(text); o.close(); s.close(); | ||
| }} |
Comment on lines
+474
to
+482
| // Check the synchronous return value for early errors. | ||
| if let Grip::Value(serde_json::Value::String(ref s)) = kick_result.result | ||
| && s.starts_with("err:") | ||
| { | ||
| return Err(AppError::User(format!( | ||
| "screenshot: chrome-scope capture failed: {s}" | ||
| ))); | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/screenshot.rs`:
- Around line 315-346: Add unit and integration tests exercising the
chrome-scope fallback branch: write tests that call the screenshot logic
(invoking ScreenshotActor::capture via the same entry path used in
screenshot.rs) and simulate the three error conditions by mocking or stubbing
the transport/actor to return (a) an error that makes
is_esm_global_option_error(...) true so try_chrome_scope_screenshot(...) is
invoked and returns success, (b) the specific sentinel `.err` case returned from
try_chrome_scope_screenshot(...) to ensure it propagates as an
Err(AppError::User(...)), and (c) a timeout/failure path to assert the final Err
branch message including version_mismatch_message()/is_actor_module_load_failure
handling; also add an e2e test that runs the screenshot subcommand end-to-end
(or an integration test that invokes the command runner) to validate the
chrome-scope fallback in a realistic environment. Ensure tests reference the
functions is_esm_global_option_error, is_actor_module_load_failure,
try_chrome_scope_screenshot, ScreenshotActor::capture, and
version_mismatch_message to locate the logic under test.
- Around line 403-408: The timestamp-based temp filenames (ts, tmp_png, tmp_err)
are collision-prone; change the creation of tmp_png and tmp_err to use a
collision-resistant suffix (e.g., append a UUID v4 or random hex) or use
tempfile::Builder/NamedTempFile to create unique temp files; ensure the new
filename still includes the ts for traceability but adds the UUID/random suffix
so concurrent runs cannot overwrite each other, and update any code that
reads/writes tmp_png/tmp_err accordingly (look for variables ts, tmp_png,
tmp_err in screenshot.rs).
- Around line 413-414: Replace the two panicking unwraps that create out_path_js
and err_path_js by returning errors with context: call
serde_json::to_string(...)? and propagate the Result using the function's
anyhow::Result return (or adjust the function signature to return anyhow::Result
if needed) and attach context via .with_context(|| "serializing screenshot path
to JSON" or similar). Specifically change the serde_json::to_string calls that
assign out_path_js and err_path_js to use the ? operator and anyhow::Context to
provide a helpful message referencing tmp_png and tmp_err.
In `@crates/ff-rdp-core/src/actors/root.rs`:
- Around line 87-136: Add unit tests for list_processes covering success,
missing "processes" field, missing "actor" entries, and actor error packets by
creating a test RdpTransport stub that returns a sequence of JSON messages via
recv() (and accepts send()); call ff_rdp_core::actors::root::list_processes with
that transport and assert: success returns Vec<ProcessInfo> with expected
ActorId and is_parent values, missing "processes" causes
ProtocolError::InvalidPacket, entries without "actor" are skipped (or cause
expected behavior), and an incoming error packet from actor "root" yields
ProtocolError::ActorError with the correct actor/kind/message; reference the
list_processes function, ProcessInfo, ActorId, ProtocolError, and RdpTransport
when locating code to test.
- Around line 123-133: The code currently uses filter_map over processes which
silently drops malformed entries; instead parse each entry explicitly and fail
fast: replace the filter_map block that builds ProcessInfo with a fallible
mapping that checks p.get("actor") and p.get("isParent"), returns a descriptive
error (including the offending entry) if actor is missing or not a string or if
isParent is not a bool, convert actor via ActorId::from and build ProcessInfo,
then collect into a Result<Vec<ProcessInfo>, _> (e.g. via
collect::<Result<_,_>>() or try_collect) and propagate or return that error from
the enclosing function so malformed process entries do not get ignored.
In `@crates/ff-rdp-core/src/actors/tab.rs`:
- Around line 172-238: Add unit tests for parse_process_target_response that
mirror the existing "frame" variant tests: create a well-formed JSON Value
containing a "process" object with "actor" and "consoleActor" (and optional
fields like "threadActor", "inspectorActor", "screenshotContentActor",
"accessibilityActor", "responsiveActor", "browsingContextID") and assert
Ok(TargetInfo) fields match; then add negative tests that pass Values missing
"process", missing "actor" inside "process", and missing "consoleActor" and
assert they return ProtocolError::InvalidPacket. Put tests alongside the current
frame tests in the same module (#[cfg(test)]) so they call
parse_process_target_response directly and cover both success and each
required-field failure case.
In `@kb/dogfooding/dogfooding-session-47.md`:
- Around line 99-106: Multiple fenced code blocks in the dogfooding session are
missing language identifiers and trigger MD040; update each omitted
triple-backtick block shown (the blocks containing the ff-rdp examples such as
the two navigate examples, the navigate with --wait-text/--wait-for examples,
the sources output, the computed "a" --prop error, and the network --since all
--jq example) by adding the language tag "text" after the opening ``` so they
become ```text; ensure every similar block at the other reported locations (the
blocks around the snippets at the indicated ranges) is annotated the same way.
🪄 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: 735d80d4-5adc-44c6-bc72-e2e827db3fda
📒 Files selected for processing (6)
crates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-core/src/actors/device.rscrates/ff-rdp-core/src/actors/root.rscrates/ff-rdp-core/src/actors/tab.rscrates/ff-rdp-core/src/lib.rskb/dogfooding/dogfooding-session-47.md
Comment on lines
+315
to
+346
| let capture_result = ScreenshotActor::capture( | ||
| ctx.transport_mut(), | ||
| &screenshot_actor, | ||
| browsing_ctx_id, | ||
| full_page, | ||
| &prep, | ||
| ) | ||
| .map_err(|e| { | ||
| if is_actor_module_load_failure(&e) { | ||
| // The root screenshot actor module failed to load on this build | ||
| // (the failure mode users hit on certain Firefox channels: the | ||
| // ESM import requires a `global` option that is not present in | ||
| // the DevTools distinct global). Skip the headless hint and | ||
| // surface a clean version-mismatch message. | ||
| AppError::User(format!("screenshot: {}", version_mismatch_message())) | ||
| } else { | ||
| ); | ||
|
|
||
| match capture_result { | ||
| Ok(data) => Ok(data), | ||
| Err(ref e) if is_esm_global_option_error(e) => { | ||
| // Firefox 149+ regression: `capture-screenshot.js` calls | ||
| // `ChromeUtils.importESModule` without the `global` option, | ||
| // which fails in the DevTools distinct global. Fall back to the | ||
| // chrome-scope loader workaround that loads the same module via | ||
| // the DevTools loader (which has the correct global). | ||
| try_chrome_scope_screenshot(ctx, browsing_ctx_id, full_page) | ||
| } | ||
| Err(ref e) if is_actor_module_load_failure(e) => { | ||
| // Generic actor-module-load failure not caused by the ESM global | ||
| // option issue — surface a clean version-mismatch message. | ||
| Err(AppError::User(format!( | ||
| "screenshot: {}", | ||
| version_mismatch_message() | ||
| ))) | ||
| } | ||
| Err(e) => Err(AppError::User(format!( | ||
| "screenshot: screenshotActor.capture failed ({e}) — \ | ||
| screenshots require headless mode; relaunch with: ff-rdp launch --headless" | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add command-level coverage for the chrome-scope fallback path.
The new fallback logic is substantial, but tests currently cover only the error-detector predicate. Please add tests that validate fallback branching and result handling (success, .err sentinel, timeout), and an e2e for screenshot if feasible.
As per coding guidelines, **/*.rs: Make Rust code unit testable; add tests if feasible and add e2e tests for all commands/subcommands.
Also applies to: 373-524, 725-766
🤖 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/src/commands/screenshot.rs` around lines 315 - 346, Add
unit and integration tests exercising the chrome-scope fallback branch: write
tests that call the screenshot logic (invoking ScreenshotActor::capture via the
same entry path used in screenshot.rs) and simulate the three error conditions
by mocking or stubbing the transport/actor to return (a) an error that makes
is_esm_global_option_error(...) true so try_chrome_scope_screenshot(...) is
invoked and returns success, (b) the specific sentinel `.err` case returned from
try_chrome_scope_screenshot(...) to ensure it propagates as an
Err(AppError::User(...)), and (c) a timeout/failure path to assert the final Err
branch message including version_mismatch_message()/is_actor_module_load_failure
handling; also add an e2e test that runs the screenshot subcommand end-to-end
(or an integration test that invokes the command runner) to validate the
chrome-scope fallback in a realistic environment. Ensure tests reference the
functions is_esm_global_option_error, is_actor_module_load_failure,
try_chrome_scope_screenshot, ScreenshotActor::capture, and
version_mismatch_message to locate the logic under test.
Comment on lines
+87
to
+136
| pub fn list_processes(transport: &mut RdpTransport) -> Result<Vec<ProcessInfo>, ProtocolError> { | ||
| let request = json!({"to": "root", "type": "listProcesses"}); | ||
| transport.send(&request)?; | ||
|
|
||
| let response = loop { | ||
| let msg = transport.recv()?; | ||
| let from = msg.get("from").and_then(Value::as_str).unwrap_or_default(); | ||
| if from == "root" { | ||
| if msg.get("type").is_some() { | ||
| continue; | ||
| } | ||
| if let Some(error) = msg.get("error").and_then(Value::as_str) { | ||
| return Err(ProtocolError::ActorError { | ||
| actor: "root".to_owned(), | ||
| kind: crate::error::ActorErrorKind::from_code(error), | ||
| error: error.to_owned(), | ||
| message: msg | ||
| .get("message") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("") | ||
| .to_owned(), | ||
| }); | ||
| } | ||
| break msg; | ||
| } | ||
| }; | ||
|
|
||
| let processes = response | ||
| .get("processes") | ||
| .and_then(Value::as_array) | ||
| .ok_or_else(|| { | ||
| ProtocolError::InvalidPacket( | ||
| "listProcesses response missing 'processes' field".into(), | ||
| ) | ||
| })?; | ||
|
|
||
| let result = processes | ||
| .iter() | ||
| .filter_map(|p| { | ||
| let actor_str = p.get("actor").and_then(Value::as_str)?; | ||
| let is_parent = p.get("isParent").and_then(Value::as_bool).unwrap_or(false); | ||
| Some(ProcessInfo { | ||
| actor: ActorId::from(actor_str), | ||
| is_parent, | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add unit tests for list_processes response parsing and error paths.
This new protocol surface should have direct tests for success, missing processes, missing actor, and actor error packets.
As per coding guidelines, **/*.rs: Make Rust code unit testable; add tests if feasible and add e2e tests for all commands/subcommands.
🤖 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-core/src/actors/root.rs` around lines 87 - 136, Add unit tests
for list_processes covering success, missing "processes" field, missing "actor"
entries, and actor error packets by creating a test RdpTransport stub that
returns a sequence of JSON messages via recv() (and accepts send()); call
ff_rdp_core::actors::root::list_processes with that transport and assert:
success returns Vec<ProcessInfo> with expected ActorId and is_parent values,
missing "processes" causes ProtocolError::InvalidPacket, entries without "actor"
are skipped (or cause expected behavior), and an incoming error packet from
actor "root" yields ProtocolError::ActorError with the correct
actor/kind/message; reference the list_processes function, ProcessInfo, ActorId,
ProtocolError, and RdpTransport when locating code to test.
…ests - screenshot.rs: replace .unwrap() on serde_json::to_string with .context()? - screenshot.rs: unique temp paths (ts+pid+rand), .part→final atomic rename, TempFileGuard Drop struct for cleanup on all exit paths - screenshot.rs: full_page and browsing_context_id round-tripped through serde_json::to_string for JS interpolation safety - screenshot.rs: all chrome-scope fallback errors now carry the '(Firefox 149+ chrome-scope fallback … also failed)' prefix - root.rs: list_processes refactored to use actor_request helper, removing hand-rolled recv loop and infinite-loop hazard - root.rs: add unit tests for list_processes (happy, missing field, filtered entry) - tab.rs: dedupe parse_target_response / parse_process_target_response via shared parse_target_response_inner(inner, wrapper_key) helper - tab.rs: add unit tests for parse_process_target_response (happy path, optional fields absent, missing process wrapper, missing consoleActor) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- recorder.rs: open with .read(true).append(true) so the file handle has GENERIC_READ access on Windows. `fs2::lock_exclusive` calls LockFileEx which fails with "Access is denied. (os error 5)" when the handle only has FILE_APPEND_DATA. This was causing 6 recorder tests to fail on windows-latest CI while passing on macos / ubuntu. - root.rs (list_processes): replace filter_map with a fail-fast collect so malformed process entries surface as ProtocolError::InvalidPacket instead of being silently dropped (CodeRabbit PR #73 feedback). Added tests: missing 'actor' fails, non-bool 'isParent' fails, missing 'isParent' defaults to false. - dogfooding-session-47.md: tag all fenced code blocks with `text` to satisfy MD040 (CodeRabbit PR #73 feedback). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- session 48 verifies iter-61h on Firefox 151 (works) and re-surfaces 4 issues from session 47 (3 still broken, 1 fixed). Also finds new bugs: --full-page screenshot produces viewport-only PNG; dom shape is polymorphic (object for 1, array for >1). - rust-developer agent memory documents the chrome-scope fallback pattern used in iter-61h for future tasks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
+433
to
+445
| // Step 3: build unique, unpredictable temp file paths. | ||
| // | ||
| // We combine a millisecond timestamp, the process ID, and a | ||
| // nanosecond-resolution entropy nibble to make the filename both unique | ||
| // across parallel invocations and non-predictable (mitigates symlink races | ||
| // on world-writable /tmp). | ||
| let ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis(); | ||
| let pid = std::process::id(); | ||
| let rand = u128::from(std::time::Instant::now().elapsed().subsec_nanos()) ^ ts; | ||
| let tmp_png = std::env::temp_dir().join(format!("ff_rdp_chrome_cap_{ts}_{pid}_{rand}.png")); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/ff-rdp-cli/src/script/recorder.rs (1)
183-192: ⚡ Quick winConfirm Windows
fs2locking fix (.read(true)+.append(true))
WindowsLockFileExrequires the underlying handle to have GENERIC_READ/GENERIC_WRITE access, and opening strictly with.append(true)can trigger “Access is denied”; opening with both.append(true).read(true)is the standard workaround forfs2/LockFileEx on Windows. Adding read permission should be harmless here since writes remain append-based and the exclusive lock still gates concurrent access. Optionally add a Windows-only test (or an integration check) if CI covers Windows to prevent regressions of this OS-specific requirement.🤖 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/src/script/recorder.rs` around lines 183 - 192, Keep the OpenOptions change that opens the recording output with both read and append access: ensure the call chain OpenOptions::new().read(true).append(true).open(output_path) remains in recorder.rs and retain the explanatory comment about Windows/LockFileEx; preserve the with_context(...) error wrapping for that open. Optionally add a Windows-only integration test (cfg(windows)) exercising the recorder open/lock path to prevent regressions, but do not remove or conditionalize the .read(true) change.
🤖 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/screenshot.rs`:
- Around line 444-448: The current temp-filename entropy uses
Instant::now().elapsed().subsec_nanos() to build `rand` which can collide;
replace this with a proper random/unique value (e.g., generate a u128 from the
rand crate or a UUID v4) and use that value in the `tmp_png`, `tmp_part`, and
`tmp_err` names; update the code that defines `rand` (the variable named `rand`
computed from Instant) to call a secure/random generator (for example use
rand::random::<u128>() or uuid::Uuid::new_v4().as_u128()) and add the
corresponding use/import so the three temp path joins for `tmp_png`, `tmp_part`,
and `tmp_err` use the new random/uuid value.
---
Nitpick comments:
In `@crates/ff-rdp-cli/src/script/recorder.rs`:
- Around line 183-192: Keep the OpenOptions change that opens the recording
output with both read and append access: ensure the call chain
OpenOptions::new().read(true).append(true).open(output_path) remains in
recorder.rs and retain the explanatory comment about Windows/LockFileEx;
preserve the with_context(...) error wrapping for that open. Optionally add a
Windows-only integration test (cfg(windows)) exercising the recorder open/lock
path to prevent regressions, but do not remove or conditionalize the .read(true)
change.
🪄 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: 892f24cf-48cd-4b7c-9807-24a48b686b80
📒 Files selected for processing (8)
crates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/script/recorder.rscrates/ff-rdp-core/src/actors/root.rscrates/ff-rdp-core/src/actors/tab.rskb/.claude/agent-memory/rust-developer/MEMORY.mdkb/.claude/agent-memory/rust-developer/project_screenshot_firefox150.mdkb/dogfooding/dogfooding-session-47.mdkb/dogfooding/dogfooding-session-48.md
✅ Files skipped from review due to trivial changes (3)
- kb/.claude/agent-memory/rust-developer/project_screenshot_firefox150.md
- kb/.claude/agent-memory/rust-developer/MEMORY.md
- kb/dogfooding/dogfooding-session-47.md
The "warning: connected to Firefox 151, but ff-rdp is tested against Firefox 120–150" string was printing to stderr on every CLI invocation, including each step of `ff-rdp run` — a 20-step script burned 20 identical warnings. Firefox publishes a new major version roughly every four weeks and the RDP surface rarely breaks across releases, so the warning was more noise than signal (dogfooding session 48 flagged this as the top friction item after the unfixed same-URL navigate timeout). - connection.rs: delete `warn_if_version_unsupported` method and the three orphaned unit tests for it. Keep `COMPATIBLE_FIREFOX_MIN/MAX` constants — `doctor` still uses them for its informational probe. - connect_tab.rs, tabs.rs: drop the call sites. - doctor.rs: soften the `firefox_version` probe — newer-than-tested versions now report Pass with detail "Firefox N (newer than tested range X–Y, but supported)" instead of Warn. Updated the corresponding unit test accordingly. Net effect: agents see exactly zero version-related stderr noise during normal operation; `ff-rdp doctor` still surfaces the version informationally for users who want to verify. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot and CodeRabbit both caught that `Instant::now().elapsed().subsec_nanos()` called immediately after `Instant::now()` returns ~0 nanoseconds — so the "entropy" component of the temp-file stem was effectively constant per process. Concurrent invocations sharing the same millisecond timestamp and pid (think: two screenshot calls fired by one agent in quick succession) would have produced colliding paths. Switch to `getrandom::getrandom(&mut [u8; 8])` for 64 bits of OS-quality random. Falls back to SystemTime nanos XOR pid-rotate if the OS RNG is unavailable (still varies per invocation, just not cryptographically strong). The filename stem is factored out so the .png / .png.part / .err trio all share the same random suffix — the JS still derives one path from another, so we can't independently randomise them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
309
to
316
| Some(v) => Probe { | ||
| name: "firefox_version", | ||
| status: Status::Warn, | ||
| status: Status::Pass, | ||
| detail: format!( | ||
| "Firefox {v} is outside the tested range {COMPATIBLE_FIREFOX_MIN}–{COMPATIBLE_FIREFOX_MAX}" | ||
| ), | ||
| hint: Some( | ||
| "some commands may misbehave on this version; report regressions at https://github.com/ractive/ff-rdp/issues".to_owned(), | ||
| "Firefox {v} (newer than tested range {COMPATIBLE_FIREFOX_MIN}–{COMPATIBLE_FIREFOX_MAX}, but supported)" | ||
| ), | ||
| hint: None, | ||
| }, |
Comment on lines
+435
to
+448
| // Combine a millisecond timestamp, the process ID, and 8 random bytes | ||
| // from the OS RNG to make the filename both unique across parallel | ||
| // invocations and non-predictable (mitigates symlink races on | ||
| // world-writable /tmp). Earlier revisions used | ||
| // `Instant::now().elapsed().subsec_nanos()` which is ~0 immediately after | ||
| // the Instant was constructed — review feedback (Copilot + CodeRabbit on | ||
| // PR #73) caught that. `getrandom` is already a workspace dep. | ||
| let ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap_or_default() | ||
| .as_millis(); | ||
| let pid = std::process::id(); | ||
| let rand: u64 = { | ||
| let mut buf = [0u8; 8]; |
Comment on lines
+486
to
+501
| .context("serializing browsing context id") | ||
| .map_err(|e| AppError::from(anyhow::anyhow!("{e}")))?; | ||
| let full_page_js = serde_json::to_string(&full_page) | ||
| .context("serializing full_page flag") | ||
| .map_err(|e| AppError::from(anyhow::anyhow!("{e}")))?; | ||
|
|
||
| // The JS writes to a `.part` file first, then renames it to the final path | ||
| // via `moveTo(null, outName)`. `moveTo` is atomic on the same filesystem, | ||
| // so the Rust poll loop never observes a partially-written PNG. | ||
| let js = format!( | ||
| r#"(function() {{ | ||
| var outPath = {out_path_js}; | ||
| var partPath = {part_path_js}; | ||
| var errPath = {err_path_js}; | ||
| var bcId = {bc_id_js}; | ||
| var full = {full_page_js}; |
| b.setOutputStream(s); b.writeByteArray(arr); b.close(); s.close(); | ||
| return f; | ||
| }} | ||
| function writeText(path, text) {{ |
Comment on lines
+584
to
+592
| && let Ok(meta) = std::fs::metadata(&tmp_png) | ||
| && meta.len() > 8 | ||
| { | ||
| // Read the PNG bytes before the guard drops and removes the file. | ||
| let png_bytes = std::fs::read(&tmp_png).map_err(|e| { | ||
| AppError::from(anyhow::anyhow!( | ||
| "screenshot: could not read chrome-scope temp PNG: {e}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01d96918-0203-4524-8453-ad485f2a0680
📒 Files selected for processing (5)
crates/ff-rdp-cli/src/commands/connect_tab.rscrates/ff-rdp-cli/src/commands/doctor.rscrates/ff-rdp-cli/src/commands/screenshot.rscrates/ff-rdp-cli/src/commands/tabs.rscrates/ff-rdp-core/src/connection.rs
💤 Files with no reviewable changes (2)
- crates/ff-rdp-cli/src/commands/connect_tab.rs
- crates/ff-rdp-cli/src/commands/tabs.rs
…ording) Four findings from Copilot + CodeRabbit on the latest review pass: - doctor.rs: split out-of-range version detail so older-than-tested-range Firefox builds say "older than" and newer ones say "newer than" (was always "newer than", misleading for older builds). - screenshot.rs writeBytes/writeText: change tmp file mode from 0o644 to 0o600. Screenshots may contain sensitive page content and error sentinels can include stack traces; in a multi-user system the world-readable default leaks them until cleanup. - screenshot.rs writeBytes: pass arr.length explicitly to nsIBinaryOutputStream.writeByteArray — older Firefox builds bind the XPIDL signature as (array, length); newer builds accept Uint8Array alone but tolerate an extra length arg. The two-arg form is portable. - try_chrome_scope_screenshot: poll deadline now derives from cli.timeout (floored at 10s to avoid early give-up). The previous hard-coded 10s could cut off slow --full-page captures on long pages, which already has a deferred bug in iter-61h. `ff-rdp screenshot --full-page --timeout 30000` now works as expected. Deferred (heavier lifts, will go in a follow-up iter): - Command-level chrome-scope fallback tests - PNG triple-encode round-trip refactor - Forwarding prepareCapture metadata (DPR/zoom/rect) to chrome-scope path - Async-promise temp-file leak when poll deadline fires - is_esm_global_option_error string-match fragility - Early-return synchronous "ok" assumption Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
screenshotregression deferred across dogfooding sessions 44–47. Root cause: Firefox 149+ introduced a DevTools distinct global that requiresChromeUtils.importESModuleto receive aglobaloption — butcapture-screenshot.jsdoesn't pass it, so the rootscreenshotActormodule fails to load on Firefox 150.listProcesses→ parent process →getTarget→ chrome-privilegedconsoleActor, evalsloader.require("…/capture-screenshot")in chrome scope (where the ESM loader works), writes the PNG viansIFile, and polls the temp file back. Live-verified on Firefox 150 Nightly against example.com (32 KB) and news.ycombinator.com (212 KB).actors/device.rs::parse_app_version: Firefox 150 renamedgetDescription'sappVersion/platformVersionfields to lowercaseversion/platformversion. With this fixdoctor'sfirefox_versioncheck now passes instead of warning "Firefox version not advertised", and the screenshot error message reports the real version when downgrade-fallback applies.Test plan
cargo fmt --checkcleancargo clippy --workspace --all-targets -- -D warningscleancargo test --workspace -q— 948 tests passff-rdp screenshot -o /tmp/x.pngon headless Firefox 150 against example.com → 1366×683 PNG, 32168 bytes, valid contentff-rdp doctornow reportsfirefox_version: 150(statuspass)--full-pageon Firefox 149+; the flag is forwarded but the chrome-scopecaptureScreenshotmay handle it differently from the two-step protocol🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation