Skip to content

fix(subagent): free settled names and advertise wait timeout - #119

Open
asto18089 wants to merge 4 commits into
pinvou3-cleanfrom
fix/subagent-name-release-and-wait-timeout
Open

asto18089 wants to merge 4 commits into
pinvou3-cleanfrom
fix/subagent-name-release-and-wait-timeout

Conversation

@asto18089

@asto18089 asto18089 commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

No-Issue: issues are disabled on this repository; the defects below were verified from a live multi-agent session transcript (2026-10-23) and against the code.

A real workload — parent on deepseek/deepseek-flash, four research children searching the day's A-share news through the model-facing agent tool — hit three avoidable walls in sequence. Transcript evidence is quoted verbatim; each has a red/green test.

1. Parallel type=general starts collide although none of them writes

Three of four action=start calls in one block were refused:

Failed to spawn sub-agent: write-scope contention with agent_7e2950c4 (roots: ["."], files: [], contracts: []); serialize the work, narrow the claim, or use worktree isolation

general is write-capable and defaults its claim to the whole workspace; nothing on the surface said read-only roles claim no scope. The roster text, the type enum text and the tool description now say that general claims the parent workspace and that explore/planner/reviewer/advisor never contend. The spawn-time refusal appends the read-only recovery path (spawn_claim_error_with_guidance); the ledger message shared with action=claim keeps its writer-only wording. No authority change: write_authority stays unadvertised per codewhale-hq#5426/codewhale-hq#5435.

2. A cancelled child kept its session name

After action=cancel and after the child's cancelled completion sentinel had been delivered, a same-name re-launch was refused with "already in use ... (status: cancelled) ... Wait for its completion event". The spawn check reserved a name for any same-session agent regardless of status.

A name is now held only by a live child — running, or interrupted with a wakeable checkpoint (session_name_is_reserved); completed/failed/cancelled/budget-exhausted holders release it. resolve_agent_ref_inner prefers the live holder, then the most recently started settled one, so a retried name resolves to the retry instead of reading as ambiguous; two live holders (only possible from a hydrated older ledger) still error "ambiguous".

3. wait promised a bound the model could not reach

The until text said "default 30s, max 120s" while timeout_secs was parse-accepted only. The parent issued eight ~30 s wait(until=all) calls, each answered with "Do not poll — wait again". timeout_secs is advertised again for action=wait (integer, range and text tied to SUBAGENT_WAIT_{MIN,DEFAULT,MAX}_TIMEOUT_SECS); the timeout alias stays parse-accepted. The advertised surface is now 14 fields; docs/SUBAGENTS.md updated. The tool description was trimmed to stay inside its 1024-token budget: the duplicated "In Operate" write-scope sentence, and the detached clause tightened from "use detached=true only for independent or long work" to "detached=true marks work that must outlive the active turn" (same guidance direction, fewer tokens).

Round 2 (review follow-ups, 729b17c)

A fresh 12-lane review found one code defect and a handful of gaps; all fixed here:

  • resolve: the live-name filter now excludes prior-boot records, matching the spawn gate. Hydration rewrites a child that was Running at shutdown into a prior-boot Interrupted record; the filter counted it, so after a same-name retry settled, bare status/followup/wait silently targeted the stale ghost instead of the retry. A lone ghost still resolves (resume_from keeps working) via the newest-settled fallback. Pinned by session_name_resolution_ignores_prior_boot_interrupted_ghost; mutation-verified (dropping the boot filter reddens exactly that test).
  • spawn refusal: advice per holder status. An interrupted child is already terminal — its completion event has fired and cancel only touches Running children — so the refusal now says "resume it with action=followup ... or open a new agent with a different name"; a running holder keeps the wait/cancel advice. Pinned by the updated interrupted test (asserts "cancel it" is absent) plus a new running-holder test; mutation-verified (collapsing the per-status advice reddens exactly the interrupted test).
  • text: implement now discloses that it claims the workspace under the same rules as general; custom discloses its read-only default without explicit write authority; test joins the never-contending role list in the tool description. The 1024-token budget still passes.
  • pins: the ledger-prefix + read-only-guidance contract is asserted end-to-end in shared_write_claim_is_registered_before_parallel_launch_and_manifested, and the timeout alias parse is pinned by wait_timeout_alias_stays_parse_accepted.
  • docs: docs/FLEET.md points at the 14-field surface; docs/zh_hans/SUBAGENTS.md syncs the count, the field list and the lifecycle-extras line, and gains the timeout_secs paragraph; docs/SUBAGENTS.md aligns the name-resolution section with the boot-scoped behavior and the parked-holder advice; the stale agent tool: simplify the 32-field schema so models stop erroring on it codewhale-hq/Codewhale#5324 comment in the schema test is corrected.

Round 3 (review follow-ups, 596dc1f)

A fresh 9-lane review confirmed all three fixes root-caused and complete and found one test-contract defect plus polish; fixed here:

  • the alias pin was false: wait_timeout_alias_stays_parse_accepted called a copy of the production key list, so dropping the documented timeout alias from the real read left the whole suite green (verified by mutation: full --lib run, zero red). The read now lives in wait_timeout_secs — called by the until=completion arm and pinned directly by the test; re-running the alias-drop mutation reddens exactly that test.
  • the live-holder predicate is spelled once: is_live_session_name_holder is now the single liveness filter behind both the spawn gate and name resolution, so the boot scope cannot drift between them again (the drift was the round-1 resolve defect; mutation re-verified: dropping it reddens exactly the ghost test).
  • WRITE_SCOPE_CONTENTION_PREFIX moved to coord/ledger.rs beside the format string it matches — one literal, imported by the spawn path.
  • The at-most-one-holder invariant is documented at reserved_session_name_holder; docs/zh_hans/SUBAGENTS.md gains the session-names section and the "为 action=wait" wording fix.

Round-3 checks: cargo fmt --check and clippy (CONTRIBUTING allow list) clean; targeted tools::subagent fleet:: tools::registry prompts:: 1122/0 and core::engine 632/0; full --lib 12240 passed with only the two declared load-timing flakes, each passing alone; both mutations re-run red/green on the new head.

Round 4 (review follow-ups, 2318a05)

A fresh 6-lane round-3 review confirmed all three fixes root-caused and complete and found two text defects plus two hazards this PR widens; all four are fixed here:

  • zh cross-reference: docs/zh_hans/FLEET.md still pointed at the "对外公布的 12 字段列表" while this PR moved the surface to 14 (its own zh SUBAGENTS.md update already says 14). Now 14.
  • guidance role list: the spawn-time read-only guidance omitted test while the tool description and both docs include it. Now aligned, with the unit test's role list.
  • id-shaped session names: validate_session_name accepts id-shaped strings and an unnamed child takes session_name = id, so a retry can hold a settled sibling's agent id as its name — the exact-id fast path then let the settled record shadow the live child (with silent no-op cancel/interrupt). Name matching now governs whenever the ref is any child's session name; the fast path applies only to refs that name no child. Pinned by id_shaped_session_name_resolves_by_the_name_rules_not_the_settled_id (mutation-verified: ungating the fast path reddens exactly it).
  • lenient wait reads: the until=all/until=activity arms read timeout_secs/timeout with a type-lenient inline get().and_then(as_u64), so a wrong-typed value silently became the 30s default while the completion face hard-errored. All three faces now share wait_timeout_secs, so a malformed advertised field errors identically everywhere. Pinned by wait_rejects_a_malformed_timeout_on_every_face_like_completion (mutation-verified), and the wait-bound constants cross-pin now covers MIN too.
  • docs: docs/SUBAGENTS.md and docs/zh_hans/SUBAGENTS.md state the checkpoint caveat, the per-status refusal advice and the id-vs-name resolution rule; zh aligns the claim/boot terminology with the file's conventions (认领/启动).

Round-4 checks: cargo fmt --check clean; clippy (CONTRIBUTING allow list) clean for this change — the pre-existing cloned_ref_to_slice_refs at repo_law.rs:638 is unchanged; targeted tools::subagent fleet:: tools::registry prompts:: 1124/0; full --lib 12233 passed with 3 load-timing flakes in the untouched remote_control module (each passes alone; the module passes 77/77 alone).

Checked and left alone

  • planner children reporting bash refused date: bounded read-only shell allowlist by design (command_safety.rs).
  • summary_location: "previous_line" on a cancelled child's sentinel: cosmetic.

Testing

Local toolchain is stable 1.99.1; the hot target of another checkout was reused via CARGO_TARGET_DIR.

  • cargo fmt --all -- --check
  • cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings + the CONTRIBUTING allow list: clean for this change. Pre-existing on pinvou3-clean with 1.99 clippy and not touched here: too_many_arguments ×5 (context_report.rs, engine.rs, turn_loop.rs, lib.rs), collapsible_if (turn_loop.rs:5407), cloned_ref_to_slice_refs (repo_law.rs:638).
  • cargo test -p codewhale-tui --locked --lib: 12236 passed, 2 failed, 13 ignored — both failures (client::operation_tests::forkguard_model_operation_auxiliary_retry_keys, runtime_api::tests::omitted_runtime_models_use_the_active_provider_default, "deadline has elapsed") pass when re-run alone; load-timing flakes while a second full suite ran on the same machine, unrelated modules.
  • Targeted: tools::subagent + fleet:: + tools::registry + prompts:: 1119/0; core::engine 632/0. After round 2: tools::subagent 597/0 (three new tests: ghost resolution, running-holder advice, timeout-alias pin).
  • Red/green: with the three behaviours mutated back (reservation → always true, timeout_secs removed from the schema, guidance → identity) exactly these 5 go red while 9 neighbours stay green. Round-2 mutations: dropping the resolve boot filter reddens exactly session_name_resolution_ignores_prior_boot_interrupted_ghost; collapsing the per-status advice reddens exactly spawn_session_name_still_held_by_interrupted_same_session_child. spawn_session_name_released_once_same_session_holder_settles, session_name_resolution_prefers_live_holder_then_newest_settled, spawn_contention_error_names_the_read_only_path, subagent_tool_schemas_advertise_real_type_and_role_vocabulary, wait_schema_text_discloses_timeout_bound_and_timed_out_receipt.

Checklist

  • This PR adds a new layer/module/abstraction — it names or deletes the layer it replaces (no new layer; two private helpers on SubAgentManager and one free function)
  • Updated docs or comments as needed (docs/SUBAGENTS.md, schema doc comment)
  • 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 only when its CodeWhale gitlink advances to the squash of this PR on pinvou3-clean (the public-submodule gate requires gitlink = maintenance-branch head), together with the scripts/fork-guard.sh EXPECTED_HEAD/EXPECTED_COMMITS and docs/fork-modifications.md register update that is already outstanding for #115–#117.

A four-child research fan-out run through the model-facing `agent` tool on
2026-10-23 hit three avoidable walls in a row:

1. Three of four parallel `type=general` starts were refused with
   "write-scope contention ... (roots: ["."], files: [], contracts: [])".
   Each general defaulted to the whole workspace although none of them
   needed to write, and nothing on the surface said that read-only roles
   claim no scope. The roster text, the `type` enum text and the tool
   description now say that `general` claims the parent workspace and
   that explore/planner/reviewer/advisor never contend; the spawn-time
   refusal names that read-only path (the ledger text, shared with
   `action=claim`, keeps its writer-only wording).

2. After cancelling the first child and receiving its completion
   sentinel, re-launching under the same name was refused with "already
   in use ... (status: cancelled) ... wait for its completion event".
   A session name is now held only by a live child (running, or
   interrupted with a wakeable checkpoint); completed, failed, cancelled
   and budget-exhausted holders release it. Name resolution prefers the
   live holder, then the most recently started settled one, so a retried
   name resolves to the retry instead of reading as ambiguous. Two live
   holders (possible only from a hydrated older ledger) stay ambiguous.

3. The `until` text promised "default 30s, max 120s" but the schema had
   no field to set it: the parent repeated eight 30-second waits while
   every receipt said "do not poll". `timeout_secs` is advertised again
   for `action=wait` (range and text tied to the runtime constants); the
   `timeout` alias stays parse-accepted. The tool description was trimmed
   to stay inside its 1024-token budget.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Fresh round-1 review (from scratch, 12 review lanes + independent verification)

Verdict: request-changes grade — one code MAJOR and two doc regressions that this PR itself creates must be fixed; everything else verified clean. Posted as a plain comment because GitHub does not allow a formal review verdict on one's own PR.

Rebase: the branch is already based on the current pinjou3-clean tip (ea9fdc90f); no rebase needed.

Verified good — all three claims are real, root-caused, and their fixes land

  1. Read-only fan-out guidance. The ledger refusal is produced at coord/ledger.rs:542-549; spawn_claim_error_with_guidance runs as the first statement of the spawn Err arm (mod.rs:6637) while the string is still the raw ledger message — the "Failed to spawn sub-agent: " wrapper is added later (mod.rs:9747), so the prefix check is live, not dead code. The advertised remedies work against the ledger's real predicate: roots overlap by path containment (ledger.rs:84-92), disjoint write_roots genuinely co-register (disjoint_roots_no_overlap pin, ledger.rs:1326), worktree=true claims are inert in both directions (ledger.rs:518/523). Read-only roles and prompt-only starts truly register no claim (mod.rs:9599-9605, forced ReadOnly at mod.rs:13148-13152); action=claim correctly keeps the writer-only wording (coord.rs:1263). write_authority stays unadvertised.

  2. Name release on settle. The reserved set is exhaustive against the six-variant enum (mod.rs:470-481); spawn_background_with_assignment_options is the only consumer of options.name, so no spawn path bypasses the check; check-then-insert is await-free under the write guard; hydration re-derives a crashed Running into a prior-boot Interrupted that the boot filter excludes from reservation (mod.rs:4526-4530, 4202-4207). All four settled statuses release the name via the real spawn path.

  3. timeout_secs re-advertisement. Pure surface change: the parse+clamp (both keys, same constants) is byte-identical at base. Advertised min/max/default equal the runtime clamp exactly; gating pattern matches until. The description trim was necessary and information-preserving — without it the description estimates 1060 tokens against the 1024 budget test, and the trimmed "root start" clause had no distinct semantics (the write-scope default is depth-blind, mod.rs:13134-13143).

Testing re-verified independently

  • Targeted tools::subagent: 594 passed / 0 failed on the PR head.
  • My own mutation replay (not the author's): reservation→always-true reddens exactly spawn_session_name_released_once_same_session_holder_settles + session_name_resolution_prefers_live_holder_then_newest_settled; schema-field rename reddens exactly the two schema tests; guidance-prefix mutation reddens exactly spawn_contention_error_names_the_read_only_path. 9 neighbours stay green in every run — matches the body's red/green claim precisely.
  • cargo fmt --check clean. Clippy with the CONTRIBUTING allow list is clean for this change except the pre-existing, PR-disclosed cloned_ref_to_slice_refs at repo_law.rs:638 (byte-identical at base; file untouched). CI rollup green.

MAJOR-1 — resolve's live filter lets a prior-boot hydrated ghost silently hijack name resolution

resolve_agent_ref_inner has a scoping asymmetry with reserved_session_name_holder: the spawn gate is boot-scoped (excludes is_from_prior_session, mod.rs:4202-4207), but resolve's name filter checks only owner_session_id (mod.rs:6991-6999) and its live filter checks only status (mod.rs:7007-7009). Hydration rewrites persisted Running → Interrupted(RESTART), keeps owner_session_id (mod.rs:4576), backdates started_at (mod.rs:4532), and inserts into the same self.agents map (mod.rs:4583).

Sequence: resume a conversation after restart where child x was running (now a hydrated prior-boot Interrupted ghost that passes the owner filter) → respawn x (allowed — spawn excludes prior boot) → while the retry runs, resolution is loudly "ambiguous" (matches base) → once the retry settles, the live filter finds only the ghost, so status x / peek x / followup x / targeted wait x silently target the stale ghost for up to 1h of retention. Base behavior for the same sequence was a loud "ambiguous; use an agent_id" — this PR converts a transient loud failure into a silent wrong-target one, in exactly the behavior the PR is about.

Minimal validated fix: add && !self.is_from_prior_session(agent) to the live filter only (mod.rs:7007-7009). Safe: ghost-only resolution (e.g. resume_from before any retry) still works through the newest-settled fallback, and the backdated started_at guarantees the retry wins the fallback. The PR's own resolution test misses this because its seed helper inserts current-boot agents only (tests.rs:9661-9689).

MAJOR-2 — the PR falsifies two of its own cross-references (docs)

  • docs/FLEET.md:237 still says "see docs/SUBAGENTS.md for the advertised 13-field list" — the PR moves the surface to 14 and updated SUBAGENTS.md and the mod.rs doc comment, but missed this pointer. One-word fix.
  • docs/zh_hans/SUBAGENTS.md:209 still lists timeout_secs(wait)under 解析接受但未公布 — the direct Chinese opposite of what this PR just did to the English doc. (The zh file's 12-field count drift at :197 predates this PR — the T7 12→13 change also skipped it — and can ride along or be a tracked follow-up.)

MINORs

  • m1 — the reworded refusal's "cancel it" is a silent no-op for Interrupted holders. cancel_agent early-returns for non-Running (mod.rs:5466-5469), and an Interrupted child's terminal sentinel already fired, so "wait for its completion event" is equally dead there; the un-wakeable flavors are real (interrupt fallback checkpoint has empty messages mod.rs:6055-6060; heartbeat orphan mod.rs:7285-7290; resume requires continuable + non-empty). Retention (1h) bounds it, and the new test pins the misleading text. Suggest a per-status reword: for Interrupted advertise "resume it with followup, or use a different name". Also the PR's own doc comment says "interrupted with a wakeable checkpoint" but the code reserves any Interrupted(_) — align one or the other (docs/SUBAGENTS.md inherits the same phrasing).
  • m2 — nothing pins the ledger contention message format end-to-end, so an upstream reword of "write-scope contention" would silently disable the new guidance (the hand-copied fixture in the unit test would not notice). One contains("Read-only children never contend") assertion in the existing e2e contention test (tests.rs:9963) would make the drift loud.
  • m3 — the timeout alias is parse-accepted but pinned by no test (neither schema-test list nor agent_tool_unadvertised_fields_remain_parse_accepted exercises it); docs now pin it textually only.
  • m4 — stale comment tests.rs:5883-5887 still says timeout_secs "moved off the advertised schema with agent tool: simplify the 32-field schema so models stop erroring on it codewhale-hq/Codewhale#5324", directly above a test this PR updated to the contrary.
  • m5 — disclosure asymmetry in the very sentence-block the PR edited: the general clause now discloses the whole-workspace claim, but implement (also write-capable with the same ["."] default, mod.rs:9883) does not; likewise custom's default read-only behavior is disclosed nowhere model-facing, and the read-only lists omit test.
  • m6 — the description trim did slightly more than disclosed: "use detached=true only for independent or long work" became "detached=true marks work that must outlive the active turn" (budget-driven; direction preserved). Worth a word in the body since the budget arithmetic is the real reason.

Refuted during adjudication (for the record)

A reported concern that a same-process conversation boundary leaves the closed conversation's Interrupted children phantom-reserving names was refuted: finalize_session_close_inner drops finalized agents from the in-memory roster (mod.rs:4722-4724), freeing their names at the boundary. The residual variant (a child already Interrupted before the boundary blocks its name in the next conversation of the same process, unaddressable, until 1h retention) is pre-existing and byte-identical at base — not this PR's defect, worth a follow-up issue at most.

Bottom line

The PR is genuinely useful, tightly scoped (no smuggled hunks — every hunk serves a claimed fix), well-tested, and its three claims survive adversarial verification including live error-path tracing and independent mutation replay. Fix MAJOR-1 (a validated one-liner) and the two doc points, ideally sweep m1–m4 in the same round, and this is mergeable.

resolve: scope the live-name filter by boot like the spawn gate. Hydration
rewrites a child that was Running at shutdown into a prior-boot Interrupted
record; the live filter counted it, so once a same-name retry settled, bare
status/peek/followup/wait silently targeted the stale ghost instead of the
retry. A lone ghost still resolves (resume_from keeps reaching the
pre-restart checkpoint) via the newest-settled fallback.

spawn refusal: advice per holder status. An interrupted child is already
terminal — its completion event fired and cancel only touches Running — so
the refusal now points at followup (checkpoint permitting) or a different
name; a running holder keeps the wait/cancel advice.

text: the type vocabulary and the tool description disclose that implement
claims the workspace under the same rules as general and that custom starts
read-only without an explicit write authority; test joins the never-
contending role list. The timeout alias stays parse-accepted and is now
pinned, as is the ledger-prefix + read-only-guidance contract end-to-end.

docs: FLEET.md points at the 14-field surface; zh_hans/SUBAGENTS.md syncs
the count, the field list and the lifecycle-extras line, and gains the
timeout_secs paragraph; SUBAGENTS.md aligns the name-resolution section
with the boot-scoped behavior and the parked-holder advice.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round 2 pushed as 729b17c — every round-1 finding is fixed:

  • MAJOR (ghost hijack in resolve): the live-name filter in resolve_agent_ref_inner now excludes prior-boot records, matching the spawn gate's boot scoping. A hydrated prior-boot Interrupted ghost can no longer outrank the current boot's child; a lone ghost still resolves via the newest-settled fallback, so resume_from keeps working. New pin: session_name_resolution_ignores_prior_boot_interrupted_ghost (lone ghost / live retry / settled retry, all three branches).
  • Doc regressions: docs/FLEET.md:237 now says 14-field; docs/zh_hans/SUBAGENTS.md syncs the count (12→14), the field list (profile_query + timeout_secs), the lifecycle-extras line (drops timeout_secs, adds the timeout alias), and gains the timeout_secs paragraph.
  • m1: the name-refusal advice is now per status — an interrupted holder gets "resume it with action=followup … or open a new agent with a different name" (cancel is a silent no-op off Running, and the completion event already fired); a running holder keeps wait/cancel and is pinned by a new test. The session_name_is_reserved doc comment and docs/SUBAGENTS.md no longer claim "wakeable checkpoint" — the code reserves any Interrupted.
  • m2: shared_write_claim_is_registered_before_parallel_launch_and_manifested now asserts the refusal starts with the ledger prefix and carries "Read-only children never contend" — a reword on either side fails loudly.
  • m3: wait_timeout_alias_stays_parse_accepted pins the timeout alias read.
  • m4: the stale agent tool: simplify the 32-field schema so models stop erroring on it codewhale-hq/Codewhale#5324 comment above the schema test is corrected.
  • m5: implement now discloses workspace-claim parity with general, custom discloses its read-only default, and test joins the never-contending list in the tool description (budget still passes).
  • m6 (disclosure only, in the body): the description trim also tightened the detached clause — now stated explicitly in the round-2 body section.

Not fixed here (pre-existing, disclosed in the round-1 review): a child already interrupted before a same-process conversation boundary keeps its name unaddressable in the next conversation until the 1h retention sweep — identical at base, needs its own decision (owner-scoped reservation vs finalize-and-drop), tracked as a follow-up.

Verification on 729b17c: cargo fmt --all -- --check clean; cargo test -p codewhale-tui --locked --lib tools::subagent 597/0; clippy with the CONTRIBUTING allow list clean except the pre-existing, PR-disclosed cloned_ref_to_slice_refs at repo_law.rs:638; both round-2 mutations red/green exact (boot-filter drop → ghost test only; advice collapse → interrupted test only).

Round-2 fresh review found one test-contract defect and a few
polish items; all fixed here.

- alias pin: wait_timeout_alias_stays_parse_accepted called a copy
  of the production key list, so dropping the documented 'timeout'
  alias from the real read left it green (verified by mutation:
  full suite green under the regression). The read now lives in
  wait_timeout_secs, which the until=completion arm calls and the
  test pins.
- live-holder predicate spelled once: is_live_session_name_holder
  is now the single liveness filter behind both the spawn gate and
  name resolution, so the boot scope cannot drift between them
  again (the drift was the round-1 defect).
- WRITE_SCOPE_CONTENTION_PREFIX moved next to the ledger format
  string it matches, imported by the spawn path — one literal.
- at-most-one-holder invariant documented at the lookup helper;
  zh_hans gains the session-names section and the 'for action=wait'
  wording fix.

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-2 fresh review — full re-review from scratch (9 lanes + mutation chain), base pinvou3-clean @ ea9fdc90f (PR was 0 behind; rebase no-op). Head reviewed: 729b17c0d; one MAJOR found, fixed and pushed as 596dc1f3d.

Claims — all three root-caused and complete

  • Claim 1 (read-only fan-out contention): base root cause confirmed (validate_spawn_write_contract defaults a write-capable request with no scope to write_roots = ["."], two such claims overlap → the ledger's write-scope contention refusal). Fix verified end-to-end: prefix match is byte-exact against coord/ledger.rs, the spawn arm is the only wired call site, read-only roles/custom-without-authority/prompt-only starts provably never register a contending claim, action=claim keeps the writer-only wording (defensible — its caller is already a writer). The worktree advice was attack-verified for real: worktree=true claims are excluded from contention in both directions (register_claim skips isolated incomers and isolated existing records), so "or worktree=true" is true advice.
  • Claim 2 (settled name release + resolution): base refused same-name spawn for any same-session status; head reserves only Running|Interrupted of the current boot, resolve prefers live → newest-settled, per-status advice matches what cancel/followup actually do (cancel is a no-op on non-Running; interrupt fires the completion event; double-followup by name idempotently forwards to the resumed target). The two safety invariants were attacked hard and hold: (a) at most one current-boot live holder per name can exist — the spawn fn is sync &mut self, gate precedes both inserts, no writer ever flips a settled status back to Running; (b) the newest-settled fallback can never be hijacked by a prior-boot ghost — hydration runs only at manager construction and reconstructs ghost started_at ≤ hydration time < any current-boot spawn (duration_ms is age-at-persist, one uniform shift per payload, all-Instant chain).
  • Claim 3 (timeout_secs re-advertised): base really had it parse-accepted only while until promised the bound; all three wait arms (completion/all/activity) honor the same 1..=120 clamp the schema now advertises; constants single-source; 14-field surface counted and consistent across schema/doc-comment/en/zh/FLEET/tests; description trim verified as pure duplication removal, static token estimate 1017 ≤ 1024; KV-prefix cost is the disclosed one-time-per-session refill.

Scope: every hunk maps to a claim or a disclosed intentional item; no smuggled changes, nothing to split; commit hygiene and DCO clean; PR body honest against the diff.

MAJOR (fixed in 596dc1f)

  1. The timeout alias pin was false. wait_timeout_alias_stays_parse_accepted reconstructed the production read (parse_optional_u64(&input, &["timeout_secs", "timeout"]) inline in the test), while the real read — and the thing the docs declare a compat contract — lives at the until=completion call site. Mutation proof: deleting "timeout" from the production key list left the entire --lib suite green (the only failure was the declared forkguard timing flake), so the pin could never catch the regression it exists for, and dropped transcripts would silently get the 30 s default. Fixed by extracting wait_timeout_secs (the production read) which the test now calls; the same mutation now reddens exactly that test. Related asymmetry noted for follow-up: the until=all/activity arms in coord.rs still read the alias via a type-lenient inline get().or_else() — different mechanism, same contract.

Polish (fixed in the same commit)

  • The live-holder predicate was spelled twice (gate helper + resolve inline filter) — the exact drift class that produced the round-1 resolve defect. Now one predicate, is_live_session_name_holder, consumed by both; mutation re-verified (boot-scope drop → exactly the ghost test red).
  • WRITE_SCOPE_CONTENTION_PREFIX moved to coord/ledger.rs next to the format! it matches (one literal; e2e pin unchanged and still green).
  • At-most-one-holder invariant documented at reserved_session_name_holder; zh_hans gains the session-names section (was en-only) and "因 action=wait" → "为 action=wait".

Follow-ups (not fixed here, by scope)

  1. Id-shaped session names: unnamed children take session_name = id (agent_<8hex>), and validate_session_name doesn't reject that pattern. With settled names now free, a new child named exactly a settled sibling's id is captured by the exact-id fast path for up to the 1 h retention window. Pre-existing class (cross-boot at base) but the release change widens the same-boot window; cheap hardening: reject ^agent_[0-9a-f]{8}$ names, or let the fast path yield when the probed record's session_name differs from the ref.
  2. cancel/interrupt by name on a newest-settled child returns a success-shaped no-op receipt; a "already settled" note when the resolved target isn't Running would help stale parents.
  3. An Interrupted child's name stays reserved after a successful followup resume (resume runs name-less); the refusal could mention resume_targets ("already resumed as agent_X") instead of only offering followup-again.
  4. The standalone agents/wait face still advertises no min/max and reads the alias leniently — the same problem class this PR fixes on the agent face; unify in a follow-up (the two faces' constants are already cross-pinned).
  5. Fleet preflight/registration contention refusals bypass the read-only guidance (operator-facing surface; deliberate scoping, recorded here so it isn't rediscovered).

Verification

  • fmt + clippy (-D warnings, CONTRIBUTING allow list) clean on both 729b17c0d and 596dc1f3d; CI green on 729b17c0d (all 13 checks).
  • Targeted: tools::subagent fleet:: tools::registry prompts:: 1122/0, core::engine 632/0; full --lib 12240 passed with only the two PR-declared load-timing flakes, each passing 3/3 alone; 13 named PR tests individually green.
  • Mutation chain (own worktree, isolated): reservation→always-true reds the release + resolution-preference tests; drop resolve boot filter reds exactly the ghost test; collapse per-status advice reds exactly the interrupted-advice test; remove schema field reds exactly the wait-schema test; drop production alias read — suite green at 729b17c0d (the MAJOR), alias-test-only red at 596dc1f3d (the fix).
  • Performance: no new allocation, loop, or per-turn cost on any touched path; schema build is memoised per registry, scans stay O(roster) once per named tool call. Clean.

Environment note, unrelated to this PR: runtime_api::tests::events_endpoint_respects_since_seq_cursor deterministically stack-overflows under the default 8 MiB debug stack on this machine (passes with RUST_MIN_STACK=16 MiB; CI green; file untouched here) — worth its own hygiene look.

- resolve: the exact-id fast path now yields to the name rules whenever
  the ref is any child's session name (id-shaped names are legal and
  unnamed children take session_name = id), so a settled record can no
  longer shadow the live — or newest-settled — child that name means.
- wait: until=all and until=activity share wait_timeout_secs with the
  completion face, so a malformed timeout_secs is a hard error on every
  face instead of a silent 30s default; the wait-bound constants test
  also cross-pins MIN now.
- spawn refusal guidance names test alongside the other read-only roles,
  matching the tool description and both docs.
- docs: zh_hans/FLEET.md points at the 14-field surface (was stale at
  12); zh_hans/SUBAGENTS.md aligns the claim terminology (认领) and boot
  wording (启动) with the file's conventions and gains the per-status
  refusal advice and id-vs-name rules; docs/SUBAGENTS.md states the
  checkpoint caveat, the refusal advice and the id-vs-name rule too.

New pins: id_shaped_session_name_resolves_by_the_name_rules_not_the_
settled_id, wait_rejects_a_malformed_timeout_on_every_face_like_
completion (both mutation-verified red on the reverted behavior).

Signed-off-by: asto <asto18089@126.com>
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-4 fresh review — from scratch, full re-verification (base unchanged)

Reviewed fresh from scratch at head 2318a056b45 (4 commits, base pinjou3-clean @ ea9fdc90f1c — the base has not moved, so this head is 0 behind and no rebase is needed; GitHub reports mergeable clean and all 14 CI checks green). This round supersedes nothing that is still open: there are no blocking findings. The PR is ready to merge.

What was verified

The head tree was reconstructed offline and verified byte-exact against the server tree (2043/2043 blobs), then reviewed lane by lane: claims-vs-implementation, feature bugs, edge cases, elegance/reuse, smuggled-changes audit, performance, and tests.

All three fixes are root-caused and complete. Every premise was checked in code, not just in tests:

  • Read-only fan-out disclosure (wall 1). The workspace-claim disclosure is present at all three surfaces a model reads before picking a type — the roster (FleetRole::Worker.description, rendered by action=roster), the type enum text, and the tool description. The spawn refusal appends the read-only guidance keyed on WRITE_SCOPE_CONTENTION_PREFIX, and action=claim correctly keeps the writer-only ledger wording (the claim path never routes through spawn_claim_error_with_guidance). The "never contend" claim is enforced, not just written: worker_coordination_claim builds no claim when the role's ceiling is write = false.
  • Settled names are freed (wall 2). Reservation is now liveness-scoped (Running | Interrupted, prior-boot excluded) at the single spawn gate, and resolution prefers the live holder, then the newest settled. The per-status refusal advice is truthful: cancel/interrupt are silent no-ops off Running (verified in cancel_agent/interrupt_child), and action=followup on an interrupted child is a real resume path (resume_from_checkpoint_with_policy), so "checkpoint permitting" is accurate.
  • The advertised wait bound is reachable (wall 3). timeout_secs is advertised with minimum/maximum tied to SUBAGENT_WAIT_{MIN,MAX}_TIMEOUT_SECS, the timeout alias stays parse-accepted, and all three wait faces (completion, all, activity) share one strict read (wait_timeout_secs); I traced dispatch_wait's routing to confirm no face was missed.

No smuggled changes. Every hunk in the 9-file delta maps to a claimed fix area; nothing touches Cargo.lock, the authority clamps, or unrelated modules. Commit messages match this repo's convention and DCO passes.

Edge cases probed, all sound:

  • No two-live-holder hole via resume: resumed children spawn unnamed (name: None — the old name stays owned by the terminal record), so a resume can never produce two live holders of one name.
  • No reservation gap: SubAgent::new registers children as Running, and the gate plus insertion run in one synchronous &mut self call.
  • Hydration backdates started_at (now − runtime), so a prior-boot ghost can never beat the current boot's settled child in the newest-settled fallback.
  • A live caller's own agent id cannot be name-shadowed: the gate refuses spawning a name that a live child holds.
  • Retention age-eviction bounds the maps the new scans walk; the resolve path allocates the same order of memory as before. No performance regressions found.
  • The strict timeout read changes no clamping semantics; out-of-range integers still clamp exactly as the two faces did before.

Elegance: the contention prefix lives beside its producer as a single literal, the liveness predicate is spelled once for both the gate and resolution (the drift that caused the round-1 defect is now structurally impossible), and the en/zh docs agree on the 14-field surface. Nothing reinvents existing helpers.

Independent verification (all rerun locally on a byte-exact reconstruction)

  • cargo fmt --all -- --check: clean.
  • Clippy with the CI command (cargo clippy --workspace --all-features --locked -- -D warnings + the allow list): clean — the only report is the pre-existing cloned_ref_to_slice_refs at repo_law.rs:638, untouched here.
  • Targeted tools::subagent fleet:: tools::registry prompts::: 1124 passed / 0 failed.
  • Full --lib with the CI env (RUST_MIN_STACK=16777216, test debug off): 12233 passed / 3 failed — exactly the three declared remote_control load-timing flakes, each passing alone and the module passing 77/77 in isolation (both rerun). Without RUST_MIN_STACK the suite aborts on an unrelated stack overflow, which is an environment artifact, not a finding.
  • Six mutations, each reddens exactly its pin and nothing else: ungating the exact-id fast path → id_shaped_session_name_resolves_by_the_name_rules_not_the_settled_id; reverting one wait face to the lenient read → wait_rejects_a_malformed_timeout_on_every_face_like_completion; dropping the timeout alias → wait_timeout_alias_stays_parse_accepted; dropping the resolve-side boot scope → session_name_resolution_ignores_prior_boot_interrupted_ghost while spawn_session_name_held_by_prior_session_agent_does_not_collide stays green; removing test from the guidance role list → spawn_contention_error_names_the_read_only_path; making reservation unconditional → spawn_session_name_released_once_same_session_holder_settles.

Non-blocking notes (no action required)

  1. docs/design/CLAUDE_CODE_PARITY.md:172 still says "12-field agent tool" — it is an event-history list describing what agent tool: simplify the 32-field schema so models stop erroring on it codewhale-hq/Codewhale#5324 did at the time, not a live pointer, so leaving it is defensible; it will read stale once someone consults it for the current count.
  2. The new docs paragraphs enumerate the settled states as prose "budget-exhausted" while the wire string is budget_exhausted (spelled correctly elsewhere in the same file). Prose is fine; just noting the dual spelling.
  3. The separate agents/wait tool schema carries inline 1/120 literals instead of the constants (pre-existing; values agree, and the agent-tool side is now cross-pinned).
  4. A settled child whose id-shaped string later becomes a newer child's name is reachable only through the name rules from then on (documented tradeoff in docs/SUBAGENTS.md; the gate makes it impossible to shadow a live holder).

Verdict

Useful, root-caused, complete, elegant, free of smuggled changes, and free of known feature or performance bugs. Recommend merge. The parent-repo follow-up (CodeWhale gitlink + fork-guard.sh register for #115–#117) remains correctly out of this PR's scope.

Review basis: full delta re-read at head 2318a056b45; all tests, clippy, and mutations executed locally against a reconstruction verified byte-exact (2043/2043 blobs) against the server head tree.

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.

1 participant