fix(bin): claim a merged PR only when the pr step and every step after it completed - #3527
fix(bin): claim a merged PR only when the pr step and every step after it completed#3527loom-loki wants to merge 4 commits into
Conversation
…r it completed fm-crew-state mapped a no-mistakes `outcome=passed` run straight to the fixed detail "run passed: PR merged/closed". A run reaches outcome=passed once its steps finish without failing, which includes a run whose `pr` and `ci` steps were SKIPPED - such a run opened and merged nothing, yet the reader asserted a merge it had never observed. State the rule generally rather than special-casing one step: the merge claim is made only when the run's own `pr` step AND every step it reports after that one read `completed`. A skipped `pr` step means nothing was opened; a completed `pr` step whose later step never ran means a PR was opened but never carried to a merge. In both cases the detail names the step that stopped short instead of asserting a merge. No step name after `pr` is hardcoded - the steps a merge claim depends on are read from the run's own step order. Generalises nm_ci_step_status into nm_step_rows/nm_step_status, keyed on the NUMERIC third column so a findings row is never mistaken for a step row (the findings table is emitted first, so a finding whose id is `pr` would otherwise shadow the real `pr` step and report its severity as the step status), and adds nm_passed_pr_detail. Tests cover the skipped `pr` step, the completed `pr` step with a skipped `ci` step, `pr` as the last reported step, a run with no `pr` row, the still-valid merged case, and a negative case that fails if the findings-row guard is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NATZQwEuv3ztPckNnUtSM
…-row evidence comments
Confidence Score: 4/5The merge-reporting fix is broadly sound, but the step parser should be constrained to the steps table before merging. A valid numeric file path on a finding whose id matches Files Needing Attention: bin/fm-crew-state.sh Reviews (1): Last reviewed commit: "no-mistakes(document): record observed m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # about a version, not a law. | ||
| nm_step_rows() { | ||
| printf '%s\n' "$RUN_OUT" \ | ||
| | grep -E "^[[:space:]]*[A-Za-z0-9_-]+,[[:space:]]*\"?[A-Za-z0-9_-]+\"?[[:space:]]*,[[:space:]]*[0-9]+[[:space:]]*," |
There was a problem hiding this comment.
If a pr finding has a numeric file path such as 123, the global numeric-third-column predicate accepts it as a step row and nm_step_status selects it before the genuine pr row, causing a completed run to report the finding severity as the PR-step status and incorrectly mark the merge state unknown.
There was a problem hiding this comment.
Thanks for flagging this - I checked it against real no-mistakes axi status output rather than reasoning about the format, and the shadowing path does not open on v1.60.2.
The predicate's third column requires bare digits ([0-9]+, no quotes accepted). The TOON encoder quotes a string field whose value would otherwise read as a number, so a findings row for a file named 123 renders quoted.
To confirm rather than assume, I put a file literally named 123 in a scratch repo and let the review step report on it. All four findings rendered like this (v1.60.2, eb4e379):
findings[4]{id,severity,file,action,description}:
unquoted-dir-mass-delete,error,"123",auto-fix,"..."
self-labeled-bug-comment,warning,"123",ask-user,"..."
mtime-boundary-off-by-one,info,"123",ask-user,"..."
extra-args-silently-ignored,info,"123",ask-user,"..."
The file column is "123" - quoted. Feeding those exact rows through the predicate rejects every one of them. The same encoder rule is visible elsewhere in the same output: agent_pid is a numeric-looking string and renders quoted ("185869"), while genuine numbers such as duration_ms render bare (0, 22991).
So the row shape your comment describes - pr,warning,123,auto-fix,..., with a bare numeric third column - would indeed be accepted by the predicate and would shadow the genuine pr row. But that is not a shape this version emits; it emits pr,warning,"123",auto-fix,..., which the predicate rejects.
One caveat worth stating plainly, since it is the honest limit of the evidence: this is a fact about one version's output shape, not a guarantee. If the encoder ever stopped quoting numeric-looking strings, the predicate would need a quote-aware third column. The negative test added in this PR (test_findings_row_is_not_read_as_a_step_row) pins the general property that a findings row must never be read as a step row, so a regression of that kind has somewhere to fail.
1 participant
Intent
Contribute an already-written, already-proven fix upstream to this project. Nothing to design, nothing new to implement.
THE BUG: bin/fm-crew-state.sh claimed a no-mistakes run had merged a pull request when it had not. A run reaches outcome=passed once its steps finish without failing, which INCLUDES a run whose pr and ci steps were SKIPPED. Such a run opened and merged nothing, yet the reader mapped outcome=passed to the fixed detail "run passed: PR merged/closed", asserting a merge it had never observed. Verified on no-mistakes v1.60.2: run 01M1EA5NJVP18AE7SPY5MBYW42 reported pr,skipped and ci,skipped under outcome=passed while the forge still had that branch PR open and unmerged, and the work was reported as landed.
THE FIX (deliberate design decision): state the rule GENERALLY rather than special-casing one step. The merge claim is made only when the run own pr step AND every step it reports after that one actually completed. This is deliberate: the same false-merge claim existed one trigger over, when ci rather than pr was skipped, and both are closed by the one general rule. Do NOT reintroduce a per-step special case, and do NOT hardcode any step name after pr - the steps a merge claim depends on are read from the run own step order, because axi status carries no merge signal to key on directly. A run that passed with the pull request opened but checks unverified now reports that honestly instead of claiming a merge.
Implementation: generalises the previous ci-only nm_ci_step_status into nm_step_rows/nm_step_status with a NUMERIC-third-column guard, and adds nm_passed_pr_detail. The numeric guard is load-bearing and deliberate: the findings table is emitted BEFORE the steps table, so without it a finding whose id is
pris matched first and shadows the genuine pr step row, reporting the finding SEVERITY as the step status. nm_ci_step_status is deliberately kept as a narrow wrapper that returns only running/fixing so its caller falls through unchanged.ACCEPTED REFINEMENTS ALREADY APPLIED IN THIS BRANCH (do not undo, do not re-litigate):
<step>,running,0,0in the steps table beside<step>,running,<duration>,...in a separate active_steps table whose third column is a duration. The fixture and the comment both record that this shape was observed on that version rather than inferred, because a vendor output shape is a fact about a version, not a law. Mutation-tested: break the reader so it stops matching the active run steps row and that test fails with the exact incident signature, a green PR reading as still validating.TESTS: tests/fm-crew-state.test.sh covers the skipped pr step, a completed pr step with a skipped ci step, pr as the last reported step, a run with no pr row at all, the still-valid genuinely-merged case, the active-run both-tables case above, and a NEGATIVE case for the findings-row guard. That negative case is deliberate and has been verified to fail when the guard is removed - an earlier attempt at it would have passed with the guard gone, which is exactly what it must not do.
SCOPE CONSTRAINT (explicit, deliberate): exactly two files - bin/fm-crew-state.sh and tests/fm-crew-state.test.sh. Touching a third file is out of scope. This fix carries no dependency on any forge-specific or provider-specific support code and must not acquire one.
DELIVERY: this is a contribution to somebody else project. The pull request body must stand on its own for a maintainer with none of this context: what the bug is, how it reproduces, why the fix is stated generally rather than per-step, and what the tests prove. Do not merge. Stop when the pull request is open and its checks are green.
NOTE ON THIS RUN: a previous run of this same branch reached the push step and failed there on a transient SSH authentication error from the forge; the identical command has since succeeded repeatedly. The two review-fix commits from that run are preserved on this branch and custody was returned through the supported recovery path. Nothing else changed.
What Changed
bin/fm-crew-state.shmapped a terminaloutcome=passedrun to the fixed detailrun passed: PR merged/closed, asserting a merge it had never observed. A run reachesoutcome=passedonce its steps finish without failing, which includes a run whoseprandcisteps were skipped - the shape no-mistakes takes when it cannot resolve a push provider, so nothing was opened or merged. Reproduced on no-mistakes v1.60.2: run01M1EA5NJVP18AE7SPY5MBYW42reportedpr,skipped,0,17andci,skipped,0,16underoutcome=passedwhile the forge still had that branch's pull request open and unmerged, and the work was reported as landed.nm_passed_pr_detailclaims a merge only when the run's ownprstep and every step the run reports after it readcompleted; otherwise the detail names the step that stopped short and ends inmerge state unknown to the run. The rule is stated generally rather than special-cased on one step because the same false claim existed one trigger over (axi run --skip=cileavesprcompleted,ciskipped, PR open), and no step name afterpris hardcoded - the steps a merge claim depends on are read from the run's own step order, sinceaxi statuscarries no merge signal to key on directly. Supporting change: the ci-onlynm_ci_step_statusis generalised intonm_step_rows/nm_step_statuswith a numeric-third-column guard, needed because the findings table is emitted before the steps table and a finding whose id isprwould otherwise shadow the real step row and report the finding's severity as the step status;nm_ci_step_statusremains as a narrow wrapper returning onlyrunning/fixingso its caller falls through unchanged.tests/fm-crew-state.test.shcovering a skippedprstep, a completedprstep with a skippedcistep,pras the last reported step, a run with noprrow at all, the still-valid genuinely-merged run (fixture from real run01M1669Y82JTHWEBSG7PR2TKNH), an active run rendering both thestepsandactive_stepstables (fixture built fromaxi statuscaptured mid-step on live v1.60.2 runs, pinning that the numeric guard did not break the green-checks override behind the PR fix(backends): scope zellij tabs by firstmate home #252 incident), and a negative case that fails if the findings-row guard is removed.Risk Assessment
✅ Low: A well-bounded two-file change to one read-only reporting helper that replaces an over-claiming fixed string with a conservative, generally-stated derivation, backed by fixtures copied from real observed output and a negative test for the load-bearing guard; every traced input produces a correct detail and no downstream consumer parses the changed string.
Testing
I reproduced the reported bug end-to-end before testing the fix: running bin/fm-crew-state.sh from the base commit against the real v1.60.2
axi statusoutput for run 01M1EA5NJVP18AE7SPY5MBYW42 (pr and ci skipped, outcome passed) printsrun passed: PR merged/closed, and this branch printsrun passed, PR step skipped: no PR was opened or merged, merge state unknown to the run; the same before/after holds for the--skip=cishape, while the genuinely landed run still reads as merged on both. I followed the detail through to the surfaces a captain and firstmate actually see - the /bearings Underway row and the fleet-snapshot JSON - and confirmed the longest new detail (87 chars) survives the board's 90-char truncation with its suffix intact. Each of the five new merge-claim tests fails individually against the pre-fix reader with the exact false-merge signature and passes here, and the two invariant tests that necessarily pass both before and after are pinned by mutation instead: widening the numeric-third-column guard makes the findings-row test fail with a finding severity reported as the pr step status, and narrowing it so an active run's own steps row stops matching makes the active-run test fail with a green PR reading as still validating. The targeted suite (65 cases) and the downstream fleet-snapshot view suite are green, no other suite asserts on this detail string, and the worktree is clean at d813ec0 with all mutations reverted. This is a CLI text-line change with no rendered UI surface, so the evidence is CLI transcripts rather than screenshots.Evidence: Before/after reader transcript over the real observed axi status shapes
Evidence: Corrected detail on the captain-facing /bearings row and the fleet-snapshot JSON
Evidence: Regression tests failing pre-fix, plus both guard mutations failing their tests
Evidence: The bug and the fix, in one pair of lines
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-crew-state.test.sh- full targeted suite, 65 cases green, including all 7 new/changed casesbash tests/fm-fleet-snapshot-view.test.sh- downstream consumer of the crew-state detail, greenManual before/after E2E:bin/fm-crew-state.sh <id>run from base 8988af2 and from d813ec0 over four realaxi statusshapes (pr+ci skipped / pr completed+ci skipped / genuinely merged / findings row shadowing the pr step), via a hermetic fakeno-mistakesand a real throwaway git worktreeManual E2E of the active-run both-tables shape (ci,running,0,0in steps besideci,running,1m40s,...in active_steps) with a green ci log tail, confirming the green-checks override still firesRegression proof: base-commit reader swapped in, then each new test run individually (test_terminal_passed_pr_skipped_claims_no_merge,test_terminal_passed_ci_skipped_claims_no_merge,test_terminal_passed_with_pr_as_last_step_claims_no_merge,test_terminal_passed_without_a_pr_row_claims_no_merge,test_findings_row_is_not_read_as_a_step_row) - all five fail pre-fixMutation A:nm_step_rowsthird column widened[0-9]+->[^,]+;test_findings_row_is_not_read_as_a_step_rowfails and the reader emitsrun passed, PR step warning: ...Mutation B:nm_step_rowsthird column narrowed to[1-9][0-9]*so an active run'sci,running,0,0no longer matches;test_ci_monitoring_with_active_steps_table_still_reads_the_step_rowfails withstate: working · validating (running)Downstream surfaces:bin/fm-bearings-snapshot.shUnderwaydoingrow andbin/fm-fleet-snapshot.sh --json.tasks[].current_staterendered for all three passed-run shapesgit status --porcelainafter every mutation/restore cycle - worktree clean at d813ec0bin/fm-crew-state.sh:316- Evidence-count mismatch between the change's narrative and its code comments. The user intent states the active-run shape was "sampled on four different running steps" on v1.60.2 (eb4e379), but both evidence comments - bin/fm-crew-state.sh:316 and tests/fm-crew-state.test.sh:407 - say "sampled on three different running steps". The recorded decision transcript quotes exactly three verbatim captures (test, document, review, with durations 7s/18s/19s); the fourth duration mentioned there (1m40s) appears only as the illustrative fixture value the instruction introduced with "e.g.", so I could not confirm it as an observed fourth sample. I left the comments at "three" rather than raising them to "four", because that is the number the available evidence supports and inflating it would assert a capture I cannot verify. The author holds the captures and should reconcile the two: the PR body will be written from the intent and must stand on its own for a maintainer who will also read the code comment, so a maintainer comparing them currently sees the change's own evidence claim contradict itself. Fixing it is a one-word edit in whichever of the two is wrong (both comments if four is correct, the intent/PR body if three is).🔧 Fix: record observed minute-scale durations in step-row evidence comments
1 info still open:
bin/fm-crew-state.sh:316- Residual, and outside this phase's edit scope: the user intent still says the active-run shape was "sampled on four different running steps", while both evidence comments now say three - and the author has explicitly confirmed three is the true count (three distinct running step names: review, test, document) and that the intent narrative was wrong. Since the pull request body is written from the intent and must stand on its own for an upstream maintainer who will also read bin/fm-crew-state.sh:316 and tests/fm-crew-state.test.sh:407, the PR body must say three, not four; copying the intent verbatim would ship a PR whose own evidence claim contradicts the code it delivers. I can only edit documentation and doc comments, so I cannot correct the intent text or the PR body from here - flagging it for whoever authors the PR body.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.