Repository navigation
fix(subagent): release interrupted claims and inherit worktree isolation - #123
Conversation
asto18089
left a comment
There was a problem hiding this comment.
Fresh review of d031e21f5 (2026-10-25, from scratch)
Verdict: CHANGES REQUESTED — 0 blockers, 4 majors (3 code, 1 scope), ~10 minors. Posted as a COMMENT because this is my own PR.
Base / rebase. pinvou3-clean tip is still ea9fdc90f, this PR's parent — no rebase needed. #119 (declared disjoint) is still open and is not a base: git merge-tree --write-tree --merge-base=ea9fdc90f d031e21f5 2318a056b is conflict-free, and the two liveness notions (write-claim here, session-name there) are independent and complementary. Either order merges cleanly; #119 first keeps this body's references accurate.
Gates (local stable 1.99.1, warm target with identical Cargo.lock)
cargo fmt --all -- --check: clean.cargo clippy -p codewhale-tui --all-targets --all-features --locked+ CONTRIBUTING allow list: only the disclosed pre-existingcloned_ref_to_slice_refsatrepo_law.rs:638. (Without-A clippy::collapsible_ifthere is also a pre-existingturn_loop.rs:5407hit — untouched file.)- Full
cargo test -p codewhale-tui --locked --lib: 12240 passed / 1 failed / 13 ignored — exactly the disclosed numbers; the failure isforkguard_model_operation_auxiliary_retry_keys, which passes alone (1/1, 0.09s). - PR CI: all green (fork-ci
check17m51s).
Dynamic verification (targeted mutations, each reverted afterwards)
- Restoring the old paired-parked liveness allowance:
waiting_for_user_headless_worker…,interrupted_child_stops_gating_later_writers,interrupted_child_resume_does_not_contend…red;write_capable_grandchild…green — the body's mutation claim is exact. parent_runs_in_isolated_worktree → false: grandchild test red. Droppinglevels == 0: discovery test red.workspace_relative_mutation_pathpassthrough: abs-path test red.- Removing the
rev-parse/branch/worktreeadmissions:agent_readonly_shell_admits_real_reconnaissance_shapesred, 67 othercommand_safetytests green. Binary sniff →false:contract_read_refuses_binary_payloads…red. Budget →usize::MAX: schema test red. Absolute image path refused:resolve_image_path_accepts_absolute…red.
All 11 new/modified tests are genuine regression pins.
Scope hygiene
All 34 hunks map to the six declared areas; no smuggled hunks, no drive-by refactors or formatting churn. All nine named tests exist. The [shell.readonly.command] tag, the "required":["path"] pin, the harness/ambiguity discovery tests and the /etc/hosts pin are intact. The three "checked and left alone" claims hold in code: no detached field on SubAgent/AgentWorkerSpec/AgentWorkerRecord so cancel_all_running_for_session does hit detached children; allowed_tools is absent from the 13 advertised fields; #119 does edit the custom text and the field count.
MAJOR
M1 — resolve_absolute_target is a fifth hand-rolled copy of the deepest-existing-ancestor walk (crates/tui/src/tools/subagent/mod.rs:16203-16218). codewhale_core::walk_deepest_existing (crates/core/src/lib.rs:235-275) is documented as "the single source for the deepest-existing walk … so their fallback semantics can never drift apart again (the round-45 finding was four copies …)", and the write tool's own boundary walk (tools/spec.rs:1245, resolve_nonexistent_path) and core/authority.rs:639 already delegate to it. The claim-scope lane is exactly the lane that must agree with the write tool's resolution, and the copy already diverges in two corners: a missing path ending in ./.. (lexical cancel via normalize_path_components(candidate) vs the core's pop-without-record + compose) and the canonicalize-failure fallback (lexically normalized ancestor vs the core's raw ancestor spelling). Both fail closed today, so there is no escape — but this is precisely the drift the consolidation exists to prevent. Replace with codewhale_core::walk_deepest_existing(candidate, DeepestExistingFallback::DeepestAncestor).compose() (codewhale_core is already in use at mod.rs:9424).
M2 — Isolation inheritance silently removes write serialization inside a worktree. register_claim skips the overlap check for any isolated_worktree=true claim and excludes isolated rows from everyone else's check (coord/ledger.rs:518-527). That rule's premise was one isolated claim per worktree. With inheritance, a worktree child (claim .) can fan out several write-capable children, each with the default . claim, all admitted into the same checkout with no contention against each other or the parent — while the identical fan-out in the shared root is refused with "serialize the work, narrow the claim, or use worktree isolation" (the ledger has no parent/child exemption). The new test pins exactly this shape (parent and grandchild both isolated_worktree=true, roots ["."], both admitted), and the new docs sentence ("registered on the same isolated footing") does not disclose it. The fix trades a false refusal for a lost guarantee. Remedy: carry the worktree identity on isolated claims (the owner's workspace is at hand at the seam) and run the overlap check among claims sharing a worktree — or namespace inherited claims against the worktree as a coordination sub-root. Confinement itself is intact (prepare_child_workspace + cwd containment), so this is a serialization gap, not an escape.
M3 — is_agent_readonly_git_branch mis-models --column and admits git branch --list --column -d victim (command_safety.rs:849-875). In git, --column is --[no-]column[=<style>] — attached form only; it never consumes the next word (verified on git 2.54: git branch --column -d victim deletes the branch). The parser lists it under value_flags and does index += 2 without inspecting the swallowed token, so -d victim passes the checker uninspected. The shape fails today only because git's own --list/-d mutual-exclusion rejects it (usage error, rc 129) — the grammar's contract is read-only by construction, not by downstream argv validation. The shared option engine already refuses a dash-prefixed value (options_match_allowlist, command_safety.rs:1063); the bespoke parser diverged from it. Fix: refuse a swallowed value that starts with - in both new parsers (or treat --column as a plain switch), and pin git branch --list --column -d x in the reject table. Everything else I could construct refuses or is read-only (-lD, --list=x, --contains -D x, --format -d foo, worktree list --expire prune, rev-parse --git-path/--parseopt/--sq-quote); the preamble admits only --no-pager/-C, so no -c core.fsmonitor=… injection.
M4 (scope) — six fixes across four subsystems should be split. No hunk is unrelated, but command_safety.rs widens a trust-boundary surface (what read-only roles may execute), file.rs/dispatch.rs/vision/tools.rs have nothing to do with the coordination ledger, and the subject fix(subagent): … / "reproduced four defects" under-describe the diff. CONTRIBUTING distinguishes well-scoped PRs from ones that "mix scope" or touch trust-boundary surfaces, and reverting a grammar problem would also revert the claim-liveness fixes. Suggested cut: (A) coordination — fixes 1, 2, 4, the mutation_paths half of 3, docs; (B) read-only git grammar + refusal message; (C) receipts — binary read, schema budget, image_analyze. M1–M3 land in their respective halves.
MINOR
- Body: "no new layer: one private helper on
SubAgentManager, three free functions" — actual: 1 method + 6 free functions (workspace_relative_mutation_path,resolve_absolute_target,is_agent_readonly_git_branch,is_agent_readonly_git_worktree,bounded_valid_shape,contract_read_payload_is_binary) + 2 consts. "the F12 test stays green" is an internal label — name the test. - Body:
rev-parse --git-pathis listed among "mutating forms"; it is read-only and refused only by table omission. "coordination_workspace_prefixcan never strip" is overstated — in the nested-repo layout (anchor adopts a single child checkout) the worktree lands inside the coordination root. - Body/message: "a PDF … is refused" is not guaranteed — a PDF with a NUL-free first 8 KB still decodes lossily. The refusal advice names
unzip(not in the read-only grammar) and "PDF/document readers" (no model-visible PDF tool for sub-agents), so for the read-only roles most likely to hit it onlyfileis actionable. Routing a%PDFhead to the existingPdfTextCommand(as the legacyReadFileTool::executedoes) would be the natural improvement. parent_runs_in_isolated_worktreedoc says "live claim" butfindmatches any row including released history (harmless — same flag). The manifest-fallback leg is untested at the spawn seam; only the claim leg is.coordinate inspectshows the parked interrupted claim with no active/inactive marker, and the contention remediation text (mod.rs:15829) still lists "ask the parent to cancel it" first — a no-op for an interrupted blocker — ahead ofcoordinate action=release, which now actually works on it. Parked rows persist until capacity eviction (128) or session close.- Declaration seams still refuse absolute in-workspace spellings via the same
normalize_claim_path(write_roots,coordinate action=claim; mod.rs:13183-13188), so declare-vs-mutate spelling is now inconsistent. By design per the body (the ledger speaks repo-relative) and not receipt-fed, so a follow-up;read_lints(lsp.rs:312-319) is the same class vs its sibling LSP ops. - Grammar: both new parsers re-implement the
agent_text_filter_options_matchloop;-lmeant--create-reflogbefore git 2.20 (worth a comment); the refusal message under-advertises (npm show|info,comm, the gh read-only families,codewhale --version,-C/--no-pagerpreambles) — the conservative direction. vision/tools.rs:102still says "must be a relative path" in the traversal branch now that in-workspace absolutes are admitted; the relative branch dropped theRootDirarm (Windows-only\foo, still caught by containment).- Tests:
scoped_mutation_paths_accept_absolute_targets…is red on base only via the signature change, and its refusal asserts cannot distinguish absolute from traversal (shared message); the registry call-site wiring is unpinned. The discovery test's red power is coupled toMAX_PARENT_LEVELS == 4with no in-test guard. Unpinned:FE FFBOM, 2048/2049 boundary, schema withoutproperties,--column -dreject,branch -D/-M/-c/-C/-f/--edit-description/-u,worktree lock/move/repair,rev-parse --parseopt/--sq-quote, depth-3 inheritance. - Docs: resume-after-a-completed-writer is inherent to checkpoint continuation (resume serializes only against live writers); one sentence that the resumed loop must re-orient would complete the new paragraph. zh parity is fine (the 7-vs-2 line asymmetry is wrapping; the translation is complete and accurate).
Verified correct
- F1: full state matrix — the only live→not-live flips are paired cells with agent ≠ Running (all intended);
SubAgentStatushas no pre-Running state, so no admission window; prior-boot Running agents are rewritten to Interrupted on load;interrupt_child→WaitingForUserparking andresume_from_checkpointnew-id dispatch verified end to end;release_stale_write_claimsandfinalize_session_close(non-terminal records included) now clear interrupted owners; no consumer of the predicate regresses. - F2: worktrees are siblings of the checkout; a grandchild's cwd is containment-checked against the worktree; the manifest
worktreeflag has no cleanup/removal consumer anywhere, so no worktree-deletion regression;action=claimandvalidate_write_scoperead the record flag; depth 3+ inherits. - F3: workspace and candidate canonicalize symmetrically (
/var→/private/varon both sides);<ws>/new/../../etc/passwd,<ws>/link-to-outside/x,<ws>/existing-file/child, trailing./..all fail closed; relative inputs are untouched; only absolute inputs pay 1–3 stats; image containment still goes through canonicalize +starts_with. - F4: single production caller (
create_isolated_worktree);Tried:lists only the walked chain;project_context.rsfind_git_roothas no sibling scan. - F5: every family the message lists is admitted and every refused shape is refused; the message is emitted only for read-only-bash refusals;
PARALLEL_READONLY_PREFIXEShas one consumer and stays consistent. - F6: the sub-agent
readwires toexecute_contract_read(the fixed path); no other model-facing lossy reader exists; serde_jsonpreserve_ordermakes the field inventory deterministic; the budget is global;apply_patch(1506 B) stays verbatim.
Method: 9 independent review lanes plus personal verification of every MAJOR and all mutations; warm target /tmp/cw-pr122-base/target.
|
Fix wave pushed as M1 — shared deepest-existing walk. M2 — same-worktree serialization. M3 — grammar. Minors fixed: binary-read guidance names Not done, deliberately: the PR split. The six areas were reviewed together and each carries its own red/green; splitting now would orphan this review thread for marginal revert-granularity. Remaining review minors (declaration-seam absolute spellings, Validation at |
asto18089
left a comment
There was a problem hiding this comment.
Fresh review round 2 (2026-10-25) — 45e5207c5 on ea9fdc90f
From-scratch re-review: 13 lanes (one per fix area, smuggling sweep, body audit, elegance, performance, adversarial MAJOR verification, and a live gates+mutation battery in a separate worktree). Base pinvou3-clean has not advanced since ea9fdc90f, so the branch already rides latest main — no rebase needed.
Summary: all six fix areas are real, root-caused, and honestly documented (body audit: every claim traced TRUE; the checklist inventory — two helpers, five free functions, one ledger field, one delegation — is exact). Smuggled changes: none (all 15 files attribute to the six areas; commit boundaries clean; no dep/CI/version content). Performance: no regressions; git_repo_root is a material improvement (per-spawn $HOME scans → ≤5 probes). Gates: fmt clean; clippy has only pre-existing errors, none in PR files; targeted suites green; full lib 12242/1/13 with the known rotating flakes (each passes alone, none in PR modules). Mutations re-run fresh: disabling the paired-parked allowance reddens exactly the three F1 tests; disabling the sibling check or the ancestor exemption reddens the F2 tests; breaking the 2048-byte budget boundary reddens the schema test.
However, two MAJOR gaps survive — both are completions of this PR's own claims, with minimal fixes available.
MAJOR-1 — cwd-narrowed children bypass the same-worktree serialization this PR adds
The spawn seam keys the persisted claim's worktree_root on the child's own workspace (mod.rs:6721-6725, same shape at the fleet seams mod.rs:4784-4788, 4843-4847). A child spawned with an explicit cwd: sub inside a worktree parent resolves its workspace to <wt>/sub (worktree.rs:42-75), inherits isolation via parent_runs_in_isolated_worktree, and registers worktree_root = Some("<wt>/sub"). The sibling predicate compares exact string equality (ledger.rs:534-541), so it never matches the parent's or a root sibling's Some("<wt>"). Meanwhile a cwd-only spawn explicitly keeps the parent's full root set (mod.rs:9637-9645), an isolated . claim authorizes every path (ledger.rs:133-141), validate_write_scope checks only the owner's own claim, and the unbounded-shell gate is skipped for isolated children entirely (shared_write_claim → None). Result: worktree parent claims . at <wt>; spawn sibling A (root workspace) and sibling B (cwd: sub) — or two cwd-narrowed siblings — and both are admitted to one checkout; the unchecked fan-out section 2 claims to have closed is still reachable. The pinned test cannot catch it: every agent in worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not is constructed with ToolContext::new(&worktree) and no cwd override, and it calls spawn_background_with_assignment_options directly, so prepare_child_workspace never runs.
Fix direction: when isolated_worktree, resolve the key to the containing worktree — walk the parent chain (the walk worktree_claim_exemptions already performs), take the nearest ancestor claim whose worktree_root contains the child workspace, and use that root; fall back to the child's own root for claim-less manifest-isolated parents. Plus a regression test: one cwd-narrowed sibling against a worktree-root sibling must contend.
MAJOR-2 — a resumed writer can lose its claim permanently (Fix-1 interaction)
Fix 1 makes parked claims non-live, which makes them sweepable and evictable: coordinate action=release removes every non-live owner's row (ledger.rs:606-639) — and the shell-gate refusal text itself instructs the model to "run agents/coordinate action=release to clear a stale claim", so a model following instructions sweeps a parked interrupted sibling; capacity eviction (128) also targets oldest-inactive, i.e. precisely parked claims. resume_from_checkpoint_with_policy restores the claim only by ledger lookup (mod.rs:5993-5998); on a miss the resumed child is write-capable (profile preserved, mod.rs:6012-6015) but registers no claim — every mutation then fails with "has no registered write claim" (mod.rs:4124-4128), and agent action=claim refuses too ("has no write claim to expand", mod.rs:4047-4055), so there is no recovery short of a fresh spawn. The resume comment promises "the worker record's launch manifest is read below as the durable fallback", but the manifest is read only for worktree (the isolation flag), never for the scope it already carries (writable_roots/files/contracts, mod.rs:6832-6843). The lockout this PR fixes recurs in narrower, unrecoverable form.
Fix direction: when the lookup misses, rebuild the claim from the manifest via the existing worker_coordination_claim (mod.rs:1332-1402), guarded to skip empty-scope manifests (claim-less worktree children must stay claim-less); claim_pre_namespaced: true then routes through the existing spawn-seam re-registration and its contention admission.
MINORs (each personally traced; none merge-blocking on its own)
Command safety:
- The dash-prefixed-value guard in the branch parser is unpinned — mutation-verified: reverting it to the old unconditional
index += 2swallow leaves the suite 75/75 green (only the--columnswitch shape is pinned). Add a reject pin likegit branch --list --sort -d x(the worktree parser's value-flag path likewise). - False refusals re-feed the retry loop the new message warns against: bare
git branch(a real listing),-a/-r/-v, combined shorts, all attached=spellings (--column=always,--format=x,--short=8),--exec-path. The message listsrev-parseunqualified though only a flag subset is admitted —branchis carefully qualified,rev-parseis not. (--format='%(…)'is unreachable anyway: the caller charset gate rejects parens.) parallel_classifier_stays_unchanged_for_parent_auto_approvestill asserts the parent's parallel chunks "keep rejecting" the relaxed families, but this PR addsgit rev-parsetoPARALLEL_READONLY_PREFIXES— safe (same bounded table) but the stated invariant no longer holds as written, and neither direction is pinned.
Receipts:
4. The UTF-16 BOM arms are unpinned — mutation-verified: deleting both starts_with arms keeps the test green because both UTF-16 fixtures carry NULs (h\x00i\x00). Make one fixture genuinely NUL-free (e.g. \xFE\xFF\x4F\x60…). The message pin also doesn't assert file (the round-1 requirement), and UTF-16 text files are told to unzip -p a .pptx.
5. The reduced schema receipt hardcodes "type":"object" even for composition-only (oneOf-over-string) schemas, showing "properties":[] — a model reading only the receipt could infer no fields exist; the pointer text mitigates.
Ledger/genealogy:
6. Two legacy None-root isolated claims satisfy None == None and will contend with each other across different worktrees after upgrade (reachable via restored headless records); the field doc says legacy keeps the never-contending footing — consider requiring existing.worktree_root.is_some().
7. The exemption-walk cap for _ in 0..15 is a bare literal whose safety depends on MAX_SPAWN_DEPTH_CEILING = 8; nothing documents the coupling. A mid-chain pruned worker record (prune at 256 terminal records) silently truncates the walk → false contention against a deeper live ancestor.
8. The ancestry exemption is one-directional: a worktree parent widening its own claim is refused by its own live child, and a resume initiated by the root session loses genealogy (parent_agent_id becomes the resumer's), so the resumed child can contend with its still-running original parent.
9. Smaller: an interrupted child's in-flight write can still land until the async abort takes effect (strictly better than the base's permanent lockout; awareness only); parked rows are reclaimed only at session close/release/eviction and show in inspect; a deleted parent worktree before spawn registers a ghost-path claim (fail-closed, but the failure surfaces later without naming the cause).
Worktree discovery:
10. The no-repo error's "Tried:" still lists five absolute paths including all four ancestors (disclosure reduced, not eliminated; ancestors were only probed, never scanned). The narrowing also silently breaks the workspace-sibling-of-repo layout (previously resolved via the ancestor child-scan); no migration note anywhere.
Body/docs:
11. "Disjoint from #119" is topic-disjoint, not file-disjoint: six shared files; hunk ranges are disjoint (nearest pair ~5 base lines apart) and the contention message is byte-identical, so composition holds — whichever merges second rebases into the other's context. Worth one qualifying word in the body.
Elegance (all small): worktree_claim_exemptions is a third hand-rolled parent-chain traversal beside worker_ancestry and ensure_caller_controls_descendant (unify on the dual-source semantics this PR introduces); the worktree_root prep expression is written out identically at three sites; the branch/worktree parsers duplicate the value-flag consumption block; the interrupted-test preamble (~40 lines) is duplicated.
Verdict
Not approving in this state: MAJOR-1 leaves the PR's own serialization claim incomplete and MAJOR-2 converts the headline fix into a new unrecoverable dead end in a plausible scenario. Both fixes are small, in-scope, and carry obvious red/green tests. Everything else — usefulness, root-causing, honesty of the body, layering, performance, hygiene — holds up cleanly.
|
Round-2 disposition (fix wave pushed as MAJOR-1 — fixed ( MAJOR-2 — fixed ( Minors fixed ( Deliberately left (each disclosed in the round-2 review, none merge-blocking): the one-directional exemption walk (a parent widening its own claim is refused by its live child; a root-session resume serializes against its original parent) — a design question, not a regression, and the doc already says resumed loops re-serialize against live writers; the triplicated ancestor-chain traversal ( Gates at |
ed712da to
cee2d40
Compare
Fresh round-3 review (2026-10-25) — head
|
|
Fix wave landed on top (head
Gates at the new head: fmt clean; clippy unchanged (same 7 pre-existing, all in untouched files); targeted 1490/0/4; full lib twice — 12286/4 and 12289/1, all the known rotating load-timing flakes in untouched modules, each passing alone. Disclosed-not-fixed follow-ups listed in the body's round-3 section (expand-path process-lock asymmetry, duplicated isolation precedence, fleet-seam keying, root-resume genealogy, parentheses posture). |
Review round 4 (2026-10-28)Fresh multi-lane re-review of MAJOR 1 — Fix 5 does not reach execution for the probes it exists to unblockThe classifier work is real: MAJOR 2 — Whitespace-padded absolute paths validate one path and write another
Mutation-claim correction (this body's Testing section)The claim that "disabling the ancestor exemption reddens it at the first spawn together with
Other findings (all minor or nit; none block once the majors land)
Verified sound (so it doesn't get re-litigated)Fix 1 end-to-end: the base defect premise reproduces from VerdictCHANGES_REQUESTED on the two majors: (1) make Fix 5 effective for bare spellings (extend |
Fix wave (2026-10-28, head
|
asto18089
left a comment
There was a problem hiding this comment.
Fresh review (2026-10-28, head f8f204b13)
A fresh 15-lane re-review of the full diff (6e1ee88ee..f8f204b13, 19 files), independent of the four prior rounds, plus an executed verification lab. Verdict: approve (no blockers, no majors). GitHub refuses an APPROVE event on a self-authored PR, so the verdict is recorded as a comment review. Non-blocking findings below.
Rebase check: pinvou3-clean is at 6e1ee88ee and the merge-base of this branch equals it, so the branch already sits exactly on the base tip — no rebase needed.
Verification executed at f8f204b13
cargo fmt --all -- --check: clean.cargo clippy -p codewhale-tui --all-targets --all-features --locked: exactly the 7 pre-existing errors, all in files this PR does not touch (context_report.rs:422,engine.rs:1876/8523,turn_loop.rs:881/5497,lib.rs:12432,repo_law.rs:638) — matches the body's account.- Targeted suites, all 0 failed, counts matching the Testing section exactly:
tools::subagent::618,command_safety::75,tools::shell::142,tools::file::126,vision::tools::19,core::engine::654,coord::40. - Executed mutations at this head (each reverted after, 9 runs, 0 compile errors): the true ancestor-exemption kill reddens BOTH
worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_notandwrite_capable_grandchild_of_a_worktree_child_inherits_isolation; restoring paired-parked liveness reddens all three liveness pins; restoring the trim reddens the padded-path pin; reverting the hardener whitelist, the serdeskip_serializing_if, and the dual-separator join each redden exactly their pin. The corrected round-4 mutation claim is verified by execution.
Findings per criterion
- Useful / complete / root-caused — yes. All six fixes verified real against base code, each root-caused as claimed: the paired-parked liveness lockout (old predicate +
resume_from_checkpoint's new-id re-dispatch confirmed; resume cannot break because no in-place resurrection path exists), the worktree isolation inheritance (everyoptions.isolated_worktreeconsumer migrated to the combined flag; explicit shared-root cwd structurally impossible viavalidate_existing_child_cwd), the absolute-spelling claim lane (delegates through the crate's singleresolve_deepest_existing, the same walk as the write tool's boundary), the$HOMEdiscovery scan (anchor-only child scan; anchor adoption and ambiguity-at-anchor tests byte-identical to base), the read-only git probes (classifier + hardener + parallel lane all verified; no admitted mutating shape found in an adversarial enumeration, empirically cross-checked against git 2.43), and the two receipt defects. - Elegant — yes. Near-minimal surface (3 private methods + 2 free functions + 1 const + 3 fields), deliberate anti-drift delegation instead of reimplemented walks, all new costs constant-bounded (15-hop cap, 128-row ledger cap, 8 KiB sniff, 2 KB budget) with no performance regression found on the spawn/mutate/resume paths (the per-spawn full
worker_records.clone()is pre-existing at base). - Smuggled changes — none. Full commit/file/hunk/deletion audit: every hunk maps to a declared fix or a disclosed round minor; all 76 deleted test names reappear in the added set (zero dropped pins); exactly the three declared struct fields, no dep/CI/version changes; topic-disjointness from #119 confirmed at hunk granularity.
- Bugs — none found. Feature-wise and performance-wise clean in all 15 lanes; the minors below are edge-case robustness, accuracy, and pin-strength items, all failing in the safe direction.
Non-blocking findings
Minors worth a follow-up (none blocks):
tools/shell.rs:3997— the new hardener comment says "process-spawning hazards … belong to diff/log/show only", butgit blameexecutes a repo-configureddiff.<drv>.textconvby default (verified on git 2.43), andblameis admitted both in the classifier tables and inPARALLEL_READONLY_PREFIXESwithout a--no-textconvsplice. The exposure is pre-existing and not widened here, but the comment cements a false invariant — splice--no-textconvforblameor correct the comment and file the follow-up.tools/subagent/mod.rs:16077— the[shell.readonly.command]guidance omits-fprint0/-truncatefrom the find refused set (the round-3 body claim "enumerates the full find refused set" is false as written); harmless-direction omissions too (ghread-only families,npm show|info,codewhale --versionare admitted but unlisted). No dangerous-direction mismatch was found.tools/subagent/mod.rs:4176— the re-anchor offset joins rawto_string_lossybytes (NFD on APFS) against NFC-normalized roots/paths, so a cwd-narrowed child with a non-ASCII cwd on macOS refuses every natural spelling. Fail-closed, but it is exactly the refusal class this PR fixes; the shared lane NFC-normalizes throughnormalize_claim_path.tools/subagent/mod.rs:6166— the resume manifest-claim fallback converts rebuild errors into claim-less resumes silently: a write-capable child resumes, then refuses every mutation with no logged cause. Fail-safe but opaque (the fleet seam refuses on the same errors); worth a log line. The "claim-less manifests stay claim-less" leg is code-true but untested on the resume path.core/engine/dispatch.rs:345— therequiredomission tail sits insideif !required.is_empty(), so a reduced schema whose required names are ALL skipped (>200 bytes) silently implies nothing is required; andVALID_SHAPE_INVENTORY_BYTES(1024) is enforced but no test observes it — the overflow fixture trips the 200-byte name cap instead.tools/subagent/mod.rs:16600—workspace_relative_mutation_path's deepest-existing fs walk runs beforenormalize_claim_path's cheap length/control-char refusal, so an adversarial many-component absolute path performs thousands of stats before being rejected. Bounded and fail-closed; check length first.docs/SUBAGENTS.md:207(+ zh twin) — "the ledger does not surface what completed while it was parked" overstates:coordinate inspectdoes list parked-period rows (without active markers). Suggested wording: "does not mark which of those writers completed".tools/subagent/mod.rs:6869(+ body §2) — "namespacing … fails by construction — worktrees live outside the checkout" is false in the deliberately-supported nested-checkout-adoption topology (the worktree lands under the wrap root and the prefix strips fine); the real guard isnamespace_write_claim's isolated early return. Behaviorally moot post-fix; fix the stated invariant so a future refactor does not trust it.
Body-accuracy notes (no code change required):
- "Mutation: … reddens exactly these three" undercounts:
resumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_goneandswept_cwd_narrowed_worktree_child_resume_rekeys_on_its_manifest_worktree_rootalso redden via their stale-sweep legs. Conservative direction (more pins than claimed). - The round-4 "the pins' stub parents were never live owners" narrative is not reproducible from head code statically — the stub spawn inserts the agent as
Runningand nothing demotes it in-test, so the corrected claim appears to hold with or without the round-4 status pokes. The pins are armed either way, and the executed run at this head confirms them. - Checklist accounting: "three private helpers on
SubAgentManager" omitsreanchor_isolated_mutation_paths; the struct-field count is three, not two.
Pin/coverage notes (optional):
- attached
git worktree list --expire=<time>is admitted in code but unpinned (the branch parser's attached values are pinned); - "branch/worktree listing parsers stay interactive-only" is asserted in a comment but never asserted false for benign shapes;
- the padded-path pin stops one step short of end-to-end: the write-tool half (untrimmed
required_str, raw absoluteness inresolve_path) rests on separately-verified stable semantics — a registry-lane padded twin ofchild_write_tool_resolves_absolute_spellings_against_the_childs_workspacewould close it; - no end-to-end
execute()admission pin for a bare absoluteimage_path(the feature is pinned at unit level only); - the hardener's defense-in-depth is thinned for the three admitted subcommands (documented classifier-owns-shape trust boundary; only
git pushpins the mutating negative at the seam).
Disclosed leftovers re-checked and accepted: the ancestry-walk triplication (now sharing worker_parent_ref), the fleet seams keying spec.workspace (including the post-sweep descendant corner), the one-directional exemption, the "Tried:" ancestor listing, and the row-over-manifest resume precedence pin (the disagreeing state is unreachable from production code, so the absence of the pin is fine until one side becomes mutable).
Recommendation: merge. Items 1–8 are follow-up material at your discretion; none blocks.
JensenChen28
left a comment
There was a problem hiding this comment.
Reviewed fixed head f8f204b13b87471d9a8533c35fcc6fb1cbc86310 against merge-base 6e1ee88ee. The current target is pinvou3-clean at f1eb10efcf; a read-only integration diagnostic is conflict-free (tree d517d3e25659bf8d6a587b6d69ab2700c654e698), and the affected vision blob is unchanged in that combination.
Standards
[P2] Allow the inherent Windows prefix in an otherwise valid absolute image path. In crates/tui/src/vision/tools.rs:100-106, the new is_absolute() branch still rejects Component::Prefix(_). A normal Windows drive-absolute path, including the canonical verbatim form, necessarily contains such a component, so an image inside the workspace is rejected before the canonicalized containment check can validate it. The newly added, non-platform-gated resolve_image_path_accepts_absolute_paths_inside_the_workspace constructs an absolute temporary path and expects success, but encounters the same rejection on Windows. Admit valid absolute prefixes while keeping traversal and canonicalized workspace containment checks; do not weaken the outside-workspace negative case.
No further required issue was confirmed in the inspected claim-liveness, restored-claim/worktree inheritance, sibling-versus-ancestor, absolute-mutation containment, anchored repository discovery, bounded Git admission, or schema/binary-read paths.
Spec
The same P2 leaves the stated screenshot/attachment contract incomplete: absolute and relative spellings of an in-workspace image are intended to be equivalent, but the new allowance currently works only on platforms without Windows prefixes. Both independent review axes confirmed this branch and the new test's construction. The separate absolute mutation-path helper does not have this same prefix rejection.
Validation: cumulative diff whitespace passed locally; current-head CI check/gate and analysis contexts report success. The Windows finding is source-confirmed, not a claim that I ran a native Windows Rust test. Cargo is unavailable in this environment; I did not rerun the author's native/mutation suites or compile the latest-target combination. No code changes, merge, queue, or publishing actions were performed.
zhuowp
left a comment
There was a problem hiding this comment.
固定审核:head f8f204b13b87471d9a8533c35fcc6fb1cbc86310,tree e5025e5b0bd6e883d3054471b303915636d2ec22;实际目标 pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c;merge-base 6e1ee88ee8f8ea28eec7be3d8add3a591468412a。 API base.sha仍为merge-base,不是当前目标头;只读组合树 d517d3e25659bf8d6a587b6d69ab2700c654e698 无文本冲突,未编译/执行。Cargo清单和锁未变;本PR也未更新其他父仓的gitlink。
请求修改,确认三项P2:
- 同一worktree内并行写者的无界命令缺少对应冲突保护。
crates/tui/src/tools/subagent/mod.rs:6884–6888,16239–16246、coord/ledger.rs:544–565:本次新增继承隔离允许多个无祖先关系的Full-shell写者在同一checkout内以不重叠声明并行运行,但后续无界mutation门仍排除全部isolated claims;workspace级执行策略不能证明命令只改声明目录。因此可能覆盖同checkout其他live写者产出。预期是已有多写者checkout保护随新准入链路延续;不是要求独占写者的全部shell永远按File声明隔离,也不是发现越出worktree或只读角色提权。引入类型为新增准入暴露旧“isolated没有同checkout peer”假设的组合缺口。已从真实spawn、new_with_owner(enforce_write_claim=true)、registry到ShellTool策略直接静态核对;抽取predicate探针支持判断,但没有运行全应用覆盖文件演练。建议按containing-worktree识别live peer并保留祖先委托、不同worktree和readonly豁免,或提供真实范围限制;完整证据及反证在本轮本地审计详单。 - Windows合法绝对图片路径仍被拒绝。
crates/tui/src/vision/tools.rs:100–106新绝对分支拒绝Component::Prefix(_),而盘符/UNC/verbatim绝对路径必带Prefix;真实execute在:299调用resolver,所以workspace内截图/附件绝对路径在canonical containment前失败,相对路径可用。新增:607起测试在Windows也同样失败。这是本PR新增修复的跨平台缺口,旧版也拒绝绝对路径,不能误称已可用功能回退。原生Windows逐字抽取resolver探针复现;只移除绝对分支Prefix拒绝的负控使同一文件两种拼写成功,最终containment仍须保留。未调用视觉provider。 - 新增padded-path测试的expected未按ledger分隔符契约归一化。
crates/tui/src/tools/subagent/tests.rs:3591–3595用Windows canonical path原始反斜杠构造expected,而生产normalize_claim_path(:13542)会转换并按正斜杠拼接。即使正确保留前导空格,Windows断言仍不等;影响原生测试交付,不是产品路径安全漏洞。静态调用链及原生ASCII探针确认,探针Unicode trait仅在ASCII域等价适配,未跑整crate测试。expected应使用ledger规范并单独保留空格语义。
独立审核:A需求与行为、B生产者/消费者和实际目标兼容、C状态/中断/恢复、D权限与真实shell接缝、E测试有效性/CI;首报固定后由未参与实现者裁决。A/B/C候选曾提前到协调者而未交换给首审;需求包作者自评接触已披露。早期继承候选的D/E作废,最终D/E为空上下文独审。实际覆盖19个变更文件:spawn→scope/ledger→manifest→loop→interrupt/fresh-resume、同root sibling/ancestor、cwd/reanchor/absolute mutation、discovery、read-only git argv、binary sniff、schema反馈及测试构造。巨大tests diff按函数身份重对齐,546个既有顶层函数体未变、35增改body语义核查;其余159行外围diff及全部非函数项已专用比较,并经独立提取交叉验收;不是把原始Git移动误配hunk冒称逐句审核。
验证与限制:裁决重跑Windows resolver正/负控及padded探针;其余manager/registry测试以静态断言审查为主。无固定干净树全Cargo产品/测试目标成功,也无真实OS进程中断/恢复和provider旅程;一次缓存树cargo尝试中止不计合规构建。当前head fork CI成功但只覆盖Ubuntu;不证明Windows,也不证明最新target组合。B已核目标26文件增量与本PR接缝(stream chunks→final parse→schema反馈、guidance、Cargo配置),相关缺陷片段组合未修复;新partial-input完整组合执行仍未验证。不采纳作者大计数。既有外审Windows问题仍成立,与第2项同根;未按批准数决定结论。
四类结论:代码审核请求修改,三项仍未修;功能验证部分完成,关键静态链路与定向原生探针成立,全装配/目标组合/进程恢复仍欠证据;合并资格未满足/未确认,当前head绿灯不能替代真实组合门禁及外审,rules空列表/classic404也不能推出完全无保护;发布资格未评估,无发布授权及跨平台交付证明。未修改产品、合并、入队或发布。
Live multi-agent session (2026-10-24, deepseek-flash) reproduced four defects;
each fix below carries a red/green test.
1. An interrupted write-capable child kept its whole-workspace claim live
forever. Its worker record parks in WaitingForUser with a continuable
checkpoint, and the liveness predicate counted a paired parked record as a
live writer — but a resume re-dispatches under a NEW id, so the parked
claim could never be exercised again. Every later write-capable spawn in
the session (including claims narrowed to a subdirectory) was refused with
'write-scope contention with <the interrupted child>', and the child's own
followup resume contended with its predecessor; cancel is a no-op on an
interrupted child (it only touches Running children). Liveness now follows
the paired agent's status: only a Running agent from this boot is a live
claimant; the approval-prompt case (Running + WaitingForUser) is unchanged.
2. A write-capable grandchild spawned from inside an isolated worktree was
refused with 'shared writer workspace ... must remain inside coordination
root'. Worktrees are created as siblings of the checkout, so namespacing
the grandchild's claim against the coordination root fails by
construction; the grandchild is de facto confined to the worktree. The
spawn seam now inherits the parent's isolation (live claim flag, falling
back to the launch manifest) and the persisted-claim prompt says
worktree-relative for isolated claims.
3. Scoped writes spelled with an absolute path inside the child's own
workspace were refused ('must be repo-relative') while the same target as
a relative path succeeded. mutation_paths now strips the workspace prefix
from absolute paths that resolve inside the workspace (canonicalizing the
deepest existing ancestor, since a write may create the file); absolute
paths outside and traversal are refused as before. image_analyze gets the
same treatment: absolute spellings resolve through the existing
workspace-containment check.
4. worktree discovery on a non-repository workspace walked up to $HOME and
scanned its children, reporting unrelated repos ('Multiple git
repositories found under /Users/asto...'). Child-scan now happens only at
the anchor; ancestors are probed for containment only.
Plus two text/efficiency defects from the same session:
- the bounded read-only shell grammar admits the git probes a worktree child
needs (rev-parse, branch --show-current|--list, worktree list) and the
refusal message now enumerates what IS allowed (a reviewer retried refused
shapes 13 times against the old one-liner);
- contract read refuses binary payloads (.pptx/.docx/.zip, UTF-16, NUL-laden
containers) instead of returning 50KB of mojibake, and validation receipts
bound oversized input schemas to their field inventory instead of echoing
the multi-KB agent schema on every semantic refusal.
Signed-off-by: asto <asto18089@users.noreply.github.com>
…parsers git's --column is an attached-only OPTARG (--column[=<style>]), so the separate-word spelling leaves the next token a real flag. The branch parser listed it as a value flag and swallowed that token uninspected, admitting e.g. 'git branch --list --column -d x'; only git's own --list/-d mutual-exclusion kept the shape from deleting anything. Both listing parsers now refuse a dash-prefixed value (the shared option engine already does), --column moves to the switch list where it belongs, and the admit/reject tables pin the swallow shape, -D, --edit-description, -u, worktree lock/move, rev-parse --parseopt, and the -l/--list -v/-z listing forms. Also documents that -l means --list only since git 2.20. Signed-off-by: asto <asto18089@users.noreply.github.com>
…est-existing walk Two review findings on the isolation-inheritance change: The ledger skipped the overlap check for every isolated claim, a rule premised on one isolated claim per worktree. With inheritance, a worktree child could fan out several write-capable children all claiming "." in the same checkout and none of them would contend — a fan-out the shared root refuses. Isolated claims now carry their canonical worktree root, and claims registered against the same root contend unless the existing owner is an ancestor of the new one (the ancestor chain is resolved from the spawn seam, claim expansion, and the headless preflight/commit paths), so parallel siblings refuse with the usual contention receipt while parent -> child -> grandchild chains stay admitted. Records persisted before the field existed deserialize with a nil root and keep the legacy footing. resolve_absolute_target was a fifth hand-rolled copy of the deepest-existing-ancestor walk; it now delegates to codewhale_core::walk_deepest_existing — the single implementation behind the write tool's own boundary — so the claim lane cannot drift from the tool lane. Tests: sibling contention, depth-3 chain admission, and the manifest-fallback leg of isolation inheritance (a claim-less worktree parent still confers isolation). Signed-off-by: asto <asto18089@users.noreply.github.com>
…a-boundary pins The binary-read refusal pointed read-only roles at unzip (not in their grammar) and 'PDF/document readers' (no such model-visible tool); it now names `file` as the read-only step and marks unzip as needing a role that permits it, and the sniff doc no longer implies every PDF is caught (an ASCII-headed one passes the NUL sniff). The image path's relative branch re-arms the RootDir component refusal (Windows rooted spellings) and drops its stale 'must be a relative path' wording now that in-workspace absolutes are admitted. The schema-budget test pins the 2048/2049 boundary and the no-top-level-properties reduction; the binary test pins the UTF-16BE BOM arm. Signed-off-by: asto <asto18089@users.noreply.github.com>
…ntation State the two behaviors the isolation work leaves true but previously undocumented: within one worktree unrelated descendants still contend (only the ancestor chain is exempt), and a resumed loop is only serialized against writers live at resume time, so it must re-orient to whatever completed while it was parked. Signed-off-by: asto <asto18089@users.noreply.github.com>
…esumed claims from the manifest Round-2 review of this PR (comment 5642624693) confirmed two gaps in its own claims. A cwd-narrowed child of a worktree parent keeps the parent's full root set, but the spawn seam keyed its claim's worktree_root on the child's own workspace — so two children of one worktree with different cwd subdirectories (or one narrowed and one not) registered disjoint keys and could race the same checkout unchecked, the fan-out same-worktree serialization exists to refuse. The key now resolves to the containing worktree: the nearest ancestor claim whose root contains the child's workspace supplies it; a child that created its own worktree (never nested) or has no isolated ancestor keys on its own workspace, and a resumed claim keeps its recorded root. A legacy None root predates serialization and names no worktree, so it keeps the documented never-contending footing instead of None == None matching every other None. An interrupted child's parked claim is no longer live, which makes it sweepable (coordinate action=release — the recovery the contention receipts recommend — removes every non-live owner) and evictable at ledger capacity. Resume restored the claim only by ledger lookup, so a swept row left the write-capable child claim-less with no way back: every mutation refused, and action=claim only expands an existing row. The launch manifest already carries the scope; the resume now rebuilds the claim from it when the row is gone (claim-less manifests stay claim-less), matching the manifest fallback the isolation flag already had. Also: the exemption-walk hop cap is a named constant documented against MAX_SPAWN_DEPTH_CEILING; the read-only shell refusal qualifies rev-parse (a flag subset is admitted, not the family) and the branch listing spellings; the anchor-only discovery test notes its tempdir-outside-a-checkout assumption. Red/green: cwd_narrowed_children_of_a_worktree_serialize_against_the_worktree (reverting the key to the child workspace reddens it at the sibling contention step), resumed_child_restores_claim_scope_from_its_manifest_ when_the_row_is_gone (removing the manifest fallback leaves the resumed child claim-less and reddens it), legacy_none_root_isolated_claims_ never_contend (restoring None == None reddens it). Signed-off-by: asto <asto18089@users.noreply.github.com>
…ng spellings Round-2 review (comment 5642624693) mutation-verified that the dash-prefixed-value guard in the branch parser could be reverted to the old unconditional swallow without reddening any test — only the --column switch shape was pinned. Pin the true value-flag path too (branch --list --sort -d x, worktree list --expire -v: a dash-prefixed value is refused, not swallowed uninspected), and likewise pin the parent parallel chunk surface for git rev-parse in both directions — the widened PARALLEL_READONLY_PREFIXES had left the stays-unchanged-for-parent-auto-approve comment stating an invariant the list no longer has. Bare and flag-only listing spellings (git branch, git branch -a, git branch -r -v) are real-git listings and were false-refused, exactly the retry-bait the refusal message warns against; without --list a positional still refuses, so git branch <name> stays dead. The refusal message now qualifies rev-parse (a bounded flag subset is admitted, not the family) and names the branch listing spellings. Signed-off-by: asto <asto18089@users.noreply.github.com> Signed-off-by: asto <asto18089@users.noreply.github.com>
Round-2 review (comment 5642624693) mutation-verified that the UTF-16 BOM arms of the binary sniff were dead weight for the tests: both UTF-16 fixtures carried NUL bytes, so deleting the BOM arms kept the suite green and a NUL-free UTF-16 CJK file would have regressed to mojibake undetected. The fixtures are now genuinely NUL-free, and the test pins the neighboring boundary cases (a UTF-8 BOM is text, a NUL exactly past the 8 KiB sniff window is outside the heuristic) plus the `file` pointer the message makes. The refusal no longer tells a UTF-16 text file to unzip a .pptx — the extractor advice is now conditional on the payload actually being a zip-based document. The reduced validation receipt carries the schema's declared type instead of a hardcoded "object", so a composition-only (oneOf-over-string) schema no longer describes a shape the tool never promised. Signed-off-by: asto <asto18089@users.noreply.github.com> Signed-off-by: asto <asto18089@users.noreply.github.com>
…hor isolated claim paths
Two review-round-3 majors in the same-worktree serialization this PR
introduced:
- The serialization key had no durable record. ChildLaunchManifest carried
only the `worktree` flag, never the root, so an interrupted cwd-narrowed
worktree child whose ledger row was swept (coordinate action=release, or
capacity eviction) and whose resume came from a non-ancestor caller
re-keyed its restored claim on its own subdirectory — the exact-equality
sibling predicate never fired and the resumed writer raced the checkout
unchecked. The same hole let a claim-less worktree parent's cwd-narrowed
children each key on their individual workspaces and never serialize
against each other. The spawn seam now computes the containing-worktree
root once for every isolated child (claim or claim-less), stamps it into
the launch manifest, and both the resume path and
containing_worktree_root fall back to that recorded root when the row is
gone. Legacy manifests (no field) keep the old behavior.
- Isolated claims were never re-anchored to the child's cwd. Shared
children get their mutation paths re-anchored through
coordination_workspace_prefix, but the isolated lane returned paths
unchanged, so a worktree child spawned with `cwd: crates/app` and
scoped write_roots ["crates/app"] had every natural in-scope spelling
refused ("main.rs" judged against the worktree frame) while the only
passing spelling wrote one directory deeper than intended. Validation
now maps each relative path into the worktree frame by lexically
prefixing the child workspace's offset from the claim's recorded root;
absolute spellings, legacy None roots, and undecidable drift are kept
as-is and still refused.
Pinned by swept_cwd_narrowed_worktree_child_resume_rekeys_on_its_manifest_
worktree_root (sweep through the real release path, root-caller resume,
manifest root asserted, fresh worktree writer refused), claim_less_worktree_
children_cwd_descendants_serialize_through_the_manifest_worktree_root
(manifest stamp + sibling contention between two narrowed descendants),
and cwd_narrowed_scoped_worktree_child_validates_paths_in_its_own_frame
(natural spelling admitted, traversal and absolute refused, non-covering
scope still refuses).
Signed-off-by: asto <asto18089@users.noreply.github.com>
…tings, pin the refusal guidance Round-3 review found the listing grammars refusing the most idiomatic read-only spellings — exactly the retry-loop fuel the refusal rewrite was meant to drain: `git branch --list --sort=-committerdate`, `--column=always`, `--color=never`, combined shorts like `-arv`, and the value-flag happy paths (`--format`, `--contains`, `worktree list --expire <time>`, `--verbose`) were all unpinned, so deleting them from the grammar stayed green. Both bespoke parsers now classify `--flag=value` tokens: the value is bound to its flag in the same token, so nothing is swallowed uninspected (the dash-value guard is unaffected). Branch also admits OPTARG-attached `--column=<style>`/`--color=<when>` per git's real grammar (the old comment claimed git "only reads" the attached form; it is an OPTARG whose separate-word form attaches nothing) and combined shorts whose every letter is a known read-only single; `git worktree list` gains `--expire=<time>` and `-vz` clusters. `--porcelain=v1` stays refused — git 2.54 takes no attached value there. The `[shell.readonly.command]` guidance now enumerates the full find refused set (-execdir/-ok/-okdir/-fprintf/-fls/-fprint were refused but unlisted) and names comm among the admitted filters, and the rewrite is pinned red/green by shell_readonly_refusal_enumerates_the_admitted_families — it was the one fix hunk whose reversion nothing in the suite detected. Signed-off-by: asto <asto18089@users.noreply.github.com>
…mutation-blind - bounded_valid_shape: the reduced receipt carries the schema's DECLARED type (pinned now — every fixture used to declare "object" or nothing, so hardcoding "object" again survived the suite), and its property inventory truncates at a cap with an explicit "... and N more" marker instead of being able to exceed the very budget it enforces. - The registry's mutation_paths call site is pinned end-to-end: an isolated worktree child writing an absolute in-workspace spelling through the real SubAgentToolRegistry only succeeds when the lane resolves against the child's own context workspace (the unit tests could not see the call site). - Comments say what the code does: worktree placement is guarded as "outside the parent checkout" (resolve_worktree_path never promised sibling-hood), expand_write_claim's exemption note is scoped to the isolated lane (the shared lane ignores exempt_owners), the discovery test states its real fixture depth and pins home's presence in the "Tried:" list so deepening the fixture cannot turn it vacuous, and the UTF-16 fixture bytes now actually decode to 你好 (they said 你水). - DRY: the parent-reference resolution shared by worktree_claim_exemptions and ensure_caller_controls_descendant is one named function (worker_parent_ref) instead of a third inline copy, and workspace_relative_mutation_path delegates through the crate's single resolve_deepest_existing wrapper like the write boundary does. Signed-off-by: asto <asto18089@users.noreply.github.com>
… message The CodeQL diff-scanner flagged a pre-existing assertion (shifted into the pull request's attribution window by ~1000 lines of earlier test additions) under rust/cleartext-logging: interpolating `client.base_url()` — resolved from the session's credential-bearing provider config — into the panic message is a logging sink for config-derived data. Pin the endpoint with a static message instead; the assertion itself is unchanged. Signed-off-by: asto <asto18089@users.noreply.github.com>
Round-4 review (comment 6461629607) findings, fixed: - fix(shell) MAJOR: hardened_readonly_argv now executes the git orientation probes it classifier-admits (rev-parse, branch listing forms, worktree list). The whitelist kept refusing every bare spelling at the execution seam; only piped shapes ran. Pinned by readonly_argv_executes_the_git_orientation_probes_without_a_shell, which fails closed for a mutating subcommand. - fix(claim-lane) MAJOR: normalize_claim_path no longer trims, and workspace_relative_mutation_path judges the raw tool string, so the claim lane validates exactly the path the write tool resolves. The trim let a whitespace-padded absolute validate as its trimmed target while the tool materialized <ws>/␣/…; a whitespace-only root also normalized to the whole-workspace '.'. The internal '' -> '.' identity (coordination_workspace_prefix) is kept; whitespace-only spellings refuse. Pinned by padded-path assertions in scoped_mutation_paths_accept_absolute_targets_inside_the_workspace. - fix(tests): the grandchild-isolation and sibling pins now spawn the worktree parent live, so ancestor-exemption removal turns the grandchild admission into a contention refusal (the round-4 mutation lab showed the pin surviving that mutation). - fix(subagent): lexically_under joins both separators, so a Windows-spelled offset lands in the forward-slash ledger frame instead of failing closed; worker_ancestry resolves parents through worker_parent_ref like the other two walks. - fix(tools): bounded_valid_shape byte-bounds the reduced inventory (long names skip into the omitted tail) and omits 'type' for schemas that declare none instead of asserting 'object'; the binary-read refusal tells shell-less roles to delegate and the sniff doc no longer claims a separate UTF-32 arm. - fix(ledger): worktree_root omits None from the persisted shape (skip_serializing_if) with a round-trip pin across the field introduction; the engine fixture comment and the ~4.5KB figure are corrected. - fix(vision): absolute image paths get the same lexical traversal guard as relative ones, so a non-existent escaping target refuses with the containment message instead of a resolve error. - fix(shell classifier): the branch listing grammar's rustdoc moves to the branch parser; attached_readonly_value returns bool (call sites only tested is_some); mutation_paths normalizes the workspace once per batch. - docs(SUBAGENTS) en+zh: containing-worktree keying, the claim-less manifest leg, the legacy never-contending footing, the one-directional exemption, and honest resumed-loop re-orientation wording. Signed-off-by: asto <asto18089@users.noreply.github.com>
…nd ledger frame Round-5 review findings (reviews 5687369981, 5692483474), on a rebase onto pinvou3-clean@f1eb10efc whose combination tree equals the one both reviewers computed (d517d3e). - fix(vision): absolute image paths no longer refuse Component::Prefix. A Windows drive or UNC absolute spelling necessarily carries a prefix, so the lexical guard rejected every absolute path before the canonicalized containment check could validate it. The absolute branch now refuses only ParentDir (traversal) and leaves prefix/root shape to the containment check, mirroring normalize_path_components. Pinned by a cfg(windows) test covering drive-absolute and verbatim spellings plus the outside-workspace negative; the non-gated absolute test is Windows-correct again. - fix(subagent) MAJOR: the unbounded-shell contention gate now covers the isolated lane. Same-worktree admission deliberately admits several unrelated writers on disjoint claims, and an unbounded command cannot prove it stays inside the owner's share, so a live peer keyed on the SAME worktree root blocks it, with the refusal naming that peer and the worktree remediation. The exemptions mirror admission: the owner's ancestor chain is exempt (delegation keeps working), a peer on a different worktree never shared the checkout, legacy None-root rows keep the never-contending footing, and liveness is the same predicate both gates consult. One consequence, mirroring the shared lane's existing shape: a parent's own unbounded shell is gated by the live same-worktree descendants it fanned out, while their commands stay exempt against it. Pinned red/green by same_worktree_disjoint_sibling_blocks_unbounded_shell, same_worktree_ancestor_chain_keeps_unbounded_shell, and same_worktree_gate_ignores_settled_and_foreign_worktree_peers. - fix(tests): the padded-path pin's expected is built in the ledger's canonical frame independently of the code under test - backslashes fold to forward slashes and leading separators collapse (a Windows verbatim prefix reduces to its literal ? component), so the native-Windows assertion compares equal while the leading-space semantics stay pinned. docs(SUBAGENTS) en+zh state the shell gate's extension to the same-worktree lane. Signed-off-by: asto18089 <asto18089@126.com>
|
Responding to review 5709512014 (and #pullrequestreview-5709398775 with the same two findings). Both P2s are fixed and pushed ( P2-1: outer tool cancellation leaves an unstamped reservation live → Fix: Tests: P2-2: the standalone Workflow builder omits the shell source → Fix: Tests: Validation: Disclosed limits: this environment is macOS arm64; no 30-minute real-product timeout run and no two-agent OS overwrite drill were executed — the tests model the cancellation with a 1s inner deadline and real child processes. |
JensenChen28
left a comment
There was a problem hiding this comment.
当前固定 head 71cc262 的 headless workflow shell-job-source 绑定已从源码闭合;但原 outer-tool-timeout 空 reservation 回收 P2 仍只部分修复,保留 CHANGES_REQUESTED。
同一个共享 coordination manager 存在三个取消窗口:execute_full 在发布 token 后、构造 guard 前还会 await read;settle 在 await write 前先设置 settled=true;普通 Drop 的 try_write 遇合法并行 reader/writer 占锁即放弃,未留下可重试回收标记。外层 timeout 恰落这些等待/竞争点时,Running owner 的空 reservation 可继续被 prune/consult 当成有效占用,直到 owner 终态才可回收。
请让已取消操作的清理可在锁竞争后重试或延后完成,并只在实际完成后确认 settled;保持真实活 job 围栏,勿直接删除仍有活工作的占用。两轴核了实际 Arc 共享和调用链,不是拿另一把锁假设竞争;新增测试仍未覆盖这些争锁取消场景。本轮未运行 Rust,不声称执行了取消竞争反例。
JensenChen28
left a comment
There was a problem hiding this comment.
Follow-up at 71cc262; both author responses and the exact increment were reviewed independently.
Standards
The existing cancellation-lifecycle P2 remains partially unresolved. A reservation opens at tools/subagent/mod.rs:16705-16708, but InFlightDispatchGuard is installed only at :16774, after the awaited manager read at :16716 (and the refusal-path write at :16770). Cancellation at those waits still leaves an unowned token. At the other boundary, settle sets settled=true at :3354 before manager.write().await at :3355; cancellation while waiting makes Drop skip reconciliation even though completion has not occurred.
Spec
This is the same original timeout finding, not an additional duplicate. Cancellation during the guarded registry call is improved, but a contended Drop only tries the lock once and schedules no eventual reconciliation. Contention is reachable: parallel checkpoint_subagent_progress takes the same manager write lock at :11882 and performs checkpoint/persistence preparation under it. Dropping between polls does not make that shared lock uncontended on another runtime worker. After the lock holder releases and the shell ends, a still-Running owner can therefore keep the unstamped frame blocking successors.
Own the reservation immediately on admission, retain reconciliation responsibility until stamping actually completes, and ensure eventual shell-aware reconciliation after a contended cancellation. Preserve live/unconfirmed jobs. Add cancellation tests at admission/settle lock waits and under Drop contention; the added tests cover the middle dispatch window instead.
The separate standalone Workflow shell-source finding is resolved in source: the real builder binds the context's shell manager, and the constructor identity/lifecycle tests target that path.
Validation: exact source/call-chain and regression-boundary inspection; diff whitespace validation passed. Cargo/native concurrency/product timeout runs were not executed locally, and author test counts were not adopted as local evidence.
Standards: 1 existing P2 partially unresolved. Spec: the same 1 existing P2 partially unresolved; standalone Workflow finding resolved.
zhuowp
left a comment
There was a problem hiding this comment.
固定审核 head 71cc26218f568c8aec718aed62ff9d5c6847ba5c,tree 58949b6725e45ed80571801c3157b45f2fbbe8fa;实际 target/merge-base pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c;最新个人审核请求 34891166935。请求修改,确认两项P2:
- 取消清理在共享锁竞争下仍会遗留占用。
crates/tui/src/tools/subagent/mod.rs:3353–3373,16705–16779:settle在await写锁之前设置settled=true,外层工具timeout此时丢弃future后,Drop会跳过尚未完成的清理。token发布到guard安装之间也还有await;普通Drop争锁只尝试一次,没有延后回收。共享manager可被其他任务的checkpoint等持锁,“两次poll之间丢弃”不意味着其他runtime worker不持锁。timeout只生成工具错误并继续Running,空stamp仍由owner活性保留,prune也不会移除,故shell已经结束后同frame后继claim/无界调用仍可能被拒直到owner终态。这是原取消生命周期P2的残余,与两条同head外审同根,不重复计数。请从准入开始持有清理责任,实际完成后才确认settled,并保证争锁取消后最终按shell truth回收,同时保留真实活job/未知停止围栏。原guard的受控锁探针及负控支持取消窗口;未声称跑过完整产品30分钟竞争或测得现场发生频率。 - 新增test helper的cfg不匹配使Windows lib-test编译失败。
crates/tui/src/tools/shell.rs:2073–2074的mark_stop_unconfirmed_for_test与crates/tui/src/tools/workflow/mod.rs:130–131的coordination_manager_for_test仅cfg(test),唯一调用却分别位于cfg(unix)测试(crates/tui/src/tools/subagent/tests.rs:25465/25532,26041/26088)。Windows中二者无消费者,TUI继承workspace的warnings=deny。原生Windows固定源执行cargo test -p codewhale-tui --lib --locked --offline tools::subagent::tests::interrupted_child得到exit101,两条dead_code错误,实际执行0个测试。两处是同一条件编译根因;请与Unix调用者对齐cfg或提供有效Windows消费者,保留lint并重跑。此项不同于已经修复的Windows缺字段E0063;不主张产品binary因此不能构建。
独立五角色FIRST冻结后由非实现者裁决;累计22文件/1181个raw U0片段的源码覆盖已验收,包括大测试文件的原生TokenTree整item等价、普通注释/模块环境核对和49个新增/改义item的oracle审查。源码覆盖闭合不等于阻断已修。新请求与首报的head/tree/target、依赖/调用者/配置/夹具和正文一致,接受同源证据继承,未冒称在新请求后重跑五份首审。当前18份review、12条discussion均已重新处置;独立Workflow真实shell source接线、旧retention/Windows字段/NFD等修复保持源码闭合,旧批准不抵消以上问题。
两项均直接关联本PR生命周期/回归测试,不能同时满足“严重度低、正常使用不受影响、主题低关联”三条件,不列非阻断。
四类结论:代码审核请求修改;功能验证部分完成,局部helper/guard探针与真实crate分开,Windows真实lib-test失败,Unix进程链、provider/PTY及完整产品旅程未获本轮通过证据;合并资格未满足/未建立,当前固定SHA checks/statuses为空,静态CI配置不代表执行,仍须满足适用CI、外审与实际门禁;发布资格未评估且未授权。未改产品、合并、入队、发布、部署或启动CI。
Round-10 review findings (reviews 5733598466, 5733899413, 5734379754) at 71cc262; base pinvou3-clean unchanged at f1eb10e. The in-flight dispatch guard kept three cancellation windows in which an abandoned dispatch left its reservation unstamped and held up by the owner's Running status alone, blocking same-frame successors until the owner went terminal: - Admission: the guard was constructed only after the peer-check awaits, so a cancellation parked between opening the token and constructing the guard stranded the entry with no reconciler at all. The guard now takes ownership the instant the token exists, and the refusal path releases through guard.cancel(), which counts as done only after the release ran under the lock. - Settle: settled was flagged before the completion leg's write().await, so a cancellation during that wait made Drop skip the reconciliation even though completion never ran. The flag now flips only after the completion leg ran. - Contended Drop: the drop's single try_write gave up when the lock was legitimately held across another task's await (checkpoint preparation under the same manager), leaving the entry stranded. The reclaim is now handed to a detached task that queues on the lock and runs the same shell-aware reconciliation once the holder releases; without a runtime to spawn on, the conservative owner-liveness answer is kept. The reclaim stays keyed on the token (no-op against a completed entry) and keeps stamping genuinely live work instead of clearing it, so the live-job and unconfirmed-stop fences are unchanged. New pins: settle abandoned at the lock wait still reconciles; contended drop eventually stamps a live job and opens a finished frame; refusal release abandoned at the lock wait reopens the frame. All three were verified red under targeted mutations (settled-before-write, spawn disabled, cancel-before-release) and green after restore. Signed-off-by: asto <asto18089@126.com>
The second round-10 review P2: mark_stop_unconfirmed_for_test and coordination_manager_for_test were #[cfg(test)]-only helpers whose only callers sit inside #[cfg(unix)] tests, so the Windows lib-test target denies them as dead_code (warnings=deny) and fails to compile. Gate both with #[cfg(all(test, unix))] the way the neighboring background_child_pid_for_test already is; workflow's shell_job_source_for_test keeps #[cfg(test)] because a portable lib.rs pin consumes it. Signed-off-by: asto <asto18089@126.com>
|
修复波已推(2 提交 → P2「取消清理在共享锁竞争下仍会遗留占用」— 已修(
token 键控不变(对已完成条目是 no-op),live job / unconfirmed-stop 围栏全部保留。新增三钉:settle 在锁等待处被取消仍对账;争锁 drop 最终给活 job 盖章、给已结束 frame 开门;refusal 释放在锁等待处被取消仍重开 frame。三钉在定向突变下(settled 先置/禁用 spawn/cancel 先置)逐一转红,还原后逐字节恢复转绿。 P2「test helper 的 cfg 不匹配使 Windows lib-test 编译失败」— 已修(第二提交)。 门禁(本机 rustc/cargo 1.99.1): 仅修复与钉测试,未合并、未入队。请在 |
JensenChen28
left a comment
There was a problem hiding this comment.
固定 head 39494700841317a7d98dca78119c5f209ac8c0de;最新目标 pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c。完整复核相对已审 71cc 的四文件增量、关联调用链及作者逐项回复,未改动的累计范围继承此前固定源码审核。
Standards
两项原 P2 已从源码闭合:guard 在 token 准入后、下一 await 前接管;settle/cancel 实际完成后才置 settled;争锁 Drop 排队执行原 shell-aware reclaim。两个仅 Unix 测试消费的 helper 改为 all(test, unix),保留 warnings=deny 及可移植 getter。未发现本增量新的阻断问题。
Spec
取消、拒绝、锁等待及争锁回收仍按 token 和真实 shell 状态处理;活 job 与 unconfirmed-stop 围栏保留,已消失 token 为 no-op。原取消清理与 Windows lib-test cfg 两项 P2 关闭。无 runtime/运行时已停止时仍保守占用,延后回收依赖活跃 runtime 与锁最终释放。
非阻断 P3:新增测试用 sleep 协调锁持有,settle/refusal 断言也未显式等待 detached 回收完成;建议增加锁就绪 acknowledgement 和有界最终状态等待,提升调度稳健性。这不是本轮实测失败。
实际执行:目标 head/base 分支 fetch 成功;累计 git diff --check 返回0;新两提交 Signed-off-by 已核。两轴执行固定源码/cfg/调用者/占用链检查。未运行 Cargo、原生 Windows、进程竞争、浏览器或完整产品超时测试,未采用作者测试计数作为本人结果。
批准此固定 head;其他审核者与适用 CI 仍独立处理。本轮未合并、入队、开启 auto-merge 或重跑 CI。
JensenChen28
left a comment
There was a problem hiding this comment.
Reviewed 3949470 against pinvou3-clean at f1eb10e, including the full four-file increment from 71cc262 and all new author/peer discussion.
Standards
No blocking finding remains. The original cancellation P2 is resolved: the guard owns its token before the next admission await; settle/cancel mark completion only after the guarded mutation; contended Drop queues the same token-keyed, shell-aware reconciliation. Both Unix-only test helpers now match their Unix consumers without weakening the warnings policy or the portable getter.
Nonblocking P3: the new tests coordinate lock acquisition with sleeps, and the settle/refusal assertions do not explicitly wait for detached reconciliation to finish. Prefer readiness acknowledgements and a bounded eventual-state assertion. This is a scheduling-robustness observation, not a reproduced test failure.
Spec
The three cancellation windows are source-closed. Live and unconfirmed shell jobs retain their fence: even a terminal owner with an empty stamp is checked against owner_has_running_job before consult/prune can release it, while deferred reclamation waits. Completed/missing tokens remain no-ops, and the standalone Workflow source binding stays resolved. Deferred cleanup assumes an active runtime and eventual lock release; the documented conservative fallback outside a runtime remains.
Validation: independent two-axis plus root source/caller/cfg/test-oracle inspection, full diff whitespace validation, and both new commits' DCO trailers checked. Cargo/native Rust, native Windows lib-tests, provider/PTY and full-product timeout journeys were not executed locally. Author test/mutation totals and peer's prior native result are not claimed as my execution.
Standards: 0 blockers, 1 nonblocking P3. Spec: 0 blockers. Approved.
The three cancellation-window tests coordinated the lock holder with a bare sleep and asserted the detached reconciliation immediately, which left two scheduling holes the review flagged as non-blocking: - A loaded runtime could reach the abandonment window before the holder task actually acquired the write lock, letting settle/cancel complete uncontended and failing the is_err assertion. The holder now acknowledges the acquisition over a oneshot, awaited with a bounded timeout, so the guard is provably parked on the lock when the deadline fires. - The settle and refusal tests asserted the reconciled frame right after the holder released, assuming the detached reclaim task had already run. Both final assertions now poll in a bounded loop, matching the idiom the drop-contention test already uses. Test-only; no production change. Targeted: the three tests 3/3, the in-flight/cancelled-dispatch/timeout family 12/12, fmt clean, clippy diagnostic set byte-identical to the base head. Signed-off-by: asto <asto18089@126.com>
P3 follow-up wave pushed (
|
JensenChen28
left a comment
There was a problem hiding this comment.
批准固定 head 6c40e3bb21f0a45f98fef3480af3ed567ece13a8;最新目标/merge-base pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c。完整复核相对已审3949470084的一文件增量(61+/16−)及当前作者回复,未改累计范围继承此前固定源码审核。
Standards
无新增阻断。只改subagent/tests.rs;生产取消回收与Unix helper cfg逐对象保持不变,原两项P2仍源码闭合。oneshot确认holder已取得锁,两处有界最终轮询不再假定detached回收立即完成;live-job盖章与dead-frame释放断言保留。
Spec
测试P3是部分改善:最终状态等待已修,但不能称锁竞争完全确定。crates/tui/src/tools/subagent/tests.rs:25883、25944、25989、26050的holder仍在ack后独立sleep150ms便放锁;若接收者恢复或30ms取消future被调度拖过该窗口,settle/cancel仍可正常完成而假失败,contended Drop也可能走同步fast path而未覆盖deferred分支。建议用第二个release握手,在取消/drop实际发生后才释放holder;保留有界超时。此项仍为非阻断P3,未声称本轮复现失败,未把它当生产回收缺陷。
实际执行:目标head/base fetch成功、累计git diff --check为0、新提交Signed-off-by核对;两轴检查真实增量及完整三个函数的oracle。未运行Cargo、编译/原生测试或负控,作者3/3和12/12不作本人结果。其他审核者及门禁独立处理;未改PR代码、触发CI、合并、入队或启用auto-merge。
zhuowp
left a comment
There was a problem hiding this comment.
固定审核 head 6c40e3bb21f0a45f98fef3480af3ed567ece13a8,tree 2a19c7937b513227fdeec725eec39e834b1e798a;实际target/base/merge-base pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c,个人请求 34897609474。批准此固定版本的累计代码审核。
此前两项P2已独立复核关闭:取消guard在准入后下一await前接管,settle/cancel实际清理后才标记完成,争锁Drop延后执行相同token的shell-aware回收;真实活job及未确认停止的围栏保留。两个仅Unix消费的helper现在与消费者cfg一致,warnings=deny未放宽。
五份独立原首报、版本补报及受影响C/E复查冻结后,由未参与实现者裁决。累计22文件/1107原始片段的实际静态覆盖及完整链验收闭合;旧范围按精确item/range和当前caller/依赖/配置适用性继承,不把补报冒称新增盲审,也未按批准数投票。
本轮当前源完整Windows locked/offline lib-test编译exit0(--no-run执行0测试);随后8个精确选例先验证唯一匹配,再逐例实际各1 passed; 0 failed。包括三取消锁竞争、interrupt释放、worktree继承与Unicode/分隔符路径。每次精确运行各显示12052项被过滤;本轮共执行上述8个不同选例,其余选例未执行。旧受控guard负控仅为机制证据,含manager/shell替身,不与这8例叠加业务数量;作者测试数字未作为本轮执行。两次磁盘不足构建记录保留为环境失败。
保留非阻断P3:oneshot和最终轮询已改善测试,但holder仍150ms自行释放,取消与deferred分支受调度影响;release握手可进一步加强。此为测试健壮性限制,未复现产品功能失败,未套用低关联豁免。
四类结论:代码审核通过,原两P2关闭,无新增确认阻断;功能验证部分完成,Unix真实进程/停止回收、完整provider/CLI/PTY、真实Git worktree及重启旅程未在本轮执行,Windows绿灯不替代这些环境;合并资格未建立,截至冻结的精确SHA快照checks/statuses均空,适用CI、其他外审和实际保护门禁仍须满足;发布资格未评估且未授权。没有产品修改、触发CI、合并、入队或发布。
…gs, honest fleet catalog, and the ordering pins The round-17 review's deferred minors, triaged: the valuable ones land here; the conflict-surface churn items stay deferred with reasons in the body. 1. The classifier-bounded probe lane admits two shapes real tools accept that the table refused: POSIX getopt clusters for wc's all-switch row (wc -cl is exactly wc -c -l; a letter that is not an admitted switch, wc -cx / wc -c5, still refuses exactly as the tool itself rejects it) and GNU grep's numeric context shorthand (grep -3 pat f is exactly grep -C 3 pat f, rewritten to the exact-match form in the option context only - past -- a -3 is a file name and stays an operand; rg does not accept the spelling and its row does not gain it). Both ride the shared row table, so the parallel auto-approve classifier's admitted set grows by exactly these spellings - the delta is disclosed in the body and pinned on both classifiers. 2. The evidence catalog no longer advertises the bash lane to a worker whose envelope grants no shell authority: enforce_tool_authority refuses every bash call for shell != ReadOnly, so the fleet-durable verifier was shown a surface it can never use. A read-only-shell envelope keeps the lane. Pinned both directions. 3. The network-guard's before-spawn ordering now has its tool-entry leg: under a Deny policy an admitted gh read refuses with the policy error inside execute (no child runs), a non-gh read runs untouched. The gh shape parses --limit client-side, so even the dropped-guard mutant path fails at argument parsing and can never reach the network. 4. The round-16 git-stage-glob E2E now drives BOTH call sites at tool level (unpiped and piped-with-wc), so neither site can regress alone behind the unit pins. 5. The bounded stdin-write tests' upper guards widen from 3x to 6x the deadline (15 s flaked once under a heavily loaded host; the 30 s -> 60 s outer hang guards keep broken-mechanism mutants red). Deferred with reasons (round-17 minors): the test-suite dedup and the legacy cfg-gate reason comments (churn in the #123 conflict surface), the discovery double-parse and per-entry star-match allocation (bounded economy below the noise floor), head --lines=3 (the no-equals grammar is a documented deliberate shape), the empty-named filter wall (exotic, fail-closed), the chain pin through the real spawn seam (the threaded runtime is unobservable without a real grandchild spawn - follow-up), and the 19 long commit subjects (no gate exists; the parent squash collapses them). Gates at this head: tools::shell:: 252/0/1-ignored, command_safety:: 98/0, tools::subagent:: 609/0, tools::registry:: 49/0, tools::verifier:: 9/0, forkguard_ 240/0; fmt clean; fork-CI clippy clean under its exact flags. Mutations: each new wall neutered reddens exactly its own pins. Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
…gs, honest fleet catalog, and the ordering pins The round-17 review's deferred minors, triaged: the valuable ones land here; the conflict-surface churn items stay deferred with reasons in the body. 1. The classifier-bounded probe lane admits two shapes real tools accept that the table refused: POSIX getopt clusters for wc's all-switch row (wc -cl is exactly wc -c -l; a letter that is not an admitted switch, wc -cx / wc -c5, still refuses exactly as the tool itself rejects it) and GNU grep's numeric context shorthand (grep -3 pat f is exactly grep -C 3 pat f, rewritten to the exact-match form in the option context only - past -- a -3 is a file name and stays an operand; rg does not accept the spelling and its row does not gain it). Both ride the shared row table, so the parallel auto-approve classifier's admitted set grows by exactly these spellings - the delta is disclosed in the body and pinned on both classifiers. 2. The evidence catalog no longer advertises the bash lane to a worker whose envelope grants no shell authority: enforce_tool_authority refuses every bash call for shell != ReadOnly, so the fleet-durable verifier was shown a surface it can never use. A read-only-shell envelope keeps the lane. Pinned both directions. 3. The network-guard's before-spawn ordering now has its tool-entry leg: under a Deny policy an admitted gh read refuses with the policy error inside execute (no child runs), a non-gh read runs untouched. The gh shape parses --limit client-side, so even the dropped-guard mutant path fails at argument parsing and can never reach the network. 4. The round-16 git-stage-glob E2E now drives BOTH call sites at tool level (unpiped and piped-with-wc), so neither site can regress alone behind the unit pins. 5. The bounded stdin-write tests' upper guards widen from 3x to 6x the deadline (15 s flaked once under a heavily loaded host; the 30 s -> 60 s outer hang guards keep broken-mechanism mutants red). Deferred with reasons (round-17 minors): the test-suite dedup and the legacy cfg-gate reason comments (churn in the #123 conflict surface), the discovery double-parse and per-entry star-match allocation (bounded economy below the noise floor), head --lines=3 (the no-equals grammar is a documented deliberate shape), the empty-named filter wall (exotic, fail-closed), the chain pin through the real spawn seam (the threaded runtime is unobservable without a real grandchild spawn - follow-up), and the 19 long commit subjects (no gate exists; the parent squash collapses them). Gates at this head: tools::shell:: 252/0/1-ignored, command_safety:: 98/0, tools::subagent:: 609/0, tools::registry:: 49/0, tools::verifier:: 9/0, forkguard_ 240/0; fmt clean; fork-CI clippy clean under its exact flags. Mutations: each new wall neutered reddens exactly its own pins. Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Round-20 review wave on the compound read-only probe slice: - the fixed spawn-env overrides now clear BASHOPTS/SHELLOPTS: bash honors both at startup from the inherited process environment, so an operator-exported BASHOPTS=dotglob would widen * over the dotfiles the GLOBIGNORE wall keeps cold (same class as the round-6 ZDOTDIR and GLOBIGNORE pins); the table pin now covers six fixed keys - the three mode-000 root-runner skip notes route through one shared helper with a single targeted clippy::print_stderr exemption, keeping the crate-level deny lint clean under --all-targets clippy - rebase repair: a duplicated bounded-stdin doc comment block landed above the cd-operand test in the #123/#124 rebase; the copy is gone and the separating blank line restored Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Round-20 review wave on the compound read-only probe slice: - the fixed spawn-env overrides now clear BASHOPTS/SHELLOPTS: bash honors both at startup from the inherited process environment, so an operator-exported BASHOPTS=dotglob would widen * over the dotfiles the GLOBIGNORE wall keeps cold (same class as the round-6 ZDOTDIR and GLOBIGNORE pins); the table pin now covers six fixed keys - the three mode-000 root-runner skip notes route through one shared helper with a single targeted clippy::print_stderr exemption, keeping the crate-level deny lint clean under --all-targets clippy - rebase repair: a duplicated bounded-stdin doc comment block landed above the cd-operand test in the #123/#124 rebase; the copy is gone and the separating blank line restored Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
…nd close round-21 review findings The round-20 rebase of this branch onto the #123/#124-merged main resolved subagent/tests.rs by dropping six merged #123/#124 test fns and the `mod identity_selection;` declaration, silently orphaning the 11 forkguard_agent_identity_* tests in the still-present tests/identity_selection.rs (26 KB, byte-identical, no longer compiled). The dropped fns exercise surfaces this slice does not touch (same-worktree peer gating, ancestor walk-up non-adoption, claim-path normalization, scoped mutation paths) and all 18 tests pass restored, so the deletion was pure coverage loss, not a forced adaptation. This restores them union-keep-both — the resolution the body already documents for this file. Also: pin the "Do not retry variations" guidance fragment in the replacement enumeration test (the one fragment the deleted shell_readonly_refusal_enumerates_the_admitted_families pin carried that the replacement dropped), and correct the readonly_shell_bound_command doc comment, which said the pipefail prefix keys on "a pipeline" while the predicate is a plain `contains('|')` (an || sequence matches and receives the harmless no-op prefix). Receipts at this head: readonly_ 150/0, command_safety 101/0, forkguard 285/0 (--test-threads=2), tools::subagent:: 648/0, rustfmt clean on both touched files. Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
Summary
No-Issue: issues are disabled on this repository; the defects below were reproduced from a live multi-agent test session transcript (2026-10-24, parent and children on
deepseek/deepseek-flash, runtime 0.9.12) and traced to the code. Each code fix carries a red/green test; the F1 fix is mutation-verified.This is topic-disjoint from #119 (session-name release,
waittimeout_secs, contention guidance text): #119 explains who contends and how to recover; this PR fixes the ledger and path rules that made two of those recoveries impossible. Six files are shared and every hunk range is disjoint, so whichever PR merges second rebases into the other's context.1. An interrupted child kept its write claim live forever (session-level write lockout)
A
generalchild (default claim["."]) was interrupted by its parent. Afterwards every write-capable spawn in the session was refused — including claims narrowed to a subdirectory, since.covers everything — and so was the child's own resume:Reproduced six times over ten minutes;
cancelon the interrupted child returned its status unchanged (cancel only touches Running children). Root cause: an interrupted child's worker record parks inWaitingForUser(its checkpoint is continuable), andis_live_coordination_ownercounted a paired parked record as a live writer — on the premise that "the user can still answer and the child will write again". But a resume always re-dispatches under a new id (resume_from_checkpoint) and registers that id's own claim, so the parked claim can never be exercised, and the new id's claim contended with its own predecessor. The one test that pinned the premise (waiting_for_user_headless_worker_is_not_a_live_coordination_owner, paired half) asserted the self-defeating behavior.Fix: a paired record follows its agent — only a
Runningagent from this boot is a live claimant. The interactive case the old comment protected is the approval prompt, where the agent is stillRunningwith aWaitingForUserrecord; that stays live (now pinned explicitly). Headless records keep the existing rule.Tests:
interrupted_child_stops_gating_later_writers(production spawn → interrupt with a continuable checkpoint → a narrowed later writer is admitted; the parked claim stays on the ledger as inactive history),interrupted_child_resume_does_not_contend_with_its_own_parked_claim(resume registers the restored claim under the new id), and the corrected paired half of the headless test. Mutation: restoring the old paired-parked allowance reddens exactly these three;write_capable_grandchild_of_a_worktree_child_inherits_isolationstays green.2. A worktree child could not spawn a write-capable child
From inside an isolated worktree,
agent(action=start, type=general)was refused:A read-only
exploregrandchild from the same worktree worked, which pins the refusal to claim registration. Worktrees are created as siblings of the checkout (resolve_worktree_pathforbids nesting), socoordination_workspace_prefixcan never strip the coordination root from a worktree workspace — yet the grandchild, having noworktree/cwdrequest of its own, is already de facto confined to that worktree (prepare_child_workspacereturns no override; acwdmust sit under the parent workspace). Only the ledger disagreed.Fix: the spawn seam inherits the parent's isolation —
parent_runs_in_isolated_worktreeuses the resume path's precedence (parent's registered claim flag, then its durable launch manifest, which also covers a claim-less worktree parent:claim_less_worktree_child_still_confers_isolation_through_its_manifest) — and applies it to the process-lock skip,namespace_write_claim,register_claimand the launch manifest'sworktreeflag. The persisted-claim prompt now saysworktree-relativefor isolated claims (it saidcoordination-root-relativefor every worktree child, which was already inaccurate). A shared parent confers nothing: an out-of-root workspace under a shared parent is still refused (pinned in the same test).Review round 1 tightened the ledger semantics this exposed: "isolated never contends" was premised on one isolated claim per worktree, so a worktree child could have fanned out overlapping writers into one checkout unchecked. Isolated claims now carry the worktree root they are confined to — resolved to the containing worktree through the ancestor chain, so a
cwd-narrowed child serializes against its worktree rather than its subdirectory; claims registered against the same root contend unless the existing owner is an ancestor of the new one (the work was delegated downward), so parallel siblings refuse with the usual contention receipt while parent → child → grandchild chains stay admitted.worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_notpins both sides; the rule is documented indocs/SUBAGENTS.md(both languages).Test:
write_capable_grandchild_of_a_worktree_child_inherits_isolation.3. Scoped writes refused the absolute spelling of an in-workspace path
A child with
write_roots ["fixture/calc"]wrote<workspace>/fixture/calc/inside.txtand gotwrite scope path must be repo-relative without traversal: <that path>; the relative spelling succeeded.mutation_pathsfed the raw tool path straight intonormalize_claim_path, which refuses every absolute path. Now an absolute path that resolves inside the child's workspace is stripped to its relative form first — viacodewhale_core::walk_deepest_existing, the single deepest-existing implementation the write tool's own boundary uses, so the claim lane cannot drift from it (canonicalizing the deepest existing ancestor, since a write may be creating the file) — then validated as before; absolute paths outside the workspace and..traversal stay refused.image_analyzehad the same rule — thecomputer_usescreenshot receipt hands the model an absolute path, and the firstimage_analyzecall with it was refused — and now resolves absolute spellings through its existing workspace-containment check.Tests:
scoped_mutation_paths_accept_absolute_targets_inside_the_workspace,resolve_image_path_accepts_absolute_paths_inside_the_workspace(the/etc/hostspin keeps refusing, asserting on the containment message).4. Worktree discovery scanned
$HOMEfor repositoriesworktree=trueon a session workspace that is not a repository failed with:git_repo_rootwalked up to four ancestors and scanned each one's children for checkouts — leaking unrelated project names, and with exactly one sibling it would have silently adopted a stranger's repository. The child-scan now happens only at the anchor (the harness and ambiguity-at-anchor tests are untouched); ancestors are probed for containment only, so the non-repository case reaches the existing "requires a git repository. Tried: …" error.Test:
worktree_discovery_never_adopts_or_enumerates_an_ancestors_sibling_repos(two siblings → no enumeration; one sibling → no adoption).5. Read-only shell: git orientation probes and a refusal that caused a 13-step retry loop
Read-only roles were refused
git rev-parse --show-toplevel,git branch --show-currentandgit worktree listwhilegit status/git logpassed — exactly the probes a worktree child needs to orient itself.rev-parsejoins the plain subcommand table (read-only in every form; also added toCOMMAND_ARITYandPARALLEL_READONLY_PREFIXES, so the parent's parallel chunks accept the same bounded flags).branchandworktreetake positional operands that create things, so they are special-cased to their listing forms only:branch --show-currentalone or--list/-l(patterns become read-only), andworktree listwith no operands. Mutating forms (branch dev,branch -d,branch -D,--edit-description,worktree add/remove/prune/lock/move,rev-parse --parseopt) are pinned refused, as are value-flag swallows (branch --list --column -d x: git's--columnis attached-only, so the checker refuses a dash-prefixed value instead of swallowing it uninspected).rev-parse --git-pathis read-only but refused by the same flag bounding that keepsrev-parseto its read-only option table.The
[shell.readonly.command]refusal said only "did not match the bounded read-only shell grammar"; a reviewer child retried variations 13 times (~141k input tokens) before the parent intervened. It now enumerates the admitted families and the refused shapes and tells the model not to retry variations. The tag the existing test pins is unchanged.Tests: both admit/reject tables in
command_safety.rsextended.6. Two receipt-quality defects from the same transcript
readon a.pptxreturned 50KB of replacement characters with no warning (the contract reader decodes lossily). It now refuses binary payloads — a NUL in the first 8KB, or a leading UTF-16/UTF-32 BOM — with a pointer to an extractor; Latin-1 text with a loneÿand UTF-8 with a truncated sequence keep the lossy tolerance. Test:contract_read_refuses_binary_payloads_but_keeps_lossy_text.invalid_inputreceipt echoed the tool's full input schema asvalid_shape; foragentthat is ~4.5KB appended to each semantic refusal (agent not found, contention, unknown profile — eight times in this session). Schemas over 2048 bytes are reduced to their field inventory (propertiesnames +required) with a pointer back to the advertised schema; small schemas stay verbatim and the existing"required":["path"]pin holds. Test:tool_validation_feedback_bounds_oversized_schemas_to_their_field_inventory.Checked and left alone
detached=truechild cancelled between turns. CodeWhale's turn-end paths are detached-aware (detached children takebackground_runtime()and never enter the foreground registry;waiting_for_subagentswas emitted correctly). The cancellation came from the host'sOp::CancelSubAgentscascade, whichcancel_all_running_for_sessionapplies to every Running child becausedetachedis never persisted on the record. Whether the host's stop/reclaim should spare detached work is a host policy decision; raised in the parent repository rather than changed here.customrequiresallowed_toolsbut the field is not advertised in the schema (parse-accepted; the error names it and recovery works). fix(subagent): free settled names and advertise wait timeout #119 already edits thecustomrole text and the advertised-field count, so this is left to that PR / a follow-up to avoid a conflicting hunk.write/readaccept../under Full Access while scoped children refuse it: the claim ledger speaks repo-relative paths by design and the root's boundary is the sandbox posture, not the tool. Not a defect at that posture.testrole cannot execute a non-cargo test suite (built-in gates only): a posture design question, not a code defect.Review round 1 (2026-10-25)
Fresh multi-lane review of
d031e21f5(comment 5639604543) found no blockers and no smuggled changes; four fixes landed on top:fix(subagent)— same-worktree serialization (above) and the shared-core walk delegation (section 3).fix(shell)— the read-only git listing parsers refused dash-prefixed values and--columnmoved to the switch list (section 5).fix(tools)— binary-read guidance now names what the refusing role can actually run (file; unzip needs a role that permits it), the sniff doc no longer implies every PDF is caught, the image path re-arms theRootDirrefusal and drops its stale wording, and the schema-budget/binary tests gained boundary pins.docs(subagents)— same-worktree serialization and resumed-loop re-orientation documented.Not done, deliberately: splitting the PR. The six areas were reviewed together and each is independently tested; splitting now would orphan this review thread for marginal revert-granularity. The remaining review minors (declaration-seam absolute spellings,
read_lints,coordinate inspectactive markers) are follow-ups, not regressions.Review round 2 (2026-10-25)
Fresh 13-lane re-review of
45e5207c5(comment 5642624693) confirmed two MAJOR gaps in this PR's own claims; both are fixed on top:worktree_rooton the child's own workspace, so acwd: subchild registered<wt>/subwhile the parent and root siblings registered<wt>— the exact-equality sibling predicate never fired, and since a cwd-only spawn keeps the parent's full root set, two children of one worktree could still race the checkout unchecked. The key now resolves to the containing worktree (the nearest ancestor claim whose root contains the child's workspace); a child that creates its own worktree keys on it, and a resumed claim keeps its recorded root.cwd_narrowed_children_of_a_worktree_serialize_against_the_worktreepins admission of the narrowed descendant, the recorded containing root, the root-sibling refusal that keys on it, and the own-worktree negative.coordinate action=release— the recovery the contention receipts themselves recommend) and evictable at ledger capacity; resume restored the claim only by ledger lookup, so a swept row left the write-capable child claim-less with no way back (action=claimonly expands an existing row). The resume now rebuilds the claim from the durable launch manifest when the row is gone — the same fallback the isolation flag already had; claim-less manifests stay claim-less.resumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_gonesweeps the row through the real release path, resumes, and asserts the manifest scope and liveness.Also landed from the round-2 minors: a legacy
Noneworktree root keeps the documented never-contending footing instead ofNone == Nonematching every otherNone(legacy_none_root_isolated_claims_never_contend); the branch parser's dash-value guard and the sniff's UTF-16 BOM arms are now genuinely pinned (both were mutation-verified revertible), with a NUL-free UTF-16 fixture, the UTF-8-BOM and NUL-past-the-window boundary cases, and the bare/flag-onlygit branchlisting spellings admitted; the parallel auto-approve invariant comment matches the widenedPARALLEL_READONLY_PREFIXESand is pinned in both directions; the exemption-walk cap is a named constant documented againstMAX_SPAWN_DEPTH_CEILING; the binary-refusal message no longer tells a UTF-16 text file to unzip a.pptx, and the reduced validation receipt carries the schema's declared type instead of a hardcoded"object".Deliberately left (disclosed in the review): the exemption walk is one-directional (a parent widening its own claim is refused by its live child; a root-session resume loses genealogy and serializes against its original parent); the ancestor-chain traversal remains triplicated across
worktree_claim_exemptions,worker_ancestry, andensure_caller_controls_descendant; the no-repo discovery error still lists the probed ancestor paths in "Tried:"; the fleet seams keyworktree_rootonspec.workspace, which stays correct while each fleet task owns its worktree.Review round 3 (2026-10-25)
Fresh 14-lane review of the rebased head
cee2d4049(rebased ontopinvou3-clean@6e1ee88ee, #122 — pure replay; comment 6400055651) confirmed all six fixes and the body's claims (36/37 — the one miss was this checklist's own helper count, corrected below), found no smuggled changes and no performance regressions, and reported two MAJOR gaps in the round-2 work; both are fixed on top (528e5c0a5):ChildLaunchManifestcarried only theworktreeflag — never the root — so a swept row (thecoordinate action=releaserecovery the contention receipts themselves recommend, or capacity eviction) let a cwd-narrowed worktree child resume keyed on its own subdirectory when the caller was not an ancestor: the exact-equality sibling predicate never fired and the resumed writer raced the checkout unchecked. The same hole let a claim-less worktree parent's cwd-narrowed children each key on their individual workspaces and never serialize against each other. The spawn seam now computes the containing-worktree root once for every isolated child (claim or claim-less), stamps it into the launch manifest, and both the resume path andcontaining_worktree_rootfall back to that recorded root when the row is gone; legacy manifests without the field keep the old behavior. Pinned byswept_cwd_narrowed_worktree_child_resume_rekeys_on_its_manifest_worktree_root(real release path, root-caller resume, and the behavioral half — a fresh worktree writer is refused) andclaim_less_worktree_children_cwd_descendants_serialize_through_the_manifest_worktree_root(durable stamp + narrowed-sibling contention). Mutation-verified: stampingNone, dropping the resume fallback, and dropping the containment fallback each redden exactly their pins.coordination_workspace_prefix; the isolated lane returned them unchanged, so a worktree child spawned withcwd: crates/appand scopedwrite_roots: ["crates/app"]had every natural in-scope spelling refused (main.rsjudged against the worktree frame) while the only passing spelling wrote one directory deeper than intended.validate_write_scopenow maps each relative path into the worktree frame by lexically prefixing the child workspace's offset from the claim's recorded root; absolute spellings, legacyNoneroots, and undecidable workspace/root drift are kept as-is and still refuse. Pinned bycwd_narrowed_scoped_worktree_child_validates_paths_in_its_own_frame(natural spelling admitted, traversal and absolute refused, non-covering scope still refuses) and the end-to-end registry-lane pinchild_write_tool_resolves_absolute_spellings_against_the_childs_workspace. Mutation-verified (bypassing the re-anchoring reddens the frame test).Round-3 minors landed with them (
237875630,4e2595904): the listing grammars admit attached-value spellings (--sort=-committerdate,--column=always,--color=never,worktree list --expire=<time>) and combined shorts (-arv,-vz) — the value is bound to its flag in the same token, so the dash-value guard is unaffected; the[shell.readonly.command]guidance enumerates the fullfindrefused set andcomm, and the rewrite itself is now pinned red/green (shell_readonly_refusal_enumerates_the_admitted_families) — it was the one fix hunk whose reversion nothing detected; the reducedvalid_shapereceipt carries its declared type under test and truncates its own property inventory at a cap; comments and fixtures that overstated the worktree-placement guard, the shared-lane exemptions, the discovery fixture's depth, or the fixture's own bytes are corrected (the UTF-16 fixture really is 你好 now); the parent-reference resolution shared by the two ancestry walks is one named function, and the claim lane delegates through the crate's singleresolve_deepest_existingwrapper.Left as disclosed follow-ups:
expand_write_claimstill takes the shared-workspace process lock unconditionally for isolated owners (pre-existing asymmetry); the claim-flag-then-manifest isolation precedence is written twice (helper and resume site); the fleet seams still keyworktree_rootonspec.workspace(each fleet task owns its worktree); the root-session resume's one-directional genealogy stands; the classifier's parentheses posture keeps--format %(refname:short)refused while--format=%his admitted.Review round 4 and fix wave (2026-10-28)
Fresh 14-lane review of
a252a10fb(rebase check:pinvou3-cleanstill at6e1ee88ee, no-op; comment 6461629607) confirmed the six fixes and found no smuggled changes and no performance regressions, but reported two MAJORs — both reproduced first-hand — plus minors;f8f204b13fixes them:rev-parse/branch --show-current/worktree listbut the unchangedhardened_readonly_argvgit whitelist refused every bare spelling at the execution seam (shell.rsuntouched by this PR; read-only shells rundirect_argv = true, so only piped shapes ever executed). Fixed: the hardener admits the three subcommands (their process-containment needs no spliced flags — the classifier owns the read-only shape), pinned byreadonly_argv_executes_the_git_orientation_probes_without_a_shell, which also pins the closed failure for a mutating subcommand.normalize_claim_pathtrimmed, so a whitespace-padded absolute path validated as its trimmed target while the write tool materialized the literal space-led path under the workspace (required_strnever trims;resolve_pathtests absoluteness on the raw string), and a whitespace-only root normalized to the whole-workspace.. Fixed at the root: the claim lane now judges exactly the string the tool resolves — no trim anywhere in the lane (the internal""->"."identity fromcoordination_workspace_prefixis kept, whitespace-only spellings refuse). Pinned by padded-absolute/padded-relative/whitespace-only assertions inscoped_mutation_paths_accept_absolute_targets_inside_the_workspace.lexically_underjoins both separators (Windows offsets no longer fail closed; unit-pinned),bounded_valid_shapebyte-bounds its reduced inventory and omitstypefor schemas that declare none (overflow and no-type pins added),worker_ancestryresolves parents throughworker_parent_reflike the other two walks, the binary-read refusal tells shell-less roles to delegate (message pins updated; sniff doc's vacuous UTF-32 clause reworded),PersistedWriteClaim.worktree_rootomitsNonefrom the persisted shape (round-trip pin added), the absolute image-path lane applies the relative lane's lexical traversal guard (missing-target pin added), the branch-listing rustdoc moved onto the branch parser,attached_readonly_valuereturns bool,mutation_pathsnormalizes the workspace once per batch, the fleetworktree_root: Nonerationale describes the lane fleet specs can actually take, anddocs/SUBAGENTS.md(en + zh) now documents the containing-worktree keying, the claim-less manifest leg, the legacy never-contending footing, the one-directional exemption, and honest resumed-loop re-orientation wording.Verification at
f8f204b13(treee5025e5b0, byte-identical to the local verification commit):cargo fmt --checkclean; clippy unchanged (the same 7 pre-existing errors in untouched files); targetedtools::subagent::618/0,coord::40/0,tools::shell::142/0,command_safety::75/0,tools::file::126/0 serial,vision::tools::19/0,core::engine::654/0; full lib 12283 passed with 3 failures, each the known load-timing flake class and each 1/1 when re-run alone. Executed mutations against the new pins: the true ancestor-exemption kill reddens both worktree pins; restoring the trim reddens the padded-path pin; reverting the hardener whitelist, the serdeskip_serializing_if, and the separator join each redden exactly their pin; removing the vision guard reddens the missing-traversal pin. Not done, deliberately: the piped-path glob-metachar edge (git branch --list *under a real shell), cross-process same-worktree serialization (pre-existing mechanism the PR only extends), and the row-root-over-manifest resume precedence pin — all disclosed follow-ups.Testing
Local toolchain is stable 1.99.1; a warm target with an identical
Cargo.lockwas reused viaCARGO_TARGET_DIR.cargo fmt --all -- --checkcargo clippy -p codewhale-tui --all-targets --all-features --locked+ the CONTRIBUTING allow list: clean for this change. Pre-existing and not touched here:cloned_ref_to_slice_refsatrepo_law.rs:638(also recorded as pre-existing in fix(subagent): free settled names and advertise wait timeout #119).Targeted:
tools::subagent::+command_safety::+tools::file::+core::engine::+vision::tools::— 1447 passed, 0 failed, 4 ignored.Full
cargo test -p codewhale-tui --locked --libat the review-fix head45e5207c5: 12242 passed, 1 failed, 13 ignored — the one failure isclient::operation_tests::forkguard_model_operation_auxiliary_retry_keys, the load-timing flake already recorded in fix(subagent): free settled names and advertise wait timeout #119; it passes when re-run alone (1/1).Mutation (review fixes): disabling the same-worktree sibling check reddens exactly
worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not(sibling refusal); the manifest-leg test stays green in both.disabling the ancestor exemption reddens it at the first spawn together with— CORRECTED in round 4: the round-4 executed lab could not reproduce the grandchild half under either natural placement (truncating the ancestor walk leaves the parent exempt via start-inclusion; killing the sibling conjunct disables contention outright), because the two pins' stub-spawned parents were never live owners, so their admission legs could not observe exemption removal at all. Round 4 fixes this by marking the parents live; the corrected claim — a true ancestor-exemption kill (write_capable_grandchild_of_a_worktree_child_inherits_isolation!exempt.contains(x)->true) reddens bothworktree_sibling_grandchildren_contend_while_a_descendant_chain_does_notandwrite_capable_grandchild_of_a_worktree_child_inherits_isolation— is now pinned (see the round-4 section).Mutation: restoring the old paired-parked liveness allowance reddens exactly
waiting_for_user_headless_worker_is_not_a_live_coordination_owner,interrupted_child_stops_gating_later_writers,interrupted_child_resume_does_not_contend_with_its_own_parked_claim;write_capable_grandchild_of_a_worktree_child_inherits_isolationstays green.Round 2 head: targeted
tools::subagent::599/0,command_safety::75/0,tools::file::126/0 (serial — the two pdf budget tests flake only in parallel, base-confirmed),core::engine::633/0,vision::tools::19/0; full lib (RUST_MIN_STACK=32MiB) 12244 passed, 2 failed (both the known load-timing flake class in PR-untouched modules, each 1/1 alone).Round 3 head
4e2595904, plusa252a10fb(a follow-up test-hygiene commit that stops echoing the child's resolved endpoint into an assert message — the pre-existing flow CodeQL's diff scan attributed to this PR once earlier hunks shifted it ~1000 lines):cargo fmt --all -- --checkclean; clippy unchanged (the same 7 pre-existing errors, all in untouched files); targetedtools::subagent::+command_safety::+tools::file::+core::engine::+vision::tools::1490 passed / 0 failed / 4 ignored; full lib (RUST_MIN_STACK=32MiB) run twice at this head — 12286 passed / 4 failed and 12289 passed / 1 failed, every failure the known load-timing flake class in PR-untouched modules with rotating membership (the two pdf budget tests,omitted_runtime_models…); each identified failure passes alone. Round-3 mutations (each reverted after):worktree_root: Nonein the manifest stamp reddens both round-3 MAJOR-1 tests and nothing else;.or(manifest…root.filter(is_empty))reddens the swept-resume test; theroot.is_empty()containment kill reddens the claim-less test; neutralizingreanchor_isolated_mutation_pathsreddens the frame test.Round 2 mutations (killed by test at the new assertions): bypassing the containment walk reddens
cwd_narrowed_children_of_a_worktree_serialize_against_the_worktree; removing the manifest fallback reddensresumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_gone; restoringNone == Nonereddenslegacy_none_root_isolated_claims_never_contend. Reverting the branch parser's dash-value guard or deleting the BOM arms now reddens the shell/file suites (they did not before).Checklist
SubAgentManager(parent_runs_in_isolated_worktree,worktree_claim_exemptions,containing_worktree_root) plus theWORKTREE_ANCESTOR_WALK_LIMITconstant, nine free functions beside the code they serve, one ledger field (PersistedWriteClaim.worktree_root), one manifest field (ChildLaunchManifest.worktree_root), and one shared-core delegation)docs/SUBAGENTS.md,docs/zh_hans/SUBAGENTS.mdworktree section; predicate and seam comments)Follow-up (parent repository)
pinvou-agentpicks this up when its CodeWhale gitlink advances to the squash of this PR onpinvou3-clean, with thescripts/fork-guard.shEXPECTED_HEAD/EXPECTED_COMMITSanddocs/fork-modifications.mdregister update that is already outstanding for #115–#119.Round 5 (2026-10-29, head
c776f421a): rebase onto the current base + review fixesRebase. The branch is rebased onto
pinvou3-clean@f1eb10efc(base had advanced by #125 and #114). Every replayed commit's tree matches the server-side combination tree both round-5 reviews computed (d517d3e2565, chain commit 13), so the rebased head is content-identical to the verified conflict-free combination; the PR diff stays at 19 files and the merge-base is now the actual base tip.Fixed (reviews 5687369981 and 5692483474):
vision/tools.rs). The absolute branch refusedComponent::Prefix, so a drive-absolute or verbatim (\\?\) spelling — which necessarily carries a prefix — was rejected before the canonicalized containment check could validate it. The lexical guard now refuses onlyParentDir(traversal); prefix/root shape is left to the containment check, mirroringnormalize_path_components. Pinned by acfg(windows)test (drive-absolute, verbatim, and the outside-workspace negative); the non-gated absolute test is Windows-correct again.subagent/mod.rs). Same-worktree admission deliberately admits unrelated writers on disjoint claims, and an unbounded command cannot prove it stays inside the owner's share, so a live peer keyed on the SAME worktree root now blocks it (live_peer_same_worktree_claim_owners), with the refusal naming the peer and the worktree remediation. Exemptions mirror admission exactly: the owner's ancestor chain is exempt (delegation keeps working), a peer keyed on a different worktree never shared the checkout, legacyNone-root rows keep the never-contending footing, liveness is the shared predicate. One consequence, mirroring the shared lane's existing shape: a parent's own unbounded shell is gated by the live same-worktree descendants it fanned out, while their commands stay exempt against it. Pinned red/green by three new tests; all four load-bearing wiring choices (gate leg, ancestor exemption, liveness, root equality) were mutation-verified (each kill reddens exactly its pin, executed and reverted).subagent/tests.rs). The expected is built in the ledger's canonical frame independently of the code under test — backslashes fold to forward slashes and leading separators collapse (a verbatim prefix reduces to its literal?component) — so the native-Windows assertion compares equal while the leading-space semantics stay pinned; a cross-platformnormalize_claim_pathpin now guards the separator contract on Linux too.Verification.
cargo fmtclean;cargo clippy -p codewhale-tui --all-targets --all-features --lockedreports exactly the pre-existing errors (now 6:repo_law.rs:638was fixed by base #125; none in PR files).tools::subagent::622 +vision::tools::19 = 641 passed, 0 failed (637 at the previous head + 4 new pins).docs/SUBAGENTS.mden+zh state the shell gate's extension to the same-worktree lane.Known limits. The Windows legs (prefix admission, padded-path equality) are pinned by
cfg(windows)/contract tests but were not executed on a native Windows host in this round — CI remains Ubuntu-only. The Windows-only padded-path inequality is reachable only natively, hence the added cross-platform contract pin.Round 6 (2026-10-29, head
06f5c03dc): the in-flight shell lifecycle gap and two review P3sRebase.
pinvou3-cleanis still atf1eb10efc(re-verified immediately before this push), and the branch remains a single linear commit on the base tip — no replay was needed.The P2 both reviewers escalated (reviews 5697466967, 5697622475, 5697735654): owned running shell work now participates in the claim lifecycle. Both demanded arrival orders are covered by one mechanism — a transient reservation the executor opens for every gate-covered unbounded dispatch:
agents/interruptaborts the loop and commitsInterruptedwhile the job stays in the sharedShellManager. The interrupted owner's dispatch had stamped its reservation with the still-running job ids, and a stamped entry answers to the shell manager alone — so an overlapping writer, and the resume under its new id, are refused with an "unbounded shell work is still in flight" receipt naming the owner until the job actually exits or is cancelled (exec_shell_cancel). The shell manager is the truth source: a finished or killed job releases the frame without anyone assuming a writer is gone. The reviewers' "stop-failure must not be treated as no-writer" is met by never stopping and never assuming.worktree_claim_exemptionschain: ancestor delegation keeps working, a peer keyed on a different worktree never shared the checkout, legacyNone-root rows keep the never-contending footing, proven read-only work never registers a reservation.SharedShellManagerits children dispatch through (set_shell_job_source).coordination_lane_of: claim row root → manifest root → containing worktree → shared checkout), so a reservation blocks only its own worktree or the shared checkout. The fleet claim seams (preflight_worker_coordination,register_worker_with_coordination) consult the same reservations.Pinned by
interrupted_child_background_shell_keeps_the_worktree_occupied(order 1: refusal names the owner; different-worktree peer admitted; exemption consult; release leg),resume_of_interrupted_child_waits_for_its_running_background_shell(the transfer leg: resume refused while the job runs, then proceeds and restores its manifest scope),unbounded_dispatch_holds_a_reservation_against_later_arrivals(order 2 through the real dispatch seam with a real background shell: disjoint-claim spawn refused, gate refuses B's own dispatch,kill_runningreleases both directions, foreground dispatch releases at completion), andcoordination_lane_precedence_matches_the_isolation_seam(row root → manifest root → shared). Mutation-verified: killing the admission consult, the gate consult, the shell-truth leg, or the completion stamp each redden exactly their pins (executed and reverted).P3 (both reviews): NFD re-anchoring now folds into the ledger frame.
reanchor_isolated_mutation_pathsapplies the ledger's NFC-per-component fold to both sides of the offset computation and to the joined path — the same foldnormalize_claim_pathgives every registered root — so a worktree subdirectory the OS spelled NFD (macOS) no longer fails the strict string containment. Only the comparison frame folds; on-disk spellings are untouched, and traversal/absolute refusals stand. Pinned bynfd_cwd_narrowed_scoped_child_validates_in_the_ledger_frame(NFD offset + NFC-scoped claim: the natural spelling admitted; ASCII and NFC controls; traversal and absolute negatives) plus unit pins on the fold. Mutation: reverting the fold reddens exactly the frame test.P3 (zhuowp): the Windows outside fixture. The
cfg(windows)outside-workspace negative now uses a second tempdir — outside the workspace by construction — instead of theC:system hosts file, so the pinned containment message is reached deterministically instead of depending on a system file existing forcanonicalize.Verification at
06f5c03dc(tree57b4b8e5d):cargo fmt --all -- --checkclean;cargo clippy -p codewhale-tui --all-targets --all-features --lockedreports exactly the six pre-existing errors, none in touched files; targetedtools::subagent::627,tools::shell::142 (serial),vision::tools::19,command_safety::75,core::engine::662 — all green; full lib 12300 passed with 4 failures, each the known load-timing flake class in PR-untouched modules (forkguard_model_operation_auxiliary_retry_keys+ threeremote_controltests) and each 1/1 when re-run alone.Notes and disclosed limits. A reservation conflict refuses without pushing a durable contention receipt: the occupancy is transient by construction, and a persisted receipt would outlive its cause across a restart — the refusal names the peers and the same remediation verbs the peer gate receipts use. The consults take the shell mutex briefly inside the coordination lock; the reverse nesting does not exist (
touch_running_shell_ownersdrops the shell lock before taking the manager's). An orphaned shell from a crashed host process is not covered on the next boot — the same boundary the per-workspace process-lock posture already accepts. The previously disclosed follow-ups (one-directional exemption walk, fleetspec.workspacekeying, piped-path glob edge, cross-process serialization) stand unchanged.Round 7 (2026-10-30, head
2071e94cd): the unconfirmed-stop residual and the reservation pruneRebase.
pinvou3-cleanis still atf1eb10efc(re-verified via API immediately before this push; the branch's merge-base is the base tip), so the branch remains a linear pair of commits on the base — no replay was needed. (Git :443 was blocked this session, so the push went through the Git Data API chain; the pushed tree is byte-asserted equal to the locally verified treee4e7214b1.)P2 residual (review 5699580547): a sent stop is no longer treated as a confirmed exit. The kill path's bounded SIGTERM→SIGKILL escalation discarded the final
wait_child_boundedoutcome — after a reap-grace expiry (a child in uninterruptible sleep that SIGKILL has not taken yet)kill()still returnedOk, the job becameKilled, and the round-6 reservation consult (which counts onlyRunningjobs) handed the write scope to a successor while the old child could still have been alive. Now:terminate_child_process_groupreturns whether the direct child's exit was observed; a grace expiry recordsstop_unconfirmedon the job.any_running_job,running_task_ids_for_owner,owner_has_running_job) keeps treating the job as a potential writer until a later poll's single boundedtry_waitobserves the exit and clears the flag — "signal delivered" and "exit observed" are different states, and only the latter releases the frame. The stop interface stays bounded everywhere: the confirmation is onetry_waitper consult, never a wait.collect_outputretains the child handle exactly while the flag is set, so that later observation is possible; a confirmed stop releases the handle as before.P3 (same review): dead reservations are pruned by the mutating legs.
open_in_flight_unboundedandcomplete_in_flight_unboundednow drop entries that hold no writer, using the consult's own predicate — so a prune racing a shared-lock consult cannot change its answer, consults stay read-only, and a long session's frame list is bounded by live work instead of growing with dispatch history.Pinned by
escalation_confirms_a_term_ignoring_child_exitandbounded_reap_reports_grace_expiry_while_the_child_lives(the real stop primitive's confirmed and grace-expiry halves),unconfirmed_stop_keeps_the_write_scope_until_the_child_is_reaped(flag held across every query; the OS ends the real child; the next consult'stry_waitclears the flag),kill_confirms_a_normal_escalation_exit(a real escalation throughShellManager::killleaves no unconfirmed state),unconfirmed_stop_keeps_a_real_reservation_until_the_child_is_confirmed_dead(both arrival directions refuse while unconfirmed; both release after the real child's death), andmutating_legs_prune_dead_in_flight_reservationsplus the tightened final leg ofunbounded_dispatch_holds_a_reservation_against_later_arrivals(the dead entry is pruned, not just skipped). Mutations M1 (confirm no-op), M2 (flag ignored by the scope query), and M3 (prune disabled) each redden exactly their pins, executed and reverted.Verification at
2071e94cd(treee4e7214b1):cargo fmt --all -- --checkclean;cargo clippy -p codewhale-tui --all-targets --all-features --lockedreports exactly the six pre-existing untouched-file errors;tools::shell::150 andtools::subagent::555 green; full lib 12305 passed with 5 failures, each in the known load-timing flake class in PR-untouched modules (forkguard_model_operation_auxiliary_retry_keys+ fourremote_controltests), each 1/1 alone.Environment note (not a regression). On this arm64 host the full-lib binary can abort before any test finishes with a stack overflow in
runtime_api::tests::decide_approval_400s_on_bad_decision. The same abort reproduces at the round-6 head06f5c03dcwithout this round's changes, so it is not caused by this branch — it is a marginal test-thread stack depth exposed by incremental-build layout shifts;RUST_MIN_STACK=33554432runs the suite through. Flagging it here in case upstream wants the test server on an explicitly sized thread.Disclosed limits. The un-reaped state itself — a child that survives SIGKILL for the whole reap grace — requires an uninterruptible sleep and cannot be spawned portably, so tests model the flag and drive the real confirmation against a real child the OS ends, and the primitive's false branch is pinned through the controllable grace-expiry counterexample; the in-kill assignment (
stop_unconfirmed = !reaped) is consequently covered by contract at both ends rather than by an end-to-end negative. Fork CI remains Ubuntu-only; the unix-only legs are cfg-gated. The previously disclosed follow-ups (no durable contention receipt, orphaned-shell-after-crash boundary, one-directional exemption walk, fleetspec.workspacekeying, piped-path glob edge, cross-process serialization) stand unchanged.Round 9 (2026-10-30, head
71221288d): unconfirmed writers survive retention and destructionRebase.
pinvou3-cleanis still atf1eb10efc(re-verified via API immediately before this push; the branch's merge-base is the base tip), so the branch remains linear on the base — no replay was needed, and there are no conflicts to resolve. (Git :443 was blocked again this session, so the round went through the Git Data API chain: the three changed blobs were pushed and SHA-matched individually, the resulting tree is byte-asserted equal to the locally verified treec0edde071, and the ref move was a non-forced fast-forward over2071e94cd.)P2 residual (review 5703015199): cleanup and destruction honor the potential-writer state. Round 7 taught the coordination queries to keep an unconfirmed writer's frame, but the retention paths could still delete the record itself.
cleanup()'s retain exempted onlyRunningjobs, andenforce_finished_job_bounds()counted every non-Running record toward the 128-record/8 MiB ceilings — so aKilled + stop_unconfirmedjob already older thanFINISHED_SHELL_MAX_AGElost its record, and the only retained child handle with it, to an ordinarylist_jobs()or background-spawn cleanup;any_running_job()then found no record and the frame could be released while the old child may still have been alive. Now:holds_write_scope(), with one boundedtry_waitin the retain pass so an already-dead child is confirmed and reclaimed in the same sweep instead of lingering forever;BackgroundShell::dropgives an unconfirmed scope holder the same final bounded stop aRunningjob gets, so shutdown — or a future eviction path that regresses — cannot discard the possibly-live handle silently;try_waitper observation, never a wait.P2 (same review): the Windows test-target compile regression.
killed_shell_does_not_wait_for_blocked_reader_threadswas missing the unconditionalstop_unconfirmedfield in itsBackgroundShellinitializer, which breaks the Windows lib-test target with E0063; the initializer is complete again. Cross-platform note: this arm64 host cannot cross-build the Windows target (ring's C build script fails under the host toolchain), so the fix was verified by compiling an identical initializer on Linux — green with the field, and exactlyerror[E0063]: missing field 'stop_unconfirmed'without it; the Linux-visible field set is the Windows-visible set minus the already-presentwindows_job.Pinned by
cleanup_keeps_an_unconfirmed_stop_until_the_exit_is_confirmed(a zero age bound evicts every finished record that holds no write scope; the unconfirmed record survives it, and once the OS ends the real child the same bound reclaims it),capacity_eviction_spares_an_unconfirmed_stop_until_the_exit_is_confirmed(under the production age bound the ceilings count the unconfirmed record out; once it is confirmed it rejoins the finished population and the ceiling reclaims the oldest — exactly that record),dropping_the_manager_stops_a_child_with_an_unconfirmed_stop(destroying the manager stops the possibly-live child behind the retained handle; observed via the reaped pid answeringkill(pid, 0)with ESRCH), and the tightenedunconfirmed_stop_keeps_a_real_reservation_until_the_child_is_confirmed_dead— its modeled stop now records theKilledstatus the kill path actually produces, and after a zero-age cleanup the consult still refuses a later disjoint spawn and an unbounded dispatch (both arrival directions), releasing only after the real child's death. Mutations M1 (cleanup retention reverted), M2 (capacity filter reverted) and M3 (Drop condition reverted) each redden exactly their own pins, executed and reverted.Verification at
71221288d(treec0edde071):cargo fmt --all -- --checkclean;cargo clippy -p codewhale-tui --all-targets --all-features --lockedreports a single pre-existing error (core/engine/turn_loop.rscollapsible_if), reproduced on the stash-verified pre-round content — a file this PR never touches; no clippy errors in any touched file.tools::shell::149 passed / 0 failed andtools::subagent::629 passed / 0 failed, including all four new or tightened pins. Full lib (RUST_MIN_STACK=32MiB): 12310 passed / 3 failed and, rerun at the same tree, 12311 passed / 2 failed — every failure inremote_control::tests, the same load-timing flake class in PR-untouched modules recorded in round 7, with rotating membership between runs; each identified failure passes 1/1 alone. Exact-head CI dispatched as run41013690599.Disclosed limits. Unchanged from round 7: the un-reaped state itself — a child that survives SIGKILL for the whole reap grace — cannot be spawned portably, so the flag is modeled and every confirmation leg is driven against a real child the OS ends. The capacity leg is pinned at the record ceiling; the byte ceiling shares the same
holds_write_scopefilter line and is not separately counterexampled. Fork CI remains Ubuntu-only; the unix-only legs are cfg-gated, and the Windows compile regression is verified by the Linux-identical-initializer probe above rather than a native Windows build. The previously disclosed follow-ups (no durable contention receipt, orphaned-shell-after-crash boundary, one-directional exemption walk, fleetspec.workspacekeying, piped-path glob edge, cross-process serialization) stand unchanged.