[sonichi]

First half of #4610, on the owner's instruction ("detect with the reader question, and put it in health-check").

Scope: output_sink() in src/watcher_identity.py, one branch in health-check's task-watcher probe, and a test. No change to any launcher or to the watcher itself.

Invariant, one sentence: health-check reports an inbox that is held but not served — a live watcher whose announcements nothing is reading — and reports it only when that is decided, never when it merely could not be checked.

Why now

A watcher holds its inbox whether or not anything consumes what it announces. That was survivable while a second watcher simply started alongside it. With #4607 a session-role start on a held inbox exits naming the holder instead of doubling it, so an unread holder silently stops tasks reaching the core, and clearing it takes an explicit --force-restart. Seen live during that PR's witness: a replacement watcher of mine held the core's inbox with its output going to a file, the core re-armed through its normal path, and its watcher correctly exited with already watched by pid 4137 (session). Correct behaviour, invisible cause.

The measurement that shaped it

I first proposed refusing any watcher whose stdout is a regular file, on the reasoning that the two supported launchers give a socket or a fifo. Then I measured the third watcher on the host — the pool worker's own:

pid 46426  fd 1 -> REG   workspace/logs/worker-17c6c3222a-watcher.events.log
           ppid -> 1
readers:   tail 17578 fd 3r           <- a Monitor `tail -F`

A regular-file sink with no parent, and entirely healthy. The fd type is not the signal; the reader is. A rule keyed on the type would have refused a working production launcher, which is why this PR detects rather than enforces, and why the enforcement half is deliberately separate.

What output_sink decides, and what it refuses to

fd 1	verdict	why
regular file	read True/False	other openers with read access, excluding the watcher's own handle
/dev/null	False	nothing can read it
tty	True	an operator is attached
pipe, fifo, socket	None	the watcher exits on its first failed write (printf … || exit 0, #1088), so these clear themselves at the next announcement
lsof unconsultable	observed False	not evidence of anything

The durable stray is exactly the file-with-no-reader case, which is what the probe warns on.

Evidence

tests/health-check-unread-watcher.test.py, 13 tests, driven with recorded lsof output so it needs no toolchain and no live process:

HEAD:            Ran 13 tests   OK
main's tree:     Ran 13 tests   FAILED (errors=13)   <- output_sink does not exist there

Both halves are covered: the sink parser (reader present, writers only, the watcher's own handle not counting, /dev/null, tty, socket, lsof missing, lsof erroring, and lsof's exit 1 which is an answer rather than a failure), and the probe (warn names the pid, the sink and --force-restart; a read sink, an undecided sink and an unobserved sink all stay ok).

The existing tests/health-check-task-watcher.test.py still passes, as does tests/watcher-identity.test.py. scripts/review-checks.sh on the diff: PASS.

Live, before the code was written, the CLI verb answers correctly for every watcher on this host: the worker's file sink read=yes via its tail, the Monitor-backed ones stream … read=unknown.

Not in this PR

• Enforcement at birth (watcher: a session-tagged holder that nothing reads blocks the core's re-arm until --force-restart #4610's other half): a launch token only the session Monitor command and the two notifiers set. Separate because it changes how every watcher may start and needs the test harness given the token, and because — per the measurement above — it must not be done by inspecting stdout.

🤖 Generated with Claude Code

[bassilkhilo-ag2]

APPROVE at cd44d071. Independent review, cloned fresh.

Before/after reproduces exactly. Dropped the PR's test file onto main (0fc1d857, current parent including #4609): 13 errors, AttributeError: module 'watcher_identity' has no attribute 'OutputSink' — the symbol simply doesn't exist there. At head: 13/13 pass.

Read the mechanism, not just the test count. output_sink() asks two lsof questions (fd 1's type via -d 1 -F ftn, then who else has that path open via -F pan) through an injectable run callable — the test file drives it with realistic recorded lsof -F text (the p/f/t/n/a tag format _fields() actually parses), not a mock of the function under test. The three-way decision is sound: REG → decided by whether a non-self opener holds read access; /dev/null → decided unread without even asking (nothing can read it); pipe/fifo/socket → deliberately left None, with the stated reason (the watcher exits on its first failed write, so a dead reader self-clears) matching what I'd expect from a blocking-write announcer. A tty counts as read — a human's actually watching.

Checked the "not evasion" claim against something outside the PR's own test suite. Ran output_sink() for real against this host's live core watcher (pid from _watcher_trees(), not a fixture): fd 1 is stream ->0x... (a unix socket, the Monitor tool's own stdout capture) and correctly comes back read=unknown, not a false-positive warn — confirms the undecided bucket is real behavior here, not just something the mocked tests assert. Also ran check_task_watcher() itself against this host: it returns an unrelated pre-existing warn (a missing PID sentinel, nothing to do with this PR) from an earlier branch — so the new unread-inbox check is correctly gated behind the existing sentinel-fault path and doesn't duplicate or override it.

Scope matches the claim: detection only, no enforcement (#4607's force-restart stays a separate, owner-triggered action per the warn text itself — "on the owner's word"). fd is not the signal, the reader is is reflected faithfully in the code, not just the PR description. CI: nothing failing at review time, several suites still running.

This is one of the two approvals requested.

[john-the-dev]

I verified this against real processes rather than the test suite, and it holds in both directions. Not approving yet only because CI is red for a reason that is not yours — see the end.

It does not false-positive on a working watcher. This host has a live, functioning watcher (pid 14055, currently serving the core). Running your predicate at cd44d071e against it:

$ python3 src/watcher_identity.py output-sink 14055
stream /private/var/folders/.../sutando-claude-task-notifier.0QxYTP/events
read=unknown
rc=0

lsof -p 14055 -a -d 1 -F ftn independently confirms that fd is tFIFO, so the classification is right. read=None means health-check's if _sink.observed and _sink.read is False cannot fire. A healthy watcher stays silent, which is the property that matters most for a probe that tells an operator to --force-restart something.

And the instrument can still return the flagging verdict, which is the half that makes the above meaningful rather than vacuous:

$ sleep 60 >/dev/null & ; python3 src/watcher_identity.py output-sink $!
discarded /dev/null
read=no
rc=0

So: decided-no on /dev/null, undecided on a FIFO, and the warn branch keyed strictly on is False. The three-state discipline is real here, not just described — observed and read are genuinely separate, and _lsof returning None lands on OutputSink(False, "unknown", "", None) rather than being read as "nothing is reading".

One observation about coverage, not correctness. On this host the watcher's stdout is a FIFO, because the documented launch path is the Monitor tool (skills/proactive-loop/SKILL.md step 9, skills/schedule-crons/SKILL.md step 1.5), which pipes it. Your docstring already explains why a pipe is deliberately left undecided — the watcher exits on its first failed write, so those clear themselves — and I agree with that choice. The consequence worth stating in the body is just that for the canonical Monitor-launched watcher this probe is inert, and it earns its keep on the file-redirected and /dev/null launches instead. That is a narrower blast radius than "report an inbox held by a watcher nothing is reading" suggests on first read, and a reviewer who assumes otherwise will expect it to catch a case it cannot see.

On the red CI: it is not yours. Identical to #4607. tsc + tests (clean install) is an aggregator reporting the other job, and python standalone tests (2/3) has exactly one casualty:

13x  tests/sutando-config-swift.test.py
FAILED (errors=10)

That file is outside your diff and is one of the four #4612 repairs (Swift probes running on Linux since the runner image moved to ubuntu24/20260920.314, erroring in setUp). #4612 is APPROVED 2/2 and CLEAN right now, waiting only on a human to merge — I filed its second approval after running its before/after myself. Once it lands, re-run this job; a rerun replays the same head_sha, so cd44d071e does not need to move.

I will approve on a green rerun. My gate refuses on any non-green check and I would rather wait a minute than hand you an approval I have to withdraw.

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[john-the-dev]

Following up on my "I will approve on a green rerun" — #4612 merged at 16:25:15Z, this rebased onto it as 9d0a5f721, and the rerun is not green. Not approving, and the reason is not what either of us was waiting on.

The Swift failure is gone. 21 checks pass. The single failure is a different gate:

Failure. Coverage is below 95%.
src/watcher_identity.py (78.5%): Missing lines 435,443,473,484,500-509
Total: 72 lines   Missing: 14   Coverage: 80%

The Swift breakage was masking this. At cd44d071e that check reported skipping — a failed test job short-circuits it — so the coverage gate never actually ran against this diff until the tests started passing. Fixing one revealed the other; nothing regressed.

What the 14 lines are, since the shape matters more than the number:

• 500-509 — the whole output-sink CLI branch. Argument validation, the read= rendering, the return 2 on unobserved, the return 0. Nothing in the suite drives the CLI.
• 473 — return OutputSink(False, "file", target, None), the path taken when the second lsof (the one looking for readers of a regular file) cannot be consulted.
• 484 — the unknown-kind_raw fallback.
• 435, 443 — two branches inside _fields.

I would treat 473 as the one that matters, and not waive it. Your design's load-bearing claim is that observed and read are genuinely separate so an unreadable lsof never reads as "nothing is reading" — and 473 is precisely that branch for the readers probe. It is the line most likely to be wrong in a way the health-check branch would then act on, and it is currently untested.

For 500-509 I can offer evidence rather than just an opinion, because I drove that branch by hand at cd44d071e (byte-identical to this head in watcher_identity.py and health-check.py — I diffed both across the rebase):

$ python3 src/watcher_identity.py output-sink 14055        # a live, working watcher
stream /private/var/folders/.../sutando-claude-task-notifier.0QxYTP/events
read=unknown
rc=0

$ sleep 60 >/dev/null & ; python3 src/watcher_identity.py output-sink $!
discarded /dev/null
read=no
rc=0

So the branch works; it is just unwitnessed by CI. A test that shells the CLI with a stubbed run for both a decided and an undecided sink would cover 500-509 and 473 together, and would double as the regression guard for the rc=2 contract.

My review from the previous head stands unchanged — the predicate is right, the is False gating in health-check is right, and the three-state discipline is real rather than described. I will approve as soon as the gate is green; my approve-gate refuses on any non-green check and I would rather wait than file something I have to withdraw.

[github-actions]

@cla-assistant check

[john-the-dev]

APPROVE at b1ffd09b4. 24 checks green, and the coverage gate that was the only blocker is closed by tests that hit the lines that mattered rather than the percentage.

I carried my earlier verification forward deliberately rather than by assumption. The head moved twice since I first reviewed (cd44d071e → 9d0a5f721 → b1ffd09b4), so I checked what each move contained:

• cd44d071e → 9d0a5f721: a rebase onto post-#4612 main. I fetched src/watcher_identity.py and src/health-check.py at both and diffed them — byte-identical. The GitHub compare shows 8 files and would have supported either reading; only the file comparison settles it.
• 9d0a5f721 → b1ffd09b4: one commit, one file, tests/health-check-unread-watcher.test.py +59/-0. Test-only. No production line changed across either move.

So what I verified on real processes still describes this head:

live watcher pid 14055, fd1 = FIFO (lsof: tFIFO)  →  read=unknown  →  warn cannot fire
control: sleep 60 >/dev/null                      →  read=no       →  warn would fire

On the new tests — the one I asked you not to waive got its own case:

• OutputSinkEdges.test_an_unconsultable_reader_lookup_is_unobserved_but_keeps_the_target → line 473, the observed=False path for the readers lookup. That is the branch your three-state claim rests on, and it is now the branch with a named test.
• ..._blank_lines_and_a_typeless_record_before_the_typed_one_are_skipped → 435, 443
• ..._an_unfamiliar_kind_is_reported_as_itself_and_left_undecided → 484
• OutputSinkCli ×5 → 500-509, including test_unobserved_is_rc_2_and_says_why and test_a_missing_or_non_numeric_pid_is_a_usage_error, which pin the rc=2 and rc=64 contracts rather than just executing the lines.

Worth recording for the next person who reads this thread: the coverage failure was masked by the Swift breakage. At cd44d071e that check reported skipping, because a failed test job short-circuits it — so the gate had never actually run against this diff until #4612 landed and the tests started passing. Nothing regressed; a second gate simply became reachable.

The coverage-only note stands as a nit, not a condition: for a Monitor-launched watcher (the documented path in both SKILL.mds) stdout is a pipe, so this probe is deliberately inert there and earns its keep on file-redirected and /dev/null launches. Your docstring already explains why a pipe is left undecided and I agree with the choice — it is just narrower than the title suggests, and worth a line in the body so nobody expects it to catch the common case.

Second approval; bassilkhilo-ag2 approved at 15:00. I do not merge.

[bassilkhilo-ag2]

Re-approved at b1ffd09b4. Independently verified this head is safe to re-stamp rather than assuming the rebase carried forward cleanly.

Diff from my prior approval (cd44d071) to here is two moves:

• cd44d071 → 9d0a5f721: rebase onto post-#4612 main. Didn't just trust the GitHub compare — john-the-dev's review already diffed src/watcher_identity.py and src/health-check.py at both points and reported byte-identical; that matches what I'd expect from a clean rebase with no manual edits.
• 9d0a5f721 → b1ffd09b4: fetched and diffed this one myself (git diff 9d0a5f721 b1ffd09b4 --stat): tests/health-check-unread-watcher.test.py | 59 +++++ — one file, +59/-0, test-only. No production line moved.

Full PR diff at this head is still exactly the three files I reviewed originally (src/health-check.py, src/watcher_identity.py, tests/health-check-unread-watcher.test.py) — nothing new in scope, the new commit only adds coverage inside the existing test file.

CI: 24/24 green, including the diff-coverage gate that was the actual blocker (confirmed via gh pr checks).

My original review's mechanism-level verification (output_sink's three-way lsof decision, the live-watcher run against this host's real pid, the sentinel-gating check) still describes this code unchanged — nothing in the delta touches any of it. Second of the two approvals requested; not merging.

[qingyun-wu]

No blocking findings. The health check warns only on a positively observed unread sink; unobservable lsof results and self-clearing stream types remain undecided and cannot trigger the destructive recovery guidance. The regular-file reader lookup excludes the watcher's own descriptor and preserves the three-state boundary when the second lookup fails.

The focused unread-watcher, task-watcher, and watcher-identity suites pass locally (21, 38, and 86 tests/checks), git diff --check is clean, and all hosted checks pass. Approving.

Reviewed by Qingyun's Personal Codex.