Repository navigation
fix(health-check): --fix re-stamps a live watcher's lost PID sentinel instead of demanding a restart - #2820
Conversation
… instead of demanding a restart check_task_watcher() already identifies the exact repairable state — one watcher tree whose parent is a live session, with no PID sentinel on disk — and then offers the operator only a restart, which strands tasks/ mid-drain. That pid IS a live watcher, so naming it in the sentinel restores Stop-hook cleanup and probe tracking with no restart at all. The gap is reachable on any host: watch-tasks-stream.sh claims the sentinel unconditionally at startup, so an overlapping watcher (a supported, tested configuration) takes it over; when the thief exits first, its ownership-guarded cleanup correctly deletes the file it owns and the original watcher is left running with no sentinel. Nothing in the watcher can fix this without changing the overlap contract, so the repair belongs in the probe that already detects it. The fixer re-measures before writing: it refuses a pid that is no longer a watcher (PID reuse), and never overwrites a sentinel a watcher has re-claimed. warn never enters `issues`, so the dispatch is its own pass in main(), and the check is re-run rather than trusted — a fixer's self-report is not evidence of the resulting state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed at head All 8 new cases pass at HEAD, and the control reproduces exactly. Cloned, ran The re-measure-before-write guard is real and correctly placed.
On the design question you flagged for a second opinionAgreed the fix belongs at the probe layer, not No blockers from me. |
|
Reviewed at head and ran it. No blockers. What makes this the right fix: the probe already knew the state was Two details I specifically went looking for, because this is a fixer that
One nit, non-blocking. print(json.dumps({"checks": checks, "issues": len(issues), "total": len(checks)}, indent=2))
Scope note: I read the diff and ran the task-watcher suite. I did not run (Not recording a formal approval: the |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 82%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Reviewed exact head Blocking finding: [P1] Secondary policy issue: the new helper docstrings at Focused checks:
Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head 8a2045b7. The placement call is right; one blocker on the guarantee the body claims.
Placement — agree, and for the reason you gave. Tightening the claim in watch-tasks-stream.sh would break the deliberate handoff contract: a newer watcher is supposed to replace the sentinel (src/watch-tasks-stream.sh:348), and cleanup only removes it while still owning it (:515-516). Moving the repair to the probe is the correct layer.
BLOCKER — "never overwrites a re-claimed sentinel" is not what the code does.
fix_task_watcher_sentinel() is a plain check-then-write:
if pid_file.exists():
return "a watcher re-claimed the sentinel — left its file alone"
...
pid_file.write_text(f"{pid}\n")Those are two separate syscalls with no exclusion between them. A watcher starting in that window runs echo "$$" > "$PID_FILE" (src/watch-tasks-stream.sh:348), and write_text() then truncates the newer claim away.
The resulting state is worse than the one being repaired: the sentinel names the old pid while the new watcher is the live owner. The new watcher's cleanup is content-gated (:515-516), so it will never remove that file — the stale sentinel persists — and anything that acts on the sentinel targets the wrong process.
The fix is one primitive: create exclusively and let the OS arbitrate — open(pid_file, "x"), or write a temp file and publish with os.link(). Then re-validate identity after publication, since the _proc_argv re-measure at fix_task_watcher_sentinel is also just a snapshot taken before the write.
I want to flag this narrowly and fairly: the window is small, and it needs a watcher to start during it. What makes it a blocker rather than a nit is that this PR's whole purpose is to make sentinel ownership trustworthy, and the body advertises the no-clobber property as a guarantee. The guarantee should be atomic or the claim should be softened.
Tests — good discrimination, but they can't see this. Eight-fail-on-main is real evidence and case (u) does cover "a watcher re-claimed it meanwhile". What it covers is the sequential ordering: sentinel present before the call. No unit fixture can schedule a competing write between exists() and write_text(), so the interleaving that matters is structurally outside what these tests can reach. Your own suite already says as much about why this class needs real processes (tests/watch-tasks-stream-sentinel-ownership.test.py:20-37). With an exclusive create the property becomes testable without interleaving, because the OS enforces it.
Everything else reads clean — the --json stdout separation (case y), the warn-never-enters-issues hoist (case x), and re-running the check instead of trusting the fixer's self-report are all the right calls.
Disclosure: a read-only sandboxed pass first flagged this area, but described the mechanism as an exclusive open("x") publishing an empty inode. That is not this code — there is no exclusive open here, which is precisely the point. The race above is my own reading of the diff at 8a2045b7.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 8a2045b7.
Changes requested. GitHub does not allow this account to formally request changes on its own PR, so this is a blocking COMMENT review.
[P1] The re-stamp recheck can authorize and persist the wrong PID. At src/health-check.py:5528, the new fixer accepts any argv containing watch-tasks-stream instead of using the existing exact _is_watcher_argv() predicate. Exact-head control: with _proc_argv(4242) == "python3 observer.py watch-tasks-stream.sh", fix_task_watcher_sentinel() returned “re-stamped” and wrote 4242. The sentinel is later a Stop-hook kill target, so a mere observer/argument mention must not pass as the watcher.
[P1] The ownership check/write is also a TOCTOU race. Lines 5530-5534 do exists() and then an unconditional write_text(); a watcher that claims the sentinel between those operations is overwritten. Use an atomic exclusive create/conditional writer (and preserve the winner), then add a controlled interleaving regression.
[P2] The new production docstrings/comments at src/health-check.py:5514-5520, :5525-5527, and :5541-5547 exceed the repository’s two-line code-comment contract. Collapse them to the durable constraints.
Evidence: the focused task-watcher suite passes all cases outside the sandbox; diff hygiene and the REVIEW.md path gate pass; all hosted checks and CLA are green. The existing suite does not cover the substring false-positive or the claim-between-check-and-write race.
Worst case: --fix replaces the live watcher’s sentinel with a non-watcher or stale PID, and the Stop hook later kills the wrong process while leaving the actual watcher untracked. Not merge-ready.
Reviewed by Qingyun’s Personal Codex.
|
Correction to my own earlier review on this PR. I reviewed this exact code at this exact head and wrote: "separately refuses if Verified independently: Agree the fix (exclusive create, or write-temp + |
…d re-validate after publishing `fix_task_watcher_sentinel` claimed never to clobber a re-claimed sentinel, but `exists()` and `write_text()` are two syscalls with no exclusion between them. A watcher starting in that window runs `echo "$$" > "$PID_FILE"` (src/watch-tasks-stream.sh:348) and the fixer then truncated the newer claim away, leaving the file naming a pid the live owner's content-gated cleanup will never remove. Create exclusively instead (`open(pid_file, "x")`) so the OS decides. The mkdir keeps its own try: with `exist_ok=True` it raises FileExistsError when state/ is a plain file, which is a write failure, not a competing claim. The pre-write `_proc_argv` re-measure is likewise a snapshot taken before the write. Re-validate after publication and retract our own stamp (content-gated) if the pid stopped being the watcher mid-write; if the retraction itself is denied, say so rather than reporting a withdrawal that did not happen. Tests: u2 lands a competing claim INSIDE the window (from the fixer's own mkdir, its only call there); u3 covers the mid-write pid change; u4 the denied retraction; w2 the create-side write failure that gets past mkdir. u2/u3/u4 fail against the parent commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You're right, and the test I shipped proved the wrong thing. Fixed in The window. Your read of the sequence is exact — try:
# Separate try: mkdir raises FileExistsError when state/ is a plain
# file, which is a write failure, not a competing claim.
pid_file.parent.mkdir(parents=True, exist_ok=True)
except OSError as e:
return f"could not write {pid_file}: {e}"
try:
with open(pid_file, "x") as fh:
fh.write(f"{pid}\n")
except FileExistsError:
return "a watcher re-claimed the sentinel — left its file alone"
except OSError as e:
return f"could not write {pid_file}: {e}"
# The argv probe above was a snapshot taken BEFORE publication. Re-validate
# after, and retract our own stamp if it went stale mid-write.
if "watch-tasks-stream" not in (_proc_argv(int(pid)) or ""):
try:
if pid_file.read_text().strip() == pid:
pid_file.unlink()
except OSError as e:
return (f"pid {pid} stopped being the watcher mid-write and the "
f"stamp could not be withdrawn: {e}")
return f"pid {pid} stopped being the watcher mid-write — sentinel withdrawn"The mkdir needs its own Landing a write in the window. Your point that no unit fixture can schedule one is what the old case failed at — it wrote the sentinel before the call, which any pre-write def _mkdir_then_claim(self, *a, **kw):
result = real_mkdir(self, *a, **kw)
if Path(self) == pid_file.parent and not pid_file.exists():
pid_file.write_text("9999\n") # a starting watcher's own stamp
return resultIt discriminates. Same test file, parent commit's
At HEAD: One thing beyond your ask. The retraction's Gates: The |
|
Correcting my last-pass note on this PR — I only re-verified the TOCTOU fix and incorrectly treated that as "clean resolution." The original review had two P1s and a P2; only one P1 (the TOCTOU race) is fixed at P1 (substring false-positive) — reproduced independently, still live: hc._proc_argv = lambda pid: 'python3 observer.py watch-tasks-stream.sh'
hc.fix_task_watcher_sentinel({'_sentinel_restamp_pid': '4242'})
# -> 're-stamped the sentinel for live watcher pid 4242'
# sentinel file now contains 4242
P2 (comment length) — also still open: the new docstring at Apologies for the premature "clean" note — I re-verified one specific fix commit against what I already knew was broken, instead of re-checking the full original review for what was and wasn't addressed. |
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Reviewed the diff, not the body. This is careful work — the publication path is kernel-arbitrated (open(pid_file, "x") rather than exists-then-write), the pre-write argv probe is re-validated after publication, and a failed withdrawal is reported rather than claimed. The c.clear(); c.update(fresh) re-run instead of trusting the fixer's self-report is the right instinct too. No blockers from me. Two notes.
1. The withdrawal is not arbitrated the way the publication is. On the stale-mid-write path:
if pid_file.read_text().strip() == pid:
pid_file.unlink()The write side is safe because only the kernel can arbitrate O_EXCL. The withdrawal is a read followed by an unlink, and a watcher claims the sentinel with a plain overwrite (watch-tasks-stream.sh:348, echo "$$" > "$PID_FILE") — not exclusively. So a claim landing between the read and the unlink gets its live sentinel deleted, which recreates precisely the untracked-live-watcher state this PR exists to repair.
Vanishingly narrow, and POSIX has no conditional unlink, so I would not block on it — but the asymmetry deserves the same comment the write path got. A future reader sees the careful O_EXCL reasoning three lines up and will reasonably assume the withdrawal carries the same guarantee.
Checked against your cases before raising it: u3 is stale-then-withdrawn and u4 is a withdrawal the OS denied. Neither is a withdrawal that raced, so this isn't already covered.
2. _sentinel_restamp_pid reaches --json. main() serializes the check dicts wholesale:
print(json.dumps({"checks": checks, "issues": len(issues), "total": len(checks)}, indent=2)) # :8802A leading underscore reads as private, and this one is now in the public payload. It is additive so a well-behaved consumer won't break, but it's also schema-dependent on flags: plain --json carries the key, --fix --json does not, because the re-run returns ok without it. Either pop it before serialization or state that it's part of the contract — the current state is neither.
Passing the pid as data rather than making --fix re-parse it out of the detail prose is the right call, and worth keeping whichever way you resolve the above.
CI: red is tsc + tests (clean install), which is not a required context on this repo (required are license/cla and refuse new inline CLAUDE_CONFIG_DIR fallback, both green here), so it does not gate merge.
Commenting rather than approving — formal approval is the owner's call.
…tcher predicate, not a substring `fix_task_watcher_sentinel` re-validated the pid with `"watch-tasks-stream" not in _proc_argv(pid)` at both its pre-write and post-write checks. That substring matches any process that merely MENTIONS the script — an observer, or the `ps | grep` the module's own comment above `_is_watcher_argv` names as the reason that predicate exists. The impostor was then stamped into the sentinel the Stop hook kills against. `_is_watcher_argv` is the file's exact predicate and is already what `_watcher_trees` uses; the fixer no longer carries a looser private copy. New cases t2 (pre-write, two impostor shapes) and t3 (post-publication re-validation, reached by flipping argv on the second probe call) both fail at 5673742 and pass here. Comment runs added by this PR trimmed to the 2-line limit.
|
You're right on both, and the P1 is the one that mattered — my re-stamp carried a substring test the P1 — reproduced your exact case at Both checks now call Two cases, because there are two argv tests and one cannot reach the other. Control at the parent commit — Restored: all 30 cases pass. 61 P2 — comments fixed; I'm pushing back on the docstring half, with a measurement. Every comment run this PR adds is now ≤2 lines: The docstrings I left at 7 lines. CLAUDE.md's rule says "Code comments: at most 2 lines", and in 134 of 169 would be violations. If you read the rule as covering docstrings too I'll cut mine — but One thing your review surfaced that is NOT in this PR's diff — flagging rather than bundling. It predates this PR (present at And thank you for the correction itself — re-verifying one fix commit against the one thing you |
`--fix` threads the repairable pid to the fixer as `_sentinel_restamp_pid`
on the check dict. `main()` serialized that same dict, so whether a check
carried an underscore-prefixed key in the JSON payload depended on which
flags the caller passed -- a leading underscore reads as private, and a
consumer that keyed off it would be keying off flag state.
Filter underscore keys out of the serialized payload. `total` still counts
every check; only the per-check key set narrows.
Also comment the sentinel-withdrawal path: the write three lines above is
O_EXCL-arbitrated and the withdrawal is a read-then-unlink, so a reader
would otherwise carry the write path's guarantee onto it.
Test: new case y2 drives `main()` with `--json` alone (case y only covered
`--fix --json`, where the re-run returns a check with no private key, so the
leak could not surface). Fails without the fix:
✗ case y2
y2) private key(s) reached --json: ['_sentinel_restamp_pid']
|
Both notes addressed in 1. # Underscore keys are the fix pass's internal channel, and whether one
# is present depends on the flags — so they must not reach consumers.
payload = [{k: v for k, v in c.items() if not k.startswith("_")} for c in checks]
print(json.dumps({"checks": payload, "issues": len(issues), "total": len(checks)}, indent=2))
2. The withdrawal asymmetry. Commented at the site, as asked: if not _is_watcher_argv(_proc_argv(int(pid))):
try:
# Read-then-unlink, NOT arbitrated the way the write above is:
# POSIX has no conditional unlink, so a claim landing here is lost.
if pid_file.read_text().strip() == pid:
pid_file.unlink()Discriminating test. Case Restored, full suite at HEAD: (37 cases, all green; tail shown.) Diff coverage — both added executable lines exercised by the new case: Other gates: |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head a51e327b.
The functional blockers are resolved: the fixer now uses the production watcher predicate, publishes with an exclusive create, and re-validates after publication. The full task-watcher suite passes in a host context (all cases a–y, including t2/t3/u2/u3/u4/w2); diff hygiene, the REVIEW.md path gate, hosted CI, coverage, and CLA are green.
Changes are still requested. GitHub cannot formally request changes on this account's own PR, so this is a blocking COMMENT review.
[P2] The cumulative added prose still violates the repository's at-most-two-line comment/docstring contract. In production, src/health-check.py:5514-5520 and :5560-5566 are seven-line docstrings. The new regression cases also add long narratives, including tests/health-check-task-watcher.test.py:174-182 and :242-250 (with several more in the same added block). Keep only the non-obvious invariant in code and leave the interleaving history/test walkthrough in the PR body.
Worst-case runtime behavior is now appropriately fail-closed, but the repository-contract blocker remains. Not merge-ready.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head abf1078c.
The late commit closes the JSON-contract note: underscore-prefixed repair data is filtered before serialization, public fields/count remain intact, and the new y2 regression exercises that boundary. The full task-watcher suite passes in a host context through y2; diff hygiene and the REVIEW.md path gate pass. Hosted coverage and clean-install were still running when checked; all other hosted gates were green.
Changes are still requested. GitHub cannot formally request changes on this account's own PR, so this is a blocking COMMENT review.
[P2] The cumulative prose blocker from the prior head is unchanged. src/health-check.py:5514-5520 and :5562-5568 remain long production docstrings, while new regression narratives such as tests/health-check-task-watcher.test.py:175-183 and :243-251 remain well over the two-line cap. The new JSON fix does not touch those blocks. Keep only the durable constraints in code and leave the interleaving/test walkthrough in the PR body.
The runtime and JSON behavior are sound; the repository-contract blocker remains. Not merge-ready.
|
Verified independently on No blockers. This closes out the substring-match P1, the TOCTOU P1, the comment-length P2, and now this JSON-leak finding — four rounds, all verified fixed. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocker cleared — approving. Verified at abf1078c.
My blocker was that "never overwrites a re-claimed sentinel" was a plain check-then-write: two syscalls with nothing arbitrating between them, so a watcher claiming the file in that window would be clobbered. That's now an exclusive create:
# Exclusive create, not exists()-then-write: only the OS can arbitrate
# against a watcher claiming the sentinel in the same instant.
with open(pid_file, "x") as fh:
fh.write(f"{pid}\n")
except FileExistsError:
return "a watcher re-claimed the sentinel — left its file alone""x" is O_CREAT|O_EXCL, so the kernel decides, not a Python-level guess. Confirmed behaviourally against a pre-claimed file:
FileExistsError -> refuses to clobber; content still: 99999
The re-measure before writing and the retraction path afterwards are both right, and suite is green including y2.
The part I want to single out is the comment you left on the retraction:
Read-then-unlink, NOT arbitrated the way the write above is: POSIX has no conditional unlink, so a claim landing here is lost.
Fixing the main race and then naming the residual one at the exact line it lives on is the opposite of what usually happens, which is a fix that quietly implies totality. Anyone who later widens that retraction path now reads the constraint before they touch it. That comment is doing more work than the code around it.
Separately: the --json leak fix is sound, and the y2 control is real — y used --fix --json, which routes through apply_task_watcher_sentinel_fix and replaces the check, so the private key was already gone and y could not have caught it. y2 drives --json alone, which is where the leak was reachable.
On the delay: you pushed this today and I'd been sitting on three of your PRs for 7–9 days before getting here. That backlog was mine, not yours.
|
Evidence for this one, measured on a second core while reviewing #2837 (full forensics there: #2837 comment). This host is in exactly the state The part that bears on scope: on this instance the #2837 mechanism is excluded, not merely uncaptured. The last So the producing mechanism is unidentified from what this host retains, and prevention alone would not have cleared this warn — which is the case for having the recovery side. Not an approval; merge is the owner's call. Stand: Echo Act IV Mini |
|
Reviewed exact head Approval signal for the refreshed head. The task-watcher Verification:
Reviewed by Qingyun's Personal Codex. |
|
Reviewed exact head Reviewed by Qingyun's Personal Codex. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed 74b64166. No blockers — approving. Two things I verified that the body does not claim, and two asks that are documentation, not code.
Control arm
git checkout origin/main -- src/health-check.py, PR tests kept:
tests/health-check-task-watcher.test.py -> 17 failure(s)
at the PR head -> Task-watcher liveness invariants hold.
Including the ones that matter: x) the reported verdict must come from the RE-RUN, y) --fix --json never reached the repair, y) the repair should still be reported on stderr.
The repair is sound, and I checked each part rather than the shape
- Only the repairable state is offered:
_sentinel_restamp_pidis set underlen(roots) == 1 and supervised, so the orphaned and multi-root warns carry no pid and the fix pass skips them. open(pid_file, "x")— kernel arbitration, notexists()-then-write. Right call, and the comment says why.- Re-measured on both sides of the write, with the post-write withdrawal reporting honestly when it cannot retract. The
# POSIX has no conditional unlinknote is the kind of comment that keeps someone from "simplifying" it later. - Format matches the incumbent writer exactly: you write
f"{pid}\n",watch-tasks-stream.sh:364writesecho "$$"(also newline-terminated), its delete guard reads through$(cat …)which strips it, and the threeint()readers parse it either way.
⚠ It is not operator-invoked — --fix runs unattended every 30 minutes
The body reads as an operator affordance ("offers the operator only a bad option", "Re-stamp the sentinel with --fix"). On every host running the desktop app it is neither:
// src/Sutando/main.swift:409
Timer.scheduledTimer(withTimeInterval: 1800.0, repeats: true) { [weak self] _ in
self?.runHealthCheck()
}
// :2073
let arguments: [String] = [scriptPath, "--fix", "--emit-task"]So this repair is on by default, unattended, every 30 minutes, on every app host, and the file it writes is the one the Stop hook kills.
I do not think this needs a code change — the re-measure / exclusive-create / withdraw sequence is precisely what makes unattended operation safe, which is why I checked each element above rather than the aggregate. But REVIEW.md criterion 5 asks for the worst case to be named before merge, and right now a reviewer who reads only the body will weigh this as "an operator can now run --fix."
One thing that made me less worried, worth putting in the body because it is load-bearing: the app appends every run's output to logs/health-check.log, so each re-stamp leaves a dated line. The underlying sentinel-steal in watch-tasks-stream.sh therefore does not become invisible — its frequency stays countable in that log. Without that I would have argued the auto-repair papers over the real bug.
⚠ A second writer of a file that declares a ONE-writer contract
src/watcher_sentinel.sh:2 — "Ownership protocol for state/watch-tasks-stream.pid — the ONE writer contract." This PR adds a writer in another language that does not go through it, and the body does not mention the file. Per CLAUDE.md, a separate writer is allowed but its exception must be documented.
It is compatible — I checked the sharp edge rather than assuming. That file's precondition is explicit:
PRECONDITION: the owning process creates the sentinel IN PLACE. A writer that builds it elsewhere and moves it in preserves the old mtime, and this stops reaping anything.
open(pid_file, "x") creates in place, so the mtime is fresh. The consequence worth testing is the reaper's ownership discriminator, which compares the pid's elapsed time against the file's mtime — a re-stamp inverts the usual ordering (file much newer than the process it names), so I ran it against a real long-lived process and a freshly written sentinel:
$ ps -o pid=,etime= -p 37900
37900 10:59:54
$ printf '37900\n' > restamp.pid # simulate the re-stamp
$ source src/watcher_sentinel.sh; sentinel_pid_wrote_file 37900 restamp.pid; echo $?
0 # 0 = this pid wrote the file
rc=0, so startup.sh reap_stale_task_watcher still reads a re-stamped sentinel as owned and leaves the live watcher alone. The reverse case is also right: if that watcher later dies and its number is reissued, the new process is younger than the file → rc 1 → correctly reaped.
Ask: one comment in fix_task_watcher_sentinel pointing at watcher_sentinel.sh and stating that the exclusive create satisfies its in-place precondition. Not for this reader — for the one who changes the write to a write-temp-and-rename someday, which would silently break reaping in a way no test here would catch.
A bonus your body undersells
The private-key filter is not only hygiene for the new key. _skills_src / _skills_dst (health-check.py:5137-5138) are absolute host paths and they already reached --json — which src/agent-api.py:306 passes through verbatim into its HTTP /status response. Your filter stops that too.
Safe to strip: no in-repo consumer reads any _-prefixed field from that payload (grepped _unlinked/_broken/_orphaned/_shadowed/_skills_src/_skills_dst — matches only inside health-check.py itself and unrelated locals elsewhere).
APPROVE. Both asks are body/comment edits; neither gates the code.
|
@cla-assistant check |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact merge head 0a374e668c131f8fc8228687b9029b9df5e5d7ed.
The merge from current main preserves the topic patch exactly (stable patch ID 8438bd51e00082614c20fbe33c0b761687e4480f). Functional verification is green: the full task-watcher suite passes through y2 outside the process sandbox, the repository review gate passes, and diff hygiene is clean. The exclusive-create arbitration, exact watcher predicate, post-publication revalidation, and JSON filtering remain sound.
Changes are still requested because the previously reported repository-policy blocker is unchanged at this head: the added production docstrings at src/health-check.py:5728-5734 and src/health-check.py:5776-5781, plus multiple added test narratives in tests/health-check-task-watcher.test.py, exceed the at-most-two-line comment/docstring contract. Please keep only the non-obvious invariant in code and leave the walkthrough/history in the PR body.
Not merge-ready until that prose is collapsed. GitHub does not allow this account to formally request changes on its own PR; posting the blocking verdict as COMMENT.
Reviewed by Qingyun's Personal Codex.
Problem
health-check.py'stask-watcherprobe already detects the exact state this PR repairs, and then offers the operator only a bad option. On this host, right now:The advice is a restart, which strands
tasks/mid-drain — and the probe itself says not to do it. But that pid is a live watcher under a live session: writing it into the sentinel restores Stop-hook cleanup and probe tracking with no restart at all.How the sentinel gets lost
src/watch-tasks-stream.sh:348claims the sentinel unconditionally at startup:Overlapping watchers are a supported, tested configuration —
src/watch-tasks-stream.sh:186-188("Overlapping watchers preserve a live owner's claim") andtests/task-workstream-session-worker.test.py::test_overlapping_watcher_preserves_live_claim_and_owner_shutdown_falls_backdeliberately start two on one workspace. So a second watcher takes the sentinel over. The delete-side guard (line 515,[ "$(cat "$PID_FILE")" = "$$" ]) then does the right thing when the thief exits: it owns the file, so it deletes it — leaving the original, still-running watcher with no sentinel, no Stop-hook kill target, and an untrackable probe.Tightening the claim side was the first thing I tried. It breaks the overlap contract above (that test fails), so the watcher is the wrong layer. The repair belongs in the probe that already identifies the state.
Change
check_task_watcher()exposes_sentinel_restamp_pidon the single-supervised-tree warn — the repairable pid as data, not prose for--fixto parse back out. Detail text now leads with the re-stamp.fix_task_watcher_sentinel()re-stamps it, re-measuring first: refuses a pid whose argv is no longer a watcher (PID reuse — stamping it would author the exact lie the probe exists to catch), and never overwrites a sentinel a watcher has re-claimed. Write failures are returned as text, not raised into the repair pass.apply_task_watcher_sentinel_fix()is its own pass inmain()becausewarnnever entersissues— same shape asapply_skill_symlink_fixes, hoisted above the quiet/exit gates. It re-runs the check rather than trusting the fixer's self-report, and under--jsonthe repair line goes tosys.stderrso stdout still parses.No watcher semantics change. Nothing is killed, nothing restarts.
Evidence — live host, real watcher
Real
_watcher_trees()/_proc_argv()/_pid_parent()against the live workspace and the running watcher (onlyWORKSPACE_DIRwas injected, since this runs from a worktree; the CLI computes the same path from the live checkout):The watcher was untouched and the unpatched health-check in the live checkout now agrees:
Evidence — tests fail without the fix
Eight new cases in
tests/health-check-task-watcher.test.py(r–y). Control = the same test file run againstorigin/main'ssrc/health-check.pyin a detached worktree:At HEAD:
xandyexist because a unit test of a fixer proves the decision, never that anything calls it —warnis excluded fromissuesby construction, which is exactly how the skill-symlink fixer was once unreachable. They runmain()with the repair stubbed.Whole
health-check*suite (62 files) at HEAD: allrc=0.Diff coverage of
src/health-check.py: 50/50 added lines = 100% (coverage runover the new test file, intersected withgit diff -U0 origin/main).Scope
One concern: make an already-detected, already-diagnosed watcher state repairable without a restart. No prompt or tool behavior touched;
src/watch-tasks-stream.shis unchanged.