Skip to content

fix(shell): stdin EOF for payload-less foreground and sync runs - #139

Open
asto18089 wants to merge 9 commits into
pinvou3-cleanfrom
fix/shell-stdin-eof
Open

asto18089 wants to merge 9 commits into
pinvou3-cleanfrom
fix/shell-stdin-eof

Conversation

@asto18089

@asto18089 asto18089 commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Slice 1 of the #126 split (full pre-split content preserved on archive/p126-pre-split-8705ccd85; slices 2/3 are the compound read-only probe lane and the test role contract — slice 2 is #126, rebased on this branch).

1. Foreground and sync shell commands never delivered stdin EOF (a $SHELL probe wedged 120 s)

Live evidence: the session's second bash case ran $SHELL, which expands to /bin/zsh. The command hung until the 120 s foreground watchdog killed it (transcript case "bash 陷阱用例 — FAIL").

Root cause: spawn_background_sandboxed (the lane behind both foreground and background tool commands) spawns with stdin(Stdio::piped()) and keeps the write end in BackgroundShell.stdin. execute_foreground_via_background closed the pipe only when a stdin payload was supplied; with none, the write end stayed open for the life of the process — any stdin-reading command (cat, a bare shell) blocked on a pipe that never delivers EOF, and the watchdog killed it at the 120 s default. The sync lane was worse: execute_sync_sandboxed set .stdin() only for payload runs, so std's default Stdio::inherit() handed the child the host TTY.

Fix: foreground without stdin data now closes the write end immediately after spawn (with data, the existing write-then-close order is unchanged and pinned); the sync lane uses Stdio::null() when there is no payload. The background lane keeps the open pipe — deferred action="interact" stdin streaming is a feature (test_write_stdin_streams_output, every_advertised_stdin_spelling_reaches_the_command stay green). The PTY foreground lane's contract is unchanged at spawn (pipe-only EOF close; the reader holds the master; the writer is a dup of the master fd on unix, so dropping the dup delivers nothing — round 14 rewired that construction, disclosed below). One unix behavior change this slice accepts and documents in the guidance: a foreground combined_output run with a stdin payload delivers the payload but no longer receives end-of-file afterwards (base delivered it via the old writer-box drop), so such a run ends at the foreground watchdog; interact close_stdin:true on a unix terminal task likewise no longer delivered end-of-file (Windows kept the ownership box) — resolved in the round-1 wave: an explicit close now writes the same \n + VEOF through the dup itself (base parity, pinned by interact_close_stdin_delivers_eof_to_a_terminal_command). The bash guidance text now states the foreground stdin contract so a bare $SHELL probe exits instead of being written again.

Tests: foreground_without_stdin_data_gives_stdin_readers_eof (red before: 3.0 s timeout kill; green after: exits on EOF), sync_lane_without_stdin_data_gets_eof_not_inherited_stdin (red: TimedOut against a held-open harness pipe; green: Completed).

Round-5 additions on the same stdin surface (re-anchored here):

  • the interact refusal states the accurate reason stdin is unavailable for each state (a pipe that was really closed vs. no writable stdin: PTY/combined_output, persistent, or finished tasks) instead of base's single generic stdin is not available for task N message;
  • a foreground command later detached to the background keeps running with its stdin at end-of-file, and interact on it says so and points at background=true;
  • the verifier's background gate jobs get end-of-file right after spawn (write_stdin(task_id, "", true)) instead of idling on the held-open pipe until the gate timeout.

The bash guidance sentence and the stdin schema (maxLength: 65536, inline-cap note) describe the foreground stdin contract; the model-facing tool description carries the same contract (shortened wording, scoped to what the lowercase surface exposes).

Merge order

Independent of every other open PR's hunks except the shared-file positions noted on #126's ledger; slice 2 (#126) is rebased on this branch and slice 3 on that, so merging this unblocks both.

Review wave (round 1, b374d4a)

From the 2026-11-04 9-lane fresh review: scrubbed the tests.rs blank-line sweep (166 pure-churn deletions; the file keeps its blank-line style and all new pins), replaced the dead unbounded-write armor arm with an exhaustive variant match (a future third unix variant now fails to compile instead of wedging), made the Windows/ohos owned-write path distinguish a worker panic from a deadline stall, applied the stall conjunct to close_stdin_pipe and dropped its unneeded pub, restored the healthy stdin handle on write/clone error paths, logged the verifier gate close failure instead of swallowing it, pinned the schema maxLength literals to the enforced 64 KiB byte cap, and fixed two copy defects (semicolon splice, stale comment). A second wave commit delivers the round-1 decision on the unix PTY close semantics (Option B: explicit close writes \n + VEOF through the dup; stalled writes still claim nothing; the spawn-path close stays pipe-only). Deliberately deferred: the same-family Stdio::inherit() in run_bounded_child_observed (tools/process.rs:118) — tracked in the review thread (issues are disabled on this repo).

Review wave (round 2, f28a861)

From the 2026-11-05 round-2 10-lane fresh review (comment 5781947688, pinned at 3eed997):

  • BLOCKER — the SIGPIPE guard only deferred the kill. A SIGPIPE generated while blocked stays PENDING on the worker thread, and the guard's mask restore delivered it before pthread_sigmask returned — with the process at SIG_DFL the host died at signal 13 one instruction after the "handled" EPIPE, in exactly the documented stall-recovery shape (reproduced standalone at exit 141 in both the immediate-EPIPE and blocked-mid-write recovery forms). The drop now checks the thread's pending set and consumes a pending SIGPIPE with sigwait before restoring. Std's SIG_IGN makes test binaries blind to this (discarded at generation, never pending), so the new sigpipe_guard_survives_a_broken_pipe_restore pin re-execs the test binary as a helper that re-arms SIG_DFL first: block-and-restore kills the helper at signal 13 (mutation-verified), consume-before-restore exits 0.
  • MAJOR — honest close results. A stalled close_stdin on a terminal task returned Ok while the schema promises a delivered end-of-file, and a second close silently dropped the interact surface (also Ok). Both stalled close paths now return Err citing the stall and keep the handle attached; the Delivered/Failed arms keep their dispositions with accurate comments. The stalled-terminal-close pin asserts the Err, and a new unit pin classifies bounded_stdin_eof_delivery as Delivered/Failed/Stalled against real pipes.
  • MAJOR — one bounded writer. The worker machinery existed three times (eof delivery, owned fallback, the inline unix clone block) and the SIGPIPE fix had to touch all three. The eof delivery and the unix clone payload write now ride the single bounded_stdin_write_owned helper (a returned clone is dropped, never reattached), the deadline-dependent refusal copy is shared in named functions, and the cfg_attr dead-code allows the duplication required are gone.
  • Honesty and copy: the EOF refusal names the raw-mode exception; the generic refusal names the lost-writer case; close_stdin's schema discloses raw mode and the Windows handle-only release; the inline cap docs drop the "atomic" claim and scope the pipe-buffer rationale to Linux (byte counting noted in the schema); the run-never-started context says the mentioned task id was never registered; the foreground close treats an evicted record as success instead of aborting a finished run; the PTY lane acquires the interact writer before the reader thread spawns (a dup/take_writer failure no longer orphans a reader thread); the VEOF hardcode is acknowledged against portable-pty's live c_cc[VEOF] injection.
  • Test hygiene: the three hang-guard scaffolds collapse into run_stdin_scenario_with_hang_guard (worker panics surface as failed verdicts); the stalled-close guard bounds with >=; the lowercase tool description's stdin sentence gets its own pin.

Deferred unchanged, disclosed: the sync-lane inline payload write stays SIGPIPE-guarded but deadline-less (no production caller can reach it with a payload today; same family as the tracked run_bounded_child_observed/process.rs follow-up); Windows-lane CI coverage for the owned-path helper test.

Review wave (round 3, 8a390de)

From the 2026-11-05 round-3 10-lane fresh review (pinned at f28a861c6; base 11cba403d == the pinvou3-clean tip, no rebase needed):

  • BLOCKER — Windows/ohos build break: the two unix-PTY-only close refusal helpers (stdin_stalled_close_refusal, stdin_stalled_close_not_applied_refusal) had no cfg while their only references sit inside the #[cfg(all(unix, not(target_env = "ohos")))] PTY-close arm — dead code on Windows and ohos, fatal under -D warnings. Gated with the arm's predicate, mirroring the PtyFile variant. (A libc::sigwait availability concern raised during review was refuted: unix/linux_like/mod.rs compiles linux_l4re_shared — which declares sigwait — into every target_os = "linux" target including ohos.)
  • False parity claim fixed: bounded_stdin_eof_delivery's doc claimed a terminal with VEOF disabled "received plain bytes from the box too" — portable-pty 0.9.0 writes nothing at all in that state (unix.rs guards if eot != 0). The comment now states the real divergence and the close_stdin schema discloses the disabled-VEOF state alongside raw mode.
  • Accuracy: the guard doc's sigpending parenthetical (it also reports process-directed signals); write_stdin's lead comment (the owned-path Timeout/Disconnected/spawn-failure arms deliberately leave the slot empty); the stdin_write_stalled field doc (a stalled close sets it too).
  • New pin: close_stdin_that_stalls_on_fresh_delivery_refuses_and_stays_attached — the fresh-stall close arm (Stalled → Err) had no guard (the not-applied pin only reaches the skip arm). The record-level pin drives write_stdin("", true) against a full pipe wrapped in PtyFile; mutation-verified red (Stalled → Ok revert).
  • Pin hardening: the SIGPIPE helper now asserts SIGPIPE is unblocked after the drop (mutation-verified red via restore-then-reblock) and writes a ran-canary the driver asserts (a renamed helper would leave the pin silently green via libtest's exit-0-on-unmatched-filter).

Deferred unchanged, disclosed: the sync-lane deadline-less payload write (no production caller with a payload); Windows-lane execution coverage for the owned caller branch (compile-checked only; a local windows/ohos cargo check cannot get past rquickjs-sys without the platform C toolchains, so the cfg proof here is by construction, same as the PtyFile gating).

Follow-ups tracked in the review comment: the owned-platform post-stall empty-close early Ok (base behavior, Windows/ohos only), the unpinned deadline value (by design), the unpinned leading \n of the ^D pair.

Review wave (round 4, 5e32eff)

From the 2026-11-06 round-4 10-lane fresh review (every lane re-derived from scratch; rebase verified as a no-op — merge-base == the pinvou3-clean tip 11cba403d):

  • MAJOR — false Testing claim, and a collapsible_if this branch adds. The previous Testing bullet claimed the upstream --all-targets clippy variant "reports only the pre-existing core/engine/turn_loop.rs:5502 collapsible_if (base code this branch does not touch)". Reproduced at base and head with the ci.yml lint flags: base reports TWO pre-existing sites (turn_loop.rs:5611, subagent/mod.rs:8058) and the reviewed head reported THREE — the third (verifier.rs:587) is this branch's own gate-close block, and the cited line was stale. ci.yml's comment declares collapsible_if no longer allowed ("the allowances were hiding exactly the class of problem that reached the gate"), so an upstream landing would have gone CI-red. Fixed: the gate close is now one let-chain; the upstream variant reports exactly the two base sites again.
  • Copy accuracy on this branch's own strings: the stdin schema and the run-cap comment said an oversized non-reading write "would block the executor" — since the bounded writer, such a write ends as the 5-second deadline-stall refusal; both now say so. The closed-stdin refusal and the close_stdin schema name disabled or remapped VEOF next to raw mode (a remapped VEOF consumes the literal EOF byte as ordinary input, same shape as raw mode).
  • Honest lane docs: the sync-lane payload write now documents that this manager-API lane has neither MAX_INLINE_STDIN_BYTES nor the bounded writer's deadline (no production caller reaches it with a payload; disclosed deferral).
  • Compile-enforced invariant: the manager-level close_stdin_pipe cannot fail, so it returns () — the always-Ok Result and the ? at the foreground call site are gone.
  • New pin: spawn_aborts_with_run_never_started_context_when_the_payload_stalls — the spawn-abort context copy ("the run never started … never registered") was the one behavior change with no red/green pin. The manager lane has no inline cap, so the pin drives a 2 MiB payload against a non-reading child, asserts the context copy, asserts the task-targeted recovery copy does NOT leak onto this path, and bounds the abort at >= STDIN_WRITE_DEADLINE. The tool surface cannot produce this stall on Linux (its 64 KiB cap equals the default pipe buffer), which is why the pin drives the manager API directly.

Deferred unchanged, disclosed: run_bounded_child_observed's Stdio::inherit() (process.rs follow-up); the sync-lane deadline-less payload write; Windows-lane execution coverage for the owned caller branch; the unix pipe-lane close after a stalled write still returning Ok (self-corrects via the stall flag); sigwait's return value unchecked (needs an externally timed handler-carrying signal); the three deadline pins without a local hang guard (red via nextest slow-timeout, or a suite wedge under plain cargo test); maxLength counts code points while enforcement counts UTF-8 bytes ("mirrors the same number" is deliberate).

Review wave (round 6, 7deeca8)

From the 2026-11-06 round-6 10-lane fresh review (every lane re-derived from scratch; rebase verified no-op — merge-base == the pinvou3-clean tip 11cba403d; the upstream all-targets clippy variant reproduced at base and head: the same two pre-existing collapsible_if sites, none introduced):

  • Two stale comments fixed (comment-only wave, 7deeca87d, tree ba63eb17c): the PTY spawn comment said "a stalled close releases it" about the dup'd master fd — no stall path releases it (both stall arms restore the handle into the record; the Failed delivery arm releases); and the BlockedSigpipeGuard SAFETY note claimed a failed pthread_sigmask leaves "the failure mode ... the pre-guard one" — the write-phase half holds, but the drop's restore would then install the zeroed previous mask (an empty SIG_SETMASK, leaving this thread's signals unblocked), which is not the pre-guard mode. Both comments now state the real consequences.
  • Honest boundary notes confirmed, no change needed: execute_sync_sandboxed has no production background=false caller at head (tests + the Windows PowerShell retry only), so the sync half of the fix is correct but latent today; the run_bounded_child_observed (tools/process.rs) Stdio::inherit() deferral remains the top tracked follow-up (the tasks.rs foreground-gate lane keeps the hang family this slice fixes for shell lanes).
  • Follow-up minors recorded in the review comment (none merge-blocking): the stalled-close refusal could disclose the late \n\x04 landing once the queue drains; the closed-stdin refusal's lead clause overclaims EOF in raw mode (its own parenthetical corrects it); the spawn-abort composed message pairs the inner "kill it and start a new one" with the outer "do not try to cancel or kill it" (the outer clause is authoritative and last); the Failed close arm conflates fd-clone/spawn failure with child-gone (EMFILE-level trigger); three new pins carry load-sensitive timing bounds (red-direction only; observed once on a loaded host, green isolated); the refusal-tail copy and the close_stdin schema disclosures are copy-verified but unpinned.
  • Mutation re-proof at this slice: 6 site mutations — 5 reddened exactly at their predicted pins with extras (M4 even fails to compile, proving the foreground close is load-bearing); the sixth is an equivalent mutant (the foreground close's stall conjunct is provably false pre-write), and the joint tail mutation reddens the contract.
  • A default-features cargo test -p codewhale-tui --lib compile break reported during review was refuted by a cold-target reproduction (175/0/1, exit 0).

Deferred unchanged, disclosed: the sync-lane deadline-less payload write; Windows-lane execution coverage; sigwait return unchecked; maxLength code-point vs byte counting; pipe-lane close after a stalled write returning Ok.

Testing

  • Targeted: tools::shell 175 passed / 0 failed / 1 ignored (pre-existing) and tools::verifier 9/0 green at the round-6 wave head (7deeca87d, tree ba63eb17c, byte-identical to the locally gated tree; the same counts were re-verified at 5e32effd before the wave, and re-run from a cold target dir with default features during review).
  • cargo fmt --all -- --check — clean.
  • Fork-CI clippy flags (--workspace --all-features --locked -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or -A clippy::collapsible_if -A clippy::assertions_on_constants) — clean on codewhale-tui. The upstream --all-targets variant (-D warnings, no collapsible_if allow, the ci.yml lint flags) reports only the two pre-existing base sites (core/engine/turn_loop.rs:5611, tools/subagent/mod.rs:8058 — both fail identically on base 11cba403d); this branch introduces none.
  • Every fix carries red/green pins; the stalled-tty pin waits for raw mode before writing (the round-18 CI failure's fix is included here).

Checklist

  • Docs/comments updated (bash guidance, schema description, tool description, verifier gate-spawn comment).
  • Tests added for every behavior change (mechanical changes carry red/green pins; the model-facing refusal-copy wordings are copy-verified in review, with the two primary refusal shapes pinned and the residual tails tracked as follow-up pins).
  • Verified TUI behavior manually if UI changes (no UI change).

No-Issue: slice 1 of the #126 split; the split ledger on #126 tracks the whole series

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review — 9-lane re-review at e13a366 (base pinvou3-clean @ f1eb10e == current main tip, so no rebase was needed; #126/#140 stack positions unaffected)

Direction: close to ready — the slice is real, root-caused, and the pipe/sync lanes are correctly fixed; 0 blockers, 2 MAJOR defect findings (one root), 2 MAJOR hygiene findings, and a required CI check that is currently red. All gates reproduced green locally: tools::shell:: 152 passed / 0 failed / 1 ignored, cargo fmt --all -- --check clean, fork-CI clippy flags (--workspace --all-features --locked -D warnings …) clean. The headline fix was mutation-verified: disabling the new close_stdin_pipe call turns foreground_without_stdin_data_gives_stdin_readers_eof red (details below).

What was verified true

  • Root cause and fix (claims C1–C5): TRUE. Base spawn_background_sandboxed piped stdin unconditionally and execute_foreground_via_background closed the write end only when a payload was supplied (base shell.rs:4363–4370); the sync lane inherited the host TTY on payload-less runs (base:2338–2340). HEAD closes the pipe for every foreground run (shell.rs:4686, before the fallible heavy-permit attach — strictly better than base's order, which could leak the write end on an attach error) and spawns the sync lane with Stdio::null() (2567–2576). The background lane keeps the open pipe for deferred interact streaming, pinned by the untouched test_write_stdin_streams_output.
  • Verifier gate jobs (C9): TRUE — write_stdin(task_id, "", true) at verifier.rs:587–593 delivers a real EOF (empty payload skips the write, the close block still drops the pipe); the pre-PR behavior idled on the open pipe until BACKGROUND_GATE_TIMEOUT_MS = 600 s. Other gate paths spawn via Command::output() (stdin closed by std) and were correctly left alone.
  • Bounded writes: TRUE for every production stdin-carrying path (foreground-via-background spawn, background spawn, interact): 5 s deadline + stall flag on all platforms; EPIPE/partial-write errors surface honestly; post-stall close cannot claim EOF (order + flag conjunct pinned).
  • Tests (C11): TRUE — all 12 named pins exist, are new in this diff, and assert positively (exact-output / exit-status / error-copy / deadline-fired, not absence-of-timeout). The stalled-tty pin synchronizes on a real RAW-READY output poll, not a sleep. No pre-existing test was modified or weakened (see M3 for the one exception-shaped churn). Count claim reproduced exactly: 152 passed / 0 failed / 1 ignored.
  • Mutation check: in a scratch tree I disabled manager.close_stdin_pipe(&task_id) (shell.rs:4686) — foreground_without_stdin_data_gives_stdin_readers_eof goes red (the probe wedges to the 3 s timeout, result.success false). The pin discriminates.
  • Smuggling: only blank-line churn (M3 below). Every functional hunk serves the declared slice: shell.rs 17/17 hunks in-scope, verifier.rs 3/3, guidance.rs 1 (a 6-sentence stdin-contract paragraph — borderline breadth, all clauses verified accurate against code, kept). guidance.rs and BashTool::description() share one source (shell.rs:5084), and the lowercase-bash variant's shortened sentence is accurately scoped to its schema.

MAJOR findings

M1 — unix PTY close_stdin: true regressed to a silent no-op that returns success (and Windows diverges)

shell.rs:1571–1585 (HEAD) vs base shell.rs:1363–1375. Base write_stdin(_, close=true) dropped the take_writer box; UnixMasterWriter::drop injects \n + VEOF into the master (verified against portable-pty 0.9.0 unix.rs:393–409 — the PR's own comment at shell.rs:2929–2936 records the probe: "cat on a pty exited 0 the instant the box was dropped"). HEAD's close only acts on StdinWriter::Pipe: for the dup'd PtyFile it drops the dup (delivers nothing — the reader holds the master), sets no EOF flag, and returns Ok(()). Consequences: interact{close_stdin:true} on a unix tty task gets a success receipt while the child waits forever, and the dropped dup destroys the interact surface (later interacts refuse generically) — the silent-false-success class this PR eliminates everywhere else. Windows keeps the take_writer box (2974–2982), so close_stdin:true still delivers EOF there: the platforms now disagree. The schema field text ("Close stdin after sending", shell.rs:5165–5168) promises neither behavior on PTY.

Two coherent resolutions (mutually exclusive — please pick one):

  • B (recommended): explicit close delivers EOF again on unix — write \n + VEOF through the dup on close=true for PtyFile, then drop and set stdin_closed_at_eof. Drop stays invisible; only an explicit close is child-visible; unix/Windows/base parity restored; schema text honored. The spawn-path close (close_stdin_pipe) stays pipe-only, so the documented "no end-of-file follows the payload" guidance contract is untouched.
  • A: refuse honestly — close_stdin:true on a terminal lane returns an explicit error ("a terminal lane's stdin has no end-of-file; kill the task instead") and keeps the dup. No child-visible change from today's HEAD, but a documented capability base had (ending input-driven tty work via interact) is dropped, and Windows still EOFs — a caveat would still be needed.

M2 — same root, second surface: foreground combined_output + stdin outcome changed Completed → TimedOut

Base delivered payload then closed stdin for combined_output foreground runs (base:4363–4370 → box drop → VEOF → Completed); HEAD leaves the terminal lane open, so a stdin-reading command now runs until the watchdog and the run returns TimedOut. The guidance documents the new contract honestly (guidance.rs:88–101: "delivers the payload but its terminal lane stays open, so no end-of-file follows it … waits until the watchdog"), but the schema still accepts the combination silently, no test pins the watchdog outcome, and the PR body's "PTY foreground lane's contract is unchanged" is contradicted. Resolution folded into the M1 decision: under B this surface stays TimedOut by design — add a pin for it (red/green against the base behavior) and fix the body claim; under A additionally consider refusing combined_output+stdin at the boundary like interactive+stdin.

M3 — tests.rs blank-line sweep: 166 of the 201 deletions are pure churn

git diff shows 166 blank-line-only deletions across ~153 hunks in tests.rs and zero non-blank deletions; base had a blank line between items and HEAD abuts them, with no rustfmt.toml sanction (rustfmt does not remove inter-item blank lines, and fmt is clean at both base and head). ~83% of the slice's deletions, ~100 extra hunks of blame/backport noise, and the file contradicts its own prior style. Fix: rebase-scrub the sweep and append the ~600 new test lines without touching existing ones.

M4 — dead "armor" arm hides a future silent wedge behind a tautology

shell.rs:1476–1520: inside #[cfg(all(unix, not(target_env = "ohos")))], matches!(slot, Some(Pipe) | Some(PtyFile)) is a tautology (those are the only two variants on that cfg) and the else arm — which contains an unbounded blocking write_all under the shell-manager lock, the exact wedge this PR removes — is unreachable on every target. If a third unix variant is ever added, this converts what should be a non-exhaustive-match compile error into a silent session wedger. Fix: let Some(writer) = slot.as_ref() else { unreachable!(...) } + an exhaustive two-arm match; the future variant then fails to compile instead of wedging.

MINOR findings

  1. Panic-vs-stall divergence between the two bounded-write implementations (shell.rs:1481–1520 unix inline worker vs bounded_stdin_write_owned 910–934, the Windows/ohos production path): the unix path maps Disconnected (worker panic) to "writer failed, no stall flag"; the owned helper conflates it with TimedOut, so a panicked Windows/ohos worker permanently poisons the task with the false "the command stopped consuming its stdin" reason. Unify on one helper returning RecvTimeoutError + a shared stall-message constructor (the 3-line refusal copy is currently pasted verbatim twice).
  2. Cap units: schema maxLength: 65536 counts code points; the enforced cap is 65,536 UTF-8 bytes (5334, 6242). A 65,536-codepoint CJK payload (~192 KiB) passes the schema and is then refused — self-healing with a clear error, but confusing. Either derive maxLength from the byte cap (e.g. 16,384) or state bytes in the description; either way add the missing pin binding the schema literal to MAX_INLINE_STDIN_BYTES (this file's culture has exactly such sync tests, e.g. the timeout-description pins).
  3. Error paths drop the stdin slot without restoring it (shell.rs:1492 fd-clone failure, 1501 write error; owned path 1566): a live task permanently loses its interact surface and later refusals cite "a finished command has released its stdin" — the wrong reason. Restore the healthy handle before returning (the 1492 case wrote nothing).
  4. Split invariant between the two close paths: BackgroundShell::close_stdin_pipe (1433–1438) sets stdin_closed_at_eof unconditionally, while the write_stdin close guards the identical assignment with && !stdin_write_stalled (1584). Latent today (the only call site closes before any write), but close_stdin_pipe is pub (3239) with no caller outside the file — make it private and add the stall conjunct.
  5. stdin_available lies differently per platform after a stall: unix restores the slot (1516) so snapshots report stdin_available: true while all writes are refused; Windows/ohos leave None → false. Refusal copy is correct on both; the surfaced boolean should agree.
  6. verifier.rs:592 .ok() swallow: unreachable failure today, but if write_stdin ever becomes fallible the gate silently reverts to idling 600 s — exactly the bug this PR fixes, invisibly. A tracing::warn! would pin it.
  7. Sync-lane spawn payload is still an unbounded blocking write_all (2607–2614), and STDIN_WRITE_DEADLINE's doc (4776–4782) implies spawn payloads are bounded too. Latent — no production caller wires stdin through the sync lane today — but align the path with the bounded helper or fix the doc.
  8. Copy: description splice "; This foreground surface…" (4903–4905) should be a period; stale comment 4657–4659 still names the removed write_stdin-wrapper mechanism; the generic refusal's "until its stdin is closed" (1606) is handle-accurate but child-ambiguous for PTY (the child never observes any close).
  9. Tests: the PtyFile arm of bounded_stdin_write_owned is untested on every platform and the Windows/ohos owned path never runs in CI; inline_stdin_payload_over_the_cap_refuses is fully #[cfg(unix)] though only the boundary leg needs it; fixed 150/300 ms sleeps before request_foreground_background (5114, 5173) can spuriously fail under load — a Running-status poll (as the stalled-tty pin does) is robust.

CI / body

  • Required link check is red: the body has neither a closing keyword nor a No-Issue: line (pr-issue-link.yml). Add e.g. No-Issue: slice 1 of the #126 split; the split ledger on #126 tracks the whole series.
  • Body corrections for the next push: (a) the claim "instead of claiming 'already at end-of-file' for every case" misdescribes the before-state — base's refusal was a generic stdin is not available for task N (base:1381) and never mentioned EOF; (b) scope the PTY-writer bullet to unix (Windows keeps the box); (c) "the model-facing description carries the same sentence" is same-contract, not same-words ("EOF" vs "end-of-file").

Follow-up (out of this PR's scope — please file)

run_bounded_child_observed/run_bounded_child (tools/process.rs:128–141) leave tokio's default Stdio::inherit() on stdin-less runs; all three production callers are model-driven script executors — tools/tasks.rs:501 (gate commands), core/engine/tool_catalog.rs:1784 (code_execution Python), tools/js_execution.rs:173 (Node) — so a script calling input() inherits the host TTY (raw-mode TUI) and races it for keystrokes until the budget kill. Same wedge family as the bug fixed here, timeout-bounded. One-line Stdio::null() in the None arm; separate PR to keep this slice clean.

Verdict

Substance: the PR does what it claims, root-caused, with the strongest pin set of the slices so far. Requested changes are M1/M2 (pick B or A), M3 (scrub the churn), M4 (delete the dead arm), the required-check body fix, and optionally the minors. With those, this is an easy approve.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed — 6337b0e (implements the round-1 review, review id 5768207421, pinned at e13a366):

  • M3 — tests.rs blank-line sweep scrubbed: 166 pure-churn deletions reverted; non-blank content byte-identical to e13a366 (asserted), base blank-line style kept, all new pins intact.
  • M4 — dead unbounded-write armor arm deleted; the variant match is now exhaustive on its cfg, so a future third unix variant fails to compile instead of silently wedging under the session lock. (Side effect handled: the removed arm was load-bearing for let mut slot on unix — now explicitly documented with the same cfg_attr predicate the owned helper uses.)
  • Minor 1 — bounded_stdin_write_owned returns RecvTimeoutError: the Windows/ohos path no longer diagnoses a worker panic as "the command stopped consuming its stdin"; both bounded-write paths now share the deadline/stall semantics.
  • Minor 3 — fd-clone failure and write-error paths restore the healthy handle (a write error no longer silently strips a live task's interact surface or misreports it as finished), on both the unix and owned paths.
  • Minor 4 — close_stdin_pipe applies the stall conjunct (a stalled pipe can't claim EOF) and is private.
  • Minor 6 — verifier gate close failure logs a tracing::warn! instead of .ok().
  • Minor 8 — description semicolon splice fixed; stale comment now names close_stdin_pipe.
  • Minor 2 — schema maxLength literals pinned to MAX_INLINE_STDIN_BYTES by a new test (bash_schema_stdin_max_length_matches_the_enforced_inline_cap); the code-point-vs-bytes mismatch itself is left as disclosed (changing the advertised limit is a product call).

Gates at 6337b0e: tools::shell:: 153 passed / 0 failed / 1 ignored (152 + the new schema pin), verifier gate-job pin green, cargo fmt --all -- --check clean, fork-CI clippy flags green in an isolated target dir.

Still open — needs a decision (mutually exclusive, from the review's M1/M2):
unix PTY close_stdin:true and the combined_output+stdin foreground run lost base's end-of-file delivery (the old writer-box drop injected \n+VEOF; the dup delivers nothing). Option B (recommended): explicit close writes \n+VEOF through the dup — restores unix/Windows/base parity and honors the schema text "Close stdin after sending", while drop stays invisible and the spawn-path contract ("no end-of-file follows the payload") is untouched. Option A: refuse close_stdin:true on terminal lanes with honest copy and keep the dup (drops a capability base had; Windows still EOFs). Say the word and I'll implement either.

Also proposed as a follow-up PR (not this slice): run_bounded_child_observed/run_bounded_child (tools/process.rs:128–141) leave Stdio::inherit() on stdin-less runs for the model-driven executors (tasks.rs:501 gates, tool_catalog.rs:1784 code_execution, js_execution.rs:173) — same wedge family, one-line Stdio::null() fix.

Body corrections (claim-premise, unix scoping, same-contract wording) and the No-Issue: line for the required link check are in the updated PR description.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-1 decisions implemented — wave 2: a2ec04f

  • M1/M2 resolved (Option B, per the author's call): an explicit interact close_stdin:true on a unix terminal task now writes \n + VEOF through the dup — the same bytes the old ownership box injected on drop — and records the EOF honestly (stdin_closed_at_eof = true, so post-close refusals cite end-of-file truthfully). Drop stays invisible; a stalled write still claims nothing; the spawn-path close stays pipe-only, so the documented "no end-of-file follows the payload" contract is untouched. Windows/ohos behavior unchanged. Pinned by interact_close_stdin_delivers_eof_to_a_terminal_command (mutation-verified red: disabling the VEOF write leaves the command running and the pin fails at its 6 s bound; with the fix it exits promptly).
  • Gates at a2ec04f: tools::shell:: 154 passed / 0 failed / 1 ignored, fmt clean, fork-CI clippy flags green.

Follow-up tracker (issues are disabled on this repo, so this thread carries it): run_bounded_child_observed/run_bounded_child (crates/tui/src/tools/process.rs:128-141) leave tokio's default Stdio::inherit() on stdin-less runs; all three production callers are model-driven executors — tools/tasks.rs:501 (gate commands), core/engine/tool_catalog.rs:1784 (code_execution Python), tools/js_execution.rs:173 (Node) — so a script calling input() inherits the host TTY and races it for keystrokes until the budget kill. Fix: cmd.stdin(Stdio::null()) in the None arm, same treatment the shell sync lane got in this PR. Separate PR.

That closes every review finding except the deliberately-deferred minors listed in the round-1 review (schema code-point units note, stdin_available stall-snapshot inconsistency, test robustness nits, Windows/ohos CI coverage for the owned path).

@asto18089
asto18089 force-pushed the fix/shell-stdin-eof branch from a2ec04f to 9148c17 Compare November 4, 2026 17:47
asto18089 added a commit that referenced this pull request Nov 5, 2026
In-range lock refresh closing all 4 open Dependabot alerts (4 high):

- undici 7.29.1 -> 7.30.1 via wrangler 4.150.0 -> 4.156.1 -> miniflare
  5.20261006.2-beta -> 5.20261101.0-beta in root, web/, and
  telemetry-ingest/: closes GHSA-xjj6-f33q-929c / CVE-2026-109869
  (alerts #139, #142, #145; fetch DoS via unhandled error on excessive
  content-encodings). This closes the follow-up PR #120 left open:
  wrangler 4.156.1 (latest stable) exact-pins miniflare
  5.20261101.0-beta (2026-11-01), which exact-pins undici 7.30.1, so a
  plain `npm update wrangler` per directory lands there. Collateral,
  all npm-canonical: workerd 1.20261006.1 -> 1.20261101.1 (+ its five
  platform binaries), tslib 2.8.1 -> 2.9.1 in root and
  telemetry-ingest, `libc` metadata added to the existing
  @img/sharp-* entries in root and telemetry-ingest, wrangler's
  @cloudflare/workers-types peer range following to ^5.20261101.1,
  web's nested miniflare ws 8.21.0 hoisting to the top level
  (content-identical), and the nested miniflare/node_modules/undici
  copies in web/ and telemetry-ingest/ moving to a single top-level
  7.30.1 entry (root's undici was already top-level). No other package
  versions change.
- @azure/msal-node 6.0.1 -> 7.1.2 in extensions/vscode/: closes
  GHSA-xcx6-jx8p-x3h5 / CVE-2026-109461 (alert #136; auth-code flow
  origin validation; first patched 7.1.0) via the native in-range
  path: @azure/identity 4.14.1 (2026-10-29) requires @azure/msal-node
  ^7.1.0 and satisfies @vscode/vsce's declared ^4.13.2, so
  `npm update @azure/identity` moves identity 4.14.0 -> 4.14.1 and
  msal-node -> 7.1.2 with no manifest change. msal-common follows
  upstream's native shape (top-level 16.14.2 for msal-browser 5.25.1,
  nested 16.15.0 under msal-node; both exact pins). msal-node 7.x
  keeps the confidential-client surface identity uses (jsonwebtoken
  ^9, node >= 20); the path is dev-only (vsce).

Verified:
- npm ci green in root, web/, telemetry-ingest/, extensions/vscode/
- telemetry-ingest: npm run typecheck, npm run check (wrangler 4.156.1
  deploy --dry-run), npm test (171 passed)
- web: npx wrangler --version -> 4.156.1, npm run cf-typegen, npm test
  (408 passed in 47 files)
- extensions/vscode: npm run check (tsc), vsce package
  --no-dependencies (valid 13-file vsix), and an identity -> msal-node
  7 live smoke: ClientSecretCredential.getToken() builds and parses a
  real AAD protocol response through msal-node 7.1.2
- npm audit: root and telemetry-ingest report 0 vulnerabilities; web
  reports 20 moderate advisories (glob via @opennextjs/cloudflare
  1.20.9, minimatch via the eslint toolchain) ingested after this
  branch's initial verification; all devDependencies, none open GitHub
  alerts, left for a separate decision. No HIGH findings remain in any
  of the four refreshed manifests: the base audits carried 1 HIGH for
  undici in root, web/, and telemetry-ingest/, and 2 HIGH for
  msal-node in extensions/vscode (the advisory this bump closes)

Out of scope, unchanged: npm audit reports follow-redirects (high) in
integrations/{feishu,wecom}-bridge and qs + minimatch (moderate) in
extensions/vscode; none of these are open GitHub Dependabot alerts, so
they are left for a separate decision.

Signed-off-by: asto <asto18089@126.com>
First slice of the #126 split: the foreground/sync stdin lifecycle.

- Foreground and sync shell commands without a stdin payload no longer
  hold the write end open for the process lifetime: foreground closes
  it right after spawn, the sync lane spawns with `Stdio::null()`, so
  a stdin-reading probe (`cat`, a bare `$SHELL`) gets end-of-file and
  exits instead of idling until the 120 s watchdog.
- A foreground command later detached to the background reports its
  closed stdin on `interact` instead of an opaque refusal.
- stdin writes are bounded everywhere (64 KiB inline cap, owned-writer
  fallback for Windows/ohos, 5 s deadline with a stall flag), a PTY
  writer no longer injects EOF on drop (dup'd master fd), and the
  verifier's background gate jobs are handed EOF at spawn instead of
  idling on the held-open pipe.
- bash guidance and the schema describe the foreground stdin contract;
  the model-facing description carries the same sentence.

Tests: 11 new pins (foreground EOF, sync-lane EOF, detached-foreground
interact receipt, PTY interact surface, stalled-tty bound, interact/
inline caps, bounded owned write) + the verifier gate-job EOF pin.
Targeted: `tools::shell::` 152 passed / 0 failed / 1 ignored
(pre-existing), `tests::verifier::` green; fmt clean; fork-CI clippy
flags clean; upstream all-targets clippy reports only the pre-existing
core/engine/turn_loop.rs:5499 collapsible_if (base code).

Split of #126; full content preserved on
archive/p126-pre-split-8705ccd85. Stacks: none (base f1eb10e).
Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Round-1 review wave on the stdin EOF slice (9-lane fresh review pinned
at e13a366):

- scrub the tests.rs blank-line sweep (166 deletions were pure churn;
  the file keeps its blank-line-between-items style and the new pins)
- drop the dead unbounded-write armor arm in `write_stdin`: the
  variant match is now exhaustive on its cfg, so a future third unix
  variant fails to compile instead of silently wedging under the
  session lock; the fd-clone failure and write-error paths restore the
  healthy handle instead of stripping a live task's interact surface
- `bounded_stdin_write_owned` now returns `RecvTimeoutError` so the
  Windows/ohos path distinguishes a worker panic (writer failure, no
  stall flag) from a real deadline stall, matching the unix path
- `close_stdin_pipe` applies the same stall conjunct as the
  `write_stdin` close (a stalled pipe must not claim EOF) and the
  manager wrapper drops its unneeded `pub`
- the verifier's gate-job stdin close logs a warning instead of
  swallowing errors, so a future failure cannot silently revert gates
  to idling on the open pipe
- copy: fix the semicolon splice in the lowercase bash description and
  the stale comment naming the removed write_stdin wrapper
- pin the schema maxLength literals to the enforced
  MAX_INLINE_STDIN_BYTES byte cap

Not in this wave: the unix PTY `close_stdin:true` semantics decision
(explicit VEOF delivery vs honest refusal) is awaiting a call, and the
sibling `run_bounded_child` inherit in tools/process.rs is follow-up
material. PTY foreground lane: `tools::shell::` 153/0/1 green; fmt
clean; fork-CI clippy flags green.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Round-1 review decision (Option B): base delivered end-of-file to a
terminal task through the ownership box's drop (`cat` on a pty exited 0
the instant it dropped); the dup'd-master rewrite made that drop
invisible, so interact close_stdin:true on a unix terminal task became
a silent no-op that returned success while the child waited forever
(and Windows still EOFs via its box — the platforms diverged). The
explicit close now writes the same \n + VEOF through the dup itself and
records the EOF honestly; a stalled write still claims nothing, and
the spawn-path close stays pipe-only so the documented "no end-of-file
follows the payload" contract is untouched.

Pinned by interact_close_stdin_delivers_eof_to_a_terminal_command.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Main's #123/#124 added `seed_running_shell_for_test` while this slice
was in flight; the rebased replay needs its `BackgroundShell` literal to
carry the two stdin state fields this slice introduced.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
@asto18089
asto18089 force-pushed the fix/shell-stdin-eof branch from 9148c17 to 8c6c00d Compare November 5, 2026 02:23

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh 10-lane re-review at 8c6c00dc0 (the rebase onto 11cba403d landed concurrently; the audited content tree is byte-identical to 667507fae, verified). Re-rebuilt from scratch per the fresh-review protocol — prior round's conclusions were not assumed.

State of this head: MergeQueue: CLEAN, gates reproduced green locally — tools::shell 276 passed / 0 failed / 1 ignored (pre-existing), tools::verifier 9 / 0, cargo fmt --all -- --check clean, fork-CI clippy flag set clean on codewhale-tui (isolated target dir). Stack-tip legs also green (tools::subagent 639/0, fleet 532/0).

What verified clean:

  • Claims 10/10 TRUE — root cause ($SHELL wedge → never-closed stdin pipe / TTY-inheriting sync lane), both fixes, the PTY dup contract (justified against portable-pty 0.9's UnixMasterWriter::drop), the accepted combined_output behavior change + guidance disclosure, the round-5 refusal states, all copy surfaces (internally consistent across guidance/schema/tool-description/refusals), all eight wave-1 items, and all three named behavior pins (completion-asserting, not timeout-asserting).
  • Smuggling CLEAN — all 25 hunks across the 4 files classify as stdin-EOF family; zero slice-2/slice-3 leakage; churn nil (tests.rs pure additions +699/−0; all 35 deletions reconcile line-for-line to replaced behavior; the ordered-reverted blank-line sweep is verifiably absent). One sibling spawn site exists tree-wide (tools/process.rs:134-137) and it is exactly the disclosed follow-up.
  • Rebase byte-faithful — diff 9148c1785 8c6c00dc0 == main's advance, zero drift in the PR's own files; the 9-commit chain is strictly linear; the armed-test seam integrations from main #131 are content-preserved.
  • Mutation pins — static analysis: all 14 new pins are STRONG (each reds when its fix is reverted, driving real tool/manager entry surfaces, no fake pins); the three key pins carry the prior cycle's dynamic mutation verification at identical content.

BLOCKER 1 — interact_close_stdin_delivers_eof_to_a_terminal_command (tests.rs:5920) has no platform gate. Its two load-bearing assertions exist only under all(unix, not(target_env = "ohos")): on Windows the close arm falls to _ => {} (ConPTY box close delivers no EOF and stdin_closed_at_eof is never set), so the test fails deterministically at both asserts; on ohos the tty spawn panics at its expect. The two sibling tty tests carry exactly #[cfg(all(unix, not(target_env = "ohos")))] (tests.rs:5405, 5481) — the omission is an oversight. fork-ci is ubuntu-only so the PR rides green today, but ci.yml declares Test (windows-latest) a required check for heavy PRs (ci.yml:541), and this change is heavy by its default-heavy *) arm — the upstream matrix and any Windows run are guaranteed red. Fix: one cfg line matching the siblings.

BLOCKER 2 — #[cfg(windows)] test killed_shell_does_not_wait_for_blocked_reader_threads (tests.rs:3669) no longer compiles on Windows. Its BackgroundShell { ... } literal (tests.rs:3673-3711) ends at last_lifecycle_bytes with no .. rest and omits the two new unconditional fields stdin_closed_at_eof (shell.rs:1142) and stdin_write_stalled (shell.rs:1148) → E0063 on every Windows test build. The same-blind-spot bug was fixed for seed_running_shell_for_test in the initializer commit, but this second, windows-gated literal was missed — byte-identical in the pre-rebase head, invisible to Linux CI. Fix: seed both fields false in the literal.

BLOCKER 3 — an abandoned bounded-write worker can kill the entire host process via SIGPIPE. Chain, all verified in source: the unix write worker does a plain blocking write_all on a dup of the pipe write-end (shell.rs:1567-1569, no SIGPIPE mitigation); on STDIN_WRITE_DEADLINE expiry the error returns while the worker stays alive blocked mid-write (shell.rs:1594-1600, the "abandoned to a background writer" state) and the error copy explicitly instructs "kill it and start a new one"; the kill path terminates the child process group, closing the pipe's read end; the blocked write(2) then wakes with SIGPIPE — and the production entrypoint deliberately restores SIGPIPE → SIG_DFL before any thread spawns (lib.rs:1731-1736, the codewhale doctor | head rationale). Result: following the PR's own documented recovery path after a model-drivable stall (feed a non-reading command twice, or any stall on a sub-64 KiB-pipe OS) terminates the whole host process, all sessions. The same detonation needs no user input at all: the stalled child merely exiting later kills the process. Test binaries keep std's SIG_IGN (workers get EPIPE there), which is why CI cannot see this. Base had a narrow variant of the exposure (child death during a lock-held blocking write), but this PR makes abandoned writers a routine post-error state and its copy orchestrates the trigger — so the fix belongs here: block SIGPIPE in the worker thread (pthread_sigmask(SIG_BLOCK) before the write, so the existing Ok(Err) arm handles EPIPE with the handle restored); apply the same to the ohos owned worker and consider the sync-lane payload write.

MAJOR 1 — interact close_stdin performs an unbounded inline write under the shell-manager lock (shell.rs:1679-1689). The \n\x04 delivery runs file.write_all directly, guarded only by !self.stdin_write_stalled — i.e. against a prior detected stall, not a currently-full tty input queue. Trigger: a tty task whose command stopped reading (sleep 60), an interact payload that fills the tty input queue (~4 KiB) and completes inside the 5 s deadline (no stall flag), then close_stdin: true — the 2-byte write blocks indefinitely while execute_interact holds context.shell_manager.lock(), wedging spawn/status/wait/kill for every shell in the session until the command exits. This contradicts the PR's own invariant ("Every platform bounds the write now", shell.rs:1521) — the close-path write is the one write that got no bound. Two independent lanes converged on this. Fix: route the close delivery through the same bounded worker (or a would-block probe that sets the stall flag and skips the EOF claim).

Minors (fix in this PR or disclose a decision):

  1. Windows/ohos close_stdin:true on a terminal task is a silent no-op returning Ok (_ => {} arm) — the exact behavior the wave-2 commit fixed for unix, left in place for Windows; the commit message's "Windows still EOFs via its box" is contradicted by the dependency (portable-pty 0.9 implements EOF-on-drop only in UnixMasterWriter; the ConPTY writer has no such drop). Per the repo's explicit-platform-interface rule: document the divergence in schema/copy or refuse explicitly.
  2. Stale docs invalidated mid-PR: the PtyFile variant doc ("PTY closes never claim EOF", shell.rs:881-882), the stdin_closed_at_eof field doc (shell.rs:1136-1141), the spawn-path comment (shell.rs:3153-3157), and the tests.rs:5075 "on Windows the write is deliberately blocking" justification — all now false after the bounded-write/close commits; refresh before a future reader "corrects" the code back.
  3. stdin_closed_at_eof = true is set even when the \n\x04 write failed or partially landed (both results let _ =-ignored, shell.rs:1685-1686), and in raw mode VEOF is ordinary input — the post-close refusal "stdin readers have seen end-of-file" can be false. Check the write result or soften the claim.
  4. Spawn-time payload stall kills the child and fails the whole run (shell.rs:3374-3379), contradicting the STDIN_WRITE_DEADLINE comment's abandon-and-continue description (shell.rs:8033-8040); also PTY lanes (~4 KiB queues) now fail tty:true/combined_output runs with large payloads that base delivered. Align comment and behavior.
  5. Caller-side thread::spawn panic under the manager lock permanently poisons the manager (shell.rs:1567, 938 — extends the pre-existing spawn-site pattern to a new hot path): std::thread::Builder::spawn + error mapping is cheap insurance against a permanent "lock poisoned" DoS.
  6. The stalled-write lock hold is n × 5 s aggregate under the global manager mutex (deliberate, documented) — worth one sentence in the docs/commit body so the aggregate is on record.
  7. Test-robustness: the stalled-write pin's 30 s hang guard vs its own ">30 s starvation observed" comment (bump to 60 s like its sibling); foreground_without_stdin_data_gives_stdin_readers_eof's 2900 ms elapsed assert under a 3000 ms watchdog is tight for cold pwsh on the Windows leg.

Disclosed deferrals re-confirmed accurate (not re-reported): schema maxLength code-point units, stdin_available stall-snapshot platform divergence, owned-path CI coverage (gap boundary verified — it is exactly the write_stdin owned block, though the panic-arm coverage hole is broader than disclosed), and the run_bounded_child_observed/Stdio::inherit follow-up (tools/process.rs:134-137, separate PR).

Stack note for the branch owner: this session independently rebased the full stack and hit the same seams — the armed-test insertion from #131 collides with the blank-line-style changes in both downstream legs (resolved by keeping the test and matching each leg's style), and the old #140 tip predates the BASHOPTS commit, so a naive diff against it shows the walls/helper/doc-comment as "added" — they are merge-order-correct, not drift. The live fix/fleet-test-role-contract head (00e650609) currently reports CONFLICTING against its base after the #126 branch moved; resolution will need the same seam handling.

Verdict: substance is approve-direction — the root cause is fixed completely and root-caused (10/10 claims true), the bounded-write design is the right shape (per-call cap, deadline, stall flags, per-state refusals), there is no smuggling, and every behavior change is pinned. But the required Windows leg is deterministically red or non-compiling (BLOCKERs 1-2) and the PR's own documented recovery path can kill the host process (BLOCKER 3), with one session-wedge MAJOR — do not merge until those four land. The cfg gate and field seeds are one-liners; the SIGPIPE block and bounded close-write are small, self-contained, and testable.

Fresh-review follow-ups on the stdin EOF slice (review 5779706794):

- Host kill: a bounded stdin write that hits the deadline abandons a
  thread blocked mid-write on the handle; the documented stall
  recovery (killing the task, or the stalled command exiting later)
  closed the read end, and the resumed write raised SIGPIPE on a
  process that runs with SIGPIPE at SIG_DFL (run_with_args) —
  killing the whole host process. Every stdin write worker (unix
  fd-clone, owned fallback, the new close delivery) now blocks
  SIGPIPE for the write (BlockedSigpipeGuard) so the abandoned write
  observes EPIPE and the existing write-error arm handles it; the
  sync-lane inline payload write gets the same guard. Not directly
  testable on unix CI (test binaries keep SIG_IGN), so this is a
  mechanism fix pinned by code path, not a red/green test.
- Session wedge: interact close_stdin on a unix terminal task
  delivered the ^D bytes inline under the shell-manager lock; two
  bytes into a tty whose input queue filled earlier (completed
  payload, no stall flag) block as forever as a large payload would.
  The delivery now runs on a bounded worker
  (bounded_stdin_eof_delivery): Delivered claims end-of-file, a
  failed write consumes the handle without claiming, and a deadline
  stall restores the handle under the stall flag.
- The unix ^D pin is now cfg-gated like its sibling tty pins — the
  close semantics it asserts exist only under
  all(unix, not(target_env = "ohos")); on Windows it was
  deterministically red and on ohos it panicked.
- The windows-only killed_shell test's BackgroundShell literal seeds
  the two stdin state fields it was missing (E0063 on windows test
  builds).
- Worker spawns use thread::Builder and map spawn failure to a write
  error instead of panicking through the shell-manager lock
  (permanent poison); the owned fallback documents that a spawn
  failure releases the writer with the failed closure.
- A spawn-time stdin write failure now says the run never started
  (no task id exists for the kill-and-restart copy), and the
  MAX_INLINE_STDIN_BYTES / STDIN_WRITE_DEADLINE comments state what
  the deadline actually does today (bounded waits, n x 5 s aggregate
  under the lock, spawn aborts).
- Copy: the eof refusal says "stdin was closed" (terminal closes are
  not pipes); guidance names tty in the no-terminal-stdin clause;
  the schema's close_stdin describes the newline + EOF delivery; the
  stale PTY-close comments (variant doc, field doc, spawn path, test
  gate justification) now match the delivered behavior; Windows
  close_stdin on a terminal task refuses explicitly instead of
  returning a silent Ok (ConPTY has no EOF to deliver).

New pin close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof
is red against the stall-conjunct-removal mutant (verified: the
mutant's close waits the full deadline and the pin reds at its
elapsed bound) and green here.

Targeted: tools::shell 169 passed / 0 failed / 1 ignored
(pre-existing), tools::verifier 9 / 0, tools::registry 48 / 0;
cargo fmt --all -- --check clean; fork-CI clippy flag set clean on
codewhale-tui in an isolated target dir. The windows-gated edits are
verified by construction only (field names copied from the
declarations; platform-neutral syntax) — this host has no windows C
cross toolchain for the dependency build scripts.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed — 3eed99777 (implements review 5779706794, which was pinned at 8c6c00dc0):

  • BLOCKER 3 (SIGPIPE host kill): every stdin write worker — the unix fd-clone worker, the owned fallback, and the new close delivery — blocks SIGPIPE for the write (BlockedSigpipeGuard); an abandoned post-deadline writer now observes EPIPE and flows into the existing write-error arm instead of killing the host process when the stalled task is killed or simply exits later. The sync-lane inline payload write gets the same guard. Pin limit disclosed: not red/green-testable on unix CI (test binaries keep std's SIG_IGN, so both mutant and fix pass there) — mechanism fix, pinned by code path and the comments at run_with_args / STDIN_WRITE_DEADLINE.
  • MAJOR (close wedge): the terminal close_stdin delivery now runs on a bounded worker (bounded_stdin_eof_delivery) instead of inline under the manager lock: Delivered claims end-of-file, Failed consumes the handle without claiming (also fixes the flag-on-failed-delivery minor), Stalled restores the handle under the stall flag. Pinned by close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof — mutation-verified red: removing the stall conjunct makes the close wait the full deadline on the full queue and the pin reds at its elapsed bound (observed 5.0003 s).
  • BLOCKER 1: interact_close_stdin_delivers_eof_to_a_terminal_command now carries #[cfg(all(unix, not(target_env = "ohos")))] like its sibling tty pins.
  • BLOCKER 2: the windows-only killed_shell_does_not_wait_for_blocked_reader_threads literal seeds both stdin state fields (E0063 on windows test builds fixed).
  • Minors: worker spawns use thread::Builder with error mapping (no poison-on-spawn on the new hot paths; the owned fallback documents that a spawn failure releases the writer); a spawn-time stdin write failure now says the run never started (the kill-and-restart copy has no task id there); the MAX_INLINE_STDIN_BYTES / STDIN_WRITE_DEADLINE comments state the real semantics (bounded waits, n × 5 s aggregate under the lock, spawn aborts); the eof refusal says "stdin was closed" (terminal closes are not pipes); guidance names tty in the no-terminal-stdin clause (pin updated); the schema's close_stdin discloses the newline + EOF delivery; the four stale PTY-close doc comments (variant, field, spawn path, test-gate justification) refreshed; Windows terminal close_stdin refuses explicitly instead of a silent Ok (ConPTY has no EOF to deliver).

Deliberately not taken: the sync-lane unbounded payload write (latent — no production caller reaches it with a payload; same family as the process.rs:134-137 follow-up, still tracked below), the stdin_available stall-snapshot platform divergence (intrinsic to the ownership design: the unix path can restore a dup, the owned path cannot), and the already-disclosed test-robustness nits. The deferred-minor list from the round stays tracked in this thread.

Gates at 3eed99777: tools::shell 169 passed / 0 failed / 1 ignored (pre-existing; 168 + the new pin), tools::verifier 9 / 0, tools::registry 48 / 0, cargo fmt --all -- --check clean, fork-CI clippy flag set clean on codewhale-tui in an isolated target dir. The windows-gated edits are verified by construction only (field names copied from the declarations; platform-neutral syntax) — this host has no windows C cross toolchain for the sqlite/ring build scripts, so the required Test (windows-latest) leg remains the authoritative check for them.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh re-review of 3eed997 (fix/shell-stdin-eof, base 11cba40 — which is still today's pinvou3-clean tip, so no rebase was needed). 10 independent review lanes over the whole diff, with the three load-bearing findings re-verified against the code and, for the blocker, reproduced empirically. Supersedes my earlier review comment (5779706794 @ 8c6c00d): the wave it produced fixed that round's blockers, but its own BlockedSigpipeGuard carries the new blocker below — found only because this pass re-derived everything from scratch.

Verdict: substance sound — every claim TRUE, root cause real and fully covered, smuggling CLEAN, no undisclosed regressions, tests genuinely mutant-sensitive — but 1 BLOCKER + 2 MAJORs before merge.


BLOCKER

B1 — BlockedSigpipeGuard does not prevent death by SIGPIPE; it moves the kill into its own Drop

shell.rs:918-966 (BlockedSigpipeGuard), sites at shell.rs:1007 (eof-delivery worker), shell.rs:1055 (owned worker), shell.rs:1695 (unix interact/spawn worker), shell.rs:3016 (sync-lane payload write).

A signal generated while blocked is not discarded — it becomes pending on that thread. write(2) to a broken pipe returns EPIPE and generates SIGPIPE; the guard has SIGPIPE blocked, so the write returns −1 (handled as an error, good), and then the guard's Drop restores the mask with SIG_SETMASK — POSIX delivers pending unblocked signals before pthread_sigmask returns. Since production runs with SIGPIPE at SIG_DFL (lib.rs:1731-1736, the comment's premise is correct), the process dies at signal 13 inside the Drop.

Reproduced outside the repo (standalone program mirroring the guard byte-for-byte, SIG_DFL armed first):

  • immediate EPIPE shape: write returned -1 → mask restore → exit 141; "SURVIVED" never printed.
  • stall-recovery shape (worker blocked mid-write on a full pipe — 65536 bytes, then reader closed, i.e. exactly "kill the task while a stalled write is parked"): write loop ended after 65536 bytes: r=-1 → exit 141.
  • proposed fix (sigpending + sigwait consuming the pending SIGPIPE before restore): survives, exit 0.

This falsifies the two comments that state the guard's purpose — shell.rs:922-926 ("the resumed write would otherwise kill the whole host process… instead of failing with EPIPE like every handled write error" — it still kills, one instruction later, inside the Drop) and shell.rs:5258-5265 ("the worker observes EPIPE instead of taking the process down"). It is also invisible to the suite for the reason the PR itself documents: test binaries run with std's SIG_IGN, at which masking is a no-op because ignored signals are discarded at generation.

Production paths that can hit it: an interact/spawn payload write that ends in EPIPE (child died mid-write, or the documented stall recovery — kill after a deadline stall) on the pipe lane, the sync-lane payload write, and the owned worker on ohos (unix-family target). PTY master writes are unaffected (EIO, no SIGPIPE), which is why nothing observed it.

Fix direction (validated in the repro): in the unix Drop, check sigpending() for SIGPIPE and consume it with sigwait() before SIG_SETMASK. Please pin the mechanism with a test that re-arms SIG_DFL in the test process itself (libc::signal(SIGPIPE, SIG_DFL) at test start overrides std's SIG_IGN) — that makes the guard testable and would have caught this.

MAJOR

M1 — A stalled close_stdin on a terminal task returns success while nothing was delivered and stdin is still open

shell.rs:1850-1854: the BoundedEofDelivery::Stalled arm sets the stall flag, restores the handle, and returns Ok(()). The schema (shell.rs:5651) promises unconditionally: "on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte". The same protocol's payload-timeout arm returns Err (shell.rs:1736-1743), and the Windows close returns Err — so the Ok here is inconsistent and dishonest: interact maps Ok to a plain success snapshot, and a model that does not write again never learns stdin is still open; the command runs to its watchdog instead of exiting on EOF. The arm's own comment ("mirroring the payload-timeout disposition") is inaccurate — the payload-timeout disposition is an Err. Worse, a second close then skips both guards (!stdin_write_stalled conjunct at 1826-1828; the tail Pipe-only match at 1875-1877) and silently drops the PTY handle — destroying the interact surface — also returning Ok.

Reachable without any earlier stall: a payload write that lands exactly as the tty input queue fills (write completes within the deadline, queue now full) followed by close_stdin: true.

Fix: return Err from the Stalled close arm (the stall refusal copy already exists), consider annotating the Failed arm, and pin with a test that closes a queue-full raw-mode tty whose payload writes all completed.

M2 — The bounded stdin worker machinery is hand-rolled three times in one PR

bounded_stdin_eof_delivery (shell.rs:985-1020), bounded_stdin_write_owned (shell.rs:1029-1069), and the ~90-line inline unix block in write_stdin (shell.rs:1652-1745) are the same shape three times: channel, named thread::Builder, guard, write_all+flush, recv_timeout dispatch, and near-identical refusal strings. The cost already materialized: the SIGPIPE fix in 3eed997 had to edit all three sites. The unix inline block is behaviorally bounded_stdin_write_owned applied to an fd clone — wrapping the clone as StdinWriter::Pipe(ChildStdin::from(dup_fd)) and calling the helper collapses it to clone-plus-one-match (the original slot stays in slot for restoration either way; Timeout keeps only the clone with the worker, which is what the current code does too). Please dedupe (or keep the helper + one dispatch site), and hoist the duplicated Timeout/Disconnected refusal strings into consts — the tests pin substrings, so copies can drift silently.

Minors / nits

  1. shell.rs:1023 — doc link [`ShellTask::write_stdin`] points at a type that exists nowhere (it's BackgroundShell); the only ShellTask occurrence in the repo is this link.
  2. shell.rs:1040 — "the allow only covers the non-unix side" states the opposite of the attribute below it: #[cfg_attr(all(unix, not(target_env = "ohos")), allow(dead_code))] covers the unix non-ohos side (where the fn is dead because unix uses the fd-clone path).
  3. shell.rs:1896-1900 — after a Delivered close on a raw-mode terminal, the refusal claims "stdin readers have seen end-of-file"; in raw mode the ^D is ordinary input and the master stays open (the reader holds it), so a raw-mode program never observes EOF. Byte behavior is base parity, but base never claimed EOF — qualify the copy.
  4. shell.rs:5651 — the close_stdin schema clause doesn't disclose the Windows split (shell.rs:1863-1870 releases the handle and returns Err) or the stall no-delivery case (M1).
  5. Schema maxLength: 65536 (shell.rs:5602/5607/5612) counts code points; enforcement counts UTF-8 bytes (shell.rs:5817, 6726) — a schema-legal 196 KB CJK payload is refused. Graceful refusal + drift pin make this acceptable; worth a wording note.
  6. shell.rs:5246-5255 — "fits a Linux pipe buffer in one atomic write": atomicity ends at PIPE_BUF (4096), and the rationale is Linux-only — on macOS (16 KiB default) a cap-legal payload to a non-reading child still burns the full 5 s deadline under the global lock and aborts the spawn. The deadline is what bounds it (the PR says so elsewhere); scope the const doc to Linux.
  7. Sync-lane inline payload write (shell.rs:3012-3021) remains a plain blocking write — no deadline. Latent only (no production caller reaches the sync lane with a payload today), but the const doc's "Every accepted payload is delivered through the bounded stdin writer" is true only of the tool surface. Follow-up candidate.
  8. After an owned-path writer loss (spawn failure/panic, Windows/ohos), later refusals fall to the generic cause list (shell.rs:1902-1905) whose three enumerated causes are all false for a running pipe task. A fourth clause would keep the chain honest.
  9. shell.rs:5168 — manager.close_stdin_pipe(&task_id)? in the (practically unreachable) eviction race aborts a finished foreground run with "Task not found"; treating not-found as success would match the pipe-leak invariant the comment argues.
  10. shell.rs:3540-3551 — the "run never started" context wraps an inner error that tells the model to "kill it and start a new one" for a task id that was never registered.
  11. shell.rs:3357-3376 — on dup failure the already-spawned PTY reader thread is orphaned (detached; base spawned the reader only after take_writer succeeded). Tiny; note only.
  12. shell.rs:1008 — b"\n\x04" hardcodes VEOF; portable-pty's Drop reads live c_cc[VEOF] and injects nothing when disabled. Parity holds for every realistic termios state; one word of acknowledgment in the comment.
  13. Tests: bounded_stdin_write_owned_returns_the_writer_and_times_out_clean is cfg(unix) while Windows CI is the only place that path runs in production — the in-code "covered by its own helper test" claim holds only on unix hosts. BoundedEofDelivery::Failed, the worker-panic Disconnected branches, and the Windows close-refusal copy are unpinned (a line in the PR body would do); close_stdin_on_a_stalled_terminal_task… uses > where >= matches the deadline exactly; the hang-guard (thread+channel) pattern is copy-pasted 3× with inconsistent tails — a small helper would do.
  14. Commit 1 (d6787cc) carried the tests.rs blank-line sweep and the splice bug, both repaired in b374d4a — net diff clean, noted for bisect archaeology only.

Clean bills (verified, not assumed)

  • Claims & completeness: all 13 claim groups TRUE against code+tests; root cause (write end held open / Stdio::inherit()) confirmed in base and fully covered across all 11 spawn paths inventoried (sync, foreground, background, persist, PTY/tty, interactive, read-only grammar, verifier background+sync gates, process.rs deferral, other tools untouched-and-out-of-claim). The deferred run_bounded_child_observed/tasks.rs inherit is genuinely the same family and honestly disclosed.
  • Smuggling: mechanical word-diff of both description strings shows exactly one inserted sentence (stdin) plus its splice fix; guidance clauses all stdin-related; verifier change is the gate close only; zero test deletions/modifications; whitespace pairing clean; all 5 commits on-message (net-diff-wise). CLEAN.
  • Regressions: every behavior delta (16 traced) is the disclosed fix, strictly-better hardening, or documented contract text — including the combined_output payload lane (base EOF → head watchdog, disclosed + pinned + parity via explicit close), Windows close silent-Ok → honest Err, and the removal of base's spurious \n+^D at teardown/persistent handoff. No golden/external text pins break.
  • Concurrency/perf: manager mutex is the single writer (all callers verified under lock); stall latch = one 5 s wait per task ever; abandoned workers bounded at one per task and self-clean on child death; fd count per terminal task unchanged vs base (portable-pty's box held a dup too); no unbounded memory/thread growth; happy path ~one thread spawn per tool call.
  • Tests: every load-bearing behavior has a dedicated mutant-sensitive pin whose RED path is driven by real backpressure or watchdog failure; the sync-lane re-exec helper deterministically reconstructs the never-EOF inherited stdin CI can't show; no process leaks on failure paths; all four struct-literal sites seeded.

Gates at 3eed997

  • cargo fmt --all -- --check — clean.
  • Fork-CI clippy (--workspace --all-features --locked -D warnings -A uninlined_format_args -A too_many_arguments -A unnecessary_map_or -A collapsible_if -A assertions_on_constants, isolated target after cargo clean -p codewhale-tui so the lint genuinely re-ran) — clean.
  • tools::shell:: — 163 passed / 0 failed / 1 ignored (pre-existing ignore). One parallel-load flake on the first run: the pre-existing background_tty_command_has_controlling_terminal lost a stdout-capture race under full-suite load and passes in isolation (0.11 s) and on the rerun — pre-existing test, unrelated to this diff.
  • tools::verifier:: — 9 passed / 0 failed.
  • PR CI on this head: all lanes green (check 18m53s, Analyze (rust) 16m11s, CodeQL/security/type-check/link/credentials pass).

Requested actions

  1. B1 — consume the pending SIGPIPE before restoring the mask (validated sigpending+sigwait pattern), and make the guard testable by re-arming SIG_DFL in the test process as described.
  2. M1 — Err on the stalled close (+ disclosure for Failed), with the queue-full-close pin.
  3. M2 — collapse the triple worker implementation; const the shared refusal strings.
  4. Minors: 1-4 are one-line comment/copy fixes worth taking now; 5-12 fine as follow-ups (7 and 13 deserve a tracker entry); 14 no action.

Note for the stack: #126 is rebased on this branch, so a fix wave here needs #126 rebased on top afterwards; #140 is already conflicting independent of this.

No-Issue context: slice 1 of the #126 split; findings and receipts in this comment are self-contained.

…ed writer

Round-2 fresh-review fixes (comment 5781947688):

- Host kill (BLOCKER): the SIGPIPE guard only deferred it. A SIGPIPE
  generated while blocked stays PENDING on the worker thread, and the
  guard's SIG_SETMASK restore delivers it before pthread_sigmask
  returns — with the process at SIG_DFL (run_with_args) the host died
  at signal 13 one instruction after the "handled" EPIPE, in exactly
  the documented stall-recovery shape (reproduced standalone: exit
  141 for both the immediate-EPIPE and the blocked-mid-write
  recovery; a sigwait consume survives). The drop now checks this
  thread's pending set and consumes a pending SIGPIPE with sigwait
  before restoring the mask. Std's SIG_IGN makes test binaries blind
  to this (a generated SIGPIPE is discarded at generation and never
  becomes pending), so the new
  sigpipe_guard_survives_a_broken_pipe_restore pin re-execs the test
  binary as a helper that re-arms SIG_DFL first: block-and-restore
  kills the helper at signal 13, consume-before-restore exits 0.
- Honest close results (MAJOR): a stalled close_stdin on a terminal
  task returned Ok while the schema promises a delivered end-of-file,
  and a second close silently destroyed the interact surface (also
  Ok). Both stalled close paths now return Err citing the stall (the
  delivery timed out / the close was not applied) and keep the handle
  attached; the Delivered/Failed arms keep their dispositions with
  the comments now describing them accurately. The
  stalled-terminal-close pin asserts the Err, and a new unit pin
  (bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered)
  classifies the close worker's outcomes against real pipes.
- One bounded writer (MAJOR): the worker machinery existed three
  times (the eof-delivery worker, the owned fallback, the inline unix
  clone block), and the SIGPIPE fix had to touch all three. The eof
  delivery and the unix clone payload write now ride the single
  bounded_stdin_write_owned helper — a returned clone is dropped,
  never reattached, so the original slot keeps deciding what is
  restored — and the deadline-dependent refusal copy is shared in
  named functions (format captures need fns, not consts). The
  cfg_attr dead-code allows the duplication required are gone, and
  the helper doc now describes the real ownership split (clone vs
  handoff) without the ShellTask link that pointed at a type that
  does not exist.
- Honesty and copy: the EOF refusal names the raw-mode exception; the
  generic refusal names the lost-writer case; close_stdin's schema
  discloses raw mode and the Windows handle-only release; the inline
  cap docs drop the "atomic" claim, scope the pipe-buffer rationale
  to Linux, and note the byte counting; the run-never-started context
  says the mentioned task id was never registered; the foreground
  close treats an evicted record as success instead of aborting a
  finished run; the PTY lane acquires the interact writer before the
  reader thread spawns so a dup/take_writer failure no longer
  orphans a spawned reader thread; the VEOF hardcode is acknowledged
  against portable-pty's live c_cc[VEOF] injection.
- Test hygiene: the three hang-guard scaffolds collapse into
  run_stdin_scenario_with_hang_guard (worker panics surface as failed
  verdicts), the stalled-close guard bounds with >=, and the
  lowercase tool description's stdin sentence gets its own pin.

Deferred unchanged, disclosed: the sync-lane inline payload write
stays SIGPIPE-guarded but deadline-less (no production caller can
reach it with a payload today; same family as the tracked
run_bounded_child_observed/process.rs follow-up); Windows-lane CI
coverage for the owned-path helper test.

Targeted: tools::shell 167 passed / 0 failed / 1 ignored
(pre-existing), tools::verifier 9 / 0; cargo fmt clean; fork-CI
clippy flag set clean in an isolated target dir. The ohos/windows
gated edits are verified by construction only — this host has no
ohos/windows C cross toolchain for the dependency build scripts
(rquickjs-sys/sqlite build scripts fail before codewhale-tui).

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-2 fix wave landed as f28a861 (parent 3eed997; server-side commit created via the Git Data API because git:443 is blocked from this host — tree asserted: f28a861c6^{tree} == 84102ae8d5f83cfcb0bd9acb84da16f66852bf0b, byte-exact against the locally committed e6f682f5d; DCO trailer present).

Fixes from this review, with verification:

  1. B1 (blocker) — the guard now consumes the pending SIGPIPE before restoring the mask. The drop checks sigpending() for SIGPIPE and consumes it with sigwait(); the falsified comments are rewritten to describe the pending-delivery mechanics. Pin: sigpipe_guard_survives_a_broken_pipe_restore re-execs the test binary as a helper that re-arms SIG_DFL (what run_with_args does in production), writes into a readerless pipe under the guard, and survives the restore. Mutation-verified red: with the consume removed (plain block-and-restore), the helper dies at signal: 13 (SIGPIPE) and the pin fails with exactly that message; with the fix it exits 0.
  2. M1 (major) — both stalled close paths on a unix terminal task now return Err (deadline-stalled delivery → stdin_stalled_close_refusal; post-stall close → stdin_stalled_close_not_applied_refusal) instead of Ok-with-silent-handle-drop; the Delivered/Failed arms keep their dispositions and their comments now state the real reasoning. Pins: the stalled-terminal-close pin asserts expect_err + "was not applied" + a prompt return (>= deadline bound), and a new unit pin (bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered) classifies Delivered/Failed/Stalled against /dev/null, a readerless pipe, and an O_NONBLOCK-filled pipe.
  3. M2 (major) — one bounded writer. bounded_stdin_eof_delivery and the unix clone payload write now ride the single bounded_stdin_write_owned helper; the inline worker block is gone (~70 lines), the deadline-dependent refusal copy is shared in stdin_stalled_write_refusal / stdin_writer_failed_refusal / stdin_stalled_close_refusal / stdin_stalled_close_not_applied_refusal (functions, so the deadline format-capture stays real), the now-unneeded cfg_attr allow(dead_code) pair is deleted, and the helper doc describes the real ownership split (unix clone vs owned handoff) with the broken ShellTask doc link and the inverted allow-attribution sentence removed.
  4. Minors taken: raw-mode qualifier on the EOF refusal; fourth (lost-writer) clause on the generic refusal; close_stdin schema discloses raw mode + the Windows handle-only release; byte counting noted in the stdin schema copy; "atomic write" dropped and the pipe-buffer rationale scoped to Linux in both const docs; run-never-started context says the task id was never registered; manager-level close_stdin_pipe treats an evicted record as success (with the foreground comment updated); PTY lane acquires the interact writer before the reader thread spawns (no orphaned reader on dup/take_writer failure); VEOF hardcode acknowledged; hang-guard scaffolds collapsed into run_stdin_scenario_with_hang_guard; lowercase description sentence pinned.

Disclosed, not changed: the sync-lane inline payload write remains SIGPIPE-guarded but deadline-less — no production caller can reach it with a payload today, and bounding it belongs in the same follow-up as the tracked run_bounded_child_observed/process.rs inherit; Windows-lane CI coverage for the owned-path helper test (no Windows host for this wave); the 5 s deadline value stays lower-bound pinned by design.

Gates on the wave tree: tools::shell:: 167 passed / 0 failed / 1 ignored (pre-existing) — includes the two new SIGPIPE pins, the close-worker classification pin, the reworked stalled-close pin; tools::verifier:: 9 / 0; cargo fmt --all -- --check clean; fork-CI clippy flag set clean on an isolated target after cargo clean -p codewhale-tui (genuine re-lint). ohos/windows-gated edits verified by construction only — this host's cross targets fail in dependency build scripts (rquickjs-sys/sqlite) before reaching codewhale-tui.

Downstream: #126 (base = this branch) inherits the commits without a rebase; #140 was already conflicting independent of this. CI is running on f28a861.

Round-3 wave from the fresh 10-lane re-review at f28a861:

- BLOCKER: stdin_stalled_close_refusal and
  stdin_stalled_close_not_applied_refusal had no cfg, but their only
  references sit in the all(unix, not(target_env = "ohos")) PTY-close
  arm - on Windows and ohos both fns are dead code, fatal under -D
  warnings (ci.yml RUSTFLAGS, ohos.yml, fork-ci clippy). Gate them with
  the arm's predicate, mirroring the PtyFile variant.
- The bounded_stdin_eof_delivery doc claimed a terminal with VEOF
  disabled "received plain bytes from the box too"; portable-pty 0.9.0
  writes nothing at all in that state (unix.rs guards if eot != 0), so
  the comment now states the real divergence and the close_stdin schema
  discloses the disabled-VEOF state alongside raw mode.
- Accuracy: the guard doc's "sigpending only sees thread-directed
  signals" parenthetical (sigpending also reports process-directed
  signals); write_stdin's lead comment (the owned-path Timeout,
  Disconnected, and spawn-failure arms deliberately leave the slot
  empty); the stdin_write_stalled field doc (a stalled close sets it).
- New pin close_stdin_that_stalls_on_fresh_delivery_refuses_and_
  stays_attached: the fresh-stall close arm (Stalled -> Err) had no
  guard; the record-level pin drives write_stdin("", true) against a
  full pipe wrapped in PtyFile and asserts the Err, the deadline wait,
  the restored handle + stall flag, the no-EOF flag, and the fast
  follow-up refusal. Mutation-verified red (Stalled -> Ok revert).
- Harden sigpipe_guard_survives_a_broken_pipe_restore: the helper now
  also asserts SIGPIPE is unblocked after the drop (a no-op restore
  went undetected; mutation-verified red) and writes a ran-canary the
  driver asserts, so a renamed helper cannot leave the pin silently
  green via libtest's exit-0-on-unmatched-filter.

Gates: tools::shell 174/0/1, tools::verifier 9/0, fmt clean, fork-CI
clippy flags clean (isolated target dir).

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh round-3 re-review of this PR at f28a861c6 (base 11cba403d = the pinvou3-clean tip, merge-base verified — no rebase needed; slice 1 of the #126/#140 stack, no parent PR). Ten fresh lanes: spawn-path completeness, SIGPIPE-guard falsification, close_stdin state machine, concurrency/wedge audit, test mutation-sensitivity, schema/copy/verifier accuracy, smuggling + commit hygiene, platform cfg correctness, PR-body claim audit, performance/resource audit. Local gates re-run at the reviewed tree (all green), and the two previously-pending CI lanes (check, Analyze (rust)) finished green — CI is now fully green at f28a861c6.

Verdict: approve direction. One BLOCKER (platform build breakage) was found, verified, and fixed in the wave below; one scary-looking MAJOR candidate was refuted; claims and disclosure check out; smuggling is clean. The wave commit now at the tip (8a390dee, tree-identical to the locally tested 6097e3cdc) carries every fix.

Dimension 1 — useful, root-caused, complete: YES

  • Both base wedges re-verified in origin/pinvou3-clean line-by-line: spawn_background_sandboxed piped stdin unconditionally and base closed it only for payload runs (execute_foreground_via_background), and the sync lane fell back to Stdio::inherit() for payload-less runs. The fix reaches all 11 spawn paths (census re-derived from scratch: 5 spawn sites in shell.rs + 2 in verifier.rs, expanded per lane) — each now delivers EOF, keeps the pipe for a documented deferred-interact reason, or was already inherently safe. The one ordering subtlety is right: the foreground close runs before the fallible heavy-permit attach, so no later failure can leak the write end.
  • The verifier gate change is quantified and sound: a stdin-reading gate previously idled the full 10-minute BACKGROUND_GATE_TIMEOUT_MS; the close is free (empty payload + pipe close = flag flip + drop), and no pass-regression is possible (gates are spawned with no stdin data, so a gate needing stdin was already doomed to timeout-fail).

Dimension 2 — elegance: YES

One bounded writer (bounded_stdin_write_owned) is the single write path on every platform; refusal copy lives in named shared functions; the cfg discipline (per-predicate gating of PtyFile/BoundedEofDelivery with documented dead_code rationale) is consistent — except for the one slip the wave fixes (below). No reinvented wheels; portable-pty semantics are used correctly (dup'd master writer instead of the EOF-on-drop box is the right construction, with the box's \n+c_cc[VEOF] sequence reproduced for explicit close).

Dimension 3 — smuggling: CLEAN

Mechanical hunk classification over the word-diff: exactly 4 files, +1845/−36, tests.rs all additions (zero deletions), all 30 hunks classified 28 CLAIM / 2 DISCLOSED-WAVE / 0 SMUGGLED / 0 refactor-churn. Zero slice-2/slice-3 content leaked (no grammar/readonly/fleet/test-role topics). Commit-by-commit coherence checked across all 6 commits — each matches its message; the intermediate 165-blank-line sweep in d6787ccc0 was fully reverted by b374d4aad (net-zero vs base, disclosed). No undisclosed behavior deltas.

Dimension 4 — defects

BLOCKER (found this round, fixed in the wave): the two unix-PTY-only close refusal helpers break the Windows and ohos builds. stdin_stalled_close_refusal / stdin_stalled_close_not_applied_refusal are defined without a cfg, but their only references (shell.rs:1877/1889) sit inside #[cfg(all(unix, not(target_env = "ohos")))]. On not(unix) and on ohos both fns are dead code — fatal under the repo's -D warnings (ci.yml RUSTFLAGS, ohos.yml, fork-ci clippy flags). The PR's own check lane is ubuntu-only so it cannot catch this; ci.yml's Windows legs, ohos.yml, and the release builds would go red after merge. The wave cfg-gates both fns with the same predicate, mirroring the discipline the PR itself applies to the PtyFile variant (the write-refusal pair stays un-gated: refs at 1748/1757 keep them alive on every platform).

Refuted (for the record): libc::sigwait availability. The platform lane's archive grep suggested linux/ohos libc 0.2.191 lacks sigwait. Resolved empirically and by source: unix/linux_like/mod.rs pulls linux_l4re_shared (which declares sigwait) into every target_os = "linux" target — gnu, musl, and ohos — and this host's clean cargo check probe plus the green 173-test run compiled the guard's Drop on linux-gnu. Not a defect.

No MAJOR findings. The round-2 fixes all verify fresh: the SIGPIPE guard's consume-before-restore is correct for every reachable interleaving (blocked-set is exactly {SIGPIPE}; SETMASK restore; both construction sites same-thread; the production SIG_DFL premise is real at lib.rs:1735/cli/src/main.rs), and re-verified by mutation this round: deleting the sigwait consume turns sigpipe_guard_survives_a_broken_pipe_restore red (helper dies at signal 13). The stalled-close honesty, handle reattachment, evicted-record Ok, writer-before-reader ordering, and Delivered/Failed/Stalled classification all check out against the code and their pins.

Minor follow-ups (disclosed, non-blocking, unchanged by this wave):

  1. Owned-platform (Windows/ohos) post-stall write_stdin("", true) still returns Ok via the empty-close idempotent arm before the stall check — the M1-class dishonesty this PR fixed on unix survives on the ownership lane (base returned Ok there too, so not a regression; untestable on unix CI). Follow-up candidate alongside the already-tracked Windows-lane test coverage.
  2. Windows/ohos owned caller branch has no execution coverage on any lane (the helper test runs on Linux; the caller-side slot retake/handback semantics are compile-checked only) — already disclosed in the PR body; restated here so it stays on the ledger.
  3. Sync-lane payload write remains SIGPIPE-guarded but deadline-less — disclosed deferral, still production-unreachable with a payload today (caller census re-verified).
  4. The combined_output foreground + payload shape now deterministically ends at the watchdog instead of base's prompt EOF — disclosed in guidance/schema/code/tests; bounded and recoverable via detach + close_stdin; recorded as the PR's single regression-direction change.
  5. The deadline value stays deliberately unpinned (the >= bounds are self-referential); the leading \n of the ^D pair is unpinned (a close delivering only \x04 would pass the PTY close pin) — both noted as test-hardening candidates.

Copy accuracy: every model-facing string checked against enforced behavior; two false/overstated internal comments were found and fixed in the wave (below). The stale Testing-section numbers and two line-cite drifts in the PR body are corrected in the body refresh.

Wave (round 3, 8a390dee)

  • BLOCKER fix: cfg-gate stdin_stalled_close_refusal / stdin_stalled_close_not_applied_refusal to the unix PTY close arm's predicate.
  • Honesty copy: the bounded_stdin_eof_delivery doc claimed a terminal with VEOF disabled "received plain bytes from the box too" — portable-pty 0.9.0 (unix.rs:400, if eot != 0) writes nothing when VEOF is disabled, so the explicit close and the old box genuinely differ in that state; the comment now says so and the close_stdin schema discloses the disabled-VEOF state alongside raw mode.
  • Accuracy fixes: the guard doc's "sigpending only sees thread-directed signals" parenthetical (sigpending also reports process-directed signals); the write_stdin lead comment ("every exit below restores the slot" — the owned-path Timeout/Disconnected/spawn-fail arms deliberately leave it empty); the stdin_write_stalled field doc (a stalled close sets it too).
  • New pin: close_stdin_that_stalls_on_fresh_delivery_refuses_and_stays_attached — the fresh-stall close arm (Stalled → Err) had no guard at all (the neighboring not-applied pin only reaches the skip arm); the new record-level pin drives write_stdin("", true) against a full pipe wrapped in PtyFile and asserts the Err, the deadline wait, the restored handle + stall flag, the no-EOF flag, and the fast follow-up refusal. Mutation-checked red before landing (Stalled→Ok revert).
  • Pin hardening: the SIGPIPE helper now also verifies the mask is actually restored after the drop (a no-op-restore mutant went undetected) and writes a ran-canary the driver asserts (a renamed helper would leave the pin silently green via libtest's exit-0-on-unmatched-filter); mutation-checked red (restore → no-op).

Gates on the wave tree: tools::shell 174/0/1 (the fresh pin included), tools::verifier 9/0, cargo fmt --check clean, fork-CI clippy flags clean in an isolated target dir. CI re-runs on the new head.

Deferred unchanged, disclosed in the body: sync-lane deadline-less payload write; Windows-lane execution coverage for the owned caller branch.

Round-4 review wave at 8a390de:

- collapse the verifier gate-close if-let pair into a let chain: the
  upstream all-targets clippy variant (-D warnings, collapsible_if not
  allowed) reported the verifier.rs block this branch adds as a third
  collapsible_if; the two remaining reports are the pre-existing base
  sites (core/engine/turn_loop.rs:5611, tools/subagent/mod.rs:8058)
- the stdin schema and the run-cap comment claimed an oversized write
  "would block the executor"; since the bounded writer, every such write
  surfaces as the 5-second deadline-stall refusal
- the sync-lane payload write now documents that this manager-API lane
  has neither the inline cap nor the bounded writer's deadline
- the closed-stdin refusal and the close_stdin schema name disabled or
  remapped VEOF next to raw mode as states that consume the EOF bytes as
  ordinary input
- manager close_stdin_pipe returns () — the close cannot fail, so the
  false Result contract and the ? at the foreground call site are gone
- new pin: spawn_aborts_with_run_never_started_context_when_the_payload_
  stalls drives the uncapped manager lane against a non-reading child
  and asserts the run-never-started / never-registered abort copy and
  the bounded-deadline elapsed bound

Gates: tools::shell 175/0/1, tools::verifier 9/0, fmt clean, fork-CI
clippy flags clean.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-4 fresh review — approve direction

Full fresh re-review (10 independent lanes, every conclusion re-derived from scratch, no prior-round state assumed) at 8a390dee; rebase verified as a no-op: merge-base == the pinvou3-clean tip 11cba403d (this PR is the stack root; #126/#140 sit downstream). CI on 8a390dee was green, including check and Analyze (rust).

Verdict: the PR does what it claims, at root, and nothing is smuggled — one MAJOR was found and it lives in the PR description's Testing claim, not in the diff. It is fixed in wave 5e32effd (tree d9b917a75b, asserted byte-identical to the locally built wave commit).

MAJOR — false Testing claim + a collapsible_if this branch adds (fixed in 5e32effd)

The body claimed the upstream --all-targets clippy variant "reports only the pre-existing core/engine/turn_loop.rs:5502 collapsible_if (base code this branch does not touch)". Reproduced at base and head with the ci.yml lint flags (cargo clippy --workspace --all-targets --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or):

  • base 11cba403d reports two pre-existing sites: core/engine/turn_loop.rs:5611 and tools/subagent/mod.rs:8058;
  • the reviewed head reported three — the third (verifier.rs:587) is this branch's own gate-close block, directly contradicting "base code this branch does not touch";
  • the cited line was stale (5611, not 5502).

ci.yml's own comment declares collapsible_if no longer allowed ("the allowances were hiding exactly the class of problem that reached the gate"), so this would have gone CI-red on any upstream landing. Fixed by collapsing the gate close into one let-chain (the form turn_loop.rs itself already uses). After the wave, the upstream variant reports exactly the two base sites again; fork-CI flags stay clean.

Also in the wave: the stdin schema and the run-cap comment said an oversized non-reading write "would block the executor" — false since the bounded writer (it ends as the 5-second deadline-stall refusal); the closed-stdin refusal and close_stdin schema now also name disabled or remapped VEOF; the sync-lane write documents that this manager-API lane has neither the inline cap nor the bounded deadline; the manager close_stdin_pipe returns () (the close cannot fail — false Result contract dropped); and a new pin, spawn_aborts_with_run_never_started_context_when_the_payload_stalls, covers the previously unpinned spawn-abort copy via the uncapped manager lane (2 MiB payload → deadline → context asserted, elapsed ≥ STDIN_WRITE_DEADLINE; the tool surface cannot produce this stall on Linux where the cap equals the default pipe buffer).

Gates on the wave tree: tools::shell 175/0/1, tools::verifier 9/0, cargo fmt --check clean, fork-CI clippy flags clean, upstream all-targets = the two base sites only.

Lane findings — verified clean

  • Claims/root-cause (2 lanes): every spawn path verified at HEAD (sync Stdio::null, foreground pipe close before the heavy-permit attach, PTY dup-of-master with unchanged spawn contract, persistent null, background open-pipe-by-contract, verifier gates EOF-at-spawn, foreground verifier gate output()→null, process.rs inherit disclosed-deferred). A same-family sweep across the whole crate (diagnostics, resource_admission, github/cli, test_runner, hooks, plugins, pdf, native_memory) found no other unbounded-stdin wedge. The record holds the only write end after spawn — dropping it is the EOF mechanism, i.e. the root fix.
  • Close state machine: the fresh-stall arm genuinely refuses and stays attached; double-close in every state traced; evicted-record-as-success is honest; every Ok arm's guarantee enumerated. Two disclosed-class minors remain: the unix pipe-lane close after a stalled write still returns Ok while the abandoned worker holds the fd clone (self-corrects — the stall flag stays down and later writes refuse), and the already-disclosed Windows/ohos owned post-stall empty-close early-Ok.
  • SIGPIPE: consume-before-restore verified sound for all in-process interleavings (in-process SIGPIPE is thread-directed; the process-directed case is explicitly scoped out in the struct doc); the pin exercises the production BlockedSigpipeGuard and is doubly mutation-sensitive (revert sigwait → helper dies at signal 13; drop the restore → sigismember assert; rename the helper → ran-canary assert). Two narrow robustness minors left open, both needing externally-timed signals: sigwait's return value is unchecked (EINTR would deliver the still-pending SIGPIPE at restore), and a failed pthread_sigmask(SIG_BLOCK) would restore a zeroed mask.
  • Concurrency/ownership: no double-take (the one expect is provably guarded), no leaked writer or fd on any error path, the only blocking-under-lock window is the documented bounded 5 s wait, and the stall flag structurally prevents a second writer per task.
  • Cross-platform: full cfg reachability table clean on every -D warnings lane (windows-msvc, ohos, macos, musl, android); libc::sigwait/pthread_sigmask availability verified in the registry source per target (linux_l4re_shared compiles into every target_os = "linux" env including ohos).
  • Smuggling: all 26 hunks across the 4 files attributed to the §1 fix or a named wave bullet; tests.rs deletions 0; no Cargo/CI/docs/config touch; no behavior change outside the stdin surface.
  • Design/perf: bounded_stdin_write_owned is genuinely new (no in-tree prior art; tokio::spawn_blocking ruled out by the abandon-on-timeout semantics); one deadline site, three callers; SIGPIPE cost is ~3 syscalls per guarded region, never per byte; no busy-waits; the 64 KiB cap math is correct for Linux and degrades harmlessly elsewhere.
  • Copy: every model-visible string verified against the code, including the portable-pty 0.9.0 if eot != 0 disabled-VEOF claim (read in the registry source) and the Windows handle-only-release disclosure. The maxLength (code points) vs byte-enforcement unit mismatch stays a deliberately disclosed wart.

Test-suite audit

Cataloged 19 new pins + 1 scaffold. Strongest: the SIGPIPE re-exec driver/helper (production guard, canary-protected), the fresh-stall close pin (the only guard on that arm), the bounded-write manager pin. Weaknesses (disclosed, not fixed): three deadline pins have no local hang guard — on a regression they redden via nextest's slow-timeout (60 s period) or wedge a plain cargo test run; the helper halves of the two re-exec pins stay permanently green without their env keys (rename drift is canary-protected, driver deletion is not — inherent to the pattern).

Follow-ups (disclosed, unchanged)

run_bounded_child_observed's Stdio::inherit() (process.rs, tracked); sync-lane deadline-less write; Windows-lane execution coverage for the owned caller branch; pipe-lane post-stall close Ok; sigwait EINTR; unscaffolded deadline pins; maxLength unit note; manager-API uncapped inline write (comment added this wave).

CI is re-running on 5e32effd; the tree is byte-identical to the locally gated wave commit.

The PTY spawn comment said "a stalled close releases it" about the
dup'd master fd: no stall path releases it — both stall arms restore
the handle into the record (the fresh-stall arm under the stall flag,
the not-applied arm verbatim), and it is the Failed delivery arm that
releases. The comment now says that.

The BlockedSigpipeGuard SAFETY note claimed a failed `pthread_sigmask`
leaves "the failure mode ... the pre-guard one". The write-phase half
of that claim holds (the write runs at the process default
disposition), but the drop's restore would then install the zeroed
`previous` mask — an empty `SIG_SETMASK`, leaving this thread's
signals unblocked — which is not the pre-guard mode. The comment now
describes the real consequence instead of a false reassurance.

Comment-only; no behavior change. Gates at this tree: tools::shell
175/0/1, tools::verifier 9/0, `cargo fmt --all -- --check` clean,
fork-CI clippy flags clean.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-6 review (fresh 10-lane re-derivation + gates, pinned at the round-6 wave 7deeca87d)

Every lane re-derived this PR from scratch (no prior-round conclusions were supplied to the lanes); the rebase onto pinvou3-clean was verified a no-op (merge-base == tip 11cba403d).

Verdict: claims TRUE, root cause fixed at lane level, smuggling CLEAN, 0 blocker / 0 major. Two comment-level factual defects found and fixed in this round's wave (7deeca87d, tree ba63eb17c — comment-only, landed via the Git Data API chain after both 443 push forms failed, no behavior change).

Gates (all reproduced at 5e32effd before the wave)

  • cargo fmt --all -- --check — clean.
  • Fork-CI clippy flags (--workspace --all-features --locked -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or -A clippy::collapsible_if -A clippy::assertions_on_constants) — clean.
  • Upstream ci.yml all-targets clippy variant at base and head: both fail with exactly the two pre-existing collapsible_if sites (core/engine/turn_loop.rs:5611, tools/subagent/mod.rs:8058); this branch introduces none (test code included — the lib test configuration lints clean beyond the two lib sites).
  • tools::shell 175/0/1 and tools::verifier 9/0 — re-run twice on this host (warm and cold target dirs, default features both times; the Testing section's numbers reproduce).

What the lanes confirmed

  • Root cause and completeness (lane A): both base mechanisms verified at base exactly as the body describes — execute_foreground_via_background closed the pipe only if stdin_data.is_some() (base shell.rs:4584-4590), and execute_sync_sandboxed left std's default inherit() on payload-less runs (base shell.rs:2493). Head fixes both at the spawn/lane level; the spawn-lane matrix shows every lane with its intended stdin disposition, and the interact surface of the background lane is preserved. Honest boundary finding, disclosed in the body and re-confirmed: execute_sync_sandboxed currently has no production caller with background=false (tests + the Windows PowerShell retry only), so the sync half of the fix is correct but latent today; and the same-family Stdio::inherit() in run_bounded_child_observed (tools/process.rs) survives as the tracked deferral, including the tasks.rs foreground-gate lane.
  • Implementation (lane B): full write_stdin truth table (35 arms, three cfg families) — every arm restores-or-deliberately-discards with honest flags; the SIGPIPE guard verified POSIX-correct on its live paths (thread-directed SIGPIPE pending-at-drop, sigwait consume before SIG_SETMASK restore, nesting safe, run_with_args/cli main both arm SIG_DFL).
  • Platform/cfg (lane C): every cfg predicate exact on linux/macos/windows/ohos; ohos tty early-return proven to make the stripped PTY block unreachable; portable-pty 0.9.0 claims verified against the pinned crate source (UnixMasterWriter::drop if eot != 0, no ConPTY Drop); try_clone_to_owned/ChildStdin: From<OwnedFd> compile-proven on stable 1.98 for linux/ohos/macos.
  • Integration (lane D): every caller of the changed functions gets the intended contract; verifier gate close deadlock-free with its Err arm provably unreachable; the close-before-heavy-permit reorder strictly dominates base on every post-spawn early-return (base leaked on attach_heavy_permit failure and never closed at all without a payload).
  • Adversarial state machine (lane I): 12 interleavings (kill-during-write, late-landing payloads, close-vs-eviction, raw-mode closes, concurrent members) — all consistent; the stall flag enforces one-writer-per-handle at every lock-release point; no false EOF claim in any reachable state.
  • Performance/resources (lane H): measured worker roundtrip 15–60µs (thread-per-write immaterial at tool cadence); no fd leak on any path; abandoned-worker retention bounded by child lifetime with kernel-woken EPIPE/EIO; 5s bounded lock holds strictly improve base's permanent wedge; the verifier gate close converts a 10-minute idle per stdin-reading gate into an immediate completion.
  • Tests (lane E): every mechanical fix carries a red/green pin that reddens under its target mutation; asserts are drift-hardened (exact equality, prefix + negative substrings, rename canaries); scaffolds kill or self-release children on all exit paths. Pins cover 21 new tests; the generic/persistent/finished refusal tail and the close_stdin schema disclosures are copy-verified but not pinned — acceptable, noted below.
  • Smuggling (lane G): all 36 deletions attributed to in-scope rewrites; fork-guard fingerprints intact; guidance paragraph sentence-audited; verifier-gate extension and the 64 KiB cap are body-disclosed.

Fixed in this round's wave (comment-only)

  1. Stale parenthetical in the PTY spawn comment (shell.rs:3399): "a stalled close releases it" — no stall path releases the dup (both stall arms restore the handle; the Failed arm releases). Comment now says so.
  2. False failure-path claim in the BlockedSigpipeGuard SAFETY comment (shell.rs:951): "a failed pthread_sigmask … the failure mode is the pre-guard one" — the write-phase claim is right, but the drop would then restore the zeroed previous mask (an empty SIG_SETMASK, unblocking this thread's signals), which is not the pre-guard mode. Comment now describes the real consequence.
    (A cold-target default-features reproduction of cargo test -p codewhale-tui --lib was also run this round after a lane reported a dead_code break there: it compiles and passes 175/0/1 — no break exists.)

Follow-ups (unchanged deferrals plus this round's new minors; none merge-blocking)

  • Same-family Stdio::inherit() in run_bounded_child_observed (tools/process.rs) — the tasks.rs foreground-gate/code-execution lane keeps the hang family this PR fixes for shell lanes; disclosed every round, still the top-priority follow-up.
  • Copy-layer minors (all conservative/under-claiming direction): the stalled-close refusal could disclose that the abandoned \n\x04 may land late when the queue drains; the closed-stdin refusal's lead clause overclaims EOF in raw mode (its own parenthetical corrects it); the spawn-abort composed message pairs inner "kill it and start a new one" with the outer "do not try to cancel or kill it" (the outer clause is authoritative and comes last); "delivering the inline stdin payload failed" overclaims certainty on the writer-panic arm (inner copy truthfully says unknown).
  • Comment-layer minors: the Failed-arm disposition conflates fd-clone/spawn failure with "child already gone" — under fd exhaustion it would drop a live task's interact surface while reporting Ok (EMFILE-level trigger; the write path's clone-failure arm restores instead); the guard's zeroed-previous restore consequence (now documented, not behavior-fixed — SIG_BLOCK on local stack sets cannot realistically fail).
  • Test-layer minors: three new pins carry load-sensitive timing (elapsed < 2900ms bound, one blind 150ms sleep before the detach request, one boundary leg that can slow-mark >60s under heavy parallel load — observed red once on a loaded 2-way host, green isolated, red-direction only); three deadline pins rely on nextest's 60s/600s bounds under mutation (disclosed in the body); refusal-tail copy is unpinned (see above).
  • Retained base quirks adjacent to this surface: close_stdin on a task with no handle returns Ok while the schema promises a delivered EOF; the foreground close's own lock acquisition remains a theoretical poisoned-lock leak (base-identical); the UI jobs-panel stdin send can hold the manager lock up to 5s on a stalled task (bounded; base was unbounded).
  • Previously disclosed, still deferred: sync-lane deadline-less manager-API payload write (no production caller); Windows-lane execution coverage; sigwait return unchecked; maxLength code-point vs byte counting ("mirrors the same number" is deliberate); pipe-lane close after a stalled write returning Ok.

Waves: this round = comment-only, tree ba63eb17c; tools::shell 175/0/1 + tools::verifier 9/0 + fmt + fork-CI clippy re-verified on the wave tree.

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已按 head 7deeca87d1a0e884761ed8d322ac5531ec696059 / base 11cba403dc6bd6ca5c4dca55042785b03449ac8e 完成四个文件的规范与需求双轴审查。正常 EOF、payload、后台交互及 verifier 路径的修复已确认,但仍有一个源码确认的 P2 进程资源隔离问题,修复前暂不能批准。

依据本仓 SECURITY.md,详细位置、触发条件、影响和修复建议保留在本地私密审核证据中,未在公开讨论披露复现步骤或安全边界细节;需通过维护者认可的私密渠道处理。当前结论不声称已执行利用、真实信息泄漏或原生平台攻击测试。

实际验证:最新 base/head fetch、完整 diff/调用链及平台 cfg 源码检查、git diff --check、相关原生 API/固定依赖源码核对,并只读核对该 head CI。本机未运行 Cargo/Rust、原生 Unix/Windows 或 provider 测试;作者报告的测试计数不作为本轮执行结果。发送前已核对最新 head/base 和讨论未变化。

@asto18089

Copy link
Copy Markdown
Collaborator Author

已收悉(review 5800879419)。按本仓 SECURITY.md 约定:该事项走维护者认可的私密渠道处理,公开线程不携带任何位置、触发条件或边界细节。修复与验证结论在私密渠道闭环后,本 PR 的复核依赖(含 review 5800940768 指出的组合阻塞)随之解除。

@zhuowp zhuowp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes on 7deeca8.

One P2 resource-ownership and isolation regression remains on a supported execution path. The changed process-I/O implementation does not preserve the previous isolation and lifetime properties across all relevant launches. Please address this before merging. Precise conditions, source-chain evidence, limitations and counterevidence are retained in the local restricted v3 report; no public reproduction or stronger exploit claim is made here.

The intended no-payload EOF behavior and the full cumulative four-file change were reviewed separately. Current Linux CI executes the applicable affected cases; the Windows-specific reader fixture is not claimed as Linux execution. Passing EOF cases do not establish the broader ownership property. No new local runner or live exploit was performed.

Nonblocking follow-up: a failure after process creation must not be described as “run never started”; no automatic retry path was confirmed. Source, execution, merge gates and release authorization remain separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants