diff --git a/AGENTS.md b/AGENTS.md index f4940d9915e..1815895369f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -377,7 +377,7 @@ Require the matching `resolved` event, forbid `--yes`, and require the worker to Resume fleet supervision immediately after the decision lands. Judge validation by the currently attributed run step through `bin/fm-crew-state.sh`, not by shell liveness or the last status event. -Running, fixing, or CI states remain working; parked approval or fix-review states require the worker to follow the active gate help; passed or checks-passed is done; failed or cancelled is failed exactly as `bin/fm-crew-state.sh` prints it - only that state line reclassifies an orphaned ci monitor after green checks as held-for-merge done, or a terminal failed record with the daemon unreachable as unknown, never the raw run record. +Running, fixing, or CI states remain working; parked approval or fix-review states require the worker to follow the active gate help; passed, passed-with-override, or checks-passed is done; failed or cancelled is failed exactly as `bin/fm-crew-state.sh` prints it - only that state line reclassifies an orphaned ci monitor after green checks as held-for-merge done, or a terminal failed record with the daemon unreachable as unknown, never the raw run record. A worker hand-editing, committing, aborting, or restarting during an active validation run duplicates pipeline ownership outside the supersession sequence above; steer it back to the gate response flow. The worker reports the PR when CI first becomes green rather than waiting for merge monitoring to finish. diff --git a/bin/fm-crew-state.sh b/bin/fm-crew-state.sh index 331c45c2b84..10fa855d11c 100755 --- a/bin/fm-crew-state.sh +++ b/bin/fm-crew-state.sh @@ -50,9 +50,15 @@ # the ledger has been asked whether a live sibling run exists. # The run-step is AUTHORITATIVE: running/fixing -> working, ci -> working, # awaiting_approval/fix_review -> parked (with gate findings), terminal -# passed/checks-passed -> done, failed/cancelled -> failed. EXCEPT: while -# the active step is ci, `axi status` alone cannot tell "still waiting on -# checks" from "checks green, waiting on merge" (see nm_ci_checks_state) - +# passed/checks-passed/passed-with-override -> done, failed/cancelled -> +# failed. The outcome NAME is not proof of a merge: passed and +# passed-with-override read their PR detail off the run's own pr_state +# field (nm_outcome_pr_detail) rather than asserting merged/closed from +# the outcome alone (2026-09-20 firstmate-lint-debt-blocking-prs +# incident). An unmapped terminal outcome still reads unknown rather than +# a guessed done/failed. EXCEPT: while the active step is ci, `axi +# status` alone cannot tell "still waiting on checks" from "checks green, +# waiting on merge" (see nm_ci_checks_state) - # a ci-step log-tail check overrides working -> done once checks read # green, so a green PR is never silently read as still-validating. And a # terminal FAILED run whose only failure is the ci monitor step, after @@ -463,6 +469,29 @@ EOF [ "$(nm_ci_checks_state)" = green ] } +# Honest PR-state clause for a terminal passed/passed-with-override outcome. +# The outcome NAME alone is not proof of a landing: a run can reach a passed +# outcome while still holding for the captain's merge word (2026-09-20 +# firstmate-lint-debt-blocking-prs incident - outcome=passed reported +# alongside pr_state=open in the same status block, with the forge +# independently confirming merged=false, merged_at=null). Read the run's own +# pr_state field instead of asserting a merge the outcome name does not +# prove. merged, open, closed and none are each a fact the run record states; +# every other value - an absent field from an older no-mistakes, a +# not-yet-observed state, a spelling this reader does not know - is a single +# unknown, never a guess in either direction. +nm_outcome_pr_detail() { + local pr_state + pr_state=$(strip_quotes "$(nm_field pr_state)") + case "$pr_state" in + merged) printf 'PR merged' ;; + open) printf 'PR open, not yet merged' ;; + closed) printf 'PR closed, not merged' ;; + none) printf 'no PR opened' ;; + *) printf 'PR merge state unknown' ;; + esac +} + # Reclassify a terminal failed run as done (held-for-merge) when # nm_failed_run_is_green_held_ci matches, surfacing the run's PR URL so the # supervisor reads the concrete review-ready outcome instead of a failure. @@ -513,10 +542,14 @@ nm_effective_ci_step_status() { } # Root cause of the PR #252 incident (2026-07): for a repo where merge is left -# to the captain, no-mistakes' ci step (and therefore top-level status/outcome) -# stays "running" for the ENTIRE CI-monitor phase, including long after GitHub -# reports every check green - it only reaches outcome=passed once the PR is -# actually merged (or failed/cancelled if closed). `axi status`'s steps[] table +# to the captain, no-mistakes' ci step (and therefore top-level status) can +# stay "running" for the ENTIRE CI-monitor phase, including long after GitHub +# reports every check green. Do not read outcome=passed itself as proof the PR +# was merged: the 2026-09-20 firstmate-lint-debt-blocking-prs incident +# observed outcome=passed reported alongside pr_state=open, so a passed +# terminal outcome can still be holding for the captain's merge word +# (nm_outcome_pr_detail is the one owner of deriving an honest reading from +# the run's own pr_state field for that case). `axi status`'s steps[] table # never distinguishes "still waiting on checks" from "checks green, waiting on # merge": both read as plain `ci,running,...`. The only place that transition is # recorded is the ci step's own log text, e.g. "all CI checks passed - still @@ -676,10 +709,20 @@ if [ "$HAVE_RUN" = 1 ]; then has_gate=0 nm_has_gate && has_gate=1 + # 2026-09-20 audit of every arm below: checks-passed's "PR ready for + # review" and the failed/cancelled arms never assert a landing, so they + # stay as they were; only passed and passed-with-override previously + # overclaimed a merge from the outcome name alone. The coarse + # ledger-fallback case above and the no-outcome status fallback below + # were audited the same way and likewise left unchanged: "run + # completed"/"ci running"/"validating (...)" assert nothing about a merge. if [ -n "$outcome" ]; then case "$outcome" in - passed) RUN_STATE="done"; RUN_DETAIL="run passed: PR merged/closed" ;; + passed) RUN_STATE="done"; RUN_DETAIL="run passed: $(nm_outcome_pr_detail)" ;; checks-passed) RUN_STATE="done"; RUN_DETAIL="checks green: PR ready for review" ;; + passed-with-override) + RUN_STATE="done" + RUN_DETAIL="run passed (approved past a waived check): $(nm_outcome_pr_detail)" ;; failed) if nm_reclassify_failed_run_as_held_green; then :; else RUN_STATE=failed; RUN_DETAIL="run failed" diff --git a/tests/fm-crew-state.test.sh b/tests/fm-crew-state.test.sh index 356f126c94f..555c8433622 100755 --- a/tests/fm-crew-state.test.sh +++ b/tests/fm-crew-state.test.sh @@ -378,6 +378,54 @@ outcome: passed EOF } +# The 2026-09-20 firstmate-lint-debt-blocking-prs incident shape: outcome +# passed with pr_state open, still holding for the captain's merge word. +run_passed_pr_open() { # + cat < + cat < + cat < cat </dev/null + fm_write_meta "$d/state/feat-passed-open.meta" "window=fm:fm-feat-passed-open" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_passed_pr_open fm/feat-passed-open)" + local out; out=$(run_crew_state "$d" feat-passed-open) + assert_contains "$out" "state: done" "passed run with an open PR still reads done" + assert_contains "$out" "PR open, not yet merged" "an open PR's detail must not claim it merged" + assert_not_contains "$out" "PR merged" "an open PR must never be reported as merged" + pass "terminal passed run with an open PR reads its honest pr_state, not a guessed merge" +} + +# Pins the 2026-09-20 firstmate-detect-dropped-ci-event fix: an unmapped +# terminal outcome must not degrade to unknown when the run plainly finished. +test_terminal_passed_with_override_reads_done_not_unknown() { + reset_fakes + local d; d=$(new_case passed-override) + make_repo_on_branch "$d/wt" fm/feat-passed-override + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-passed-override.meta" "window=fm:fm-feat-passed-override" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_passed_with_override fm/feat-passed-override)" + local out; out=$(run_crew_state "$d" feat-passed-override) + assert_contains "$out" "state: done" "passed-with-override is a real terminal outcome, not unknown" + assert_not_contains "$out" "state: unknown" "a finished run must never read as unknown" + assert_contains "$out" "PR open, not yet merged" "passed-with-override still reads its honest pr_state" + pass "terminal passed-with-override run is classified done, never unknown" +} + +# A run record without the pr_state field proves nothing about the PR either +# way; the detail must say so rather than assert a landing or its absence. +test_terminal_passed_absent_pr_state_reads_unknown() { + reset_fakes + local d; d=$(new_case passed-no-pr-state) + make_repo_on_branch "$d/wt" fm/feat-passed-no-pr-state + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-passed-no-pr-state.meta" "window=fm:fm-feat-passed-no-pr-state" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_passed fm/feat-passed-no-pr-state)" + local out; out=$(run_crew_state "$d" feat-passed-no-pr-state) + assert_contains "$out" "state: done" "a passed run without pr_state still reads done" + assert_contains "$out" "PR merge state unknown" "an absent pr_state must read unknown" + assert_not_contains "$out" "no PR opened" "an absent pr_state must not claim no PR was opened" + assert_not_contains "$out" "PR merged" "an absent pr_state must not claim a merge" + pass "terminal passed run without pr_state reports the merge state as unknown" +} + +# pr_state: none is the record stating no PR was ever opened - a known fact +# that must stay distinct from the absent-field unknown default. +test_terminal_passed_pr_state_none_reads_no_pr_opened() { + reset_fakes + local d; d=$(new_case passed-pr-none) + make_repo_on_branch "$d/wt" fm/feat-passed-pr-none + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-passed-pr-none.meta" "window=fm:fm-feat-passed-pr-none" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_passed_pr_none fm/feat-passed-pr-none)" + local out; out=$(run_crew_state "$d" feat-passed-pr-none) + assert_contains "$out" "state: done" "a passed run that opened no PR still reads done" + assert_contains "$out" "no PR opened" "pr_state none is a known fact, not an unknown" + assert_not_contains "$out" "merge state unknown" "a stated none must not read as the unknown default" + assert_not_contains "$out" "PR merged" "a run that opened no PR must never report a merge" + pass "terminal passed run with pr_state none reports no PR opened, not unknown" +} + test_terminal_failed() { reset_fakes local d; d=$(new_case failed) @@ -2684,6 +2798,10 @@ test_ci_fixing_after_green_stays_working test_top_level_fixing_ci_running_after_green_stays_working test_top_level_fixing_done_log_stays_working test_terminal_passed +test_terminal_passed_open_pr_reads_honest_detail +test_terminal_passed_with_override_reads_done_not_unknown +test_terminal_passed_absent_pr_state_reads_unknown +test_terminal_passed_pr_state_none_reads_no_pr_opened test_terminal_failed test_terminal_failed_ci_orphan_after_green_reads_done test_terminal_failed_ci_orphan_status_only_reads_done