Skip to content

fix(bin): stop misreporting a live no-mistakes run as failed and suppress false wedge alerts - #3001

Open
Humper wants to merge 3 commits into
kunchenguid:mainfrom
Humper:fm/fm-crewstate-abort-c2
Open

fix(bin): stop misreporting a live no-mistakes run as failed and suppress false wedge alerts#3001
Humper wants to merge 3 commits into
kunchenguid:mainfrom
Humper:fm/fm-crewstate-abort-c2

Conversation

@Humper

@Humper Humper commented Aug 25, 2026

Copy link
Copy Markdown

Intent

Fix worker health reads that report a RUNNING no-mistakes validation as failed, and fix the weaker liveness readings around it. This is firstmate's own shared tracked material, so firstmate-coding-guidelines applies: one sentence per line in tracked Markdown, plain dash never em dash, no agent co-author, shellcheck-clean bin scripts via bin/fm-lint.sh, colocated tests named .test.sh, and tests that exercise behavior through an executable interface rather than asserting implementation source bytes.

THE BUG, REPRODUCED. bin/fm-crew-state.sh's nm_runs_status_for_branch() walks no-mistakes runs newest-first and returned the first row matching the branch whose short sha matched the worktree HEAD. That sha-identity rule exists to reject a stale run from a rewritten tip, but during a no-mistakes fix round it inverts. The pipeline takes custody of the branch and commits in its own gate clone, so the running run's recorded sha advances past (and is often not even an object in) the crew's worktree. The check skipped the live run and accepted an older cancelled row whose sha still equalled that HEAD, yielding failed - run cancelled for a validation that axi status reported at the same moment as running at review/fixing. Captured live on 2026-08-23 against no-mistakes v1.53.0 on branch fm/hm-vault-scripts-k2: running run at head 1f645c15 which git rev-parse could not resolve in that worktree at all, plus an older cancelled row at worktree HEAD 8de4d0d9. When the run concluded and custody returned, the worktree HEAD became exactly the head the finished run reported, confirming the mechanism from both sides. The false-failure direction is the unsafe one: it invites tearing down live work and reporting a failure to the captain that never happened. It cost real supervision decisions twice in one night, so treat it as a correctness bug in a safety-relevant predicate, not a cosmetic one.

WHAT WAS ASKED FOR, AND THE DECISIONS MADE.

  1. The sha-identity rule must not reject the live run just because the pipeline advanced the branch, but must keep whatever it was actually defending against - deleting the check was explicitly ruled out. A run whose head is an ancestor or descendant of the worktree head is plausibly the same work; an unrelated sha is not. Chosen rule: split the head verdict into six outcomes instead of a boolean, so "unverifiable" (a head this worktree simply does not have) is no longer collapsed into the same rejection as "unrelated" (a head we do have, that diverged) or "absent" (no head at all). Exactly one cell of the resulting policy changes: an unverifiable head may keep an ACTIVE run attributed, and may never produce a terminal verdict. Every pre-existing rejection is preserved - a rewritten or diverged tip, a run left behind by local commits, and a run with no head at all are all still refused, each keeping the incident it was written for.

  2. Precedence: an active run named by axi status should dominate the coarse runs-list fallback outright rather than being one input among several, because axi status names the run id and its live step while the coarse table has only a sha guaranteed to drift in exactly the situation that matters. The existing precedence comments in that file document prior incidents and were required not to be reintroduced. Made structural: while axi status names this crew's own branch with a run that has not concluded, a terminal word from the coarse table can only describe superseded work, so it is dropped rather than reported. The coarse walk also phases each row by its own status word, so a live row is judged active and a cancelled row terminal.

  3. Close the repeated unknown - source: none reading, or state precisely why it cannot be closed. Closed: it occurred on a crew whose run was live, whose pane was legitimately idle waiting on that run, and whose last status line was a resolved: event that is deliberately never a state - every source declined at once, and attributing the live run is what answers. A second contributor was found and fixed in the follow-up commit: the coarse mapping's old catch-all emitted unknown sourced to the run step, which reads as a confident finding ABOUT the run and suppressed every other source that could have answered. The mapping now returns a verdict only for a word it understands, HAVE_RUN keys off that return value, an unrecognised word says nothing and lets the pane and status log answer, and gate words map to parked.

  4. False wedge on a long foreground call: a crew sitting in a legitimate nine-minute foreground no-mistakes axi run raised stale: idle 382s, possible wedge, because liveness was inferred only from signals that branch custody silences (the pane rendering and worktree writes). Across a busy fleet that produced seven consecutive false escalations in one night, since a review step routinely runs 20 to 130 minutes against a 240s threshold. Added the run's own active_steps activity clock as a fourth liveness input, probed only at the escalation threshold, and chose to SUPPRESS the escalation outright rather than defer it to a cadence: escalating a crew the supervisor can prove is working is a contradiction, and an hourly re-surface across sixteen workers is still a stream of wakes. FM_STALE_ESCALATE_SECS is deliberately left at 240 - raising it would buy quiet by slowing detection of a genuinely wedged crew - and now governs only the case where nothing can prove liveness. What keeps this from becoming a hiding place is evidence strength, not cadence: the mere existence of an active step is refused because a frozen run keeps one forever, only activity reported INSIDE the quiet window suppresses, the idle window restarts from that proof, and the untouched timer escalates the moment the run stops proving progress. Accepted residual, recorded in code and in docs/architecture.md: a pipeline that logs forever without converging is left to no-mistakes' own idle timeout and fix-round caps.

  5. Report, do not fix, unless the fix is small and clearly correct: herdr agent list returned null name and null state for every pane, so the herdr busy source appeared dead. Established as a wrong invocation, not a version or product defect, and deliberately left as a documented finding with NO code change: both agent list and agent get name those fields agent and agent_status and neither exposes name or state, and firstmate reads agent get, so it was never affected. Recorded in docs/herdr-backend.md with the dated commands and output.

A separately reported second source - bare axi status answering about a DIFFERENT branch when this branch has no run, which reported a healthy task as another branch's cancelled, ci failed - was reproduced live and found to need no code change, because branch equality is already a hard precondition in every consumer and the coarse walk matches branch before sha. What was missing was coverage, so two tests now pin that the BRANCH check is what refuses. Both pass before this change as well as after, which is the point: they guard existing protection rather than a new fix. Please do not flag them as redundant.

TESTS. The reproduction is a regression test: a branch with a running run at one sha and an older cancelled run at the worktree's sha must read as running. The ancestor and descendant cases that are accepted are covered, as is the genuinely-unrelated sha that is still rejected, plus the foreign-branch refusals, the unrecognised coarse word falling through to both the pane and the status log, a gate word reporting parked, and the activity probe's wall-clock bound falling back rather than disabling on a bad value.

DELIVERY. no-mistakes, and this repo has CI so it must not be skipped. Merge waits on the captain - do not merge.

HISTORY OF THIS BRANCH. A prior validation attempt died at the review step on an infrastructure error, not a finding. Its review-round fixes were preserved and carried forward as the second commit here rather than being re-derived, and the branch was rebased onto current origin/main. Local verification before starting this run: tests/fm-crew-state.test.sh, tests/fm-watch-triage.test.sh, tests/fm-teardown.test.sh, bin/fm-lint.sh, and bin/fm-doc-audience-check.sh all pass.

What Changed

  • fm-crew-state.sh: replaced the boolean sha-identity check in nm_runs_status_for_branch() with a six-outcome head verdict (ancestor/descendant/unrelated/absent/unverifiable/etc.), so a head this worktree cannot resolve is treated as unverifiable rather than rejected - an unverifiable head can keep an ACTIVE run attributed but never yields a terminal verdict, while unrelated, diverged, and headless runs are still refused.
  • fm-crew-state.sh / fm-nm-run-lib.sh: gave an active run named by axi status outright precedence over the coarse no-mistakes runs table fallback, and made the coarse walk phase each row by its own status word so a live row reads active and a cancelled row reads terminal.
  • fm-classify-lib.sh: reworked the coarse status mapping so it returns a verdict only for recognized words (gate words map to parked), letting HAVE_RUN key off that return value instead of emitting a catch-all unknown sourced to the run step that previously suppressed other status sources.
  • fm-watch.sh: added the run's own active_steps activity clock as a fourth liveness input, probed only at the stale-escalation threshold, so a crew with proven in-window run activity has its wedge escalation suppressed outright instead of deferred, with FM_STALE_ESCALATE_SECS left unchanged and governing only the unprovable case; hardened the activity probe's wall-clock parsing to fall back rather than disable on a bad value.
  • fm-teardown.sh / fm-test-run.sh: adjusted call sites for the updated crew-state/status APIs.
  • Added tests/fm-crew-state.test.sh and tests/fm-watch-triage.test.sh coverage for the new head-verdict outcomes, axi-status precedence, coarse status mapping fallthrough, and the activity-probe bound; extended tests/fm-afk-return.test.sh and tests/fm-teardown.test.sh.
  • Updated docs/architecture.md, docs/configuration.md, and docs/herdr-backend.md to describe the new head-verdict policy, wedge-liveness behavior, and the reproduced-but-unaffected herdr agent list finding.

Risk Assessment

✅ Low: This fix round correctly resolves all four outstanding findings (in-row activity parser now takes only each row's first " ago" token before taking the minimum across rows, the afk-return fixture now copies fm-nm-run-lib.sh and a repo-wide sweep confirms no other fixture is missing it, the probe-cost comment now accurately describes crew_run_active_within's early-exit conditions, and the ci-green-at-unverifiable-head exception is now documented in both the precedence header and inline at the override site with regression tests pinning the boundary); all changes are shellcheck-clean, test-only or comment/logic fixes scoped exactly to the prior findings, and no new risk is introduced.

Testing

Ran the four test files most directly tied to the branch's intent: tests/fm-crew-state.test.sh (exit 0, includes the named regression 'a running row whose head is held by the pipeline outranks an older cancelled row at the worktree HEAD' plus the six-outcome head-verdict policy and the ci-green-at-unverifiable-head exception), tests/fm-watch-triage.test.sh (exit 0, includes the named activity-parser regression for '40m2s ago: ... gave up after 3s ago retrying' and the wall-clock-bounded activity probe), tests/fm-teardown.test.sh (exit 0), and tests/fm-afk-return.test.sh (exit 0, confirms the fixture now copies fm-nm-run-lib.sh alongside fm-classify-lib.sh). Also swept tests/*.sh for any other fixture building a fake bin/ containing fm-classify-lib.sh without fm-nm-run-lib.sh; every other reference sources fm-classify-lib.sh directly from $ROOT/bin (a real checkout with the sibling file present), so no other fixture needs the same fix. All targeted suites passed with no failures; working tree is clean with no transient artifacts to remove.

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

🔧 **Rebase** - 1 issue found → auto-fixed ✅
  • ⚠️ docs/architecture.md - merge conflict rebasing onto origin/main

🔧 Fix applied.
✅ Re-checked - no issues remain.

🔧 **Review** - 4 issues found → auto-fixed ✅
  • 🚨 bin/fm-nm-run-lib.sh:226 - fm_nm_last_activity_secs still takes the minimum over EVERY ' ago' token anywhere in the whole active_steps block, not the first ('own') token per row. This is a standing decision from an earlier review round (id activity-parser-min-over-in-row-free-text), explicitly restated with required regression tests: 'a single row whose last_activity is "40m2s ago: log: gave up after 3s ago retrying" must yield 2402 and never 3'. Verified live against the current code:
    fm_nm_last_activity_secs on a row with last_activity="40m2s ago: log: gave up after 3s ago retrying" returns 3, not 2402.
    The required regression test was never added either (tests/fm-watch-triage.test.sh only covers the trailing-scalar-after-the-table case, not a second ' ago' token inside the same row's free text). Because last_activity is a frozen, truncated agent log line, any small duration-shaped substring the agent happened to write becomes permanent proof of liveness and suppresses every future wedge escalation for that crew - the exact failure this probe exists to prevent, and the exact failure the standing decision required be closed.
  • 🚨 tests/fm-afk-return.test.sh:21 - install_runner still does not copy bin/fm-nm-run-lib.sh into the fixture's fake bin, even though bin/fm-classify-lib.sh sources it (bin/fm-classify-lib.sh:56) and the fixture's own comment says this list must track classify-lib's sourced siblings. This is a standing decision from an earlier review round (id afk-return-fixture-missing-nm-run-lib) that was explicitly restated as required, including 'grep the whole tests/ tree for any other fixture that builds a fake bin containing fm-classify-lib.sh and fix every one the same way'. Verified live: sourcing bin/fm-classify-lib.sh from a bin/ directory built exactly like install_runner's produces 'fm-nm-run-lib.sh: No such file or directory' on stderr and leaves fm_nm_* undefined. bin/fm-afk-return.sh, the script under test, merges stderr into asserted output, so it now runs with a spurious error and undefined run-classification primitives on every invocation in this suite; the existing substring assertions still pass, which is why it ships silently.
  • ⚠️ bin/fm-watch.sh:617 - The probe-ordering comment still says 'Where there is no run to find, it returns without spending its bound', which a prior review round already identified as false and the user explicitly required be corrected (id run-probe-cost-comment-overstates-early-exit). crew_run_active_within's early returns cover a missing id, a bad window, no no-mistakes binary, no/torn-down worktree, a non-ship kind, and a detached HEAD - none of which is 'no run'; for any stale ship crew on a branch the bounded axi status call is still made, and the refusal happens after it on the branch-equality check. The comment was left unchanged despite this round's own fix_summary claiming to harden exactly this file.
  • ⚠️ bin/fm-crew-state.sh:37 - The precedence header still states the invariant as an absolute - 'the one rule that must survive with nothing loaded is that a head this worktree cannot place NEVER yields a terminal verdict' - while the ci-green override at bin/fm-crew-state.sh:611-613 still sets RUN_STATE="done" (a terminal verdict) reachable at an unverifiable head via fm_nm_run_attributable's active-phase exception. A prior review round ruled this exception is intentional and required three things: (i) amend the header to state the narrow exception explicitly, (ii) put the same safety argument in the code at the ci-green override site itself, (iii) add tests pinning both that the other two paths (outcome-bearing terminal run, coarse terminal word) still cannot produce a terminal verdict on an unverifiable head, and that the ci-green override does. None of the three landed: the header is unchanged, there is no comment at the override site, and no such tests were added to tests/fm-crew-state.test.sh.

🔧 Fix: Fix wedge-liveness parser, afk fixture, and two doc gaps
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-crew-state.test.sh
  • bash tests/fm-watch-triage.test.sh
  • bash tests/fm-teardown.test.sh
  • bash tests/fm-afk-return.test.sh
  • grep sweep of tests/*.sh for other fixtures copying fm-classify-lib.sh into a fake bin dir (none found needing fm-nm-run-lib.sh besides fm-afk-return.test.sh, which already copies it)
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

Humper added 3 commits August 25, 2026 01:12
…the branch

A no-mistakes fix round takes custody of the branch and commits in its own gate
clone, so a healthy run's recorded head is routinely NOT an object in the crew's
worktree. The attribution rule collapsed "this worktree cannot resolve that head"
into the same rejection as "that head diverged from ours", so the coarse
runs-list walk skipped the live row and accepted an older cancelled row that
still sat at the worktree HEAD. A crew mid fix-round therefore read as
`failed - run cancelled` while `axi status` reported the same run as running at
review/fixing. The false-failure direction is the unsafe one: it invites tearing
down live work and reporting a failure that never happened.

Captured live on 2026-08-23 against no-mistakes v1.53.0: branch
fm/hm-vault-scripts-k2 had a running run at head 1f645c15 which
`git rev-parse --verify 1f645c15^{commit}` in that crew's worktree could not
resolve at all, plus an older cancelled row at the worktree HEAD 8de4d0d9. When
that run later concluded and custody returned, the worktree HEAD became exactly
the head the finished run reported, confirming the mechanism from both sides.

Split the head verdict into six outcomes instead of a boolean, so "unverifiable"
(a head we simply do not have) is no longer confused with "unrelated" (a head we
have and that diverged) or with "absent" (no head at all). Exactly one cell of
the resulting policy changes: an unverifiable head may keep an ACTIVE run
attributed, and may never produce a terminal verdict. Every existing rejection is
preserved - a rewritten or diverged tip, a run left behind by local commits, and
a run with no head at all are all still refused, each keeping the incident it was
written for.

Make the precedence structural rather than emergent: while `axi status` names
this crew's own branch with a run that has not concluded, a terminal word from
the coarse table can only describe superseded work, so it is dropped instead of
reported. The coarse walk now also phases each row by its own status, so a live
row is judged as active and a cancelled row as terminal.

This also closes the `unknown - source: none` reading seen on a crew whose run
was live, whose pane was legitimately idle waiting on that run, and whose last
status line was a `resolved:` event that is deliberately never a state: every
source declined at once, and attributing the live run is what answers.

Separately, the wedge detector inferred liveness only from things branch custody
silences. A crew in a long foreground validation renders nothing and writes
nothing to its worktree, so a nine-minute run drew
`stale: idle 382s, possible wedge` - and across a busy fleet that produced seven
consecutive false escalations in one night, because a review step routinely runs
20 to 130 minutes against a 240s threshold.

Add the run's own active_steps activity clock as a fourth liveness input, probed
only at the escalation threshold, and SUPPRESS the escalation outright when it
proves progress rather than deferring it to a cadence: escalating a crew the
supervisor can prove is working is a contradiction, and a deferral that still
re-surfaces hourly is still a stream of wakes across sixteen workers.
FM_STALE_ESCALATE_SECS is deliberately left at 240 - raising it would buy quiet
by slowing detection of a genuinely wedged crew - and now governs only the case
where nothing can prove liveness.

What keeps this from becoming a hiding place is the strength of the evidence, not
a cadence: the mere existence of an active step is refused, because a frozen run
keeps one forever. Only activity reported INSIDE the quiet window suppresses, the
idle window restarts from that proof, and the untouched timer escalates the
moment the run stops proving progress. Accepted residual, recorded in the code
and in docs/architecture.md: a pipeline that logs forever without converging is
left to no-mistakes' own idle timeout and fix-round caps.

A reported second source - bare `axi status` answering about a DIFFERENT branch
when this branch has no run, which reported a healthy task as another branch's
`cancelled, ci failed` - turned out to need no code change: branch equality is
already a hard precondition in every consumer of that command, and the coarse
walk matches each row on branch before its sha. Verified by reproducing it live.
What was missing was coverage, so two tests now pin that the BRANCH check is what
refuses: a terminal foreign-branch run carrying this worktree's own real head,
and a foreign coarse row sitting at this worktree's exact HEAD sha. Both pass
before this change as well as after, which is the point - they guard existing
protection rather than a new fix.

Also record what `herdr agent list` actually reports, after a probe reading
`.name` and `.state` returned null for every pane and read as a dead source.
Both `agent list` and `agent get` name those fields `agent` and `agent_status`;
firstmate reads `agent get` and was never affected. No code change.
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified.

The new attribution policy preserves terminal-run rejection for unverifiable heads, limits active attribution by branch and phase, and uses bounded recent activity rather than mere active-step existence to suppress wedge alerts.

Reviews (1): Last reviewed commit: "no-mistakes(review): Fix wedge-liveness ..." | Re-trigger Greptile

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: first look on current main 038d0f7ec6ba (#2942). Never messaged the captain.

class=corrective. Stops a live no-mistakes run from being mapped failed (coarse sha / terminal-word fallback while axi status still shows an attributable live step) and adds wedge_suppress_run_active as a fourth liveness input so a quiet pane during a real NM fix pass is not a possible wedge. STALE_ESCALATE_SECS is not raised. Not a change to when a genuine silent pane escalates after the run stops proving activity.

VISION.md (inspected bin/fm-crew-state.sh nm_run_attributable / coarse-precedence, bin/fm-nm-run-lib.sh, bin/fm-classify-lib.sh run-activity probe, bin/fm-watch.sh wedge_suppress_run_active, teardown comment/attribution, tests):

  • One captain, one interface: aligns (false failed / false wedge hid real in-flight work).
  • Authority is explicit: aligns (no new grant; refuses a destructive teardown implication from a wrong failed).
  • Scripts own the mechanics: aligns (attribution + age are exact).
  • A restart is a non-event: aligns (live run remains inspectable).
  • Delegation with a spine: aligns.
  • The fleet outlives any vendor: aligns (binds to no-mistakes axi status semantics).
  • Scope: aligns.

This HEAD: 1f4f1d72b5ed95860e0777d918b19068f80ea55d. MERGEABLE / UNSTABLE, ahead 3 / behind 0.
Attestation matches THIS HEAD.
CI action_required: CI 32798069067, Require no-mistakes 32798069077. Not first-time (Humper has open #2802); workflows not approved this pass. Greptile is not a merge gate.

Security: none. Overlap: bin/fm-classify-lib.sh with open #2801 (default-behavior hold); bin/fm-teardown.sh with #2804 / herdr pair #2637/#2692 (this hunk is attribution/comments + wedge, not pool isolation). Not a lock PR.

Land-eligible rec: NO. Captain-flag NOW: no (CI not green; classify/teardown overlap).

This is waiting-on-author / CI (fork checks still action_required). Not a captain-decision. Not a merge I will recommend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants