fix(bin): stop reporting cancelled or zero-check CI as green - #2415
fix(bin): stop reporting cancelled or zero-check CI as green#2415localxcrm wants to merge 5 commits into
Conversation
A checks-passed claim is now done only when at least one check completed with success and none are pending, failed, cancelled, skipped, or never-run. Zero checks is a distinct non-success state.
…e is environmental
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c32b24b20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| *"CI check cancelled"*|*"CI checks were cancelled"*|*"cancelled without"*) printf 'CANCELLED' ;; | ||
| *"checks failed"*|*"CI failures"*|*"issues detected"*) printf 'FAILURE' ;; | ||
| *"CI checks running"*|*"waiting for checks"*|*"base branch advanced"*"re-arming CI monitor timeout"*) printf 'PENDING' ;; | ||
| *"no CI checks reported"*|*"repository declares no CI"*) ;; |
There was a problem hiding this comment.
Captain, for repos using no-mistakes' trusted no_ci: true mode, the upstream skill says checks-passed is valid when that declaration covers a zero-check repo (source), and this branch even selects repository declares no CI; mapping it to an empty conclusion makes fm_ci_ready_verdict return empty/not-ready, so a legitimate checks-passed run is reported blocked (or stays working in the CI monitor) and the PR is never surfaced as ready. Please distinguish the trusted declaration from a generic no CI checks reported line and score it as the accepted success case.
Useful? React with 👍 / 👎.
1 participant
Intent
Fix the no-mistakes CI-ready verdict so cancelled, skipped, never-run, or zero checks are never reported as green. A green verdict is valid only if at least one check completed with success AND none are pending, failed, cancelled, skipped, or never-run. Zero checks is a distinct non-success state, not success. Cover both cancelled-as-green and zero-checks-as-green. A cancelled-only run cannot produce checks-passed. A run with zero checks cannot produce checks-passed. A genuinely all-success run still produces checks-passed. Tests must prove those three cases from code, not from a comment. Do not weaken an unrelated assertion to make a suite pass. Shared firstmate material; do not push to the default branch; do not merge. Open the PR against the configured upstream and stop at green.
What Changed
bin/fm-ci-verdict-lib.shas the single owner of the CI-ready rule: a check-conclusion set scorespassedonly when at least one check completed with success and none are pending, failed, cancelled, skipped, or never-run; zero checks scoresempty, everything elsenot-passed. It is sourced as a library and also runnable as a command.bin/fm-crew-state.shto route every green claim through that scorer: the ci-log marker is now classified into a conclusion (including new cancelled/CI failures/waiting for checks/repository declares no CIphrases) before scoring, a terminalchecks-passedoutcome reportsblockedwithout scored green evidence andunknownwhen the ci log is unreadable, and a barecompletedrun with no outcome — fromaxi statusor the coarse runs list — now readsunknowninstead ofdone, which also removes the coarse early-emit that skipped scoring entirely.tests/fm-ci-verdict-lib.test.shand extendedtests/fm-crew-state.test.shto pin the cancelled-only, zero-checks, and genuinely all-success cases (each new guard verified to fail against the base commit); registered the new test file inbin/fm-test-run.shand updatedAGENTS.md,docs/architecture.md, anddocs/scripts.mdto state the scored verdict and the outcomeless-run states. Review flagged two accepted boundaries (a CI-less repo now staysworkingrather thandone, and the no-run status-log fallback still promotes a crew's ownchecks greenline); the pre-existingtests/fm-watch-triage.test.shflake reproduces on the base tree and is untouched by this change.Risk Assessment
✅ Low: Both accepted fixes are correct and covered by tests that fail on regression, no existing fixture or consumer regresses, and every run-attributed path to done now requires scored evidence or a merged outcome.
Testing
Ran the two targeted suites (
tests/fm-ci-verdict-lib.test.sh,tests/fm-crew-state.test.sh) — both pass — then produced the real evidence by driving the actualbin/fm-crew-state.shCLI over throwaway git worktrees with a fakeno-mistakesbackend, side by side on base 1238402 and target 9eb7125: achecks-passedrun over cancelled checks and one over zero checks both used to printstate: done · checks green: PR ready for reviewand now printstate: blocked · run reported checks-passed without green check evidence, while the genuinely all-success control still printsstate: done · checks greenand a terminalcompletedrun with no outcome now readsunknowninstead ofdone. To show the tests pin this from code rather than from a comment, I grafted the target test file onto the base tree and ran each new case in isolation: all of them fail on base and pass on target, and the all-success control passes on both, so no assertion was weakened. I also captured a transcript of the shared scorer's public command across success/cancelled/skipped/pending/failed/never-run/zero-check inputs, confirmed the runner coverage guard still passes, and exercised the downstream crew-state consumers —fm-inactive-reconcilepasses, andfm-watch-triagefails non-deterministically with a different case each run on both base and target (it stubs the crew-state reader, so this change cannot reach it), which I recorded as a pre-existing environmental flake rather than a regression. This is a CLI-only change with no rendered UI surface, so the evidence is CLI transcripts rather than screenshots. Scratch trees were removed and the worktree is clean.Evidence: Before/after CLI transcript: fm-crew-state.sh verdict on base vs target
==== BASE 1238402 (before the fix) ==== # 1. Run claims outcome: checks-passed, but every CI check was CANCELLED $ fm-crew-state.sh cancelled-only state: done · source: run-step · checks green: PR ready for review # 2. Run claims outcome: checks-passed, but ZERO checks ever ran $ fm-crew-state.sh zero-checks state: done · source: run-step · checks green: PR ready for review # 3. Genuinely all-success run (control: green must still be green) $ fm-crew-state.sh all-success state: done · source: run-step · checks green: PR ready for review # 4. Terminalcompletedrun carrying no outcome at all $ fm-crew-state.sh no-outcome state: done · source: run-step · run completed ==== TARGET 9eb7125 (with the fix) ==== # 1. Run claims outcome: checks-passed, but every CI check was CANCELLED $ fm-crew-state.sh cancelled-only state: blocked · source: run-step · run reported checks-passed without green check evidence # 2. Run claims outcome: checks-passed, but ZERO checks ever ran $ fm-crew-state.sh zero-checks state: blocked · source: run-step · run reported checks-passed without green check evidence # 3. Genuinely all-success run (control: green must still be green) $ fm-crew-state.sh all-success state: done · source: run-step · checks green: PR ready for review # 4. Terminalcompletedrun carrying no outcome at all $ fm-crew-state.sh no-outcome state: unknown · source: run-step · run completed without an outcomeEvidence: New tests fail against base code (proves the fix from behavior)
# Target tests/fm-crew-state.test.sh grafted onto the BASE 1238402 tree, each case run in isolation. # "not ok" on base + "ok" on target = the test pins the fix from code. vs BASE test_checks_passed_cancelled_only_is_not_done not ok - checks-passed over cancelled checks must not be done (unexpected: 'state: done') vs BASE test_checks_passed_without_green_evidence_is_not_done not ok - checks-passed with zero checks must not be done (unexpected: 'state: done') vs BASE test_checks_passed_with_success_evidence_is_done ok - a checks-passed claim corroborated by success evidence is done <-- control, green not weakened vs BASE test_completed_without_outcome_is_not_done not ok - a completed run with no outcome must not be done (unexpected: 'state: done') vs BASE test_coarse_completed_row_is_not_done not ok - coarse completed without check evidence must not be done (unexpected: 'state: done') vs BASE test_ci_monitoring_no_checks_terminal_is_not_green not ok - terminal no-checks ci-monitor run -> working (missing: 'state: working') vs BASE test_ci_ready_done_log_without_evidence_stays_working not ok - uncorroborated ci-ready status log stays working (missing: 'state: working') vs BASE test_prefixed_red_marker_after_green_is_not_green not ok - a later prefixed cancelled marker overrides the earlier green (missing: 'state: working') vs BASE test_ci_failures_phrase_after_green_is_not_green not ok - a later CI failures line overrides the earlier green (missing: 'state: working')Evidence: Shared CI-ready scorer driven through its public command interface
# A green verdict requires >=1 completed success AND zero pending/failed/cancelled/skipped/never-run. $ bin/fm-ci-verdict-lib.sh (no checks at all) -> empty $ bin/fm-ci-verdict-lib.sh SUCCESS -> passed $ bin/fm-ci-verdict-lib.sh SUCCESS SUCCESS SUCCESS -> passed $ bin/fm-ci-verdict-lib.sh CANCELLED -> not-passed $ bin/fm-ci-verdict-lib.sh SUCCESS CANCELLED -> not-passed $ bin/fm-ci-verdict-lib.sh SKIPPED -> not-passed $ bin/fm-ci-verdict-lib.sh SUCCESS SKIPPED -> not-passed $ bin/fm-ci-verdict-lib.sh PENDING -> not-passed $ bin/fm-ci-verdict-lib.sh SUCCESS QUEUED -> not-passed $ bin/fm-ci-verdict-lib.sh FAILURE -> not-passed $ bin/fm-ci-verdict-lib.sh never-run -> not-passed $ bin/fm-ci-verdict-lib.sh SUCCESS never-run -> not-passed $ printf "SUCCESS\nSUCCESS\n" | bin/fm-ci-verdict-lib.sh -> passed $ printf "SUCCESS\nCANCELLED\n" | bin/fm-ci-verdict-lib.sh -> not-passed $ printf "\n\n" | bin/fm-ci-verdict-lib.sh -> emptyEvidence: Targeted suite result
Evidence: fm-watch-triage flake characterization (fails on base and target alike)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-crew-state.sh:313- The marker selector grep and nm_ci_marker_conclusion no longer cover the same phrases: the classifier recognizes "CI failures" (line 300) and "waiting for checks" (line 301), but neither appears in the grep -E alternation on line 313 that picks the marker. Because selection istail -1of grep-matching lines, a red line using only those phrases is invisible to the selector and an EARLIER green line wins. Concrete failing sequence: ci.log tail = ["all CI checks passed - still monitoring until merged or closed", "CI failures: 2 jobs red"] -> marker = the green line -> conclusion SUCCESS -> fm_ci_ready_verdict passed -> nm_ci_checks_state green -> state: done while CI is red. This is the same false-green class the change exists to prevent, and the drift is introduced here (the pre-change grep and case lists had identical coverage). Fix: keep the two lists in sync - add "CI failures" and "waiting for checks" to the grep alternation, or drop them from the classifier if the CLI never emits them.bin/fm-crew-state.sh:451- The coarse attribution path maps runs-list statuscompletedstraight to RUN_STATE=done ("run completed") without consulting the new scorer, so the authorized failure remains reachable through a sibling path in the same function. Concrete sequence: crew's own run ends with outcome=checks-passed where every check was cancelled (or there were zero checks);axi statusanswers with another crew's run (the exact condition the coarse fallback exists for, modelled by tests/fm-crew-state.test.sh:799);no-mistakes runsshows a head-matching rowcompleted fm/<branch> <sha>-> RUN_SOURCE=coarse -> state: done. That contradicts the invariant this change just documented at AGENTS.md:349 ("a checks-passed claim is done only when bin/fm-crew-state.sh scores at least one successful check"). The coarse path cannot score (RUN_OUT holds another run's id), so the earliest supported boundary is the coarse status mapping itself:completedwould have to become unknown/blocked rather than done. That trades false-green for false-unknown on every legitimately merged run, so it is a product-behavior decision rather than a mechanical fix.bin/fm-crew-state.sh:299- Five patterns in nm_ci_marker_conclusion are missing their leading*:"CI checks were cancelled"*and"cancelled without"*(line 299),"CI failures"*and"issues detected"*(line 300),"waiting for checks"*(line 301),"repository declares no CI"*(line 302). They only match when the phrase starts the line, so any real log line carrying a timestamp or indent prefix (e.g. "[12:00:04] CI checks were cancelled without reporting a verdict") falls through every arm to the empty conclusion. The mis-scoring is currently masked because nm_ci_checks_state collapsesemptyandnot-passedtonot-ready, but the lib deliberately keeps them as distinct verdicts, so the bug becomes live the moment a caller distinguishes them - and the new tests pass only because FM_FAKE_CI_LOGS fakes are unprefixed. Add the leading*to each.tests/fm-crew-state.test.sh:813- test_coarse_run_does_not_probe_other_branch_ci_log_for_ready_status still names the property "coarse run does not probe another branch's ci log", but with the coarse early-emit removed the assertions no longer distinguish probing from not-probing: FM_FAKE_CI_LOGS is a pending marker, so a regression that DID probe the other branch's log would read not-ready and still emitstate: working, and the test would pass. The guard for the original cross-branch bug is now vacuous. Restore it by setting FM_FAKE_CI_LOGS to the green marker ("all CI checks passed - still monitoring until merged or closed"): current code skips the block for coarse and stays working, while any code that probed the other branch's log would emit done and fail the existing assert_not_contains "state: done".bin/fm-bearings-snapshot.sh:246- bin/fm-ci-verdict-lib.sh declares itself the "ONE owner" of the green-verdict rule, but fm-bearings-snapshot.sh keeps an independent jq scorer over statusCheckRollup that reaches a different answer for conclusions the intent names as never-green: a check with status COMPLETED and conclusion SKIPPED (or NEUTRAL/STALE) is not in the failing list and is not pending, so an all-skipped rollup renders as checks "passing" in the captain's PR snapshot. This is pre-existing and untouched by the diff, and it scores GitHub rollup data rather than a no-mistakes run verdict, so it may be deliberately outside the stated scope ("the no-mistakes CI-ready verdict") - flagging it as the one immediately competing semantic owner rather than proposing a redesign.bin/fm-ci-verdict-lib.sh:32- fm_ci_ready_verdict duplicates the identical trim/success/short-circuit body across the args branch and the stdin branch, so the scoring rule now has two copies to keep in sync inside its own single-owner file. It collapses to one loop by normalizing the input first, e.g.if [ "$#" -eq 0 ]; then set -- $(cat); fi(or reading stdin into a positional list) and then running the single loop over "$@". Non-functional cleanup; behavior is unchanged.bin/fm-crew-state.sh:302- Consequence worth stating explicitly, not a defect: "no CI checks reported - still monitoring until merged or closed" used to score green, and now scores empty -> not-ready. On a repo that genuinely configures no CI, a crew therefore never reaches done from the ci-monitor path (it stays working until the PR merges, and a terminal checks-passed run reads blocked). That is exactly what the intent requires ("Zero checks is a distinct non-success state, not success"), so it is correct as specified - but it changes steady-state fleet surfacing for CI-less repos, since crew_absorb_class maps blocked/unknown to "none" and those wakes will surface to the captain instead of being absorbed.🔧 Fix: sync CI marker patterns and fail coarse completed closed
4 issues (1 warning, 3 infos) still open:
tests/fm-ci-verdict-lib.test.sh:36-out=$(score)invokes bin/fm-ci-verdict-lib.sh with zero arguments, which by contract enters the stdin branch (while IFS= read -r raw). Command substitution does not redirect stdin, and bin/fm-test-run.sh:1549 runs serial scripts asbash "$script" 2>&1 | tee "$out"with JOBS=1 by default, so the test inherits the invoker's stdin. Run from a terminal (or any runner whose stdin is an open pipe),readblocks and the whole suite hangs at this line instead of asserting. It only appears to work when stdin happens to be /dev/null or already at EOF. Fix:out=$(score < /dev/null)- the zero-checks assertion is unchanged (empty stdin still yieldsempty) and the hang is removed. The sibling blank-line case on line 39 already pipes its input and is fine.bin/fm-crew-state.sh:513- Sibling path check after closing the coarse hole: on the full path, whenaxi statusreportsstatus: completedwith NOoutcome:field, the no-outcome case arm still maps it to RUN_STATE=done / "run completed" without consulting the scorer - the same shape (done with zero check evidence) that COARSE_STATUS=completed was just changed to reject at line 456. I could not prove from source that the CLI ever emits a completed run without an outcome, so I am not claiming reachability; the supporting signal is bin/fm-teardown.sh:1251, which treats the OUTCOME (cancelled|failed|passed|checks-passed), not the status word, as the terminal marker - implying a completed row lacking an outcome is a shape the codebase already guards against elsewhere. If it is reachable, mapping it to unknown would match the coarse decision; if the CLI guarantees an outcome on every completed run, this arm is dead and can stay as is. Raising it as a question rather than a fix because it is the same product-behavior tradeoff (false-green vs false-unknown) already decided once for the coarse path.bin/fm-crew-state.sh:456- Downstream consequence of the accepted coarsecompleted-> unknown mapping, recorded rather than proposed for change: bin/fm-inactive-reconcile.sh:341-345 only creates a durable terminal-outcome record (and supervisor wake) forstate: doneorstate: failed, so an inactive crew whose finished run is attributed only coarsely now produces no terminal record where it previously produced a done record. The crew is still surfaced through the ordinary wake path (crew_absorb_class maps unknown to "none", so its wakes are not absorbed), and the coarse fallback is transient - a later scan whereaxi statusreturns the crew's own run takes the full path and scores properly. Fail-closed and consistent with the invariant the user chose; noting it because it was not visible when that mapping was decided.tests/fm-crew-state.test.sh:549- test_prefixed_red_marker_after_green_is_not_green asserts a genuine behavior (a later prefixed cancelled line must beat an earlier green one) but cannot fail if the leading*globs are removed again: with the anchored patterns, "[12:00:04] CI checks were cancelled without reporting a verdict" matches no case arm and yields the empty conclusion -> verdictempty, and nm_ci_checks_state collapsesemptyandnot-passedto the samenot-ready, so the crew readsworkingeither way. No crew-state-level test can distinguish them while that collapse exists; only a direct assertion on the classifier could. The companion test_ci_failures_phrase_after_green_is_not_green does not have this problem - it fails on a regression of the grep list. No action needed unless a caller ever distinguishes empty from not-passed.🔧 Fix: reject outcomeless completed run and unblock verdict test stdin
1 info still open:
bin/fm-crew-state.sh:606- Boundary note on the durable fix, no action recommended. After this round every run-attributed route to done requires evidence (line 475 outcome=passed/merged, 480 checks-passed + scored green, 528 ci-step + scored green, 550 status-log + scored green, and both bare-completedshapes now read unknown at 456 and 516). The one remaining way an unscored "checks green" claim still becomes done is the no-run fallback here: when NO run is attributable at all - worktree HEAD rewritten past the run head, or the run aged out ofno-mistakes runs --limit- and the pane is exact-idle, map_log_state promotes the crew's owndone: PR ... checks greenline to state done with source status-log. That is reachable after a cancelled-CI run whose crew already wrote that line, but crew-state has literally no run to score at that point, and step 4 of this file's header documents the fallback as intentional. There is no earlier shared boundary that would restore the invariant without removing the fallback itself, so this is the honest edge of the fix rather than a defect to patch.tests/fm-crew-state.test.sh:615- The intent requires "a cancelled-only run cannot produce checks-passed" be proven from code, but no test pinned it.test_ci_monitoring_cancelled_only_is_not_greenpasses against the base bin too (on base, "CI checks were cancelled" was an unrecognized marker, so it scoredunknownand nothing overrodeworking) — it was green for the wrong reason and guarded nothing. Theoutcome: checks-passed+ cancelled-checks path, where base actually reportedstate: done · checks green, had no coverage at all. Fixed by addingtest_checks_passed_cancelled_only_is_not_done, verified to fail against base 1238402 and pass on the target.bash tests/fm-ci-verdict-lib.test.sh— 4 scoring cases (cancelled, zero-checks, all-success, skipped/pending/failed/never-run), all passbash tests/fm-crew-state.test.sh— full suite passes including all CI-verdict cases and the addedtest_checks_passed_cancelled_only_is_not_doneAddedtest_checks_passed_cancelled_only_is_not_donetotests/fm-crew-state.test.shpinningoutcome: checks-passed+ cancelled ci.log → not doneManual end-to-end CLI drive ofbin/fm-crew-state.sh <id>over 7 scenarios against a real throwaway git worktree + fakedno-mistakes axi status/axi logs/runs, run against BOTH base 1238402 and target f7040c7 (bash drive-crew-state.sh <repo> <label>)Per-test regression proof: each new guard test run individually against the base bin/ (FM_ONE_TEST=<test> bash tests/one.sh) — confirmed 8 of them fail on the old behavior, which is how the untested cancelled case was foundMutation testing ofbin/fm-ci-verdict-lib.sh: (M1) empty→passed, (M2) CANCELLED treated as success, (M3) all-success→not-passed; each mutation caught by both suitesbash drive-scorer.sh <repo>— direct CLI transcript ofbin/fm-ci-verdict-lib.shover 25 arg/stdin conclusion setsbin/fm-test-run.sh --list --changed --base 12384026— confirmstests/fm-ci-verdict-lib.test.shandtests/fm-crew-state.test.share auto-selected from the changed pathsbash tests/fm-test-run.test.sh— exit 0, confirms the new test file's family registration is consistentbash tests/fm-documentation-audiences.test.sh,bash tests/fm-supervision-instructions.test.sh,bash tests/fm-ensure-agents-md.test.sh— doc-contract suites owning the edited AGENTS.md / docs/ files, all exit 0🔧 Fix: verify cancelled checks-passed guard; watcher flake is environmental
1 warning still open:
tests/fm-watch-triage.test.sh:393- tests/fm-watch-triage.test.sh fails non-deterministically in this sandbox, with a DIFFERENT case each run, on both the base tree (1238402) and the target (9eb7125). Target runs failed on 'provably-working signal did not advance its .seen-* suppressor', then 'watcher did not surface a working: note whose crew has no running pipeline and an idle pane', then 'watcher did not surface a turn-end whose crew is not provably working'; base runs failed on a fourth and fifth distinct case. The suite spawns a real background watcher and polls for exit/queue side effects, so it is timing-sensitive. It also cannot be affected by this change: every case stubs the crew-state reader through FM_CREW_STATE_BIN/FM_FAKE_CREW_STATE, so the real verdict code never runs inside it. The one overlapping case passed 3/3 when run in isolation on the target. Informational: pre-existing environmental flake, not a regression from this change, and not fixed here because it is out of this change's scope.bin/fm-test-run.sh tests/fm-ci-verdict-lib.test.sh tests/fm-crew-state.test.sh— both suites pass, 0 failuresManual end-to-end CLI transcript: realbin/fm-crew-state.sh <id>driven over throwaway git repos with a fakeno-mistakesservingaxi status/axi logs, run against base 1238402 bin/ and target 9eb7125 bin/, for cancelled-only, zero-checks, all-success, and no-outcome scenariosPer-test base comparison: targettests/fm-crew-state.test.shgrafted onto a base 1238402 tree, each case run in isolation —test_checks_passed_cancelled_only_is_not_done,test_checks_passed_without_green_evidence_is_not_done,test_completed_without_outcome_is_not_done,test_coarse_completed_row_is_not_done,test_ci_monitoring_no_checks_terminal_is_not_green,test_ci_ready_done_log_without_evidence_stays_working,test_prefixed_red_marker_after_green_is_not_green,test_ci_failures_phrase_after_green_is_not_greenall fail on baseControl check:test_checks_passed_with_success_evidence_is_donepasses on both base and target (green was not weakened)Manual scorer transcript:bin/fm-ci-verdict-lib.shpublic command driven over success / cancelled / skipped / pending / failed / never-run / zero-check inputs, via both argv and stdinbin/fm-test-run.sh --check-coverage— FM_TEST_COVERAGE ok total=147 (new test file is registered in a family/lane)bin/fm-test-run.sh tests/fm-watch-triage.test.sh tests/fm-inactive-reconcile.test.sh— downstream crew-state consumers; inactive-reconcile passes, watch-triage flakes on base and target alikebash tests/fm-watch-triage.test.shrepeated on target and on the base tree to characterize the flake✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.