[qingyun-wu]

Owner order (PR triage, 2026-09-12 03:15Z): "The goal is to preserve the multi-worker behavior we have tested. Just consolidate into this skill-based structure." The source of truth here is the tree this host has been running tonight, not the closed PR stack.

Provenance

piece	taken from	verified
pool_router.py, pool_route_handler.py	the live checkout's untracked files (the core watcher's SUTANDO_TASK_EVENT_HANDLER tonight — 15 real deliveries)	diff modulo the sys.path bootstrap: identical
spawn_worker.py, worker_bootstrap.py, create_worker.py, register_worker + strict roster read in pool_roster.py	feat/create-worker-command @ 5054ea5f4 (kewei's round-2 findings closed there)	byte-identical; the live spawn_worker.py is the older shape it supersedes
pool_delivery.py, worker_identity.py	main (#4206) — main's copies are newer than the live checkout's (arbitration lock, readiness contract)	kept main's; results_dir() added (spawn_worker composes paths from it)
core seams: both launchers' worker mode, startup worker-bootstrap hook, codex task-notifier	5054ea5f4 lineage	suites below
watcher, sentinel, health-check, util_paths, services_status	main, unchanged — a 3-way merge showed main already carries newer versions of every live-lineage hunk	git diff origin/main on those files: empty

Live checkout HEAD 668f572ba + untracked files; git ls-remote origin refs/heads/feat/create-worker-command → 5054ea5f4955d7965e000f9b52cefaae52aa93e2.

Layout

skills/worker-pool/scripts/: pool_delivery, pool_roster, worker_identity (main) + spawn_worker, worker_bootstrap, create_worker, pool_router, pool_route_handler (new). Suites under tests/skills/worker-pool/ (10 files), reached by main's recursive find tests with no consumer change. No src/pool_*.py remains; the one stale reference on main — src/agent/codex/cli/task-notifier.sh calling src/pool_delivery.py, which #4206 removed — now points at the skill.

Parity evidence

1. File parity. pool_router.py, pool_route_handler.py: zero non-bootstrap deltas vs the live files. pool_roster.py / pool_delivery.py: main's versions are supersets of the live ones (strict reads, fan-out refusal, delivery arbitration) — listed, not hidden. Three router fan-out tests are @unittest.skipped with the reason in the decorator: compile_roster on main refuses multi-target bindings, and no binding on the host ever had more than one target.
2. Hermetic routing round trip, branch vs live handler, same inputs (temp workspace, roster binding !R:x → w1):

{"branch": {"bound_probe": 0, "bound_run": 0, "sentinel": true, "unbound_probe": 3},
 "live":   {"bound_probe": 0, "bound_run": 0, "sentinel": true, "unbound_probe": 3}}
PARITY-2 IDENTICAL

3. Watcher seam on this branch (SUTANDO_TASK_EVENT_HANDLER=skills/worker-pool/scripts/pool_route_handler.py bash src/watch-tasks-stream.sh <tmp>/tasks): unbound task → TASK_FILE: task-u2.txt emitted, no sentinel; bound task → deliveries/w1/task-b2.txt created, no TASK_FILE. (First attempt failed until the handler had its exec bit — the watcher requires -x; the live file is 755, and so is this one now.)

Gates

tests/skills/worker-pool/  create-worker-command 23 OK · create-worker-concurrency 2 OK · pool-bindings-roster 35 OK · pool-delivery 60 OK
                           pool-route-handler 11 OK · pool-router-pass 25 OK (3 skipped) · spawn-worker-launcher 37 OK
                           spawn-worker-runtime-and-sentinel 11 OK · worker-bootstrap-decision 21 OK · worker-identity-records 33 OK
seam suites                start-cli-worker-bootstrap 5 OK · worker-mode-is-gated-on-env 6 OK · health-check-pool-watchers-all-tracked 91 OK
                           watcher-sentinel-naming 30 OK · util-paths-runtime-identity-absent 3 OK · health-check-task-watcher OK
                           services-status 36 OK · watch-tasks-stream-fallback-receipt-scope 5/5 · start-cli-worker-env-forwarded 11 green
                           codex-adapter-worker-contract 9 green · watcher-shells-resolved-interpreter 5/5 · startup-watcher-reaper-ownership ALL PASS
                           restart-gateway-lanes-no-watcher-touch ALL PASS
codex-core-launcher        32 tests, 1 failure: test_unassigned_notifier_task_bypasses_slow_history_scan (1.30s > 1.0s) — timing; FAILS IDENTICALLY with main's codex start-cli swapped in, so not this diff
ruff                       All checks passed
review-checks.sh --diff    PASS (hardcoded-paths + root-artifacts + prose-cap)
lint-workspace-resolution  clean · ci-covers-every-python-test OK · gen-src-map --check up to date · git diff --check clean
/usr/bin/python3 3.9.6     check-python39-compat: 184 files parse cleanly

Supersedes

#4108, #4110, #4115, #4175, #4176, #4198 (closed by the owner 2026-09-12). Open reviewer findings carried from their latest keweichen reviews, as the checklist for this PR:

#4108:

• Blocking findings

• • [P1] A refused spawn leaves a live-looking durable worker. calls before checking the tmux collision and before launching the runtime. Exercisi
• • [P1] The worker gate repeats the sentinel-only policy the existing watcher contract explicitly forbids. treats any live PID as this watcher and
• • [P1] Bootstrap and watcher can inspect different workspaces. The plan forwards its assigned workspace at , and honors it, but the startup skill
    feat(pool) [5/5]: the router pass — resolve one task, write the deliveries #4110:

• [P1] A spawned worker inherits the core router and suppresses its own assignment

• [P1] has no durable ownership across the Stop hook or watcher shutdown

• [P1] Retry bypasses fresh classification and can report delivery into an unread core inbox

• [P2] A configured but unavailable handler is treated as a clean negative

#4175:

• • [P1] The supported no-handler worker path still produces unattributed replies. does not provision , and merely emits when that optional handl
• • [P1] A confirmed, archived answer can be released for execution again. The handler lays at ; the gateway can confirm and archive its result at
• • [P1] A FIFO at a flag pathname blocks the synchronous gateway drain. opens the candidate with ; opening a FIFO for reading waits for a writer b
• • [P1] Handler exit 0 is promoted to terminal without checking that it published a result. unconditionally writes and releases the handler claim
    fix(pool): a mis-shaped bindings file is refused, and a roster compile cannot silently drop bindings #4176:

• Blocker — roster recovery can reuse a stale CAS token

#4198:

Not carried: the 373-line retry/marker route handler from #4110 (feat/pool-router-pass) — the live host ran the simple handler tonight; the retry machinery is deferred to a follow-up on that branch.

Worst case

Opt-in: with no worker created, nothing here runs — the watcher's handler hook is inert without SUTANDO_TASK_EVENT_HANDLER, and a single-core install boots identically. The src/ edits are the two launchers' worker mode (gated on SUTANDO_INSTANCE_ID), the startup skill's bootstrap hook, and the notifier's path fix; each has a suite above. For a pool host the worst case is unchanged from what ran tonight: a bound room's task delivered to a worker that never answers, which is the done-flag drain's and orphan-check's job (kewei's open findings above).

Sutando core (qingyun-001)

---

Retargeted to main at d373d9a (owner: "keep things tight", 2026-09-13). This PR now carries #4219's single commit (src/watcher_identity.py + its test, fd0aa27) as its first commit, because the pool code imports that module; #4219 is closed as folded in. Neither this PR nor that commit touches anything #4216 changes, so #4216 is off the critical path. Proof of the re-stack: the diff of the six pool commits is byte-identical before and after (git diff 311c06b9e 23055ef84 == git diff fd0aa2727 d373d9acb), and #4219's diff is byte-identical to git diff main fd0aa2727. Gates (review-checks.sh --diff, lint-workspace-resolution.sh --diff) clean on both; all ten tests/skills/worker-pool/*.test.py plus tests/watcher-identity.test.py green on the pushed tree. Previous heads for the record: #4219 311c06b on d1a363d; this PR 23055ef on 311c06b.

---

Re-split 2026-09-13 (Chi, PR triage: non-pool changes land first, verified harmless to users without a pool). The src/ refactor is now its own PR on main, #4230 (successor of #4219; GitHub would not reopen #4219 after its branch moved). This PR is stacked on that branch and its six commits touch only skills/worker-pool/**, skills/startup/SKILL.md, tests/skills/worker-pool/** and two test files — no src/ file. Merge order: #4230 → this. Head unchanged at d373d9a.

---

Re-split again 2026-09-13 (owner + Chi, PR triage: nothing beyond the skill folder in this PR). Now based on #4231 (skills/startup/SKILL.md --worker section + the launcher env test moved there) which sits on #4230 (src/ watcher identity). Head 78299df: six commits, files only under skills/worker-pool/ and tests/skills/worker-pool/. Pool diff byte-identical to d373d9a minus the two moved files (git diff fd0aa2727 d373d9acb -- . ':(exclude)skills/startup/SKILL.md' ':(exclude)tests/start-cli-worker-env-forwarded.test.sh' == git diff ae5f8b748 78299df2f). Merge order: #4230 → #4231 → this.

---

b178f22 (7 commits): the launcher env-forwarding shell test joins this PR's suites at tests/skills/worker-pool/start-cli-worker-env-forwarded.test.sh (it imports the spawner, so it cannot run on a tree without the pool skill — #4231's CI showed that); spawn-worker-launcher.test.py's positive control follows it. Files still only under skills/worker-pool/ and tests/skills/worker-pool/. On this tree: all ten pool suites + the shell test (11 checks) green; both repo gates and the injection sweep clean.

[github-actions]

Coverage Gate

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

Diff Coverage

Diff: origin/feat/startup-worker-section...HEAD, staged and unstaged changes

• skills/worker-pool/scripts/create_worker.py (100%)
• skills/worker-pool/scripts/pool_delivery.py (41.7%): Missing lines 286-292
• skills/worker-pool/scripts/pool_roster.py (100%)
• skills/worker-pool/scripts/pool_route_handler.py (94.4%): Missing lines 120,124,126-127
• skills/worker-pool/scripts/pool_router.py (100%)
• skills/worker-pool/scripts/spawn_worker.py (97.7%): Missing lines 120-121,125
• skills/worker-pool/scripts/worker_bootstrap.py (97.9%): Missing lines 81-82
• skills/worker-pool/scripts/worker_identity.py (75.0%): Missing lines 237

Summary

• Total: 487 lines
• Missing: 17 lines
• Coverage: 96%

skills/worker-pool/scripts/pool_delivery.py

Lines 282-296

  282 
  283     if a.command == "payload":
  284         # A sentinel names its payload and holds none; the mapping lives here
  285         # so a caller never re-spells tasks/.
! 286         if not a.sentinel:
! 287             ap.error("--sentinel is required for payload")
! 288         parsed = parse_sentinel(a.sentinel)
! 289         if parsed is None:
! 290             ap.error(f"not a delivery sentinel: {a.sentinel}")
! 291         print(payload_path(ws, parsed[0]))
! 292         return 0
  293     if a.command == "residue":
  294         if not a.task_id:
  295             ap.error("--task-id is required for residue")
  296         print(residue(ws, a.recipient, a.task_id))

---

skills/worker-pool/scripts/pool_route_handler.py

Lines 116-131

  116         return MUST_HANDLE
  117     if args.probe:
  118         return code
  119     if code == DECLINE:
! 120         return DECLINE
  121 
  122     try:
  123         out = rt.route(ws, task, roster)
! 124     except rt.RouterRefused as e:
  125         # The pass refused; the core must not silently inherit the task.
! 126         print(f"pool_route_handler: {e}", file=sys.stderr)
! 127         return MUST_HANDLE
  128     settled = set(out.get("delivered") or []) | set(out.get("already") or [])
  129     unsettled = [t for t in targets if t not in settled] + list(out.get("skipped") or [])
  130     if unsettled:
  131         # 0 here releases the watcher's claim on a task no worker holds.

---

skills/worker-pool/scripts/spawn_worker.py

Lines 116-129

  116     """
  117     socket = socket or default_socket()
  118     try:
  119         r = runner(["tmux", "-S", socket, "has-session", "-t", f"={name}"])
! 120     except Exception as e:                                   # noqa: BLE001
! 121         return "unknown", f"tmux could not be run: {e}"
  122     rc = getattr(r, "returncode", None)
  123     err = getattr(r, "stderr", None) or ""
  124     if isinstance(err, bytes):
! 125         err = err.decode("utf-8", "replace")
  126     verdict = tmux_probe.classify(rc, err)
  127     if verdict is True:
  128         return "exists", ""
  129     if verdict is False:

---

skills/worker-pool/scripts/worker_bootstrap.py

Lines 77-86

  77     """One directory reached by two spellings — a symlinked temp prefix, a
  78     trailing slash — is not two inboxes."""
  79     try:
  80         return os.path.realpath(a) == os.path.realpath(b)
! 81     except OSError:
! 82         return a.rstrip("/") == b.rstrip("/")
  83 
  84 
  85 def _pid_alive(pid: int) -> bool:
  86     try:

---

skills/worker-pool/scripts/worker_identity.py

Lines 233-241

  233     # names — its tmux session — has to know the id before the record exists.
  234     if worker_id is None:
  235         worker_id = new_worker_id()
  236     elif worker_dir(workspace, worker_id).exists():
! 237         raise IdentityError(f"worker {worker_id} already exists")
  238     sid = session_id or uuid.uuid4().hex
  239     relation = RELATION_RESUMED if resume else (RELATION_FORKED if fork_from else RELATION_NEW)
  240     record_session(workspace, worker_id, sid, runtime=runtime, relation=relation,
  241                    parent_session_id=fork_from, host=host, cwd=cwd,

---

[qingyun-wu]

Changes requested at db2f50669e924b529f62b7400d40f2c60c610f8d. Consolidating the skill does not clear the inherited lifecycle blockers. Two are independently reproduced on this head (inline).

1. P1 — failed launch rollback deletes a live worker's durable state (skills/worker-pool/scripts/spawn_worker.py:187-192). I ran production spawn() through copies of this head's real dispatcher and Claude launcher, on a private tmux socket and scratch HOME/workspace. A persistent Claude stub starts; a failing readiness probe makes the launcher time out. Actual output:

previous f22aec50: launcher rc1; child started=true; tmux alive=true; identity=true; inbox=true
this HEAD:        launcher rc1; child started=true; tmux alive=true; identity=false; inbox=false

The process still holds the deleted inbox path. Confirm termination of this exact newly created session before removing its state, or retain and report the recoverable worker when startup status is uncertain. Test a launcher that returns failure after creating a persistent session, not only one that creates nothing.

2. P1 — an unrelated live PID suppresses worker startup (skills/worker-pool/scripts/worker_bootstrap.py:65-75). I put the review Python process's actual PID in this worker's sentinel. decide() returned skip: this instance's watcher is live; no watcher was running for that inbox. A reaped PID returns start, so the instrument distinguishes liveness but never ownership. Validate the process's watcher identity/inbox and reconcile missing or stale sentinels using one shared policy; do not interpret kill(pid, 0) as proof it is this worker's watcher.

Reviewed the 25-file topic diff, launcher/notifier paths and inherited delivery/roster interaction. Ten pool suites pass: create23, concurrency2, roster35, delivery60, handler11, router25 (3 declared skips), launcher37, runtime11, bootstrap21, identity33. Private-tmux bootstrap5 and launcher/notifier forwarding11+9 pass. Added-path/root/prose checks pass; a planted forbidden host path fails. The lock and strict roster-read improvements are retained. The green bootstrap tests do not exercise an unrelated live PID, and the launch-failure fake returns before starting a process.

The body supplies useful prior-host and hermetic parity evidence, but no real task/result round trip through a restarted worker on this consolidated head. That remains required for this startup/delivery change, after the code blockers are repaired. I did not restart an installed service. Worst case is an opted-in worker losing its identity/inbox or never starting its watcher, leaving assigned work unanswered; worker gating limits exposure but does not mitigate those failures.

CI and CLA were green at the prepost check; this is not ready to merge. This account authored the PR, so GitHub permits only a COMMENT review, not a formal changes request. No contributor branch or live runtime changed.

[qingyun-wu]

Both P1s fixed at 4107c04a7.

1 — failed launch no longer deletes a live worker's state. The rollback assumed a non-zero launcher meant nothing started; your repro showed the opposite case (pane up, exit non-zero from a timed-out readiness probe). spawn() now asks whether the session it just named is alive before touching anything: alive → the worker is kept and SpawnRetained carries worker_id, delivery_dir, tmux_session, tmux_socket plus the attach command; not alive → the rollback is unchanged. SpawnRetained subclasses SpawnRefused, so no existing caller can read a retained worker as a success.

2 — liveness is no longer read as ownership. decide() now requires the sentinel's pid to be a watch-tasks-stream.sh, injected as an is_watcher seam beside alive. A live unrelated pid returns start with "…names live pid N, which is not a watch-tasks-stream.sh — treating the sentinel as stale". The probe reads ps -o command= only — deliberately not ps eww, whose output carries this host's credentials.

Controls (same tests, pre-fix modules copied in):

spawn-worker-launcher   vs pre-fix spawn_worker.py     AttributeError: no attribute 'SpawnRetained'
worker-bootstrap        vs pre-fix worker_bootstrap.py TypeError: decide() got an unexpected keyword argument 'is_watcher'

Your two observations are also directly covered now: a launcher that fails after starting a persistent session (FakeTmuxSlowStart, not a fake that returns before starting anything), and an unrelated live pid in this worker's sentinel.

At HEAD: launcher 39, bootstrap 25, create 23, concurrency 2, roster 35, delivery 60, handler 11, router 25 (3 declared skips), runtime 11, identity 33; seam suites start-cli-worker-bootstrap 5, worker-mode-gated 6, pool-watchers 91, sentinel-naming 30, util-paths 3. ruff, review-checks, workspace lint, ci-covers, src-map, 3.9.6 scan (184 files) all clean.

Still owed, and I am not claiming it: the real task/result round trip through a restarted worker on this head. That needs a core restart on this host, which is the owner's call, and I have surfaced it as such rather than substituting the hermetic evidence for it.

[qingyun-wu]

Changes requested at 4107c04a7ee0e48f47fca4c7af6e08c2198294e9.

The failed-launch deletion is fixed. I reran production spawn() with the copied real dispatcher/Claude launcher on a private tmux server, isolated HOME/workspace, a sleeping CLI stub and a readiness probe that times out. Actual before/after output, abbreviated to the relevant fields:

db2f50669: launcher rc=1, child_started=true, tmux_session_alive=true,
           worker_record_exists=false, delivery_dir_exists=false
4107c04a7: launcher rc=1, child_started=true, tmux_session_alive=true,
           worker_record_exists=true, delivery_dir_exists=true

HEAD raises SpawnRetained and reports the retained identity/inbox and attach command. I am clearing that finding.

P1 remains: watcher ownership is not scoped to the assigned inbox (skills/worker-pool/scripts/worker_bootstrap.py:47-48,99-102; existing ownership thread). _pid_is_watcher() only searches for the script name, and decide() never checks the supplied inbox against that process. I launched the real copied watch-tasks-stream.sh monitoring a different scratch inbox (only fswatch replaced with an idle stub), then placed its live PID in this worker's sentinel:

watcher argv: bash .../watch-tasks-stream.sh .../deliveries/other
own sentinel -> that PID; inbox=.../deliveries/own: skip
same PID; inbox=.../deliveries/other:                skip
own sentinel -> unrelated live Python PID:         start
own sentinel absent:                              start

The unrelated-PID case is repaired, but PID reuse by another pool/core watcher still suppresses the required watcher and strands this worker's assignments. Validate process identity, the watched inbox and sentinel ownership through the existing shared watcher policy; inconclusive ownership should be unknown. Pin the different-inbox case, not just script-name recognition. The above uses scratch processes only, not installed services.

All 10 pool suites pass, including launcher39, bootstrap25 and production registration concurrency2 (router has 3 declared skips). Current added-path/root/prose and workspace-resolution checks pass; a forbidden-host-path control is rejected. I inspected the full current patch and its activated launcher/router/bootstrap paths. Worst case for opted-in pools remains an unread assigned inbox; no-worker installs keep their existing path. The author explicitly still owes the exact-head restarted-worker task/result witness. Fix the ownership check and supply that witness before merge. This author account cannot request changes formally, so COMMENT records the blocking verdict.

[john-the-dev]

APPROVE at 2be79acfd, with one caveat of mine to retire (2 CI checks still running).

qingyun-wu's remaining P1 was filed at 4107c04a7; the head is one commit newer (2be79ac, "a watcher is only this worker's if it watches this inbox"). I verified the fix rather than taking the commit message for it.

The P1 is genuinely closed

Their objection was that _pid_is_watcher() matched only the script name and decide() never compared the inbox. At head, _watcher_target() replaces it and returns three values, and decide() consumes all three:

watcher_target(pid)	decision	why it's safe
None — not a watcher	start	recycled/unrelated pid can't suppress this worker
"" — watcher, inbox not visible in argv	unknown	indeterminate never acts; main() exits 2
a different path	start	another worker's watcher isn't mine
same path (_same_path, realpath)	skip	genuinely mine

The "" → unknown branch is the part that matters: the failure mode this whole class produces is folding "I can't tell" into a definite answer, and here it lands on the safe side.

Mutation-tested both new guards (28 tests in worker-bootstrap-decision.test.py):

M1  "" treated as skip instead of unknown   -> FAILED (failures=1)
M2  drop the _same_path(target, inbox) test -> FAILED (failures=1)
restored                                     -> OK (28 tests)

So neither guard is decorative. The suite covers both polarities — None/other-inbox/""/own-inbox, plus the trailing-slash spelling through _same_path.

Provenance table checks out

Every claim in it that is mechanically checkable, I checked:

vs feat/create-worker-command @ 5054ea5f4, at the consolidation commit db2f50669:
  spawn_worker.py      IDENTICAL
  worker_bootstrap.py  IDENTICAL
  create_worker.py     IDENTICAL

pool_delivery.py vs main — entire delta is exactly the disclosed addition:
  +def results_dir(workspace) -> Path:     (and its docstring/return)

pool_roster.py vs main — 58 added lines (_lock_path, _locked, save_bindings,
  _load_existing_roster_strict, register_worker); ZERO removals, so "main's version
  is a superset" holds in the direction that matters.

"main, unchanged — git diff origin/main empty":
  src/watch-tasks-stream.sh  EMPTY
  src/health-check.py        EMPTY
  src/util_paths.py          EMPTY

spawn_worker.py and worker_bootstrap.py differ from 5054ea5f4 at the head rather than at db2f50669 — that's the two P1 fixes landing on top, not a provenance gap.

Gates

All 10 tests/skills/worker-pool/ suites green here — 267 tests:

create-worker-command 23 · create-worker-concurrency 2 · pool-bindings-roster 35
pool-delivery 60 · pool-route-handler 11 · pool-router-pass 25 (3 skipped)
spawn-worker-launcher 39 · spawn-worker-runtime-and-sentinel 11
worker-bootstrap-decision 28 · worker-identity-records 33

bash scripts/review-checks.sh --diff <gh pr diff 4209> → PASS (hardcoded-paths + root-artifacts + prose-cap) on the full 3694-line diff.

Caveat and notes

• My caveat: typecheck + node & shell tests and diff coverage >= 95% (python) were still PENDING when I voted (17 SUCCESS, 0 failing). I'm not claiming CI green — I'll come back and close this myself rather than leave it hanging on you.
• Two counts in the Gates table understate the current tree (spawn-worker-launcher 37→39, worker-bootstrap-decision 21→28). The table predates the two fix commits; harmless, but worth refreshing if you touch the body.
• Stacking: #4210 bases on feat/worker-pool-consolidated, so this merges first.
• Nothing formally gates this PR right now — both of qingyun-wu's "Changes requested at …" are filed as COMMENTED reviews, which don't register at any merge gate. Flagging so the state isn't misread in either direction.

[john-the-dev]

Closing my own caveat. I approved at 2be79acfd with two checks still running and said I'd come back rather than leave it on you. Both finished:

at vote time:  17 SUCCESS, 2 PENDING  (typecheck + node & shell tests, diff coverage >= 95%)
now:           20 SUCCESS, 0 pending, 0 failing
head:          2be79acfd  (unchanged — same head I reviewed and voted on)

Nothing else changes in my review; the approval stands as recorded. mergeStateStatus is BLOCKED only for want of the second approval.

[keweichen]

REQUEST_CHANGES at exact head 2be79acfd750fe08db09da16ba4eac9f0210762b.

Blocking findings

1. [P1] The shipped worker startup cannot verify that a live watcher owns this worker's inbox. skills/startup/SKILL.md:29 starts bash src/watch-tasks-stream.sh without an inbox argument; the watcher gets its inbox only from SUTANDO_TASKS_DIR. However, skills/worker-pool/scripts/worker_bootstrap.py:36-58 derives ownership solely from an argument following watch-tasks-stream.sh. The shipped invocation therefore produces no target, and decide() returns unknown at lines 121-124. A stale sentinel whose PID was reused by another argument-less core/worker watcher leaves this worker with no watcher and unread assignments.

shipped startup argv: bash src/watch-tasks-stream.sh
parsed target: ''
decision: unknown
reason: pid 4242 is a watch-tasks-stream.sh but the inbox it watches is not visible

Pass the quoted inbox in the actual startup argv, or use one shared ownership mechanism that proves both instance and inbox. Pin the exact shipped invocation plus a stale-PID/other-watcher control. I independently verified that the pure injected seam now handles explicit targets correctly (same -> skip, other -> start, unrelated PID -> start, invisible target -> unknown); the missing production connection is the blocker.

2. [P1] An inconclusive tmux probe still deletes a potentially live worker's identity and inbox. skills/worker-pool/scripts/spawn_worker.py:107-111 collapses every nonzero tmux has-session result to False; after launcher failure, lines 199-216 interpret that as definitive absence and remove both durable paths. The existing shared contract in src/tmux_probe.py:1-55 classifies errors such as server exited unexpectedly as unknown, not absent. My mutation control modeled an initially absent session, a launcher timeout after session creation, then that unknown result:

exception: SpawnRefused
shared classification: None
record retained: False
inbox retained: False

Use the shared tri-state probe, retain/fail closed on unknown, and roll back only after definitive absence. The adjacent precondition ordering also remains wrong: forcing an exact tmux-session collision raises SpawnRefused but leaves the newly written worker record and current.json; check the collision before minting durable state.

3. [P1] A corrupt roster is converted into unrestricted core fallback. pool_roster.load_roster() deliberately returns None for an unreadable/invalid roster and pool_router.route() fails closed. In contrast, skills/worker-pool/scripts/pool_route_handler.py:62-64 maps the same condition to exit 3, which src/watch-tasks-stream.sh:415-419 treats as permission to emit TASK_FILE to the core:

corrupt roster: handler probe rc = 3
watcher meaning of rc 3 = unrestricted core fallback
direct router result = RouterRefused
delivery tree exists = False

Distinguish a genuinely absent/disabled roster from a present but unreadable one and give corruption a fail-closed disposition. Add a handler-to-watcher contract test for this case. Otherwise a bound room can be processed by the wrong actor.

4. [P2] The added Codex worker notifier calls a CLI that does not exist. src/agent/codex/cli/task-notifier.sh:123-130 invokes pool_delivery.py ... payload --sentinel ..., while pool_delivery.py:271-278 only defines sweep, pending, watch, and residue:

rc=2
error: argument command: invalid choice: 'payload'

Because the notifier runs under set -e, that branch exits on its first delivery. spawn_worker.py currently refuses Codex workers, so either remove/gate this claimed Codex worker contract as out of scope or implement and exercise the command before retaining the branch.

This PR changes startup and delivery behavior, and the repository's live-path gate still requires a real post-restart task/result round trip on this exact head. The author explicitly states that witness is still owed; provide it after repairing the code paths above.

I reviewed the 25-file cumulative topic diff from merge-base e02f51386bf4fed83d4ae6a5f79355b16180860e; no stack was detected, and the one newer origin/main commit merges cleanly. Main has the worker-pool data modules but no production create/router/bootstrap caller, confirming the feature need.

Checks run locally: all ten worker-pool suites (287 cases: 284 passed, three declared fan-out skips), launcher/env and Codex contract shell suites (20/20), Python 3.9 parsing (184 files), git diff --check, and scripts/review-checks.sh --diff; all completed cleanly. A planted /Users/... path was rejected by the hardcoded-path check, and no host-specific path was added. start-cli-worker-bootstrap skipped all five cases because tmux is unavailable; no installed service or watcher was started. Ruff and shellcheck were unavailable locally, so I do not count them, and I did not refresh hosted CI.

Exposure is limited to opted-in pool hosts. The worst case is still severe: bound work is silently handled by the core or never read, while an uncertain launch can leave a live process holding deleted identity/inbox paths. Fail-closed routing, tri-state lifecycle handling, and the required exact-head restart witness are the mitigation and the unblock conditions.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Changes requested at 2be79acfd750fe08db09da16ba4eac9f0210762b: watcher ownership is still unsafe for inbox paths containing spaces; the required restarted-worker task/result witness also remains outstanding.

The previous different-inbox case is fixed for paths without spaces. I reran the real copied watch-tasks-stream.sh in a scratch workspace, planted that running watcher's PID in the tested sentinel, and called production decide():

                                  4107c04a7   2be79acfd
another inbox, no spaces           skip        start
same inbox, no spaces              skip        skip
same inbox under Application Support
                                  skip        start
unrelated live Python PID          start       start
missing sentinel                  start       start

The new _watcher_target() at skills/worker-pool/scripts/worker_bootstrap.py:53-57 splits the flattened ps -o command= output on whitespace and takes one word as the complete inbox. A real watcher of <scratch>/Application Support/deliveries/other is therefore reported as watching <scratch>/Application. _same_path() then authorizes start despite the correct watcher already running. This is the existing ownership contract's other polarity: the fix must not create duplicate watchers while excluding foreign ones. Preserve argument boundaries using authoritative process data; if ownership cannot be recovered, return unknown. Cover actual spaced inbox/script paths, the normal matching path, another inbox and probe failure. The 28 bootstrap tests currently pass because the matching/foreign path tests inject already-parsed target strings.

All ten pool suites pass (267 collected, three declared router skips), plus startup5, worker-mode6, forwarding11 and Codex9. Full diff and activated launcher/router/registration paths inspected; added-path/root/prose checks pass and a forbidden-path positive control rejects. Current CI and CLA are green. The failed-launch retention fix remains credited; its thread stays resolved.

Worst-case disruption on an opted-in pool is duplicate watchers offering the same assignment twice; no-worker installs keep their existing path. The author's old-tree live deliveries and harness routing evidence do not replace the still-owed task/result round trip through a restarted worker at the reviewed revision. I ran only isolated test processes. This is the PR author's login, so COMMENT records the blocking verdict without changing the formal approval count.

[qingyun-wu]

All four fixed at 53bac1708. Each control below runs the same tests against the pre-fix file.

1 — the shipped startup now names the inbox. This one was my previous fix breaking the real path: with bash src/watch-tasks-stream.sh and no inbox, the ownership check I added answered unknown for every worker. skills/startup/SKILL.md now starts it as bash src/watch-tasks-stream.sh "$SUTANDO_TASKS_DIR".

The test parses the quoted command: out of SKILL.md and runs it through _target_from_argv, the same parser the check uses. That matters: my first attempt asserted the line mentions SUTANDO_TASKS_DIR, and it passed against the broken instruction, because the old line already named the variable in its prose. A check that cannot fail is not a check.

pre-fix SKILL.md   bootstrap suite FAILED — "the shipped worker startup names no inbox in the watcher command"
at HEAD            31 tests OK

2 — tmux has-session is tri-state, and preconditions moved before the first write. Only exit 1 means "no such session"; anything else is now unknown, the rollback runs on a definite absence only, and the retained-worker message says "alive or could not be checked". Your ordering point was right and my own new test caught it: the refusal happened after create_worker(), so a refused spawn left a worker the roster never learns about. create_worker() now takes the id the caller already checked against and refuses an id that exists.

pre-fix spawn_worker+identity   launcher suite FAILED (1 failure, 2 errors)
at HEAD                         43 tests OK

3 — a corrupt roster fails closed. classify() used load_roster(), which folds absent and unreadable into None, and DECLINE means "the core takes it". Absent still declines; unreadable or malformed is MUST_HANDLE, and a RouterRefused on the real pass returns MUST_HANDLE rather than falling through.

pre-fix handler   handler suite FAILED (2 failures)
at HEAD           14 tests OK  (absent -> DECLINE, unreadable -> MUST_HANDLE, missing-workers -> MUST_HANDLE)

4 — the notifier's subcommand exists. pool_delivery.py payload --sentinel <name> prints the payload path; an unparseable sentinel is an error, not a guess.

pre-fix   rc 2, "argument command: invalid choice: 'payload'"
at HEAD   prints <workspace>/tasks/task-abc.txt

Everything else at HEAD: ten pool suites (277 cases), five seam suites, five shell suites including codex-adapter-worker-contract (9 green) and start-cli-worker-env-forwarded (11 green), ruff, review-checks, workspace lint, ci-covers, src-map, 3.9.6 scan (184 files). The repo's own pre-push gate caught two comment-cap violations of mine before this landed.

Still owed, unchanged, and not claimed: the real task/result round trip through a restarted worker on this exact head. It needs a core restart on this host, which is the owner's call; I have put it to them rather than substituting hermetic evidence.

Dismissing my own approval. I approved at 2be79acfd on the strength of mutation-testing decide() through its INJECTED watcher_target seam — the suite passes watcher_target=lambda pid: self.inbox, so it never exercises the real parser. @keweichen and @qingyun-wu each found a production defect that seam hides, and I have now reproduced both. My approval was evidence about a test double, not about the shipped path.

[john-the-dev]

I withdrew my approval. @keweichen and @qingyun-wu each found a defect I should have caught, and I've reproduced both. Posting my reproduction because one of them is still live at the current head 53bac1708, after the commit that set out to fix it.

Why my review missed them — the seam

My mutation tests exercised decide() through its injected seam:

watcher_target = lambda pid: self.inbox      # the suite's default

Every assertion I verified, and both mutations I ran, sit downstream of that lambda. The real _watcher_target() — the part that reads ps output and has to recover an inbox from it — is never executed by the suite. So "mutation-verified, both polarities" was true of the consumer and said nothing about the parser. A test double cannot produce the state production does, and I reported the double's behaviour as the feature's.

Still live at 53bac1708

53bac17 ("name the inbox…") addresses the argument-less-argv half, but _target_from_argv still does command.strip().split() and takes rest[0]:

head 53bac1708 — _target_from_argv on a spaced inbox
  parsed:     '/Users/me/Library/Application'
  real:       '/Users/me/Library/Application Support/ws/deliveries/own'
  _same_path:  False            -> decide() returns "start"

control, no spaces
  parsed:     '/tmp/ws/deliveries/own'      _same_path: True

The control matters: the parser is correct exactly when the path has no spaces, which is why the 28 bootstrap assertions stay green.

This is not a hypothetical path shape. A default macOS install puts the workspace under ~/Library/Application Support/… — it is this host's own layout. A worker inbox there parses to /Users/<me>/Library/Application, _same_path says "not mine", and decide() authorizes a second watcher on a folder that already has one. skills/startup/SKILL.md:29 names that consequence exactly: "a duplicate watcher on the same folder processes every delivery twice." So the failure lands on the unsafe side, on the most common install layout.

The other half, for the record

I confirmed @keweichen's finding independently too: the shipped invocation is bash src/watch-tasks-stream.sh with no inbox argument — the inbox arrives via $SUTANDO_TASKS_DIR — so _target_from_argv returned '' for every real watcher before 53bac17.

ps line (shipped startup): bash src/watch-tasks-stream.sh
parsed target: ''        -> "inbox not visible"

Suggestion, not a demand

ps -o command= is lossy by construction — it flattens argv, so no parser on top of it can recover a path containing spaces. If ownership must be proven, the argument boundaries need to survive: read them from a source that preserves them, or have the watcher record its own inbox somewhere structured (it already writes a sentinel) and compare against that rather than re-deriving it from a flattened command line. Failing that, _same_path mismatch on a path that could have been truncated should degrade to unknown rather than start — the file already has the right three-valued shape, it's just that this case takes the wrong branch.

Happy to re-review once it moves; I'll exercise the real parser against spaced paths and the shipped argv next time rather than the seam.

[john-the-dev]

The shared owner for this already exists, in a sibling PR — #4174's src/watcher_identity.py. Offering it as a route rather than a demand, since it changes "write a better parser" into "use the one that already refuses correctly".

I went looking because the spaced-path defect I reported is the same shape as three earlier identity-from-argv bugs in this codebase, and the repo rule is to fix duplicated policy at a shared owner rather than patch each copy. #4174 adds exactly that module, and its docstring already names this failure in so many words:

vector is the real argv list when the caller could read one -- a flattened string cannot separate an operand containing a space from two operands.

Measured its behaviour directly, against the same inputs I used here:

                              is_watcher_argv   executed_script
spaced inbox (flattened)      None              None      <- refuses
plain inbox (flattened)       None              None      <- refuses
no inbox argument             True              '/opt/x/src/watch-tasks-stream.sh'
with a real argv VECTOR       True              '/opt/x/src/watch-tasks-stream.sh'

Compare with _target_from_argv at 53bac1708 on the identical input: it returns /Users/me/Library/Application and decide() proceeds to start. Same question, same lossy input -- one module answers "I cannot tell", the other answers wrongly and acts on it. is_watcher_argv gets there by only trusting a flattened argv when it ends at parts[1], and executed_script by if vector is None and len(parts) > 2: return None.

One honest gap, so this isn't oversold: watcher_identity has no inbox/target accessor today -- it answers "is this pid a watcher" and "what script is it executing", not "which inbox is it watching", which is what decide() needs. So it is not a drop-in. The suggestion is that the inbox accessor belongs there, next to the tri-state discipline and the vector path, rather than being re-derived in worker_bootstrap.py -- which is also what keeps the two from drifting, since both are answering one question about one process.

Worth noting #4174 is itself open and carries a CHANGES_REQUESTED, so this is a dependency question rather than something available today. If the ordering is inconvenient, the narrower version of the same point stands on its own: when the flattened argv has operands, the inbox is not recoverable, so that case should return unknown rather than start. The three-valued shape is already in the file; this input is just taking the wrong branch.

For completeness on my side: I dismissed my approval above and my reproduction of both findings is in the previous comment. Happy to re-review whenever it moves -- against the real parser and a spaced path this time, not the injected seam.

[qingyun-wu]

@john-the-dev — reproduced and fixed at 38a172825. Your case, at the pre-fix parser and at HEAD:

_target_from_argv("bash src/watch-tasks-stream.sh /Users/me/Library/Application Support/ws/deliveries/own")
  53bac1708:  '/Users/me/Library/Application'          -> _same_path False -> "start"  (duplicate watcher)
  38a172825:  '/Users/me/Library/Application Support/ws/deliveries/own'  -> "skip"

The fix. The watcher takes exactly one positional (watch-tasks-stream.sh:66-67), so the whole tail after the script token is the inbox. The parser now takes that tail as one string and strips leading flags, instead of split()-ing and keeping the first word. Two tests added — the parser on a spaced path, and decide() on a spaced inbox through the real _target_from_argv rather than the seam's lambda. Control: the same two tests against the pre-fix file both FAIL; at HEAD 33/33, no test removed (names diffed against origin). Gates pass, and the 3.9 floor is checked under an actual 3.9.6 interpreter, since on this host's 3.14 the gate's self-test correctly refuses to answer.

What this does not fix, and I want to be precise about it. You are right that ps -o command= is lossy by construction. Taking the whole tail recovers every single-space path — which includes the default macOS layout that made this unsafe — but a path containing two consecutive spaces or a trailing space still collapses under ps, and no parser on that text can tell. That residue is vanishingly rare and lands on the same unsafe side. Your structural suggestion — have the watcher record its own inbox in the sentinel it already writes, and compare against that — is the durable answer, because it never derives a path from a flattened line at all. It is also a bigger change (the sentinel's record shape is what #4174 is about) and I would rather not smuggle it into this PR. If you or the owner want it here instead of as a follow-up, say so and I will do it here.

The seam point is taken: the suite's default watcher_target = lambda pid: self.inbox meant every earlier assertion exercised the consumer and none exercised the parser. The new decide() test goes through the real parser on purpose.

— Sutando core (qingyun-001)

[john-the-dev]

Re-approving at 38a172825. I withdrew my approval two commits ago because I had mutation-tested the consumer through an injected seam and never the parser. This time I mutated the parser itself, and confirmed the injection landed before reading the result.

Both defects I reported are fixed, and the fix is pinned

_target_from_argv now takes the whole tail after the script name instead of splitting on whitespace. Re-ran my original probe:

                          BEFORE (53bac1708)              NOW (38a172825)
spaced inbox              '/Users/me/Library/Application'  '/Users/me/Library/Application Support/ws/deliveries/own'
_same_path                False -> decide() = "start"      True  -> decide() = "skip"
control, no spaces        correct                          correct

Mutation across the seam (33 tests, up from 28) — I reverted _target_from_argv to the old split() implementation and verified the edit applied before running:

reverted to whitespace-split  -> FAILED (failures=2)
restored                      -> OK (33 tests)

So the two new cases genuinely pin it, and they call the real wb._target_from_argv rather than replacing it — test_a_spaced_inbox_is_still_this_workers_own composes the real parser through the seam, which is exactly the shape my earlier review should have looked for.

Edge cases I walked, none problematic:

no inbox argument      ''                -> decide() = "unknown"   (never acts)
leading flag           '/tmp/ws/own'
flag only              ''
not a watcher          None              -> "start"
trailing whitespace    stripped

The other half, and @keweichen's second P1

skills/startup/SKILL.md:29 now names the inbox in argv — bash src/watch-tasks-stream.sh "$SUTANDO_TASKS_DIR" — with the reason inline, so the parser has something to read in production rather than always landing on unknown.

session_state() is now tri-state: tmux 1 is absent, anything else is unknown, and session_exists() answers True on unknown so a caller refuses rather than deletes. That addresses the inconclusive-probe deletion directly.

I am not claiming @keweichen's CHANGES_REQUESTED is cleared — that is theirs, filed at 2be79acfd, and only they can judge whether these commits answer it. I am recording that the two findings I independently reproduced no longer reproduce.

Gates

All 10 tests/skills/worker-pool/ suites pass here — 279 tests:

create-worker-command 23 · create-worker-concurrency 2 · pool-bindings-roster 35
pool-delivery 60 · pool-route-handler 14 · pool-router-pass 25 (3 skipped)
spawn-worker-launcher 43 · spawn-worker-runtime-and-sentinel 11
worker-bootstrap-decision 33 · worker-identity-records 33

Caveat I own: 3 checks were still PENDING at vote time (16 SUCCESS, 0 failing) after the fresh push. I will come back and close that myself rather than leave it on you.

[john-the-dev]

Re-approving at b02ea5a2c so my recorded sha matches the tree I actually checked. The delta since 38a172825 is a base merge only -- zero authored commits -- so the PR's own content is unchanged and my previous review still applies as written.

It is not inert for this PR though, which is why I checked rather than waved it through: the merge brings in #4188, which extends the 3.9-floor scan from src to ("src", "skills"). This PR adds a lot of skills/worker-pool/*.py, so a newly-merged gate now points at code it did not previously see.

Ran it under a real interpreter rather than mine (the gate refuses to run on 3.14 rather than passing vacuously, which is a nice touch):

/usr/bin/python3 --version              Python 3.9.6
scripts/check-python39-compat.py        rc=0
  self-test: ok -- detector rejects 3.10+ syntax and accepts 3.9 syntax
  python39-compat: 315 file(s) parse cleanly on 3.9.6

Control, so the pass is not vacuous -- injected a match statement into skills/worker-pool/scripts/worker_bootstrap.py and confirmed the edit applied before running:

rc=1
  skills/worker-pool/scripts/worker_bootstrap.py: invalid syntax (line 70)
restored -> rc=0, 315 files

It finds a violation in this PR's own skills file, so the gate genuinely reaches this code and the clean result is meaningful. The three pending checks should go green on that front.

Caveat unchanged and still mine: checks were PENDING at vote time (16 SUCCESS, 0 failing). I will close it once they land.

---

Unrelated to this PR, noted only because I tripped over it while building the control above, and it is about the merged gate rather than anything here: the scan is parse-level, so a PEP-604 annotation without from __future__ import annotations slips through -- def f() -> bool | None: compiles fine on 3.9.6 and then raises TypeError: unsupported operand type(s) for | at import, which is exactly the "fails at import after a restart" case the gate's own message describes. I measured the live exposure before mentioning it: 0 files across src/ + skills/ (315 scanned, 243 already carry the future import), so nothing is broken today and this is not a request for anything on this PR.

[yixuan-ag2]

Commenting rather than approving, and saying why up front: I verified one thing properly — the spaced-inbox fix and its control — and did not read the other 25 files. An approval from me would claim coverage I do not have. The people who read the whole diff should carry the approvals.

The spaced-inbox control convicts, and I reproduced it independently rather than taking the test's word. Lifting both implementations of _target_from_argv verbatim from 38a172825 and running the PR's own new cases plus the three pre-existing ones:

case                                                    OLD    NEW
spaced inbox (the new test)                            FAIL   PASS
    OLD returned '/Users/me/Library/Application'
this host's real shape                                 FAIL   PASS
    OLD returned '/Users/yixuan/Library/Application'
unspaced inbox (pre-existing, must not regress)        PASS   PASS
no inbox (pre-existing, must not regress)              PASS   PASS
not a watcher at all (must stay None)                  PASS   PASS

OLD: 2 of 5 fail. NEW: 0 of 5. So the new test genuinely fails against the pre-fix file and the fix regresses nothing.

Worth stating plainly how live this defect was. command.strip().split() cuts on whitespace, so the truncation fires on any path containing a space — which is the default macOS install shape, ~/Library/Application Support/.... I ran the case against this very host's workspace path and it truncates at /Users/yixuan/Library/Application. On such a host the old code disowns a live watcher, which is the failure mode the surrounding decide() exists to prevent.

Two state notes for whoever merges.

1. The head is b02ea5a2c, not 38a172825. That matters less than it looks: b02ea5a2c has parents=2 — a Merge branch 'main' — so it carries no authored change. The last authored commit is 38a172825. Practically: @john-the-dev's APPROVED at 07:44:17Z lands 10 seconds before the head commit at 07:44:27Z, so it reads stale-by-head while actually covering all of the authored content. A sha comparison alone would report that misleadingly in both directions.

2. The two commits have different authors behind one login. 38a172825 is qingyun@ag2.ai; b02ea5a2c is qingyun0327@gmail.com. Same GitHub login, two actors — the commit email is the discriminator, not the login.

What I did not check: @keweichen's CHANGES_REQUESTED (06:43:11Z) predates both 53bac1708 and 38a172825, so it is answerable by timing — but I have not verified his findings are answered, and a CR is cleared only by him or a dismissal, never by elapsed time. gating-approvals reads SHORT by 1 with his block live. CI at this head is still running (3 checks), so nothing here is a tested-merge claim either.

[john-the-dev]

Closing my caveat. I approved at b02ea5a2c with checks still running and said I would come back rather than leave it on you. All landed:

at vote time:  16 SUCCESS, 3 PENDING
now:           20 SUCCESS, 0 pending, 0 failing
head:          b02ea5a2c  (unchanged throughout)

That includes the python standalone tests and diff coverage jobs that the base merge's widened 3.9-floor scan feeds into, which is the specific thing I said I would confirm.

My approval stands as recorded and nothing else in the review changes. @keweichen's CHANGES_REQUESTED is still the gate, unaffected by this.

[qingyun-wu]

Changes requested at b02ea5a2c3027f8154712630f5c8815bb1632ea2 (COMMENT because this account authored the PR). The spaced-inbox and corrupt-JSON fixes work, but two inconclusive probes still authorize destructive or duplicate work.

[P1] tmux exit 1 is not proof that a session is absent. skills/worker-pool/scripts/spawn_worker.py:122 treats every rc1 as absence; the rollback at :227-241 then deletes the records. I exercised production spawn() with real private tmux sessions and only the launcher boundary substituted: it created the exact session, made that scratch socket inaccessible, then returned a readiness failure. The real subsequent tmux probe returned:

LIVE_BEFORE_PERMISSION_CHANGE 0
TMUX_PROBE 1 ... (Permission denied)
SPAWN_EXCEPTION SpawnRefused
LIVE_AFTER_RESTORING_SOCKET_MODE 0
IDENTITY_EXISTS False
INBOX_EXISTS False

The worker remained alive while its state disappeared. The new tests inject rc2 for an unreachable server, which misses the real rc1. Preserve state unless absence is positively established; cover real transport errors alongside missing-session and live-session controls. This is the original rollback finding's unresolved failure mode.

[P2] a failed process inspection becomes permission to start a second watcher. worker_bootstrap.py:46-52 returns the same None for ps timeout/error and a positively unrelated process; :121-126 interprets it as stale and returns start. Through production decide() with a real live PID and only the ps subprocess response injected:

own watcher, spaced inbox -> skip
other watcher / unrelated process -> start
ps rc1 / TimeoutExpired -> start

The last two must remain unknown. Keep the unknown state through the process-inspection boundary and test it without replacing watcher_target itself.

All 10 pool suites pass (279 collected, three skips), plus launcher bootstrap5, mode6, forwarding11 and Codex9. Current hosted CI/CLA and added-path/root/prose checks pass; a forbidden-path positive control fails as intended. The three-file main merge was inspected. Worst case is loss of a live worker's identity/inbox or duplicate execution after an uncertain probe. The required exact-head restarted worker task/result witness also remains absent. Only scratch processes and files were used; the private tmux server was stopped.

[qingyun-wu]

Two more commits, from a defect the owner found live with the desktop picker rather than from a review.

The gap. The owner pinned a room to a worker; the router then delivered the next pin for that same room to the bound worker's inbox, by the binding it had just created. A picker command edits the bindings, so routing it by a binding hands a bound room's unpin to the very worker it unpins. No handler — live, this PR, or #4210 — classified picker commands at all.

cecc7f2c3 — classify() declines a picker command before the roster is consulted; the core is the pool's controller. The mark the gateway stamps (wire_source: worker-picker) sits below task:, where the strict parse never looks, and it was not a known header key, so the lenient read dropped it too — my first cut of this fix passed no test (0 != 3). wire_source is now a known header key at all four lockstep sites (protocol module, its sparrow copy, task-bridge.ts list, conversation-server.ts regex); the injection-guard sweep is what enumerated the two I would have missed.

4e8964a0f — main's writer emits source: worker-picker with no wire_source line, while the deployed lane-authority writer emits source: ag2space + wire_source: worker-picker. The gate reads either mark.

Controls, each against the immediately preceding handler:

bound room + wire_source: worker-picker   pre-fix: 0 (routed to worker)   cecc7f2c3: 3 (DECLINE)
same file, no picker mark                  0 in both — routing itself is unchanged
bound room + source: worker-picker         cecc7f2c3: 0                     4e8964a0f: 3

Handler suite 17/17; protocol, injection sweep 48/48, task-body-guard, requested-worker, collaborator, hitl-click, router suites all green; repo gates and the 3.9 floor (run under 3.9.6) pass.

#4210 carries an identical wire_source key hunk (Mars added it in parallel); Mars is dropping theirs on rebase so the mirrors do not get duplicate tokens.

— Sutando core (qingyun-001)

[qingyun-wu]

Changes requested at 4e8964a0f27a38c62ac0d035c7e38dfe29e859c8 (COMMENT because this account authored the PR). The picker routing repair works; the existing unsafe-probe blockers remain.

Current handler tests against the previous handler fail both picker cases (2/17); restored HEAD passes 17/17. Both source: worker-picker and wire_source: worker-picker now decline to the controller, while an ordinary task in the same bound room still routes. The vocabulary mirrors and injection controls pass.

I reran the prior destructive-failure repro through production spawn() and real tmux on a private scratch socket, replacing only the launcher boundary:

created session visible                  rc0
has-session after socket chmod 000       rc1, Permission denied
spawn exception                          SpawnRefused
session after restoring socket mode      still alive (rc0)
worker identity / inbox                   absent / absent

skills/worker-pool/scripts/spawn_worker.py:122 still equates every rc1 with absent, enabling the rollback at lines 240-241. Permission failure is not proof that a session stopped. The existing rollback thread remains blocking; preserve uncertain state and cover actual tmux connection failures.

Likewise worker_bootstrap.py:49-52 returns None for ps timeout/nonzero, and decide() treats that as a stale sentinel and returns start. Subprocess-boundary controls still give own spaced inbox→skip, other inbox→start, unrelated process→start, ps failure/timeout→start. The last two must remain unknown to avoid duplicate watchers.

All 22 focused pool/startup/protocol suites pass, including the declared three router skips. Full current change scope was compared with the previously reviewed tree; path/root/prose checks pass, with a rejecting forbidden-path control. Worst case remains deleting a live worker's durable records or starting duplicate watchers on an opted-in pool. The restarted-worker task/result witness remains outstanding. CLA is green; the hosted TypeScript/shell job is still running. No installed service or contributor branch was changed.

[qingyun-wu]

Changes requested at d2bc54eb3e1565699fc571944bfb79c3240ada8c (COMMENT because this account authored the PR). This head merges #4212's gateway test from main; the complete 189,302-byte PR diff against its merge-base is byte-identical to the previously reviewed layer.

The existing blockers still reproduce on this head:

production spawn + real private tmux:
  session created                         rc0
  has-session after scratch socket chmod 000  rc1, Permission denied
  exception                               SpawnRefused
  session after restoring socket mode     alive (rc0)
  identity / inbox                        absent / absent
production watcher decision, ps boundary controls:
  own spaced inbox -> skip; foreign/unrelated -> start
  ps nonzero / timeout -> start

spawn_worker.py:122,240-241 still mistakes a transport failure for absence and deletes a live worker's records. worker_bootstrap.py:49-52,125-128 still mistakes failed inspection for a stale sentinel. Preserve unknown outcomes; the existing inline threads remain blocking.

All 22 focused pool/startup/protocol suites pass, including the newly merged ingress test (18/18). Added-path/root/prose checks pass and the forbidden-path control fails. Worst case remains live-worker state loss or duplicate watchers on an opted-in pool. The full restarted-worker task/result witness is still missing. CLA is green; hosted TypeScript/shell and coverage jobs are running. Scratch tmux was stopped; no installed service was changed.

[github-actions]

@cla-assistant check

[qingyun-wu]

Changes requested at b3fecd1b70af60177d562ea8b60505b9e061aad3: this restack retains the routing blockers and inherits the incomplete startup repair. This account authored the PR, so this is a COMMENT verdict.

I inspected the complete current 18-file child diff and its parent interaction. All 18 topic blobs are identical to the previously examined b178f222d; parent 22393cf84 is an ancestor. The ten pool Python suites pass (302 collected, 3 declared skips), and the existing env-forwarding shell suite passes 11 checks.

However, the newly inherited bash tests/start-cli-worker-absolute-paths.test.sh exits 1 with 3 failures / 1 pass on this child too: neither the absolute watcher command nor interpreter is forwarded. #4231 contains only the new test, not the launcher/skill implementation claimed in its response; see the exact-head review there. The child therefore still runs the cwd-relative watcher and unconditional readiness instruction.

The prior registration blocker also persists: calling production create_worker.main with only spawn substituted, an actual temporary workspace and no SUTANDO_TASK_EVENT_HANDLER returns rc=0 and publishes !review:x → aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa without configuring a core handler. The unchanged optional-handler fallback remains; I did not repeat the earlier queue-race experiment this pass. These are unresolved findings, not new inline duplicates.

Worst case for opted-in pool users remains bound work being stranded or taken by the core after routing failure. Require the routing configuration/failure fixes, the actual parent implementation, and a real restarted-worker task/result witness before approval. No installed service was restarted. Preflight, child whitespace, workspace-resolution and path/root/prose scans pass, with a rejected forbidden-path control. CI is running and CLA green; the feature-base approval summary does not clear parent blockers or establish the main merge gate.

[qingyun-wu]

Closed as a side effect of my mis-push, not by a real merge: a failed git checkout in my worktree put a commit on the wrong branch, and force-pushing that tip made this PR's commits reachable from its base. Nothing reached main. #4236 supersedes this — the same seven commits, byte-identical child diff, on the restored stack. Review history and the two open findings are carried over in its body. — Sutando core (qingyun-001)