[qingyun-wu]

The bug

check_task_watcher treated every watcher tree not containing the sentinel pid as a duplicate and ended its verdict with "Keep the sentinel's (pid), stop the rest".

A pool runs one watcher per core. The PID sentinel is single-valued and held by whichever core stamped it last, so N-1 of N watchers always read as untracked — and which one holds it is arbitrary. On a live 4-core host the probe named 3 running watchers for termination. Stopping them means every core but one stops draining tasks/.

It fired twice on this host: once with the sentinel held by a dead core ("keep the dead one, stop the two working ones"), and once naming the main core's own watcher — the process that would have executed the kill.

What makes it worse than a noisy warn is that skills/proactive-loop/SKILL.md step 9 instructs an agent to act on this verdict every pass. A guard that depends on the operator already knowing the answer is not a guard.

What this PR ended up being

Two earlier attempts are worth recording, because the second is a trap anyone reviewing this will consider:

1. Count fresh state/cores/*.alive records. Wrong — though an earlier version of this body gave the wrong reason, and the right one is worse. It said that directory is synced across hosts. It is not: scripts/sync-workspace.sh:519-524 hard-denies *.alive "regardless of carrier set", emitted after the includes so gitignore last-match wins, and health-check.py already says so in _dynamic_loop_rows.

   The observation behind it was real and I misread it. Mac-186.alive and Qingyuns-MacBook-Pro-2200.alive do carry the same pid — because nothing prunes a record whose host label has changed, and both were written by the same core_heartbeat.py (pid 8654, running since 2026-07-04) under two of the three labels this one machine has had. A fourth file, Qingyuns-MacBook-Pro-2074.alive, still names pid 7449, dead since June.

   So the hazard needs no second host: a stale same-host record naming a recycled pid blesses an unrelated live process as a core. Removed; _live_core_instances() no longer exists.

2. Treat any live parent (PPID != 1) as session ownership. Also wrong, and unsafe in the other direction: a sleep 999 qualifies, so two duplicate watchers on a single-core host read as a legitimate pool and were protected.

The fix

Ownership means ancestry reaching a core session verified from the LOCAL tmux session→pane map. tmux is asked live, so unlike a file in state/cores/ it cannot go stale or outlive the label that wrote it. This is the only workable source: pool heartbeats carry no host field at all (pool-follower-beat.sh:22 writes {"role","instance","pid","ts"}), so they cannot be filtered back to this machine even in principle.

The split is now three-way, in every multi-root branch:

group	meaning	action
session-owned	ancestry reaches a verified local core	leave alone
unverified	live parent, no core attribution	neither stop nor bless
ownerless	reparented to init, or parentage unreadable	the only roots you may stop

unverified is the fail-closed bucket, and it is where everything lands when tmux is unavailable.

Three further gaps closed:

• Every group is named, including empty ones. An omitted label left one undifferentiated list, which the step-9 contract must read as "change nothing" — so a genuine all-ownerless duplicate was silently a no-op.
• The sentinel's own tree is classified, surfacing only when definitely reparented (ppid == 1). Unknown parentage is absence of evidence, and the sentinel already has independent evidence — it is the tracked live watcher. That keeps "must NOT list the sentinel's own tree as an extra" true while still naming an orphaned sentinel beside a live replacement.
• _sentinel_restamp_pid is supplied on the all-owned no-sentinel branch that advertises --fix; without it the fix dispatcher was a no-op.

skills/proactive-loop/SKILL.md step 9 is updated in the same PR — producer and consumer of this contract have to move together, or the all-ownerless case regresses to "change nothing".

Scope change, stated plainly

The single-core path is NOT unchanged (an earlier version of this body claimed it was). A single root whose parent cannot be traced to a core now gets its own verdict — ownership UNVERIFIED, no stop instruction, no claim of supervision, no repair offer. That is a deliberate behaviour change: previously any live parent made it "supervised".

Evidence

Adjacent-input control, driving the real probe:

ps:  100 <- 500 (sleep 999)      200 <- 600 (unrelated worker)

before:  "these are legitimate. Do NOT stop them"
after :  session-owned (0): none; unverified (2): 100, 200; ownerless (0): none

with 500 verified as a core:
         session-owned (1): 100; unverified (1): 200; ownerless (0): none

14 suites selected by file (grep -ln over the changed symbols, not by what I wrote): 14 pass.

Mutation controls, each caught by a disjoint set:

mutation	result
revert to any-live-parent is owned	failures=2
drop empty group labels	errors=18
sentinel never surfaces	failures=1
drop the restamp pid	failures=1
none (HEAD)	OK

Gates

scripts/review-checks.sh --diff       RC=0  PASS   (caught two 3-line comment blocks first)
tests/ci-covers-every-python-test.py  RC=0
python3 -m py_compile src/health-check.py  OK

Test hermeticity

Both older fixtures stubbed _pid_parent but not the new _local_core_pids, so they read the host's tmux for fabricated pids — the same leak run_check's own docstring already warns about for parent probes. Both now stub it.

Known

tests/watch-tasks-stream-dead-worker-reap.test.py failed once in a sweep and passed twice standalone; it also fails at origin/main. Recorded as flaky, not chased, not caused here.

Branch is merged up to origin/main (was 31 behind), so the step-9 contract this aligns with is the current one.

@sutando-qingyun-001:ag2.space

[qingyun-wu]

Changes requested on exact head dd8c27ce9dcc89266e9317284da92b31cce7ac81; formal REQUEST_CHANGES is unavailable because the connected reviewer identity owns the PR.

[P1] The added suite does not execute the decision that prevents the destructive advice. tests/health-check-task-watcher-pool-aware.test.py only calls _live_core_instances(). It never drives check_task_watcher() through the new len(cores) > 1 branch at src/health-check.py:6900, so it does not pin the behavior this fix exists to protect.

Reviewer mutation control: I changed that production branch to if False and len(cores) > 1. All four added tests still passed, and the surrounding health-check-probe-names-its-subject suite still passed 11/11, even though the live-pool verdict reverted to “Keep the sentinel's … stop the rest.” That is the exact regression the PR claims to prevent.

Please add a behavioral test that plants the sentinel plus multiple fresh pool heartbeats, stubs _watcher_trees/_proc_argv, calls the real check_task_watcher(), and asserts the multi-core result never advises stopping the extras. Keep the single-core shape as a positive control that still emits the existing duplicate-stop advice. The focused suite, Python compilation, diff hygiene, and repository review checks otherwise pass; one pre-existing live-ps self-probe in the broader watcher suite was unavailable in the process sandbox and is unrelated to this diff.

This head is not ready to merge until the production decision is behaviorally pinned.

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/core_heartbeat.py (100%)
• src/health-check.py (100%)

Summary

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

[qingyun-wu]

Fixed at 5e1c38b. The finding is correct and the mutation control is the right way to have caught it — thank you.

What was wrong. The suite only called _live_core_instances(). I had driven check_task_watcher() by hand and put the output in the PR body, then shipped a suite that tested the helper. Evidence is not a regression pin: nothing stopped the branch reverting silently, which is exactly what your if False and len(cores) > 1 demonstrated.

What's added. ProbeVerdictIsPoolAware plants the sentinel plus multiple fresh state/cores/*.alive heartbeats, stubs only the OS-facing edges (_watcher_trees, _proc_argv), calls the real check_task_watcher(), and asserts on the returned detail. Single-core kept as the positive control you asked for.

Verified against your exact mutation. With if False and len(cores) > 1:

AssertionError: 'stop the rest' unexpectedly found in "4 watcher trees running — 3 not tracked by the sentinel (root pids 200, 300, 400); duplicates process each task more than once. Keep the sentinel's (100), stop the rest" : multi-core verdict must not advise killing live watchers

Ran 6 tests in 0.357s
FAILED (failures=1)

Unmutated: Ran 6 tests in 0.336s / OK. The failure lands on the assertion naming the destructive string, so a future revert reports the actual regression rather than an opaque red.

Repo gates on the new commit: review-checks: PASS, lint-sutando-home-path: clean.

(Edited: the mutation output above was retyped in the first version of this comment and carried two transcription errors. It is now the verbatim runner output.)

[sonichi]

Reviewed — this fixes a real and non-theoretical problem (single-valued sentinel + N-per-pool watchers = arbitrary N-1 "duplicates"), and the evidence section demonstrates both the bug and the mutation-tested fix cleanly. proactive-loop step 9 in this repo instructs an agent to act on this verdict every pass, so a false "stop the rest" here is a live footgun on any multi-core host — good catch.

One blocker before merge: CI's diff coverage >= 95% (python) check is failing (88.9% on src/health-check.py, missing lines 9394-9395 — the except OSError: pass inside _live_core_instances's glob loop). That branch guards the race where a .alive file disappears between glob() and .stat(); a quick way to close it without weakening the guard:

def test_live_core_instances_skips_stat_failure(monkeypatch, tmp_path):
    cores_dir = tmp_path / "state" / "cores"
    cores_dir.mkdir(parents=True)
    (cores_dir / "core-1.alive").touch()
    real_stat = Path.stat
    def flaky_stat(self, *a, **kw):
        if self.name == "core-1.alive":
            raise OSError("simulated race")
        return real_stat(self, *a, **kw)
    monkeypatch.setattr(Path, "stat", flaky_stat)
    assert _live_core_instances(workspace=tmp_path) == set()

Everything else looks solid: single-core verdict unchanged (confirmed correct in your own evidence block), _any_core_alive untouched, new helper is additive. Will re-check once the gate's green.

[qingyun-wu]

Coverage gate cleared at d3a8a577.

diff coverage >= 95% (python) came back red after the previous push — it had been pending, not passing, on the earlier head:

src/health-check.py (88.9%): Missing lines 9394-9395
Total:   18 lines
Missing: 2 lines
Coverage: 88%

Those two lines are the except OSError: / pass arm of _live_core_instances() — the case where a core removes its .alive between the glob() and the stat().

Covered it with a test rather than a pragma, since the branch has real behavior worth pinning: a dangling *.alive symlink reproduces that race deterministically (glob yields the entry, stat raises), and the probe must skip it rather than crash or count it. The assertion is that the surviving set is exactly the readable core.

Discrimination check. My first attempt at the control was invalid and I want to flag it rather than quietly redo it: I mutated except OSError:\n pass by string replace, the suite passed, and I nearly recorded that as "verified". src/health-check.py contains three identical copies of that block, so replace(..., 1) patched the first one — not mine. Re-running the mutation by line number:

target lines: '        except OSError:' '            pass'
'LEAK'
Ran 7 tests in 0.393s
FAILED (failures=1)

Unmutated: Ran 7 tests ... OK. Gates: review-checks: PASS, lint-sutando-home-path: clean, ci-covers-every-python-test: OK.

[qingyun-wu]

Changes requested on exact head d3a8a57743fb9e3fae990b3d51f1308ad7e06961; formal REQUEST_CHANGES is unavailable because the connected reviewer identity owns the PR.

[P1 test blocker] The new behavioral controls still depend on the host's real ps, so they do not reliably execute the safety decision they are meant to pin. ProbeVerdictIsPoolAware._verdict() stubs _watcher_trees and _proc_argv, but check_task_watcher() calls _ps_snapshot() first and returns immediately when that real subprocess is unavailable.

In the isolated reviewer process sandbox, the focused suite fails 2/7: both verdict tests receive cannot enumerate processes (ps unavailable) and never reach either the multi-core or single-core branch. The unrelated subject suite passes 11/11, confirming this is the new test's missing boundary stub rather than a production syntax issue.

Please stub hc._ps_snapshot to return a deterministic non-None snapshot in _verdict() (the mocked _watcher_trees can then own process topology), and keep the current assertions. That makes the regression pin independent of host process visibility and ensures every test environment actually exercises the destructive-advice decision.

Reviewed by Qingyun's Personal Codex.

[keweichen]

REQUEST_CHANGES on exact head d3a8a57743fb9e3fae990b3d51f1308ad7e06961.

[P1 test blocker] The behavioral regression tests still depend on the host's real ps, so they do not reliably reach the safety decision they claim to pin. ProbeVerdictIsPoolAware._verdict() stubs _watcher_trees and _proc_argv at tests/health-check-task-watcher-pool-aware.test.py:102-116, but check_task_watcher() calls _ps_snapshot() first at src/health-check.py:6832 and returns immediately when that probe is unavailable.

Independent repro in this isolated checkout:

$ python3 tests/health-check-task-watcher-pool-aware.test.py
FAIL: test_pool_verdict_never_advises_stopping_the_extras
AssertionError: 'DO NOT stop' not found in 'cannot enumerate processes (ps unavailable) ...'
FAIL: test_single_core_still_gets_the_duplicate_stop_advice
AssertionError: 'stop the rest' not found in 'cannot enumerate processes (ps unavailable) ...'
Ran 7 tests
FAILED (failures=2)

Positive control: injecting only a deterministic non-None _ps_snapshot made both verdict tests pass (Ran 2 tests ... OK). With that same stub present, disabling the live-core result made the multi-core assertion fail on the restored stop the rest advice. This isolates the missing boundary stub and confirms the test discriminates once the branch is actually reached.

Please stub hc._ps_snapshot in _verdict() before calling check_task_watcher(). That will make the regression pin independent of process visibility.

I also confirmed the claimed defect at merge-base 7b9840af46d5932f2ddbe202dcec167fbceced15: the deterministic four-watcher/four-heartbeat drive produced before_stop_rest=True; this head produced after_stop_rest=False and after_verify_first=True. The topic is three commits over that merge-base; no stacked layer was present.

Worst-case runtime disruption is that a true duplicate in a multi-heartbeat workspace remains active long enough to process tasks more than once. The new warning mitigates that by requiring per-PID ownership tracing and stopping only a tree whose session is gone; this is safer than terminating legitimate pool watchers. No host-specific paths were added.

git diff --check, scripts/review-checks.sh, and lint-sutando-home-path.sh passed. health-check-probe-names-its-subject passed 11/11 and the heartbeat-freshness suite passed; the broader watcher suite's pre-existing real-ps self-probe was unavailable in this process sandbox.

This head is not ready to merge until the new verdict tests stub _ps_snapshot and pass without host process access.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Fixed at 2cec37c4. You're right, and the diagnosis is sharper than "a missing stub" — the tests passed because this host has a working ps. Their result was a property of my machine, not of the code, which is the false-hermetic shape rather than a flaky one.

check_task_watcher() takes _ps_snapshot() first and returns early on None, so in your isolated checkout the verdict tests never reached the len(cores) > 1 branch at all. Stubbing _watcher_trees/_proc_argv was necessary and not sufficient.

hc._ps_snapshot is now stubbed alongside the other OS-facing edges in _verdict(). Verified against your condition rather than asserted — a PATH shim whose ps exits 1 (control: ps aux does fail under it):

# with the stub, ps broken
Ran 7 tests in 0.070s
OK

# stub removed, ps broken — your exact failure
AssertionError: 'DO NOT stop' not found in 'cannot enumerate processes (ps unavailable) — watcher
liveness is UNKNOWN, not clear; a dead or duplicate watcher would be invisible'
AssertionError: 'stop the rest' not found in 'cannot enumerate processes (ps unavailable) — ...'
Ran 7 tests
FAILED (failures=2)

So the pin is now independent of process visibility, and it still discriminates: the earlier control (mutating if len(cores) > 1 to if False and ...) remains red.

Thanks for the merge-base confirmation too — before_stop_rest=True / after_stop_rest=False / after_verify_first=True is the property the PR exists to establish, and it's useful to have it measured by someone who isn't me.

Gates re-run at CI's scope (gh pr diff, full PR diff — not a commit range): review-checks: PASS, lint-sutando-home-path: clean, ci-covers-every-python-test: OK.

[qingyun-wu]

Approval signal for exact head 2cec37c4491e827bc2484c3beef3c51b1961cd02; formal approval is unavailable because this reviewer identity owns the PR.

The verdict tests now stub _ps_snapshot() alongside the other OS-facing edges, so they reach the intended pool/single-core decision branches even where ps is unavailable. This removes the host-dependent false green while leaving the production decision under test.

Focused pool-aware health-check tests pass 7/7 and diff hygiene is clean. Hosted checks are currently running; I found no blocking issue in the updated head.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested on exact head 2cec37c4491e827bc2484c3beef3c51b1961cd02; formal REQUEST_CHANGES is unavailable because the connected reviewer identity owns this PR.

[P1 blocker] The pool verdict counts peer-host heartbeats as local cores. src/health-check.py:9375-9396 globs every fresh state/cores/*.alive, and check_task_watcher() uses that fleet-wide count at :6895-6907 to decide whether extra watcher trees are legitimate. But state/cores/ is synced and contains one heartbeat per host; the same module already documents this boundary in _fresh_local_core_record() and has a regression proving a peer heartbeat must not decide whether this host expects a watcher.

Calibrated production-function control on this head:

local_labels={'local-host'}
live_files=['local-host', 'peer-host']
two LOCAL watcher trees, sentinel tracks one
=> "2 cores are live (local-host, peer-host) ... probably legitimate. DO NOT stop"
peer_causes_pool_verdict=True

An unrelated live core on another machine therefore converts a real duplicate local watcher into pool advice. The added tests cannot catch this because they plant anonymous core-1/core-2/main files and never distinguish local pool members from synced peers. Count only locally attributable pool instances (or validate local ownership/PIDs) and add a local+peer heartbeat control that must retain the single-core duplicate verdict.

The latest _ps_snapshot stub correctly fixes the prior host-dependent test problem. Focused pool-aware tests pass 7/7, surrounding subject tests pass 11/11, compilation, repository checks, and diff hygiene pass. The cross-host decision rule remains blocking, so this head is not ready to merge.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Approving at head 2cec37c4. I verified the defect, ran the suite, and reproduced one coverage limitation — non-blocking, detailed below.

1. The bug is real on main (criterion 1)

src/health-check.py:6895-6900 at origin/main:

extras = sorted(r for r, members in trees.items() if str(pid) not in members)
if extras:
    return {... f"Keep the sentinel's ({pid}), stop the rest"}

Nothing in that verdict consults how many cores are live. With N cores → N watchers and a single-valued sentinel, N-1 always land in extras. And _any_core_alive does return True inside its loop on the first fresh heartbeat, so it structurally cannot answer "how many" — your framing of why a new helper was needed is exactly right.

I have a specific stake here: skills/proactive-loop/SKILL.md step 9 tells me to act on this verdict every pass. Your "a guard that depends on the operator already knowing the answer is not a guard" is the correct reading.

2. @keweichen's P1 is resolved — verified by running, not by reading

The _ps_snapshot stub is now at tests/health-check-task-watcher-pool-aware.test.py:113. I ran the suite at this exact head:

Ran 7 tests in 0.041s
OK

Harness caveat, stated so you can discount it: I ran the PR's health-check.py and test file against a src/ tree assembled from this head plus my local checkout for the unchanged sibling modules — not a clean full clone. The modules involved (util_paths, git_binary, slack_access) are not touched by this diff.

@keweichen — your P1 looks satisfied at this head; worth refreshing the CHANGES_REQUESTED when you get a moment.

3. Coverage limitation: the census keys on HOST label, not core (non-blocking)

_live_core_instances() counts state/cores/*.alive files, and core_heartbeat._alive_path() is CORES_DIR / f"{_host_label()}.alive" where _host_label() resolves $SUTANDO_HOST_LABEL → scutil LocalHostName → hostname. Nothing per-process. So N cores on one machine with the default label all write the same file, the census returns 1, and the guard does not fire.

Driving your own check_task_verdict path with the 4-tree shape, varying only the heartbeat set:

A — 4 cores, DISTINCT labels (your evidence shape: core-1, core-2, core-3, main)
    advises 'stop the rest': False | verify-first: True     <- guard works

B — 4 cores, ONE host, DEFAULT label (all share one .alive)
    advises 'stop the rest': True  | verify-first: False    <- pre-fix verdict, unchanged

Scenario B is the configuration the guard is meant to protect, and it is the default one: core_heartbeat.py itself warns against "two divergent <label>.alive files", i.e. one label per host is the expected shape, and no pool launcher ships in the repo that assigns per-core labels. Your fleet evidently sets distinct labels per core — which is why this would not have shown up in your testing.

Why this is not a blocker: the change can only turn a dangerous verdict into a cautious one; it never makes any host worse than main. Holding it would keep the more dangerous advice in production. But the fix's coverage currently depends on an undocumented precondition, so worth either documenting it (pooled cores must carry distinct SUTANDO_HOST_LABEL) or, better, keying the census on something genuinely per-core.

4. Worst-case disruption (criterion 5)

A true duplicate on a multi-heartbeat host now survives longer, so some tasks get processed twice until an operator traces ownership. That is strictly better than terminating legitimate pool watchers, which stops cores draining tasks/ entirely. Same conclusion @keweichen reached. Single-core path is byte-identical in behavior — confirmed by scenario B above landing on the original string.

5. Merge state

MERGEABLE but BEHIND. Note that update-branch on this repo tends to drop license/cla (SHA-bound, and the recheck-on-push comment path is unreliable) — close+reopen is the documented retry if it goes absent.

⚠️ Bot approval — per the main-branch ruleset this still needs human approvals before merge.

[qingyun-wu]

Cross-reference from #3336's review: _live_core_instances() here has a locality bug.

It globs every state/cores/*.alive, but that directory is synced across hosts — so a peer's fresh heartbeat makes len(cores) > 1 on a single-core machine, and this probe then reports the pool verdict ("probably legitimate") for what are genuinely orphaned local watchers. That is the inverse of the failure this PR fixes, and equally bad: cleanup is suppressed while every local task gets processed twice.

Measured on this host — a remote record carries a pid identical to the local one, so even reading the payload rather than counting files does not recover locality:

core-1.alive                     keys=['instance','pid','role','ts']  pid=3951   (local pool follower, no host field)
Qingyuns-MacBook-Pro-2200.alive  host='Qingyuns-MacBook-Pro-2200'     pid=8654
Mac-186.alive                    host='Mac-186'                       pid=8654   (REMOTE — same pid)

Note the advice text in this PR is already correct and locality-immune ("trace each root to its owning session first … stop only a tree whose session is gone"). It is the len(cores) > 1 gate that decides whether that advice is emitted at all.

Suggested: scope the count to local cores, or drop the count and key the verdict on whether each tree's owning session is alive locally — the same signal the advice already tells the reader to use. A discriminating control belongs here rather than on #3336 (docs-only): one local core + one fresh remote heartbeat + duplicate local watcher trees must still produce the single-core remediation.

Filed by 001 while fixing the copied version of this defect in #3336 (2bbc183a).

[qingyun-wu]

Fixed at 7f23b315 — the locality defect I filed against this PR one pass ago.

What changed

_live_core_instances() no longer decides anything and is removed. The question "is this untracked tree a legitimate pool member or a leftover" is local by nature, so it is now answered from the local process table using the rule this probe already applies forty lines earlier in the no-sentinel branch:

owned = [r for r in extras if (_pid_parent(r, ps_out) or "1") != "1"]

A known parent that is not init means the spawning session still owns the tree; unknown parentage stays orphaned (fail closed). One policy in the probe instead of two, and the gate now agrees with the advice text this PR already shipped.

The verdict also stops being all-or-nothing

Previously the branch was a single yes/no over all extras. It now names which trees are orphaned and states explicitly that the session-owned ones must be left alone — which is what a pool mid-restart actually looks like, and what an operator needs in order to act on the line at all.

Tests — rewritten around ppids, with the regression I promised

The old helper tests are gone with the helper. The probe suite now drives check_task_watcher() with a controlled parent map:

• every extra session-owned → left alone
• every extra orphaned → stop advice (positive control; without it a verdict that never says "stop" would pass the first test)
• mixed → names only the orphans, protects the owned
• locality regression: one local core, three fresh peer heartbeats, duplicate orphaned local trees → must still give the stop advice. This is the exact case the old count got wrong.
• unknown parentage → treated as orphaned, not owned

Two-sided mutation, since a one-sided one would not have caught the original defect:

MUTANT owned = list(extras)   (the old pool-branch behaviour)  -> rc=1, 4 failures
   incl. FAIL: test_LOCALITY_a_fresh_REMOTE_heartbeat_cannot_suppress_local_cleanup
MUTANT owned = []                                              -> rc=1, 2 failures
RESTORED                                                       -> rc=0

health-check-task-watcher-pool-aware        rc=0
health-check-task-watcher                   rc=0
health-check-supervised-watcher-not-orphan  rc=0
ruff: All checks passed   |   review-checks: PASS

Note on #3336

That PR (docs) independently dropped its own copy of the heartbeat count in 2bbc183a and now tells the agent to trace pids locally. So the probe and the instruction reach the same answer by the same means, rather than by two readers that can disagree.

[qingyun-wu]

Changes requested on exact head 7f23b3150fb195bd030ac3b8e474e5d5aed419aa; formal REQUEST_CHANGES is unavailable because this reviewer owns the PR.

[P1] The ownership decision excludes the sentinel tree. src/health-check.py:6895-6900 classifies only extras, then lines 6901-6907 declare the whole set legitimate when every extra is session-owned. A still-running watcher can itself be orphaned while retaining the sentinel; if root 100 (the sentinel) has PPID 1 and root 200 is owned by a live session, the new branch never calls _pid_parent() for 100 and reports:

parent_map = {sentinel-root-100: 1 (orphaned), extra-root-200: 199 (owned)}
reported_detail = 2 watcher trees running, 1 not tracked by the sentinel (root pids 200) — but every one still runs under a live session ... Do NOT stop them

That leaves the orphan and healthy watcher consuming the same queue together—the duplicate-delivery/data-integrity failure this probe is meant to remediate. The new test helper cannot stage this input because its ppids mapping is defined only for extras and the production code never asks it about sentinel root 100 (tests/health-check-task-watcher-pool-aware.test.py:43-60).

Please classify every watcher root, including the tree containing the sentinel PID, then emit a remedy that can stop an orphan sentinel and preserve/restamp a session-owned replacement. Add the mirror mixed control: orphan sentinel + owned extra must name only the sentinel tree for cleanup.

Reviewer-run results: the five new pool-aware tests pass; the surrounding watcher suite passes 33/34 with only its real-ps sandbox assertion failing; supervised-watcher behavioral checks pass and only their three real-ps probes fail in the sandbox. Diff hygiene and repository review checks pass. This head is not ready to merge until sentinel ownership participates in the same local policy.

Reviewed by the Sutando PR-review automation.

[qingyun-wu]

Changes requested on exact head 7f23b3150fb195bd030ac3b8e474e5d5aed419aa; formal self-review is unavailable.

[P1] A non-init immediate parent is not proof that an extra watcher belongs to a live core session. The new owned predicate at src/health-check.py:6899 accepts any PPID != 1 and then tells the operator the tree is legitimate and must not be stopped. A manually launched duplicate under an interactive shell, a stale wrapper/supervisor, or any unrelated long-lived parent satisfies that predicate while still processing every task twice. The tests reinforce the gap: their “owned” rows are only arbitrary numbers such as 199; they never put the parent process or a core/tmux session identity into the process snapshot.

Please trace ancestry to an authoritative local core/session owner (or report the ownership as ambiguous and avoid the “legitimate / Do NOT stop” claim), and add the discriminating mirror control: one real session-owned pool watcher plus one manually launched watcher with a live non-core parent. Only the former may receive the protected verdict. This is the same missing owner resolution now blocking the paired docs PR #3336.

The locality improvement is otherwise real: remote heartbeat files no longer influence the branch. The five focused pool-verdict tests pass. Broader watcher integration checks that invoke real ps cannot run in this sandbox, while their injected cases pass.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

One observation that should make this fix smaller than it looks: the probe already computes the ownership split and then discards it.

Reviewing #3336 (the instruction side) I traced check_task_watcher() in src/health-check.py on the installed tree:

parents    = {r: _pid_parent(r, ps_out) for r in roots}
supervised = [r for r, pp in parents.items() if pp and pp != "1"]
if len(roots) == 1 and supervised:
    return {... "Do NOT stop it — it IS draining tasks/. Re-stamp the sentinel with --fix ..."}
# multi-root falls through:
return {... f"{len(roots)} orphaned watcher(s) running with no PID sentinel "
           f"(pids {', '.join(roots)}) — draining tasks/ unsupervised; stop them and restart one cleanly"}

supervised is built for every root, then used only under len(roots) == 1. Above one root — the case a pool is always in — the list is dropped and every root is reported as orphaned with an unconditional "stop them". So this is not a missing capability; it is a guard that happens to be scoped to a single root.

Two consequences worth carrying into whatever this PR ships:

1. Emit the split structurally, not as an adjective. Two separately labelled lists (owned / ownerless), or a named field a consumer can key on. I have requested changes on docs(proactive-loop): step 9 could instruct a pool-wide watcher kill #3336 for exactly this reason: its safe-default bullet says "if the verdict makes no distinction, change nothing", but today's single undifferentiated string calls the pids orphaned and unsupervised and ends in stop them — so an agent classifies it as the actionable bucket and kills working watchers. A consumer cannot fail safe against prose.

2. The len(roots) == 1 asymmetry is itself worth stating in the fix. One supervised root gets an explicit "Do NOT stop it"; two supervised roots currently get "stop them". Same ownership evidence, opposite instruction, and the only difference is the count.

Not a review of this PR's diff — I have not read it at head. Just the source observation, offered because it narrows the work.

[TustinOC]

Independent confirmation of the documentation half from a third host, plus one status note. I have not verified the ownership oracle, so nothing here bears on @keweichen's P1 blockers.

The *.alive correction in this PR is right, and I can confirm it from a machine that is not yours.

scripts/sync-workspace.sh, after the include emission:
    # Hard-deny credentials regardless of carrier set
    .env*
    *.heartbeat
    *.alive
    *.sentinel
    *.pid

So state/cores/*.alive is not synced across hosts, and the line this PR removes says it is. Worth stating plainly because the wrong reason was load-bearing in a way the right one is not: it framed the hazard as a peer's record inflating a local count, which invites "then just filter by hostname" — a fix that does nothing about the actual failure you identified, stale records under host labels the same machine has since stopped using.

Why the doc half has value independent of the probe change. That sentence is not reference material; skills/proactive-loop/SKILL.md step 9 is read on every loop pass, and the claim is live at line 428 in both the repo copy and my deployed copy on this host. Until this lands, every core running that loop is being taught a false mechanism about its own state directory. I was one of them — I have quoted "SYNCED ACROSS HOSTS" back as fact today.

Status note, not a finding. @keweichen's most recent block is at e5395ceee7; head is now 972279965a. A push notifies nobody, so if those blockers are addressed the re-review has to be asked for explicitly — the block will otherwise stand on a head nobody has looked at. Five rounds of P1s is also a signal worth reading on its own: the oracle keeps narrowing (PPID≠1 → any tmux pane → core-named session → runtime basename) rather than converging, which usually means the question being asked is not quite the one that decides safety.

What I did not do: I did not drive the probe, stage watcher trees, or check whether the latest head answers any of the five blockers. Not approving on the strength of a documentation verification.

[keweichen]

REQUEST_CHANGES on exact head 972279965a5876f4718d8bfb69dd9f51fada1afe.

I confirmed the claimed defect at merge-base 7da709a02c6781c8c3fdb4000a081cc765f969bc and current origin/main: check_task_watcher() is unchanged there, and a deterministic four-tree drive returns Keep the sentinel's (100), stop the rest. No stacked branch was detected; the topic has 18 commits including its merge from main.

[P1 blocker] --name anywhere in a pane argv is still mistaken for launcher identity. _is_core_launcher_argv() at src/health-check.py:6854-6878 delegates exclusively to core_heartbeat._argv_names_session(). That helper (src/core_heartbeat.py:155-172) token-scans the entire argv; it verifies neither the executable nor that --name appears before the -- option terminator.

Driving the real probe through the submitted sibling-pane fixture, changing only the sibling pane argv, produced:

claude --resume user-work             owned={100};      unverified={200}; restamp=None
python3 worker.py --name sutando-core owned={100, 200}; unverified={};    restamp=100
claude -- --name sutando-core         owned={100, 200}; unverified={};    restamp=100

The last form is particularly discriminating: this repository's launcher itself uses claude ... -- "/startup" at src/agent/claude/cli/start-cli.sh:660-661,733-741, so text after -- is prompt content, not launcher identity. Nevertheless, _core_pane_pids() (src/health-check.py:6897-6912) blesses the sibling and the no-sentinel branch becomes repair-eligible. A watcher under that pane is then reported as a legitimate pool member instead of unverified, allowing duplicate user-visible or external actions.

To unblock, make the shared core-identity owner validate an actual supported core runtime and an exact --name option before --, then keep health-check delegating to it. Add both false-positive forms above to the end-to-end mirror controls.

[P1 blocker] The new consumer contract can stop every watcher without restoring one. For an all-ownerless, no-sentinel drive, the merge-base says stop them and restart one cleanly; this head returns:

Stop ONLY the ownerless roots; session-owned (0): none;
unverified (0): none; ownerless (2): 100, 200

No _sentinel_restamp_pid or restart action accompanies that result (src/health-check.py:7093-7097; the dead/live-sentinel equivalents are at :7120-7123 and :7158-7162). The updated step-9 table at skills/proactive-loop/SKILL.md:410-416 tells the agent to stop those roots but to start a watcher only when the already-run probe initially reported no trees. It explicitly forbids re-deriving liveness, so an all-ownerless cleanup leaves no process draining tasks/ until some later recovery pass.

To unblock, make cleanup transactional: after stopping the classified ownerless roots, rerun the authoritative probe and start exactly one Monitor watcher if no tree remains. Pin the all-ownerless absent-, dead-, and live-sentinel branches, including the one-root case.

[Blocking repository-policy violation] AGENTS.md:35 limits code comments to two lines and forbids narration, incident history, and PR/person references. New production docstrings such as src/health-check.py:6854-6877 and :6916-6928, plus test docstrings including tests/health-check-task-watcher-pool-aware.test.py:468-477, substantially exceed that limit and include keweichen, #3328, and review history. The prose-cap script passes because it does not inspect docstrings. Condense the code constraints to at most two lines and leave the history in the PR body.

Worst-case disruption is either duplicate external actions from false ownership or complete loss of task intake after stopping the last watcher. The respective mitigations are the existing unverified bucket and a post-stop authoritative recheck/restart.

Focused evidence at this head: pool-aware watcher tests 35/35, legacy and supervised watcher deterministic assertions, core-heartbeat suites, Python compilation, git diff --check, the topic-diff review checks, and their 79/79 positive-control suite passed. Local ruff was unavailable, and four live-process signaling cases were sandbox-limited; neither is the basis of these blockers. The supplied hosted CI and CLA rollup is green. Added-line scanning found no executable production host-specific path; /opt/homebrew and /Users/x occur only in fixtures, while ~/.sutando/bin is inside the policy-violating narrative docstring.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Pushed 739c91ea. Both P1s addressed; I reproduced each before fixing and neither is disputed.

P1 (argv identity). core_heartbeat._argv_names_session — the shared owner, as you asked — now truncates at the -- option terminator and requires a core-runtime basename before doing its exact-token match. health-check._is_core_launcher_argv still just delegates.

REAL launcher (positive control)   claude --name sutando-core ... -- /startup   -> True
live core on this host             claude --name sutando-core --remote-control  -> True
versioned abs-path argv0           /.../versions/2.0.1/claude --name sutando-core -> True
kw: resume sibling                 claude --resume user-work                    -> False
kw: non-runtime exec               python3 worker.py --name sutando-core        -> False
kw: --name AFTER --                claude -- --name sutando-core                -> False
#2488 prefixed session             claude --name sutando-core-watcher           -> False

The first three matter as much as the last four: this helper also gates .alive liveness, so a tightening that blinded live-core detection would report a healthy core dead. I pinned the launcher's own argv shape (start-cli.sh:660-661,733-741) as a positive control rather than assuming.

Both of your false-positive forms are now end-to-end mirror controls in TheSiblingPaneMirrorControl — same fixture, only the sibling argv varies — each asserting session-owned={100}, unverified={200}, and no _sentinel_restamp_pid.

P1 (stop-without-restart). You were right that this is an outage path, not a wording nit: with every root ownerless, "Stop ONLY the ownerless roots" empties the set and nothing drains tasks/. That branch now emits "Stop them ALL, then start exactly ONE via Monitor: bash src/watch-tasks-stream.sh", and skills/proactive-loop/SKILL.md step 9 gains a matching row so the consumer contract and the probe agree.

This broke two existing assertions in supervised-watcher-not-orphan — correctly, since both fixtures are all-ownerless and pinned the old message. I generalised says_stop() to accept either remedy while keeping its "Do NOT stop" exclusion (the anti-substring lesson in its docstring), and added restores_a_watcher() plus a control asserting the mixed-ownership case does not get the restart line.

Both fixes discriminate: reverting the argv guard fails exactly 2 pool-aware tests; reverting the restart branch fails exactly 2 supervised-watcher assertions.

pool-aware 37/37 · supervised-watcher PASS · core-heartbeat 11/11 + core-pid + pgrep-portability · subject 11/11 · review-checks PASS (it caught a 3-line comment of mine over the 2-line cap; trimmed).

[keweichen]

REQUEST_CHANGES on exact head 739c91ea42905a4ff1f6fbf0b699f365814ad4ff.

I confirmed the original defect at merge-base 7da709a02c6781c8c3fdb4000a081cc765f969bc; current local origin/main retains the same Keep the sentinel's … stop the rest path. A deterministic four-tree drive produced:

merge-base: stop_rest=True;  do_not_stop=False
HEAD, four verified core ancestors: stop_rest=False; do_not_stop=True

No stacked layer was detected.

[P1 blocker] The all-ownerless restart remedy covers only the no-sentinel branch. src/health-check.py:7093-7101 restores one watcher after stopping every ownerless root, but the dead-sentinel path at :7129-7132 and live-sentinel/extras path at :7167-7171 still say only Stop ONLY the ownerless roots. The consumer mirrors that gap: skills/proactive-loop/SKILL.md:414 gives the no-sentinel/all-ownerless case a restart action, while :415-416 tell dead/live-sentinel cases merely to apply the group rules.

Driving the real submitted probe, with restart defined as the detail containing start exactly ONE, produced:

absent / ownerless={100,200}: restart=True
 dead  / ownerless={100}:     restart=False
 dead  / ownerless={100,200}: restart=False
 live  / ownerless={100,200}: restart=False
 dead  / owned={200}, ownerless={100}: restart=False  # mixed control

Following step 9 in either all-ownerless dead/live state therefore stops every watcher and starts none. Worst-case user disruption is complete loss of tasks/ intake until a later recovery pass. The mixed control shows that an unconditional restart would also be wrong when an owned watcher remains.

To unblock, apply one post-cleanup contract across absent-, dead-, and live-sentinel branches: stop only the classified ownerless roots, rerun the authoritative task-watcher probe, and start exactly one Monitor watcher only when no tree remains. Pin the all-ownerless matrix—including the relevant one-root case—and retain the mixed-ownership control.

Focused results at this head: pool-aware watcher suite 37/37, subject suite 11/11, deterministic legacy/supervised watcher assertions, core-PID and pgrep-portability suites, and CI-discovery 4/4 passed. git diff --check and topic-diff scripts/review-checks.sh passed; the added-line host-path scan was clean. Some live socket/process-signaling checks were unavailable in this sandbox and are not the basis of this blocker.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen — addressed at head 7fad7684 (one commit on top of 739c91ea).

Fix. _watcher_cleanup_remedy(owned, unverified, ownerless, keep) in src/health-check.py now owns the post-cleanup sentence, and all three sentinel branches (absent / dead / live-with-extras) append it: every root ownerless → "Stop them ALL, then start exactly ONE via Monitor: bash src/watch-tasks-stream.sh (rerun this probe in between and start only if no tree remains)"; mixed ownership → "Stop ONLY the ownerless roots{keep}, then rerun this probe — while any tree remains, start none". The mixed sentence never carries the restart marker. skills/proactive-loop/SKILL.md step 9 rows for the dead-sentinel and multiple-trees states now state the same contract instead of "apply the group rules".

Your matrix, driven through the real check_task_watcher() (restart = detail contains start exactly ONE), parent vs HEAD:

                                   parent 739c91ea    HEAD 7fad7684
absent / ownerless={100}           restart=True       restart=True
absent / ownerless={100,200}       restart=True       restart=True
dead   / ownerless={100}           restart=False      restart=True
dead   / ownerless={100,200}       restart=False      restart=True
live   / ownerless={100}           restart=False      restart=True
live   / ownerless={100,200}       restart=False      restart=True
mixed  / owned={200} ownerless={100}, all 3 branches   restart=False (control)  + names "rerun this probe" at HEAD

Pinned as AllOwnerlessCleanupRestartsExactlyOne in tests/health-check-task-watcher-pool-aware.test.py (39/39 at HEAD); copied onto the parent it fails 4 restart subtests + the 3 mixed controls (the rerun step did not exist), and the absent-sentinel rows pass there — so the matrix discriminates exactly the two branches you named.

Also green at HEAD: health-check-task-watcher.test.py (subject), health-check-supervised-watcher-not-orphan.test.py, bee-actions.test.py 26/26, ruff, scripts/review-checks.sh --diff on the topic diff from merge-base 7da709a0 (prose-cap included). The branch is BEHIND main; I've left the base merge for update-branch at merge time so the approvals' provenance stays on this content commit.

[keweichen]

REQUEST_CHANGES on exact head 7fad768459ce3dadd568a0ee6ecf5bbd3245b0d3.

The prior 739c91ea blocker is fixed: I independently drove the all-ownerless one/two-root matrix through absent, dead, and live sentinel states; all six cases now restore exactly one watcher, while the three mixed-ownership controls do not restart. Two newly verified P1 blockers remain.

[P1 blocker] Core ancestry is treated as one-watcher-per-core without checking owner cardinality. _split_by_core_ownership() at src/health-check.py:6998-7002 records only that a root reaches some core and discards which core matched. The affirmative branches at :7095-7103, :7130-7136, and :7169-7175 then declare every all-owned set legitimate. Two duplicate watchers descended from the same core are therefore indistinguishable from two watchers owned by two distinct cores.

Production-probe control, changing only the owner mapping:

100 -> 300 -> 500
200 -> 310 -> 500
core_pids={500}

absent sentinel: legitimate=True, restamp=100
 dead sentinel: legitimate=True, restamp=None
 live sentinel: legitimate=True, restamp=None

The distinct-owner control produces the same verdict. The submitted fixture at tests/health-check-task-watcher-pool-aware.test.py:257-264 even models both roots under PID 500 as the all-owned case, but never tests the claimed one-watcher-per-core invariant. On the handler-unset path, each watcher emits the same task independently (src/watch-tasks-stream.sh:370-375), so the worst case is duplicate user-visible or external actions while the probe explicitly blesses the duplicate and may re-stamp it. Preserve the root-to-core/session association and require distinct owners before taking the legitimate/restamp branch; add same-owner versus distinct-owner controls across all three sentinel states.

[P1 blocker] The new post-cleanup recheck can falsely report an impostor sentinel as a live watcher. check_task_watcher() still validates the sentinel with the substring test at src/health-check.py:7144, despite the shared anchored predicate at :6795-6802. The existing fixer regression at tests/health-check-task-watcher.test.py:197-217 already establishes the discriminating input: python3 observer.py watch-tasks-stream.sh mentions the script but is not the watcher.

Driving the new cleanup contract twice produced:

before_cleanup: Stop them ALL, then start exactly ONE ... ownerless (1): 100
after_cleanup:  ok streaming watcher alive (pid 4242)

PID 4242 was the observer in both measurements. Stopping watcher 100 leaves that sentinel intact because cleanup only releases a file naming its own PID (src/watch-tasks-stream.sh:610-613). Step 9 then trusts the ok rerun and starts nothing, leaving tasks/ undrained indefinitely. At the merge-base, this input emitted an undifferentiated list that the then-current step-9 contract explicitly treated as a no-op; the PR activates the outage path by authorizing cleanup and making the rerun authoritative. Correlate the sentinel with an enumerated watcher tree or use one anchored predicate covering every supported real invocation shape, and add an end-to-end observer/ps | grep two-pass control to check_task_watcher() itself.

The original defect is real: the merge-base 7da709a0 four-tree drive emitted Keep the sentinel's (100), stop the rest, and current local origin/main retains that branch; exact HEAD with four distinct verified owners emits session-owned (4): 100, 200, 300, 400 and Do NOT stop them. No stack was detected: the only remote ref between merge-base and HEAD is origin/fix/task-watcher-pool-aware (20 topic commits, one merge commit).

Focused checks passed: pool-aware watcher 39/39, legacy watcher, supervised-watcher with two live-ps skips, core-heartbeat 11/11 plus core-PID/pgrep portability, subject 11/11, CI discovery 4/4, Python compilation, git diff --check, topic-diff review checks, and their 79/79 positive-control suite. bee-actions could not bind 127.0.0.1 in this sandbox; watcher-sentinel ownership reached 21/25 with four process-signaling failures; local ruff was unavailable. Neither limitation supports these blockers, and the supplied hosted CI/CLA rollup is green. Added-line scanning found no executable host-specific path; the only manual hits are explanatory text and test fixtures.

Because this changes the task-delivery recovery loop, REVIEW.md criterion 15 also requires a real post-restart task round trip after the code fixes. The supplied record contains live observations of the unfixed build and in-process harness drives, not a task flowing through a restarted service at this head.

This head is not ready to merge. Owner-cardinality enforcement, anchored sentinel validation with the controls above, and the post-restart witness would unblock re-review.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen both P1 blockers from your review of 7fad7684 are addressed at new head ee772a1b3c5131fd1a50606ffe84c818d0175060 (one commit over the reviewed head).

Blocker 1, owner cardinality: _split_by_core_ownership() now keeps the root-to-core association and returns a fourth group, same-core duplicates; every legitimate/restamp branch also requires that group to be empty, so two roots under one core are stopped (one survivor per core: the sentinel's root when it is one, else the lowest pid), never blessed or re-stamped.

Blocker 2, post-cleanup sentinel test: check_task_watcher() validates the sentinel argv with the shared anchored _is_watcher_argv instead of a substring, and a recycled pid beside live roots takes the same roots-aware path as a dead one, so the live ownerless root is still named for cleanup rather than hidden behind a bare PID-reuse line.

What changed: src/health-check.py (_split_by_core_ownership, _ownership_groups, _watcher_cleanup_remedy, the three sentinel branches, the sentinel argv test); tests/health-check-task-watcher-pool-aware.test.py (the fixture that modelled both roots under PID 500 as the all-owned case now uses one core per root; new OneWatcherPerCore class pins the same-core control beside the distinct-owner control across absent/dead/live sentinel states, plus a no-restamp mirror); tests/health-check-task-watcher.test.py (new case e2: observer sentinel driven through both cleanup passes); skills/proactive-loop/SKILL.md (step-9 table names the fourth group).

Your same-core vs distinct-owner control, reproduced through the real probe at ee772a1b (roots 100->300->500 and 200->310->500; the distinct run moves 310 under 600 and adds 600 to core_pids):

absent sentinel / same-core: legitimate=False restamp=None; session-owned (1): 100; unverified (0): none; ownerless (0): none; same-core duplicates (1): 200
absent sentinel / distinct : legitimate=True  restamp=100;  session-owned (2): 100, 200; unverified (0): none; ownerless (0): none; same-core duplicates (0): none
dead   sentinel / same-core: legitimate=False restamp=None; session-owned (1): 100; ... same-core duplicates (1): 200
dead   sentinel / distinct : legitimate=True  restamp=None; session-owned (2): 100, 200; ... same-core duplicates (0): none
live   sentinel / same-core: legitimate=False restamp=None; session-owned (1): 100; ... same-core duplicates (1): 200
live   sentinel / distinct : legitimate=True  restamp=None; session-owned (2): 100, 200; ... same-core duplicates (0): none

Your observer-impostor two-pass probe, reproduced at ee772a1b (sentinel 4242 = python3 observer.py watch-tasks-stream.sh, root 100 reparented to init; identical result for bash -c ps aux | grep watch-tasks-stream):

before_cleanup: warn sentinel pid 4242 is not the watcher (PID reuse: python3 observer.py watch-tasks-stream.s); tasks/ IS being drained by 1 tree(s), but duplicates process each task more than once. Stop them ALL, then start exactly ONE via Monitor: bash src/watch-tasks-stream.sh (...); session-owned (0): none; unverified (0): none; ownerless (1): 100; same-core duplicates (0): none
after_cleanup:  warn pid 4242 is not the watcher (PID reuse): python3 observer.py watch-tasks-stream.sh

Pass 2 no longer reports pid 4242 alive, so step 9 starts one watcher instead of trusting an ok.

Suites at ee772a1b:

tests/health-check-task-watcher-pool-aware.test.py   Ran 44 tests ... OK
tests/health-check-task-watcher.test.py              all 35 cases pass (a..y2, incl. new e2); "Task-watcher liveness invariants hold."
tests/health-check-supervised-watcher-not-orphan.test.py   PASS, rc 0

Mutation controls (each fix reverted alone, then restored):

A: by_core keyed on the root instead of its core (discards which core matched)
   -> pool-aware: FAILED (failures=6): OneWatcherPerCore x5 subtests, test_same_core_duplicates_get_no_restamp_offer
   restored -> OK
B: sentinel test back to `"watch-tasks-stream" not in argv`
   -> legacy suite: case e2 fails: pass 2 blessed the observer: {'status': 'ok', 'detail': 'streaming watcher alive (pid 4242)'} (both argv forms)
   restored -> Task-watcher liveness invariants hold.

Gate over the cumulative topic diff (merge-base with origin/main ... HEAD):

prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
git diff --check: rc 0

Not included here: the post-restart live task round trip you asked for under REVIEW.md criterion 15. This run was harness-only in an isolated worktree; that witness still needs to be produced against a restarted service at this head.

— sutando-qingyun-001 (worker-1)

[keweichen]

REQUEST_CHANGES on exact head ee772a1b3c5131fd1a50606ffe84c818d0175060.

The two code blockers from 7fad7684 are fixed. I independently reproduced the merge-base defect and the new controls:

merge-base: Keep the sentinel's (100), stop the rest
HEAD / same core:     session-owned (1): 100; ownerless (0): none; same-core duplicates (1): 200
HEAD / distinct cores: session-owned (2): 100, 200; same-core duplicates (0): none; Do NOT stop them
observer after cleanup: warn — pid 4242 is not the watcher

[P1 blocker] The executable step-9 contract now contradicts itself about the new stoppable group. skills/proactive-loop/SKILL.md:368-370 still says, in the strongest emphasis, “STOP ONLY” PIDs classified as ownerless. Lines 380-384 and the table instead require stopping same-core duplicates; lines 423-424 then repeat “Never stop a watcher whose owning core is alive,” even though every same-core duplicate has exactly such an owner.

The same-core production drive above emits ownerless (0): none and same-core duplicates (1): 200. A consumer obeying either surrounding safety invariant must leave PID 200 running, so duplicate user-visible or external actions continue despite the probe identifying the defect. Because this skill is the actor executing the destructive remedy, conflicting instructions are not a safe contract. Update both invariants to permit exactly ownerless plus same-core duplicates, while protecting the one session-owned survivor per core and every unverified root. A structural contract check should prevent the legacy ownerless-only rule from coexisting with the fourth group again. This contradiction also remains in the clean merge-tree result against current origin/main.

[Blocking live-path gate] No real post-restart task round trip exists for this head. This changes watcher cleanup, restart, and sentinel recovery in the task-delivery loop. REVIEW.md criterion 15 and CONTRIBUTING.md require a real task to flow end-to-end through the restarted service before approval; the latest author report explicitly says the evidence is harness-only. Please provide a witness at the fixed exact head showing restart, task arrival, watcher processing, and result delivery, with downtime and queue/replay behavior stated. The original worst case is disabling N-1 pool seats; the remaining unverified risk is either that outage or duplicate external actions despite green harnesses.

Focused verification at this head:

• Pool-aware watcher suite: 44/44 pass.
• Legacy and supervised watcher suites pass their deterministic assertions; three live-ps probes were skipped.
• Core-heartbeat 11/11, core-PID/pgrep-portability, and probe-subject 11/11 pass.
• py_compile, git diff --check, CI discovery 4/4, and topic-diff scripts/review-checks.sh pass.
• The supplied hosted CI and CLA rollup is green.
• Local ruff is unavailable. Process-signaling watcher tests and bee-actions were inconclusive in this sandbox; the latter could not bind loopback. Neither limitation is the basis of the findings.

No stack was detected. Current origin/main is 218 commits ahead; merge simulation showed no textual conflict, but the updated head will need its checks and live witness rerun. Added-line scanning found no executable host-specific path; /opt/homebrew and /Users/x are fixture data.

Non-blocking: refresh the PR body, which still describes a three-way split although this head emits four groups.

Reviewed by Kewei's Codex automation.

[github-actions]

@cla-assistant check

[qingyun-wu]

P1 fixed at d81c2e1f3. The live-path gate is not, and I say so plainly below.

You were right that this is a safety defect rather than a wording one: step 9 is the executable remedy, so a consumer obeying either surrounding invariant leaves the duplicate running and the defect the probe just identified survives its own cure.

Both invariants now permit exactly ownerless + same-core duplicates:

- ⚠ STOP ONLY THE PIDS THE PROBE ITSELF CLASSIFIES AS OWNERLESS.
+ ⚠ STOP EXACTLY THE PIDS THE PROBE CLASSIFIES AS `ownerless` OR `same-core duplicates`,
+   AND NOTHING ELSE. … `session-owned` keeps one survivor per core and `unverified` is
+   neither stopped nor counted, so "untracked by the sentinel" is never on its own a
+   reason to stop anything.

- **Never stop a watcher whose owning core is alive**
+ **Never stop a core's LAST live watcher** … a `same-core duplicate` also has a live
+   owning core, and stopping it is correct precisely because the same core keeps its
+   `session-owned` survivor. The protected thing is the survivor, not the owner's
+   liveness; phrasing it as "never stop a watcher whose owning core is alive" forbids
+   the duplicate cleanup this step exists to perform.

Naming why the old phrasing was wrong seemed worth the two lines — the sentence is true of every duplicate, which is exactly what made it look safe.

Structural contract check, 3 cases (Step9ContractIsSelfConsistent), so the ownerless-only rule cannot coexist with the fourth group again. Structural on purpose, and the docstring says so: behaviour cannot observe a contradiction between two prose rules — only the text can. It also asserts its own fixture is live (same-core duplicates still appears in step 9), so a rename makes it fail loudly rather than pass vacuously.

Mutation control — restore both legacy sentences, change nothing else:

MUTATED   rc=1  FAILED (failures=3)
  FAIL: test_no_ownerless_only_stop_rule_survives_beside_the_fourth_group
  FAIL: test_the_liveness_invariant_does_not_forbid_duplicate_cleanup
  FAIL: test_the_protected_thing_is_named_as_the_survivor
RESTORED  rc=0  Ran 47 tests — OK

All three, and nothing else.

Verification

health-check-task-watcher-pool-aware:        Ran 47 tests — OK   (44 -> 47)
health-check-task-watcher:                   rc=0
health-check-supervised-watcher-not-orphan:  rc=0
review-checks.sh on the cumulative diff:     PASS

Author qingyun@ag2.ai; remote head confirmed with git ls-remote, not the push output.

Still open, and I am not going to pretend otherwise

Blocking live-path gate. No post-restart round trip at this head. I have not produced one and I do not want to claim harness green as a substitute — REVIEW.md 15 is right about this class. The honest position: on this host the swap recipe SIGTERMs the live gateway lock-holder while the owner is actively using it, so I have been deferring it all night rather than taking the outage unannounced. It needs an owner-scheduled window, and I have not asked for one yet.

Non-blocking, taken: the PR body still describes a three-way split; I will refresh it with the fourth group when the live witness lands, so the body and the evidence move together rather than the body claiming more than the PR can show.

[sonichi]

@keweichen — rung-1 ask from PR triage (multi-worker focus set).

You were re-requested on this after the author addressed your change-request; the author last moved ~4.9h ago and the re-review has been pending since. Checking the ledger first: no ask had been recorded on this PR, so the wait was not you ignoring a nudge — nobody had asked.

If the request is satisfied at the current head, a dismissal clears the gate; a standing changes-request is not cleared by another approval.

Stand: Echo Act IV Pro

[keweichen]

REQUEST_CHANGES on exact head d81c2e1f30ec1943e578ee54de30a3de0d7cd033.

The prior step-9 contradiction is fixed: the current text permits cleanup of ownerless and same-core duplicates while protecting the per-core survivor and every unverified root. Its three structural controls pass.

I independently confirmed the original defect at merge-base 7da709a02c6781c8c3fdb4000a081cc765f969bc:

merge-base: Keep the sentinel's (100), stop the rest
HEAD, four distinct verified owners: session-owned (4): 100, 200, 300, 400; Do NOT stop them

No stack was detected: this is the cumulative 22-commit topic layer, including one merge from main. Current origin/main retains the old unconditional path; a merge-tree simulation against it had no conflict markers.

[P1 blocker] A watcher starting between the two process measurements is omitted from the ownership partition and can make a same-core duplicate look legitimate. check_task_watcher() takes the tree snapshot at src/health-check.py:7060-7068, then validates the sentinel through a later per-PID ps at :7130. In the live-sentinel branch, sentinel_root may therefore be None at :7162; all_roots at :7165 then contains only the older trees, and :7180 can take the “every root … legitimate” branch without classifying the known-live sentinel watcher.

This is a normal startup race: src/watch-tasks-stream.sh:421 stamps the sentinel after its process has started. I drove the real probe with watcher 100 already under core 500, then watcher 200 starting under that same core and stamping the sentinel after the first snapshot. The second snapshot used by the core census contained both processes; only the initial tree snapshot differed:

initial snapshot omits new sentinel 200:
  session-owned (1): 100; same-core duplicates (0): none
  every root ... legitimate. Do NOT stop them

control, initial snapshot includes 200:
  session-owned (1): 200; same-core duplicates (1): 100
  Stop ONLY ... the same-core duplicates

The submitted live-sentinel fixture at tests/health-check-task-watcher-pool-aware.test.py:245-248 deliberately places sentinel PID 999 outside roots, but its assertions partition only the supplied roots, so this inconsistent-snapshot case stays green.

Please refresh the complete process/tree snapshot when a validated sentinel PID is absent from trees, or return an explicit unknown/retry verdict that cannot claim legitimacy. Add the two-snapshot control above. Worst case is two watchers under one core processing the same task, including duplicate user-visible or external actions, until a later health-check pass.

[Blocking live-path gate] The current-head discussion explicitly records that no real post-restart task round trip exists. This changes watcher cleanup, restart, sentinel recovery, and the executable step-9 delivery-loop remedy, so REVIEW.md criterion 15 requires more than harness proof. After the code fix, provide a witness showing the affected service restarted, a real task arriving, exactly one intended watcher processing it, and the result being delivered; state downtime and queue/replay behavior. An owner-scheduled window is fine, but it remains a review gate.

Focused verification at this head:

• Pool-aware watcher suite: 47/47 pass.
• Legacy and supervised watcher suites pass; three live-ps probes were skipped.
• Core-heartbeat 11/11, core-PID/pgrep-portability, and probe-subject 11/11 pass.
• Python compilation, test discovery 4/4, git diff --check, and topic-diff scripts/review-checks.sh pass.
• watcher-sentinel-ownership reached 21/25; four live-process signaling cases were inconclusive here and are not the basis of the blocker.
• Local ruff was unavailable; the supplied hosted ruff and remaining CI/CLA rollup are green.

The added-line host-path scan is clean. /opt/homebrew and /Users/x occur only in fixtures; /tmp/sutando-tmux.sock is the existing runtime-socket default. Non-blocking: refresh the PR body, which still describes three groups although this head emits four.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Changes requested at d81c2e1f30ec1943e578ee54de30a3de0d7cd033 (COMMENT because this account authored the PR).

I inspected the complete six-file diff, watcher enumeration/ancestry, heartbeat identity owner, and the step-9 consumer. The existing inconsistent-snapshot blocker is independently reproducible through the production check_task_watcher() and _watcher_trees() with a real scratch sentinel:

sentinel = 200; watcher 100 and newly started watcher 200 share core 500
snapshot before 200 starts: session-owned (1): 100; same-core duplicates (0): none
verdict: these are legitimate. Do NOT stop them
complete snapshot: session-owned (1): 200; same-core duplicates (1): 100
verdict: Stop ONLY the ownerless roots and the same-core duplicates, keep the sentinel's (200)

At src/health-check.py:7161-7167, a validated sentinel absent from the earlier tree snapshot is omitted from all_roots; the affirmative predicate at 7180 consequently certifies an incomplete census. Refresh the complete snapshot or return unknown/retry when the live sentinel cannot be located. Add this two-snapshot control. Worst case is duplicate task processing, including duplicated external actions, until another health pass.

The author's current-head comment explicitly says the real post-restart task/reply witness is still absent. That existing live-path gate also remains: supply the scheduled service window, exactly one processing path, and delivered result after the code fix.

The 47-case pool-aware suite passed; full added-line review checks passed. The previous comment's proposed two-line cap on docstrings is withdrawn: current REVIEW.md explicitly excludes them. No live service was restarted and no contributor branch was modified.

[qingyun-wu]

Closed at the owner's direction (PR triage, 2026-09-12 03:45Z: "close the ones that are dated"). This belongs to the August rescue line, superseded by the worker-pool design v1 (#4037) and the skills/worker-pool layout landed in #4206; the live behavior is being consolidated on top of main. Branch preserved. Sutando core (qingyun-001)