feat(bin): refuse merges whose evidence commit is not the PR head - #2660
Open
clca wants to merge 5 commits into
Open
feat(bin): refuse merges whose evidence commit is not the PR head#2660clca wants to merge 5 commits into
clca wants to merge 5 commits into
Conversation
A worker measures its suite figure and standing exploit result, reports them, and the validation pipeline then commits again on top - a fix round, a documentation step, a rebase onto a newer base. The reported figures now describe a commit that is no longer the head that would merge. That was caught four times by hand (PR 115 and PR 119 on 2026-08-09, PR 160 on 2026-08-19), and only because firstmate compared the reported commit against the live head every single time. More discipline is not the fix: briefing every worker to "re-measure on the final head" already loses the race, because the pipeline can commit after the worker's last action. The guard goes in firstmate's own merge path. - bin/fm-evidence-record.sh records evidence_head=<sha> plus an optional one-line evidence_note= into the task's durable metadata. The durable record is the store rather than the PR body: it is firstmate-private, it already holds the parallel pr= and pr_head= values, and the PR body is written by the pipeline and editable afterwards by anyone. - bin/fm-pr-merge.sh reads the live head from the forge and refuses unless it equals the recorded commit, before recording any state or arming any poll. The refusal names both commits, quotes what was measured, and gives the exact re-record command for the live head. - An absent record refuses on the same path. Passing silently would leave the guard defeatable by never recording, and nothing distinguishes "no claim was made" from "the claim was lost". The remedy is that one command, not a bypass flag, so a task predating this record is never stranded. An unconfirmable head refuses too: with nothing to compare against, the merge stops. - bin/fm-brief.sh gives both PR-based ship modes the matching worker contract - record at measurement time, re-record after every re-measurement. local-only and scout scaffolds omit it because neither reaches this merge path and neither has a pipeline that can commit after the worker. - fm_pr_metadata_identity_parse now tolerates the evidence keys after pr=. Re-recording happens while the merge poll is already armed, which appends those lines after pr=; without this the watcher's revalidation of the armed poll would fail on exactly the remedy the refusal asks for. Tests cover matching, mismatched, absent, and unconfirmable heads, the refuse -> re-measure -> merge round trip on an armed task, and the recorder's own write, replace, refuse, and survival behavior, all through the real scripts.
… writes, split refusals
Two defects in the merge-evidence guard, both found by review. The identity parse enumerated which keys may appear after pr=, and that allowlist was wrong three times running: the evidence recorder, fm-spawn's traceparent and relaunch transaction, and fm-decision-hold's review record all append there, and nothing stops a fourth. Every such writer was a latent refusal of a merge on valid work, discovered only when a correct merge was wrongly blocked. Replace the allowlist with a shape test: any well-formed key=value line is tolerated, while a line that is not a record line, a second pr=, and an invalid pr_head= are still refused. The two test fixtures that built an "ambiguous" record out of an unknown key after pr= now use two irreconcilable pr= lines, which is the ambiguity their name claims. The brief's evidence-recording section sat after the line telling the worker it was finished, so a worker reading the definition of done in order stopped before reaching the instruction the whole guard depends on. Move it above every finishing line in both PR modes and fold recording into each finishing condition. The scout, local-only, and charter variants were checked and have no such ordering. A colocated assertion now pins the ordering and the condition.
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
1 participant
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
Turn a merge-time check that firstmate currently performs BY HAND into a guard that cannot be skipped: refuse a merge when the evidence a worker reported was measured on a commit that is no longer the pull request's head.
Problem, with real instances: a worker measures its full-suite figure and the standing injection-exploit result, reports them, and the validation pipeline then commits AGAIN on top (a fix round, a document step, or a rebase onto a newer base). The figures in the report and in the PR description now describe a commit that is no longer the head that would merge. Observed four times across 2026-08-08 and 2026-08-09, and again on 2026-08-19. In each case a reported suite figure described a commit that a later fix round, document step or rebase had already superseded; one was overtaken twice. Every one was caught only because the reported commit was compared against the live head by hand.
Why it matters: the reported suite figure and the security-check result are the two things firstmate verifies before a merge, precisely because the pipeline has been observed declaring checks-passed on a red suite, and because a target repository may have no automated checks configured at all. A figure that silently describes an earlier commit turns that verification into theatre.
Approach, and what is explicitly NOT the fix: more discipline is not the fix - instructing every brief to "re-measure on the final head" has already been done and still loses the race, because the pipeline can commit after the worker's last action. The guard lives in firstmate's own merge path: bin/fm-pr-merge.sh refuses when the commit the evidence was measured on does not equal the pull request's head. The reported commit is recorded machine-readably in the durable task record (state/.meta) rather than the PR body, so the guard reads a record firstmate owns and no PR-description edit can forge. A guard that refuses must state exactly which commit it expected and which it found, or it will be worked around rather than fixed; the refusal message says what to re-measure and on which commit.
Acceptance criteria:
Constraints: push remote is the fork clca/firstmate; the PR opens from that fork into kunchenguid/firstmate base main; never push to kunchenguid/firstmate; never force-push any branch but this one; never add an agent name as a commit co-author; do not touch projects/, data/, state/, or config/.
ACCEPTED DECISIONS ALREADY APPLIED AND COMMITTED - do not re-litigate these; they are settled requirements in their current form:
A. Identity-record parse SHAPE is deliberately tolerant, by decision. Three separate scripts append keys after pr= in state/.meta - bin/fm-evidence-record.sh (evidence_head=, evidence_note=), bin/fm-spawn.sh (traceparent=, control_relaunch_tx=), and bin/fm-decision-hold.sh (decisions_reviewed=, decision_keys=) - and nothing prevents a fourth. An enumerated allowlist was wrong three times running, and each future appender would be a latent refusal of a merge on valid work. So the parser tolerates any well-formed key=value line it does not recognise. Tolerance is NOT laxity: a line that is not key=value at all, a second pr=, an invalid pr_head=, or a truncated record are still refused loudly, and the keys the guard reads (pr=, evidence_head=) are still located correctly regardless of what surrounds them. A regression test parses and merges a record carrying a key the parser has never seen. Do not propose narrowing this back to an allowlist or adding another named key.
B. merge-head TOCTOU: KEEP the local comparison. gh pr merge --match-head-commit exists, but bin/fm-pr-merge.sh merges through gh-axi pr merge, whose flag set has no head-match flag, and routing around gh-axi to plain gh contradicts the standing instruction to use gh-axi for GitHub operations. The gh-axi-side change is filed separately. The remaining window is seconds rather than the hours the manual habit left open, which is a strict improvement. Do not propose a partial atomic-merge implementation.
C. Evidence writes are interrupt-safe: fm_pr_evidence_write and bin/fm-evidence-record.sh mirror the cleanup trap bin/fm-pr-check.sh already wraps around the same operation, so two writers of one record do not have asymmetric interrupt behaviour.
D. Unconfirmable-head refusals are split: "gh is not on PATH" and "gh could not answer" are distinct messages with distinct actionable fix lines, so a host with gh-axi but no plain gh is not worse off than before this change and is not pointed at gh auth status to diagnose a missing binary.
E. bin/fm-brief.sh's definition of done records the measured commit BEFORE any finishing line, and recording is part of the finishing condition rather than an appendix, in both PR-based modes; a worker reading in order can no longer stop before reaching the instruction the guard depends on. The scout, local-only, and charter variants were checked and have no such ordering problem. A colocated assertion pins the ordering and the condition.
Known-good measurement carried forward: on commit 4268fc9 the full suite is 2259 pass / 11 fail across 150 suites, and all 11 failures reproduce unchanged at base 81bcb10 with the same assertions, so this change introduces zero new failures. The pre-existing failing suites are arm-pretool-check, backend-orca, composer-lib, gotmp, muse-harness, remote-secondmate-lifecycle-e2e, remote-secondmate-trace-context, teardown, turnend-guard, wake-queue, and watcher-lock.
What Changed
bin/fm-pr-merge.shnow runs an evidence guard before it records any state or arms any poll: it reads the task'sevidence_head=fromstate/<id>.meta, fetches the pull request's live head, and refuses when the two differ (printing both commits, the recorded note, and the re-record command), when no evidence commit is recorded, when the record is malformed, and when the head cannot be confirmed — with separate messages forghmissing from PATH andghfailing to answer.bin/fm-evidence-record.shis the only writer of that record, backed byfm_pr_evidence_read/fm_pr_evidence_write/fm_pr_evidence_note_validinbin/fm-pr-lib.sh, which replace the record atomically under the shared per-task metadata lock with an interrupt-safe cleanup trap, validate the SHA, and bound the optional note to one control-character-free line of 200 characters (non-ASCII allowed, counted by character).fm_pr_metadata_identity_parsenow tolerates any well-formedkey=valueline afterpr=instead of an enumerated allowlist, while still refusing non-record lines, a secondpr=, and an invalidpr_head=.bin/fm-brief.shemits an evidence-recording section in both PR-based ship scaffolds (no-mistakesanddirect-PR), shell-quoted and bound to the scaffoldingFM_HOME/state directory, placed above every finishing line and folded into the definition of done;AGENTS.md,docs/architecture.md, anddocs/scripts.mddocument the guard and its remedy, and tests addtests/fm-evidence-record.test.shplus new cases in the merge, brief, and PR-identity suites covering matching, mismatched, absent, and unconfirmable heads against the real scripts (registered inbin/fm-test-run.sh'spr-forgefamily).Risk Assessment
✅ Low: The change is well-bounded and fail-closed by design: every new refusal happens before any state is written or poll armed, the tolerant metadata parse was verified to still reject the injection shapes it claims to (second
pr=, non-record lines) while survivingfm-spawn.sh's relaunch rewrite, tests drive the real scripts rather than re-implementations, and every source-verifiable acceptance criterion checks out - the three remaining findings are informational nits, none of which affects the guard's decision on a normally configured host.Testing
Baseline targeted suites (fm-pr-merge, fm-evidence-record, fm-brief, fm-pr-check-security) all passed, and the lane coverage guard accepts the new suite. On top of that I drove the real bin/fm-pr-merge.sh and bin/fm-evidence-record.sh through a sandboxed replay of that incident: the merge refuses when nothing is recorded, proceeds when the recorded commit is the live head, and refuses naming both the measured commit and the moved head after a simulated pipeline commit — with no gh-axi merge submitted on any refusal. The refusal's printed recording command was executed verbatim from a shell carrying none of the merge's environment and cleared the block, proving the guard is escapable only by re-measuring. I also confirmed the tolerant metadata parse (an unseen key keeps the armed poll valid and merges; a non-record line is refused) and the two distinct unconfirmable-head refusals for a failing gh versus an absent gh, and captured real generated briefs showing the recording section above every finishing line in both PR-based modes. Evidence is two CLI transcripts in the evidence directory. Criterion 6 (shellcheck) was deliberately not exercised: this phase is barred from running linters and the lint phase owns it. No UI surface is involved, so no visual artifact applies.
Evidence: Merge evidence guard — end-to-end CLI transcript (replays the 2026-08-09 incident)
Source: Merge evidence guard — end-to-end CLI transcript (replays the 2026-08-09 incident)
Evidence: Generated fm-brief.sh scaffolds — evidence-recording section and ordering, all three ship modes
Source: Generated fm-brief.sh scaffolds — evidence-recording section and ordering, all three ship modes
Evidence: Stale-evidence refusal (excerpt from the transcript)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-brief.sh:373- The scaffolded recording command is emitted as a bare$FM_ROOT/bin/fm-evidence-record.sh $ID ...— neither shell-quoted nor bound to the home the brief was scaffolded against — unlike every other absolute path this scaffold emits (STATUS_FILEat line 189 andHERDR_LAB_HELPERat line 280 both go throughshell_quote, and tests/fm-brief.test.sh:472 pins that invariant with a root literally namedfirstmate helper's root). Two consequences: (1) a firstmate root containing a space or apostrophe emits a command line the worker cannot run as printed; (2) fm-brief.sh resolves FM_ROOT (line 102) and FM_HOME (line 103) independently and derives STATE from FM_HOME (line 112) — it bakes$STATE/$ID.statusabsolute for exactly that reason — but the emitted command carries no home binding, and bin/fm-evidence-record.sh falls back toFM_HOME=${FM_HOME:-$FM_ROOT}with crew panes inheriting no FM_HOME (bin/fm-spawn.sh:2762 injects it only for KIND=secondmate; only GOTMPDIR and TRACEPARENT are exported to crew panes at lines 2790/2795). Fix: emit the command through shell_quote and prefix it with the resolved home, e.g.FM_HOME=<quoted $FM_HOME> <quoted $FM_ROOT>/bin/fm-evidence-record.sh ..., matching theFM_HOME=<home> <home>/bin/...convention documented in docs/configuration.md:505.bin/fm-pr-lib.sh:228-fm_pr_evidence_note_validmatches^[[:print:]]*$underlocal LC_ALL=C, so any non-ASCII byte is rejected — verified: a note likesuite 4208 pass — exploit blockedis refused. The refusal in bin/fm-evidence-record.sh:82 says only "the note must be one printable line of at most 200 characters", which does not describe the actual ASCII-only constraint, so a worker whose note contains an em dash or other common non-ASCII punctuation sees a message that appears to contradict its own input. Behavior is fail-loud and re-runnable, so this is friction rather than a hole; either widen the class or say "printable ASCII" in the message.bin/fm-pr-merge.sh:123- The live-head lookup is now implemented twice against the same forge fact with different semantics: bin/fm-pr-merge.sh:123 usesgh pr view <number> --repo <owner>/<repo>, while bin/fm-pr-check.sh:77 usesgh pr view <url>from inside the task worktree and only when that worktree still exists. Both feed the same merge invocation moments apart, so they can disagree (worktree gone -> nopr_head=recorded even though the guard read a head fine). Consolidating the read into onefm_pr_libhelper would keep the guard and the recorder from drifting; the current divergence is benign becausepr_head=is documented as optional, so this is a cleanup rather than a defect.🔧 Fix: quote and home-bind evidence commands, widen note charset
3 infos still open:
bin/fm-pr-merge.sh:139- The guard's live-head read isgh pr view "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO", which does not pin the forge host: with--repo OWNER/REPO(no host), gh resolves the repository against its default host, whichGH_HOSTin the environment overrides.fm_pr_url_parse(bin/fm-pr-lib.sh:178) already guarantees this URL is on github.com, and the sibling read in bin/fm-pr-check.sh:77 pins the host by passing the full$URL. So on a host whereGH_HOSTnames an enterprise instance, the guard compares the recorded evidence commit against a head read from a different forge - normally a spurious refusal, but a same-numbered PR there whose head coincided would pass stale evidence through the check that exists to stop exactly that. Fix is one argument, not the helper consolidation that was deferred: pass the canonical"$URL"instead of"$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO", matching fm-pr-check.sh.tests/fm-brief.test.sh:372-assert_evidence_precedes_every_finishchecks the ordering per finishing line, but enforces the second half of its own claim - "each finishing line must make recording part of its own condition, not an appendix" - with a single file-widegrep -q. The generated no-mistakes brief has two finishing lines (append \done: {summary}`andappend `done: PR {url} checks green`); only the second names recording, and the first passes solely because the other line matched somewhere in the file. Decision E requires recording to be part of the finishing condition in both PR-based modes; that holds today via the DOD opener ("complete only when committed ... AND the commit ... is recorded"), but the assertion does not pin it - deleting that opener would leavedone: {summary}` with no recording condition and the test would still pass. Make the condition check per finishing line, or assert the DOD opener explicitly.tests/fm-evidence-record.test.sh:166- The matrix at the top of the file claims case (d) covers "a short, uppercase, or non-hex commit", but the loop's uppercase entry is"$(printf '%s' "$SHA_A" | tr 'a-f' 'A-F')zz"andSHA_Ais forty1s, sotris a no-op and the value is refused only for being 42 characters. Nothing exercises an uppercase SHA at all - and the actual behavior is the opposite of what the matrix says: bin/fm-evidence-record.sh:70 deliberately lowercases before validating, so an uppercase SHA is accepted and normalized. That normalization is what makes the guard's string equality against the lowercased live head (bin/fm-pr-merge.sh:140) hold, and it is currently untested; dropping thetrat line 70 would break a worker who pasted an uppercase SHA and no test would catch it. Either record a genuinely uppercase valid SHA and assert it reads back lowercased, or correct the matrix wording.✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-pr-merge.test.sh tests/fm-evidence-record.test.sh tests/fm-brief.test.sh tests/fm-pr-check-security.test.sh— 4 suites, 0 failures (merge-guard matrix incl. matching/stale/absent/unconfirmable-head, evidence-record writer and tolerant-parse regressions, brief ordering assertion, PR identity + poll security suite)bin/fm-test-run.sh --check-coverage— confirms tests/fm-evidence-record.test.sh is claimed by a CI lane (FM_TEST_COVERAGE ok total=151)Manual end-to-end walkthrough in a throwaway FM_HOME sandbox with gh/gh-axi mocks, replaying PR 119:bin/fm-pr-merge.sh task-p119 https://github.com/kunchenguid/firstmate/pull/119with no record, thenbin/fm-evidence-record.sh task-p119 <sha> '<note>', then merge on a matching head, then moving the mocked live head and merging againRan the refusal's printed remedy command verbatim inbash -cwith FM_HOME/FM_ROOT_OVERRIDE/FM_STATE_OVERRIDE unset, then re-ran the merge to confirm the refusal clearsAppendedsome_future_writer_key=...afterpr=and re-checkedfm_pr_poll_artifacts_validplus a live merge (tolerated); appended a non-key=valueline and re-checked (refused)Ran the merge with aghthat fails and with noghon PATH at all, confirming two distinct refusals and zero gh-axi merge calls in either caseGenerated real briefs viabin/fm-brief.sh <id> firstmate --mode no-mistakes|direct-PR|local-onlyand inspected the Definition of done ordering and finishing lines✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.