Repository navigation
Conversation
asto18089
left a comment
There was a problem hiding this comment.
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_sandboxedpiped stdin unconditionally andexecute_foreground_via_backgroundclosed 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 withStdio::null()(2567–2576). The background lane keeps the open pipe for deferredinteractstreaming, pinned by the untouchedtest_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 untilBACKGROUND_GATE_TIMEOUT_MS= 600 s. Other gate paths spawn viaCommand::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-READYoutput 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_eofgoes red (the probe wedges to the 3 s timeout,result.successfalse). 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 onclose=trueforPtyFile, then drop and setstdin_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:trueon 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
- Panic-vs-stall divergence between the two bounded-write implementations (shell.rs:1481–1520 unix inline worker vs
bounded_stdin_write_owned910–934, the Windows/ohos production path): the unix path mapsDisconnected(worker panic) to "writer failed, no stall flag"; the owned helper conflates it withTimedOut, so a panicked Windows/ohos worker permanently poisons the task with the false "the command stopped consuming its stdin" reason. Unify on one helper returningRecvTimeoutError+ a shared stall-message constructor (the 3-line refusal copy is currently pasted verbatim twice). - Cap units: schema
maxLength: 65536counts 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 toMAX_INLINE_STDIN_BYTES(this file's culture has exactly such sync tests, e.g. the timeout-description pins). - 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).
- Split invariant between the two close paths:
BackgroundShell::close_stdin_pipe(1433–1438) setsstdin_closed_at_eofunconditionally, while thewrite_stdinclose guards the identical assignment with&& !stdin_write_stalled(1584). Latent today (the only call site closes before any write), butclose_stdin_pipeispub(3239) with no caller outside the file — make it private and add the stall conjunct. stdin_availablelies differently per platform after a stall: unix restores the slot (1516) so snapshots reportstdin_available: truewhile all writes are refused; Windows/ohos leaveNone→false. Refusal copy is correct on both; the surfaced boolean should agree.- verifier.rs:592
.ok()swallow: unreachable failure today, but ifwrite_stdinever becomes fallible the gate silently reverts to idling 600 s — exactly the bug this PR fixes, invisibly. Atracing::warn!would pin it. - Sync-lane spawn payload is still an unbounded blocking
write_all(2607–2614), andSTDIN_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. - 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). - Tests: the
PtyFilearm ofbounded_stdin_write_ownedis untested on every platform and the Windows/ohos owned path never runs in CI;inline_stdin_payload_over_the_cap_refusesis fully#[cfg(unix)]though only the boundary leg needs it; fixed 150/300 ms sleeps beforerequest_foreground_background(5114, 5173) can spuriously fail under load — aRunning-status poll (as the stalled-tty pin does) is robust.
CI / body
- Required
linkcheck is red: the body has neither a closing keyword nor aNo-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.
|
Fix wave pushed — 6337b0e (implements the round-1 review, review id 5768207421, pinned at e13a366):
Gates at 6337b0e: Still open — needs a decision (mutually exclusive, from the review's M1/M2): Also proposed as a follow-up PR (not this slice): Body corrections (claim-premise, unix scoping, same-contract wording) and the |
|
Round-1 decisions implemented — wave 2: a2ec04f
Follow-up tracker (issues are disabled on this repo, so this thread carries it): That closes every review finding except the deliberately-deferred minors listed in the round-1 review (schema code-point units note, |
a2ec04f to
9148c17
Compare
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>
9148c17 to
8c6c00d
Compare
asto18089
left a comment
There was a problem hiding this comment.
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 acceptedcombined_outputbehavior 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):
- Windows/ohos
close_stdin:trueon 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 inUnixMasterWriter; the ConPTY writer has no such drop). Per the repo's explicit-platform-interface rule: document the divergence in schema/copy or refuse explicitly. - Stale docs invalidated mid-PR: the
PtyFilevariant doc ("PTY closes never claim EOF", shell.rs:881-882), thestdin_closed_at_eoffield 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. stdin_closed_at_eof = trueis set even when the\n\x04write failed or partially landed (both resultslet _ =-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.- Spawn-time payload stall kills the child and fails the whole run (shell.rs:3374-3379), contradicting the
STDIN_WRITE_DEADLINEcomment's abandon-and-continue description (shell.rs:8033-8040); also PTY lanes (~4 KiB queues) now failtty:true/combined_outputruns with large payloads that base delivered. Align comment and behavior. - Caller-side
thread::spawnpanic 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. - 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.
- 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>
|
Fix wave pushed —
Deliberately not taken: the sync-lane unbounded payload write (latent — no production caller reaches it with a payload; same family as the Gates at |
asto18089
left a comment
There was a problem hiding this comment.
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+sigwaitconsuming 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
shell.rs:1023— doc link[`ShellTask::write_stdin`]points at a type that exists nowhere (it'sBackgroundShell); the onlyShellTaskoccurrence in the repo is this link.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).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.shell.rs:5651— the close_stdin schema clause doesn't disclose the Windows split (shell.rs:1863-1870releases the handle and returns Err) or the stall no-delivery case (M1).- 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. shell.rs:5246-5255— "fits a Linux pipe buffer in one atomic write": atomicity ends atPIPE_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.- 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. - 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. 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.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.shell.rs:3357-3376— ondupfailure the already-spawned PTY reader thread is orphaned (detached; base spawned the reader only aftertake_writersucceeded). Tiny; note only.shell.rs:1008—b"\n\x04"hardcodes VEOF; portable-pty's Drop reads livec_cc[VEOF]and injects nothing when disabled. Parity holds for every realistic termios state; one word of acknowledgment in the comment.- Tests:
bounded_stdin_write_owned_returns_the_writer_and_times_out_cleaniscfg(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-panicDisconnectedbranches, 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. - 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 deferredrun_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 aftercargo clean -p codewhale-tuiso 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-existingbackground_tty_command_has_controlling_terminallost 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
- B1 — consume the pending SIGPIPE before restoring the mask (validated
sigpending+sigwaitpattern), and make the guard testable by re-arming SIG_DFL in the test process as described. - M1 — Err on the stalled close (+ disclosure for Failed), with the queue-full-close pin.
- M2 — collapse the triple worker implementation; const the shared refusal strings.
- 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>
|
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: Fixes from this review, with verification:
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 Gates on the wave tree: 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
left a comment
There was a problem hiding this comment.
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-cleanline-by-line:spawn_background_sandboxedpiped stdin unconditionally and base closed it only for payload runs (execute_foreground_via_background), and the sync lane fell back toStdio::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):
- 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. - 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.
- Sync-lane payload write remains SIGPIPE-guarded but deadline-less — disclosed deferral, still production-unreachable with a payload today (caller census re-verified).
- 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. - The deadline value stays deliberately unpinned (the
>=bounds are self-referential); the leading\nof the ^D pair is unpinned (a close delivering only\x04would 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_refusalto the unix PTY close arm's predicate. - Honesty copy: the
bounded_stdin_eof_deliverydoc 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 theclose_stdinschema discloses the disabled-VEOF state alongside raw mode. - Accuracy fixes: the guard doc's "
sigpendingonly sees thread-directed signals" parenthetical (sigpending also reports process-directed signals); thewrite_stdinlead comment ("every exit below restores the slot" — the owned-path Timeout/Disconnected/spawn-fail arms deliberately leave it empty); thestdin_write_stalledfield 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 driveswrite_stdin("", true)against a full pipe wrapped inPtyFileand 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
left a comment
There was a problem hiding this comment.
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
11cba403dreports two pre-existing sites:core/engine/turn_loop.rs:5611andtools/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, not5502).
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 gateoutput()→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
BlockedSigpipeGuardand is doubly mutation-sensitive (revert sigwait → helper dies at signal 13; drop the restore →sigismemberassert; 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 failedpthread_sigmask(SIG_BLOCK)would restore a zeroed mask. - Concurrency/ownership: no double-take (the one
expectis 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 warningslane (windows-msvc, ohos, macos, musl, android);libc::sigwait/pthread_sigmaskavailability verified in the registry source per target (linux_l4re_sharedcompiles into everytarget_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_ownedis genuinely new (no in-tree prior art;tokio::spawn_blockingruled 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 != 0disabled-VEOF claim (read in the registry source) and the Windows handle-only-release disclosure. ThemaxLength(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
left a comment
There was a problem hiding this comment.
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.ymlall-targets clippy variant at base and head: both fail with exactly the two pre-existingcollapsible_ifsites (core/engine/turn_loop.rs:5611,tools/subagent/mod.rs:8058); this branch introduces none (test code included — thelib testconfiguration lints clean beyond the two lib sites). tools::shell175/0/1 andtools::verifier9/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_backgroundclosed the pipe onlyif stdin_data.is_some()(baseshell.rs:4584-4590), andexecute_sync_sandboxedleft std's defaultinherit()on payload-less runs (baseshell.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_sandboxedcurrently has no production caller withbackground=false(tests + the Windows PowerShell retry only), so the sync half of the fix is correct but latent today; and the same-familyStdio::inherit()inrun_bounded_child_observed(tools/process.rs) survives as the tracked deferral, including thetasks.rsforeground-gate lane. - Implementation (lane B): full
write_stdintruth 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,sigwaitconsume beforeSIG_SETMASKrestore, nesting safe,run_with_args/climain 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::dropif 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_permitfailure 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_stdinschema 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)
- 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. - False failure-path claim in the
BlockedSigpipeGuardSAFETY comment (shell.rs:951): "a failedpthread_sigmask… the failure mode is the pre-guard one" — the write-phase claim is right, but the drop would then restore the zeroedpreviousmask (an emptySIG_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 ofcargo test -p codewhale-tui --libwas also run this round after a lane reported adead_codebreak 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()inrun_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\x04may 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-previousrestore consequence (now documented, not behavior-fixed —SIG_BLOCKon local stack sets cannot realistically fail). - Test-layer minors: three new pins carry load-sensitive timing (
elapsed < 2900msbound, 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_stdinon 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;
sigwaitreturn unchecked;maxLengthcode-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
left a comment
There was a problem hiding this comment.
已按 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 和讨论未变化。
|
已收悉(review 5800879419)。按本仓 SECURITY.md 约定:该事项走维护者认可的私密渠道处理,公开线程不携带任何位置、触发条件或边界细节。修复与验证结论在私密渠道闭环后,本 PR 的复核依赖(含 review 5800940768 指出的组合阻塞)随之解除。 |
zhuowp
left a comment
There was a problem hiding this comment.
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.
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 thetestrole contract — slice 2 is #126, rebased on this branch).1. Foreground and sync shell commands never delivered stdin EOF (a
$SHELLprobe 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 withstdin(Stdio::piped())and keeps the write end inBackgroundShell.stdin.execute_foreground_via_backgroundclosed 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_sandboxedset.stdin()only for payload runs, so std's defaultStdio::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 — deferredaction="interact"stdin streaming is a feature (test_write_stdin_streams_output,every_advertised_stdin_spelling_reaches_the_commandstay 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 foregroundcombined_outputrun with astdinpayload 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; interactclose_stdin:trueon 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 byinteract_close_stdin_delivers_eof_to_a_terminal_command). The bash guidance text now states the foreground stdin contract so a bare$SHELLprobe 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):
interactrefusal 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 genericstdin is not available for task Nmessage;interacton it says so and points atbackground=true;write_stdin(task_id, "", true)) instead of idling on the held-open pipe until the gate timeout.The bash guidance sentence and the
stdinschema (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_pipeand dropped its unneededpub, restored the healthy stdin handle on write/clone error paths, logged the verifier gate close failure instead of swallowing it, pinned the schemamaxLengthliterals 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-familyStdio::inherit()inrun_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):
pthread_sigmaskreturned — 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 withsigwaitbefore restoring. Std's SIG_IGN makes test binaries blind to this (discarded at generation, never pending), so the newsigpipe_guard_survives_a_broken_pipe_restorepin 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.close_stdinon 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 classifiesbounded_stdin_eof_deliveryas Delivered/Failed/Stalled against real pipes.bounded_stdin_write_ownedhelper (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.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 livec_cc[VEOF]injection.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; base11cba403d== thepinvou3-cleantip, no rebase needed):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 thePtyFilevariant. (Alibc::sigwaitavailability concern raised during review was refuted:unix/linux_like/mod.rscompileslinux_l4re_shared— which declaressigwait— into everytarget_os = "linux"target including ohos.)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.rsguardsif eot != 0). The comment now states the real divergence and theclose_stdinschema discloses the disabled-VEOF state alongside raw mode.sigpendingparenthetical (it also reports process-directed signals);write_stdin's lead comment (the owned-path Timeout/Disconnected/spawn-failure arms deliberately leave the slot empty); thestdin_write_stalledfield doc (a stalled close sets it too).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 driveswrite_stdin("", true)against a full pipe wrapped inPtyFile; mutation-verified red (Stalled → Ok revert).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 checkcannot get past rquickjs-sys without the platform C toolchains, so the cfg proof here is by construction, same as thePtyFilegating).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
\nof 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-cleantip11cba403d):collapsible_ifthis branch adds. The previous Testing bullet claimed the upstream--all-targetsclippy variant "reports only the pre-existingcore/engine/turn_loop.rs:5502collapsible_if (base code this branch does not touch)". Reproduced at base and head with theci.ymllint 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 declarescollapsible_ifno 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.stdinschema 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 theclose_stdinschema 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).MAX_INLINE_STDIN_BYTESnor the bounded writer's deadline (no production caller reaches it with a payload; disclosed deferral).close_stdin_pipecannot fail, so it returns()— the always-OkResultand the?at the foreground call site are gone.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'sStdio::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 plaincargo test);maxLengthcounts 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-cleantip11cba403d; the upstream all-targets clippy variant reproduced at base and head: the same two pre-existingcollapsible_ifsites, none introduced):7deeca87d, treeba63eb17c): 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 theBlockedSigpipeGuardSAFETY note claimed a failedpthread_sigmaskleaves "the failure mode ... the pre-guard one" — the write-phase half holds, but the drop's restore would then install the zeroedpreviousmask (an emptySIG_SETMASK, leaving this thread's signals unblocked), which is not the pre-guard mode. Both comments now state the real consequences.execute_sync_sandboxedhas no productionbackground=falsecaller at head (tests + the Windows PowerShell retry only), so the sync half of the fix is correct but latent today; therun_bounded_child_observed(tools/process.rs)Stdio::inherit()deferral remains the top tracked follow-up (thetasks.rsforeground-gate lane keeps the hang family this slice fixes for shell lanes).\n\x04landing 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); theFailedclose 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 theclose_stdinschema disclosures are copy-verified but unpinned.cargo test -p codewhale-tui --libcompile 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;
sigwaitreturn unchecked;maxLengthcode-point vs byte counting; pipe-lane close after a stalled write returning Ok.Testing
tools::shell175 passed / 0 failed / 1 ignored (pre-existing) andtools::verifier9/0 green at the round-6 wave head (7deeca87d, treeba63eb17c, byte-identical to the locally gated tree; the same counts were re-verified at5e32effdbefore the wave, and re-run from a cold target dir with default features during review).cargo fmt --all -- --check— clean.--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 oncodewhale-tui. The upstream--all-targetsvariant (-D warnings, nocollapsible_ifallow, theci.ymllint flags) reports only the two pre-existing base sites (core/engine/turn_loop.rs:5611,tools/subagent/mod.rs:8058— both fail identically on base11cba403d); this branch introduces none.Checklist
No-Issue: slice 1 of the #126 split; the split ledger on #126 tracks the whole series