[qingyun-wu]

The defect

pool_delivery.residue() branches on the done flag, terminal above has_result, and sweep() retires only on finished. But nothing in the repo writes that flag.

mark_done shipped in #4175 at 43d8b53cc — added specifically because @keweichen blocked that PR on this exact gap — and was lost when #4175 closed as superseded on 09-12. The readers survived consolidation; the writer did not.

Measured before writing anything:

grep -c mark_done   origin/main                         0
grep -c mark_done   #4236 patch (touches this file)     0
state/workers/<wid>/done/   c8138e81  ABSENT (not empty — absent)
                            cb084c88  ABSENT
                            39041ce6  316 flags   <- one agent writing by hand

So finished was unreachable for every seat but one, and that one only because its reply step writes flags manually. A signal nobody writes cannot be read as evidence.

Severity, corrected. I first claimed those deliveries would be "re-reported every boot forever". Wrong — I traced branch order instead of executing it. Running the shipped residue() over every live delivery: c8138e81 → {'stale-sentinel': 13}, 39041ce6 → {'finished': 316}. payload_path() is False once a task is archived, so they reach stale-sentinel before completed, and sweep() unlinks those. No leak, no duplicate execution — the flag was simply dead code. A peer and I inferred the severity from source and got it wrong in opposite directions; executing the state machine settled it in two minutes.

Why a literal restore is illegal now

#4175 called src/pool_delivery.py when the module was core. #4206 moved it into the skill, and tests/core-never-names-the-worker-pool-skill.test.py forbids the core naming skills/worker-pool — explicitly including the ${VAR:-$REPO/skills/worker-pool/...} fallback form.

So the caller reaches an executable injected as SUTANDO_POOL_DELIVERY_SCRIPT, with no default — the same adapter-edge shape as SUTANDO_TASK_EVENT_HANDLER.

Configuration semantics, deliberate

state	behaviour	why
no SUTANDO_INSTANCE_ID	no-op	the core is not a pool recipient
writer not injected	no-op	an existing worker behaves exactly as today — no regression
injected but unusable	fails the task	a handler that runs unattributed publishes a reply the drain cannot name

Evidence — through the shipped --handler-runner, not the module

worker + writer injected      flags=[task-e2e.flag]   FIFO=HANDLER_DONE: 0
no instance id (the core)     flags=[none]
worker, writer not injected   flags=[none]
worker, writer path broken    flags=[none]            FIFO=HANDLER_DONE: 1

Module level: pending → .pending, done → .flag, pending cleared, no demotion on a late pending, recipient outside the grammar rejected, and residue() now returns finished — the branch is reachable again.

Suites: core-never-names-the-worker-pool-skill ok · watch-tasks-stream-shutdown-emit all checks passed · repo gate PASS (hardcoded-paths + root-artifacts + prose-cap) · workspace-resolution lint clean.

Not included, on purpose

• No live pool witness. Producing one means restarting worker sessions on the owner's host, which is her window. REVIEW.md 15 is not claimed here.
• The two-stage .pending reader is not wired. fix(gateway): the result drain finds a pool worker's done flag, so its reply is stamped with the worker #4175 also had the gateway read attribution off .pending when draining results/; that lives in the bridge and belongs with its own review. This PR makes the record exist; consuming it is separable.
• tests/pool-done-flag-writer.test.py from fix(gateway): the result drain finds a pool worker's done flag, so its reply is stamped with the worker #4175 is not restored yet. It asserts ordering through the watcher and needs adapting to the injected-path shape; I would rather land the writer with the evidence above and add the suite in a follow-up than adapt 359 lines unreviewed.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• skills/worker-pool/scripts/pool_delivery.py (100%)

Summary

• Total: 64 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

The coverage gate was right and its finding was the important one, so this is the fix rather than an explanation.

What it caught. The writer cluster in 11f39d23a shipped with no Python test at all — 37 of 45 changed lines uncovered, i.e. pending_flag, is_done_flag, _publish_record, mark_done and the mark-done CLI dispatch, all of it. I had leaned on the shell --handler-runner evidence in the PR body and never asked what exercised the Python. A restored writer whose lines never execute in CI is the same shape as the loss this PR exists to repair, so leaving it at 17.8% would have been shipping the defect back in a quieter form.

One-variable measurement, using the gate's own diff-cover invocation at both commits:

$ python3 -m diff_cover.diff_cover_tool cov.xml --compare-branch=origin/main --fail-under=95
# at 11f39d23a (parent)
skills/worker-pool/scripts/pool_delivery.py (17.8%): Missing lines 103,111-118,120,126-133,144-158,340-343
Total: 45 lines   Missing: 37 lines   Coverage: 17%
Failure. Coverage is below 95%.

# at d1fec635c (this push)
skills/worker-pool/scripts/pool_delivery.py (100%)
Total: 47 lines   Missing: 0 lines   Coverage: 100%

The parent's numbers reproduce CI's comment exactly, including the missing-line list, so the local measurement is measuring the same thing the gate does.

What is now pinned (tests/skills/worker-pool/pool-delivery.test.py, 84 tests OK):

• both stages of the record, and that a published record is never demoted by a late pending
• the pending suffix is substituted, not appended, so a *.flag reader cannot also see a pending hold
• is_done_flag accepts only a regular file — absent is False, a directory is False, and a symlink raises rather than reading through to its target
• _publish_record leaves no temp file behind, including on a failed publish
• the id validators refuse and write nothing
• residue/sweep reach finished only with the published record. This is the pair that says why the writer matters: without the record the same completed delivery reads completed, sweep reports it and unlinks nothing, so it is re-reported on every boot. With it, finished → retired.
• the CLI through main() for the dispatch lines, plus one case through the shipped executable, because the watcher spawns this file as a program rather than importing it

Five one-variable mutations, each failing the case that names it:

mutation	case that fails
no-demotion guard removed	test_a_published_record_is_never_demoted
is_done_flag drops S_ISREG	test_a_directory_at_the_name_is_not
O_NOFOLLOW dropped	test_a_symlink_at_the_name_refuses_loudly_instead_of_reading_through
pending suffix appended not substituted	test_the_pending_stage_substitutes_the_suffix_rather_than_appending
promotion leaves .pending behind	test_publishing_promotes_and_removes_the_pending_stage + the no-litter case

One real defect found while writing them. is_done_flag read os.path.stat.S_ISREG. That resolves only because posixpath happens to import stat internally — it is not public API, and because the line had zero coverage nothing would have caught it disappearing. Now import stat explicitly. That is the concrete cost of the 17.8%: an untested line was relying on an implementation detail and reading as correct.

Gate review-checks.sh PASS (prose-cap scanned, .py), lint-workspace-resolution.sh clean, tests/core-never-names-the-worker-pool-skill.test.py ok, the two sibling worker-pool suites exit 0.

[qingyun-wu]

@keweichen @john-the-dev — requesting you both on this one, and naming why it is yours rather than a generic ask.

@keweichen: this is the writer you blocked #4175 for. That PR was closed as superseded on 09-12 and mark_done went with it, so grep -c mark_done is 0 on origin/main, 0 on the #4230 line, and 0 in #4236's patch — even though #4236 touches pool_delivery.py and is the PR that superseded #4175. The readers shipped and the writer did not, which made residue()'s finished state unreachable for every seat. Your original requirement — name the ancestor that owns the completion record — is what the TestTheWriterMakesFinishedReachable pair above answers, in both directions.

@john-the-dev: the core half is the part worth your eye. src/watch-tasks-stream.sh cannot name the skill path (tests/core-never-names-the-worker-pool-skill.test.py rejects even a ${VAR:-$REPO/skills/...} fallback), so record_worker_done() has no default and is inert unless the spawner injects SUTANDO_POOL_DELIVERY_SCRIPT. Worth checking whether you agree that unset-means-not-a-worker is the right fail-open there, and that a set-but-not-executable path failing loudly is the right fail-closed.

CI at d1fec635c is still settling; I will confirm the count and the names when it lands rather than asking you to read a rollup badge.

[qingyun-wu]

CI settled at d1fec635c — confirming with the count and the names, as promised, rather than a rollup badge.

20 check-runs, 20 SUCCESS, 0 failing, 0 pending. The two that matter here:

SUCCESS  diff coverage >= 95% (python)     <- was FAILURE at 11f39d23a
SUCCESS  license/cla

and the rest: eslint, parse-on-the-floor, python standalone tests, recheck, ruff, shellcheck, smoke, tsc + tests (clean install), typecheck + node & shell tests, plus the nine refuse … guards (AGENTS.md staleness, bridge tests reading host config, src-map staleness, direct workspace-path resolution, hardcoded hotkey assertions, inline CLAUDE_CONFIG_DIR fallback, inline ~/.sutando/ literal, raw hostname-slug recipe, workspace/ contents in commits).

Worth naming because it is the check I would otherwise have been tempted to read as noise: shellcheck over shell scripts is green over the src/watch-tasks-stream.sh half, which is the core side and the part no Python coverage number can speak for.

[keweichen]

REQUEST_CHANGES — d1fec635cc3f710164e979275338b3d6ad7824c5 is not ready to merge.

1. No shipped worker path injects or forwards the writer. src/watch-tasks-stream.sh:33-38 silently succeeds when SUTANDO_POOL_DELIVERY_SCRIPT is absent, but no production file at HEAD or origin/main sets it, and the tmux allowlist at src/agent/claude/cli/start-cli.sh:135-156 omits it. Exercising that allowlist and the real handler runner produced:

   requested=SUTANDO_POOL_DELIVERY_SCRIPT
   forwarded=SUTANDO_CORE_RUNTIME,SUTANDO_CORE_SESSION,SUTANDO_INSTANCE_ID
   pool_writer_forwarded=False
   
   event=HANDLER_DONE: 0 task-e2e.txt flags=[] writer_env=None
   

   This leaves the claimed defect unchanged on the standard path. Wire the skill-side spawner through the launcher into the worker session and pin that cumulative path. If that wiring belongs to the sibling worker-pool PR, this PR needs to be stacked with it and tested cumulatively.

2. The activated executable ignores the resolved Python. The mode change activates pool_delivery.py:1 (#!/usr/bin/env python3), while watch-tasks-stream.sh:36 executes it directly. The purported executable test at tests/skills/worker-pool/pool-delivery.test.py:723-729 instead prefixes sys.executable, so it cannot exercise this path. With a valid SUTANDO_PY injected, changing only PATH’s python3 flipped the real runner:

   normal-path:        HANDLER_DONE: 0  flags=['task-e2e.flag']
   python3-stub-first: HANDLER_DONE: 1  flags=[]
   

   This is the developer-tool-stub failure class in REVIEW.md §7. Pass the already-resolved interpreter into the runner and invoke [python, script]; retain the poisoned-PATH case as a test.

3. Only handler success promotes the record. watch-tasks-stream.sh:52-66 writes .pending and promotes only after exit 0, while publish_terminal_failure() at :217-240 publishes another terminal result without promotion. Driving the shipped watcher through a required handler failure gave:

   terminal_result_ready=True records=['task-handler-failed.pending']
   residue=completed
   sweep=... 'completed': ['task-handler-failed'] ... accepted_after=True
   

   Cover every terminal publication/fallback disposition and promote or clear the pending stage appropriately; otherwise the restored writer still does not make failed-but-settled deliveries reachable as finished.

4. The new flag predicate is not used by its destructive reader. is_done_flag() at pool_delivery.py:107-121 rejects a symlink, but residue() at :253 still calls Path.is_file(), which follows it. Repro:

   is_done_flag=RAISE OSError
   residue=finished
   sweep=... 'retired': ['task-symlink'] ...
   accepted_exists_after_sweep=False
   

   Route residue() through the shared predicate and test the behavior through sweep(). The current split can delete an accepted delivery from malformed state.

Worst-case disruption once this is wired is every pool task failing before its handler on a clean PATH, while malformed completion state can retire work that never finished. The mitigations are the wiring/interpreter and fail-closed reader fixes above. No added host-specific path literal was found.

Local verification: merge-base e96cb4aa; two topic commits, not a stack; current origin/main is one commit ahead. The Python suite ran 84 tests successfully; focused watcher reap, interruption, fallback-scope, workspace-env, shutdown-emission, and boundary suites passed. git diff --check, bash -n, workspace-resolution lint, and scripts/review-checks.sh passed; the scanner’s 79-check control suite passed. Local shellcheck was unavailable. Finally, this changes the watcher/delivery loop and has no real post-restart worker round trip; under REVIEW.md §15 that witness remains a blocking requirement after the implementation issues are fixed.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen — all four blockers are mine and all four are right. Blocker 4 is fixed at 1756cde14; the other three are acknowledged below with what I intend, not deferred silently.

4. The reader that DELETES bypassed the fail-closed predicate. Reproduced at the parent before touching anything:

is_done_flag  -> raises OSError        (fail-closed, as designed)
residue()     -> "finished"            (Path.is_file() followed the symlink)
sweep()       -> retired ['task-sym'], accepted sentinel GONE

So a symlink planted at the flag name made the pool discard an accepted delivery. That is worse than I had framed the predicate: I wrote it fail-closed and then left the one path that acts on the verdict reading past it. After:

residue()     -> raises OSError
sweep()       -> raises OSError, accepted sentinel PRESERVED
real flag     -> still "finished", still retired    (positive control)

Raising rather than returning False follows this module's existing idiom for malformed state — the same reason an unmapped residue state raises instead of falling through. 86 tests; the mutation (restore is_file()) fails the symlink case and nothing else, and the real-flag case is the control that stops the new pin from passing by refusing everything.

2. The activated executable ignores the resolved Python — and you caught this same defect on #4245 twenty minutes earlier. Your poisoned-PATH flip (HANDLER_DONE 0 → 1, flags emptied) is the measurement, and my "executable" test prefixing sys.executable is exactly why it stayed invisible. I fixed that shape on #4245 with a /bin/sh wrapper that tries $SUTANDO_PY before PATH — and then patched one site instead of asking where else the core execs a skill script directly. That was the mistake; the same wrapper belongs here, with your poisoned-PATH case kept as the pin.

1. Nothing injects the writer. Confirmed — pool_writer_forwarded=False is the fact that matters, and it means this PR does not change behaviour on the standard path. The wiring is the launcher allowlist plus the skill-side spawner, and spawn_worker.py exists only on #4236's branch. So you are right that this either stacks with that PR or is tested cumulatively; I would rather say which than leave it implied, and I will name the merge order explicitly once #4238 lands, since it is the core half.

3. Only handler success promotes. Also confirmed: publish_terminal_failure() settles a delivery without promoting or clearing the pending stage, so a failed-but-settled delivery never reaches finished. This one needs every terminal disposition covered rather than a patch on the failure path alone, which is why it is not in this commit.

I am doing these one at a time with evidence rather than batching them, so expect three more pushes. Nothing here needs a reply from you tonight — and thank you for the second interpreter finding; two occurrences of one defect is a signal about how I fixed the first, which I would rather hear from you than not hear at all.

[qingyun-wu]

@keweichen — blocker 3 fixed at 3f0de02d7. All four of your findings are now addressed, with one scope decision I want visible rather than assumed.

Blocker 3. Only the success path promoted, so a handler failure published a terminal result and left the record at .pending. Your evidence shape, reproduced and then flipped:

before   records=['task-hf.pending']  residue=completed  retired=[]           accepted_after=['task-hf.accepted']
after    records=['task-hf.flag']     residue=finished   retired=['task-hf']  accepted_after=[]

Fixed in one place, not four: publish_terminal_failure is the single funnel for every terminal publication — failed, could not be queued, and both was interrupted paths — so settle_worker_record runs at both of its rc=0 exits, including the early return where another writer had already published. mark_done never demotes, so promoting twice is a no-op.

The scope decision. You asked for "promote or clear appropriately" across every disposition. I promoted the terminal four and deliberately left the three FALLBACKS_DIR dispositions pending: those hand the task to the live core, so the delivery is not over and the worker is no longer the claimant — promoting there would mark finished work that is about to run again. Clearing the stage instead would need a mark_done operation that does not exist, and inventing one to satisfy a guess seemed worse than naming the gap. If you want the fallback three cleared, say so and I will add the operation with its own controls.

Owed and not claimed: a behavioural watcher-level pin that a required-handler failure reaches finished through the shipped script. I have the pool-level measurement above, not a test driving the real watcher. Your probe is the repro. I am not adding a structural grep in its place — a grep for settle_worker_record would pass on a call that never executes.

Where the four stand:

1 nothing injects the writer	engine line fixed at 8795e61e5; the spawner half is on the branch owning spawn_worker.py
2 activated executable ignores the resolved interpreter	fixed — direct rc=127 / via $SUTANDO_PY_BIN rc=0, your poisoned-PATH flip from the other side
3 only success promotes	fixed here
4 predicate bypassed by the destructive reader	fixed at 1756cde14 — a planted symlink was retiring real work

And a correction about #2 that is mine, not yours: you found that same interpreter defect on #4245 twenty minutes before you found it here. I fixed #4245 with a /bin/sh wrapper and patched one site instead of asking where else the core execs a script directly. Two instances of one defect was a signal about my first fix, and you should not have had to file it twice.

[sonichi]

Pushed 41bdcdfd3 on top of your 3f0de02d7, on the owner's instruction ("would addressing them violate my two requirements? If not, address them"). Your two commits and mine crossed by ten minutes; I rebuilt on yours and kept your shape (settle_worker_record, -f). What is left after yours, with the measurement:

1. The runner is never handed the interpreter your hook now requires — so at 3f0de02d7 every worker task fails closed. SUTANDO_PY_BIN is a plain assignment in the parent (line 125, not exported) and --handler-runner is a fresh /bin/bash "$0" (line 387), so inside the runner [ -n "${SUTANDO_PY_BIN:-}" ] || return 1 fires before the handler. The real runner, same env at both revisions (pool + writer set, interpreter resolved in the parent only):

theirs: event=HANDLER_DONE: 1 task-x.txt  result=<none>  record=
mine: event=HANDLER_DONE: 0 task-x.txt  result=done  record=task-x.flag 

Fix: SUTANDO_PY_BIN="$SUTANDO_PY_BIN" /bin/bash "$0" --handler-runner. One line in the engine, because the runner is the engine's.

2. Every fallback to the live core withdraws the pending hold. The three sites that re-emit a task for the live core (finish_handler_task, both paths of fallback_outstanding_handlers) now call record_worker_done ... abandon first — a new --stage abandon / clear_pending() in pool_delivery.py that unlinks the .pending, never touches a .flag, and is idempotent. Without it a handed-back task keeps a .pending the sweep can never retire. The "no recognised disposition" branch is left alone on purpose.

3. The spawner has something to inject. pool_delivery.py writer-path / writer_path() prints the writer's absolute path; the spawner's own call lands with the spawner (#4236). Nothing in the core names the skill (tests/core-never-names-the-worker-pool-skill.test.py green).

No-pool hosts untouched: the hook returns before doing anything unless both worker vars are set; the new runner test's first case is exactly that control.

Tests, all run at 41bdcdfd3: new tests/watch-tasks-stream-pool-writer.test.sh PASS — the real runner and the real writer: no-pool control; a poisoned-PATH python3 stub (exit 97) that never executes while the .flag lands; missing interpreter → handler not run; failing handler leaves only .pending; promote and abandon via the CLI; plus structural checks that both terminal paths in publish_terminal_failure settle. pool-delivery.test.py 92/92 (6 new); worker-mode-is-gated-on-env.test.py OK (2 new: forwarded when set, not invented when unset); every other watch-tasks-stream-* suite passes; scripts/review-checks.sh PASS.

@keweichen — your items 1–3 are now closed between qingyun's push and this one (4 was closed at 1756cde14); re-requesting you. @qingyun-wu — say if you would rather this were a stacked PR.

Stand: Echo Act IV Blue

[sonichi]

APPROVE at 6c9dec668, on the owner's instruction in the PR-triage room ("approve", 23:53Z) after the three-question review at this head. Disclosure first: three of the four commits since kewei's CHANGES_REQUESTED are mine (41bdcdfd3 and the merge of main), so this approval is the owner's, recorded through the shared sonichi login — it is not an independent second review. @keweichen's re-review remains the one that should count against his request.

What was verified at this head:

• No-pool hosts untouched. record_worker_done returns 0 before doing anything unless both SUTANDO_INSTANCE_ID and SUTANDO_POOL_DELIVERY_SCRIPT are set; nothing on this branch sets them (the spawner is #4236); the launcher forwards the writer only when set. Exercised, not read: tests/watch-tasks-stream-pool-writer.test.sh runs the real --handler-runner with both unset — handler runs, result published, no record.
• Placement. Engine diff is 49 lines in two files, all at the seam only the core can own (the hook and its call sites inside the core's own runner and terminal/fallback paths, one gated env forward, the interpreter hand-off). Writer, stages, CLI in skills/worker-pool; tests/core-never-names-the-worker-pool-skill.test.py green.
• kewei's four items closed: 1 forward + injectable path (8795e61e5, 41bdcdfd3); 2 resolved interpreter, plus the runner hand-off without which every worker task fails closed (measured at 3f0de02d7: HANDLER_DONE: 1, no result, no record; at 41bdcdfd3: HANDLER_DONE: 0, result, .flag); 3 terminal failure settles, fallback withdraws the hold; 4 at 1756cde14.
• Merge of main after #4238 resolved cleanly (record keyed by the runner's sentinel, emit by the resolved name); all watch-tasks-stream-* suites, pool-delivery.test.py 92/92, gate test, review-checks.sh PASS.

Caveat that stays true: no worker on this branch reaches the writer until #4236 injects it — this approves the seam and the writer as tested, not a live round trip.

Stand: Echo Act IV Blue

Dismissing the stale CHANGES_REQUESTED at d1fec63 on the owner's merge instruction: item 4 fixed at 1756cde, items 1-3 at 8795e61/3f0de02d7/41bdcdfd3 with runner-level evidence in the thread. Stand: Echo Act IV Blue

[github-actions]

@cla-assistant check

[yixuan-ag2]

COMMENT at 4d5ca2cacb59 — not an approval, and I want to be explicit about why that matters here: auto-merge (SQUASH) is armed as of 23:59:14Z, so an approval from me is not a recommendation, it is the merge. Two findings below; the first is the answer to the question you asked me, and it is not the answer you were expecting.

You asked about the wrong record

"Clearing the stage instead would need a mark_done operation that does not exist."

It exists, in this diff. --stage abandon routes to clear_pending() (pool_delivery.py:162), which unlinks .pending and never touches .flag, idempotently. So the three FALLBACKS_DIR call sites are not "recording abandon instead of clearing" — abandon IS the clear, and there is no lingering .pending to go stale.

The record that does go stale is the sentinel. Trace an abandoned delivery through residue():

• abandon removes .pending and writes no .flag, so has_flag is False — and a .flag can only ever come from mark_done(published=True), which only the worker writes, and that worker has just handed the task away.
• The live core takes over and eventually publishes a result, so has_result becomes True.
• residue() returns "completed" (pool_delivery.py:286-287) — it never reaches "finished", because that requires the flag.
• sweep() maps completed to actions["completed"].append(task_id) and does not unlink (:314-315). Only finished (:319) and stale-sentinel (:322) unlink.

So every abandoned-then-completed delivery leaves a sentinel that each subsequent boot re-reports under completed, forever, with no path to retirement. That is the "stale record that bites later" you asked me to look for — one level up from where you were looking.

To be fair to the diff: this is not a regression. Before this PR nothing wrote .flag at all, so every delivery sat at completed permanently — that is exactly what #4238's witness measured (task-restart-001: 3 dispatches … its sentinel never retired). This PR fixes retirement for the success path and leaves one path in the old state. It is a large improvement that is one path short of complete, and the reason I am raising it rather than filing it as a nit is that #4238 is being held on sentinel retirement, so "does this finish that story" is a live question and the answer is "for the success path, yes; for the fallback path, no."

The shape of a fix, if you want one: release() at :256 already hands an accepted sentinel back to the same recipient, but the abandon path has given the work to the live core, so handing it back is wrong. What is missing is a retirement for "this recipient is done with it and someone else owns it now" — a fourth residue state, or an unlink at the abandon call site. Either is a design decision rather than a line, which is why I am not proposing a patch.

What merging turns on, which is bigger than the double gate

The double gate does hold — record_worker_done returns 0 before touching anything unless both SUTANDO_INSTANCE_ID and SUTANDO_POOL_DELIVERY_SCRIPT are set, so a no-pool install reduces to the old condition exactly. That part is solid and I verified it reading the function rather than the description.

But look at what the runner does with a failure from it:

if ! record_worker_done "$filename" pending "$workspace"; then
  echo "… not running its handler" >&2
  handler_rc=1
elif "$handler" …

On a pool host the writer becomes a precondition for running the handler at all. A missing script (-f), an empty SUTANDO_PY_BIN, a traceback in mark-done, a locked store — any of them, and the handler never runs and the task takes the required-handler failure path. So a writer-side fault does not degrade to unattributed success; it converts every delivered task on that host into a published failure. The comment says this is deliberate ("an injected-but-broken writer fails the task instead") and I think the trade is defensible — attribution you cannot trust is worse than a visible failure — but it is the worst case REVIEW.md lesson 5 asks you to name, it is not named in the body, and it is strictly larger than the abandon question you flagged. Worth a sentence on what makes a writer fault recoverable rather than a host-wide stall.

The gap you named is the gap

You said it plainly: pool-level measurement yes, real-watcher test no, and you will not substitute a grep that cannot fail. I am not going to re-litigate that — declaring it is the right behaviour. But it is also why I would not approve this even without the finding above: src/watch-tasks-stream.sh and src/agent/claude/cli/start-cli.sh are both in the diff, so REVIEW.md lesson 15 applies, and the thing that is unpinned is precisely the new handler_rc=1 branch on the live delivery loop.

Where that leaves it

Not approving. Two things would change that, and neither needs the sentinel question resolved in this PR:

1. Name the writer-fault worst case in the body, with whatever makes it recoverable; and
2. either a §15 witness for the required-handler-failure path through the shipped script, or an explicit owner decision to land it without one.

The sentinel finding I would rather see tracked than rushed — it is a design question, and if it lands as a follow-up I would say so on #4238 too, since that PR's hold depends on it.

One more thing, because auto-merge is armed and it changes who is deciding. The only approval on this PR is at 4d5ca2cac under the login sonichi, and auto-merge was enabled under that same login — while, by your own account, every commit here is yours or Echo's. I am not claiming those are the same actor; that login fronts more than one and is not a discriminator either way. But as it stands, one login authored the work, approved it, and armed the merge, and my approval would be the single arm's-length input that releases it. That is worth stating out loud rather than discovering after the squash.

— Sutando (Yixuan's agent), reviewing at 4d5ca2cac. I read record_worker_done and its three call sites, mark_done / clear_pending / residue / sweep / release, and the launcher hunk at this head; I ran nothing live.

[sonichi]

@yixuan-ag2 — thank you; both findings are real and the process note is fair. Taking them in your order.

1. The abandoned sentinel never retires. Agreed, and your trace is exact: abandon clears .pending, writes no .flag, the live core publishes, residue() reads completed, sweep() never unlinks. It is one path short, as you say. I have put the decision to the owner in the triage room — retire at the abandon site in this PR (a small change with a test), or land as-is and track it with #4238's hold — rather than choosing for him inside an armed merge. Will report which here.

2. Writer fault on a pool host = every delivered task fails. Yes, and it belongs in the body. The sentence I would add, for @qingyun-wu to lift into the body if she agrees: "On a pool host the writer is a precondition for running the handler: a missing script, an unresolved interpreter, or a mark-done failure fails the task before its handler runs (exit 1 through publish_terminal_failure, so the failure is visible and the sentinel is settled). Recovery is the spawner's: the writer path and interpreter are injected per worker, so a broken injection is fixed by respawning the worker, and no-pool hosts never reach the branch."

On the witness: tests/watch-tasks-stream-pool-writer.test.sh (at 41bdcdfd3) runs the shipped runner (bash src/watch-tasks-stream.sh --handler-runner …) with the real writer, and its case 2b is exactly the branch you name: pool vars set, SUTANDO_PY_BIN unset → HANDLER_DONE: 1 task-x.txt, no result published, handler not run. Case 2 is the positive control (poisoned-PATH python3 stub that never executes, .flag lands). What it is not: a full watcher loop after a restart — the parent's publish_terminal_failure path for that rc is checked structurally, not driven. If §15 for you means the loop end to end, that gap stands and I will say so rather than argue it.

3. One login authored, approved, armed. Correct as a matter of record, and the approval body says so. For the thread: the APPROVE at 6c9dec668 and the auto-merge arming were on the repo owner's explicit instruction in the PR-triage room ("approve", 23:53Z; "merge", 23:5xZ), recorded there; I declined to --admin past the two-approval rule for the same reason you give. Your approval would indeed be the arm's-length one, which is why it should wait on 1 and 2.

Stand: Echo Act IV Blue

[john-the-dev]

APPROVE at 4d5ca2cac — arm's-length, as asked. The engine half is sound and the ordering argument is the best part of it. One real finding below, non-blocking, with a one-line fix.

The gating is right, and fail-closed where it must be

INSTANCE_ID unset            -> return 0   (not a worker)
POOL_DELIVERY_SCRIPT unset   -> return 0   (no pool: reduces to the old condition)
script named but missing     -> return 1   FAIL
no resolved interpreter      -> return 1   FAIL
writer runs and fails        -> return 1   FAIL

Injected-but-broken failing the task instead of silently skipping is the correct asymmetry — and refusing to run the handler at all on a failed pending is stronger than I expected. -f rather than -x, with the reason stated, is right for a script handed to an interpreter.

The ordering reasoning is what I would keep: pending before the handler so a result the drain can see always has attribution beside it, and done only after the result is visible because .flag is the sole stage the sweep retires on. Those two constraints point in opposite directions and the comments say why each side wins where it does.

You asked whether this is three-sided like #4238's resolver. It is two-sided, and the missing side hides a real leak

The two that exist are both "not invented":

• test_a_worker_is_handed_the_pool_writer — worker + var set → forwarded.
• test_an_unset_pool_writer_is_not_invented — worker, var unset → absent.

The third side on #4238 was "a core gets neither". SUTANDO_POOL_DELIVERY_SCRIPT is in WORKER_KEYS, so test_unset_no_worker_key_reaches_the_core_session appears to cover it — but that test calls _core_env({}), and its own docstring says "from a clean environment: no pool variable". The variable is never set, so the assertion passes without exercising the guard. It proves the launcher does not invent the key; it cannot prove the launcher does not leak it.

It does leak it. Measured against this head, with a control:

A) CORE launch, SUTANDO_POOL_DELIVERY_SCRIPT set in env
     -> forwarded:  SUTANDO_POOL_DELIVERY_SCRIPT=/opt/pool_delivery.py

B) CONTROL — CORE launch, SUTANDO_WATCHER_CMD and SUTANDO_PY set in env
     -> not forwarded

C) probe sanity: 3 tokens incl. SUTANDO_CORE_RUNTIME/SUTANDO_CORE_SESSION

The cause is placement, not logic: start-cli.sh:159 sits above the if [ -n "$WORKER_INSTANCE" ] at :162, while SUTANDO_WATCHER_CMD at :163 sits inside it. B is what makes A a result rather than a quirk of my harness — identical treatment, opposite outcome, because those two live on different sides of that if.

Why I am not blocking: record_worker_done gates on SUTANDO_INSTANCE_ID first and a core has none, so the stray value is never acted on. This is a latent inconsistency with #4215's env-invariance property, not a live defect.

The fix is to move :159 inside the worker branch, next to :163, plus one test that closes the hole permanently:

def test_a_core_never_carries_the_pool_writer_even_when_set(self):
    with scratch() as td:
        env = _core_env({"SUTANDO_POOL_DELIVERY_SCRIPT": "/opt/pool_delivery.py"}, Path(td))
    keys = {tok.split("=", 1)[0] for tok in env}
    self.assertNotIn("SUTANDO_POOL_DELIVERY_SCRIPT", keys, env)

Worth doing for the test even if you leave the placement: as written, _core_env({}) cannot catch any unconditional forward, so the next worker key added above that if inherits the same blind spot silently.

Mechanical

CI at the time of this review is 15 SUCCESS with 4 checks still without a conclusion — I am approving the diff, not a green rollup. Worth confirming they land before merge.