fix(bin): require a live away pusher before an away turn can end - #34
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix the away-mode turn-end guard so the mere presence of state/.afk never proves wake delivery coverage. Require a live, identity-matched away pusher using the supervise-daemon pid and portable lock identity, mirroring the existing watcher-health shape. Keep scope to this guard only: do not change the away daemon, watcher, or delivery architecture. Regress both directions: a dead pusher with queued wakes must block turn end, while a healthy identity-matched pusher must permit it. Review max-defer deliberately and state in the pull request body what an operator sees if the daemon dies at 3am: the next attempted turn end forces one bounded maintenance continuation; the loop guard then permits that turn to stop, later independent turns alert again, queued wakes remain durable, and the daemon-owned max-defer alarm cannot run after the daemon dies, so there is no autonomous endless forced-continuation loop.
What Changed
bin/fm-turnend-guard.shno longer treats the mere presence ofstate/.afkas proof of wake delivery. The away branch now calls a newfm_pusher_healthyinbin/fm-wake-lib.sh, which requiresstate/.supervise-daemon.pidto name a live pid that matches the portable lock'spidand recordedpid-identity— the same shape as the existing watcher-health predicate, and exactly the three filesbin/fm-supervise-daemon.shpublishes at startup. Watcher health stays independently mandatory; the away daemon, watcher, and delivery architecture are untouched.FM_SUP_QUEUE_PENDING) even with nostate/*.metarecord left, passes--queue-pendingto the repair-line renderer, and branches its banner on away mode: away turns are told to restore away delivery and are never told to drain (draining would consume records the daemon's cursor has not read), while session turns keep the drain-and-restore wording. The away repair line inbin/fm-supervision-instructions.shis now harness-aware —start-nativeplusFM_AFK_STATE_PREPARED=1 bin/fm-afk-start.shfor harnesses with a native tracked-background tool, the terminal-backedbin/fm-afk-launch.sh startotherwise, with an in-place warning that thestoprollback exits away mode by clearingstate/.afk.tests/fm-turnend-guard.test.sh), withtests/fm-daemon.test.shpinningfm_pusher_healthyagainst the real daemon's published pidfile and lock rather than a fixture, andtests/fm-supervision-instructions.test.shcovering the harness-aware repair text.docs/turnend-guard.md,docs/architecture.md, anddocs/scripts.mdare updated to match.Max-defer was reviewed deliberately: it is daemon-owned, so it cannot run once the daemon is dead and is not counted as substitute coverage. If the away daemon dies at 3am, the operator sees the next attempted turn end forced into one bounded maintenance continuation naming the daemon relaunch; the hook loop guard then permits that same turn to stop, so there is no autonomous endless forced-continuation loop. Later independent turns alert again until delivery is repaired, and queued wakes stay durable in
state/.wake-queuethroughout.Risk Assessment
✅ Low: The final commit is a two-branch wording fix plus strengthened regressions that closes the last open finding, leaving a tightly scoped change whose every path I traced against the away no-drain contract with no remaining inconsistencies.
Testing
Ran the three targeted suites covering the guard, the away repair-line renderer, and the daemon predicate (all pass, including the new both-directions regressions), then drove the real end-user path: a live
bin/fm-supervise-daemon.shaway pusher in a hermetic away home, with the Stop hook invoked exactly as a harness invokes it. The captured terminal transcript shows a silent, permitted turn end while the pusher is alive; after the daemon is SIGKILLed (stale pidfile andstate/.afkdeliberately left behind) the next turn end is blocked with the away-specific banner and the concrete daemon-relaunch repair; the same turn'sstop_hook_active=trueretry is permitted, so there is no endless forced-continuation loop; a later turn alerts again; the queued wake is unchanged throughout; and the daemon process is confirmed gone, so its max-defer alarm cannot substitute for coverage. No browser screenshot was possible (no Chrome executable in this environment), but the end-user surface here is a terminal banner, captured verbatim as a transcript and also rendered as a styled HTML terminal view for reviewers.Evidence: Operator terminal transcript: away guard with a real daemon killed at 3am
=== 3. Turn end while the away pusher is ALIVE (healthy, identity-matched) === exit status : 0 (0 = turn allowed to end) hook output : <silent> === 4. 3am: the away daemon dies (SIGKILL, no cleanup - pidfile and lock stay behind) === pidfile still on disk : 1293844 (pid is dead) state/.afk still present : yes === 5. Next attempted turn end -> ONE forced maintenance continuation === exit status : 2 (2 = turn blocked) ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ● TURN WOULD END BLIND - SUPERVISION IS INCOMPLETE ● Away wake delivery missing: no live identity-matched pusher holds the supervise-daemon lock. ● Delivery repair: Away mode owns wake delivery and no live identity-matched away daemon is reading the durable queue: prepare the lifecycle with bin/fm-afk-launch.sh start-native, then run FM_AFK_STATE_PREPARED=1 bin/fm-afk-start.sh as its own Claude Code background task (never shell &); if that native launch fails, roll the preparation back with bin/fm-afk-launch.sh stop, which EXITS away mode by clearing state/.afk and therefore must be followed immediately by a fresh away entry so the captain is not left unattended without it. Then confirm state/.supervise-daemon.pid names a live pid matching the daemon lock identity; do not arm a session delivery wait instead. ● This forced continuation is internal maintenance; after restoring away delivery, end silently unless a queued wake is captain-relevant under AGENTS.md section 9. ●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ === 6. Same turn, the forced continuation tries to stop again (stop_hook_active=true) === exit status : 0 (0 = loop guard permits this turn to stop; no endless loop) hook output : <silent> === 7. A later, independent turn alerts again until delivery is repaired === exit status : 2 (2 = alerts again) banner headline : ● Away wake delivery missing: no live identity-matched pusher holds the supervise-daemon lock. === 8. Queued wake is still durable after all of it === 1^I1^Isignal^Itask1.status^Idone: refactor landed === 9. The max-defer alarm is daemon-owned, so it cannot run after the daemon dies === away daemon pid 1293844: gone - its loop, and the max-defer alarm inside it, are not runningEvidence: Rendered terminal view of the same scenario (HTML)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (4) ✅
tests/fm-turnend-guard.test.sh:277- Both new away-mode cases assert only against hand-written fixtures (record_pusher_lockwrites.supervise-daemon.pid,.lock/pid, and.lock/pid-identityitself), so nothing pins the predicate to whatbin/fm-supervise-daemon.shactually publishes. The guard now fails closed on three separate daemon-internal writes:echo "$$" > "$PIDFILE"(fm-supervise-daemon.sh:1314, unchecked) and the best-effortfm_pid_identity ... > "$LOCK/pid-identity"(:1315,|| true, which leaves an EMPTY file when identity resolution fails — andfm_pusher_lock_matches_pidrejects an empty identity). If any of those drift or fail, a perfectly healthy away daemon reads as dead and every away turn end forces a maintenance continuation, whilefm-afk-start.shkeeps reporting "daemon already running" (itsdaemon_pid_matchestolerates an empty identity via the ps-command fallback), so the printed repair is unsatisfiable.tests/fm-daemon.test.shalready starts the real daemon throughbin/fm-afk-start.shfar enough to publish the pidfile+lock; add an assertion there (or in the lifecycle e2e) thatfm_pusher_healthyreturns 0 against that real published state.bin/fm-guard.sh:176- The pull-based guard still does[ -e "$STATE/.afk" ] && delivery_armed=true— the exact false-coverage assumption this change removes from the turn-end guard. With a dead away pusher the two guards now disagree:fm-guard.shstays silent about missing delivery (and even passes--afk 0to the repair-line renderer at :229) whilefm-turnend-guard.shblocks the turn. The stated intent scopes this change to the turn-end guard only, so this is flagged as a deliberate follow-up rather than something to fix here — confirm whetherfm-guard.shshould adoptfm_pusher_healthyin a separate change.bin/fm-turnend-guard.sh:86- The new away-pusher check sits behind the unchanged in-flight gate ([ "$FM_SUP_IN_FLIGHT" -gt 0 ] || exit 0), so "a dead pusher with queued wakes blocks turn end" only holds whilestate/*.metarecords exist. A dead daemon plus a non-emptystate/.wake-queuebut zero in-flight tasks (e.g. terminal wakes queued after the metas were cleaned up) still ends the turn silently, even thoughfm_supervision_statusalready computesFM_SUP_QUEUE_PENDING. Low impact since nothing is being supervised at that point, but worth confirming this matches the intended contract.bin/fm-turnend-guard.sh:120- This change makes the away branch offm-supervision-instructions.sh --repair-linereachable for the first time (previouslydelivery_armedwas unconditionally 1 under.afk, andfm-guard.shalways passes--afk 0). That line reads "Away mode owns wake delivery; load /afk and ensure the daemon is reading the durable queue instead of arming a session delivery wait" — it never names a concrete restart (bin/fm-afk-launch.sh start), unlike every per-harness branch which names an exact command, and its "instead of arming a session delivery wait" phrasing describes the old contract rather than the dead-daemon case the agent is actually facing at 3am.🔧 Fix: activate turn-end guard on queued wakes; name away restart
4 issues (1 warning, 3 infos) still open:
bin/fm-supervision-instructions.sh:113- The new away repair line tells every harness to "restart it with bin/fm-afk-launch.sh start", but.agents/skills/afk/SKILL.mdreserves that path for a harness WITHOUT a native background tool (e.g. pi): a harness WITH one (claude, codex background bash, grok — which includes the harness the rest of this guard's repair lines target) must instead runbin/fm-afk-launch.sh start-nativefollowed byFM_AFK_STATE_PREPARED=1 bin/fm-afk-start.shthrough that native tool, described there as "a deliberate no-separate-terminal exception". Following the printed line on a claude primary manufactures the detached tmux session / non-visible herdr workspace the native path exists to avoid, and it fails outright when the primary is not running under a discoverable tmux/herdr pane (fm_afk_launch_startaborts oncould not resolve the captain supervisor pane). Unlike every other repair branch, the away branch returns before the per-harnesscase "$HARNESS"block, so it cannot be harness-aware. Flagged as ask-user because the captain's round-1 decision named this exact command — confirm whether the line should branch on$HARNESS(start-native + fm-afk-start.sh for native-background harnesses,fm-afk-launch.sh startotherwise) or stay as one command.bin/fm-turnend-guard.sh:106-protected_desconly reaches the daemon-down branch (:117), and the guard still calls the repair-line renderer without--queue-pending. In the activation path this commit adds — zerostate/*.meta, queue pending, watcher healthy, no armed stub — the banner prints only "Wake delivery missing: no identity-matched delivery stub is armed for this session" plus the plain re-arm line, so nothing tells the agent that a queued wake is what kept the guard active.bin/fm-guard.sh:229-232already passes--queue-pending "$queue_arg"to the same renderer, which prefixes "After draining queued wakes, ". Outcome is still correct (arming the stub does deliver the pending records), so this is a messaging gap rather than a wedge.bin/fm-turnend-guard.sh:88- Consequence worth recording: the away daemon readsstate/.wake-queuethrough its own cursor and never drains it (docs/architecture.md,.agents/skills/afk/SKILL.mdstep 3), andbin/fm-watch.sh:1450enqueues a heartbeat record every cadence while.afkis present. SoFM_SUP_QUEUE_PENDINGis effectively true for the whole away session and stays true until a post-returnfm-wake-drain.sh/fm-afk-return.sh, which means the guard is now active at every away turn end regardless of in-flight metas — activation is no longer a proxy for "work in flight". This is benign (a healthy watcher plus a healthy pusher still exits 0, and the queue is emptied by the documented return flow), butdocs/turnend-guard.md:30describes the queue trigger generically without noting that away homes sit permanently in that state.tests/fm-daemon.test.sh:123- The new real-daemon case ends withkill -TERM "$pid"followed by a barewait "$pid". If the daemon misses or ignores the signal (itscleanuptrap runsescalate_flushand the wedge-alarm notifier stop before exiting), the suite hangs indefinitely instead of failing, and thedead_rcassertion never runs.tests/wake-helpers.shalready provides the boundedwait_for_exit "$pid" 50used elsewhere in this suite; use it here.🔧 Fix: make away repair harness-aware; name drain; bound daemon wait
1 info still open:
bin/fm-supervision-instructions.sh:116- The claude/grok away repair ends with "rolling the preparation back with bin/fm-afk-launch.sh stop if that native launch fails"..agents/skills/afk/SKILL.mdstates that rollback for a fresh ENTRY into away mode, where reverting to not-away is the right outcome; this line is printed in the opposite context — the daemon died mid-away-session and the captain is still gone.fm_afk_launch_stopclearsstate/.afkas its last step, so following the fallback silently exits away mode at 3am: the guard's away branch stops applying, the session falls back to arming a normal delivery stub, and the batched-escalation contract the captain asked for is abandoned with nothing instructing a re-entry. The failure branch is reachable —fm_afk_launch_start_nativereturns 1 whenfm_afk_launch_reconcilecannot confirm teardown of a previous herdr/tmux daemon terminal, which is a plausible state after an unclean daemon death. Consider qualifying the clause (note thatstopexits away mode and must be followed by a fresh away entry) or dropping it from the recovery-context line.🔧 Fix: warn that away rollback exits away mode
1 warning still open:
bin/fm-turnend-guard.sh:134- The banner's unconditional closing line — reworded by this branch — reads "after draining and restoring delivery, end silently", and it is printed in away mode too, directly under the away repair line that just told the session the daemon owns the durable queue. That contradicts the contract this branch establishes:docs/turnend-guard.mdstates "the away daemon reads the durable queue through its own cursor and the session must not drain it",tests/fm-turnend-guard.test.shasserts on the whole hook output that the away path never tells the session to drain, andbin/fm-afk-return.showns the drain as a return-time action. The regression only passes on a substring technicality: it matches "After draining queued wakes, ", not this line's "after draining". An away agent that follows the closing line at 3am runsfm-wake-drain.sh, which emptiesstate/.wake-queueand removes records the daemon's seq cursor has not read yet — losing exactly the away wakes this guard exists to protect. Concrete fix: branch the line on$afk(away: "after restoring away delivery, end silently…"; otherwise keep today's wording). No test pins the current phrasing, so the change is contained.🔧 Fix: stop telling away turns to drain the queue
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-turnend-guard.test.sh(includes new cases: dead away pusher with queued wakes blocks, queued wakes with nostate/*.metakeep the guard active, session-mode queued-wake repair names the drain, healthy identity-matched pusher permits the turn)bash tests/fm-supervision-instructions.test.sh(harness-aware away repair line, native vs terminal launcher paths,stopexits-away-mode caveat, no drain wording under--queue-pending 1)bash tests/fm-daemon.test.sh(97 cases, includingtest_pusher_healthy_accepts_real_daemon_publicationpinningfm_pusher_healthyto the daemon's real pidfile/lock publication)Manual end-to-end operator scenario: hermetic away home with the shippedbin/, realbin/fm-supervise-daemon.shstarted as the away pusher, live identity-matched.watch.lockwatcher holder, real record instate/.wake-queue, thenprintf '{"stop_hook_active":false}' | FM_HOME=<home> bash bin/fm-turnend-guard.shinvoked as a harness Stop hook — before the kill (exit 0, silent), afterkill -9of the daemon (exit 2 + away banner), withstop_hook_active=truein the same turn (exit 0), and on a later independent turn (exit 2 again)Verified queue durability by diffingstate/.wake-queuecontent before and after all guard invocations, and confirmed the daemon pid is gone so the daemon-owned max-defer alarm cannot rundocs/turnend-guard.md:48- docs/turnend-guard.md:48-50 restates the /afk skill's harness-specific relaunch procedure verbatim (start-native, FM_AFK_STATE_PREPARED=1 fm-afk-start.sh, fm-afk-launch.sh stop rollback, terminal-backed start). The /afk skill (.agents/skills/afk/SKILL.md) is the owner of that procedure, so the two copies can drift if the away entry path changes. I left the copy in place because it documents what the guard's own repair line emits, which is guard-owned behavior and the explicit point of this change; reducing it to a pointer is a judgment call for the author.docs/watcher-continuity.md:28- docs/watcher-continuity.md:28 says "The existing turn-end guard implementation and adapters are unchanged." That sentence is a scoped claim about the watcher-continuity change (adapters are indeed still unchanged), but read as current state it now conflicts with this change touching bin/fm-turnend-guard.sh. I did not edit it, since rewriting another change's narrative is outside this change's scope; if the author wants it unambiguous, it should say "unchanged by the continuity work".✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.