fix(bin): attribute a live no-mistakes run whose fix commits are not local - #1996
Open
mackcee wants to merge 4 commits into
Open
fix(bin): attribute a live no-mistakes run whose fix commits are not local#1996mackcee wants to merge 4 commits into
mackcee wants to merge 4 commits into
Conversation
…local bin/fm-crew-state.sh reported a healthy, actively running no-mistakes pipeline as `failed`. That verdict is one of the inputs firstmate uses to decide whether a worker needs recovery, so it can provoke a teardown or relaunch of a worker that is mid-pipeline with unpushed work, and it can send a false failure report. Root cause: a run in its pre-push phase commits every gate fix round into no-mistakes' own gate repo, never into the crew worktree, so the head it reports is not an object in the crew worktree at all. The code-identity rule cannot bind by either of its tests, attribution falls through to the coarse runs-list fallback, and that fallback skipped the live `running` row for the branch and kept scanning until an older superseded `failed` run matched the untouched worktree head exactly. An older terminal run won precisely because the live one had moved ahead. Two changes, in bin/fm-nm-run-lib.sh as the one owner of the rule: - Add a second, independent proof of ownership taken from no-mistakes' own branch_sync object, which `axi status` emits only when the run relates to the invoking worktree. It is required to state live pipeline custody of this exact branch, head, and run, and that the run was submitted from exactly the commit checked out here. The history rule stays exactly as strict as it was: an unresolvable head is an absence of evidence, not evidence of ownership, and the same absence is produced by a rewritten branch. Keeping it strict also matters because fm-teardown.sh shares it, where a false positive lets teardown abort a run it does not own. - In the coarse runs-list fallback, let the newest row for the branch decide outright. A branch's runs are sequential, so an older run is superseded evidence and can never outrank the newest one, whichever sha happens to be visible. fm-teardown.sh adopts the same composed rule, so the two callers cannot drift and an orphaned parked run whose head advanced is now abortable. Verified live against a running pipeline (no-mistakes v1.45.4): the helper now reports `working - validating (running)` matching `axi status` while the unfixed helper still reports `failed`. Six regression cases cover both directions. The custody-binds case and the coarse superseded-row case both fail without this change. The four refusal cases pin that a released, stale-cached, differently-submitted, or different-run custody claim still refuses attribution; deleting each load-bearing conjunct was checked to fail exactly its own case.
… fm-nm-run-lib header
Author
|
CI note: The failure is in Evidence that it is unrelated to this change:
Outside contributors cannot re-run jobs on this repository ( |
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
Contribute an upstream bug fix to kunchenguid/firstmate: bin/fm-crew-state.sh reports a healthy, actively running no-mistakes pipeline as 'failed'.
Why it matters. That verdict is one of the inputs firstmate uses to decide whether a worker needs recovery, so a false 'failed' can provoke teardown or relaunch of a worker that is mid-pipeline with unpushed work, and can send a false failure report to the operator.
Root cause, and it is not a rare corner. While a run is in its pre_push phase, no-mistakes commits every gate fix round into its own gate repo, never into the crew worktree. The head the run reports is therefore not an object in the crew worktree at all, so the existing code-identity rule cannot bind by either of its tests. Attribution then falls through to the coarse runs-list fallback, and that fallback skipped the live 'running' row for the branch and kept scanning older rows until a superseded 'failed' run matched the untouched worktree head exactly. An older terminal run won precisely because the live one had moved ahead. This is the normal state of every no-mistakes run that has made a fix commit and not pushed yet.
The fix, with bin/fm-nm-run-lib.sh as the single owner of the rule. First, add a second independent proof of ownership read from no-mistakes' own branch_sync object, which axi status emits only when the reported run relates to the invoking worktree; it must state live pipeline custody of this exact branch, head and run, and that the run was submitted from exactly the commit checked out here. Second, in the coarse runs-list fallback, let the newest row for a branch decide outright, since a branch's runs are sequential and an older run is superseded evidence. bin/fm-teardown.sh adopts the same composed rule so the two callers cannot drift.
Deliberate decisions a reviewer should know. The history rule stays exactly as strict as it was: an unresolvable head is an ABSENCE of evidence, not evidence of ownership, and the same absence is produced by a rewritten branch, so it must keep refusing both. That strictness matters because fm-teardown.sh shares the rule, where a false positive would let teardown abort a run it does not own; ownership for the live case is established by the branch_sync custody proof instead. Every conjunct in that custody predicate is required so the predicate stays a proof rather than a hint; three carry the safety weight and are each pinned by their own test, and two are deliberately kept as fail-closed consistency checks on a cached block even though nothing coherent can break them alone.
About the test fixtures. The commit SHAs in tests/fm-crew-state.test.sh are deliberately synthetic placeholders (a1b2c3d4, e5f6a7b8, c9d0e1f2) chosen so they cannot resolve to any real object, which is exactly what the 'head is not an object in this worktree' cases require. Do not replace them with real-looking or repo-resolvable SHAs. The observations recorded in comments come from a live pipeline and are deliberately not tied to a named task.
Constraints: public upstream repo conventions - one sentence per line in tracked Markdown, plain dash never an em dash, no agent as commit co-author, bin/*.sh shellcheck-clean via bin/fm-lint.sh, tests colocated in tests/ as .test.sh exercising behaviour rather than implementation source bytes, and docs/ changes keeping the one-owner rule so architecture and scripts docs stay cross-references rather than second copies of the rule. Scope is this attribution fix alone; an unrelated test-runner collation fix ships as its own separate PR and must not be pulled in. Add every fix as a NEW commit on top; never amend or rewrite an existing commit on this branch.
What Changed
bin/fm-nm-run-lib.shgains a second, independent proof of run ownership:fm_nm_run_owns_worktree_by_branch_syncreads no-mistakes' ownbranch_syncobject fromaxi statusand binds a run only when it states live pipeline custody (state: pipeline_owned) of this worktree's exact branch, head, run id, and submitted head.fm_nm_status_matches_worktreecomposes it with the unchanged history rule (either proof sufficient), and bothbin/fm-crew-state.shandbin/fm-teardown.shnow attribute runs through that single composed rule instead of callingfm_nm_head_matches_worktreedirectly. The history rule stays strict: an unresolvable head is still refused.no-mistakes runsfallback inbin/fm-crew-state.shno longer scans past a non-binding row. The newest row for a branch now decides outright, so a superseded olderfailedrun can no longer outrank the branch's liverunningrow.run:block via newfm_nm_block,fm_nm_without_block, andfm_nm_run_fieldhelpers, so keys thatbranch_syncalso carries (head:,status:,id:) cannot answer a question about the run;fm-crew-state.shscans anaxi statuspayload with the cachedbranch_syncblock excised for all run-detail reads. Tests intests/fm-crew-state.test.shandtests/fm-teardown.test.shcover the pre-push custody case, stale/cross-run custody caches, released and rewritten-branch refusals, and teardown's abort/no-abort split;docs/architecture.mdanddocs/scripts.mdare updated to point atfm-nm-run-lib.shas the one owner of both rules.Risk Assessment
✅ Low: The change is well-bounded to one attribution rule with a single owner, every widening conjunct is required and fail-closed in the unsafe direction, and the new regression cases genuinely fail without the fix (I traced the failing read for each rather than trusting the fix summaries); the only finding is a non-reachable consistency gap in two teardown greps.
Testing
I reproduced the reported defect end to end and confirmed the fix at the operator-visible surface: driving the real bin/fm-crew-state.sh over a throwaway git worktree with a fake no-mistakes emitting the live pre-push axi status payload, base bin/ prints
state: failed · source: run-step · run failedfor a healthy running pipeline (andstate: parkedoff a stale status log in the axi-status path), while target bin/ printsstate: workingin both, attributed to the run step via branch_sync custody. I ran tests/fm-crew-state.test.sh in full (all 58 cases pass) and tests/fm-teardown.test.sh in full, and additionally ran each of the 9 new crew-state cases and both new teardown custody cases individually against base bin/ and then target bin/, which shows the five bug-direction cases failing before the fix and passing after, while the five safety-preservation cases pass in both directions as intended. The two new teardown cases prove the same composed rule on the destructive path: a custody-proven parked pre-push run is aborted before removal, and a run whose custody names a different submitted head is never touched. I also ran the two adjacent suites that consume the changed files (fm-gotmp, documentation-audiences), both green. The only failure is environmental and pre-existing:lsofis not installed in this sandbox, so fm-teardown's leaked-process reap tests fail, and I verified they fail identically with base bin/fm-teardown.sh; because the runner exits on first failure, the lsof-gated tests after that point did not execute locally and CI owns them. No visual artifacts apply here since the change is a CLI status line, which I captured as a before/after terminal transcript.Evidence: Before/after operator transcript of bin/fm-crew-state.sh (the reported 'failed' verdict, and the fix)
===== AFTER the fix (bin/ at 8f4ee21) ===== SCENARIO A - live pre-push pipeline, 5 fix rounds deep, reported by 'axi status' (run head lives in the no-mistakes gate repo, unresolvable in the crew worktree) $ bin/fm-crew-state.sh mate state: working · source: run-step · validating (running) · status-log superseded by active run SCENARIO B - 'axi status' is showing another crew; the coarse runs list decides newest row: this branch, running, pre-push sha older row: superseded 'failed' run at the worktree head $ bin/fm-crew-state.sh mate state: working · source: status-log · pipeline validating the branch ===== BEFORE the fix (bin/ at base 2d2be63, identical scenarios) ===== SCENARIO A - live pre-push pipeline, 5 fix rounds deep, reported by 'axi status' (run head lives in the no-mistakes gate repo, unresolvable in the crew worktree) $ bin/fm-crew-state.sh mate state: parked · source: status-log · authority call on finding r2 SCENARIO B - 'axi status' is showing another crew; the coarse runs list decides newest row: this branch, running, pre-push sha older row: superseded 'failed' run at the worktree head $ bin/fm-crew-state.sh mate state: failed · source: run-step · run failedEvidence: Per-case results with pre-fix bin/ (fail-before), crew-state
--- test_prepush_pipeline_custody_is_attributed not ok - live pre-push pipeline reports working (missing: 'state: working') --- output --- state: parked · source: status-log · authority call on finding r2 --- test_coarse_older_terminal_row_does_not_outrank_newest_row not ok - superseded older run must not be reported as failed (unexpected: 'state: failed') --- output --- state: failed · source: run-step · run failedEvidence: Per-case results with pre-fix bin/ (fail-before), teardown abort authority
--- test_parked_prepush_run_owned_by_custody_is_aborted not ok - parked-prepush-custody: teardown never aborted the pipeline that owns this branch --- test_parked_prepush_run_submitted_elsewhere_is_never_aborted ok - a parked run whose custody names another submitted head is never abortedEvidence: Same per-case list with post-fix bin/ (pass-after), all 11 cases
ok - pre-push pipeline custody is attributed to its own crew ok - a cached branch_sync pipeline status does not become the run's gate verdict ok - custody released back to the user is not attributed ok - pipeline custody of a different submitted head is not attributed ok - stale cached custody block is not attributed ok - custody claim naming a different run is not attributed ok - a branch_sync block with no run object is never attributed ok - older terminal runs-list row does not outrank the branch's newest row ok - missing run head beside a branch_sync block still refuses branch-only attribution ok - a parked pre-push run proven by branch_sync custody is aborted before the worker is removed ok - a parked run whose custody names another submitted head is never abortedPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-nm-run-lib.sh:193-fm_nm_status_matches_worktreereads the run head with the unscopedfm_nm_field "$out" head, whose sed takes the first^[[:space:]]*head:line anywhere in the output. The same commit addsfm_nm_blockprecisely because "run:andbranch_sync.local:both carry a barehead:, and fm_nm_field alone would always answer with the first one" - but the scoping is never applied to this read. Failure sequence:axi statusreturns a run object with nohead:key (the shapetest_missing_run_head_falls_back_to_current_statedeliberately constructs and pins as "must not permit branch-only attribution") together with abranch_syncblock, which is exactly the block this change makes routine. The firsthead:in the output is thenbranch_sync.local.head, which is by construction equal togit rev-parse HEADin the invoking worktree, sofm_nm_head_matches_worktreesees run_full == local_full and returns 0. The run is attributed by the HISTORY rule with no evidence at all about the run's own head - branch name alone, the exact thing the history rule exists to refuse. That existing test cannot catch it because its fixture (run_parked | grep -v '^ head:') has nobranch_syncblock. This reaches the destructive caller too:fm-teardown.shnow shares this predicate and would runaxi aborton a run it has not proven it owns. Fix at the shared boundary the change already built: scope the read, e.g.fm_nm_field "$(fm_nm_block "$out" 0 run)" head, falling back to the unscoped read only when norun:block exists, and extend the missing-head fixture with abranch_syncblock so the contract is pinned.bin/fm-teardown.sh:1220-task_status_is_own_parked_runnow gatesno-mistakes axi aborton the widenedfm_nm_status_matches_worktree, so a run whose head is not an object in this worktree can authorize an abort - something teardown previously always refused. That widening is deliberate per the intent, but it is the one direction the intent names as unsafe if wrong ("a false positive lets teardown abort a run it does not own"), and no test covers it on the teardown side: tests/fm-teardown.test.sh's only negative attribution case is a branch mismatch (parked_axi_status_toon fm/some-other-task deadbeef, line 2174), and every positive case supplies a head that binds by history, so the custody branch is never executed there. All six new cases live in tests/fm-crew-state.test.sh, which only exercises read-only reporting. Add a teardown case with the pre-push shape (unresolvable head plus apipeline_ownedbranch_sync) asserting the abort fires, and its mirror with a divergingsubmitted_headasserting no abort log is written.🔧 Fix: scope run-object field reads and cover teardown custody
4 issues (2 warnings, 2 infos) still open:
bin/fm-crew-state.sh:219-nm_gate_statusgreps the WHOLE payload for^[[:space:]]*(status|state):[[:space:]]*"?(awaiting_approval|fix_review)"?$, andbranch_sync.pipeline.statusis rendered at 4-space indent, so it matches. Verified directly:grep -E '<that regex>'over a payload whose run block saysstatus: runningand whosebranch_sync.pipelinesaysstatus: awaiting_approvalreturns the branch_sync line. Failure sequence: line 465 reads the run's own status correctly (running) via the new scopednm_run_field, but line 469 then readsawaiting_approvalout of the cached branch_sync block, line 481's[ -n "$gate_status" ]fires,nm_gate_line_name/nm_gate_namefind nothing (nogate:key, no CSV row with a gate status), sogate=$statusand the crew is emitted asstate: parked | source: run-step | parked at running- a false parked verdict that also suppresses the genuine status-log state and skips thestatus-log superseded by active runreconciliation at line 546. This is the same class of false verdict the change exists to eliminate, and the change is what makes it routinely reachable: before it, a pre-push run whose head is unresolvable was never attributed, so its branch_sync-bearing payload never reached this parser; now every pre-push fix-round payload does, and every one of them carriesbranch_sync.pipeline.status. The invariant this change itself establishes is that branch_sync is an untrusted CACHED block - the commit's own comment says the conjuncts exist to "reject an inconsistent or cross-repo cached answer instead of trusting it" - yet the very next thing done with that same cached block is to let it supply the run's gate verdict unchallenged. Divergence needs only a cache not refreshed across a gate resolution: the crew answers the gate, the run resumes to running/fixing, and state/submitted_head/local.head are all unchanged so every custody conjunct still passes. Fix at the boundary the commit already drew: excise the top-levelbranch_sync:block once (e.g. aRUN_DETAIL_OUTderived from$RUN_OUT) and run the gate/step/findings scans over that. Note scoping these greps to therun:block instead would be wrong -run_parked_in_gate_blocklegitimately carriesstatus: fix_reviewinside the top-levelgate:block, a run sibling, not insiderun:.bin/fm-nm-run-lib.sh:183- The custody predicate requires[ "$state_field" = pipeline_owned ]exactly, but the comment 40 lines above documents the field's own vocabulary as "pipeline_owned, user_owned, target_changed, remote_missing, legacy_unbound, and the blocked_* variants" - i.e. the change's own documentation asserts that a state the predicate rejects outright is part of the same custody vocabulary. Ifstatecan beblocked_pipeline_ownedorblocked_pipeline_owned_recoverable(both are present as strings in the installed v1.45.4 binary), then a live pre-push pipeline in a blocked-sync situation is refused by custody, refused by the history rule (head unresolvable), and the exact false verdict this change fixes recurs for that state. Counter-evidence is in the change too: the live-traced fixture recordsstate: pipeline_ownedalongsidesafety: blocked_pipeline_owned, which suggests blocked_* is asafety/next_action code rather than astatevalue - in which case the comment's vocabulary list is what is wrong, not the predicate. I could not resolve which from the stripped binary. This needs the author's knowledge of the CLI, and loosening the conjunct is the unsafe direction (bin/fm-teardown.sh shares the rule and would gain abort authority), so it must not be changed without that answer: either correct the comment to state that blocked_* belongs tosafety, or accept the blocked_pipeline_owned* states with their own pinning test.bin/fm-nm-run-lib.sh:120-fm_nm_run_field'selsearm falls back to the unscopedfm_nm_fieldwhenever the payload has no top-levelrun:block. If a payload ever carriesbranch_sync:without arun:block, that arm readsheadandbranchstraight out ofbranch_sync.local, which describes this worktree by construction, sofm_nm_status_matches_worktreebinds on branch-name-plus-self evidence - precisely the hole this function was added to close. I found no shape that produces it (every fixture and every documented v1.45.4 status answer nests the run underrun:; the script's own comment records that bareaxi statusalways reports some run, and fm-teardown is protected incidentally because branch_sync has noid:key so itsrun_idguard trips first), so this is hardening, not a proven path. Worth noting that theelsearm is also reached by no test in either file - the flat-shape it exists for has no known producer. Cheapest close: take the fallback only when the payload has no top-levelbranch_sync:block either.no-mistakes(review): scope run-object field reads and cover teardown custody. It is the onlyno-mistakes(...)subject in the last 400 commits - the repo's history is conventional-commits with fix/feat/docs/test/perf/chore/refactor - and it names the internal validation pipeline in what the intent calls a "public upstream repo" whose stated conventions include keeping agent attribution out of commits ("no agent as commit co-author"). Flagging rather than fixing: the intent forbids amending or rewriting commits on this branch, so the only remedy is a reword or squash at merge time, which is the author's call.🔧 Fix: excise cached branch_sync from run-detail reads
1 info still open:
bin/fm-teardown.sh:1224- Commit 8f4ee21 establishes the invariant that the cached branch_sync block must never answer a question about the run's own detail, and applies it throughout bin/fm-crew-state.sh by deriving RUN_DETAIL_OUT once (fm-crew-state.sh:429) and routing every detail scan through it - including the two scans that are literally duplicated here: theawaiting_agent:grep (fm-crew-state.sh:478) andnm_has_gate(fm-crew-state.sh:238). task_status_is_own_parked_run adopted the scoped reads for id/branch/status (lines 1216, 1218, 1223) but left these two greps on the raw$out, which still contains branch_sync. Those two greps are exactly what decides the abort at line 1229 whenstatusis not awaiting_approval/fix_review, so this is the caller with the destructive blast radius, not the read-only one. I could not prove a reachable path: the live-traced branch_sync shape (state, changed, local{branch,head,clean}, pipeline{run,status,phase,submitted_head,current_head,pushed_head,pushed_at,push_generation}, target, remote, relation, safety, pr_state, note, next_action{code,command}) carries neither anawaiting_agentkey nor a baregate:key, and next_action.command's--gatetext cannot match^[[:space:]]*gate:. So this is hardening, not a live defect - reported at info because it is the completion of the invariant the fix round already drew, in one line: derivedetail=$(fm_nm_without_block "$out" 0 branch_sync)and grep that for awaiting_agent and gate, the same way the sibling script does. Note the third unscoped read here,outcomeat line 1221, is fine as-is: teardown's teardown-eligibility fixture nestsoutcome:inside the run block while fm-crew-state's fixtures put it top-level, so the unscoped read is deliberate, and a spurious outcome would only make teardown refuse to abort - the safe direction.tests/fm-teardown.test.sh:2302- tests/fm-teardown.test.sh fails attest_leaked_worktree_process_is_reapedbecauselsofis not installed in this sandbox, and fm-teardown.sh's leaked-process scan requires it. I confirmed this failure is pre-existing and unrelated: it reproduces identically with base bin/fm-teardown.sh from 2d2be63, as doestest_run_abort_precedes_process_reap_precedes_worktree_removal. I cannot fix it here (installing system packages is outside the worktree boundary). Consequence: the runner exits on first failure, so the ~9 lsof-gated process-reap tests after that point did not execute locally. Both new custody tests run before that point and passed. Remote CI owns the untested tail.bash tests/fm-crew-state.test.sh(full file, 58 cases, all pass at target)bash tests/fm-teardown.test.sh(full file; passes through both new custody cases, then hits pre-existing lsof-absent failure)Per-case fail-before/pass-after for each new crew-state case run against base bin/ then target bin/:test_prepush_pipeline_custody_is_attributed,test_coarse_older_terminal_row_does_not_outrank_newest_row,test_cached_branch_sync_gate_status_does_not_park_a_running_run,test_missing_run_head_with_branch_sync_falls_back_to_current_state,test_branch_sync_without_run_block_is_not_attributed,test_released_custody_unresolvable_head_not_attributed,test_pipeline_custody_submitted_elsewhere_not_attributed,test_stale_cached_custody_block_not_attributed,test_custody_claim_for_another_run_not_attributedPer-case fail-before/pass-after fortest_parked_prepush_run_owned_by_custody_is_abortedandtest_parked_prepush_run_submitted_elsewhere_is_never_aborted(teardown abort authority, destructive path)Manual end-user transcript: realbin/fm-crew-state.sh <id>driven over a throwaway git worktree with a fakeno-mistakesemitting the live pre-pushaxi statuspayload and the 3-row runs list, captured with base bin/ and with target bin/bash tests/fm-gotmp.test.sh(symlinks fm-nm-run-lib.sh into its fake bin; passes)bash tests/fm-documentation-audiences.test.sh(covers the changed docs/architecture.md and docs/scripts.md prose surfaces; passes)Confirmed lsof-absent failures are pre-existing:git checkout 2d2be63 -- bin/fm-teardown.sh bin/fm-nm-run-lib.shthen re-rantest_leaked_worktree_process_is_reapedandtest_run_abort_precedes_process_reap_precedes_worktree_removalScope checks:git diff --stat 2d2be63..8f4ee21shows only the 7 intended files with no fm-test-run.sh collation change; three commits sit on top of base with no rewrite; added Markdown lines contain no em/en dashgit status --porcelainclean after all temporary base/target file swaps and driver scripts were removeddocs/decision-hold-lifecycle.md:81- Pre-existing evidence drift, not caused by this change: docs/decision-hold-lifecycle.md:80-81 recordsbash tests/fm-teardown.test.shas emittingall teardown safety cases passed, but that string is absent from tests/fm-teardown.test.sh at the base commit 2d2be63 as well as at HEAD (the suite now prints per-caseok - ...lines). This change adds two teardown custody cases to that file, so the recorded output drifts further, but fixing the dated verification record is a separate consolidation and out of scope for the attribution fix. Follow-up: re-run the documented command and refresh that evidence block.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.