Repository navigation
Conversation
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>
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 Verified good — all three claims are real, root-caused, and their fixes land
Testing re-verified independently
MAJOR-1 — resolve's live filter lets a prior-boot hydrated ghost silently hijack name resolution
Sequence: resume a conversation after restart where child Minimal validated fix: add MAJOR-2 — the PR falsifies two of its own cross-references (docs)
MINORs
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: Bottom lineThe 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>
|
Round 2 pushed as 729b17c — every round-1 finding is fixed:
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: |
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>
|
Round-2 fresh review — full re-review from scratch (9 lanes + mutation chain), base Claims — all three root-caused and complete
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)
Polish (fixed in the same commit)
Follow-ups (not fixed here, by scope)
Verification
Environment note, unrelated to this PR: |
- 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>
Round-4 fresh review — from scratch, full re-verification (base unchanged)Reviewed fresh from scratch at head What was verifiedThe 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:
No smuggled changes. Every hunk in the 9-file delta maps to a claimed fix area; nothing touches Edge cases probed, all sound:
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)
Non-blocking notes (no action required)
VerdictUseful, 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 + Review basis: full delta re-read at head |
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-facingagenttool — hit three avoidable walls in sequence. Transcript evidence is quoted verbatim; each has a red/green test.1. Parallel
type=generalstarts collide although none of them writesThree of four
action=startcalls in one block were refused:generalis write-capable and defaults its claim to the whole workspace; nothing on the surface said read-only roles claim no scope. The roster text, thetypeenum text and the tool description now say thatgeneralclaims the parent workspace and thatexplore/planner/reviewer/advisornever contend. The spawn-time refusal appends the read-only recovery path (spawn_claim_error_with_guidance); the ledger message shared withaction=claimkeeps its writer-only wording. No authority change:write_authoritystays unadvertised per codewhale-hq#5426/codewhale-hq#5435.2. A cancelled child kept its session name
After
action=canceland after the child'scancelledcompletion 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_innerprefers 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.
waitpromised a bound the model could not reachThe
untiltext said "default 30s, max 120s" whiletimeout_secswas parse-accepted only. The parent issued eight ~30 swait(until=all)calls, each answered with "Do not poll — wait again".timeout_secsis advertised again foraction=wait(integer, range and text tied toSUBAGENT_WAIT_{MIN,DEFAULT,MAX}_TIMEOUT_SECS); thetimeoutalias stays parse-accepted. The advertised surface is now 14 fields;docs/SUBAGENTS.mdupdated. 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:
Interruptedrecord; the filter counted it, so after a same-name retry settled, barestatus/followup/waitsilently targeted the stale ghost instead of the retry. A lone ghost still resolves (resume_fromkeeps working) via the newest-settled fallback. Pinned bysession_name_resolution_ignores_prior_boot_interrupted_ghost; mutation-verified (dropping the boot filter reddens exactly that test).cancelonly 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).implementnow discloses that it claims the workspace under the same rules asgeneral;customdiscloses its read-only default without explicit write authority;testjoins the never-contending role list in the tool description. The 1024-token budget still passes.shared_write_claim_is_registered_before_parallel_launch_and_manifested, and thetimeoutalias parse is pinned bywait_timeout_alias_stays_parse_accepted.docs/FLEET.mdpoints at the 14-field surface;docs/zh_hans/SUBAGENTS.mdsyncs the count, the field list and the lifecycle-extras line, and gains thetimeout_secsparagraph;docs/SUBAGENTS.mdaligns 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:
wait_timeout_alias_stays_parse_acceptedcalled a copy of the production key list, so dropping the documentedtimeoutalias from the real read left the whole suite green (verified by mutation: full--librun, zero red). The read now lives inwait_timeout_secs— called by theuntil=completionarm and pinned directly by the test; re-running the alias-drop mutation reddens exactly that test.is_live_session_name_holderis 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_PREFIXmoved tocoord/ledger.rsbeside the format string it matches — one literal, imported by the spawn path.reserved_session_name_holder;docs/zh_hans/SUBAGENTS.mdgains the session-names section and the "为action=wait" wording fix.Round-3 checks:
cargo fmt --checkand clippy (CONTRIBUTING allow list) clean; targetedtools::subagent fleet:: tools::registry prompts::1122/0 andcore::engine632/0; full--lib12240 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:
docs/zh_hans/FLEET.mdstill pointed at the "对外公布的 12 字段列表" while this PR moved the surface to 14 (its own zhSUBAGENTS.mdupdate already says 14). Now 14.testwhile the tool description and both docs include it. Now aligned, with the unit test's role list.validate_session_nameaccepts id-shaped strings and an unnamed child takessession_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-opcancel/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 byid_shaped_session_name_resolves_by_the_name_rules_not_the_settled_id(mutation-verified: ungating the fast path reddens exactly it).until=all/until=activityarms readtimeout_secs/timeoutwith a type-lenient inlineget().and_then(as_u64), so a wrong-typed value silently became the 30s default while the completion face hard-errored. All three faces now sharewait_timeout_secs, so a malformed advertised field errors identically everywhere. Pinned bywait_rejects_a_malformed_timeout_on_every_face_like_completion(mutation-verified), and the wait-bound constants cross-pin now covers MIN too.docs/SUBAGENTS.mdanddocs/zh_hans/SUBAGENTS.mdstate 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 --checkclean; clippy (CONTRIBUTING allow list) clean for this change — the pre-existingcloned_ref_to_slice_refsatrepo_law.rs:638is unchanged; targetedtools::subagent fleet:: tools::registry prompts::1124/0; full--lib12233 passed with 3 load-timing flakes in the untouchedremote_controlmodule (each passes alone; the module passes 77/77 alone).Checked and left alone
plannerchildren reportingbashrefuseddate: 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 -- --checkcargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings+ the CONTRIBUTING allow list: clean for this change. Pre-existing onpinvou3-cleanwith 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.tools::subagent+fleet::+tools::registry+prompts::1119/0;core::engine632/0. After round 2:tools::subagent597/0 (three new tests: ghost resolution, running-holder advice, timeout-alias pin).timeout_secsremoved from the schema, guidance → identity) exactly these 5 go red while 9 neighbours stay green. Round-2 mutations: dropping the resolve boot filter reddens exactlysession_name_resolution_ignores_prior_boot_interrupted_ghost; collapsing the per-status advice reddens exactlyspawn_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
SubAgentManagerand one free function)docs/SUBAGENTS.md, schema doc comment)Follow-up (parent repository)
pinvou-agentpicks this up only when its CodeWhale gitlink advances to the squash of this PR onpinvou3-clean(the public-submodule gate requires gitlink = maintenance-branch head), together with thescripts/fork-guard.shEXPECTED_HEAD/EXPECTED_COMMITSanddocs/fork-modifications.mdregister update that is already outstanding for #115–#117.