Repository navigation
feat(pool): create-worker — one command, so the roster cannot be left stale - #4115
qingyun-wu wants to merge 9 commits into
Conversation
qingyun-wu
changed the title
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 1ea558a6. COMMENTED because the authenticated account authored this PR.
[P1] Persist room bindings before compiling the roster (scripts/create-worker.py:79-86). compile_with adds --room only to the temporary dictionary passed to compile_roster; nothing writes the owner declaration in state/bindings.json. The next create/recompile reloads that file, so it silently drops the earlier room. This reproduces even without concurrency, using the actual production compile_with twice:
create A with !one:x: targets_for(!one:x) = [A]
create B with !two:x: targets_for(!one:x) = ['core']
bindings.json exists = False
second roster bindings = {'!two:x': B}
Control: persisting A's declaration before the next compile retains its route. Put durable binding mutation in its shared owner, serialize it with the relevant roster mutation, and test two sequential creates/recompile plus overlapping writers. Worst case is previously bound work executing at the core without the owner changing its destination.
10 command tests and 24 roster, 29 launcher, 54 delivery tests pass, but the current tests inspect only the first compiled snapshot. The parent merge is conflict-free. Inherited #4108 runtime-launch/hold-regression blockers also still apply, and this live-path command needs a real successful create → task → answer witness once those are repaired. Hosted workspace-resolution lint is currently failing; CLA green, other tests still running.
Full child diff, parent interaction, and surrounding code reviewed. Added-path scan passes with a failing forbidden-path control. Merge after #4108 and its parents, with refreshed checks. No contributor branch or live runtime changed.
Reviewed by Qingyun’s Codex automation.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 6a9b275c. COMMENTED because the authenticated account authored this PR. The lint annotation and four added tests are verified; the room-binding persistence blocker from the previous review remains at scripts/create-worker.py:79-86.
Actual production compile_with → targets_for, in a temporary workspace:
create A bound to !one:x: !one:x -> A
create B bound to !two:x: !one:x -> core
state/bindings.json exists: False
control, persist A before creating B: !one:x -> A
The command still changes only the dictionary used for that roster snapshot. The next compilation reloads the unchanged owner declaration and drops the earlier room. Persist the binding through its shared storage owner and serialize the binding/roster read-modify-write; cover sequential creates and overlapping writers. Worst case is previously worker-bound work silently running at core.
14 command tests, 24 roster tests, 29 launcher tests and 54 delivery tests pass. Changing the roster-error exit from 1 to 0 makes exactly the new failure-reporting test fail, so that added coverage is useful but does not cover durable bindings. Full child diff and inherited launcher/roster code inspected; the current parent e9b81ff8 merges cleanly. The inherited launcher still sends only the watcher command, without launching an agent runtime. A real create → task → answer witness remains required after the functional blockers are fixed.
Added-path/root-artifact/comment-cap checks pass with a failing forbidden-path control. CLA and workspace lint are green; coverage and node/shell jobs were still running at inspection. Merge parent-first with refreshed checks; not merge-ready.
Reviewed by Qingyun’s Codex automation.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/feat/spawn-worker-launcher...HEAD, staged and unstaged changes
Summary
skills/worker-pool/scripts/pool_roster.pyLines 141-149 141 raw = json.loads(text)
142 except ValueError as e:
143 raise RosterError(f"roster is not valid JSON, refusing to touch it: {p}: {e}") from e
144 if not isinstance(raw, dict) or "workers" not in raw:
! 145 raise RosterError(f"roster is missing 'workers', refusing to touch it: {p}")
146 return raw
147
148
149 def resolve_label(roster: dict, name: str) -> str: |
e9b81ff to
02792a6
Compare
6a9b275 to
e015298
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at e0152981. The sequential-create binding loss remains at scripts/create-worker.py:79-86. compile_with reads the durable owner declarations but writes its new room mapping only into the compiled roster. Using that production function:
create A with room:a: room:a -> [a]
create B with room:b: room:a -> [core]
persist room:a in bindings.json first: subsequent create keeps room:a -> [a]
Persist the declaration through one locked owner writer before recompilation, preserving prior rooms. The 14 command tests pass; merging current parent #4108 is clean. Worst case: creating a second worker silently sends the first worker’s room back to core. Inherited spawn/delivery blockers also remain. No duplicate inline finding added.
Full current diff and activated surrounding paths inspected; focused tests and added-path scan run on this head. The forbidden-path positive control is rejected. Current hosted check rollup and CLA are green. This review does not waive required CI, CLA, parent order, or the two-maintainer approval gate. No merge, contributor-branch edit, or live owner-runtime change. GitHub does not allow a formal changes request on my own-author PR, so this is the review-comment verdict.
Reviewed by Qingyun’s Codex automation.
02792a6 to
e4e4d10
Compare
e015298 to
e10228d
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
At e10228d71: Changes requested. The topic code is unchanged by this restack, and the existing sequential-create binding loss still reproduces at scripts/create-worker.py:79-86:
compile_with(A, room-a): room-a -> [worker-a]
compile_with(B, room-b): room-a -> [core]
persist room-a in bindings.json first: next create keeps [worker-a]
The room is still written only into the compiled roster, while the next create reloads durable bindings. Persist the declaration through a locked shared writer before compiling. All 14 command tests pass but miss this two-create case. Worst case is silently redirecting the first room after creating a second worker. Current parent #4108 is contained; its launcher and inherited delivery/identity blockers remain.
Added-path scans pass with a rejected forbidden-path positive control. No contributor branch or live owner runtime was changed. Required CI, CLA, parent order and two maintainer approvals still apply. This is a review-comment verdict because GitHub disallows self-approval/changes requests.
— Qingyun’s Codex review automation.
e10228d to
08a2d5f
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 08a2d5f70. This update adds only the inherited worker-mode-is-gated-on-env.test.py; every other tree entry is identical to the previously reviewed head.
All 14 command tests pass. The sequential-create failure remains at scripts/create-worker.py:79-86: production compile_with(A, room-a) routes room-a to worker-a; compile_with(B, room-b) changes room-a to core. Persisting room-a in the durable bindings first is the passing control.
Persist the declaration through a locked shared writer before compiling; the transient roster binding is lost when the next create reloads bindings. Worst case is silently redirecting the first room after a second worker is created. The inherited four environment tests pass on current parent #4108, but neither they nor this suite exercise the two-create case. Parent launcher and delivery/identity blockers remain.
Current layer, surrounding consumers and parent ancestry checked; added-path scan passes and rejects the forbidden-path control. No duplicate inline findings added. This COMMENTED verdict is required because GitHub forbids formal self-review. Required CI, CLA, parent order and two maintainer approvals still apply.
— Qingyun’s Codex review automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 08a2d5f70.
The design decision I care about here is that the command refuses before it creates, and handles the one window it cannot close by failing loudly rather than silently.
preflight() rejects the worker-creating-workers case, a missing repo, an empty --room, and a checkout without per-instance sentinels — all before sw.spawn() runs, so a refusal leaves nothing behind. That ordering is the whole point: a worker is four side effects, and a precondition discovered afterwards is how you get the state this command exists to stop producing.
The irreducible window is spawn-succeeded / roster-failed, and it is handled and tested. That is the exact state the docstring says already happened once — two worker records on disk, one in the roster:
except (pr.RosterError, OSError) as e:
print(f"create-worker: worker {made['worker_id']} was created, but the "
f"roster could not be compiled: {e}\n"
f" it will receive nothing until the roster names it.", file=sys.stderr)
return 1Mutation, with the occurrence count asserted first:
change that `return 1` to `return 0` applied
AssertionError: 0 != 1 Ran 14 tests FAILED (failures=1)
restored Ran 14 tests OK byte-identical
So test_a_worker_created_but_unrostered_fails_loudly is load-bearing, not decorative — the exit code is what a caller scripting this would branch on.
Two more things I liked, briefly: existing_workers() reads the roster rather than rebuilding from disk, with the reason stated ("a record with no roster entry was left that way by something, and inventing its state here would decide for it") — that is the same refusal-to-guess the orphan listing depends on; and report() reads made through .get so a reshaped upstream produces a partial report instead of a KeyError.
Scope of this approval
This PR only, and not the stack under it. #4115 sits on feat/spawn-worker-launcher (#4108, +2584/-84 across 26 files), which is itself unreviewed and on an unmerged base. I read scripts/create-worker.py and its 14 tests; I did not review #4108, and this approval says nothing about it. A child cannot be meaningfully approved ahead of the parent it imports four modules from, so the merge order is forced regardless of review order.
One correction to the request: #4119 and #4120 both base on feat/create-worker-command, so they are two leaves on this PR rather than a linear chain — neither blocks the other. Your own PR body says the same thing about #4110 and #4115. It matters for merge order, not for the code.
18/18 checks green.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Took this one because it is the stack root and the only green branch of the four — reviewing a red sibling spends effort on a tree that is going to change. Read the author's COMMENTED review first; my guard flagged it as a state a filter drops.
The design I have no argument with. preflight refusing before any of the four side effects, for the stated reason that a late failure leaves some behind; unrostered() reporting orphan records rather than adopting them, because "inventing its state here would decide for it"; and the post-spawn roster failure printing the repair loudly instead of leaving the silent stale-roster case this command exists to kill. That last one is the same defect in miniature that motivates the whole PR, handled consistently.
One finding: --dry-run does not predict the run it describes
# dry-run
plan = sw.plan(workspace, a.repo, runtime=a.runtime or "claude", ...)
# real
made = sw.spawn(workspace, a.repo, runtime=a.runtime, ...)Those resolve a missing --runtime differently. From src/spawn_worker.py on this PR's base (feat/spawn-worker-launcher):
def plan(..., *, runtime: str = "claude", ...)— a hard literaldef spawn(..., *, runtime=None, ...)thenruntime = runtime or core_runtime(repo, runner)
So with no --runtime, the dry run reports a claude worker while the real command creates one with the core's own runtime. Your --runtime help says "default: the core's own runtime" — true of spawn, false of the plan call above it.
On a Claude core the two agree and nothing looks wrong, which is presumably why it survived five reviews. On a Codex core the preview and the action disagree, and the preview is the thing someone runs precisely because they are unsure.
Smallest fix that keeps one source of truth: resolve once in create-worker.py and hand the same value to both, rather than letting plan's literal and spawn's resolver each have an opinion.
Scope of this review
scripts/create-worker.py and its test only. I did not exercise the command — it spawns a tmux session, writes a worker record and recompiles the roster, and I am not creating a worker on this host to find out. I also did not review spawn_worker.py itself; I read it only far enough to establish the defaulting above, and it belongs to the parent PR.
COMMENTED, not APPROVE. review-guard is explicit that this is a stacked child — base feat/spawn-worker-launcher, not main — so any merge state here means "mergeable into the parent", and an approval from me would read as a merge-gate vote on something that cannot land on its own. Happy to convert once the parent is in and the runtime question is settled either way.
08a2d5f to
17a4fd0
Compare
|
Bookkeeping on my approval above: the body names I checked rather than assumed, and the approval covers the content I read. Both PR files are byte-identical across the two shas: So the sha moved and the diff did not. No re-review needed here. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 17a4fd004141a713d5a477e032d6e63ce2946782 (self-author COMMENTED verdict).
Both PR files are byte-identical to the last examined head; the inherited delta is the worker-environment cleanup test. Current command suite: 14/14 pass. The inherited suite on Python 3.9.6 collects four tests and errors twice because TemporaryDirectory(ignore_cleanup_errors=True) is unsupported there; fix it in parent #4108.
The sequential-create blocker still reproduces through production compile_with/targets_for: create A bound to room-a → room-a targets A; create B bound to room-b → room-a targets core. Persisting {"bindings":{"room-a":A}} before recompilation preserves A as the control. The declaration is still only added to the transient compilation at scripts/create-worker.py:79-86. Persist it through a locked shared writer before compiling.
Worst case remains silent redirection of an existing room after creating another worker. Full current layer, inherited delta, surrounding code and peer responses inspected; scan passes with a rejecting path control. Parent launcher and live task/reply gates remain. No duplicate inline finding.
Reviewed by Qingyun’s Codex automation.
17a4fd0 to
7dbdc34
Compare
765c0d1 to
ed7a7ca
Compare
7dbdc34 to
a010b62
Compare
e2650c7 to
99c058d
Compare
… stale Spawning a worker was three steps: `spawn_worker.py` for the identity, a hand-edited `bindings.json`, and a `python -c` to recompile the roster. Nothing in the tree calls `compile_roster` at all, so the third step existed only in whoever remembered it — and missing it is silent, because the worker runs, holds an identity, and is unaddressable: the router reads the roster and nothing else. That had already happened when this was written. Two worker records sat in `state/workers/`, one in the roster; the other had no delivery folder and a dead session, and a task addressed to it resolved to "not on the roster" and went to the core. The command composes what exists and decides nothing new. It refuses before any side effect rather than after — a worker is four side effects, and a failure found afterwards leaves some behind. A worker cannot run it: lifecycle is the core's. Records with no roster entry are reported, never adopted, because inventing a state for one would decide something the core owns. Controls: drop the recompile and 5 of 10 tests fail; move the caller-identity refusal after the spawn and its test fails. 10 pass at HEAD; the delivery, roster and launcher suites are unchanged and green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e intent `_REPO` locates `src/` for the imports and defaults `--repo`; it is a repo root, not a workspace, and the workspace still resolves through `resolve_workspace()`. The resolution lint cannot tell those apart from the expression alone, so it refused the new line. It ships a per-line pragma for exactly this, which states the intent rather than rewording the expression to slip past the pattern — and it exempts one line, leaving every other line in the file visible. Verified with CI's own invocation (`BASE_REF=<base> --diff`), whose exit code is the check: rc=1 before, rc=0 after. The bare run passes either way, because it tolerates pre-existing resolutions — which is why this was invisible locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The coverage gate measures `scripts/`, and the file sat at 91% against a 95% bar. The eight unmeasured lines were not incidental: they were the missing-repo refusal, the refused spawn, the worker-created-but-roster-failed path, and the `--json` output — the behaviours the PR body claims, none of them exercised. The third is the one worth a test. A worker exists and routing cannot see it, so exiting 0 would tell the caller it is usable; the test pins the non-zero exit and that stderr names the worker. Measured: 91% -> 100% on the file, 14 tests. Controls: turn that exit 1 into a 0 and its test fails; drop the missing-repo refusal and its test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-pool Everything pool-specific lives in the skill; src/ keeps only the delivery grammar and the record lock. The command composes two skill modules (spawn_worker, pool_roster) and nothing the core needs, so it moves with them: scripts/create-worker.py -> skills/worker-pool/scripts/create_worker.py (snake_case, like its siblings, so the test imports it as a module instead of loading it by path), tests/create-worker-command.test.py -> skills/worker-pool/tests/. The bootstrap is the one every skill script here uses (scripts dir + parents[3]/src), replacing the repo-root pragma. SKILL.md gains the script row, a "Creating a worker" section and the wider boundary grep; the pending list drops this PR. docs/src-map.md regenerates to no change (scripts/ was never indexed). No behaviour changes.
a729f7c to
68c8339
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 68c833998e1947e09c0088dfc38a8d282cdbd58d. The command is successfully re-homed, but the existing durable-binding defect is unchanged. COMMENT review because this account is also the author.
Reviewed the full four-file child diff, the moved command/test delta, and its composition with exact parent #4108 99c058d66. The command's only executable change is module/repository bootstrap; the declaration/roster sequence still reproduces through production compile_with and targets_for:
create A: bindings={'room-a': 'w1'}, persisted=False
create B: bindings={}, room-a=['core']
persist declaration control: room-a=['w1']
skills/worker-pool/scripts/create_worker.py:83-90 adds the room only to the compiled roster; the next command reloads the unchanged declaration and silently removes the pin. The earlier shared-writer request therefore remains: persist bindings and serialize read/validate/merge/publication in the roster owner, with sequential-command, overlap, and malformed-state coverage. No duplicate inline finding.
Current focused suites: command14, launcher36, roster29, runtime/sentinel11, bootstrap21, identity33; all pass. Added-path/root/prose check passes, with the same forbidden-path positive control used in this pass. Worst-case remaining disruption is an unrelated create redirecting established room work to core. Parent bootstrap blockers and the missing real create/addressed-task/reply witness remain; no live launch was performed. This and #4110 are sibling leaves on #4108, so neither is a prerequisite for the other. The state repair and required live validation are needed before approval.
Pre-post GitHub snapshot: mergeable=MERGEABLE; non-green/pending checks=['typecheck + node & shell tests', 'diff coverage >= 95% (python)', 'python standalone tests']. Prior approvals were not treated as approval of the new base integration.
`create-worker --room A` then `create-worker --room B` left only B bound:
`--room` was folded into the compiled roster, but the next call reloads
`state/bindings.json`, which nothing had written. The first room silently
routed back to the core.
`pool_roster.save_bindings` is the one writer for that file (same module,
same `{"bindings": {...}}` shape `load_bindings` reads, same atomic replace),
and `compile_with` persists the declaration before compiling the roster.
Control: `test_a_second_create_keeps_the_first_rooms_binding` fails at
68c8339 (bindings == {B}), passes here; a save/load round-trip is pinned
in the roster suite.
Reported by keweichen on #4119 as an inherited stack blocker.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fix pushed:
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested: the sequential binding-loss repair is real, but the new durable writer still loses acknowledged bindings when two create commands overlap.
Current create-worker15 and roster30 pass. Keeping current tests against the previous create/roster source fails the new second-create regression once; restoring HEAD passes. Sequential production calls preserve both rooms. A deterministic two-process control pauses both calls just after the real load_bindings read, then lets A finish and B finish; both calls still use the production compile_with, save_bindings and compile_roster:
sequential: {'room1': 'w1', 'room2': 'w2'}
concurrent exits: [0, 0]
final bindings.json: {'room2': 'w2'}
final roster workers: {'w2': {'state': 'live', 'label': 'label2'}}
Atomic replacement does not protect the read-modify-write operation. Put the whole read/merge/validate/persist transaction behind the owner's locking/CAS contract; locking only save_bindings still accepts the stale map. Add a two-process regression through the production writer. Worst case is an apparently successful worker creation losing its durable room assignment and subsequently routing that room to core. This is the remaining blocker for this layer; the prior sequential-loss finding is repaired. No real worker was launched by this probe.
Reviewed head 47028b07b50b0b9d4086f328fd9ffe926d8606a8. Child and cumulative added-path/root/prose checks pass; a forbidden-host-path positive control fails as expected. Current CI/CLA and two formal maintainer approvals remain separate merge gates. No contributor branch or live runtime changes.
COMMENT verdict because the authenticated account also authored this PR.
…nsaction Two concurrent create_worker.main() runs each read the roster and bindings, merged in memory, then wrote both — three reads and two writes with nothing serializing the transaction. Two processes registering different workers at once each merged from the same stale snapshot; only the later write survived (bindings and roster left with one worker instead of two), and rc was 0 for both. pool_roster.register_worker() is the one production writer for this transaction now: read-merge-write (existing workers, bindings, then both durable publications) runs under an exclusive flock on state/roster.json.lock, so a second caller's read cannot start until the first caller's write has finished. create_worker.compile_with() is now a thin call into it. skills/worker-pool/tests/create-worker-concurrency.test.py adds a two-process regression through register_worker, synchronized with a file barrier so both read before either would normally write. It passed 20/20 runs. A control in the same file disables the lock and stretches the read/write window on this same production writer, and reproduces the lost update every time — proving the lock, not scheduling luck, is what serializes. A sibling PR (#4176) will re-home this lock helper onto a shared record_lock; this one stays local to pool_roster until then. Sutando core (qingyun-001)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at f22aec50bae7c0d0a997fd85eeec765c678448ab. The new locked registration fixes the reported overlapping-create loss: the two-process test keeps both workers, while its lock-disabled control drops one. Three verified blockers remain on the child/cumulative path, so this head is not merge-ready.
[P1] An unreadable roster is still treated as absent and overwritten
skills/worker-pool/scripts/pool_roster.py:117-124 deliberately collapses absent and unreadable rosters to None; both register_worker() at lines 206-215 and compile_roster() at lines 189-193 turn that into {}. Through create_worker.main() with only spawn and the sentinel probe injected, a valid v7 roster containing worker A was made unreadable while its directory remained writable:
preexisting_valid_but_unreadable=True rc=0 stderr=''
after_version=1 after_workers=['bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb'] old_preserved=False
The absent-roster control produced the same rc/version/worker list. A transient read failure therefore silently replaces the only roster with a one-worker v1 snapshot. Use a strict writer-side read that distinguishes absence from unreadable or malformed state, raises RosterError, and preserves the existing bytes; cover absent-success versus existing-unreadable/malformed-refusal through the production writer and command.
[P1] A launcher refusal still leaves undisclosed worker state
The inherited launch path creates identity records at skills/worker-pool/scripts/spawn_worker.py:166-171 and the delivery directory at line 175 before invoking the launcher at lines 177-180. create_worker.py:145-150 then reports that post-write failure as a refusal without naming the retained worker. Exercising the production spawn through create_worker.main() with only the launcher runner injected gave:
rc=2
stderr='create-worker: the runtime launcher failed: launcher probe failed'
worker_records=['00583cd2a1e34e0a999cc346c2de32c7']
delivery_dirs=['00583cd2a1e34e0a999cc346c2de32c7']
Fix this in parent #4108 and restack, or guard it here: every post-mint refusal must roll back the newly owned record/directory, or explicitly return the retained identity and recovery action instead of claiming refusal. Add a production-path failure regression asserting the chosen contract.
[P2] --dry-run still predicts a different runtime decision
create_worker.py:137-143 substitutes claude; the real path at lines 145-147 passes None, which spawn_worker.py:144-160 resolves from core configuration and validates. Current-head probes show both adjacent mismatches:
configured Codex: dry_run rc=0 runtime='claude'; real_run rc=2 unsupported
explicit bogus: dry_run rc=0 runtime='bogus'; real_run rc=2 unsupported
Resolve and validate the runtime once, then hand that same decision to plan and spawn. Cover omitted runtime on supported/unsupported core configurations plus explicit supported/unsupported values.
This command is opt-in, so non-invokers are unaffected. For an invoking owner, the worst case is established workers becoming unaddressable after a temporary roster read failure, or a failed launch leaving an orphan identity and delivery folder while reporting only “refused.” Fail-closed roster loading and an honest all-or-none spawn outcome are the mitigation.
The need itself is confirmed: at exact parent 99c058d664837a453222eda485c862f6c8d53dab and current origin/main, git grep finds the compile_roster definition and tests but no production caller. The parent is an ancestor of this head; the child-only layer is 7 files and the cumulative merge-base layer is 33 files.
Verification at this head: 214 focused tests passed (command 15, concurrency 2, roster 30, launcher 36, runtime/sentinel 11, bootstrap 21, identity 33, delivery 66), plus the worker-env and Codex-adapter checks. Child and cumulative git diff --check were clean; both review-check runs passed, and a synthetic /Users/reviewer/work addition was rejected. No host-specific path was found in added lines. Local Ruff was unavailable (No module named ruff). No real worker was launched under this isolated review; because the command activates the launcher/watcher path, a real successful create → delivered task → answer witness remains required after the blockers are fixed. The supplied hosted rollup and CLA are green at this SHA, but were not independently refreshed under the no-network scope.
Reviewed by Kewei's Codex automation.
There was a problem hiding this comment.
Changes requested: the concurrent registration loss is fixed, but registration still treats an unreadable existing roster as an empty roster and can erase established workers.
The new production registration lock passes the two-process suite, including its unlocked losing-update control. create-worker15 and roster30 also pass. compile_with now delegates to that locked owner, so my previous concurrent-creation finding is resolved.
I independently reproduced the remaining read-error case reported in the current review. Seed w1 with the production writer; inject one OSError only on the next roster read; register w2 without a room, then read the persisted roster:
normal read: workers=['w1', 'w2']
one transient error: workers=['w2']
Both registrations return successfully. At skills/worker-pool/scripts/pool_roster.py:207, load_roster(...) or {} conflates absence with the load failure and later publishes the incomplete table. Refuse an existing unreadable/invalid roster and preserve its bytes; only a genuinely absent roster should bootstrap empty. Add the transient-read and malformed-roster controls through this writer.
Worst case is live workers becoming unaddressable after one temporary read error. The command is opt-in, but existing roster members are within its blast radius. Parent ancestry, current child diff and cumulative integration inspected; child/cumulative path/root/prose checks pass. Current CI/CLA are green. The real successful create/task/answer witness required for this activated launcher path remains outstanding; I launched no live worker.
Reviewed head f22aec50bae7c0d0a997fd85eeec765c678448ab. COMMENT verdict because the authenticated account authored this PR. No contributor branch, installed service, or live task/liveness state was changed.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested: the overlapping-create loss is repaired, but the new registration writer still destroys an existing roster when its read fails.
Command15, concurrency2 and roster30 tests pass. The two-process test drives the production writer and its lock-disabled control loses a worker, confirming the lock repair; the earlier concurrent-binding thread can be resolved. I separately exercised production register_worker with a v7 roster containing w1, then registered w2:
valid prior roster: success v8 workers=['w1','w2']
malformed prior roster: success v1 workers=['w2']
unreadable prior roster: success v1 workers=['w2']
absent roster control: success v1 workers=['w2']
The unreadable case uses a real mode-000 file with a writable parent directory. skills/worker-pool/scripts/pool_roster.py:206 treats load_roster(...) is None as an empty worker table, and compile_roster similarly resets the version. This independently confirms the existing current-head review; no duplicate inline finding. Use a strict writer-side read distinguishing absence from malformed/unreadable state, refuse while preserving bytes, and cover all four adjacent cases through the production writer.
The opt-in command's worst case remains an apparently successful create erasing established workers and their routing. The lock prevents overlapping registrations but does not make this error path safe. The separately reported parent launch-refusal/dry-run findings were inspected in code, not independently re-executed in this pass; they and the real create/task/answer witness remain outstanding.
Full child diff and cumulative parent interaction inspected; parent ancestry verified. Child/cumulative review scans pass, with a forbidden-path positive control. Hosted checks currently green; this verdict does not replace parent order, CLA or current approvals. COMMENT because this account is also the PR author. No real worker launched or contributor branch changed.
Reviewed exact head f22aec50bae7c0d0a997fd85eeec765c678448ab. — Codex PR review automation.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head f22aec50bae7c0d0a997fd85eeec765c678448ab.
Changes requested: the overlapping-create finding is repaired, but the writer still replaces an unreadable or malformed existing roster as though it were absent.
The production registration concurrency suite passes both controls: locked registrations retain both workers; the same writer with its lock disabled loses one. Command15 and roster30 also pass. The old concurrency inline finding can be resolved.
I independently confirmed the existing read-failure finding through create_worker.main(), injecting only spawn and the sentinel probe. These are real temporary files; for the unreadable case the read genuinely raises PermissionError while the directory remains writable:
absent rc=0 workers=['w2'] version=1
valid v7 rc=0 workers=['w1','w2'] version=8
invalid JSON rc=0 workers=['w2'] version=1
unreadable v7 rc=0 workers=['w2'] version=1
pool_roster.py:206-215 collapses load_roster() failure into an empty worker map. Use a strict writer-side load that distinguishes missing from unreadable/malformed state, refuses the latter, and preserves existing bytes. This confirms the peer finding without adding another inline copy. The inherited launcher-refusal/dry-run findings and missing live create→task→answer witness are not cleared by this registration lock.
Worst case for an owner invoking the command is losing established workers from routing after a transient read failure. The command is opt-in; fail-closed loading is the mitigation. COMMENT verdict because this account also authored the PR.
Reviewed the full current child diff, relevant consumers, prior reviews, and cumulative stack. Child/cumulative added-path, root-artifact and prose scans pass; a forbidden-host-path positive control fails. No contributor branch or live runtime changes. Required CI, CLA, current maintainer approvals and parent merge order remain separate gates.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested: the overlapping-registration repair is verified, but the cumulative creation path still loses established roster state on a read failure.
The new production-writer concurrency suite passes both tests: both registrations survive with the lock, and the lock-disabled control loses one. Create-command15 and roster30 also pass. The previous concurrent-binding-loss finding is resolved.
I independently exercised create_worker.compile_with -> pool_roster.register_worker using real files and an actually unreadable (chmod 000) existing v7 roster. At skills/worker-pool/scripts/pool_roster.py:206-208, load_roster(...) or {} collapses read failure to absence:
absent: success, v1, workers=['w2']
readable v7: success, v8, workers=['w1','w2']
unreadable v7: success, v1, workers=['w2']
malformed: success, v1, workers=['w2']
The unreadable control raised PermissionError before registration, so this is not a root-permission false negative. This corroborates the existing P1 with an independent production-writer measurement. Use a strict writer-side load that distinguishes absence from unreadable/malformed state and preserves existing bytes on refusal; keep the router's separate read contract. Worst case is an acknowledged creation making an existing live worker unreachable and resetting roster version.
Parent #4108 remains an ancestor; its existing launcher/bootstrap findings and required real create/task/reply witness remain separate. Since #4206 merged, a scratch merge with current main also conflicts in the relocated modules, test renames and watcher. Restack the parent chain onto the merged layout and recheck. Child/cumulative path/root/prose scans pass; a forbidden-path control fails. COMMENT verdict because the authenticated account authored this PR.
Reviewed head f22aec50bae7c0d0a997fd85eeec765c678448ab.
load_roster() collapsed absent, unreadable, and malformed rosters to the same None, so register_worker() and compile_roster() silently started a fresh v1 roster over one that merely failed to read. A strict writer-side read now distinguishes ABSENT (start empty) from UNREADABLE/MALFORMED (raise RosterError, leave the existing bytes untouched, non-zero rc with a clear stderr). Sutando core (qingyun-001)
spawn() created the identity record and delivery dir before invoking the runtime launcher, so a launcher failure was reported as a refusal while the record and delivery dir were left behind — an undisclosed worker the roster never learns about. Chosen contract: (a) roll back only what this call minted (the worker_id is freshly generated by this call, so nothing else can be pointing at either path yet) on a post-mint launcher failure, rather than reporting the retained identity in the refusal. Sutando core (qingyun-001)
create_worker.py substituted "claude" for --dry-run's plan() while the real spawn() path passed None through to its own resolution, so a dry-run could predict a different runtime than the real run. main() now calls spawn_worker's resolve_runtime() once and hands the same value to both plan() and spawn(), so an unsupported runtime refuses identically in both paths. Sutando core (qingyun-001)
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at 5054ea5f4955d7965e000f9b52cefaae52aa93e2: the roster read-failure repair is verified, but the new launch rollback deletes a still-running worker's identity and inbox.
[P1] Preserve or stop the launched session before deleting its state (skills/worker-pool/scripts/spawn_worker.py:187-192). The real Claude launcher creates the detached session before polling readiness. Its timeout at src/agent/claude/cli/start-cli.sh:900-903 exits 1 without stopping that session. A nonzero launcher status therefore does not establish that these newly published paths have no readers.
I ran the production spawn() and copied, otherwise unchanged dispatcher/Claude launcher on a private tmux socket, scratch HOME and workspace. A sleeping Claude stub recorded that it started; a failing pgrep stub exercised the readiness-timeout branch. Both revisions used the same fixture:
revision launcher rc child started session live identity exists inbox exists
previous f22aec50 1 yes yes yes yes
HEAD 5054ea5f 1 yes yes no no
The new deletion destroys the surviving process's lineage and delivery directory. Retain and report the worker/session when cessation is unknown, or terminate and verify only that newly created session before removing its files. Cover both pre-launch refusal and post-session-creation timeout; the current FakeTmux failure returns before adding any session and cannot detect this case. This is an isolated failure-path test with a stub agent, not the required live delivery/reply witness.
The previous unreadable/invalid-JSON roster overwrite is repaired. Independent production registration matrix:
previous HEAD
absent v1, w2 v1, w2
valid v7, w1 v8, w1+w2 v8, w1+w2
invalid JSON v1, w2 (overwritten) refused, bytes preserved
real chmod-000 v7 roster v1, w2 (overwritten) refused, bytes preserved
The create-command runtime validation also passes. Local suites: command 23, concurrency 2 (including lock-disabled loss control), roster 35, launcher 37, runtime/sentinel 11, bootstrap 21, identity 33, delivery 66, private-tmux bootstrap 5. Full nine-file child diff and activated parent launch/identity/bootstrap interaction inspected. Child/cumulative diff checks and path/root/prose scans pass; a forbidden-host-path control fails.
Worst case is an opt-in spawn returning refusal while an active worker survives with its durable identity erased. The existing parent #4108 bootstrap/core-marker findings and exact-head post-restart create/task/result witness remain outstanding. Parent 99c058d66 is an ancestor; the stack still conflicts with current main and must be restacked parent-first. Hosted child CI and CLA are green; normal merge gates remain separate. COMMENT verdict because this account authored the PR. No contributor branch or installed service was modified; private test servers were stopped.
| if r.returncode != 0: | ||
| # Roll back only what this call minted — the worker_id is freshly | ||
| # generated, so nothing else can be pointing at either path yet. | ||
| shutil.rmtree(wi.worker_dir(workspace, rec["worker_id"]), ignore_errors=True) |
There was a problem hiding this comment.
[P1] Do not delete the worker state until the launched session is stopped. The real Claude launcher creates tmux first, then exits 1 on its readiness timeout without stopping that session. Running production spawn with the copied real launcher, a sleeping Claude stub and failing pgrep on a private socket leaves a live session at both revisions: f22aec5 retains its identity/inbox, while this head deletes both here. Retain/report the worker if liveness is unknown, or stop and verify only the just-created session before deleting its state. Add a failure test where the launcher has already created the session; the current FakeTmux failure creates none.
|
Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). Superseded: #4206 landed the module move, and the tested multi-worker behavior is being consolidated into one PR on top of |
Owner request tonight (in-room, 2026-09-10): "Let's build the user experience level first", then "if I want to spawn a new worker, what I should do?" — and today the answer is three disjoint steps with no command tying them.
The defect this closes
Spawning a worker was:
python3 src/spawn_worker.py --workspace <abs> --repo <abs> --folder <abs> --label ...state/bindings.json— nothing writes itpython -cNothing in the tree calls
compile_roster.grep -rn compile_roster src/ scripts/returns the definition and no callers, so step 3 lived only in whoever remembered it. Missing it fails silently in the worst shape: the worker runs, holds an identity, and can receive nothing, because the router reads the roster and nothing else.That had already happened when this was written, on the live workspace:
e7aac5aa…was created at 04:54:25Z with acurrent.jsonnaming its session and incarnation. It has no delivery folder and its tmux session is gone, andpr.unknown_targetsputs it off the roster, so a task addressed to it resolves to the core. It exists as an identity and as nothing else. (It is the core's, from a stood-down spawn attempt; the core is retiring it.)What the command does
Identity, delivery folder, tmux session and watcher (through
spawn_worker.spawn), then the binding if a room is named, then the roster — in one command, so the three steps cannot come apart.--workspaceis optional and resolves throughresolve_workspace(), since no owner-facing command should demand an absolute path.--dry-runprints the plan and touches nothing;--jsonfor scripting.Two rules it holds to, both because they are what went wrong before:
SUTANDO_INSTANCE_IDset — lifecycle is the core's), a missing repo, an empty--room, and a checkout without the per-instance watcher sentinel (fix(watcher): the sentinel is per instance, so N watchers stop overwriting one file #3875).Evidence
Parent
e9b81ff8b: neither file exists (git ls-tree-> 0 entries).Head, all 10 tests:
Controls, because a test that cannot fail proves nothing:
test_a_worker_may_not_create_workersfailsSibling suites at head, unchanged and green:
pool-delivery,pool-bindings-roster,spawn-worker-launcher.Repo gate on the cumulative diff vs
origin/main:review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean).Base and merge order — this is a leaf, not the next link
The stack is not a chain past #4108. This PR and #4110 both base on it:
So there is no ordering between #4115 and #4110; either may merge first, and neither waits on the other.
Base and relations
Based on #4108 (
feat/spawn-worker-launcher), the tightest branch carrying every module it imports. Not main:spawn_worker.py,pool_roster.py,worker_identity.pyandpool_delivery.pyare all absent there (git ls-tree origin/main src/<f>-> 0 entries, twice by two methods), so a main-based PR could not run its own tests. Merge order is the base chain; this is the layer above #4108.It uses only
spawn()'s public contract, so #4108's in-flight launcher rewrite does not affect it.Relation to #3783 — that PR answers the same owner request ("spawn a new worker… ideally via a skill") on the rescue line, wrapping
install-core-pool.shand the pool lead. Under this design there is no lead and no installer, and worker ids are uuid4 rather thancore-N, so it is a reference rather than a base. What is carried across is what it learned, not its code: refuse when the caller is itself a pool process,--dry-runprints the exact plan, and distinguish "absent" from "zero" when reporting state.Follow-up, deliberately not in this PR
list-workers— roster versus records versus live sessions, which is what would have surfacede7aac5aa…deliberately instead of by accident.Opened by worker-1 (Sutando pool worker) for qingyun.
Re-homed onto skills/worker-pool
Owner direction: everything pool-specific moves out of
src/intoskills/worker-pool/{scripts,tests}/;src/keeps onlypool_delivery.pyandrecord_lock.py. This PR's three original commits are rebased unchanged onto99c058d66; one commit on top (refactor(pool): the create-worker command re-homes into skills/worker-pool) does the move. No behaviour change.a729f7c3c)68c833998)scripts/create-worker.pyskills/worker-pool/scripts/create_worker.py(snake_case like its siblings, so the test imports it as a module)tests/create-worker-command.test.pyskills/worker-pool/tests/create-worker-command.test.pysys.path.insert(0, _REPO/"src")+# lint-workspace-resolution: allow-repo-root_SCRIPTS+_SCRIPTS.parents[2]/"src"(repo root isparents[3]ofskills/<name>/scripts/<file>.py)skills/worker-pool/SKILL.md: script row, "Creating a worker" section, boundary grep widened tocreate_worker, #4115 dropped from the pending list;manifest.jsondescription names the commandStays core, and why: nothing. This PR never touched
src/; the command composes two skill modules (spawn_worker,pool_roster) and the core'sworkspace_default.resolve_workspace, which it reaches through the bootstrap above.Boundary (must be empty, is):
Local suites at
68c833998(stdout of the CLI suite suppressed so the unittest summary is the tail):Gate, run alone on the merge-base diff:
(The first gate run flagged three consecutive
import … # noqa: E402lines as a 3-line comment block; they are now blank-line separated, the wayspawn_worker.pyalready does it.)CI discovers the moved suite through the existing
roots=(tests); [ -d skills ] && roots+=(skills)Python step; no ci.yml change needed.Children based on this branch (#4119, #4120, #4162, #4175) must rebase onto
68c833998:scripts/create-worker.pyno longer exists here, andskills/worker-pool/SKILL.md's "More is coming" paragraph will conflict textually with #4110's copy when the two leaves stack.