fix(bin): name what actually ended a watcher cycle - #3280
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
|
Speaking as Kun's firstmate: Reviewed HEAD Contract-class: restore. Issue #1375's actionable-failure half: the typed VISION (per rule):
Attestation HTML binds |
"watcher: FAILED - cycle ended without an actionable reason" gives the
operator nothing to act on. A watcher that exited cleanly, one that died
leaving its lock behind, one another watcher replaced, and one that is still
live but no longer beating all reach that sentence, and all need different
responses. In the incident that prompted this it twice read as a false alarm
when it was not, and cost a twelve-hour supervision gap.
The typed prefix is unchanged, so existing consumers keep matching, and
neither the verdict nor the exit status moves. Only the evidence improves:
watcher: FAILED - cycle ended without an actionable reason:
watcher pid=19771 exited 1 without delivering a wake
(last beacon 3s ago, lock now: unheld)
An arm that merely ATTACHED holds no handle on its watcher's exit status, so
where the evidence is a real exit code it is read back from the lifecycle
ledger the OWNING arm already wrote. The lookup is bound to the same watcher
pid, the same recorded process identity, and a close no earlier than this arm
attached, so neither a recycled pid nor an older cycle can be read as this one.
It stays diagnostic: a missing or unreadable row degrades to the disposition
text and never changes the verdict.
Two details that are easy to get wrong and are covered by tests:
- Liveness inside the evidence is identity-qualified rather than bare. A
recycled pid is a live pid, and treating it as the watcher would name an
unrelated process to the operator while suppressing the recorded exit code
that is the only remaining evidence in exactly that case.
- The ledger probe is rebuilt through the same transforms the ledger applied
on the way in. lock_snapshot cleans each part and cycle_log_append cleans the
whole composite again, and cut(1) over a pair is not cut(1) over each half,
so a singly-truncated probe stops matching its own row once an identity is
long enough - which a deep worktree path reaches.
The pull warning and the turn-end block keep their verdicts, exit statuses and
alarms exactly as they were; only their wording now separates a live
identity-matched holder that has not beaten from nothing running at all, so one
fault is not read as the other. Both read the holder through a wait-free lock
check, because a guard whose job is to block a turn must not wait to phrase
itself.
Both regressions were checked against the code with their own fix removed. The
mutant for the evidence clause reproduces the reported symptom exactly - a
fresh beacon, a failed cycle, and nothing to act on - and the long-identity
mutant loses the recorded exit while keeping the verdict, which is the precise
shape of that defect.
… liveness unknown
…nal round vocabulary
12525b7 to
d97be75
Compare
2 participants
Intent
Make a watcher cycle that ends say what ended it. This is a contribution to kunchenguid/firstmate answering issue #1375, "watcher: cycle ends without an actionable reason", which the maintainers triaged as ready-for-pr and whose contract class they named themselves: "restore (actionable watcher failure + honest liveness)".
THIS CHANGE IS DELIBERATELY HALF OF THAT, AND THE PR SAYS SO. It fixes the actionable-failure half - the fault in the issue's title. The honest-liveness half (the issue's second diagnostic note, that the beacon can read fresh while supervision is already dead) was attempted at length on this branch and deliberately deferred; the reasoning is recorded in docs/verification/supervision.md and written up for the issue thread. Reviewing this as if it claims to close the whole issue would be reviewing something it does not claim.
THE PROBLEM. "watcher: FAILED - cycle ended without an actionable reason" gives an operator nothing to act on. A watcher that exited cleanly, one that died leaving its lock behind, one another watcher replaced, and one that is still live but no longer beating all reach that same sentence and all need different responses. In the incident that prompted this it twice read as a false alarm when it was not, and cost a twelve-hour supervision gap.
WHAT IT DOES. The typed prefix is unchanged, so anything already matching on it keeps matching, and neither the verdict nor the exit status nor the alarm moves anywhere in this change. Only the operator's evidence improves, with a clause naming what actually ended the cycle.
Where that evidence is a real exit code it is read back from the lifecycle ledger this repo already writes (bin/fm-watch-arm.sh's .watch-cycle-exits.log, which exists on main). An arm that merely ATTACHED holds no handle on its watcher's exit status, which is exactly why the bare sentence was reached in that case. The lookup is bound to the same watcher pid, the same recorded process identity, and a close no earlier than this arm attached, so neither a recycled pid nor an older cycle of the same watcher can be read as this one. It stays diagnostic: a missing or unreadable row degrades to the disposition text and never changes the verdict.
DELIBERATE DECISIONS A REVIEWER WOULD NOT INFER FROM THE DIFF.
Liveness inside the evidence is identity-qualified rather than bare. A recycled pid is a live pid, so bare liveness would both name an unrelated process to the operator and, by implying the watcher never exited, suppress the recorded exit code that is the only remaining evidence in exactly that case.
The ledger probe is rebuilt through the SAME transforms the ledger applied on the way in. lock_snapshot cleans each part, then cycle_log_append cleans the whole "pid:|identity:" composite again, and cut(1) applied to a pair is not cut(1) applied to each half. A probe truncated only once silently stops matching its own row past roughly a 497-character identity. That is reachable in practice, not theoretical: this repo's own pooled worktree paths are long and the identity carries the full command.
The pull warning and the turn-end block keep their verdicts, exit statuses and alarms exactly as they were. Only their wording separates a live identity-matched holder that has not beaten from nothing running at all, so an operator does not read one fault as the other. Both read the holder through a deliberately wait-free lock check that never sleeps and never watches the beacon, because a guard whose job is to block a turn must not wait in order to phrase itself. Nothing in this change waits on anything: the additions to bin/ are pure reads and printf.
Nothing was made quieter and nothing fails open. This change only adds text to failures that already fired.
TEST DISCIPLINE. Both regressions were checked against the code with their own fix removed, because a case that cannot fail proves nothing. The evidence-clause mutant reproduces the reported symptom exactly - "watcher: FAILED - cycle ended without an actionable reason" beside a fresh beacon and nothing to act on. The long-identity mutant keeps the verdict but loses the recorded exit, falling back to the generic disposition text, which is the precise shape of that defect; that fixture asserts its own identity length before relying on it so it cannot pass vacuously on a short path. The banner cases each pair a live unbeaten holder against nothing running at all, with both legs still warning and still blocking respectively.
VERIFICATION on this head: bin/fm-lint.sh clean, bin/fm-doc-audience-check.sh ok, and the fm-watcher-lock, fm-guard-stale-banner, fm-turnend-guard, fm-watch-arm and fm-claude-stop-autoarm suites all pass with failed=0.
HISTORY, so the branch shape is not surprising. An earlier version of this branch also carried the beacon-freshness work. That approach introduced five regressions of its own across four review rounds - each fix bounding one timed window and opening another - and two of them broke guarantees the change itself had stated. It was dropped rather than pursued further, and this branch was rebuilt from origin/main carrying only the half above. No part of that machinery remains: there is no timed window, no observation budget, and no receipt anywhere in this change.
Delivery: a pull request against UPSTREAM kunchenguid/firstmate from the sepinali fork, referencing issue #1375, and explicit that it fixes one of the two halves.
What Changed
watcher: FAILED - cycle ended without an actionable reasonnow carries an evidence clause naming what ended the cycle — a clean exit that released the lock, a death that left the lock behind, a lock that moved to another watcher, or a still-live watcher whose beacon stopped advancing. The typed prefix, the verdict, and the exit status are unchanged. Where the evidence is a real exit code,bin/fm-watch-arm.shreads it back from the.watch-cycle-exits.loglifecycle ledger it already writes, bound to the same watcher pid, the same recorded process identity, and a close no earlier than this arm attached; only rows classifying the watcher's own termination are eligible, so an arm-interrupted row is not reported as the watcher's fate. A missing or unreadable row degrades to the disposition text and never moves the verdict. Liveness inside the clause is identity-qualified and answers three ways — re-proven live, provably gone, or unknown — and the ledger probe is rebuilt through the same doublecycle_clean_fieldtruncation the ledger applied on the way in, so a long identity cannot silently stop matching its own row.bin/fm-guard.sh's pull warning andbin/fm-turnend-guard.sh's turn-end block separate a live identity-matched lock holder that has not beaten from nothing running at all, keeping their verdicts, exit statuses, and alarms exactly as they were. Both read the holder through a newfm_watcher_live_holder_pidinbin/fm-wake-lib.sh, a wait-free lock read that re-proves the recorded identity and never sleeps or watches the beacon.tests/fm-watcher-lock.test.sh(attached-arm owner-recorded exit, the same fixture from a deep path that asserts its own identity length, the arm-interrupted reason filter, and the unprovable-liveness pair) and paired live-holder-vs-absent-watcher legs intests/fm-guard-stale-banner.test.shandtests/fm-turnend-guard.test.sh.docs/verification/supervision.mdrecords each mutant run with its own fix removed, plus one leg (an empty recorded identity) that ships unproven and why it could not be staged. Docs also state that the beacon remains an age reading rather than a liveness proof — the second half of the reported problem, attempted on this branch and deliberately deferred, with the reasoning recorded rather than dropped.Risk Assessment
✅ Low: The change is additive text on paths that already failed — verdicts, exit statuses and alarms are provably unchanged, the ledger allow-list and double-clean probe are correct against every writer call site, no existing consumer of the typed prefix breaks, and the fix round's three regressions each have a paired-fixture test with recorded mutant output.
Testing
Ran the five targeted suites the change touches (fm-watcher-lock 35 ok, fm-guard-stale-banner 28 ok, fm-turnend-guard 71 ok, plus fm-watch-arm and fm-claude-stop-autoarm as prefix consumers) - all green, no failures, no flakes across repeated runs. Because passing tests are not evidence of the operator experience, I also drove the real scripts over live processes and captured before/after CLI transcripts against the base commit: the attached arm now names the recorded exit instead of the bare unactionable sentence, both banners name the live unbeaten holder pid, and an unprovable liveness reads as an explicit unknown - with the verdict, exit status and typed prefix unchanged in every pair. Two mutation re-checks at this head confirm the round-1 fix genuinely fails without itself (dropping the ledger reason allow-list reproduces "was killed by HUP" for a watcher its own arm TERMed, suite 31 ok / 1 not ok) and that the documented uncovered leg is honestly described (reverting it leaves the suite green at 35 ok, exit 0). This is a CLI/terminal-output change with no rendered UI, HTML or renderer surface, so the reviewer-visible evidence is terminal transcripts of the actual banners and failure lines rather than screenshots. The worktree is clean; all scratch trees were removed.
Evidence: Evidence index for this round
Source: Evidence index for this round
Evidence: Operator transcript BEFORE (base f66be0f)
Source: Operator transcript BEFORE (base f66be0f)
Evidence: Operator transcript AFTER (head 705d59d)
Source: Operator transcript AFTER (head 705d59d)
Evidence: The arm's own signal is not blamed on the watcher (both ways)
Source: The arm's own signal is not blamed on the watcher (both ways)
Evidence: Mutation re-check: ledger reason allow-list
Source: Mutation re-check: ledger reason allow-list
Evidence: Unprovable liveness stays unknown (before/after)
Source: Unprovable liveness stays unknown (before/after)
Evidence: The documented uncovered leg, verified
Source: The documented uncovered leg, verified
Evidence: Turn-end guard stays wait-free
Source: Turn-end guard stays wait-free
Evidence: Side-by-side: what the attached arm tells the operator
Evidence: Side-by-side: the WATCHER DOWN banner
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-watch-arm.sh:299-owner_recorded_exitmatches onwatcher_pid+origin=started+lock_beforebut ignores the ledger'sreasoncolumn ($8), so rows whoseexit_code/signaldescribe the ARM's fate rather than the watcher's get reported as the watcher's own exit. Concrete path: an owner arm that has confirmed its child (socycle_refresh_lock_beforehas run andlock_before=pid:W|identity:Wid) receives SIGHUP;handle_arm_signal(bin/fm-watch-arm.sh:551-560) sends the child SIGTERM, waits, then writeswatcher_pid=W origin=started exit_code=129 signal=HUP reason=arm-interrupted. An arm attached to the same W then fails its cycle, matches that row, and printswatcher pid=W was killed by HUP without delivering a wake— naming a signal the watcher never received and sending the operator looking for a SIGHUP source that does not exist.handling-handoff-failed(bin/fm-watch-arm.sh:645,cycle_log_append 1 none) has the same shape: the arm kills the watcher, but the evidence readswatcher pid=W exited 1. Suggested fix: restrict the awk match to reasons that classify the watcher's own exit (nonzero-exit,signal-exit,unexpected-clean-exit,clean-exit-delivered-wake,actionable-*), or emit distinct wording ("its arm was interrupted (HUP)") for arm-owned rows. This is user-visible operator wording and touches which ledger rows the author intends to treat as authoritative, so it needs the author's call rather than a silent edit.bin/fm-watch-arm.sh:320-cycle_watcher_still_livereturns success on bare pid liveness when the recorded identity is empty ([ -n "$identity" ] || return 0), which is the exact fallback the change's own rationale rules out ("a recycled pid is a live pid ... bare liveness would name an unrelated process to the operator and suppress the recorded exit code"). It is reachable whencycle_begin "$child" started "$(fm_pid_identity "$child" 2>/dev/null || true)"(bin/fm-watch-arm.sh:585) captures an empty identity because the child died before it could be read,cycle_refresh_lock_beforenever ran, and the freed pid is reused beforeunexplained_cycle_evidenceprobes it: the evidence then reportswatcher pid=N is still live and holds this home lockfor an unrelated process and skips the ledger lookup entirely. The window is small, but the safe reading of an unknown identity is "cannot prove still live" (return 1), which only affects the unknown-identity case and preserves every path where the identity is known.bin/fm-watch-arm.sh:344-unexplained_cycle_evidencecallscycle_watcher_still_livetwice for the same pid/identity (lines 338 and 344), re-runningfm_pid_aliveandfm_pid_identity(apsfork on non-Linux hosts) and letting the two branches disagree if the process state changes between them. Computing it once into a local and branching on that is a straight dedup with no behavior change:if cycle_watcher_still_live "$pid" "$clean_identity"; then live=1; else live=0; fi, then test$livein both places.🔧 Fix: filter arm-owned ledger rows and keep unprovable liveness unknown
2 infos still open:
bin/fm-watch-arm.sh:360- Thelive -eq 2branch returns beforeowner_recorded_exitis ever consulted, so an independently-bound recorded exit is discarded in the one case the comment abovecycle_watcher_still_livesays must not lose it ("suppress the recorded exit code that is the only remaining evidence in exactly that case"). The ledger row is bound to the watcher pid, the recorded identity, the lock snapshot andended_at >= cycle_started_at, so reporting it is not a liveness claim and would not conflict with keeping the identity unknown. Reachability is narrow and I could not construct a realistic path: a matching row only exists after the owning arm reaped the child, sofm_pid_alivesucceeding requires PID reuse andfm_pid_identitythen failing on the reusing process. Recording it as a documented tension rather than a defect; no change needed unless the author wants the unknown branch to still print the recorded exit alongside "could not be identified".bin/fm-watch-arm.sh:355-holderis read straight from$WATCH_LOCK/pidand interpolated raw into the typed evidence line at lines 362, 367, 373, 375, 381 and 385, whilelock_snapshot(line 101) runs the same file's contents throughcycle_clean_fieldbefore using it. A lock pid file containing embedded newlines or control characters would therefore split the single-linewatcher: FAILED - ...record into several lines; adapters that scan with/^watcher: FAILED/per line (.opencode/plugins/fm-primary-watch-arm.js:144, :181) still match the first line, so nothing breaks functionally, and a corrupted lock pid file is not a state this code path can produce on its own. Passingholderthroughcycle_clean_fieldwould make it consistent with every other use of that file in this script.🔧 **Test** - 1 issue found → auto-fixed ✅
bin/fm-watch-arm.sh:338- Thecycle_watcher_still_livefix has two legs, and only one ships a failing regression. Reverting the covered leg ([ -n "$current" ] || return 2->return 1) correctly failstest_unprovable_liveness_is_not_reported_as_a_deathwith the exact invented-death wording. But reverting the leg the review finding actually named — the bare-liveness fallback[ -n "$identity" ] || return 2back toreturn 0— leaves the entire fm-watcher-lock suite green (35/35, exit 0). I could not stage a case that fails against it: an empty RECORDED identity only arises at bin/fm-watch-arm.sh:602 when an owning arm's child dies beforefm_pid_identitycan read it andcycle_refresh_lock_beforenever ran, and by the time that arm reaches the evidence path its child pid is already reaped, sofm_pid_alivereturns false and both the fixed and unfixed code take the same branch. The two only diverge if that freed pid is recycled to a live process in between, which is not deterministically stageable. The fix looks correct and is defense-in-depth; the author should decide whether an unprovable-by-test branch is acceptable under the stated "every behavioural fix ships a regression that genuinely FAILS" standard, or whether a seam should be added to reach it.bash tests/fm-watcher-lock.test.sh— 35 ok, exit 0 (includes the four new cases)bash tests/fm-guard-stale-banner.test.sh— 20 ok, exit 0 (includestest_persistent_stale_beacon_banner_separates_all_three_causes)bash tests/fm-turnend-guard.test.sh— 71 ok, exit 0 (includestest_hook_block_banner_names_a_live_unbeaten_holder)bash tests/fm-watch-arm.test.sh— 14 ok, exit 0bash tests/fm-claude-stop-autoarm.test.sh— 39 ok, exit 0Manual before/after operator transcript: drove realbin/fm-watch-arm.sh(owning arm + attached arm, watcher TERMed with a fresh beacon) against both basef66be0fand this branch —demo-arm-evidence.shManual transcript: same two-arm fixture with the OWNING ARM HUPed, showing thereason=arm-interruptedledger row is not read back as the watcher's own signalManual before/after banner transcript: realbin/fm-guard.shandbin/fm-turnend-guard.shagainst a live identity-matched holder with a stale beacon, plus unheld-lock / no-holder controls, recording exit statuses —demo-guard-banners.shMutation check (scratch copy of HEAD, each mutantbash -nvalidated first): removing the evidence clause failstest_attached_arm_reports_the_owner_recorded_exitMutation check: single-truncating the ledger probe failstest_owner_recorded_exit_survives_a_long_identityMutation check: dropping thereason=allow-list failstest_attached_arm_does_not_blame_the_watcher_for_its_arms_signalwith the exact 'was killed by HUP' misattributionMutation check: collapsing unknown liveness to dead ([ -n "$current" ] || return 1) failstest_unprovable_liveness_is_not_reported_as_a_deathMutation check: restoring the bare-liveness fallback ([ -n "$identity" ] || return 0) — full fm-watcher-lock suite still green, leg uncovered🔧 Fix: record the unproven empty-identity liveness leg
✅ Re-checked - no issues remain.
bash tests/fm-watcher-lock.test.sh- 35 ok, 0 not ok, exit 0 (carries all four new cycle-evidence regressions)bash tests/fm-guard-stale-banner.test.sh- 28 ok, 0 not ok, exit 0bash tests/fm-turnend-guard.test.sh- 71 ok, 0 not ok, exit 0bash tests/fm-watch-arm.test.sh- 14 ok, exit 0 (consumer that prefix-matches the typed cycle-end failure)bash tests/fm-claude-stop-autoarm.test.sh- 39 ok, exit 0 (same prefix consumer)Manual before/after operator transcript: guard banner, Stop-hook block banner and attached-arm cycle end driven over real processes against base f66be0f and against head, with negative controls (dead holder, unheld lock) in eachManual transcript:kill -HUPthe OWNING arm so it writes an arm-interrupted row under the watcher's pid, run both with and without the reason allow-list, compare what the attached arm tells the operatorManual transcript: conditionalpsshadow blinds one attached arm's identity read while its watcher is proven still running viakill -0, base vs headMutation re-check at head: reason allow-list removed from bin/fm-watch-arm.sh:309, full fm-watcher-lock suite -> 31 ok, 1 not ok, exit 1, failing casetest_attached_arm_does_not_blame_the_watcher_for_its_arms_signalMutation re-check at head:[ -n "$identity" ] || return 2reverted toreturn 0, full fm-watcher-lock suite -> 35 ok, 0 not ok, exit 0, confirming the documented uncovered legLatency check:fm-turnend-guard.shwall time on the same live-unbeaten-holder fixture, base vs head interleaved x5🔧 **Document** - 2 issues found → auto-fixed ✅
docs/verification/supervision.md:582- The "Beacon freshness under host suspend - attempted and deferred" section records an abandoned approach and its four review rounds. docs/documentation-audiences.md states that failed hypotheses and task chronology stay in private task reports or PR evidence by default, so this sits at the edge of the maintainer-verification audience. I left it as written because the user intent explicitly requires the deferral reasoning to be recorded in this file, and the section does distil a durable design lesson rather than a delivery transcript. Flagging it as a judgment call the author may want to confirm with maintainers rather than a defect.docs/watcher-continuity.md:130- The "Active limits and verification" section still lists no limit for the half of issue watcher: cycle ends without an actionable reason under the auto-detected herdr backend #1375 this change deliberately does not fix: state/.last-watcher-beat can read fresh while supervision is already dead. That limitation predates this change, so it was not made stale here and is out of scope, but once the PR lands with the deferral stated publicly, a one-line known-limit entry there pointing at verification/supervision.md would be the natural follow-up. Proposing it rather than editing an unrelated section in this change.🔧 Fix: state the deferred beacon limit and drop internal round vocabulary
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.