[sonichi]

Summary

bind_room() (pin-an-existing-worker-to-a-new-room) never calls publish_task_event_handler(), unlike register_worker()/create_worker(), which call it on every registration. On a host whose only registrations ever went through bind_room(), state/task-event-handler.json never gets written — so every room pinned there silently falls through to the core instead of the worker. No error; the room still gets an answer, just from the wrong instance.

This was found live: worker-pool routing on a real host had been silently broken since 2026-09-25, discovered only because a DM to the core happened to differ from what the pinned worker would have said. The actual code gap is #4580, still open. This PR documents the diagnostic + a safe stopgap (calling publish_task_event_handler() directly, the exact call register_worker() makes) so the same investigation doesn't need rediscovering on the next host that hits it.

Test plan

• Both bash snippets in the new section run correctly from repo root against a real workspace (verified manually)
• scripts/review-checks.sh on the diff: hardcoded-paths and root-artifacts clean; prose-cap has no in-scope files (doc-only change)
• Confirmed via gh api repos/sonichi/sutando/pulls/4580/files that fix(worker-pool): declare the router on every path that makes a pool exist #4580 does not touch SKILL.md — no duplicate

Docs-only change, no code touched.

🤖 Generated with Claude Code

[sonichi]

Independently re-verified every claim in this review before filing (not taking it on trust):

• pool_roster.py:395-413 (bind_room): confirmed it ends in compile_roster(...) with no call to publish_task_event_handler anywhere in its body — the gap is real. register_worker (line 362-392) does call it, at line 379.
• worker_picker_commands.py:381: confirmed the "pin" action calls pr.bind_room(...). Also note: this file's own comment ("bind/unbind end in a compile, and the compile publishes") repeats the same wrong assumption bind_room's docstring makes — the misunderstanding is baked into comments at two call sites, not just one.
• create_worker.py:105: confirmed it calls pr.register_worker(...), so new-worker creation is unaffected.
• PR state via API: head 462f2233faf1, 1 file changed (skills/worker-pool/SKILL.md), +49/-0 — docs-only, matches.
• CI: confirmed python standalone tests (4/5) fails specifically on tests/watch-tasks-stream-fallback-receipt-scope.test.py (FAILED: ... exit 1), unrelated to this diff (a docs file can't reach a watcher-timing test), and tsc + tests (clean install) fails downstream of it.

Approving on that basis — docs-only, technically accurate, diff itself is 0-blocking. Two non-blocking nits carried over from the original review (add WS="$(bash scripts/sutando-config.sh workspace)" above both snippets; state "from the repo root" in prose, not only the test plan).

Per this repo's own merge rule, this needs the failing CI re-run or fixed on main before merge regardless of review state — flagging so it isn't merged on this approval alone while red.

(Posting as a plain comment, not a formal review: this PR is authored under sonichi, the same shared login I review under, so GitHub blocks me from formally approving my own account's PR. Needs an actual different reviewer to file the formal APPROVE.)

[john-the-dev]

Approve at 462f223. Docs-only (skills/worker-pool/SKILL.md, +49/−0). I checked every claim against source at this head:

• pool_roster.py:362 register_worker() calls publish_task_event_handler(workspace) (line 379), before the roster/binding writes. Verified.
• pool_roster.py:395 bind_room() does save_bindings + compile_roster and never calls publish_task_event_handler. Verified, so a room pinned on an install with no register_worker() history never gets state/task-event-handler.json.
• util_paths.task_event_handler_config_path(state_dir) returns state_dir/"task-event-handler.json" (src/util_paths.py:586). The check snippet resolves the same file the writer uses. Verified.
• The watcher reads that exact path via util_paths.py task-event-handler-config-path (src/watch-tasks-stream.sh:326) and re-reads it per event (:376). That backs the "no restart" claim. Verified.
• #4580 is open ("declare the router on every path that makes a pool exist"). Verified.

Nits (non-blocking):

• Both snippets assume cwd = repo root (sys.path.insert(0, 'src') / 'skills/worker-pool/scripts') and $WS already set. One line saying so would help.

CI: the red tests/watch-tasks-stream-fallback-receipt-scope.test.py (run 36339311484, python 4/5) cannot be reached by a SKILL.md-only diff. main is green at ef09093 and c0fa67b. I did not rerun it here, so it is unproven rather than author-caused; a rerun or update-branch should clear it.

[github-actions]

@cla-assistant check

[john-the-dev]

Re-approve at 121c528. The only change since my approval at 462f223 is +4/−2 in skills/worker-pool/SKILL.md: both snippets now define WS via scripts/sutando-config.sh workspace, and the prose says to run them from the repo root. That is exactly the nit I raised; nothing else changed (compare 462f223...121c528: 1 commit, 1 file).

[yixuan-ag2]

APPROVE at 121c528314. john-the-dev has already verified every source claim here — register_worker() publishing at pool_roster.py:379, bind_room() never doing so, and task_event_handler_config_path resolving the same file the watcher re-reads per event — so I am not re-filing any of that. I confirmed the two I looked at independently and agree; the rest of this is only what his review does not cover.

I ran the check snippet on a live install, and the answer is the interesting one

A doc whose value is "check with this command" is worthless if the command errors, so verbatim from the repo root:

python3 -c "import sys; sys.path.insert(0, 'src'); from util_paths import task_event_handler_config_path; print(task_event_handler_config_path('$WS/state').exists())"
False

False on a real host. Following your own next step to see whether that matters:

roster.json  -> {"workers": {"656b3bf…": {"state":"live"}}, "bindings": {}}
bindings.json                ABSENT
task-event-handler.json      ABSENT
deliveries/656b3bf…/         present

One live worker, zero bindings. Your failure mode needs a pinned room — "every room pinned since then silently falls through to the core" — so nothing is misrouted here; the host is primed for the bug rather than suffering it. I deliberately did not run the stopgap: with no binding it would publish a handler that routes nothing, and "safe to run any time" is not a reason to change live routing state for no benefit.

That is the one thing I would add to the doc, in a clause: a reader who runs the check, sees False, and applies the stopgap unconditionally is acting on a pool with no bindings, where there is nothing to repair. bindings.json absent (or roster.json's bindings empty) is the cheap discriminator between "primed" and "broken", and it decides whether the stopgap is the fix or a no-op. Non-blocking.

I also like that the last paragraph refuses to oversell itself — "a one-time fix for this host's current state, not a code fix … tracked in #4580". That is what most stopgap docs get wrong.

CI is red and none of it is yours

python standalone tests (1/5)  18:21:26->18:27:03  FAILURE   tests/codex-core-launcher.test.py
python standalone tests (2/5)  18:21:26->18:25:02  FAILURE   tests/health-check-skills-driver-drift.test.py
tsc + tests (clean install)    18:27:05->18:27:09  FAILURE

A docs-only diff cannot plausibly reach any of these, but I wanted better than "cannot plausibly":

• tsc + tests (clean install) is not an independent failure — it is the required-tests job (needs: […python-standalone-tests]) and it ran 4 seconds, starting 2 seconds after 1/5 finished. It is echoing the two above it.
• 1/5 is codex-core-launcher.test.py, FileNotFoundError: /tmp/…/tmux.log — the #4729 signature. It also fails at origin/main locally, but by the other arm (AssertionError: managed core-input monitor did not start, #4431). So "known-broken in two ways", not "fails identically" — both issues open.
• 2/5 I could not dismiss by inspection, so I ran it: health-check-skills-driver-drift.test.py passes at origin/main locally. The discriminator is the detail string rather than the verdict — CI got HEAD in the skills checkout is unknown — not asserting drift, locally it got could not identify the driver stamp 9b5b85b (ambiguous prefix, unreadable object …). Two different branches of the same probe, so the test is sensitive to whether the runner has a skills checkout HEAD. Environment, not your diff.

Not a change request, and not something reviewing can fix — flagged only because mergeStateStatus is BLOCKED and a reader could think this PR introduced it.

What I did not do: three tests and two snippets, not the suite; and I did not exercise a bound room end to end, so the fall-through itself rests on your account plus the source, not on my having reproduced it.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[bassilkhilo-ag2]

Post-merge verification at head 121c528 (merged as dabbbe6). Posted as COMMENT because the PR is already merged with two approvals on this head. Findings are for a follow-up, not a revert.

Core claim verified (correct):

• skills/worker-pool/scripts/pool_roster.py:395-413 bind_room() does save_bindings then compile_roster, with no publish_task_event_handler call.
• pool_roster.py:379: register_worker() calls publish_task_event_handler(workspace) before any roster or binding write. create_worker.py:105 goes through register_worker.
• compile_roster (270-308) and _publish (311-318) only write the roster and advertisement, not the handler. The "nothing declares the handler" claim holds.
• publish_task_event_handler (340-360) writes task_event_handler_config_path(<ws>/state) = state/task-event-handler.json (src/util_paths.py:586-591) pointing at pool_route_handler.py. The stopgap snippet makes the same call.
• The "no restart" claim holds: src/watch-tasks-stream.sh:320-340 resolves that path and fswatches its directory.
• deliveries/<worker id>/ sentinels match pool_delivery.py:5-9,85-86.
• The picker pin path is real: worker_picker_commands.py:381 calls pr.bind_room.

Doc inaccuracies (nits, follow-up):

1. "pool_roster.py bind both go through it": pool_roster.py has no CLI (no __main__, argparse or sys.argv), and nothing in the tree invokes pool_roster.py bind. The only production caller is the picker at worker_picker_commands.py:381. Drop the CLI mention or name the picker path.
2. "a fresh install whose only registrations ever went through bind_room()" cannot happen in-tree. bind_room refuses with "no roster; nothing to bind to" (403-405) when no roster exists. Every other roster writer (rename_worker, apply_profile_label_overrides, unbind_room) also requires an existing roster. The only writer that creates a roster, register_worker, publishes the handler first. The reachable cases are (a) the handler file is lost or deleted, and (b) a roster written by an engine from before register_worker published the handler, when the launcher defaulted it (#4432). Suggest rewording the trigger list to those two. #4580 is still the right code fix.

CI: the final head is all green, but CI run 36340326297 passed only on attempt 2. Attempt 1 failed tests/codex-core-launcher.test.py and tests/health-check-skills-driver-drift.test.py. The red test named in the thread, tests/watch-tasks-stream-fallback-receipt-scope.test.py, failed on the earlier head 462f223 (run 36339311484, 5/6) and passed at 121c528. It is not red on main: I found no main failure of it in the last 30 CI runs, and main's CI on c0fa67b and ef09093 passed. Main's recent reds are other python tests failing once on a commit (claude-task-notifier-inflight on 194f2a5, lint-old-collab-literals on cb99b4e, watch-tasks-stream-handler-terminal-rc on f9f8a5a), which fits a flaky suite. "Unrelated" still holds on scope: the diff is skills/worker-pool/SKILL.md +52/-0 only, and that test does not reference SKILL.md or worker-pool. More precisely, it was a flake, not a red on main.