fix(bin): bound the away-mode busy guard and log delivery outcomes - #3090
Open
NicholasACTran wants to merge 12 commits into
Open
fix(bin): bound the away-mode busy guard and log delivery outcomes#3090NicholasACTran wants to merge 12 commits into
NicholasACTran wants to merge 12 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (9): Last reviewed commit: "no-mistakes(document): correct FM_BUSY_G..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
NicholasACTran
added a commit
to NicholasACTran/firstmate
that referenced
this pull request
Aug 26, 2026
…rride Two findings from the automated review on PR kunchenguid#3090, both real: P1: the busy-guard-since marker persisted across away sessions, so a marker left over from a prior run could read as already-overdue on the very first busy+empty observation of a new session and escape immediately into an active supervisor turn - the exact "guard state outlives the run it measured" failure this ticket exists to prevent. inject_msg now corroborates the marker against state/.afk's own stamped session-start epoch (afk_enter already records this) and treats a marker older than the current session, or unreadable/non-numeric, as no marker at all. P2: FM_BUSY_GUARD_ESCAPE_SECS was validated only with -gt 0, so a blank, non-numeric, or negative override silently and permanently disabled the escape (and a non-numeric value additionally threw a bash integer-expression error on every attempt). It's now validated explicitly: zero disables, a positive integer sets the interval, and anything else is refused with a loud log line and the default applied.
NicholasACTran
added a commit
to NicholasACTran/firstmate
that referenced
this pull request
Aug 26, 2026
…ading zeros Re-review finding on PR kunchenguid#3090: the P2 validator accepted any digit-only override and used it as-is, so a leading-zero value like 010 could be misread by C-style octal arithmetic as eight instead of ten, and 008/009 are not even valid octal. Force base 10 explicitly (`10#`) when parsing the validated digit string, so the effective interval always equals the decimal value the operator wrote. Adds a regression test asserting the effective interval for 007/008/009/010 and a plain positive, not just that validation accepted the string.
NicholasACTran
added a commit
to NicholasACTran/firstmate
that referenced
this pull request
Aug 26, 2026
… once Captain's ruling on PR kunchenguid#3090 (option B): rather than patch a fifth finding on the same durable-marker shape, redesign it. The previous findings (a marker that outlives its away session, a config value trusted without validating how it's read) both stem from persisting the escape clock and its configuration to disk and re-deriving them on every attempt. - BUSY_GUARD_SINCE_EPOCH is now a plain in-process variable, not a state/.subsuper-busy-guard-since file. inject_msg is only ever called from housekeeping and the daemon's own shutdown trap, both inside fm_super_main's single long-lived process, so a restart (a new process) gets a fresh clock automatically - no corroboration against state/.afk is needed because there is no durable value to inherit. - FM_BUSY_GUARD_ESCAPE_SECS is resolved exactly once, at daemon start, by the new resolve_busy_guard_escape_secs into BUSY_GUARD_ESCAPE_SECS_RESOLVED: 0 disables, a positive integer up to the new BUSY_GUARD_ESCAPE_SECS_MAX (86400s) sets the interval, anything else is refused once with a loud log and the default applied. The clamp closes the overflow risk in the forced-decimal (10#) parse. - The marker file and its .afk-corroboration logic are removed entirely - nothing else in the repo read it. Regression tests rewritten to match: the restart case is now a fresh subshell (a fresh process) that must not inherit a prior subshell's elapsed time; the config cases call resolve_busy_guard_escape_secs directly and assert the resolved value, including the clamp boundary.
Owner
|
Speaking as Kun's firstmate: Reviewed HEAD Class: corrective. Away-mode's max-defer retry re-enters the same busy guard, so a false-positive busy verdict (empty composer) produced unbounded silence. This bounds that disagreement to This does not close #1956 or #3087. Those are watcher stale-hash / over-escalation bugs. This PR bounds captain-pane injection when the busy guard is stuck. No Sibling of #3089 (cause: in-pane claude+herdr launch). Complementary, not conflicting. This PR does not touch the launch path. VISION (per rule):
What is not cleared:
Security: no. This is waiting on the author, not the captain: matching attestation for this HEAD, and a response to the two remaining Greptile P1s. Merge-eligible: NO. Captain-flag NOW: NO. |
Owner
|
Speaking as Kun's firstmate: Reviewed the whole thread, every file in the full diff ( VISION (per rule, inspected evidence):
First-time fork CI: after that full-diff review I approved workflow runs Not merging. No |
NicholasACTran
added a commit
to NicholasACTran/firstmate
that referenced
this pull request
Aug 26, 2026
…ithmetic Fifth review finding on PR kunchenguid#3090: the clamp checked $((10#$raw)) AFTER converting the raw value, but bash arithmetic silently wraps on overflow rather than erroring, so a sufficiently large digit-only value could wrap back into an in-range number and defeat the clamp entirely - the same class of bug as the earlier leading-zero misparse, just on the other end of the range. resolve_busy_guard_escape_secs now strips leading zeros with a pure regex match and compares by digit count (falling back to a same-length lexicographic compare, which equals numeric order for leading-zero-free digit strings) before any arithmetic ever touches the value. A value with more digits than the clamp, or tied on digits but lexicographically past it, is rejected on that string comparison alone. Added a regression test with a 38-digit input, asserting the resolved value is exactly the default rather than merely "different from the input" - a wrapped garbage value would also differ, so the weaker assertion could pass on the bug.
NicholasACTran
added a commit
to NicholasACTran/firstmate
that referenced
this pull request
Aug 26, 2026
…e age Sixth review finding on PR kunchenguid#3090: the in-process escape clock (the prior redesign) reset on every daemon restart, and the daemon has a crash-loop guard that makes restarts a real, expected event - so a persistent busy false-positive combined with repeated restarts could defer delivery indefinitely, the exact failure this ticket exists to end. inject_msg no longer keeps a clock of its own. It measures how long the current escalation has gone undelivered via the existing durable state/.subsuper-escalations.since (already maintained by escalate_add/escalate_flush for FM_MAX_DEFER_SECS): created when an escalation lands in an empty buffer, removed the moment delivery succeeds. That value is tied to live undelivered work rather than to the daemon process's lifetime, so it is not stale data on a restart - it is an honest measurement of how long this exact escalation has genuinely gone undelivered. escalate_flush's own empty-buffer guard means inject_msg is only ever reached here with the file present. Keeps everything else from the round-4/5 redesign: config resolved once at daemon start with the overflow-safe clamp, the old per-busy-guard marker file gone, and the escape still firing only on an exact composer 'empty' read. Regression test rewritten to assert the property directly: two entirely separate subshells sharing no bash state, only the durable file, where the second must still see the same escalation as overdue. This fails against the prior in-process-clock version.
NicholasACTran
changed the title
The 2026-08-26 investigation found that away mode's max-defer escape retries the exact busy guard that just failed, so a guard false positive (the daemon's own in-pane launch method making an idle claude pane read permanently busy) produced an unbounded 95-minute silence instead of a bounded one. inject_msg now tracks how long a busy verdict has disagreed with a provably-empty composer and delivers anyway once FM_BUSY_GUARD_ESCAPE_SECS elapses, bounding any future guard false positive to that window. A successful delivery previously logged nothing, so a healthy away session and a wedged one looked identical in the daemon log; a confirmed submit now logs a line and writes a durable state/.subsuper-last-delivery timestamp. The busy-guard deferral log line now also names which branch decided (native agent-state vs the rendered-pane regex fallback), so the next occurrence is explainable by grep instead of an investigation.
…rride Two findings from the automated review on PR kunchenguid#3090, both real: P1: the busy-guard-since marker persisted across away sessions, so a marker left over from a prior run could read as already-overdue on the very first busy+empty observation of a new session and escape immediately into an active supervisor turn - the exact "guard state outlives the run it measured" failure this ticket exists to prevent. inject_msg now corroborates the marker against state/.afk's own stamped session-start epoch (afk_enter already records this) and treats a marker older than the current session, or unreadable/non-numeric, as no marker at all. P2: FM_BUSY_GUARD_ESCAPE_SECS was validated only with -gt 0, so a blank, non-numeric, or negative override silently and permanently disabled the escape (and a non-numeric value additionally threw a bash integer-expression error on every attempt). It's now validated explicitly: zero disables, a positive integer sets the interval, and anything else is refused with a loud log line and the default applied.
…ading zeros Re-review finding on PR kunchenguid#3090: the P2 validator accepted any digit-only override and used it as-is, so a leading-zero value like 010 could be misread by C-style octal arithmetic as eight instead of ten, and 008/009 are not even valid octal. Force base 10 explicitly (`10#`) when parsing the validated digit string, so the effective interval always equals the decimal value the operator wrote. Adds a regression test asserting the effective interval for 007/008/009/010 and a plain positive, not just that validation accepted the string.
… once Captain's ruling on PR kunchenguid#3090 (option B): rather than patch a fifth finding on the same durable-marker shape, redesign it. The previous findings (a marker that outlives its away session, a config value trusted without validating how it's read) both stem from persisting the escape clock and its configuration to disk and re-deriving them on every attempt. - BUSY_GUARD_SINCE_EPOCH is now a plain in-process variable, not a state/.subsuper-busy-guard-since file. inject_msg is only ever called from housekeeping and the daemon's own shutdown trap, both inside fm_super_main's single long-lived process, so a restart (a new process) gets a fresh clock automatically - no corroboration against state/.afk is needed because there is no durable value to inherit. - FM_BUSY_GUARD_ESCAPE_SECS is resolved exactly once, at daemon start, by the new resolve_busy_guard_escape_secs into BUSY_GUARD_ESCAPE_SECS_RESOLVED: 0 disables, a positive integer up to the new BUSY_GUARD_ESCAPE_SECS_MAX (86400s) sets the interval, anything else is refused once with a loud log and the default applied. The clamp closes the overflow risk in the forced-decimal (10#) parse. - The marker file and its .afk-corroboration logic are removed entirely - nothing else in the repo read it. Regression tests rewritten to match: the restart case is now a fresh subshell (a fresh process) that must not inherit a prior subshell's elapsed time; the config cases call resolve_busy_guard_escape_secs directly and assert the resolved value, including the clamp boundary.
…ithmetic Fifth review finding on PR kunchenguid#3090: the clamp checked $((10#$raw)) AFTER converting the raw value, but bash arithmetic silently wraps on overflow rather than erroring, so a sufficiently large digit-only value could wrap back into an in-range number and defeat the clamp entirely - the same class of bug as the earlier leading-zero misparse, just on the other end of the range. resolve_busy_guard_escape_secs now strips leading zeros with a pure regex match and compares by digit count (falling back to a same-length lexicographic compare, which equals numeric order for leading-zero-free digit strings) before any arithmetic ever touches the value. A value with more digits than the clamp, or tied on digits but lexicographically past it, is rejected on that string comparison alone. Added a regression test with a 38-digit input, asserting the resolved value is exactly the default rather than merely "different from the input" - a wrapped garbage value would also differ, so the weaker assertion could pass on the bug.
…e age Sixth review finding on PR kunchenguid#3090: the in-process escape clock (the prior redesign) reset on every daemon restart, and the daemon has a crash-loop guard that makes restarts a real, expected event - so a persistent busy false-positive combined with repeated restarts could defer delivery indefinitely, the exact failure this ticket exists to end. inject_msg no longer keeps a clock of its own. It measures how long the current escalation has gone undelivered via the existing durable state/.subsuper-escalations.since (already maintained by escalate_add/escalate_flush for FM_MAX_DEFER_SECS): created when an escalation lands in an empty buffer, removed the moment delivery succeeds. That value is tied to live undelivered work rather than to the daemon process's lifetime, so it is not stale data on a restart - it is an honest measurement of how long this exact escalation has genuinely gone undelivered. escalate_flush's own empty-buffer guard means inject_msg is only ever reached here with the file present. Keeps everything else from the round-4/5 redesign: config resolved once at daemon start with the overflow-safe clamp, the old per-busy-guard marker file gone, and the escape still firing only on an exact composer 'empty' read. Regression test rewritten to assert the property directly: two entirely separate subshells sharing no bash state, only the durable file, where the second must still see the same escalation as overdue. This fails against the prior in-process-clock version.
The busy+empty streak marker's wall-clock age counted time the daemon never observed the pane at all: the main loop skips inject_msg entirely during the pane-gone backoff, a crash backoff, and a restart between flushes, so the first busy+empty observation after a threshold-length gap escaped immediately and typed into a pane that may have just genuinely started a turn. The marker now records observed seconds in its contents and the last observation time in its mtime, so each delivery attempt credits at most one poll interval. A corrupted or oversized value fails closed at 0 without entering arithmetic.
…scape Each observed busy+empty tick credited at most HOUSEKEEPING_TICK_DEFAULT (15s) toward FM_BUSY_GUARD_ESCAPE_SECS, regardless of the cadence the daemon actually retries a buffered escalation at. On a slower cadence that multiplied the configured window by tick/15 - a 60s FM_HOUSEKEEPING_TICK stretched the documented 300s bound into ~1200s of away-mode silence, which is the unbounded-silence failure this escape exists to cap. BUSY_EMPTY_STREAK_STEP_MAX is now resolved once at daemon start from the effective FM_HOUSEKEEPING_TICK. A zero, negative, non-numeric, or absurdly large cadence cannot describe a real poll interval, so it is refused with one log line and the default applied - never a 0s credit (an escape that never fires) or a whole day at once. The decimal-safe, overflow-safe parse the escape resolver already used is factored into _resolve_secs_override and shared by both.
…ehavior in configuration docs
NicholasACTran
force-pushed
the
fm/firstmate-afk-escape-and-delivery-logging
branch
from
August 26, 2026 09:07
ca51c5c to
bdec393
Compare
NicholasACTran
changed the title
Owner
|
Speaking as Kun's firstmate: Re-triage of newer activity since stamp New HEAD Class: corrective. The busy-guard escape is now a durable observed-seconds streak ( This does not close #1956 or #3087. No Last-pass blockers, inspected on this HEAD:
VISION (per rule, inspected evidence):
First-time fork CI: last pass approved runs for old HEAD Not merging. Attestation now matches, remaining P1s look answered, but forge CI on this HEAD had not run yet, and the author asked the cause-fix (#3089 / later #3100) to land with this bounding fix. Still a sibling hold. Not a captain-decision hold. Merge-eligible: NO. Captain-flag NOW: NO. |
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
2 participants
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
Bound the away-mode busy guard so a false-positive (the daemon misreading an idle claude pane as busy) can never cause unbounded silence again, and make delivery success/failure explainable from the log alone. Background: a 2026-08-26 investigation (data/firstmate-afk-daemon-wedged-investigation/report.md) found away mode delivered nothing for 95 minutes because the existing max-defer retry re-enters the exact same busy guard that is failing. This PR is the BOUNDING fix (sibling of firstmate-afk-launch-fix, which is the CAUSE fix for the launch-path bug itself); both are needed together and neither alone makes away mode safe.
This branch already carries a fully validated fix through seven review rounds (all resolved, committed): the escape anchors to a durable busy+empty streak marker (state/.subsuper-busy-empty-streak-since) measuring exactly the condition being escaped - a continuous run of (busy verdict AND confirmed-empty composer) - not an in-process clock (resets on daemon restart, rejected) and not the buffered escalation's own undelivered age (measures the wrong quantity, can fire with zero seconds of real disagreement, rejected). The marker is created on first observation, reset the instant the condition breaks or on any early-return path that could not observe it, removed when the escape fires, and cleared only on a genuine fresh away-session entry (fm_afk_clear_stale_artifacts) - never on a refresh - and cleaned up consistently across bin/fm-afk-start.sh, bin/fm-afk-launch.sh's three artifact lists, and bin/fm-afk-return.sh. FM_BUSY_GUARD_ESCAPE_SECS is resolved once at daemon start with a forced-decimal parse (immune to bash's leading-zero-as-octal reading) and an overflow-safe upper clamp validated by string comparison before any arithmetic touches the value.
Skipping ci on this run: this repo blocks first-time GitHub Actions runs from a fork at action_required pending maintainer approval, so CI cannot start or report on this PR - waiting on it or looping trying to fix it is waiting on something that cannot happen. The captain explicitly ruled the finish line for this delivery is a correct, open PR carrying the no-mistakes pipeline attestation - not a green CI check - given that constraint.
What Changed
inject_msginbin/fm-supervise-daemon.shgains a bounded busy-guard escape: when a busy verdict disagrees with an affirmativelyemptycomposer forFM_BUSY_GUARD_ESCAPE_SECS(default 300,0disables), it delivers instead of deferring again. Continuity is tracked by the durablestate/.subsuper-busy-empty-streak-sincemarker, whose contents hold seconds observed and whose mtime bounds each tick's credit to one poll interval, so time the daemon never observed the pane (pane gone, crash backoff, restart between flushes) cannot buy the escape; the marker is reset on any non-emptycomposer, any non-busy verdict, any early return that never observed the pane, and once the escape fires.FM_BUSY_GUARD_ESCAPE_SECSand the per-tick cap derived fromFM_HOUSEKEEPING_TICKare resolved once at daemon start through a shared_resolve_secs_overridehelper that strips leading zeros as decimal and enforces the 86400s clamp by string comparison, never by arithmetic that could overflow.inject deliveredline and stampsstate/.subsuper-last-delivery, and busy deferral lines name which branch produced the verdict (native agent-state vs the rendered-pane busy-regex fallback) via a newPANE_BUSY_LAST_SOURCE. The escape path reuses the already-confirmedemptycomposer read rather than re-probing the backend.fm_afk_clear_stale_artifacts(bin/fm-afk-start.sh), inbin/fm-afk-return.sh, and across all three artifact lists inbin/fm-afk-launch.sh(backup, restore, and both start paths) so it is preserved on refresh and cleared only on fresh entry. Docs (docs/architecture.md,docs/configuration.md,.agents/skills/afk/SKILL.md) describe the escape, the marker lifecycle, and the delivery record, andtests/fm-daemon.test.shadds cases for deferring below threshold, escaping at threshold, one-second-short deferral,0disabling the escape, single composer read on escape, immunity to a stale escalation age, and the branch-naming deferral log, with launch/return tests extended to cover the new artifact.Risk Assessment
✅ Low: The busy-guard escape is well-bounded and fails closed at every uncertain point (uncreatable/corrupt marker, unobserved gaps, non-empty composer), the resolvers avoid both octal misreads and arithmetic overflow, the artifact lifecycle is consistent across all four owners, and the new tests exercise real behavior rather than source text; only two cosmetic log/redirect nits remain.
Testing
I ran the targeted daemon test file plus the two afk tests touched by the artifact-list changes and all passed, then went beyond unit coverage to reproduce the actual failure end to end: the real inject_msg driven on a real poll cadence against a supervisor pane that reports busy forever while its composer reads provably empty. Against the base commit the daemon defers indefinitely with an unattributable "agent mid-turn" line and never records a delivery; on this branch it logs the observed busy+empty streak growing against the configured threshold, fires the override at the bound, actually submits the encoded escalation, logs the delivery, writes state/.subsuper-last-delivery, and clears the streak marker. Setting FM_BUSY_GUARD_ESCAPE_SECS=0 restores the old always-defer behavior while keeping the explainable log lines. Transcripts for all three runs are saved as evidence; this is a daemon/log-facing change with no rendered UI surface, so log transcripts are the end-user artifact.
Evidence: Before/after operator log transcripts (base commit vs this branch vs escape disabled)
Source: Before/after operator log transcripts (base commit vs this branch vs escape disabled)
Evidence: Reproduction script for the wedged-pane scenario
Source: Reproduction script for the wedged-pane scenario
Evidence: Fixed-branch daemon log: bounded deferral then override and confirmed delivery
Evidence: Base commit 07bf0c8 daemon log, same scenario: unbounded silence
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-supervise-daemon.sh:1404- When FM_BUSY_GUARD_ESCAPE_SECS=0 (documented as "disables the escape"), the busy branch still falls into this else and logs... composer confirmed-empty for Ns straight, escapes at 0son every housekeeping tick. "escapes at 0s" reads as "the threshold is zero, so it should have escaped already", the opposite of what 0 means, which works against the intent's second goal of making delivery state explainable from the log alone. Concretely: an operator sets FM_BUSY_GUARD_ESCAPE_SECS=0 to restore the old always-defer behavior, then sees the daemon log "confirmed-empty for 45s straight, escapes at 0s" repeatedly while nothing ever escapes. Suggest branching the message onescape_secs -eq 0(e.g. "escape disabled") - and, since the marker has no consumer while disabled, skipping the per-tick marker write in that case too.bin/fm-supervise-daemon.sh:1443-printf '%s\n' "$(_now)" > "$state/.subsuper-last-delivery" 2>/dev/null || trueapplies2>/dev/nullafter the output redirect, so if creating the marker fails (read-only or missing state dir) bash emits its own "cannot create" diagnostic on the still-unredirected stderr before the suppression takes effect. The exit status is swallowed by|| true, but the noise is not. The streak-marker write four blocks earlier already uses the correct{ printf ... > "$streak_marker"; } 2>/dev/null || truebrace form; match it here.✅ **Test** - passed
✅ No issues found.
bash tests/fm-daemon.test.sh(123 ok assertions, includes all new busy-guard escape / streak-marker / resolve_busy_guard_escape_secs / resolve_busy_empty_streak_step_max / delivery-logging tests)bash tests/fm-afk-return.test.shbash tests/fm-afk-launch.test.shManual e2e: drove the realinject_msgon a real FM_HOUSEKEEPING_TICK=2 cadence against a stubbed backend whose native agent-state reads busy forever with a confirmed-empty composer, with FM_BUSY_GUARD_ESCAPE_SECS=6, capturing state/.supervise-daemon.log, the text actually submitted, and state/.subsuper-last-deliveryManual e2e baseline: same scenario run againstgit show 07bf0c8:bin/fm-supervise-daemon.sh(pre-fix daemon) - defers indefinitely, no delivery, no branch attribution in the logManual e2e opt-out: same scenario with FM_BUSY_GUARD_ESCAPE_SECS=0 - always-defer preserved, deferral lines still explain busy source, observed streak seconds, and threshold.agents/skills/afk/SKILL.md:179- Pre-existing duplication, not introduced by this change: the "Injection hardening" bullet list re-states the busy/composer guard contract already owned by the earlier delivery-guard prose (lines 95-108), and now describes only the deferring half of the busy guard with no mention of the bounded escape. Nothing there contradicts the code (the escape still requires an affirmativelyemptycomposer, so "Onlyemptypermits injection" stays true), so I did not synchronize a second copy per the placement policy. A follow-up could reduce that bullet to a pointer at the owning section instead; doing it here would be an out-of-scope restructure of unrelated prose.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.