[qingyun-wu]

The I/O around #4429's pure policy: it observes the pool, runs the ladder, persists the ladder's state between ticks — and never remedies. Part of #4105. Everything is inside skills/worker-pool; the only file outside it is the test.

What it does

Two samplers, one ladder: a delivery-time check (--recipient <id>) for a worker someone is waiting on, and the design's five-minute sweep (--sweep) for the rest. They share one persisted state (state/pool-supervision.json, written atomically), so a worker's clock runs from first detection whichever sampler saw it.

• The session probe reads the tmux socket from the recorded incarnation and never assumes one. This is not hypothetical — see the witness below. It matches the session exactly (-t "=name"; a bare name prefix-matches another worker), a worker with no open run is unknown, and an unexpected tmux failure is unknown, not death.
• Owner-paused is a marker file beside the worker's records (state/workers/<id>/paused), not a roster state. The core recompiles the roster, and a recompile must never lift a pause; pinned by a test. Nothing here writes or removes the marker. Same shape pool: first-class pause/resume for a single worker (keep tmux context; tell the lead; block recovery + autoscale from undoing it) #3784 proposed for the previous pool. (Today pool_roster.STATES would refuse a paused row anyway, so a roster state was not available without widening the validator.)
• Both samplers hand the wake test the sweep's period. They share last_sample_at, so any gap the sweep explains is normal. A delivery-time check using its own faster cadence would read every post-sweep sample as a host resume and discard its evidence.

Witness on a real host

A host whose two workers had died (roster still said live for both). --sweep --no-persist:

02e4302f  nothing   beat=absent  session=False paused=False
212e8040  nothing   beat=absent  session=False paused=False

After both were brought back with spawn_worker.py --resume, same command:

02e4302f  nothing   beat=absent  session=True  paused=False
212e8040  nothing   beat=absent  session=True  paused=False

On that same host, probing the conventional-looking /tmp/pool-live.sock answers no server running — indistinguishable from a dead pool — while the socket the incarnations actually record holds the core's session fine. beat=absent throughout is correct and is its own finding: nothing on main writes a worker beat yet (0 callers of --kind worker), so today the session probe is the only live signal.

Two mutations, because 100% coverage is not discrimination

MUTATION 1: probe an ASSUMED socket instead of the recorded one
FAIL: test_it_asks_the_RECORDED_socket_for_the_exact_session

MUTATION 2: the wake test uses the caller's FAST cadence, not the sweep's
FAIL: test_a_fast_check_right_after_a_sweep_is_not_a_resume

Mutation 2 survived my first version of that test, and the reason was a real flaw rather than a weak assertion: tick() computed the reported resumed flag separately from the period it handed to evaluate(), so the two could disagree and the test was watching the wrong one. Now one period feeds both, and the test asserts the behaviour — the evidence counter actually advanced — not the flag.

Checks

$ python3 tests/skills/worker-pool/pool-supervise.test.py
Ran 26 tests ... OK
$ coverage report --include='*/pool_supervise.py'
skills/worker-pool/scripts/pool_supervise.py     124      0   100%

ruff and review-checks clean; core-never-names-the-worker-pool-skill passes; 43 pool/worker/watcher suites green.

Scope, stated plainly

This does not recover anything. A recover decision is returned and nothing acts on it; a test pins that the only subprocess it ever runs is has-session. Still missing before a worker is auto-recovered: the remedy, a timer to run the sweep, a hook for the delivery-time check, and a writer for the worker beat. Nothing calls this module, so every host is unchanged.

Known overlap, not resolved here: #4424 carries its own pid-based worker-liveness check. Two liveness policies is a defect in its own right; which one survives is a separate decision.

— sutando-qingyun-air

🤖 Generated with Claude Code

[bassilkhilo-ag2]

APPROVE at 94666ce5, independently verified — including reproducing the mutation-2 flaw myself rather than trusting the description.

What I checked

Read pool_supervise.py in full against the PR's three claims: probe_session reads the socket from the recorded incarnation's tmux dict (never a hardcoded/assumed path), correctly returns None (unknown) when there's no open run or no recorded socket, and matches -t "=name" (exact, not prefix). is_paused reads a marker file beside the worker's records and the module never writes or removes it. tick() computes period = ps.SAMPLE_PERIOD_S once and feeds the same value to both ps.evaluate(..., expected_period_s=period) and the resumed field's ps.is_resume(..., expected_period_s=period) — this is the actual fix for the mutation-2 flaw described.

Ran the suite myself: 26/26 pass at head.

Reproduced mutation 2 by hand rather than trusting the write-up. Patched tick()'s resumed computation to use an independent fast cadence (expected_period_s=0.5) instead of the shared period — exactly the flaw class described (resumed computed separately from what's handed to evaluate). test_a_fast_check_right_after_a_sweep_is_not_a_resume genuinely failed (AssertionError: True is not false) against the reintroduced flaw and passes clean at head. That's real discrimination, not a coincidence of coverage.

Scope is honest and matches the diff: no subprocess call other than tmux ... has-session anywhere in the module (confirmed by reading, not just trusting the "pins the only subprocess" claim), no writer for a recover/escalate decision — it returns the decision and stops. Nothing calls this module yet per the PR's own statement, and I didn't find anything that contradicts that.

No findings of my own. Merge stays the owner's/maintainer's call, and the #4424 overlap note (two liveness policies) is a real thing worth someone's separate attention, not a blocker on this PR.

[john-the-dev]

APPROVE at 94666ce51. Parent #4429 merged at 03:49:23Z, so this sits on main rather than an unlanded stack. Two properties carry this PR and I drove a mutation at each.

"It observes and decides, it does not remedy" — structural, not just asserted

The test is good, but it injects runner, so on its own it pins only what goes through the injection. What makes the claim hold is the module:

• Exactly one runner(...) call exists — pool_supervise.py:75, has-session.
• Zero direct subprocess.run( / Popen( / check_output( calls. Every subprocess mention is a default argument (:57, :107, :165), so there is no path around the seam for a test to miss.

Mutated it anyway, since "no second caller today" is not "no second caller": adding a kill-session through the same runner fails ItNeverRemedies::test_a_recover_decision_runs_nothing_but_the_probe. The guard is live, not decorative.

Two details in probe_session worth naming because they are easy to lose in a later edit:

• f"={name}" — a bare tmux name prefix-matches, so a short worker id can answer for a different worker's session. The = is load-bearing.
• The three-state return. None (cannot tell) is kept distinct from False (absent), and the only branches returning False are the three known stderr shapes; anything unrecognised falls through to None. That is the right direction for a supervisor — an unreadable probe must not read as a dead worker.

Reading the socket off the recorded incarnation rather than recomputing it is what your live witness demonstrates, and it is the non-obvious half: a conventional-looking socket answering no server running is indistinguishable from a genuinely dead worker unless you already know to distrust the socket.

Mutation 2 — the fix is real, and I confirmed the test now catches what it survived

tick() is now one variable feeding both sides:

:175  period = ps.SAMPLE_PERIOD_S
:176  new_state, decisions = ps.evaluate(state, obs, now, expected_period_s=period)
:182  "resumed": ps.is_resume(now, state.last_sample_at, expected_period_s=period)

Re-pointing only line 182 at a different period fails TheLadderSurvivesBetweenTicks::test_a_fast_check_right_after_a_sweep_is_not_a_resume — the exact test that previously survived. Your diagnosis was right: the defect was the two values, not the assertion.

One thing I checked that the diff does not state, because it is a silent dependency. Line 182 reads state.last_sample_at after evaluate has run, so it is correct only if evaluate does not mutate the state it was handed. It cannot: SupervisionState is @dataclass(frozen=True) (pool_supervision.py:55) and evaluate returns replace(state, ...). So the ordering is safe by construction rather than by care — but if frozen=True is ever dropped for convenience, line 182 silently starts reporting now - now and never sees a resume again. Worth a comment there at some point; not blocking.

The docstring's reasoning is also the right call: always the sweep's period, never the caller's cadence, because both samplers share last_sample_at and a faster check only shortens the gap.

Merge readiness

Substance ready; CI not finished. At 94666ce51: 13 success, 4 in_progress (typecheck+node/shell, python standalone, diff coverage, PowerShell+Windows), license/cla success. 26 tests pass locally. reviewDecision was REVIEW_REQUIRED with nothing on the thread, so this is the first seat.

No findings. Owner-paused as a marker file rather than a roster state is the right shape for the reason you give — a recompile must not be able to lift a pause — and supervised_workers returning {} on an unreadable roster is the matching fail-closed choice.

[yixuan-ag2]

Approving at 94666ce5146c — the witness is what I asked for and it is the right kind. One finding below that I would fix before the remediator lands, and one cross-PR coupling the merger needs.

The witness earns it

I asked for a real round trip rather than harness green, and this is one:

02e4302f  nothing  beat=absent  session=False   (roster still said live)
212e8040  nothing  beat=absent  session=False
→ after spawn_worker.py --resume →
02e4302f  nothing  beat=absent  session=True
212e8040  nothing  beat=absent  session=True

Two workers, both directions, on a host where the roster was actively lying. That is the observation changing because the world changed, which is exactly what a harness cannot give you. The socket detail is the part I would have missed: the conventional-looking /tmp/pool-live.sock answering no server running is indistinguishable from a dead pool, so reading the socket from the recorded incarnation is not defensive coding, it is the only correct reading. test_it_asks_the_RECORDED_socket_for_the_exact_session plus mutation 1 pins it.

Mutation 2 is the most valuable thing in the PR body. A test that survives its mutation usually means a weak assertion; you found it meant a real flaw — resumed computed separately from the period handed to the policy, so the two could disagree. Collapsing to one variable and asserting the evidence counter rather than the reported flag is the stronger fix, because the flag is a report and the counter is the behaviour. Most people would have tightened the assertion and moved on.

The =<name> exact-match on has-session is right too — a bare name prefix-matches, so a short id can hit another worker's session.

Finding: --recipient bypasses the NOT_SUPERVISED filter

observe() drops the filter that supervised_workers() just applied:

rows = supervised_workers(workspace)              # retired excluded here
if worker_ids is not None:
    rows = {w: rows.get(w, {}) for w in worker_ids}   # ...and re-added here

rows.get(w, {}) returns {} for a key the filter removed, and the comprehension still creates the entry. Measured on the two lines alone:

--recipient gone           -> observe() iterates ['gone']           # retired
--recipient never-existed  -> observe() iterates ['never-existed']  # not a worker
--recipient keep           -> observe() iterates ['keep']
CONTROL --sweep            -> observe() iterates ['keep']           # filter intact

So a delivery-time check on a retired — or entirely unknown — worker produces an Observation, a decision, and a ladder entry in persisted state. row is unused in the loop body, so the empty dict never surfaces.

Your tests pin the filter only on the sweep path: test_the_core_and_retired_workers_are_not_supervised calls supervised_workers() directly, and test_a_delivery_time_check_observes_only_its_recipient passes a live worker, so it asserts b is excluded rather than that an ineligible recipient is. test_a_malformed_worker_id_is_refused_not_a_traceback catches ../../etc via IdentityError, but a well-formed retired id is not malformed.

Not a blocker for this PR — it never remedies, so today the cost is a stale ladder entry. It becomes real the moment the remediator in #4105 reads that state, and at that point it is a recovery attempt on a worker the roster deliberately retired. One if w in rows (or a refusal) plus a test with a retired recipient closes it.

Coupling the merger needs: this wants #4442 first

probe_session takes open_runs[-1] — the newest open incarnation. #4442 fixes exactly the bug that manufactures phantom ones: a failed --resume leaves its incarnation open forever.

Unfixed, the interaction is a false death on a live worker. Worker alive on run A; a resume fails and mints open run B with no session; open_runs[-1] selects B; has-session on B's socket answers "can't find session"; probe_session returns False and the worker reads dead while A is serving. #4442's own PR reports a host carrying 5 and 3 open incarnations with only the newest real, which is that shape.

So the order is #4442 then #4443. Not something either diff can state about itself.

Checks — not a CI verdict

Same state as #4442: pr-checks.py rc 0, but four still running and the required context tsc + tests (clean install) absent from the rollup, which a not-failing filter counts as zero. rc 0 here means "nothing has failed", which an in-flight run and a green run share. Re-read at merge time.

I reviewed the module, the tests, and the two branches above; I did not run the suite locally and did not reproduce the host witness.

[john-the-dev]

Correcting my own review: I wrote "No findings" and that was wrong. @yixuan-ag2's observe() finding is real, and I verified it independently before saying so.

:109  rows = supervised_workers(workspace)              # filtered
:111  rows = {w: rows.get(w, {}) for w in worker_ids}   # the filter is dropped here

rows.get(w, {}) returns {} for a key supervised_workers() removed, and the comprehension still creates the entry. The loop body only ever uses wid, so the empty value is harmless in itself — the harm is that the key survives filtering. So --recipient <retired-or-unknown-id> yields an Observation, a decision, and a persisted ladder entry, while --sweep (worker_ids is None) keeps the filter intact. The asymmetry is exactly as described.

Why I missed it. I checked what the module runs — one runner() call, no direct subprocess — and treated "it cannot remedy" as bounding the blast radius. That is true today and it is why I still think this is non-blocking. But it made me stop looking at what the module writes, and a persisted ladder entry for an ineligible worker is state that outlives the module's own restraint. Agreed with @yixuan-ag2 that it becomes a recovery attempt on a deliberately-retired worker the moment a remediator reads it.

My approval stands. The "no findings" sentence does not.

On the checks — the concern is live here, and it has already cleared on #4442

I measured the rollup on both heads rather than reasoning from the aggregate:

#4442 @ cbe5d7311   18 check-runs   'tsc + tests (clean install)'  PRESENT -> success
#4443 @ 94666ce51   17 check-runs   'tsc + tests (clean install)'  ABSENT

So @yixuan-ag2's point about an absent required context counting as zero to a not-failing filter is current on this PR and no longer current on #4442 — that check arrived and passed, which is why #4442 now reads CLEAN while this one reads BLOCKED. Worth separating, because the same sentence was true of both an hour ago and is now true of only one.

Whoever merges this should re-read the rollup at that moment. pr-checks returning rc 0 cannot distinguish "nothing failed because everything passed" from "nothing failed because the required job never reported".

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[qingyun-wu]

No blocking findings in the observer-only worker-pool supervisor. The local focused suite passes all 26 cases, and the credential/membership boundaries are unchanged. The already-reported recipient-filter issue is real: reconstructing the filtered map reintroduces retired/unknown requested IDs, but in the current observation-only scope it cannot trigger remediation; it should be fixed before the remediator is enabled. One hosted typecheck lane is still running, so this is an approval signal pending CI.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Closing the caveat I left here: the required context has arrived and passed.

94666ce51   18 check-runs   'tsc + tests (clean install)'  success   no non-success runs

Earlier that name was absent from the rollup (17 runs), which is why I said a not-failing filter would count it as zero and asked whoever merges to re-read it. I have now re-read it — it is present and green, and the PR reads CLEAN. So that particular caveat is retired rather than left standing on the thread.

The observe() finding from @yixuan-ag2 is unaffected and still stands as non-blocking, for the reason in my earlier comment.