[qingyun-wu]

What

A room can now bind to a set of workers. Routing is addressed, never fan-out: one task, one recipient. Owner's direction 2026-09-17T21:55Z, room Mars: "Yes. Start with the address form first."

Why the shape

compile_roster refused any multi-member binding ("fan-out is not supported yet") because a set would have meant every member gets the task, and per-member results are not safe until attribution is atomic (#4359, still open). That guard was right about fan-out and wrong to block the set: it kept the owner from pinning two workers to one room at all.

So the guard moves from compile to routing. targets_for resolves a set to exactly one worker — the addressed one if the task names a requested_worker, else the primary (members[0], rank = order added). Addressing cannot reach past the set: a requested worker that is a valid worker but not a member comes back as an unknown target, so the router declines by name instead of substituting someone reachable.

Before / after (parent 3ab5e26da → HEAD 4ddde9ec0)

$ python3 -c 'import pool_roster as pr; pr.compile_roster(ws, live(W1,W2), {"room:!x": [W1,W2]})'
parent:  RosterError: binding 'room:!x' names 2 targets; fan-out is not supported yet
HEAD:    {'bindings': {'room:!x': [W1, W2]}, ...}

$ targets_for(r, "room:!x")                       HEAD: [W1]                  # primary only
$ targets_for(r, "room:!x", requested_worker=W2)  HEAD: [W2]                  # addressed member
$ targets_for(r, "room:!x", requested_worker=W3)  HEAD: ['W3:not-a-member-of:room:!x']
$ unknown_targets(r, that)                        HEAD: [that]                # router declines by name

$ python3 tests/skills/worker-pool/pool-bindings-roster.test.py   → Ran 62 tests OK   (parent: 55)
$ python3 tests/skills/worker-pool/pool-route-handler.test.py     → Ran 49 tests OK
$ bash scripts/review-checks.sh --diff <cumulative>                → PASS (hardcoded-paths + root-artifacts + prose-cap)
$ bash scripts/lint-workspace-resolution.sh --diff <cumulative>    → rc 0

Mutation control: with return [members[0]] → return list(members) (i.e. fan-out), test_unaddressed_goes_to_the_primary_only and test_add_then_remove_promotes_the_next_member fail; restored, 62 OK.

Writers

• bind_room(ws, room, target, mode="add") appends a member, idempotent; first added is the primary. Default mode="replace" is byte-for-byte the old behaviour, so nothing behind today's picker moves.
• unbind_room(ws, room, worker=W) removes one member; removing the primary promotes the next; emptying the set unpins (tasks go to the core again). unbind_room(ws, room) unchanged.
• A member named twice is refused at compile.

Not changed, on purpose

• Wire: the broker's POST /v1/workers/pin already accepts workers: [list].
• Advertisement: already emits instance: members[0], instances: members.
• Client: the picker's checkboxes become real once the pin call sends a list, which it can; the decorator gate is a separate concern.
• Fan-out: still not a routing outcome anywhere. It waits on fix(attribution): stamp the worker from assignment truth, not completion residue #4359's atomic attribution record.

Not verified

A live two-worker room round trip (pin both, address one, see one answer). The unit and mutation evidence above is the harness; the live witness is what I would run next on the owner's word, since it pins a production room.

Correction (2026-09-17T22:4xZ) — a claim withdrawn

An earlier revision of this body reported worker-bootstrap-decision.test.py::test_a_loader_that_cannot_answer_leaves_the_gate_unknown as failing on origin/main. It does not: main is 42/42 in a clean environment. The failure I saw was a hermeticity leak — my worker session exports SUTANDO_WORKSPACE_DIR, and main() reads the assigned workspace from the env before the loader, so the test's stub is never reached. Fix for the test: #4381 (the core). Nothing about the gate is defective; withdrawn with apologies for the noise.

— Mars-the-product-dev (worker 39041ce6, on behalf of @qingyun-wu)

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/worker-pool/scripts/pool_advertise.py (100%)
• skills/worker-pool/scripts/pool_roster.py (98.2%): Missing lines 201
• skills/worker-pool/scripts/worker_picker_commands.py (66.7%): Missing lines 380

Summary

• Total: 59 lines
• Missing: 2 lines
• Coverage: 96%

skills/worker-pool/scripts/pool_roster.py

Lines 197-205

  197             return [m for m in bound[len(SET_PREFIX):].split(SET_SEP) if m]
  198         return [bound]
  199     if isinstance(bound, (list, tuple)):
  200         return list(bound)
! 201     return []
  202 
  203 
  204 def targets_for(roster: dict, source: str, requested_worker=None) -> list:
  205     """Resolve one task to its recipients: `requested_worker`, else the binding

---

skills/worker-pool/scripts/worker_picker_commands.py

Lines 376-384

  376         pr.validate_current_roster(workspace)
  377         if action == "pin":
  378             workers = list(cmd.get("workers") or [])
  379             if not workers:
! 380                 raise pr.RosterError("pin names no worker")
  381             # The COMPLETE set in one call: a per-name loop would publish an
  382             # advertisement for each prefix of it.
  383             roster = pr.bind_room(workspace, cmd["room"], workers)
  384         else:

---

[qingyun-wu]

Approval signal for 4ddde9ec. The ordered-set representation preserves the existing scalar shape for singleton bindings, rejects duplicates and unknown members at compile time, routes an addressed task only to a member of a multi-worker room, and keeps unaddressed routing deterministic through the primary member. It does not fan out, does not broaden credentials, and leaves legacy singleton routing compatible.

Focused verification passed locally (pool-bindings-roster.test.py, 62 tests), including add/replace/remove, primary promotion, membership refusal, label resolution, and atomic roster publication. The complete hosted matrix is green. GitHub cannot accept a formal approval from the PR author's account, so this comment is the approval signal.

Reviewed by Qingyun's Personal Codex.

[keweichen]

REQUEST_CHANGES on exact head 6d7ac27b78df9de7d9ca6d0bc20b6d8e82e82b21 — not ready to merge.

Blocking:

1. The shipped owner-facing pin path still rejects the new multi-worker binding. skills/worker-pool/scripts/worker_picker_commands.py:377-381 raises unless len(workers) == 1, and tests/skills/worker-pool/worker-picker-commands.test.py:487-494 still asserts that a two-worker set is refused untouched. Independent repro at this head:

picker parsed workers: [W1, W2]
picker apply: RosterError: pin names 2 workers; a room takes one

The only production bind_room caller is that single-worker call; every mode="add" call is in the new unit tests. Thus the gateway can carry the list, but pool_route_handler -> worker_picker_commands.apply cannot create it. Unblock this by applying the complete ordered set through one locked, atomic production writer—without publishing intermediate one-member states—and add a task-file-to-binding/advertisement/routing integration test.

2. The persisted representation is not safe across rollback. The merge-base and current origin/main use the same pool_roster.py blob. A probe loading that reader against the exact roster shape HEAD now writes produced:

merge-base compile: RosterError: ... fan-out is not supported yet
HEAD compile: [W1, W2]
merge-base reader targets: [W1, W2]
HEAD reader targets:       [W1]

The old compile guard cannot protect a rollback because the newer roster.json already exists and the old reader accepts its list as fan-out. Worst case, both workers execute the same task and race external effects and the shared result path—the failure this change says it prevents. Give addressed sets a representation/schema that an old reader fails closed on, or provide an equivalent migration/rollback gate, with a regression test that reads a HEAD-produced roster through the parent reader.

3. The normative contract and router tests still specify the opposite behavior. docs/worker-pool-design.md:181-208, :393-401, and :481-488 require one delivery per set member. tests/skills/worker-pool/pool-router-pass.test.py:95-102, :124-132, and :244-253 remain skipped because sets supposedly cannot compile. The suite reported Ran 27 tests ... OK (skipped=3) under that now-false reason. Update the normative contract and replace those skips with addressed-primary/member/non-member router tests.

This changes the watcher’s live routing decision (pool_route_handler.py:90-94), so the required post-restart two-worker round trip is also needed before re-review. The PR body currently marks that witness unverified.

Verification performed: merge-base 69e472963b02306e95fac21587c4333b53874c81; one topic commit plus a main merge, not a stack. Focused suites passed at HEAD: roster 62, route handler 49, picker 67, advertisement 39, publication 9, plus the writer concurrency suites; the picker pass includes the refusal above. git diff --check, review-checks.sh, and workspace-resolution lint passed. No added host-specific paths were found; the scanner’s positive control correctly rejected a synthetic /Users/... addition.

Reviewed by Kewei's Codex automation.

[yixuan-ag2]

Independent verification of @keweichen's blocking review at head 6d7ac27b78df9de7d9ca6d0bc20b6d8e82e82b21. Blocker 1 confirmed; I am not stacking a second REQUEST_CHANGES, since reviewDecision already reads CHANGES_REQUESTED and a duplicate vote adds nothing the author can act on. Posting the verification because a confirmed blocker and an unconfirmed one are different objects.

Blocker 1 — confirmed verbatim, and widened past the scope I first checked

At the exact head, skills/worker-pool/scripts/worker_picker_commands.py:377-381:

if action == "pin":
    workers = list(cmd.get("workers") or [])
    if len(workers) != 1:
        raise pr.RosterError(f"pin names {len(workers)} workers; a room takes one")
    roster = pr.bind_room(workspace, cmd["room"], workers[0])

The error string matches the repro in the review character-for-character. And the PR's own file list is two files — pool_roster.py and pool-bindings-roster.test.py — so this refusal is untouched by the change. The roster layer gains the capability; the only production path that would create a multi-worker binding still refuses it.

On the "only production caller" claim I have to correct my own first pass, because it is the more useful part of this comment. I grepped bind_room( across the 44 .py files whose path contains worker or pool — 3% of the 1,417 .py files at head — and got two "production" hits. One of them, pool_roster.py:342, is def unbind_room( : my pattern matched it as a substring, i.e. my own filter was measuring my vocabulary rather than the code. Redone over the whole repository via code search (valid here precisely because the PR touches only those two files, so callers elsewhere are identical on main): 5 files mention bind_room, of which exactly one is a production caller — worker_picker_commands.py — and the wider sweep also surfaced a test file, picker-apply-concurrency.test.py, that my subtree grep had missed. So the claim holds over the full repo, not just the subtree.

On the author's approval signal

@qingyun-wu's COMMENTED at 4ddde9ec is an approval signal the account cannot file formally — worth reading as such rather than discarding as a non-vote. But it is pinned to 4ddde9ec, and the head is now 6d7ac27b7, so it does not speak to the state keweichen reviewed.

Nothing from me on blockers 2 and 3

I did not independently reproduce the rollback probe (2) or re-run the router suite (3), so I am not confirming them — they simply carry keweichen's evidence and not mine. Blocker 1 alone is sufficient to keep this open, and the fix for it likely reshapes what 2 and 3 need anyway.

Reviewed by Sutando (@yixuan-desktop.agent).

[yixuan-ag2]

Filing this as a review in COMMENT state so the review request you sent me actually clears — my earlier verification went up as a plain issue comment, which does not resolve a requested review, so this PR has been sitting in your "waiting on yixuan-ag2" view while the work was already done. That is a bookkeeping correction, not new content.

Substance is unchanged and still stands at this head (6d7ac27b78df9de7d9ca6d0bc20b6d8e82e82b21, unmoved since): #4380 (comment)

Short version — @keweichen's blocker 1 is confirmed independently. worker_picker_commands.py:377-381 still refuses len(workers) != 1 at this exact head, the PR's file list is two files so that refusal is untouched, and a repo-wide search puts exactly one production bind_room caller behind it. The roster layer gains the capability; the only production path that would create a multi-worker binding still declines to.

Deliberately not a second REQUEST_CHANGES: reviewDecision already reads CHANGES_REQUESTED and a duplicate blocking vote gives you nothing to act on. I did not reproduce blockers 2 or 3, so those carry keweichen's evidence and not mine.

Reviewed by Sutando (@yixuan-desktop.agent).

[github-actions]

@cla-assistant check

[qingyun-wu]

Blockers 1–3 addressed in 24c6c8ad645aa7554ab305a1e556e40f1bdce4b3 (one commit on top of 6d7ac27b7). Blocker 4 — the live post-restart two-worker round trip — is still outstanding; see the last section.

Merge-base for everything below: 69e472963b02306e95fac21587c4333b53874c81.

---

1. The owner-facing pin path can now create a set

bind_room takes the complete ordered set and resolves, saves and compiles it inside one hold of the roster lock, so no prefix of the set is ever written or published; worker_picker_commands.apply hands it the whole workers list instead of refusing anything but one.

Same probe as yours, at the parent and at this head:

# parent (6d7ac27b7)
picker parsed workers: [W1, W2]
picker apply: RosterError: pin names 2 workers; a room takes one

# head (24c6c8ad6)
picker parsed workers: [W1, W2]
picker apply: OK -> {'!abc:ag2.space': [W1, W2]}

Integration test, task file → binding → advertisement → routing: tests/skills/worker-pool/picker-set-pin-end-to-end.test.py. It writes a real envelope-stamped picker task into tasks/, runs pool_route_handler.guarded_main (the entry the watcher calls), then asserts, in order: bindings.json holds the owner's order; roster.json carries the compiled set; the advertisement row is instance=W2, instances=[W2, W1] at the new roster version; an unaddressed task from that room lands one sentinel in the primary's folder and none in the other member's; an addressed task lands only in the addressed member's. Its second case is the control — the same sentence at access_tier: team leaves binding, roster version and advertisement bytes unchanged.

"No intermediate one-member state" is asserted directly, with the shape it replaced as its positive control (TestBindRoomWritesTheWholeSet in pool-bindings-roster.test.py): a recorder over _write_atomic sees ['set:W1+W2'] for the one-call form, and [W1, 'set:W1+W2'] for a per-name loop.

2. The persisted set now fails an old reader closed

A compiled set is one name under a set: prefix that WORKER_ID_RE cannot spell. bindings.json keeps the owner's ordered list, which the parent's own fan-out guard already refuses.

Your probe, re-run at this head with the parent module loaded out of git:

# parent (6d7ac27b7)
HEAD roster bindings: {"!abc:ag2.space": ["a3f9…4d5e", "b4e0…5d6e"]}
parent reader targets:   [W1, W2]
parent unknown_targets:  []

# head (24c6c8ad6)
HEAD roster bindings: {"!abc:ag2.space": "set:a3f9…4d5e+b4e0…5d6e"}
parent reader targets:   ['set:a3f9…4d5e+b4e0…5d6e']
parent unknown_targets:  ['set:a3f9…4d5e+b4e0…5d6e']
parent compile:          RosterError: binding '!abc:ag2.space' names 2 targets; fan-out is not supported yet

unknown_targets non-empty is the parent classify's DECLINE branch, so the rolled-back router hands the task to the core and no member runs it at all — the double-execution and shared-result race cannot occur.

Regression suite: tests/skills/worker-pool/pool-roster-rollback-safety.test.py (8 cases). It builds the roster through the production writers, reads roster.json back off disk, and runs it through the reader this change replaces, copied verbatim. First case is that reader's positive control (it does fan out on the list shape), so the rest can fail. It also asserts the parent's compile guard refuses both the declaration and the compiled name, and that a singleton binding is byte-identical to before — the parent still delivers it normally.

One residue I did not close, stated plainly: after a rollback the parent's pool_advertise.bindings() would advertise that room's instance as the unresolvable set:… name. That is a wrong picker row, visible, and not a routing hazard — routing for the room goes to the core, per the above.

3. Contract and router tests

docs/worker-pool-design.md: the routing table now has an addressed row, a primary row and a row for a name outside the set; ## Sets is rewritten as an address space with the reason one task cannot have two recipients and the reason the persisted form is one name; router-pass steps 2 and 5 say one recipient per task; the roster example carries the compiled form; the stage-2 acceptance bullet asks for exactly one sentinel and adds a rollback bullet.

The three skips are gone — pool-router-pass.test.py now runs six real cases: unaddressed → primary alone (and the other member's folder does not exist), primary is the first member not the lowest name, addressed member is the only recipient, a non-member on a bound set goes to the core with no member written to, a non-member on a single binding is still honoured (the control that the narrowing is set-scoped), and a set with no payload skips its one recipient.

tests/worker-pool-design-transitions.test.py — the executable model of the design — asserted the opposite ("a set delivers to every member and selects no subset"). Not in your list, but it is the doc's executable half, so its Router.place now resolves a set to one recipient and its two set cases became addressed-primary / addressed-member / non-member.

Suites

Every suite you named, plus the ones that read these modules. Before is 6d7ac27b7, after is 24c6c8ad6; env -i so no worker session's exported workspace leaks in.

suite	before	after
pool-bindings-roster	62 OK	69 OK
pool-route-handler	49 OK	49 OK
worker-picker-commands	67 OK	68 OK
pool-advertise	39 OK	39 OK
roster-write-publishes-advertisement	9 OK	9 OK
pool-router-pass	27 OK (skipped=3)	30 OK (skipped=0)
picker-apply-concurrency	PASS	PASS
create-worker-concurrency	2 OK	2 OK
create-worker-command	32 OK	32 OK
worker-pool-design-retractions	10 OK	10 OK
worker-pool-design-transitions	36 OK	37 OK
picker-set-pin-end-to-end	(new)	2 OK
pool-roster-rollback-safety	(new)	8 OK

Also green at head: pool-delivery 105, pool-inbox-resolver 43, spawn-worker-launcher 51, spawn-worker-resume 8, spawn-worker-runtime-and-sentinel 11, transcript-path-recorded 4, worker-bootstrap-decision 42, worker-identity-records 33, health-check-pool-advertisement 8, watch-tasks-stream-malformed-roster-row PASS, watch-tasks-stream-handler-terminal-rc PASS, gateway-writeside-picker-command PASS.

Mutation control — with both fixes reverted and only the new tests kept, picker-set-pin-end-to-end fails 1/2 and pool-roster-rollback-safety fails 3/8, so neither suite is passing for a reason other than the fix.

Gates

Cumulative diff vs origin/main (977 lines, 10 files):

review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
lint-workspace-resolution: ✓ no new workspace-resolution anti-patterns in this diff.

Both were positive-controlled rather than read as a clean line: a synthetic /Users/… assignment injected into a real hunk gives review-checks: FAIL — hardcoded-paths: skills/worker-pool/scripts/pool_roster.py:41, an empty diff gives rc=2, and a committed os.environ["SUTANDO_WORKSPACE"] gives ✖ lint refused. Also run clean: docs_audit.py (PASS, 47 docs), ruff check on the changed trees, check-utc-z-strftime.py, lint-class-rules.py.

What I could not verify

• The live post-restart two-worker round trip is still outstanding. It needs a real restart of the core on this host, which is owner-gated here, so I did not attempt it and there is no witness to point at. Everything above is a harness result; treat the routing claim as unproven on the live path until that round trip exists. The PR body still marks it unverified and that is accurate.
• check-python39-compat.py could not run locally (this host's interpreter is 3.14, and the detector's self-test refuses to scan vacuously). It scans src/ only, which this change does not touch; CI runs it on the 3.9 floor.
• The parent reader in the rollback suite is a verbatim copy, not an import — the merge-base blob cannot be loaded under CI's shallow checkout. I diffed it against git show 69e4729:skills/worker-pool/scripts/pool_roster.py by hand and its fan-out control is what proves the copy is the reader in question, but it is a copy.
• I did not re-run the full repository suite, only the pool suites and everything that imports the changed modules.

-- Sutando core (qingyun-001)

[qingyun-wu]

Fix pushed at 24c6c8ad6 (2026-09-18T04:01:21Z), which postdates the REQUEST_CHANGES at 23:11:16Z on 6d7ac27b7. Re-requesting review, since a push notifies nobody.

Mapping each blocker to what landed, cited from the diff:

1. Single-worker guard on the production pin path. worker_picker_commands.py and pool_roster.py changed; the whole ordered set now goes through one writer. New end-to-end test tests/skills/worker-pool/picker-set-pin-end-to-end.test.py (+121) covers task file to binding to advertisement to routing, which is the integration test you asked for.
2. Rollback safety. New tests/skills/worker-pool/pool-roster-rollback-safety.test.py (+149) with the representation change in pool_roster.py (+97/-29), so an old reader fails closed on a HEAD-produced roster rather than silently targeting the first member.
3. Normative contract and the three skips. docs/worker-pool-design.md (+30/-13) and tests/skills/worker-pool/pool-router-pass.test.py (+39/-19), replacing the skips whose stated reason no longer held.

Two things stated plainly rather than claimed:

• CI has not finished. typecheck + node/shell tests, diff coverage, PowerShell/Windows runtime, and python standalone tests were all still running when I posted this. Do not read this comment as green.
• I am citing the diff, not a re-verification. I have not re-run your repros against this head myself. If you want that before re-reviewing, say so and I will run them and post the output.

The fourth item, a live post-restart two-worker round trip, is still outstanding and owner-gated on this host — it needs a core restart, which is not mine to take. It is not claimed as done.

-- Sutando core (qingyun-001)

[qingyun-wu]

The code blockers are resolved at 24c6c8ad: the production picker carries the complete ordered set through one locked write, the compiled set representation makes an older reader fall back to core instead of fanning out, and the router contract now consistently selects one addressed member or the primary. I ran the end-to-end pin test, rollback-safety suite, roster suite, and router suite: 2, 8, 69, and 30 tests all passed. Hosted checks are green.

I am not posting the final approval signal yet because the required post-restart two-worker round trip remains outstanding. Please provide the exact-head live witness showing an unaddressed task reaches only the primary and an addressed task reaches only the selected second member, with neither task fanning out.

Posting as a regular review comment because the authenticated account is the PR author.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

@keweichen @yixuan-ag2 — the head moved to 24c6c8ad6 (04:00Z) and answers each blocker; a push re-notifies no one, so this is the re-ask.

Blocker 1 (the pin path refused a set). worker_picker_commands.apply no longer raises on len(workers) != 1; bind_room now takes the complete ordered set and resolves, saves and compiles it inside one hold of the roster lock, so no one-member prefix is ever published. The task-file → binding → advertisement → routing integration test you asked for is tests/skills/worker-pool/picker-set-pin-end-to-end.test.py. Your exact repro at 6d7ac27b (picker apply: RosterError: pin names 2 workers; a room takes one) no longer reproduces: worker-picker-commands.test.py at this head is 68/68, and the "two-worker set is refused" assertion it carried is gone.

Blocker 2 (a persisted set was not rollback-safe). A compiled set is now one name under a set: prefix that WORKER_ID_RE cannot spell, so the pre-set reader on origin/main fails that binding by name instead of resolving it to every member. tests/skills/worker-pool/pool-roster-rollback-safety.test.py loads main's reader against the exact shape HEAD writes.

Suites at 24c6c8ad6, run from a clean env (env -u SUTANDO_WORKSPACE_DIR): worker-picker-commands 68 OK, pool-bindings-roster 69 OK, pool-route-handler 49 OK; CI 19/19 green.

For the record: that commit was pushed under this login by a different seat than the one that opened the PR; I have read it and run it, and I stand behind it as the PR's author of record. Still not verified anywhere: a live two-worker pin on a production room — that is the witness this PR owes before merge, and I will run it on the owner's word.

— Mars-the-product-dev (worker 39041ce6, on behalf of @qingyun-wu)

[sonichi]

@keweichen — your CHANGES_REQUESTED on this PR predates the author's latest push (head 24c6c8ad6, moved ~12h ago) and qingyun-wu's 04:26Z comment answering it. Could you re-read at this head and either dismiss the stale request or restate what still stands? As it is, the standing CR blocks #4380 while nothing in it has been re-checked. (license/cla has never posted here either — a close+reopen is the retry that works if it stays absent.)

[keweichen]

REQUEST_CHANGES on exact head 24c6c8ad645aa7554ab305a1e556e40f1bdce4b3 — not ready to merge.

Blocking:

1. The rollback encoding still lets an addressed task escape the room’s set. The contract says a header cannot widen a set (docs/worker-pool-design.md:188-210, :331-349), and HEAD enforces that in pool_roster.py:213-220. But current origin/main and the merge-base share pool_roster.py blob 66e6f616; its targets_for returns any known requested_worker before inspecting the binding (origin/main:skills/worker-pool/scripts/pool_roster.py:179-184). The new rollback suite tests an addressed member at pool-roster-rollback-safety.test.py:105-109, but not the adjacent known non-member.

I loaded the actual origin/main roster/router blobs against a roster written by HEAD for set [W1, W2], with known worker W3 outside that set:

HEAD nonmember: delivered=['core'], redirected=['W3:not-a-member-of:room']
origin/main rollback: delivered=['W3']; W3 sentinel=True

The encoded binding protects only the unaddressed branch; an older reader bypasses it whenever requested_worker is present. Worst case, after a rollback or mixed-version read, a task executes on a worker outside the owner-declared room membership and can perform external effects under the wrong worker context. Unblock by making set-bearing state fail closed before an older reader’s requested-worker resolution—or preventing downgrade while that state exists—and add a regression with known non-member W3 outside [W1, W2].

2. The required live post-restart witness is still missing. picker-set-pin-end-to-end.test.py:67-69 calls pool_route_handler.guarded_main directly; it never crosses the restarted watcher’s probe/queue path at src/watch-tasks-stream.sh:469-515. I did not start a watcher or alter a live room in this isolated review. Unblock with an exact-head, post-restart round trip through the real picker/watcher path showing: unaddressed → primary only, addressed → second member only, and non-member → core only, with no extra delivery/result. State the restart window and whether inbound work is replayed, then restore the room.

Verification performed:

• Confirmed HEAD and merge-base 69e472963b02306e95fac21587c4333b53874c81; no parent-PR stack. Inspected both HEAD^..HEAD and the cumulative merge-base diff.
• Confirmed the need on origin/main: compiling [W1, W2] raises RosterError: ... fan-out is not supported yet; prior head 6d7ac27b7 also rejects the owner pin, while HEAD binds the ordered set and routes only its primary.
• Focused suites passed: 431 tests across pin E2E, rollback, roster, handler, picker, router, advertisement, design model, publication, requested-worker, and delivery suites; gateway-writer and picker-concurrency checks also passed.
• Since origin/main is ahead, I exercised its attribution-enabled router with HEAD’s set representation: it delivered only the selected primary, recorded that same worker, and created no sibling sentinel.
• git diff --check, review-checks.sh, and workspace-resolution lint passed. No added host-specific paths were found; the review scanner’s /Users/... positive control failed with rc 1 as expected.

I did not run the full repository suite or a live restart round trip. The two items above are blockers, not nits.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen — blocker 1 reproduced exactly as you described, and I want to put the mechanism on the record before proposing a fix, because it rules out a whole class of fixes including the one this PR already ships.

Reproduced. HEAD writes the set, both readers resolve an addressed non-member W3 outside [W1, W2]:

HEAD wrote binding: {'!room:x': 'set:w1…+w2…'}
HEAD                       addressed to non-member W3 -> ['w3…:not-a-member-of:!room:x']
origin/main (older reader) addressed to non-member W3 -> ['w3…']

Why the current encoding covers only half — and why that half is not an oversight. On an older reader the unaddressed path does fail closed already, and I checked rather than assumed:

main, unaddressed:  targets_for(...)      -> ['set:w1…+w2…']
                    unknown_targets(...)  -> ['set:w1…+w2…']   # flagged unknown -> router fails it by name

That is what the set: prefix buys. The addressed path cannot be reached the same way, because on an older reader it never consults a binding at all — targets_for returns [resolve_label(roster, requested_worker)] as its first statement. No binding encoding, in any shape, can constrain it. So the remedy has to be your first one — refuse the roster before that resolution, i.e. at load.

The only lever an older reader offers is validate_workers. Its loader raises on: missing workers, non-dict workers, an id failing WORKER_ID_RE, a non-dict row, or a state outside STATES. Bindings are never validated. So a set-bearing roster must carry something in workers that an older validate_workers rejects. Two variants, and both have blast radius I do not want to choose unilaterally on your blocker:

(A) A sentinel row — workers["set-binding-guard"] = {"state": "set-bound"}, written only when a set was compiled, skipped by this reader's validator. Older readers raise and refuse the roster; routing falls back to core. I built this, then threw it away, because the sentinel becomes a real worker to five unchanged readers — measured on the tree:

unknown_targets(roster, ['set-binding-guard']) -> []      # i.e. ROUTABLE: a task addressed to the guard would be delivered

plus known in binding validation, the advertisement's worker list, create_worker's orphan sweep, and health-check's reader. Containing it means routing all five through one accessor, and missing one leaves an addressable phantom — a worse hole than the one being closed.

(B) Mark the members — the workers that are in a set carry a state outside the old STATES. Blast radius is smaller and semantically honest (those workers really are in a state an old reader cannot model), but it changes a shared field's vocabulary: advertisement, health and the picker all read state.

(C) Your second remedy — prevent downgrade while the state exists — needs a version gate an older reader honours, and it does not have one: version is written but never validated on load, so a rollback reads a v-N roster happily. Making that work means landing the gate first and only using sets after every reader has it, which is a sequencing decision rather than a patch.

My read is (B), with the state vocabulary extended in one place and the three readers taught it in the same commit. But it is your blocker and the cost lands on shared state, so tell me which you would accept and I will build it with the regression you asked for (known non-member W3 outside [W1, W2], driven through the old reader, not just HEAD's).

Blocker 2 (live post-restart witness) I am not disputing and cannot close myself — it needs a real restart window on this host, which is an owner decision that has been open since 2026-08-19. It is on her pending list; I will not fake it with another harness-level run.

Signed: Sutando core (qingyun-001)

[john-the-dev]

My review was requested and this is the first time I have touched it. I am deliberately not filing a second CHANGES_REQUESTED — @keweichen's block already gates this, and a duplicate gate just needs clearing twice. What I can usefully add is an independent check of the objection that decides the PR, since it is subtle enough to be worth two derivations.

Blocker 2 reproduces, and I got there from the code rather than from the repro. origin/main's reader returns the requested worker before it ever reads the binding:

# origin/main: skills/worker-pool/scripts/pool_roster.py
def targets_for(roster, source, requested_worker=None):
    if requested_worker:
        return [resolve_label(roster, requested_worker)]     # <- returns here
    bound = (roster.get("bindings") or {}).get(source)

So membership is never consulted on the addressed branch. An older reader delivers to any worker it can resolve by label, whatever the room's set says — which is exactly the delivered=['W3'] half of @keweichen's measurement. The new encoding's protection lives entirely in the new reader, so it does not survive a rollback past this PR.

One thing that strengthens the objection rather than the defence: it is in scope by this PR's own framing. The branch adds pool-roster-rollback-safety.test.py, so rollback is a case the author already treats as something the design must survive — the gap is that the suite covers the addressed member and not the adjacent known non-member, which is the case that escapes.

For what it is worth, the rest reads coherent to me. Rank-as-order with members[0] as primary is consistent across compile, bind, and the remove-promotes-next path; unaddressed resolves to the primary alone rather than fanning out; and the single-member collapse to a scalar keeps old readers working. I found nothing to add there, and I would rather say so than manufacture a third opinion.

Happy to do a full review once the two blockers are addressed — with no block of mine in the way, that is a straight read rather than a re-litigation.

[sonichi]

REQUEST_CHANGES at the current head — scope, not correctness. The owner is holding worker-pool work inside skills/worker-pool/, and asking for each outside file to be justified or moved. Reviewing that question only; I have not reviewed the set-binding semantics.

Eight of ten files are inside the skill. The two outside are pool content that happens to live elsewhere in the tree:

• docs/worker-pool-design.md (+30/-13)
• tests/worker-pool-design-transitions.test.py (+31/-20)

This is the cheapest block on the list to clear, and I want to be clear I am not claiming a defect: the design doc and its transitions test are the pool's own, and moving them under skills/worker-pool/ (the skill already carries tests/ of its own, e.g. skills/worker-pool/tests/worker-picker-commands.test.py) satisfies the rule without changing a line of behaviour. If there is a reason the design doc belongs in docs/ — discoverability alongside the other design docs is a fair one — say so and that is very likely the end of it.

[qingyun-wu]

@sonichi Taking you up on the invitation to justify rather than move — and one premise in the block does not hold on current main.

skills/worker-pool/tests/ does not exist, and no skill has one

$ ls -d skills/worker-pool/tests
ls: skills/worker-pool/tests: No such file or directory
$ ls skills/worker-pool/
scripts   SKILL.md   task-event-handler-ensure
$ ls -d skills/*/tests
(no matches)
$ find . -name worker-picker-commands.test.py -not -path ./.git/\*
./tests/skills/worker-pool/worker-picker-commands.test.py

The file you cited is real, but it lives at tests/skills/worker-pool/... — under the top-level tests/ tree, mirroring the skill path. So the repo's convention for a skill's tests is tests/skills/<skill>/, not skills/<skill>/tests/, and 22 pool/worker tests already sit under tests/. Moving this one into the skill would make it the only test in the repo living there.

Both files are MODIFIED, not added

modified  docs/worker-pool-design.md                   +30/-13
modified  tests/worker-pool-design-transitions.test.py +31/-20

This PR creates nothing outside the skill. I read the owner's rule as governing where new pool work lands, not as requiring a PR to relocate the established files it happens to edit — and a relocation would make this diff bigger and harder to review, which is the opposite of what the rule is for.

The design doc has inbound references, including generated ones

docs/README.md:24                        links it
docs/catalog.json:467                    indexes it
skills/worker-pool/SKILL.md:31           cites the path
skills/worker-pool/scripts/pool_delivery.py:4   cites the path
tests/pool-beat.test.py:4                cites the path
tests/pool-supervision.test.py:4         cites the path
tests/worker-pool-design-retractions.test.py:9-10   resolves docs/ by path, for it AND the notes file
docs/worker-pool-design-notes.md:3       relative link to it

worker-pool-design-notes.md and worker-pool-design.md cross-link each other relatively, so moving one and not the other breaks the pair — and the notes file is not in this PR's diff at all. docs/catalog.json and docs/README.md are the discoverability surface you named as a fair reason, and tests/worker-pool-design-retractions.test.py would fail outright on a move.

Every other design doc lives there too: workspace-design.md, design-core-health-verdict.md, design-credential-capability-resolver.md, pointer-teacher-design.md.

Where that leaves it

I do not think either file should move, and I would rather not do it inside this PR even if we decide they should — that is a rename touching at least eight referencing sites plus a generated catalog, and it belongs in its own change where a reviewer can see it as a rename.

If the owner's hold is meant to cover edits to pool files outside the skill and not just new ones, that is a call above me and I will ask rather than assume. Say which reading you intended and I will act on it.

Thanks for flagging it as the cheapest block rather than as a defect — that framing is why this is a five-minute exchange instead of a re-architecture.

— Sutando core (qingyun-001)

[qingyun-wu]

Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted.