fix(acp): interrupted frame writes close the sole writer; fixed native-steer refusal reasons

[loganj]

🤖

Summary

• A partly written message can no longer corrupt the stream between Buzz and an agent. Buzz runs each coding agent as a child process and sends it one JSON message per line over the agent's stdin pipe. Previously, if such a write was cut off mid-line — a concurrent control operation won the race, its deadline expired, the task was aborted, or the pipe failed partway — the partial line stayed in the pipe and the next message was written directly behind it, leaving a single corrupted line on the agent's input that no parser can read. Now the connection's single stdin writer is owned by the write itself: it is handed back only after the whole line, its newline, and a flush have all completed. An abandoned or failed write closes the pipe instead, so nothing can ever append to a partial line — later operations get a clean broken-pipe transport error, which the existing retry and retirement policy already handles.
  • Intentionally unchanged: cancelling a prompt whose message was fully written works exactly as before, and an ordinary application-level error reply from the agent (for example, a JSON-RPC "bad input" error) does not close the transport — the next prompt reuses the same connection.
• Refused mid-run steers now log why, in fixed words. While an agent task is running, Buzz can try to steer it — hand additional instructions to the running task — and falls back to the existing cancel-and-merge behavior when that is not possible. When a steer is refused before anything is written to the agent, the log now records one fixed reason instead of an opaque dump of the whole error: task_absent (no running task owns the scope), sender_absent (the task has no steer channel installed), mailbox_full (the one-slot steer mailbox already holds a steer), or mailbox_closed (the task's steer receiver is gone). Before-write refusals are now distinguishable from failures after a steer was accepted, which keep carrying the underlying transport error. Behavior is unchanged — same fallback, request ownership, and return values; only the log field changes.

Related issue

N/A

Testing

• 19 new tests. In crates/buzz-acp/src/acp_frame_writer.rs (8): interrupting the write at the body or the newline boundary, or hitting an I/O failure, must close the writer and make every later write fail; a completed write keeps the writer; a real OS-pipe test interrupts a 300 KB prompt mid-body and asserts that cleanup and later requests fail with no frame appended; another real-pipe test asserts a completed prompt still cancels normally; an application-error reply leaves the connection reusable. In crates/buzz-acp/src/pool.rs (7): each fixed refusal label is produced by send_steer for its real cause, an admitted steer reaches the read loop, the labels round-trip without drift, and post-admission failures never classify as admission refusals. In crates/buzz-acp/src/lib.rs (4): the real try_native_steer is driven through each refusal branch with a real pool, queue, and signed event, and the emitted fallback log is captured and pinned — the exact fixed reason, exact message and channel, no request or error content, and the unchanged fallback return; forcing the production reason to a wrong label or deleting it fails all four. A mutation check (removing the writer-ownership transfer) fails the frame tests.
• Full local just ci at 45e00ec82 — all lanes passed, including the buzz-acp 947 unit + 9 integration tests, desktop and mobile suites, fmt, clippy, web, security, and build lanes.
• Previous head 5276ef375 added only a one-line test fix. Windows Clippy flagged the wait_for test helper as dead code because its only callers are Unix-only tests; it now carries the matching #[cfg(unix)] guard. No production code or assertions changed. Re-verified locally at this head: cargo fmt --check, cargo clippy -p buzz-acp --all-targets -- -D warnings, and the 8 frame-writer tests pass. No full local rerun — the full matrix ran remotely at this head.
• Remote CI at 5276ef375 was green — run 34530106246: 42 jobs passed, 20 conditionally skipped, 0 failed, including the Windows Rust lane (the guard's purpose) and the desktop core lane; Docker image, desktop release-candidate, and DCO checks pass. The automated security review over 00209076c…5276ef375 reported no findings.
• Current head 2224ec228 adds the fallback-log regression tests. Rebased onto main at 44c1cc7df and extended with a test-only commit: the four lib.rs tests above now pin the production fallback log. Verified locally at this head: full cargo test -p buzz-acp (952 unit + 9 integration, 0 failed), cargo clippy -p buzz-acp --all-targets -- -D warnings, and cargo fmt --check, all clean; the DCO check passed remotely. Fresh remote CI is running on this head.

[github-actions]

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

• Exact PR diff: 44c1cc7df8e0067c9d578a93ad8e4393304422b3...2224ec228be0190838a801ba43dc57a35077277b
• Model: gpt-5.6-sol

💡 Click "edited" above to see earlier reviews for this PR.

---

Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions found. Interrupted ACP frame writes now close and poison stdin before cleanup can append to a partial frame, while existing pool handling retires the affected process and preserves required retry semantics. Steer-admission logs use fixed, non-sensitive reason labels without changing fallback behavior.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

• Review was limited to read-only inspection as required; repository builds, tests, scripts, and binaries were not executed.

---

_{Generated by Codex Security Review |
Requested by: @loganj |
Workflow run}

[loganj]

@buzz-security-review 45e00ec

[loganj]

@buzz-security-review 5276ef3

[jedwards27]

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: 00209076c7a10d9e4a475466c313e8ebecf041f5..5276ef37515f6f486f9f0083e40ace58ce3891b2 (exact head 5276ef37515f6f486f9f0083e40ace58ce3891b2)
Risk: high — ACP process transport integrity, cancellation/retry lifecycle, and operator-facing steer diagnostics.

Blocking test-contract defect

The PR promises that pre-admission native-steer refusals are logged with the exact fixed vocabulary task_absent, sender_absent, mailbox_full, or mailbox_closed, without request/error content. Production projects and emits that contract in crates/buzz-acp/src/lib.rs:4402-4418, but every new label assertion stops at send_steer / admission_reason in crates/buzz-acp/src/pool.rs:8942-9083.

A causal mutation changed only the production emission to reason = "wrong_label"; the full cargo test -p buzz-acp still passed 947 unit + 9 integration tests. The regression suite therefore cannot detect the advertised operator-visible behavior becoming false or disappearing while all new tests remain green.

Consequence: fallback delivery still works, but production diagnostics can silently misclassify the refusal, defeating the PR’s stated observability contract.

Author action: add a production-seam regression that invokes try_native_steer for each real refusal branch and captures the tracing event. Assert the exact emitted reason, unchanged fallback result, and absence of request/error content; mutation-prove that changing or removing the production field fails.
Verification owner: author for the regression; reviewer for causal mutation and full-package rerun on the new head.

Integrated behavior review

No additional defect was found. write_frame transfers the sole writer out of shared state before its first await and returns it only after body, newline, and flush complete (crates/buzz-acp/src/acp_frame_writer.rs:10-24). Cancellation, timeout, abort, and partial I/O therefore retire the pipe; later writes fail BrokenPipe, and existing pool logic requeues/retires transport failures while preserving connection reuse after application errors. Real-pipe tests cover partial-frame cleanup rejection, later-write rejection, completed-write cancellation, and application-error reuse. Steer admission/fallback behavior is unchanged, and no request content enters the new fixed-vocabulary log field.

Validation: clean exact-head cargo test -p buzz-acp passed 947 unit + 9 integration in the systems lane; cargo fmt --check, cargo clippy -p buzz-acp --all-targets -- -D warnings, and exact-head CI passed. Removing writer ownership transfer caused 5/8 frame-writer tests to fail. The production-log mutation above remained green and establishes the blocker.

Confidence gap: two additional clean local full-package reruns each hit a different pre-existing timing-sensitive test while the same package passed in the systems and mutation runs and exact-head CI is green. This is not attributed to the PR. Author action: none for those flakes. Verification owner: reviewer/tooling.

Manual/native evidence: not run; this is a headless ACP transport/logging change.
Residual risk: any new head invalidates this verdict and requires delta plus focused regression review.

[jedwards27]

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: 00209076c7a10d9e4a475466c313e8ebecf041f5..5276ef37515f6f486f9f0083e40ace58ce3891b2
Risk: high — ACP child-process transport cancellation/recovery plus operator-visible steer diagnostics.

The writer-ownership change is sound: write_frame removes the sole writer before the first await and returns it only after body, LF, and flush complete. Cancellation, timeout, abort, and I/O failure therefore close/retire the pipe; subsequent cleanup or requests fail with BrokenPipe instead of appending to a corrupt frame. Existing retry/respawn behavior remains intact, completed writes still permit normal cancellation, and application-level errors preserve transport reuse. The fixed steer labels also preserve admission and cancel+merge fallback behavior and do not add request content to the log.

Blocking finding — production log contract is not regression-protected

The PR promises that refused native steers emit exact fixed reason labels, but production projects and emits that field only in crates/buzz-acp/src/lib.rs:4402-4418, while the new assertions stop at send_steer / SteerError::admission_reason in crates/buzz-acp/src/pool.rs:8942-9083.

A causal mutation changing only the production emission to reason = "wrong_label" left the full cargo test -p buzz-acp suite green (947 unit + 9 integration). The tests therefore permit the advertised operator-visible distinction among task_absent, sender_absent, mailbox_full, and mailbox_closed to regress or disappear while all new coverage passes.

Author action: add a production-seam regression that invokes try_native_steer through the four real refusal branches and captures the tracing event, asserting the exact reason, unchanged fallback result, and absence of request/error content. Confirm changing or removing the production field makes the test fail.

Verification owner: author for the test; reviewer for causal mutation and full-package rerun.

Validation

• PASS: cargo test -p buzz-acp — 947 unit + 9 integration, exact head, clean tree.
• PASS: mutation replacing slot.take() with non-owning access failed 5/8 frame-writer tests, including interrupted body/LF, I/O failure, and real-pipe coverage.
• PASS: cargo fmt --check.
• PASS: cargo clippy -p buzz-acp --all-targets -- -D warnings.
• PASS: exact-head CI inventory — 42 successful, 20 conditional skips, 0 failures, including Windows Rust.
• PASS: mergeable, no unresolved review threads, and live GitHub head still matched the reviewed SHA before submission.
• EXPECTED RED: production-log mutation above was not detected by the suite; this establishes the blocking coverage gap.

Manual/native evidence: not run; this is a headless ACP transport/logging change, and real OS-pipe tests cover the relevant native boundary.

Residual risk: two separate clean local package reruns each hit a different timing-sensitive ACP test, while another full package run and exact-head CI passed. That does not establish PR causality and requires no separate author action beyond the blocking production-seam regression above.

[loganj]

Thanks @jedwards27 — both rounds (5180191753 / 5180193130) flag the same single blocker: the fixed reason vocabulary was asserted only through send_steer/admission_reason, so the production fallback log in try_native_steer's Err arm had no test that fails when it mutates. Fixed at head 2224ec228.

Production seam, not helpers. New #[cfg(test)] mod try_native_steer_fallback_log_tests in crates/buzz-acp/src/lib.rs (+278 lines, test-only; the production Err arm is byte-identical to the parent commit) invokes the real try_native_steer for each of the four admission refusals — real AgentPool with an in-flight task_map entry carrying steer_tx, real EventQueue with the signed kind:20001 event pre-pushed per the caller invariant, real steer-ack channel. Each test captures the actual tracing event (registry + Layer/Visit, the workspace capture pattern) and asserts: exactly one event, the exact message, the exact reason label (task_absent / sender_absent / mailbox_full / mailbox_closed), the exact channel, no fields beyond message/channel/reason, a SECRET-STEER-REQUEST-CONTENT sentinel carried in the real event content stays absent from the captured log, and the false return that keeps the cancel+merge fallback.

Mutation-proven. Forcing the production emission to reason = "wrong_label" fails all four new tests (your causal mutation — the old suite stays green under it, which is exactly the gap these close). Deleting the reason field from the production log also fails all four ("exactly one fallback log must be emitted per refusal"). Both mutations were restored byte-exact; final lib.rs is ce63e3bd… at both the working state and the commit.

Validation at 2224ec228: full cargo test -p buzz-acp — 952 unit (incl. the 4 new) + 9 integration, 0 failed; cargo clippy -p buzz-acp --all-targets -- -D warnings clean; cargo fmt --check clean. Fresh CI is running on the new head.

Could you re-review the new coverage on 2224ec228?

[loganj]

@buzz-security-review 2224ec2

[jedwards27]

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: 44c1cc7df8e0067c9d578a93ad8e4393304422b3..2224ec228be0190838a801ba43dc57a35077277b (exact head 2224ec228be0190838a801ba43dc57a35077277b)
Risk: high — cancellation-sensitive child-process frame integrity and operator-visible failure classification.

No author-actionable defect remains. The changed-head patch closes the prior regression-proof gap by driving the real production try_native_steer seam through task_absent, sender_absent, mailbox_full, and mailbox_closed, capturing the emitted INFO event, asserting exact fixed reason/message/channel fields, excluding request/error content, and preserving the false result that activates existing cancel-and-merge fallback (crates/buzz-acp/src/lib.rs:4391-4667).

Both assigned review lanes independently mutation-proved this contract: replacing the production reason with a wrong label made all 4/4 tests fail; removing the production reason field also made 4/4 fail. The mutations were restored before final validation.

The underlying transport remains fail-closed: the sole writer leaves shared state before the first await and returns only after body, newline, and flush complete (crates/buzz-acp/src/acp_frame_writer.rs:10-24). Cancellation, timeout, abort, or I/O failure drops stdin so later writes receive BrokenPipe; real-pipe cases cover interrupted writes, completed-write cancellation, and application-error reuse (acp_frame_writer.rs:109-232). Admission return shapes and fallback behavior remain unchanged, while arbitrary post-admission errors are excluded from refusal classification (crates/buzz-acp/src/pool.rs:631-655,1205-1231).

Author action: none.
Verification owner: CI/release gates; all required exact-head gates are now passing.

Validation at exact head: cargo test -p buzz-acp passed 952 library tests + 9 integration tests (1 ignored); cargo fmt --check, cargo clippy -p buzz-acp --all-targets -- -D warnings, and git diff --check passed. Exact-head required checks pass, including Rust unit/lint/Windows, security, DCO, Desktop E2E Relay, macOS build, and Desktop Release Candidate.

Manual/native evidence: not applicable to this headless ACP transport/tracing change.
Residual risk: unrelated Desktop lanes were still running during delegated review but required checks are green; no author action is attached. Any new head invalidates this approval.

[jedwards27]

Verdict: APPROVE

Reviewed: 44c1cc7df8e0067c9d578a93ad8e4393304422b3..2224ec228be0190838a801ba43dc57a35077277b (exact head 2224ec228be0190838a801ba43dc57a35077277b)

Risk: high — this changes ACP subprocess framing and interruption behavior, including sole-writer ownership, cancellation/timeout recovery, retry safety, and operational tracing at the native-steer/fallback seam.

Behavior/contracts traced:

• The sole stdin writer is removed from shared state before its first await and is returned only after body, LF, and flush complete. Cancellation, timeout, abort, or I/O failure therefore drops ChildStdin, and later cleanup/request writes fail rather than appending to a truncated frame (crates/buzz-acp/src/acp_frame_writer.rs:10-24,109-232). Completed writes and application errors retain the intended reuse behavior.
• Native-steer admission remains capacity-one and nonblocking. The four pre-write refusal states—task_absent, sender_absent, mailbox_full, and mailbox_closed—retain fallback behavior, while post-admission application/transport errors remain excluded from admission classification (crates/buzz-acp/src/pool.rs:631-655,1205-1231; crates/buzz-acp/src/lib.rs:4332-4387).
• The production INFO event emits only fixed classification plus fixed message/channel fields; it does not emit request or arbitrary error content.
• Unix shell fixtures are cfg-gated; deterministic duplex coverage remains cross-platform.

Findings: no blocking or non-blocking code/product/security finding.

The prior regression-proof blocker is resolved. Four tests invoke the real production try_native_steer, independently construct each real AgentPool::send_steer refusal state, capture the actual INFO event, assert the exact reason/message/channel and field set, reject sentinel request content, and assert false so universal cancel+merge fallback remains active (crates/buzz-acp/src/lib.rs:4391-4667). Independent mutations of the production classification and production reason field caused all four tests to fail behaviorally, then passed after restoration. This is causal production-seam protection, not helper-only compile coupling.

Author action: none.

Verification owner: CI/release gate for the unrelated still-running Desktop/relay integration jobs; no author-owned gap.

Validation at exact clean head 2224ec228be0190838a801ba43dc57a35077277b:

• cargo test -p buzz-acp — PASS: 952 library tests passed, 1 ignored; 9 integration tests passed.
• cargo fmt --check — PASS.
• cargo clippy -p buzz-acp --all-targets -- -D warnings — PASS.
• git diff --check 44c1cc7df8e0067c9d578a93ad8e4393304422b3...HEAD — PASS.
• Changed-head git range-diff — the four earlier commits remained patch-identical after the base update; the new patch adds the production-seam tracing tests without further production behavior changes.
• Exact-head CI at final review: Rust lint/unit/Windows and cross-compile, security, DCO, macOS build, release-candidate, and completed Desktop/relay lanes were green; no failed checks. A few broader Desktop/relay integration jobs remained in progress.
• Live PR head rechecked immediately before submission and remained exact; PR was mergeable.

Manual/native evidence: not run. This is a headless ACP subprocess transport/tracing change; real-pipe regression coverage and exact-head cross-platform CI exercise the affected boundary more directly than GUI interaction.

Residual risk: broader Desktop/relay integration jobs were still running at submission. They had no observed failure and are an external CI freshness gate, not an author-actionable defect. Any new head invalidates this approval.