fix(bin): derive fm-crew-state's passed-outcome PR detail from pr_state - #13
Merged
Merged
Conversation
outcome=passed no longer implies a merge (2026-09-20 firstmate-lint-debt-blocking-prs: passed reported alongside pr_state=open, merged=false). Read the run's own pr_state instead of asserting merged/closed from the outcome name, and classify passed-with-override as done instead of unknown (2026-09-20 firstmate-detect-dropped-ci-event).
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
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
Observed live on 2026-09-20 on firstmate-lint-debt-blocking-prs, with the
contradicting evidence in the same command output.
THE DEFECT. bin/fm-crew-state.sh maps the pipeline outcome "passed" to the literal
detail string "run passed: PR merged/closed". That assertion is not true in
general and was not true here: no-mistakes reported outcome "passed" AND
"pr_state: open" in the same status block, and the forge independently confirmed
state=open, merged=false, merged_at=null, merged_by=none. Nothing had been merged;
the captain had not even been asked for his word yet.
So firstmate told its own supervisor that a pull request had landed when it
demonstrably had not.
WHY IT MATTERS MORE THAN A WORDING SLIP. A supervisor reading "done - run passed:
PR merged/closed" has every reason to proceed to cleanup. Teardown has its own
landed-work test and would very likely have refused, so this is a near miss rather
than a loss - but the reading is the first domino, and relying on a downstream
guard to catch a false upstream claim is exactly the posture this fleet spent
2026-09-19 and 20 dismantling everywhere else.
SECOND LIVE INSTANCE, SAME MAPPING, OPPOSITE FAILURE. On
firstmate-detect-dropped-ci-event the pipeline reached outcome
"passed-with-override" - a real terminal outcome: every step completed, approved
past one confirmed-unrelated red check. The reader has no arm for it, so it
returned "state: unknown" for a task that had cleanly finished and reported
"done: PR ... holding for merge authority" in its own status log. The first
instance asserts a landing that did not happen; this one refuses to classify a
finish that did. Both come from the same case statement treating the outcome NAME
as the authority.
What Changed
bin/fm-crew-state.shno longer prints the fixed detailrun passed: PR merged/closedfor a terminalpassedoutcome. A newnm_outcome_pr_detailhelper reads the run record's ownpr_statefield and mapsmerged/open/closed/nonetoPR merged,PR open, not yet merged,PR closed, not merged, andno PR opened; any other value (including an absent field) readsPR merge state unknownrather than guessing in either direction.passed-with-overridearm to the same outcomecase, classifying it asstate: donewith detailrun passed (approved past a waived check): <pr detail>instead of letting it fall through to the unmapped-outcomeunknowndefault.AGENTS.md's state-reading line now listspassed-with-overridealongsidepassed/checks-passedas done.bin/fm-crew-state.shto record that the outcome name is not proof of a merge, with an audit note on which arms were checked and left unchanged, and added four tests intests/fm-crew-state.test.shcoveringpassed+pr_state: open,passed-with-override, a record with nopr_statefield, andpr_state: none.Risk Assessment
✅ Low: A tightly scoped read-only reporting fix whose five pr_state arms match the installed no-mistakes binary's actual vocabulary exactly, with no downstream parser depending on the changed detail text and four genuine regression tests that fail against the pre-fix code.
Testing
Ran the targeted
tests/fm-crew-state.test.shsuite (passes at the target commit) and proved the regression property by re-running each of the four new tests against a copy of the tree with the basebin/fm-crew-state.shrestored — all four fail before the fix. For product-level evidence I wrote a driver that executes the realbin/fm-crew-state.shover throwaway git worktrees with a fakeno-mistakes axi status, and captured the supervisor-facing state line for five run records under both the base and target scripts: the base prints the falserun passed: PR merged/closedfor thepr_state: openincident record andstate: unknownforpassed-with-override, while the target printsrun passed: PR open, not yet mergedandstate: done · run passed (approved past a waived check): PR open, not yet merged; a merged PR readsPR merged,pr_state: nonereadsno PR opened, and an absent field readsPR merge state unknown. Quoted YAML values parse identically, and no other script or doc depends on the old wording. This is a CLI/text surface with no rendered UI, so the reviewer-visible artifact is the before/after CLI transcript rather than a screenshot. Temp trees were removed and the worktree is clean.Evidence: Before/after CLI transcript: fm-crew-state.sh supervisor state line over five run records at base 7ce3cda vs target 2105bf7
Source: Before/after CLI transcript: fm-crew-state.sh supervisor state line over five run records at base 7ce3cda vs target 2105bf7
Evidence: Evidence driver: runs the real bin/fm-crew-state.sh over throwaway worktrees with a fake no-mistakes axi status
Source: Evidence driver: runs the real bin/fm-crew-state.sh over throwaway worktrees with a fake no-mistakes axi status
Evidence: Key before/after lines (excerpt)
Evidence: Each new test fails against the base script
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (3) ✅
bin/fm-crew-state.sh:494- nm_outcome_pr_detail'snone|""arm collapses an ABSENT pr_state with an explicit pr_state=none, and its no-URL leg then prints the positive claim "no PR opened". Concrete path: the helper's own comment (lines 479-481) states it supports "an older no-mistakes without the field", so pr_state is empty there; an emptyprfield on a terminal run is a shape this same file already anticipates (line 510 guards[ -n "$pr_url" ]before appending the URL, and tests/fm-crew-state.test.sh's run_failed fixture emitspr: ""). With outcome=passed and both fields empty the supervisor reads "state: done - run passed: no PR opened" for a run that in fact has a PR holding for the captain's merge word - a fact asserted from two absent fields, the same failure class the intent names ("the outcome NAME is not proof"). The pr_url-conditional sub-branch is additionally not required by the intent: the intent requires that the passed detail not assert an unproven PR fact, not that it surface the PR URL, and no sibling arm (merged/open/closed) surfaces it, so its output also reads oddly ("run passed: https://...fix(crew-state): read current state from the newest state-bearing line, not the last line #1, merge state unknown"). Smallest honest remedy is to remove the pr_url read from this helper and split the arm by evidence:none)-> "no PR opened" (the forge said so),"")-> "PR merge state unknown" (we do not know). Marked ask-user because the remedy removes a component and changes supervisor-facing wording the author chose deliberately.AGENTS.md:380- AGENTS.md:380 is the fleet-captain contract for reading bin/fm-crew-state.sh and enumerates the mapping as "passed or checks-passed is done; failed or cancelled is failed exactly as bin/fm-crew-state.sh prints it". The change adds a third done-producing outcome (passed-with-override) that this sentence does not list, so the documented enumeration is incomplete for exactly the outcome the intent's second live instance is about. The sentence is not false (it never claims to be exhaustive) and the captain is told to trust the printed state line, so this is informational: add passed-with-override to that enumeration.🔧 Fix: drop unproven PR claims from passed-outcome detail
2 issues (1 warning, 1 info) still open:
bin/fm-crew-state.sh:490- nm_outcome_pr_detail introduces two distinct "we don't know" paths where the intent needs one: the"")arm prints "PR merge state unknown" and the*)arm prints the raw field back as "PR state <value>". The intent requires only that a passed/passed-with-override detail stop asserting a landing the outcome name does not prove; it does not require surfacing an unrecognized pr_state spelling verbatim. A default is genuinely needed (without one an unmatched value yields an empty substitution and a dangling "run passed: "), so the strictly narrower form is a single default arm — fold*)into the unknown default so every non-merged/open/closed value reads "PR merge state unknown". Tradeoff worth the author's call: the verbatim arm is honest and never overclaims, and it does preserve the raw word (e.g.pr_state: nonecurrently reads "PR state none"), which folding would discard. Recommending removal of the component rather than hardening it, per the simplification pass; ask-user because it is the author's deliberate supervisor-facing wording.bin/fm-teardown.sh:1800- This change makes passed-with-override a first-class terminal outcome in the crew-state reader and in AGENTS.md:380, but bin/fm-teardown.sh's task_status_is_terminal_run still enumerates onlycancelled|failed|passed|checks-passed, and task_status_is_own_parked_run at line 1755 listspassed|checks-passedamong status words. I could not construct a path this change makes reachable: conclude_task_no_mistakes_run only proceeds when the run's outcome field is empty, then issues an abort, so the post-abort status it re-reads iscancelled; a passed-with-override reading there requires the run to have concluded with an override in the window between the parked check and the abort, and that race fails safe ("REFUSED … confirm it stopped"), never toward destructive cleanup. Pre-existing and not introduced here — noted only because this change just made that outcome word significant elsewhere in the repo. No action required.🔧 Fix: split pr_state none from the single unknown default
1 info still open:
bin/fm-crew-state.sh:715- The new audit comment says "The coarse ledger-fallback case below and the no-outcome status fallback further down were audited the same way". The no-outcome status fallback is indeed below (line 750+), but the coarse ledger-fallbackcase "$COARSE_STATUS"it names is ABOVE this comment, at lines 686-702 (inside theif [ "$RUN_SOURCE" = coarse ]branch that opens at 677). In a file where these comments are the navigational contract for the next reader, the pointer sends them the wrong way. Mechanical, non-functional: reword to "the coarse ledger-fallback case above and the no-outcome status fallback below".🔧 Fix: correct audit comment's above/below pointers
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-crew-state.test.sh(full crew-state suite at target commit — all pass)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— each run against a copy of the tree with basebin/fm-crew-state.shrestored, confirming each fails before the fixManual end-to-end drivercrew-state-pr-detail-driver.sh <path-to-bin/fm-crew-state.sh>: runs the real helper over throwaway git worktrees with a fakeno-mistakes axi status/tmux/herdr, for five run records (passed+pr_state open, passed-with-override, passed+pr_state merged, passed+pr_state none, passed with no pr_state field), executed against both base 7ce3cda and target 2105bf7Variant of the same driver with quoted YAML values (pr_state: "open",pr_state: "merged") to confirmstrip_quoteshandlinggrep -rn 'PR merged/closed|passed-with-override' --include='*.sh' --include='*.md' .to confirm no other script or doc still depends on the old wordingdocs/architecture.md:86- Judgment call, left unchanged deliberately. The fm-crew-state paragraph at docs/architecture.md:78-88 enumerates, with safety rationale, the ways the state line departs from the raw run record (ci log-tail override, terminal-failed held-green reclassification with the 2026-09-05 jr-voice rationale, coarse-fallback daemon-down -> unknown). This change adds a sibling invariant of the same class - the terminal outcome NAME is not proof of a merge, so passed and passed-with-override read their PR clause off the run's own pr_state field - and that invariant is not reflected there. I did not add it: no sentence in that paragraph is now false (it never claimed passed meant merged), the paragraph already defers exact rules to the script headers, and the placement policy prefers pointers over synchronizing a fact whose owners (bin/fm-crew-state.sh's header plus AGENTS.md:380 for the agent contract) are both already accurate. Flagging so a reviewer who reads that paragraph as an exhaustive contributor-facing list can disagree and ask for one deferring sentence.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.