WIP: fix serial monitor ack ordering

[zackees]

Summary

WIP snapshot of the serial monitor investigation/fix.

• Preserve serial data frames that arrive before write_ack so JSON-RPC responses are not consumed by write() / write_async().
• Share serial port board-family inference with daemon open paths so ESP native USB CDC uses the correct DTR/RTS idle state when callers pass None.
• Make daemon serial writes all-or-error with write_all() instead of acknowledging partial writes.

Validation

• soldr cargo fmt --all completed before this WIP snapshot.
• soldr cargo check -p fbuild-serial -p fbuild-python -p fbuild-cli did not reach code validation because the zccache daemon failed during third-party dependency compilation.
• A follow-up soldr --no-cache cargo check ... run was interrupted before completion.

Status

Draft/WIP. Needs a clean compile/test pass and hardware AutoResearch validation before merge.

[coderabbitai]

Warning

Review limit reached

@zackees, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 57 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c1327f1-a79e-4f15-8656-9b7d4790845a

📥 Commits

Reviewing files that changed from the base of the PR and between 1a9e912 and e66d556.

📒 Files selected for processing (8)

• crates/fbuild-cli/src/cli/serial_probe.rs
• crates/fbuild-python/src/async_serial_monitor.rs
• crates/fbuild-python/src/json_rpc.rs
• crates/fbuild-python/src/lib.rs
• crates/fbuild-python/src/serial_monitor.rs
• crates/fbuild-serial/src/boards.rs
• crates/fbuild-serial/src/manager.rs
• crates/fbuild-serial/src/manager/tests.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-serial-monitor-ack-family

---

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.}

[zackees]

Test coverage added on top of the WIP fix

Pushed two regression-test commits covering the three behavioral changes in this PR. Both run locally against the bundled MSVC SDK once LIB is set to the standard Windows Kits\10 + MSVC\14.x\lib\x64 paths (rust-lld can't auto-discover them).

aea4d014 — test(json-rpc): regression guards for write_ack vs data ordering

• read_lines_async_drains_pending_before_wire — pure async unit test, proves the new pending queue is drained ahead of any wire access (uses an Arc<Mutex<None>> slot for the source).
• write_async_queues_data_arriving_before_ack — spins up an in-process tokio_tungstenite::accept_async server that sends a Data frame ahead of WriteAck. Asserts the data lines land in pending_lines and write_async still returns true once the ack arrives.

f2ed9df7 — test(serial-manager): regression guard for write_all partial-write loop

• PartialWriteSerialPort returns Ok(2) on the first write() call (the OS back-pressure case) and full length thereafter.
• Asserts write_to_port reports the full payload length (write_all contract) and every byte reaches the OS handle — prevents regressing to a write_ack with a partial bytes_written count.

Local validation

• fbuild-serial --lib: 119 passed, 0 failed (was 118).
• fbuild-python --lib: 35 passed, 0 failed (was 33).
• All other red CI checks on this PR (Formatting, LOC Gate, Render-and-diff, Documentation, ci_help_lists_pio_compat_flags) are pre-existing failures on main — none introduced by this branch.

The only remaining merge-blocker from the original PR body is the hardware AutoResearch validation.

[zackees]

Note for whoever picks up CI signal review on this PR:

The Check (ubuntu|macos|windows-latest) failures here are not introduced by this PR. They're caused by a pre-existing spurious failure in ci_help_lists_pio_compat_flags (crates/fbuild-cli/tests/ci_command.rs) that aborts cargo test --workspace before later crates run. The fbuild-python regression tests this PR adds therefore don't get a chance to execute in CI yet.

Filed #862 with the one-line fix (pipe stdout/stderr in run_cli_or_timeout). Once that lands on main and this branch is rebased, the workspace test run will reach fbuild-python and the new read_lines_async_drains_pending_before_wire / write_async_queues_data_arriving_before_ack / wait_for_remote_response_async_picks_up_pending_remote_line tests will actually exercise on the three platforms.

[zackees]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[zackees]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[zackees]

Post-merge simulation — workspace test is green when #862 is applied

Cherry-picked PR #862's two commits onto this branch locally and ran the affected crates with --no-fail-fast. All test groups pass, exit 0:

fbuild-cli  inline tests:          ok. 79 passed; 0 failed
fbuild-cli  tests/ci_command.rs:   ok.  3 passed; 0 failed   ← was 1 fail, fixed by #862
fbuild-cli  tests/lib_select.rs:   ok.  3 passed; 0 failed   ← was 2 fail, fixed by #862
fbuild-cli  tests/test_emu_*:      ok.  1 passed; 0 failed
fbuild-python --lib:               ok. 36 passed; 0 failed   ← +3 new tests from this PR
fbuild-serial --lib:               ok. 119 passed; 0 failed; 1 ignored   ← +1 new test from this PR
fbuild-serial doctests:            ok.  6 passed; 0 failed

So once #862 lands on main and this branch is rebased:

• Linux/macOS/Windows Check workflows will reach fbuild-python and exercise the three new json_rpc tests (read_lines_async_drains_pending_before_wire, write_async_queues_data_arriving_before_ack, wait_for_remote_response_async_picks_up_pending_remote_line).
• fbuild-serial will exercise write_to_port_completes_partial_writes_and_reports_full_length.

The remaining Check failure surfaced after #862 (fbuild-daemon::status_manager.rs:221 block_in_place panic in current-thread runtime tests) is filed as #865 — completely unrelated to this PR's serial monitor changes.

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

{}