fix(build): seal lite-scons harness JSON channel against user-script stdout#946
Conversation
…stdout User extra_scripts routinely print() progress banners (NightDriverStrip's pio_audit.py / bake_site.py do), and some spawn subprocesses that write to the inherited stdout fd; both landed ahead of the harness's JSON payload and broke overlay parsing with 'expected value at line 1 column 1'. Duplicate the real stdout for the protocol at startup, point fd 1 at stderr for the whole run, and capture each script's Python-level stdout into notes (truncated) with a stderr echo for verbose logs. Validated against NightDriverStrip env:demo's four extra_scripts in the #942 Docker harness: clean JSON, unsupported=[], banners preserved in notes. Closes #945 Part of #942 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe harness script lite_scons_harness.py now captures stdout output from user extra_scripts into ledger.notes and stderr, and seals the JSON protocol by writing final output through a duplicated file descriptor rather than direct stdout. A new Rust test validates the fix against stdout noise from scripts and subprocesses. ChangesStdout protocol sealing fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Rust as script_runtime.rs
participant Harness as lite_scons_harness.py
participant Script as extra_script
participant Notes as ledger.notes
Rust->>Harness: invoke harness process
Harness->>Harness: duplicate stdout fd, redirect stdout to stderr
Harness->>Script: run_script_captured(env, ledger, path)
Script-->>Script: print() / subprocess writes to stdout
Script->>Notes: capture output appended to notes
Harness->>Rust: write json.dump via duplicated fd
Rust->>Rust: parse clean JSON response
Compact metadata
🐰 A script that prints, a banner unfurled, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/fbuild-build/src/script_runtime_tests.rs (1)
361-364: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert captured stdout is preserved in
overlay.notes.This verifies the second half of the contract: stdout noise should not corrupt JSON and should remain available in the result payload.
Test assertion to add
assert!(overlay .global_compile .common .contains(&"-DPRINT_NOISE_OK".to_string())); + assert!(overlay + .notes + .iter() + .any(|note| note.contains("loud import-time banner")));🤖 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/fbuild-build/src/script_runtime_tests.rs` around lines 361 - 364, The test currently only checks that `-DPRINT_NOISE_OK` is added to `overlay.global_compile.common`, but it does not verify that captured stdout is preserved in `overlay.notes`. Update the `script_runtime_tests` case to also assert that the result payload’s `overlay.notes` includes the stdout noise captured by the script runtime, using the existing `overlay` value in this test to validate the contract end-to-end.
🤖 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/fbuild-build/src/lite_scons_harness.py`:
- Around line 684-701: `run_script_captured` only records Python-level stdout
from `contextlib.redirect_stdout`, so fd-level writes from subprocesses and
`os.write(1, ...)` are lost from `ledger.notes`. Update `run_script_captured` to
temporarily capture fd 1 to a temp file while `run_script(env, path)` runs, then
merge that fd-level output with the existing `buf` contents and append the
combined text to `ledger.notes` (still echoing to `stderr` as needed). Use the
`run_script_captured` wrapper and the `ledger.notes.append(...)` path as the
main points to locate the fix.
---
Nitpick comments:
In `@crates/fbuild-build/src/script_runtime_tests.rs`:
- Around line 361-364: The test currently only checks that `-DPRINT_NOISE_OK` is
added to `overlay.global_compile.common`, but it does not verify that captured
stdout is preserved in `overlay.notes`. Update the `script_runtime_tests` case
to also assert that the result payload’s `overlay.notes` includes the stdout
noise captured by the script runtime, using the existing `overlay` value in this
test to validate the contract end-to-end.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46638990-e99e-4f8e-905b-bc58835399d4
📒 Files selected for processing (2)
crates/fbuild-build/src/lite_scons_harness.pycrates/fbuild-build/src/script_runtime_tests.rs
CodeRabbit review: subprocess/os.write output bypassed the Python-level redirect and reached only stderr, dropping it from the notes contract. Swap a temp file onto fd 1 per script via dup2 and fold both capture layers into the notes entry; test now asserts both the print() banner and the subprocess raw-fd noise are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
lite_scons_harness.pyemitted its JSON result on the same stdout that userextra_scriptsprint to, so any script banner (or a subprocess inheriting the raw fd) corrupted the protocol and failed the build withfailed to parse extra_scripts runtime output: expected value at line 1 column 1.Fix (harness-side, keeping the JSON boundary unambiguous):
contextlib.redirect_stdout; captured text is preserved innotes(script stdout (<name>): ..., truncated to 4 KiB) and echoed to stderr for verbose logs.Validation
test_resolve_extra_script_overlay_tolerates_user_stdout_noise— a script that prints at import time, spawns a subprocess that writes to the inherited stdout fd, then appends a CPPDEFINE; asserts the overlay resolves and carries the define. All 14script_runtimetests + 5lite_scons_acceptancetests pass (Linux container, soldr cargo).env:demo's four extra_scripts (install_intelhex,pio_audit,bake_site,merge_image) previously reproduced the failure; with the fix the harness returns clean JSON,unsupported: [], with the audit/bake banners captured innotes.Closes #945
Part of #942
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests