fix(bin): seal terminal runs after pipeline-owned head advances - #105
Merged
Merged
Conversation
…ead advance `--complete` refused a genuinely validated PR whenever the no-mistakes run's own review and doc commits advanced the branch head past the validated head and the run then reached a terminal PASSED state. The head-drift exception required `fm_nm_run_is_active` plus `branch_sync.state=pipeline_owned`, both of which a terminal run cannot satisfy: it has released the branch by construction. Replanning at the advanced head did not help either, because the plan records that terminal run as `validation_preplan_run_id` and `--bind-run` then refuses it, while `--complete` requires a bound run. The only escape was a fresh no-mistakes run over unchanged code. The advance is now authoritative in two shapes. An active run must still currently own the branch. A terminal run must have passed, and its own reported head is the authority for the commits it produced. The existing `observed_head_full = current_head` check is what keeps that honest: a foreign commit landed after the run finished is never reported as the run's head, so it still fails completion, and a terminal run that did not pass still seals nothing. Claude-Session: https://claude.ai/code/session_018ALZVEheSodHtcmw2cuzG4
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
Fix a deadlock in bin/fm-receipt-check.sh:
--completecannot record completion for a terminal PASSED no-mistakes run whose OWN pipeline (review/doc) commits advanced the branch head beyond the validated head.The deadlock, all in bin/fm-receipt-check.sh:
validation_preplan_run_id, so--bind-runrefuses it (pre-plan check).--completerequires a bound run.fm_nm_run_is_activerequires an ACTIVE run.Net effect: a genuinely green, fully-validated PR (all criteria receipts + CI green + a terminal PASSED no-mistakes run) cannot record its completion receipt without a wasteful fresh no-mistakes run on unchanged code. This hit live on port-3123a (PR #98) and on all four wave-2 ports (#100-#103); those had to be accepted on the green PR with the receipt seal skipped.
Required outcome: add a supported
--completepath that seals a terminal PASSED no-mistakes run when the head advanced ONLY by that run's own pipeline (review/doc) commits, WITHOUT requiring a fresh run. Diagnose the exact deadlock first, then choose the minimal correct fix that recognizes pipeline-owned head advances as authoritative for the terminal run that produced them.Safety invariant that MUST be preserved:
--completemust STILL refuse to seal when the head advanced by commits that are NOT the run's own pipeline commits (genuine unvalidated drift), and must still require that the run genuinely passed.Acceptance criteria:
--completeseals a terminal PASSED run whose branch head advanced ONLY by that run's own pipeline commits, without a fresh no-mistakes run. The exact deadlock scenario (terminal passed run + pipeline-advanced head, no active run) is reproduced in a colocated test and shown to complete.--completestill refuses when the head advanced by foreign (non-pipeline) commits, and still requires the run genuinely passed. Colocated tests cover both the seal-succeeds and seal-refuses cases in the repo's existing fm-receipt-check test surface.Decisions and tradeoffs made while implementing, which a reviewer reading only the diff would not know:
The chosen fix is deliberately minimal and lives at the existing head-drift exception rather than relaxing
--bind-run's pre-plan guard. That guard is correct: after a re-plan at the advanced head, the terminal run genuinely predates the new plan. The real bug is that a re-plan was ever needed. With--completenow accepting the pipeline advance under the ORIGINAL plan generation, the re-plan trap is never entered, so the pre-plan guard was intentionally left untouched.The advance is now authoritative in exactly two shapes. An ACTIVE run must still currently own the branch (
branch_sync.state=pipeline_owned) - unchanged behavior. A TERMINAL run must have passed, and its own reported head is the authority for the commits it produced.Requiring
pipeline_ownedfor a terminal run is wrong by construction, not merely inconvenient: bin/fm-nm-run-lib.sh's own header documents that a terminal run has RELEASED the branch. So the old condition could never be satisfied by the exact case that needs sealing.The foreign-drift refusal is carried by the PRE-EXISTING
observed_head_full = current_headcheck immediately above the drift block, which was deliberately left in force for both shapes. A commit landed after the run finished is never reported as that run's head, so completion still refuses it. This is why no new drift detector was added: adding one would duplicate an invariant that already holds.A new predicate
fm_nm_run_is_terminal_passedwas added to bin/fm-nm-run-lib.sh rather than inlined in fm-receipt-check.sh, because that lib is the documented single owner of no-mistakes run-attribution primitives shared by fm-crew-state.sh, fm-teardown.sh, and fm-receipt-check.sh. Its header comment explains why head equality remains the caller's obligation.Test coverage is three cases inside one colocated test in the existing tests/fm-receipt-check.test.sh surface (no new runner): the deadlock now seals at the advanced head; foreign drift is refused (exit 2); and a terminal run that FAILED is refused (exit 2). The third case is not in the acceptance criteria as written but is required by the "still requires the run genuinely passed" half of AC2.
docs/verification/evidence-receipts.md is a maintainer-verification record, so its guarantee bullet was corrected and its recorded suite output refreshed from a real run. That recorded output was already two tests stale before this change; refreshing it was in scope because a verification record must state current behavior.
Verification actually run: tests/fm-receipt-check.test.sh 36 ok rc=0; tests/fm-crew-state.test.sh all passed (other consumer of the shared lib); bin/fm-lint.sh exit 0; bin/fm-doc-audience-check.sh ok.
Firstmate-Validation-Generation: b4977a6120f3ec41e7f72c1af0ef6c07
What Changed
--completeaccepts a branch head advanced solely by that run’s own review/doc pipeline commits.Risk Assessment
✅ Low: The change is narrowly scoped, preserves exact-head and descendant safety checks, and adds behavioral coverage for terminal pipeline advances, foreign drift, and failed runs.
Testing
Ran the focused receipt-check suite covering seal success, foreign-drift refusal, and failed-run refusal, plus the shared fm-crew-state consumer suite; all passed, with a reviewer-visible transcript saved in the evidence directory.
Evidence: Receipt-check targeted test transcript
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.
tests/fm-receipt-check.test.shtests/fm-crew-state.test.shCaptured receipt-check output to~/.no-mistakes/evidence/01M1QMJV8HRP0R09MRTCY4WGP6/fm-receipt-check.test.log✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.