Skip to content

fix(subagent): release interrupted claims and inherit worktree isolation - #123

Merged
asto18089 merged 22 commits into
pinvou3-cleanfrom
fix/interrupted-child-claim-liveness
Nov 4, 2026
Merged

asto18089 merged 22 commits into
pinvou3-cleanfrom
fix/interrupted-child-claim-liveness

Conversation

@asto18089

@asto18089 asto18089 commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

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, wait timeout_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 general child (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:

Error: write-scope contention with agent_24d6ec0b (roots: ["."], files: [], contracts: []); serialize the work, narrow the claim, or use worktree isolation

Reproduced six times over ten minutes; cancel on the interrupted child returned its status unchanged (cancel only touches Running children). Root cause: an interrupted child's worker record parks in WaitingForUser (its checkpoint is continuable), and is_live_coordination_owner counted 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 Running agent from this boot is a live claimant. The interactive case the old comment protected is the approval prompt, where the agent is still Running with a WaitingForUser record; 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_isolation stays green.

2. A worktree child could not spawn a write-capable child

From inside an isolated worktree, agent(action=start, type=general) was refused:

shared writer workspace '<sid>/.codewhale-worktrees/workspace/codex-agent-t-depth3-860236d8' must remain inside coordination root '<sid>/workspace'

A read-only explore grandchild from the same worktree worked, which pins the refusal to claim registration. Worktrees are created as siblings of the checkout (resolve_worktree_path forbids nesting), so coordination_workspace_prefix can never strip the coordination root from a worktree workspace — yet the grandchild, having no worktree/cwd request of its own, is already de facto confined to that worktree (prepare_child_workspace returns no override; a cwd must sit under the parent workspace). Only the ledger disagreed.

Fix: the spawn seam inherits the parent's isolation — parent_runs_in_isolated_worktree uses 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_claim and the launch manifest's worktree flag. The persisted-claim prompt now says worktree-relative for isolated claims (it said coordination-root-relative for 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_not pins both sides; the rule is documented in docs/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.txt and got write scope path must be repo-relative without traversal: <that path>; the relative spelling succeeded. mutation_paths fed the raw tool path straight into normalize_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 — via codewhale_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_analyze had the same rule — the computer_use screenshot receipt hands the model an absolute path, and the first image_analyze call 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/hosts pin keeps refusing, asserting on the containment message).

4. Worktree discovery scanned $HOME for repositories

worktree=true on a session workspace that is not a repository failed with:

Multiple git repositories found under /Users/asto. Specify cwd to disambiguate: /Users/asto/flutter, /Users/asto/Spec2017-iOS

git_repo_root walked 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-current and git worktree list while git status/git log passed — exactly the probes a worktree child needs to orient itself. rev-parse joins the plain subcommand table (read-only in every form; also added to COMMAND_ARITY and PARALLEL_READONLY_PREFIXES, so the parent's parallel chunks accept the same bounded flags). branch and worktree take positional operands that create things, so they are special-cased to their listing forms only: branch --show-current alone or --list/-l (patterns become read-only), and worktree list with 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 --column is attached-only, so the checker refuses a dash-prefixed value instead of swallowing it uninspected). rev-parse --git-path is read-only but refused by the same flag bounding that keeps rev-parse to 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.rs extended.

6. Two receipt-quality defects from the same transcript

  • read on a .pptx returned 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.
  • Every invalid_input receipt echoed the tool's full input schema as valid_shape; for agent that 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 (properties names + 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=true child cancelled between turns. CodeWhale's turn-end paths are detached-aware (detached children take background_runtime() and never enter the foreground registry; waiting_for_subagents was emitted correctly). The cancellation came from the host's Op::CancelSubAgents cascade, which cancel_all_running_for_session applies to every Running child because detached is 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.
  • custom requires allowed_tools but 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 the custom role text and the advertised-field count, so this is left to that PR / a follow-up to avoid a conflicting hunk.
  • The root session's write/read accept ../ 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.
  • test role 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 --column moved 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 the RootDir refusal 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 inspect active 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:

  • cwd-narrowed children bypassed the same-worktree serialization. The spawn seam keyed worktree_root on the child's own workspace, so a cwd: sub child registered <wt>/sub while 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_worktree pins admission of the narrowed descendant, the recorded containing root, the root-sibling refusal that keys on it, and the own-worktree negative.
  • A resumed writer could lose its claim permanently. Fix 1 makes parked claims non-live, which makes them sweepable (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=claim only 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_gone sweeps the row through the real release path, resumes, and asserts the manifest scope and liveness.

Also landed from the round-2 minors: a legacy None worktree root keeps the documented never-contending footing instead of None == None matching every other None (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-only git branch listing spellings admitted; the parallel auto-approve invariant comment matches the widened PARALLEL_READONLY_PREFIXES and is pinned in both directions; the exemption-walk cap is a named constant documented against MAX_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, and ensure_caller_controls_descendant; the no-repo discovery error still lists the probed ancestor paths in "Tried:"; the fleet seams key worktree_root on spec.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 onto pinvou3-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):

  • The serialization key had no durable record. ChildLaunchManifest carried only the worktree flag — never the root — so a swept row (the coordinate action=release recovery 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 and containing_worktree_root fall back to that recorded root when the row is gone; legacy manifests without the field keep the old behavior. Pinned by swept_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) and claim_less_worktree_children_cwd_descendants_serialize_through_the_manifest_worktree_root (durable stamp + narrowed-sibling contention). Mutation-verified: stamping None, dropping the resume fallback, and dropping the containment fallback each redden exactly their pins.
  • Isolated claims were never re-anchored to the child's cwd. The shared lane re-anchors mutation paths through coordination_workspace_prefix; the isolated lane returned them 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. validate_write_scope 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 workspace/root drift are kept as-is and still refuse. Pinned by cwd_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 pin child_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 full find refused set and comm, 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 reduced valid_shape receipt 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 single resolve_deepest_existing wrapper.

Left as disclosed follow-ups: expand_write_claim still 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 key worktree_root on spec.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=%h is admitted.

Review round 4 and fix wave (2026-10-28)

Fresh 14-lane review of a252a10fb (rebase check: pinvou3-clean still at 6e1ee88ee, 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; f8f204b13 fixes them:

  • MAJOR, fix 5: the classifier admitted rev-parse/branch --show-current/worktree list but the unchanged hardened_readonly_argv git whitelist refused every bare spelling at the execution seam (shell.rs untouched by this PR; read-only shells run direct_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 by readonly_argv_executes_the_git_orientation_probes_without_a_shell, which also pins the closed failure for a mutating subcommand.
  • MAJOR, fix 3: normalize_claim_path trimmed, 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_str never trims; resolve_path tests 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 from coordination_workspace_prefix is kept, whitespace-only spellings refuse). Pinned by padded-absolute/padded-relative/whitespace-only assertions in scoped_mutation_paths_accept_absolute_targets_inside_the_workspace.
  • Mutation-claim correction: see the struck-through Testing bullet — the grandchild pin could not detect exemption removal because its stub parent was never a live owner; both relevant pins now spawn the parent live and the corrected mutation claim is executed-verified.
  • Minors fixed: lexically_under joins both separators (Windows offsets no longer fail closed; unit-pinned), bounded_valid_shape byte-bounds its reduced inventory and omits type for schemas that declare none (overflow and no-type pins added), worker_ancestry resolves parents through worker_parent_ref like 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_root omits None from 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_value returns bool, mutation_paths normalizes the workspace once per batch, the fleet worktree_root: None rationale describes the lane fleet specs can actually take, and docs/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 (tree e5025e5b0, byte-identical to the local verification commit): cargo fmt --check clean; clippy unchanged (the same 7 pre-existing errors in untouched files); targeted tools::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 serde skip_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.lock was reused via CARGO_TARGET_DIR.

  • cargo fmt --all -- --check

  • cargo 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_refs at repo_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 --lib at the review-fix head 45e5207c5: 12242 passed, 1 failed, 13 ignored — the one failure is client::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 write_capable_grandchild_of_a_worktree_child_inherits_isolation — 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 (!exempt.contains(x) -> true) reddens both worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not and write_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_isolation stays 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, plus a252a10fb (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 -- --check clean; clippy unchanged (the same 7 pre-existing errors, all in untouched files); targeted tools::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: None in the manifest stamp reddens both round-3 MAJOR-1 tests and nothing else; .or(manifest…root.filter(is_empty)) reddens the swept-resume test; the root.is_empty() containment kill reddens the claim-less test; neutralizing reanchor_isolated_mutation_paths reddens 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 reddens resumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_gone; restoring None == None reddens legacy_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

  • This PR adds a new layer/module/abstraction — it names or deletes the layer it replaces (no new layer: three private helpers on SubAgentManager (parent_runs_in_isolated_worktree, worktree_claim_exemptions, containing_worktree_root) plus the WORKTREE_ANCESTOR_WALK_LIMIT constant, 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)
  • Updated docs or comments as needed (docs/SUBAGENTS.md, docs/zh_hans/SUBAGENTS.md worktree section; predicate and seam comments)
  • Added or updated tests where relevant
  • Verified TUI behavior manually if UI changes (no UI change)
  • Harvested/co-authored credit uses a GitHub numeric noreply address (n/a)

Follow-up (parent repository)

pinvou-agent picks this up when its CodeWhale gitlink advances to the squash of this PR on pinvou3-clean, with the scripts/fork-guard.sh EXPECTED_HEAD/EXPECTED_COMMITS and docs/fork-modifications.md register update that is already outstanding for #115–#119.


Round 5 (2026-10-29, head c776f421a): rebase onto the current base + review fixes

Rebase. 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):

  1. Windows absolute image paths (vision/tools.rs). The absolute branch refused Component::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 only ParentDir (traversal); prefix/root shape is left to the containment check, mirroring normalize_path_components. Pinned by a cfg(windows) test (drive-absolute, verbatim, and the outside-workspace negative); the non-gated absolute test is Windows-correct again.
  2. Unbounded-shell contention gate covers the isolated lane (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, legacy None-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).
  3. Padded-path expected per the ledger frame (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-platform normalize_claim_path pin now guards the separator contract on Linux too.

Verification. cargo fmt clean; cargo clippy -p codewhale-tui --all-targets --all-features --locked reports exactly the pre-existing errors (now 6: repo_law.rs:638 was 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.md en+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 P3s

Rebase. pinvou3-clean is still at f1eb10efc (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:

  • Order 1 (interrupt while a background job runs). agents/interrupt aborts the loop and commits Interrupted while the job stays in the shared ShellManager. 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.
  • Order 2 (A starts alone, B arrives on a disjoint claim). The reservation opens BEFORE the peer checks, and claim admission consults reservations, so registration precedes checking on both sides: of two racing writers, whoever reserves second sees the first and exactly one is refused. The peer gate also refuses a new unbounded dispatch while a peer's reservation holds the same frame.
  • Exemptions mirror admission exactly — the consults reuse each admission site's worktree_claim_exemptions chain: ancestor delegation keeps working, a peer keyed on a different worktree never shared the checkout, legacy None-root rows keep the never-contending footing, proven read-only work never registers a reservation.
  • Validity is deliberately narrow. Once stamped, only the stamped jobs decide (a live-but-idle owner must not hold a finished dispatch's frame — the round's first lab run caught exactly that hole in an earlier draft); before stamping, a live owner means the dispatch is in flight, and a terminal owner keeps an unstamped entry only while it still has some running job (the abort-mid-spawn window). Entries are transient, not persisted. The engine wires the manager to the same SharedShellManager its children dispatch through (set_shell_job_source).
  • Both consults ride the lane keying this PR already established (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_running releases both directions, foreground dispatch releases at completion), and coordination_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_paths applies the ledger's NFC-per-component fold to both sides of the offset computation and to the joined path — the same fold normalize_claim_path gives 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 by nfd_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 the C: system hosts file, so the pinned containment message is reached deterministically instead of depending on a system file existing for canonicalize.

Verification at 06f5c03dc (tree 57b4b8e5d): cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --all-targets --all-features --locked reports exactly the six pre-existing errors, none in touched files; targeted tools::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 + three remote_control tests) 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_owners drops 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, fleet spec.workspace keying, piped-path glob edge, cross-process serialization) stand unchanged.

Round 7 (2026-10-30, head 2071e94cd): the unconfirmed-stop residual and the reservation prune

Rebase. pinvou3-clean is still at f1eb10efc (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 tree e4e7214b1.)

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_bounded outcome — after a reap-grace expiry (a child in uninterruptible sleep that SIGKILL has not taken yet) kill() still returned Ok, the job became Killed, and the round-6 reservation consult (which counts only Running jobs) handed the write scope to a successor while the old child could still have been alive. Now:

  • terminate_child_process_group returns whether the direct child's exit was observed; a grace expiry records stop_unconfirmed on the job.
  • Every coordination query (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 bounded try_wait observes 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 one try_wait per consult, never a wait.
  • collect_output retains the child handle exactly while the flag is set, so that later observation is possible; a confirmed stop releases the handle as before.
  • Windows and PTY stops already wait synchronously and cannot be unconfirmed. Normal exits, ancestor delegation, different-worktree peers, and read-only exemptions are untouched.

P3 (same review): dead reservations are pruned by the mutating legs. open_in_flight_unbounded and complete_in_flight_unbounded now 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_exit and bounded_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's try_wait clears the flag), kill_confirms_a_normal_escalation_exit (a real escalation through ShellManager::kill leaves 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), and mutating_legs_prune_dead_in_flight_reservations plus the tightened final leg of unbounded_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 (tree e4e7214b1): cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --all-targets --all-features --locked reports exactly the six pre-existing untouched-file errors; tools::shell:: 150 and tools::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 + four remote_control tests), 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 head 06f5c03dc without 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=33554432 runs 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, fleet spec.workspace keying, piped-path glob edge, cross-process serialization) stand unchanged.

Round 9 (2026-10-30, head 71221288d): unconfirmed writers survive retention and destruction

Rebase. pinvou3-clean is still at f1eb10efc (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 tree c0edde071, and the ref move was a non-forced fast-forward over 2071e94cd.)

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 only Running jobs, and enforce_finished_job_bounds() counted every non-Running record toward the 128-record/8 MiB ceilings — so a Killed + stop_unconfirmed job already older than FINISHED_SHELL_MAX_AGE lost its record, and the only retained child handle with it, to an ordinary list_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:

  • both eviction legs consult holds_write_scope(), with one bounded try_wait in the retain pass so an already-dead child is confirmed and reclaimed in the same sweep instead of lingering forever;
  • BackgroundShell::drop gives an unconfirmed scope holder the same final bounded stop a Running job gets, so shutdown — or a future eviction path that regresses — cannot discard the possibly-live handle silently;
  • the stop interface stays bounded everywhere: confirmation remains one try_wait per observation, never a wait.

P2 (same review): the Windows test-target compile regression. killed_shell_does_not_wait_for_blocked_reader_threads was missing the unconditional stop_unconfirmed field in its BackgroundShell initializer, 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 exactly error[E0063]: missing field 'stop_unconfirmed' without it; the Linux-visible field set is the Windows-visible set minus the already-present windows_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 answering kill(pid, 0) with ESRCH), and the tightened unconfirmed_stop_keeps_a_real_reservation_until_the_child_is_confirmed_dead — its modeled stop now records the Killed status 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 (tree c0edde071): cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --all-targets --all-features --locked reports a single pre-existing error (core/engine/turn_loop.rs collapsible_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 and tools::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 in remote_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 run 41013690599.

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_scope filter 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, fleet spec.workspace keying, piped-path glob edge, cross-process serialization) stand unchanged.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fresh review 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-existing cloned_ref_to_slice_refs at repo_law.rs:638. (Without -A clippy::collapsible_if there is also a pre-existing turn_loop.rs:5407 hit — untouched file.)
  • Full cargo test -p codewhale-tui --locked --lib: 12240 passed / 1 failed / 13 ignored — exactly the disclosed numbers; the failure is forkguard_model_operation_auxiliary_retry_keys, which passes alone (1/1, 0.09s).
  • PR CI: all green (fork-ci check 17m51s).

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. Dropping levels == 0: discovery test red. workspace_relative_mutation_path passthrough: abs-path test red.
  • Removing the rev-parse/branch/worktree admissions: agent_readonly_shell_admits_real_reconnaissance_shapes red, 67 other command_safety tests 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

  1. 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.
  2. Body: rev-parse --git-path is listed among "mutating forms"; it is read-only and refused only by table omission. "coordination_workspace_prefix can never strip" is overstated — in the nested-repo layout (anchor adopts a single child checkout) the worktree lands inside the coordination root.
  3. 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 only file is actionable. Routing a %PDF head to the existing PdfTextCommand (as the legacy ReadFileTool::execute does) would be the natural improvement.
  4. parent_runs_in_isolated_worktree doc says "live claim" but find matches any row including released history (harmless — same flag). The manifest-fallback leg is untested at the spawn seam; only the claim leg is.
  5. coordinate inspect shows 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 of coordinate action=release, which now actually works on it. Parked rows persist until capacity eviction (128) or session close.
  6. 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.
  7. Grammar: both new parsers re-implement the agent_text_filter_options_match loop; -l meant --create-reflog before git 2.20 (worth a comment); the refusal message under-advertises (npm show|info, comm, the gh read-only families, codewhale --version, -C/--no-pager preambles) — the conservative direction.
  8. vision/tools.rs:102 still says "must be a relative path" in the traversal branch now that in-workspace absolutes are admitted; the relative branch dropped the RootDir arm (Windows-only \foo, still caught by containment).
  9. 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 to MAX_PARENT_LEVELS == 4 with no in-test guard. Unpinned: FE FF BOM, 2048/2049 boundary, schema without properties, --column -d reject, branch -D/-M/-c/-C/-f/--edit-description/-u, worktree lock/move/repair, rev-parse --parseopt/--sq-quote, depth-3 inheritance.
  10. 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); SubAgentStatus has no pre-Running state, so no admission window; prior-boot Running agents are rewritten to Interrupted on load; interrupt_child → WaitingForUser parking and resume_from_checkpoint new-id dispatch verified end to end; release_stale_write_claims and finalize_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 worktree flag has no cleanup/removal consumer anywhere, so no worktree-deletion regression; action=claim and validate_write_scope read the record flag; depth 3+ inherits.
  • F3: workspace and candidate canonicalize symmetrically (/var → /private/var on 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.rs find_git_root has 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_PREFIXES has one consumer and stays consistent.
  • F6: the sub-agent read wires to execute_contract_read (the fixed path); no other model-facing lossy reader exists; serde_json preserve_order makes 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.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed as 92eba745c..45e5207c5 (4 commits on top of d031e21f5), addressing this review:

M1 — shared deepest-existing walk. resolve_absolute_target is deleted; workspace_relative_mutation_path delegates to codewhale_core::walk_deepest_existing (DeepestAncestor + compose()), the single implementation behind the write tool's own boundary (tools::spec::resolve_nonexistent_path). All existing abs-path tests stay green; the helper doc names the delegation.

M2 — same-worktree serialization. PersistedWriteClaim gained worktree_root: Option<String> (canonical worktree path; serde-defaulted, so records persisted before the field keep the legacy never-contending footing). register_claim now takes the root plus the new owner's ancestor chain; claims registered against the same worktree root contend unless the existing owner is an ancestor of the new one — the parent delegated the work downward, but parallel siblings refuse with the usual contention receipt, matching the shared root. Sites feeding it: the interactive spawn seam (agent.workspace + runtime.parent_agent_id), claim expansion (owner's own chain, so growing a claim doesn't start contending with the delegating ancestor), and the headless preflight/commit paths (spec.workspace + spec.parent_run_id). Red/green: stubbing the same-root check reddens exactly worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not (at the sibling refusal); removing the ancestor exemption reddens it at the first spawn together with write_capable_grandchild_of_a_worktree_child_inherits_isolation; claim_less_worktree_child_still_confers_isolation_through_its_manifest (new, the manifest-fallback leg) stays green in both. The contention message text is byte-identical, so #119's WRITE_SCOPE_CONTENTION_PREFIX extraction stays semantically compatible — whoever lands second rebases that one block.

M3 — grammar. --column moved to the switch list (it is an attached-only OPTARG in git, so the separate-word spelling leaves the next token a real flag); both listing parsers refuse a dash-prefixed value instead of swallowing it uninspected, matching options_match_allowlist. Reject pins added for the swallow shape (git branch --list --column -d x), branch -D, --edit-description, -u, worktree lock/move, rev-parse --parseopt; admit pins for branch --list -v, branch -l pattern, worktree list -z. The parser doc notes -l means --list only since git 2.20.

Minors fixed: binary-read guidance names file (usable read-only) vs unzip (needs a role that permits it) and the sniff doc no longer implies every PDF is caught; the image path re-arms the RootDir component refusal (Windows rooted spellings) and drops its stale "must be a relative path" wording (test pin follows the unified message); parent_runs_in_isolated_worktree doc says "registered claim"; 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; docs (en + zh) state the same-worktree serialization rule and that a resumed loop is only serialized against writers live at resume time; PR body counts corrected (2 helpers, 5 free functions) and the internal "F12" label replaced with the test name.

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, read_lints absolute refusal, coordinate inspect active markers, registry call-site wiring pin) are recorded as follow-ups rather than regressions.

Validation at 45e5207c5: cargo fmt --all -- --check clean; clippy with the CONTRIBUTING allow list shows only the pre-existing repo_law.rs:638; full cargo test -p codewhale-tui --locked --lib 12242 passed / 1 failed / 13 ignored, the failure being the known forkguard_model_operation_auxiliary_retry_keys load-timing flake (passes alone, 1/1); the two new mutation batteries above are red exactly as claimed.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fresh review 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:

  1. The dash-prefixed-value guard in the branch parser is unpinned — mutation-verified: reverting it to the old unconditional index += 2 swallow leaves the suite 75/75 green (only the --column switch shape is pinned). Add a reject pin like git branch --list --sort -d x (the worktree parser's value-flag path likewise).
  2. 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 lists rev-parse unqualified though only a flag subset is admitted — branch is carefully qualified, rev-parse is not. (--format='%(…)' is unreachable anyway: the caller charset gate rejects parens.)
  3. parallel_classifier_stays_unchanged_for_parent_auto_approve still asserts the parent's parallel chunks "keep rejecting" the relaxed families, but this PR adds git rev-parse to PARALLEL_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.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-2 disposition (fix wave pushed as fb9ceb9ec..ed712da6d on 45e5207c5):

MAJOR-1 — fixed (fb9ceb9ec). The spawn seam now keys an isolated claim on the containing worktree: containing_worktree_root reuses the worktree_claim_exemptions walk and returns the nearest ancestor claim's root that contains the child's workspace; a child that creates its own worktree (a sibling — nesting is forbidden) and a child with no isolated ancestor fall back to their own workspace, and a resumed claim keeps its recorded root through the new write_claim_worktree_root spawn option (a root-session resume has no ancestor chain to resolve containment from). The cwd-narrowed sibling now contends with a root sibling; the narrowed descendant stays admitted through the ancestor exemption. Pinned by cwd_narrowed_children_of_a_worktree_serialize_against_the_worktree; mutation (containment walk disabled) reddens it at the key assertion.

MAJOR-2 — fixed (fb9ceb9ec). When the ledger lookup misses, resume rebuilds the claim from the durable launch manifest via the existing worker_coordination_claim — the same fallback the isolation flag already had. Claim-less manifests stay claim-less; a manifest with scope restores it, and claim_pre_namespaced routes the restore through the spawn seam's contention admission. Pinned by resumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_gone (real sweep via release_stale_write_claims, then resume); mutation (fallback removed) reddens it.

Minors fixed (fb9ceb9ec, 531ecf0b2, c108bd75b): legacy None-root isolated claims keep the never-contending footing (predicate requires is_some, unit-pinned); the exemption-walk cap is WORKTREE_ANCESTOR_WALK_LIMIT, documented against MAX_SPAWN_DEPTH_CEILING; the branch parser's dash-value guard is pinned (branch --list --sort -d x, worktree list --expire -v — both were mutation-verified revertible before) and bare/flag-only git branch listing spellings are admitted; the parallel auto-approve comment matches the widened PARALLEL_READONLY_PREFIXES and is pinned both directions; the refusal message qualifies rev-parse and names the branch listing forms; the UTF-16 sniff fixtures are NUL-free so the BOM arms are real pins, with UTF-8-BOM / NUL-past-the-window boundary cases and a Use \file`assertion added; the binary-refusal message no longer advises unzipping a UTF-16 text file; the reduced schema receipt carries the schema's declared type instead of a hardcoded"object"`; the discovery test documents its tempdir-outside-a-checkout assumption.

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 (worktree_claim_exemptions / worker_ancestry / ensure_caller_controls_descendant) — a follow-up refactor touching stable ancestry code; the "Tried:" ancestor-path listing in the no-repo discovery error (diagnostic value; repo names are no longer leaked); the fleet seams keying worktree_root on spec.workspace — correct while each fleet task owns its worktree, revisit if tasks ever share one; the remaining false-refusal spellings in the readonly grammar (--flag=value attached forms, -a without bare-branch) — fail-closed and the message steers to admitted forms.

Gates at ed712da6d: fmt clean; clippy = the 7 pre-existing errors only, none in touched files; targeted suites 599/75/126/633/19 all 0 failed; full lib 12244 passed with 2 known rotating flakes (each 1/1 alone, PR-untouched modules). PR body updated with the round-2 section, the corrected topic-disjoint wording, and the round-2 gate/mutation records.

@asto18089
asto18089 force-pushed the fix/interrupted-child-claim-liveness branch from ed712da to cee2d40 Compare October 25, 2026 15:08
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fresh round-3 review (2026-10-25) — head cee2d4049, rebased onto pinvou3-clean @ 6e1ee88ee (#122)

Rebase was a pure replay of all 8 commits (byte-identical diffstat, zero conflicts); pushed. From-scratch 14-lane re-review: per-fix deep dives A–H, smuggling sweep, body audit, elegance, performance, cross-cutting bug hunt, test-quality audit — plus fmt/clippy/targeted/full-lib gates and a fresh mutation battery at this head.

Still holds from rounds 1–2: all six fix areas are real, root-caused, and honestly described; smuggled CLEAN (all 15 files attributed hunk-by-hunk to a body claim, no deps/CI/version/config churn, linear DCO-signed commits); performance clean (spawn-time bounded costs only; git_repo_root and the schema receipt are strict wins; per-write liveness got cheaper); every behavioral claim in the body checks out against the tree (36/37 — one stale count below). Two new MAJORs, both in the PR's own core surface.

MAJOR-1 — the serialization key has no durable fallback: two reachable paths still escape same-worktree serialization

The round-2 fix keys isolated claims on the containing worktree via containing_worktree_root (subagent/mod.rs:5099–5116), which consults only ancestor claim rows. The durable manifest fallback that exists for the isolation flag and the claim scope does not exist for the root: ChildLaunchManifest carries worktree: bool but never the worktree path, and resume takes restored_worktree_root from the ledger row alone (mod.rs:6080–6095). Two escapes:

  1. Swept row + non-ancestor resume. A cwd-narrowed worktree child (workspace <wt>/app) is interrupted; its row is swept (coordinate action=release — the recovery the contention receipts themselves recommend — or capacity eviction); the resume is issued by the root (or any non-ancestor caller). containing_worktree_root finds no ancestor claim row and falls back to the child's own workspace, so the rebuilt claim keys <wt>/app while the parent and root siblings key <wt> — the exact-equality sibling predicate (coord/ledger.rs:542–550) never fires and the resumed child holds a full-checkout claim racing the checkout unchecked. The body discloses the row-present root-resume case ("serializes against its original parent" — over-contention, fail-safe); this swept-row variant is the opposite direction — silent under-serialization — and is not disclosed.
  2. Claim-less worktree parent. A worktree child spawned claim-less (the PR's own claim_less_worktree_child_still_confers_isolation_through_its_manifest premise) registers no row, so its own cwd-narrowed children all fall back to their individual workspaces and never serialize against each other.

Verified at head; same class as the round-2 MAJOR this PR treated as a must-fix, and in the fail-open direction. Fix direction: persist the resolved worktree root in the launch manifest next to the worktree flag and prefer it in both containing_worktree_root's fallback and the resume seam — or disclose both paths explicitly in the body's left-alone list.

MAJOR-2 — isolated claims are never re-anchored to the child's cwd: a scoped, cwd-narrowed worktree child cannot write where it is told to write

For shared children both claim registration and write validation re-anchor paths through coordination_workspace_prefix, so tool paths (child-cwd-relative) and claim roots (coordination-relative) always share a frame. The isolated lane skips both: namespace_write_claim and namespace_claim_paths_for_owner return paths unchanged (mod.rs:5117–5149), so claim roots stay worktree-relative while validate_write_scope is fed mutation_paths(name, &input, &self.registry.context().workspace) — the child's cwd-narrowed workspace (mod.rs:16050, context rebased at mod.rs:9799–9811). A worktree child spawned with cwd:"app" and write_roots:["app"] (worktree-relative, as the new prompt correctly states) is broken in both directions:

  • the natural spelling x.rs (meaning <wt>/app/x.rs, squarely inside the claim) validates as x.rs against root app → refused;
  • the only passing spelling, app/x.rs, is then executed by the child's file tools against its own workspace and lands in <wt>/app/app/ — one level deeper than intended (still inside the claim, so fail-closed, but wrong).

The default ["."] claim masks this (every PR test uses it). Pre-PR this spawn configuration was refused outright at registration, so the surface is newly opened by section 2 of this very PR. Verified by trace at head; repro script attached in the review notes. Fix direction: when isolated and the child workspace differs from the claim's anchor (the recorded/containing worktree root), re-anchor validated paths from the child workspace into the worktree frame — the isolated-lane analogue of coordination_workspace_prefix.

Minors (fix or disclose at your discretion)

  • Test pins: the rewritten [shell.readonly.command] guidance (the 13-retry-loop fix) is the one fix hunk with no red/green test — only the pre-existing tag assertion (tests.rs:8306) covers it, so reverting the message stays green; the bounded_valid_shape declared-type propagation is mutation-blind (every fixture declares "type":"object" or none, so hardcoding "object" again survives); the write_claim_worktree_root Some-leg (recorded-root resume preservation) is untested — deleting the field and its plumbing stays green; the mutation_paths registry call-site is pinned only through a relative-path no-op (a wrong workspace passed at mod.rs:16050 would re-break absolute spellings with nothing red); the value-flag admission path in the new git parsers has no positive pin (git branch --format <f>, worktree list --expire 2.now — deleting --format/--expire from the value-flag tables stays green).
  • Body: the checklist says "two private helpers on SubAgentManager" but the code carries three — containing_worktree_root (mod.rs:5099, added in round 2) plus the WORKTREE_ANCESTOR_WALK_LIMIT const is unlisted. Cosmetic: the UTF-16 fixture bytes FF FE 60 4F 34 6C decode to 你水, not 你好 (comment and body both mislabel it).
  • Shell grammar: attached-value read-only spellings are refused (--sort=-committerdate, --column=always, --color=never, -av) — fail-closed but exactly the retry-loop fuel this fix targets, and the most idiomatic listing form a model emits; the parser comments misstate git (--column is an OPTARG: bare form works, the separate-word form just doesn't attach).
  • Wording: the "worktrees are created as siblings / nesting forbidden" comments (and body parenthetical) overstate resolve_worktree_path, which only refuses paths inside the parent checkout; expand_write_claim's comment claims ancestors stay exempt, but the shared-lane predicate ignores exempt_owners entirely.
  • Receipts: the reduced valid_shape inventory is itself unbounded (hundreds of properties → the reduction exceeds the 2048 budget it enforces; not reachable with today's schemas).
  • Discovery test: worktree_discovery_never_adopts_or_enumerates_an_ancestors_sibling_repos comments "two levels up" but home is four hops above the fixture workspace — one fixture level deeper and the test passes vacuously, hidden by the wrong comment.
  • Refusal text: the new message says find refuses -delete/-exec but the grammar also refuses -execdir/-ok/-okdir/-fprintf/-fls/-fprint* — under-enumeration feeds retries; comm is admitted but omitted from the admitted list.
  • Consistency (pre-existing, adjacent): expand_write_claim takes the shared-workspace process lock unconditionally for isolated owners (the skip exists only at the spawn seam) — an isolated child can spawn then fail its first action=claim in a process that doesn't own the lock; parent_runs_in_isolated_worktree's claim-then-manifest precedence is re-implemented inline at the resume site (mod.rs:6101–6150) — one helper would prevent drift; the parent-ref resolution (parent_run_id.or(spec.parent_run_id)) now exists in three walks.

Verified and retracted (lanes claimed, tree disproves)

The manifest fallback does not depend on prunable records: any result carrying a continuable checkpoint parks its worker record at WaitingForUser (worker_status_from_subagent_result, mod.rs:14465–14470), which is non-terminal and exempt from prune_worker_records — so "record pruning + swept row defeats the fallback" is unreachable for every resumable child. Reported by two lanes independently; both withdrawn after code verification.

Gates and mutation battery (this head, warm target)

  • cargo fmt --all -- --check: clean.
  • cargo clippy -p codewhale-tui --all-targets --all-features --locked: 7 errors, all pre-existing and all in untouched files (context_report.rs:422, core/engine.rs:1876/8523, turn_loop.rs:881/5497, lib.rs:12432, repo_law.rs:638) — consistent with the body's claim.
  • Targeted (tools::subagent:: command_safety:: tools::file:: core::engine:: vision::tools::): 1485 passed / 0 failed / 4 ignored.
  • Full --lib (RUST_MIN_STACK=32MiB): 12282 passed / 3 failed / 13 ignored — the 3 failures are the known load-timing flake class (omitted_runtime_models_use_the_active_provider_default, plus two runtime_threads cursor-projection timing tests); all three pass alone (3/3 in 0.72s), the suite is 38 tests larger than round 2 and the modules are untouched by this PR.
  • Mutations at this head, each red/green verified then reverted:
    • killing the containing-worktree containment (root.is_empty() in the ancestor match) → cwd_narrowed_children_of_a_worktree_serialize_against_the_worktree red, sibling test green (attribution clean);
    • removing the resume manifest-scope fallback (.or_else(|| None)) → resumed_child_restores_claim_scope_from_its_manifest_when_the_row_is_gone red, claim_less_worktree_child_still_confers_isolation_through_its_manifest green (the isolation flag falls back at a separate site — the scope leg is pinned independently);
    • killing the sibling predicate (false && …) → worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not red + legacy_none_root_isolated_claims_never_contend red (its third leg pins the same predicate);
    • restoring the paired-parked liveness allowance (|| matches!(status, Interrupted(_))) → exactly the three claimed tests red, write_capable_grandchild_of_a_worktree_child_inherits_isolation green — matches the body's mutation footprint verbatim.
  • MAJOR-2 runtime probe (scratch test, reverted after): spawning a worktree parent + cwd:"crates/app" child with scoped write_roots:["crates/app"], then validate_write_scope(child, ["main.rs"]) — the natural cwd-relative spelling of a file inside the declared scope — returns Err("write 'main.rs' is outside agent … scope (roots: [\"crates/app\"])"), while validate_write_scope(child, ["crates/app/main.rs"]) is admitted (a spelling the child's own tools would resolve one directory deeper than intended). Bug confirmed live, not just by trace.

Verdict

The PR remains fundamentally sound — useful, root-caused, honest in its disclosures, clean of smuggling, and well-tested by any ordinary standard. But MAJOR-1 leaves the PR's own headline serialization rule escapable through its newest paths, and MAJOR-2 breaks a natural configuration on the surface section 2 opens. Both need a fix or an explicit disclosure before merge; minors are follow-up-grade.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave landed on top (head 4e2595904, three DCO commits, all gates re-run):

  • MAJOR-1 (528e5c0a5): ChildLaunchManifest now carries the resolved containing-worktree root for every isolated child (claim or claim-less); the resume path and containing_worktree_root fall back to it when the row is gone. Both escape paths are pinned red/green — the swept-row root-caller resume test asserts the manifest key and the behavioral half (a fresh worktree writer is refused), and the claim-less test asserts narrowed-sibling contention; stamping None, dropping the resume fallback, and dropping the containment fallback each redden exactly their pins.
  • MAJOR-2 (528e5c0a5): validate_write_scope re-anchors isolated children's relative mutation paths into the worktree frame (lexical join, absolute/legacy/drift kept as-is and refused). Pinned at the unit level (natural spelling admitted, traversal out refused, non-covering scope still refuses) and end-to-end through the real registry lane; bypassing the re-anchoring reddens the frame test.
  • Minors (237875630, 4e2595904): attached-value and combined-short listing spellings admitted; the [shell.readonly.command] guidance enumerates the full find refused set and comm and is now red/green pinned; the reduced receipt's declared type is pinned and its inventory truncated at a cap; the registry call site is pinned end-to-end; the checklist inventory is corrected (three helpers + const, nine free functions, two fields); the comment/fixture overstatements are fixed (the UTF-16 fixture really is 你好 now); worker_parent_ref and the resolve_deepest_existing wrapper close the two DRY gaps.

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

@asto18089

Copy link
Copy Markdown
Collaborator Author

Review round 4 (2026-10-28)

Fresh multi-lane re-review of a252a10fb (rebase check first: origin/pinvou3-clean is still 6e1ee88ee, so the branch is already on the latest base — no rebase needed). Method: 14 independent review lanes (six fix areas, integration surfaces, smuggling attribution, performance, concurrency, docs, design), deterministic gates (fmt, clippy, full --lib, serial tools::file::), an executed mutation lab re-checking this body's seven mutation claims, and independent second-agent confirmation of every major finding. No smuggled changes were found; CI is green on this head; clippy's 7 errors are all in untouched files, matching this body's claim exactly, and the two full-lib failures re-run green alone (remote_control:: 77/0 serial — module untouched by this PR). Two MAJOR findings, both reproduced first-hand by the reviewer:

MAJOR 1 — Fix 5 does not reach execution for the probes it exists to unblock

The classifier work is real: rev-parse / branch --show-current / worktree list now pass is_agent_readonly_shell_command, and piped spellings run (the pipe path goes through CommandSpec::shell, shell.rs:2161-2167, where the classifier is the gate). But a bare invocation — the canonical spelling of every motivating probe — is still refused at execution: read-only shells execute with direct_argv = true (shell.rs:5388-5392), which routes single commands through hardened_readonly_argv (shell.rs:3963), whose git whitelist was not touched by this PR (git diff 6e1ee88ee..a252a10fb -- crates/tui/src/tools/shell.rs is empty): diff | log | show | status | ls-files | blame | grep are matched and everything else errors with "classifier-approved Git read did not keep its subcommand in argv[1]" (shell.rs:4011-4014). So git rev-parse --show-toplevel is admitted by the classifier and then refused by the hardener; only … | cat spellings work end-to-end. The new [shell.readonly.command] guidance names the admitted families without saying they must be piped, so it directs the model into shapes that still fail — the exact retry loop this fix set out to end. Fail-closed, hence major not blocker. Suggested fix: extend hardened_readonly_argv's git match (rev-parse/branch/worktree with the same read-only arg sets the classifier already enforces), or correct the guidance to name the piped-only spelling — either closes the claim.

MAJOR 2 — Whitespace-padded absolute paths validate one path and write another

workspace_relative_mutation_path (mod.rs:16577+) trims before testing absoluteness, then strips the workspace prefix, so " <workspace>/fixture/calc/x.txt" is validated as fixture/calc/x.txt against the claim. The write tool resolves the raw string: required_str returns it untrimmed (tools/src/lib.rs:241-246) and resolve_path tests is_absolute on the raw spelling (spec.rs:1132-1138), so the padded path is not absolute and joins onto the workspace — the tool materializes <ws>/␣/… while the ledger recorded <ws>/fixture/calc/x.txt. Scope validation and execution therefore disagree about the target; the write lands outside the claimed directory (still inside the workspace, so the sandbox posture holds, but the claim boundary is bypassed). The base refused every absolute spelling outright, so this window is opened by this PR against its own doc-comment invariant at mod.rs:16572-16574. Suggested fix: trim at the boundary consumed by both lanes (or refuse padded spellings before the trim). The padded-relative variant of this divergence pre-existed; only the absolute case is new here.

Mutation-claim correction (this body's Testing section)

The claim that "disabling the ancestor exemption reddens it at the first spawn together with write_capable_grandchild_of_a_worktree_child_inherits_isolation" did not reproduce — under either natural placement, executed in a scratch worktree:

  • Truncate the walk (return vec![start_id.clone()] after resolution): only worktree_sibling_grandchildren_contend_while_a_descendant_chain_does_not reddens; the grandchild pin stays green.
  • Ignore exempt_owners entirely (!(true || exempt_owners.contains(…)) at ledger.rs:547): 5 tests redden — worktree_sibling_grandchildren…, legacy_none_root_isolated_claims_never_contend, cwd_narrowed_children_of_a_worktree_serialize_against_the_worktree, claim_less_worktree_children_cwd_descendants_serialize_through_the_manifest_worktree_root, swept_cwd_narrowed_worktree_child_resume_rekeys_on_its_manifest_worktree_root — and write_capable_grandchild… still passes (611 passed; 5 failed). Reason: that pin's stub-spawned parent is never a live owner in the test environment, so its admission leg cannot observe exemption removal at all. The behavior itself is fine — the exemptions are demonstrably load-bearing (four pins catch their removal) — but the body's Testing section should attribute the ancestor-exemption kill to worktree_sibling_grandchildren… alone, not to the grandchild pin. The other six mutation claims in the body reproduced exactly as stated (m1 paired-parked restore: 3 red + 1 green control; sibling-check kill, manifest-stamp kill, resume-fallback kill, containment kill, re-anchor kill: all red as named).

Other findings (all minor or nit; none block once the majors land)

  • reanchor_isolated_mutation_paths derives its offset from platform Display while lexically_under splits only on / — on Windows the re-anchored path mixes separators against forward-slash claim roots and fails closed (read-verified only, not executable on this host).
  • bounded_valid_shape's reduction is count-capped, not byte-capped: required is cloned uncapped and 40 long property names can exceed the 2048 budget the comment claims to honor (dispatch.rs:272-300); for schemas with no declared type the fallback still hardcodes "object" — the exact composition-only case the comment warns about (unasserted by the pins).
  • Piped-path glob edge: git branch --list * passes the classifier and runs under a real shell, where * expands — a workspace file named -d re-enters git as a flag. Contrived (needs dash-named files + piped spelling); single-command path is shell-free and unaffected.
  • Binary-read refusal guidance is not actionable for the refusing role in two cases: PDFs get no runnable extractor pointer (pdftotext is model-invisible and not in any read-only table), and ShellPolicy::None roles have no shell at all, so "file" and "unzip" are both unreachable without saying "delegate".
  • PersistedWriteClaim.worktree_root serializes unconditionally (ledger.rs:166-169 has #[serde(default)] but no skip_serializing_if, unlike its neighbors) — every new record now carries an explicit "worktree_root": null, and the engine-fixture comment describing the record shape went stale.
  • The disclosed "shared worker_parent_ref" covers only two of the three walks: worker_ancestry (mod.rs fan-in) still reads record.parent_run_id alone, so spec-only parent pointers stop fan-in ancestry early while claim-exemption ancestry continues — the exact disagreement the helper's doc warns against.
  • SubAgentSpawnOptions.write_claim_worktree_root is documented as checkpoint-resume-only but is honored unconditionally at the spawn seam (mod.rs:6907-6917) for any future caller; the fleet seam's worktree_root: None rationale comment describes a lane fleet specs can never enter (confinement + depth 0 keep isolation false) — value correct, comment misleading.
  • Docs omissions (nothing stated is wrong): the one-directional exemption (a parent expanding over a live descendant's same-root claim is refused) is undocumented in either language; the containing-worktree keying, claim-less manifest leg, and legacy-nil footing are enforced but unstated; "must re-orient to whatever completed while it was parked" is guidance-only — build_resume_prompt replays the checkpoint tail plus the follow-up, no completion surface exists.
  • Nits: rustdoc for the branch listing grammar sits on attached_readonly_value instead of is_agent_readonly_git_branch (command_safety.rs:826-864); unwrap_or_else(|| candidate.to_path_buf()) at mod.rs:16591 is dead (resolve_deepest_existing always returns Some); the UTF-32 clause in the sniff doc is vacuous (UTF-32 BOMs are caught by the NUL arm) and no UTF-32 fixture exists; the pin's "~4.5KB" agent-schema figure measures ~6.7KB on this tree; attached_readonly_value returns Option<&str> but every call site only tests is_some(); per-absolute-path workspace canonicalization in workspace_relative_mutation_path could hoist per batch (microseconds); same-root keying silently falls through to the child's own workspace when an ancestor's worktree was deleted between registration and spawn (canonicalize-only-if-exists in normalize_subagent_workspace) — fail-open, narrow window.

Verified sound (so it doesn't get re-litigated)

Fix 1 end-to-end: the base defect premise reproduces from 6e1ee88ee (paired parked counted live), the interrupt path parks WaitingForUser records while the agent goes Interrupted (mod.rs:12405-12428), and the interactive approval case keeps the agent Running (mod.rs:15453) so it stays live under the new rule; resume-under-new-id premise confirmed in resume_from_checkpoint_with_policy. Fix 3's core delegation is real: core::authority::resolve_deepest_existing (authority.rs:637) and the write boundary (tools/spec.rs) both sit on codewhale_core::walk_deepest_existing. Smuggling: every hunk of the 17-file diff attributed to a claimed fix or disclosed review item; the ~5400-line tests.rs change categorized (moved/new/disclosed-correction; no weakened unrelated pin found); zero changes outside the 17 files; all 12 commit messages match their diffs. Performance: no regressions — the only super-linear spot is the bounded exemption-walk × ledger-scan per spawn, against a capped ledger. Cross-process same-worktree serialization remains process-local, but that is pre-existing mechanism (base already persisted whole-state last-writer-wins) and the PR only extends the covered population; docs claim only parity with the shared lane, which is accurate. En/zh docs carry the same substantive rules.

Verdict

CHANGES_REQUESTED on the two majors: (1) make Fix 5 effective for bare spellings (extend hardened_readonly_argv or fix the guidance), and (2) close the padded-absolute validate/execute divergence. Plus the one-line Testing-section correction above. Everything else is follow-up grade; the PR remains genuinely useful and root-caused on the other five fixes, and the review found no smuggled content and no performance regressions. (One reviewer-side disclosure: the workflow's module-filter test leg was an invalid cargo invocation on my side and never ran; the full --lib leg covered those modules.)

@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave (2026-10-28, head f8f204b13)

Both round-4 majors and the actionable minors are fixed on top of a252a10fb (rebase still a no-op — pinvou3-clean remains 6e1ee88ee; the commit reached the branch through the Git Data API while github.com:443 was unreachable — its tree e5025e5b0 is byte-identical to the locally verified commit).

MAJOR 1 (fix 5 exec seam) — hardened_readonly_argv now executes git rev-parse, branch listing forms and worktree list bare, closing the classifier-admits-but-hardener-refuses gap; the read-only shape stays the classifier's contract (the hardener only keeps the read inside git's own process — these subcommands' read-only forms spawn no configured helper). Pinned by readonly_argv_executes_the_git_orientation_probes_without_a_shell, which also asserts git push still fails closed at the seam. Reverting the whitelist reddens exactly this pin.

MAJOR 2 (padded-absolute divergence) — fixed at the claimed root: the claim lane no longer trims anywhere. normalize_claim_path judges the raw string and workspace_relative_mutation_path tests absoluteness on it, so a padded absolute validates as the literal space-led relative components the tool actually joins (<ws>/␣␣/…), never as the trimmed target it does not touch; a whitespace-only spelling now refuses instead of normalizing to the whole-workspace . (a small hazard the old trim had). The internal "" → "." identity (coordination_workspace_prefix) is preserved — this is what the first subagent suite run caught when it regressed nine pins. Pinned by padded-absolute, padded-relative and whitespace-only assertions in scoped_mutation_paths_accept_absolute_targets_inside_the_workspace; restoring the trim reddens exactly them.

Mutation-claim correction, with a correction of my own. My round-4 comment said the "ignore exempt_owners entirely" placement left the grandchild pin green while 4 other pins reddened — on re-execution, that placement (!(true || …)) actually disabled the whole sibling predicate, not just the exemption, and every one of its 5 reds was "expected contention that never fired". The true exemption kill (!exempt.contains(x) → true) told the real story: pre-fix, write_capable_grandchild_of_a_worktree_child_inherits_isolation was blind because its stub parent was never a live owner. Both worktree pins now spawn the parent live, and the true kill reddens both — the body's corrected Testing bullet (struck-through old claim, replacement claim) is executed-verified.

Minors fixed (each with its pin or doc change): lexically_under joins both separators (Windows offsets no longer fail closed; new unit pin — reverting it reddens the pin); bounded_valid_shape byte-bounds the reduced inventory (long names skip into the omitted tail — the overflow pin failed against the old code organically, which is the strongest kill evidence I have) and omits type for schemas that declare none (no-type pin added); worker_ancestry resolves parents through worker_parent_ref like 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_root omits None from the persisted shape (round-trip pin across the field introduction — removing skip_serializing_if reddens it); the absolute image-path lane applies the relative lane's lexical traversal guard so a non-existent escaping target gets the containment message (missing-traversal pin; removing the guard reddens it); the branch-listing rustdoc moved onto is_agent_readonly_git_branch; attached_readonly_value returns bool; mutation_paths normalizes the workspace once per batch; the fleet worktree_root: None rationale now describes the lane fleet specs can actually take; SubAgentSpawnOptions.write_claim_worktree_root's doc states it is honored for any isolated child, not only resumed ones; and docs/SUBAGENTS.md (en + zh, kept at parity) documents the containing-worktree keying, the claim-less manifest leg, the legacy never-contending footing, the one-directional exemption, and re-worded resumed-loop re-orientation.

Verification at this head: cargo fmt --check clean; clippy unchanged (same 7 pre-existing errors, all untouched files); targeted suites all green — tools::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 (remote_control ×2, pdf budget ×1) and each 1/1 re-run alone. Six executed mutations, six pins killed.

Deliberately not done (unchanged disclosures, plus one new): the piped-path glob-metachar edge (git branch --list * under a real shell — contrived, needs dash-named workspace files; a classifier-level quote requirement would break legitimate unquoted patterns and deserves its own discussion), cross-process same-worktree serialization (pre-existing whole-state-last-writer-wins mechanism this PR only extends), the row-root-over-manifest resume precedence pin (row and manifest agree by construction today), the interactive seam's process-lock skip for inherited-isolated children (the coordination lock is cooperative, so no observable pin exists), and the same-root keying fallthrough when an ancestor worktree is deleted mid-window (fail-open, narrow).

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fresh review (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_not and write_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 serde skip_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

  1. 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 (every options.isolated_worktree consumer migrated to the combined flag; explicit shared-root cwd structurally impossible via validate_existing_child_cwd), the absolute-spelling claim lane (delegates through the crate's single resolve_deepest_existing, the same walk as the write tool's boundary), the $HOME discovery 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.
  2. 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).
  3. 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.
  4. 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):

  1. tools/shell.rs:3997 — the new hardener comment says "process-spawning hazards … belong to diff/log/show only", but git blame executes a repo-configured diff.<drv>.textconv by default (verified on git 2.43), and blame is admitted both in the classifier tables and in PARALLEL_READONLY_PREFIXES without a --no-textconv splice. The exposure is pre-existing and not widened here, but the comment cements a false invariant — splice --no-textconv for blame or correct the comment and file the follow-up.
  2. tools/subagent/mod.rs:16077 — the [shell.readonly.command] guidance omits -fprint0/-truncate from the find refused set (the round-3 body claim "enumerates the full find refused set" is false as written); harmless-direction omissions too (gh read-only families, npm show|info, codewhale --version are admitted but unlisted). No dangerous-direction mismatch was found.
  3. tools/subagent/mod.rs:4176 — the re-anchor offset joins raw to_string_lossy bytes (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 through normalize_claim_path.
  4. 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.
  5. core/engine/dispatch.rs:345 — the required omission tail sits inside if !required.is_empty(), so a reduced schema whose required names are ALL skipped (>200 bytes) silently implies nothing is required; and VALID_SHAPE_INVENTORY_BYTES (1024) is enforced but no test observes it — the overflow fixture trips the 200-byte name cap instead.
  6. tools/subagent/mod.rs:16600 — workspace_relative_mutation_path's deepest-existing fs walk runs before normalize_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.
  7. docs/SUBAGENTS.md:207 (+ zh twin) — "the ledger does not surface what completed while it was parked" overstates: coordinate inspect does list parked-period rows (without active markers). Suggested wording: "does not mark which of those writers completed".
  8. 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 is namespace_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_gone and swept_cwd_narrowed_worktree_child_resume_rekeys_on_its_manifest_worktree_root also 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 Running and 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" omits reanchor_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 in resolve_path) rests on separately-verified stable semantics — a registry-lane padded twin of child_write_tool_resolves_absolute_spellings_against_the_childs_workspace would close it;
  • no end-to-end execute() admission pin for a bare absolute image_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 push pins 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 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 zhuowp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

固定审核:head f8f204b13b87471d9a8533c35fcc6fb1cbc86310,tree e5025e5b0bd6e883d3054471b303915636d2ec22;实际目标 pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c;merge-base 6e1ee88ee8f8ea28eec7be3d8add3a591468412a。 API base.sha仍为merge-base,不是当前目标头;只读组合树 d517d3e25659bf8d6a587b6d69ab2700c654e698 无文本冲突,未编译/执行。Cargo清单和锁未变;本PR也未更新其他父仓的gitlink。

请求修改,确认三项P2:

  1. 同一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豁免,或提供真实范围限制;完整证据及反证在本轮本地审计详单。
  2. 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。
  3. 新增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也不能推出完全无保护;发布资格未评估,无发布授权及跨平台交付证明。未修改产品、合并、入队或发布。

asto18089 and others added 14 commits October 29, 2026 17:46
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>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Responding to review 5709512014 (and #pullrequestreview-5709398775 with the same two findings). Both P2s are fixed and pushed (71221288d..71cc26218, plain push, no force). Per finding:

P2-1: outer tool cancellation leaves an unstamped reservation live → 12d9e85fb (fix(review): reclaim cancelled dispatch frames)

Fix: execute_full now owns the reservation through an InFlightDispatchGuard (crates/tui/src/tools/subagent/mod.rs). The returned path settles through the same completion leg as before; when the loop's tokio::time::timeout drops the execute future between opening the reservation and the stamping completion after the await, the guard's Drop settles the cancellation synchronously at the drop point (try_write — the dispatch future only ever drops between polls, so the write lock is uncontended in practice; if the lock is genuinely unavailable the entry is left untouched, which is the pre-fix conservative behavior, not a new failure). The reclaim (reclaim_cancelled_in_flight_unbounded) keys the frame on the shell manager alone — the abandoned dispatch is no longer in flight, so the owner's Running status stops speaking for it. Jobs still holding their write scope (a stranded foreground child, a background spawn, a stop whose exit was never confirmed) are stamped exactly as the completion leg would stamp them and keep blocking both arrival directions; with nothing holding scope the frame opens; with no shell source the entry is left untouched and the consult's owner-liveness fallback keeps the conservative answer. No wholesale clear of possibly-live work exists on this path; refusal, ancestor exemption, and separate-worktree consults are untouched.

Tests: cancelled_dispatch_settles_its_frame_from_the_shell_manager_alone (the empty frame is reclaimed at the drop; a genuinely live job is stamped in and releases only once its exit is observed; an unwired manager keeps the conservative entry until the owner goes terminal) and outer_tool_timeout_settles_the_stranded_child_then_reopens_the_frame, driven through the real dispatch seam: the deadline drops the dispatch future mid-call, the stranded foreground child keeps both arrival directions refused while it lives, and once its cancelled exit is observed the same frame admits the successor again. Mutation evidence: an inert Drop redds the first pin at "the abandoned dispatch's empty frame is reclaimed at the drop" (left: [("agent_a", [])]) and the second at "the stranded child's task id is stamped"; both reverts were executed and restored.

P2-2: the standalone Workflow builder omits the shell source → 71cc26218 (fix(workflow): bind the headless shell job source)

Fix: build_direct_workflow_tool (crates/tui/src/lib.rs, the 12163 construction site) binds its coordination manager to the workflow context's own shell manager right after constructing the manager — set_shell_job_source(Arc::clone(&context.shell_manager)) — mirroring the Engine's construction-time wiring. This closes the confirmed chain: CLI 11904→11921 executes the Workflow tool without ever passing through the Engine at engine.rs:2281, so completion treated the missing source as zero running jobs and released a background dispatch's frame at the first return; once the dispatching owner went terminal, a successor writer was admitted against a checkout whose job still ran inside the context's ShellManager. WorkflowTool::shell_job_source_for_test / coordination_manager_for_test (cfg(test)) expose the bound source for pins. The Fleet coordination-only manager and the Runtime API manager are deliberately untouched: neither hosts in-process shell jobs (fleet workers run as separate processes whose own engines wire their own managers) — if you read those paths as needing the binding too, say so and I will follow up.

Tests: direct_workflow_builder_binds_its_context_shell_manager (Arc identity between the bound source and the builder context's shell manager) and standalone_workflow_wiring_holds_the_frame_past_a_terminal_owner, driven through the real builder: a background job stamps its task id through the builder's wiring, a terminal owner's frame refuses both arrival directions (a successor's unbounded dispatch and a disjoint spawn) while the job holds write scope, and a cancelled exit observed by the poll opens the frame for both, so the next call resumes. Mutation evidence: disabling the builder binding redds both pins at "the standalone builder binds a shell job source"; the revert was executed and restored.

Validation: cargo test -p codewhale-tui --lib subagent -- --test-threads=1 is 749 passed with 1 failure, child_permission_gate::auto_review_guardian_denial_refuses_the_call_with_a_receipt — the same failure reproduces on the unmodified baseline 71221288d (verified by stashing these commits and rerunning; the test passes 1/1 alone), so it is a pre-existing order/timing case on this branch, not introduced by these two commits. workflow:: 202 and shell:: 149 all green. cargo fmt --all -- --check clean; cargo clippy -p codewhale-tui --all-targets --all-features --locked clean under the fork-ci allow set (the stricter ci.yml allow set reports exactly one pre-existing collapsible_if at core/engine/turn_loop.rs:5499, a file these commits do not touch). Previously resolved findings (retention/destruction, the Windows missing initializer, NFD frame, image prefix, padded path, outside fixture) remain resolved.

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 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

当前固定 head 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 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 zhuowp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

固定审核 head 71cc26218f568c8aec718aed62ff9d5c6847ba5c,tree 58949b6725e45ed80571801c3157b45f2fbbe8fa;实际 target/merge-base pinvou3-clean@f1eb10efcfe2468fde42d1724d7f5f19d54b904c;最新个人审核请求 34891166935。请求修改,确认两项P2:

  1. 取消清理在共享锁竞争下仍会遗留占用。 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分钟竞争或测得现场发生频率。
  2. 新增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>
@asto18089

Copy link
Copy Markdown
Collaborator Author

修复波已推(2 提交 → 394947008)。逐项处置:

P2「取消清理在共享锁竞争下仍会遗留占用」— 已修(a4e40c105)。 三个取消窗口全部闭合:

  1. 准入窗:guard 原本只在 peer-check 两个 await 之后才构造,token 已开而无人持有清理责任。现在 token 打开的瞬间 guard 即接管(admission 即 own),refusal 路径改走 guard.cancel()。
  2. settle 旗标窗:settled=true 原在 write().await 之前置位,等锁期间被取消则 Drop 跳过对账、完成腿未跑。现在旗标只在完成腿实际持锁执行完毕后才翻转。
  3. 争锁 Drop 窗:单一 try_write 失败即放弃、无延后回收。现在争锁时把同一 shell-aware reclaim 交给 detached task 排队,持锁者释放后执行——仍然只给真实活工作盖章、不整体清除;无 runtime 可 spawn 时保持保守 owner-liveness 答案不变。

token 键控不变(对已完成条目是 no-op),live job / unconfirmed-stop 围栏全部保留。新增三钉:settle 在锁等待处被取消仍对账;争锁 drop 最终给活 job 盖章、给已结束 frame 开门;refusal 释放在锁等待处被取消仍重开 frame。三钉在定向突变下(settled 先置/禁用 spawn/cancel 先置)逐一转红,还原后逐字节恢复转绿。

P2「test helper 的 cfg 不匹配使 Windows lib-test 编译失败」— 已修(第二提交)。 mark_stop_unconfirmed_for_test 与 coordination_manager_for_test 改为 #[cfg(all(test, unix))],与其唯一消费方(#[cfg(unix)] 测试)对齐;workflow 的 shell_job_source_for_test 保持 #[cfg(test)](lib.rs 有可移植消费方)。

门禁(本机 rustc/cargo 1.99.1):tools:: 2126/0;fmt 干净;workspace clippy(CI 旗标原样)0 新诊断。合并顺序注:本轮仍触碰 shell.rs、subagent/mod.rs、tests.rs、workflow/mod.rs 四文件,与 #126 的三文件并集在 #126 rebase 时仍需留意。

仅修复与钉测试,未合并、未入队。请在 394947008 重审。

@asto18089
asto18089 requested a review from zhuowp November 2, 2026 10:13

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

固定 head 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 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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>
@asto18089

Copy link
Copy Markdown
Collaborator Author

P3 follow-up wave pushed (6c40e3bb2, 1 commit, DCO, fast-forward on 3949470084)

Dispositions for the two open external review threads:

  1. zhuowp's two P2s at 71cc26218 — already closed at 3949470084, verified again on source: (a) the cancellation-lifecycle windows are closed — the InFlightDispatchGuard takes ownership of the token immediately after admission and before the peer-check awaits, settle/cancel flip settled only after the guarded mutation ran under the write lock, and a contended Drop queues the same token-keyed, shell-aware reclaim on a detached task (eventual, not best-effort); (b) the two Unix-only test helpers (mark_stop_unconfirmed_for_test, coordination_manager_for_test) are now #[cfg(all(test, unix))], so the Windows lib-test dead_code denial is gone with warnings=deny retained.

  2. Jensen's twice-noted non-blocking P3 — fixed in this commit (test-only): the three cancellation-window tests coordinated the lock holder with a bare sleep(20ms) and asserted the detached reconciliation immediately. Now: the holder acknowledges acquisition over a oneshot awaited with a bounded timeout (so the abandonment window is provably entered — a loaded runtime can no longer let settle/cancel complete uncontended and fail the is_err assertion), and the settle/refusal tests' final assertions poll in a bounded 2 s loop instead of assuming the scheduler ran the detached reclaim, matching the idiom the drop-contention test already uses.

Gates on this machine: the three tests 3/3, the in_flight/cancelled-dispatch/timeout family 12/12, cargo fmt --check clean, clippy diagnostic set byte-identical to the base head (3949470084). No production change; one file (subagent/tests.rs, +61/−16).

Re-requesting both reviewers: @zhuowp (your two P2s above await your confirmation at the fixed head), @JensenChen28 (increment since your approval is this one commit).

@JensenChen28 JensenChen28 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

批准固定 head 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 zhuowp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

固定审核 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、合并、入队或发布。

asto18089 added a commit that referenced this pull request Nov 3, 2026
…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>
asto18089 added a commit that referenced this pull request Nov 3, 2026
…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>
@asto18089 asto18089 closed this Nov 4, 2026
@asto18089 asto18089 reopened this Nov 4, 2026
@asto18089
asto18089 merged commit e5a8af2 into pinvou3-clean Nov 4, 2026
7 checks passed
asto18089 added a commit that referenced this pull request Nov 4, 2026
Main's #123/#124 added `seed_running_shell_for_test` while this slice
was in flight; the rebased replay needs its `BackgroundShell` literal to
carry the two stdin state fields this slice introduced.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
asto18089 added a commit that referenced this pull request Nov 4, 2026
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>
asto18089 added a commit that referenced this pull request Nov 5, 2026
Main's #123/#124 added `seed_running_shell_for_test` while this slice
was in flight; the rebased replay needs its `BackgroundShell` literal to
carry the two stdin state fields this slice introduced.

Signed-off-by: asto18089 <asto18089@users.noreply.github.com>
asto18089 added a commit that referenced this pull request Nov 5, 2026
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>
asto18089 added a commit that referenced this pull request Nov 5, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants