fix(bin): never report done for a run whose pr or ci step was skipped - #1896
Open
ivannovak wants to merge 3 commits into
Open
fix(bin): never report done for a run whose pr or ci step was skipped#1896ivannovak wants to merge 3 commits into
ivannovak wants to merge 3 commits into
Conversation
no-mistakes v1.41.2 reaches outcome=passed with its pr and ci steps
silently skipped when the pipeline daemon's gh is unauthenticated
("skipping PR creation: gh CLI is not authenticated" in the step log),
so a run could report the pipeline's strongest verdict having never
opened a PR or observed a single CI check, and fm-crew-state.sh then
rendered it as "run passed: PR merged/closed".
Root cause of the missing credential, established empirically: the
daemon runs as a launchd LaunchAgent (installed 2026-07-31 by the
v1.41.2 update) and gh's token lives only in the macOS login keychain
(hosts.yml carries no token). The environment is not the boundary -
gh auth status succeeds under env -i HOME PATH with the daemon's exact
environment - and the same daemon process created PRs Aug 1-4 before
failing consistently from Aug 5 18:55 with no reboot, no gh upgrade,
and no keychain item change, so the daemon's detached security session
lost access to the login keychain at runtime. Fixing the daemon itself
is upstream (closed-source binary); this change makes firstmate stop
believing the false verdict.
fm-crew-state.sh now checks the axi status steps[] table whenever a
run reads passed, checks-passed, or bare completed: any skipped pr or
ci row reports state failed with the skipped steps named, never done,
so the fleet view cannot claim a merged PR that never existed
(companion of the separately filed fm-crew-state-false-merged-claim).
Tests: new fixtures mirror the real skipped-delivery output verified
against run 01KZBCA6DEHQC1404F34QJE0CC; watched red against the
unguarded code, green after, and red again with the guard's match
pattern sabotaged; a negative control keeps a genuinely delivered
pass reporting done.
…ng-evidence wording
5 tasks
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
1 participant
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 no-mistakes pipeline reaches outcome=passed with its pr and ci steps silently skipped, so a run can report the pipeline's strongest terminal verdict having never opened a PR or observed a single CI check. Found 2026-08-05 across five consecutive auctic-core runs (dev-5190, dev-5441, dev-5490, auctic-dompdf2-lane-stranded, auctic-phpstan-never-loaded), each showing push,completed then pr,skipped and ci,skipped; the step logs name the cause: 'skipping PR creation: gh CLI is not authenticated'. Task scope had two parts. Part 1, establish why the daemon's gh has no credential when interactive gh works: established empirically that the daemon runs as a launchd LaunchAgent (installed 2026-07-31 by the v1.41.2 update) with only HOME and PATH env; gh's token lives only in the macOS login keychain (hosts.yml has no token); env alone does not reproduce the failure (gh auth status succeeds under env -i with the daemon's exact env); the same daemon created PRs Aug 1-4 then failed consistently from Aug 5 18:55 with no reboot, no gh upgrade, and no keychain item change - so the daemon's detached security session lost login-keychain access at runtime. The daemon itself is a closed-source third-party binary, so it cannot be patched here. Part 2, make a skipped pr or ci step fail the run or at minimum forbid the passed outcome: implemented on firstmate's side in bin/fm-crew-state.sh, the single owner of run-state interpretation (verified no other bin script parses outcome). A new nm_skipped_delivery_steps helper scans the axi status steps[] table; when a run reads passed, checks-passed, or bare completed status with a skipped pr or ci row, the crew state now reports failed naming the skipped steps, never done, so the fleet view cannot claim 'run passed: PR merged/closed' for a PR that never existed. The companion rendering bug (fm-crew-state-false-merged-claim, the 'PR merged/closed' wording on genuine passes at bin/fm-crew-state.sh) is filed separately and deliberately NOT changed here; genuine passes keep their existing wording, enforced by a negative-control test. Tests: new fixtures in tests/fm-crew-state.test.sh mirror the real skipped-delivery axi output verified against real run 01KZBCA6DEHQC1404F34QJE0CC; the guard was watched fail red against the unguarded code and again with its match pattern sabotaged; a negative control keeps a genuinely delivered pass reporting done. Constraints honored: never restart or modify the shared no-mistakes daemon; no launchd state mutation (the definitive launchd-context gh test was deliberately not run because it would mutate state outside the task worktree); repo style one-sentence-per-line comments, shellcheck-clean via bin/fm-lint.sh, colocated tests extending the existing fm-crew-state.test.sh runner.
What Changed
bin/fm-crew-state.shgainsnm_skipped_delivery_steps, which scans theaxi statussteps table, andnm_missing_delivery_evidence, which derives the wording from what was actually skipped. A run readingpassed,checks-passed, or barecompletedwith a skippedprorcirow now reportsfailednaming the skipped steps and only the evidence that does not exist (no pull request opened, no CI check observed, or both) instead ofdone.no-mistakes runslist fallback no longer maps a terminalcompletedrow todone; that source carries no steps table, so it reportsunknownand points ataxi statusfor the branch's own run. Thefailed/cancelledcoarse arms are unchanged.tests/fm-crew-state.test.shadds four fixtures mirroring the real skipped-deliveryaxioutput plus six cases, including a negative control that keeps a genuinely delivered pass reportingdoneand a case covering the coarse terminal row.AGENTS.mdanddocs/architecture.mdrecord the new rule for both paths.Risk Assessment
✅ Low: Both captain-directed fixes are implemented precisely to their stated scope with regression tests and scope guards, no existing test or consumer depended on the changed coarse mapping, and no path in the script can now certify done without delivery-step evidence.
Testing
I ran the targeted colocated suite (tests/fm-crew-state.test.sh, all green) plus the three neighbouring suites that consume fm-crew-state, then went past unit tests to a product-level end-to-end run: a throwaway firstmate home with four ship crews whose fake
no-mistakes axi statusoutput mirrors the real 2026-08-05 incident, driven through the actual operator commands (fm-crew-state.sh, fm-fleet-snapshot.sh --json, fm-fleet-view.sh) once against the base commit and once against the fix. The captured transcripts show the fleet view's Current column moving fromdone / run-step - run passed: PR merged/closedtofailed / run-step - no pull request was opened and no CI check was observedfor the two runs that never opened a PR, the ci-only skip naming only the missing CI evidence, and the genuinely delivered pass stayingdonewith its wording unchanged. I then watched each new check fail: four mutations (guard removed,cidropped from the match pattern, coarsecompletedarm restored to done, guard widened to fire on completed rows) each drove a different new assertion red, so none of them - including the negative control - is inert. No screenshots or rendered HTML apply here: the changed surface is a terminal CLI/markdown fleet table with no browser or GUI rendering path, so the CLI transcripts are the end-user surface. One failure in tests/fm-teardown.test.sh turned out to be pre-existing (identical failure with base-commit fm-crew-state.sh) and unrelated to run-outcome interpretation. Working tree left clean and all scratch dirs removed.Evidence: Fleet view before/after, side by side (the operator-visible verdict flip)
Fleet view "Current" column, same four crews, same fake axi output. dev-5190 / dev-5441 : outcome=passed, pr,skipped ci,skipped, pr: "" (the real incident) auctic-dompdf2-lane-... : outcome=checks-passed, pr,completed ci,skipped, real PR URL auctic-genuine-pass : outcome=passed, pr,completed ci,completed (negative control) CREW BEFORE (2cf0283) AFTER (49a9f8f) ------------------------------ ---------------------- ---------------------- dev-5190 done / run-step failed / run-step dev-5441 done / run-step failed / run-step auctic-dompdf2-lane-stranded done / run-step failed / run-step auctic-genuine-pass done / run-step done / run-step Detail line for dev-5190 (the run with no PR at all): BEFORE: state: done · source: run-step · run passed: PR merged/closed AFTER : state: failed · source: run-step · run reported passed but skipped delivery step(s) pr/ci: no pull request was opened and no CI check was observed, do not trust the pass (daemon gh auth?) Detail line for the ci-only skip (pr step completed, PR URL exists): BEFORE: state: done · source: run-step · checks green: PR ready for review AFTER : state: failed · source: run-step · run reported checks-passed but skipped delivery step(s) ci: no CI check was observed, do not trust the pass (daemon gh auth?)Evidence: BEFORE - full CLI transcript at base commit 2cf0283 (bug reproduced)
Evidence: AFTER - full CLI transcript with the fix (fm-crew-state, fleet view, snapshot JSON)
Evidence: Instrument verification - each new guard and control watched fail red under mutation
===== MUTATION 1 - guard removed entirely (bin/fm-crew-state.sh reverted to base 2cf0283) ===== not ok - passed with skipped pr/ci -> failed (missing: 'state: failed') --- output --- state: done · source: run-step · run passed: PR merged/closed ===== MUTATION 2 - match pattern sabotaged: 'ci' dropped from the (pr|ci) alternation ===== not ok - both steps skipped -> detail names both missing pieces of evidence --- output --- state: failed · source: run-step · run reported passed but skipped delivery step(s) pr: no pull request was opened, do not trust the pass (daemon gh auth?) ===== MUTATION 3 - coarse runs-list 'completed' arm restored to done ===== not ok - coarse completed row -> unknown, not a certified pass (missing: 'state: unknown') --- output --- state: done · source: run-step · run completed ===== MUTATION 4 - guard widened to fire on completed rows too (negative controls must catch it) ===== not ok - must not deny a PR whose step completed and whose URL exists (unexpected: 'no pull request was opened') --- output --- state: failed · source: run-step · run reported checks-passed but skipped delivery step(s) pr/ci: no pull request was opened and no CI check was observed, do not trust the pass (daemon gh auth?)Evidence: e2e harness used to produce the before/after transcripts (reproducible)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
bin/fm-crew-state.sh:452- The intent claims a delivery-skipped run 'never' reads done, but the guard only covers the full axi-status TOON path. The coarse runs-list fallback still maps completed -> done ('run completed') with no delivery-step evidence: when bareaxi statusanswers another crew's branch (the exact scenario exercised by test_cross_branch_attribution_via_runs_list and ..._picks_most_recent_row, which includes a completed row), a delivery-skipped run is attributed from theno-mistakes runslist as a plaincompletedrow with matching short-sha and reportsstate: done · run completed, bypassing nm_skipped_delivery_steps entirely. The coarse row for such a run has no pr-url column (no PR ever existed), which is available evidence: nm_runs_status_for_branch already parses each row and could surface pr-url presence so the coarse completed mapping refuses done (or reports unknown pending full attribution) for a completed row without a PR URL. This is pre-existing sibling behavior, not introduced by the diff, and changing it alters coarse-path reporting semantics, so it needs the author's call rather than an auto-fix.bin/fm-crew-state.sh:472- The failure detail 'no PR/CI evidence exists, do not trust the pass' overstates the situation when only the ci step was skipped: in the run_checks_passed_ci_skipped fixture the pr step completed and a real PR URL exists (pr: https://github.com/o/r/pull/3), so only CI evidence is missing. The wording could steer an operator away from inspecting a PR that does exist. Consider naming only the actually-missing evidence (e.g. derive the message from which steps were skipped).🔧 Fix: close coarse runs-list done bypass, derive missing-evidence wording
✅ Re-checked - no issues remain.
tests/fm-teardown.test.sh:1568- tests/fm-teardown.test.sh fails atherdr-preflight-missing-adapter: the retryable pre-return refusal was not explained visibly. This is NOT a regression from this change: I re-ran the same suite against a copy of the tree with bin/fm-crew-state.sh reverted to base 2cf0283 and it fails with the identical assertion. The test exercises herdr backend preflight refusal (a missing bin/backends/herdr.sh adapter), which is unrelated to run-outcome interpretation. Noted so it is not mistaken for fallout from the skipped-delivery guard; remote CI owns whether it needs attention.bash tests/fm-crew-state.test.sh- full colocated suite including the four new skipped-delivery fixtures and the two new coarse runs-list cases (all green)End-to-end product run: stood up a throwaway FM_HOME with four ship crews (dev-5190, dev-5441, auctic-dompdf2-lane-stranded, auctic-genuine-pass) and a fakeno-mistakesserving the real incident's axi output, then ranbin/fm-crew-state.sh <id>,bin/fm-fleet-snapshot.sh --json, andbin/fm-fleet-view.shagainst BOTH the base-commit bin/ and the fixed bin/ (harness: e2e-harness.sh in the evidence dir)Instrument verification / mutation 1:git show 2cf0283:bin/fm-crew-state.shover a copied tree, thenbash tests/fm-crew-state.test.sh-passed with skipped pr/ci -> failedgoes red, printing the exact bug outputstate: done ... run passed: PR merged/closedInstrument verification / mutation 2: sabotaged the match pattern by droppingcifrom(pr|ci)- the derived-wording assertionboth steps skipped -> detail names both missing pieces of evidencegoes redInstrument verification / mutation 3: restored the coarse runs-list arm tocompleted) RUN_STATE="done"-coarse completed row -> unknown, not a certified passgoes redInstrument verification / mutation 4: widened the guard to"?(skipped|completed)"?so it fires on completed rows - the negative controlmust not deny a PR whose step completed and whose URL existsgoes red, proving the controls are not inertbash tests/fm-fleet-snapshot-view.test.sh- the renderer that consumes fm-crew-state (green)bash tests/fm-watch-triage.test.sh(green)bash tests/fm-wake-queue.test.sh- exercises crew_is_provably_working over the real helper (green)bash tests/fm-teardown.test.sh- one failure, re-run against base-commit fm-crew-state.sh to confirm it pre-exists this changeSpot-check of the intent's single-owner claim:grep -rn outcome bin/- only bin/fm-teardown.sh also readsoutcome, and only to tell terminal from non-terminal after an abort (task_status_is_terminal_run, bin/fm-teardown.sh:1180), deriving no pass verdict, so no second bypass path exists✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.