From 333e8714f866efe746211f95b533e01cab1709ae Mon Sep 17 00:00:00 2001 From: clca Date: Wed, 19 Aug 2026 16:17:38 +0200 Subject: [PATCH 1/5] feat(merge): refuse a merge whose evidence outran the pull request head 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= 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. --- AGENTS.md | 1 + bin/fm-brief.sh | 28 ++++ bin/fm-evidence-record.sh | 87 +++++++++++ bin/fm-pr-lib.sh | 81 ++++++++++ bin/fm-pr-merge.sh | 63 ++++++++ bin/fm-test-run.sh | 1 + docs/architecture.md | 5 +- docs/scripts.md | 3 +- tests/fm-brief.test.sh | 41 +++++ tests/fm-evidence-record.test.sh | 232 +++++++++++++++++++++++++++++ tests/fm-pr-check-security.test.sh | 12 ++ tests/fm-pr-merge.test.sh | 226 +++++++++++++++++++++++++++- 12 files changed, 774 insertions(+), 6 deletions(-) create mode 100755 bin/fm-evidence-record.sh create mode 100755 tests/fm-evidence-record.test.sh diff --git a/AGENTS.md b/AGENTS.md index 67ec0d69609..ffbb49efc07 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -355,6 +355,7 @@ The worker reports the PR when CI first becomes green rather than waiting for me For PR-based ship tasks, the ready signal depends on mode: `no-mistakes` reports `done: PR checks green` after CI is green, while `direct-PR` reports `done: PR ` after opening the PR. Run `bin/fm-pr-check.sh ` - it records `pr=` and the forge's `pr_head=` when available in the task's meta and arms the watcher's merge poll. +`bin/fm-pr-merge.sh` refuses any merge whose recorded evidence commit is not the pull request's live head, and refuses when no evidence commit is recorded; clear either refusal by re-measuring on the named head and recording it with `bin/fm-evidence-record.sh`, never by working around the guard. Tell the captain the PR's full URL, always the complete `https://...` link rather than a bare `#number`, a concise outcome summary, and the no-mistakes risk level when applicable. A captain instruction to merge is explicit authority; `yolo` is the only standing routine authority. For any custom `state/.check.sh` you write yourself, keep it an ordinary single-link mode-`0700` file, print one line only when firstmate should wake, print nothing otherwise, finish before `FM_CHECK_TIMEOUT`, then bind its current bytes with `bin/fm-check-register.sh ` before the watcher may execute it. diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index 206e5a947ae..f74cb53e81f 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -49,6 +49,11 @@ # declared-external-wait verb (FM_CLASSIFY_PAUSED_VERB, default "paused") from # "blocked:": pause for a known external wait expected to clear on its own, # blocked when firstmate must act. +# PR-based ship briefs (no-mistakes and direct-PR) carry an evidence-recording +# section: the worker records the commit each reported verification was measured +# on through bin/fm-evidence-record.sh, and bin/fm-pr-merge.sh refuses to merge +# unless that commit is still the PR head. local-only and scout scaffolds omit it +# because neither reaches that merge path. # Ship tasks include a project-memory section so durable project-intrinsic # learnings can be committed to AGENTS.md through the project's delivery path; # it carries the AGENTS.md authoring bar (widely useful knowledge only, pointers @@ -348,6 +353,25 @@ echo "scaffolded: $BRIEF (scout; replace {TASK})" exit 0 fi +# Every PR-based ship mode carries the same evidence-recording contract, because +# bin/fm-pr-merge.sh refuses to merge unless the recorded commit equals the pull +# request's live head. The worker is the only party that knows which commit its +# figures came from, and the pipeline can commit after the worker's last action, +# so the contract is stated as "record at measurement time, re-record after every +# re-measurement" rather than "measure the final head", which cannot win that +# race. local-only ships no PR and scouts ship no change, so neither carries it. +IFS= read -r -d '' EVIDENCE_SECTION <' +\`\`\` +Record it again after EVERY re-measurement, and after anything that moves your branch head - a review fix round, a documentation commit, a rebase onto a newer base - because your earlier figures then describe a commit that is no longer the head. +The merge refuses when the recorded commit is not the pull request's head, and it refuses when nothing is recorded, so an unrecorded measurement stops the task rather than shipping unverified. +EOF +EVIDENCE_SECTION=${EVIDENCE_SECTION%$'\n'} + # Ship task: shape Setup / Rule 1 / Definition of done by this task's explicit # delivery mode, validated above. The generated DOD opens with the fixed # "Delivery contract: mode=" line that bin/fm-spawn.sh checks against its own @@ -363,6 +387,8 @@ This task ships **direct-PR**: you raise the PR yourself, without the no-mistake The task is complete only when committed on your branch. When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, then append \`done: PR {url}\` to the status file and stop. Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome. + +$EVIDENCE_SECTION EOF ;; local-only) @@ -401,6 +427,8 @@ Two firstmate-specific rules layer on top of that guidance: - Avoid \`--yes\`: it would silently bypass firstmate's authority check and any required captain escalation. After /no-mistakes reports CI green (the CI-ready return point - do not wait for it to keep monitoring in the background until merge), append \`done: PR {url} checks green\` and stop. You are finished. + +$EVIDENCE_SECTION EOF ;; esac diff --git a/bin/fm-evidence-record.sh b/bin/fm-evidence-record.sh new file mode 100755 index 00000000000..2dba76ef5b8 --- /dev/null +++ b/bin/fm-evidence-record.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# Record the exact commit a task's reported verification evidence was measured +# on, into that task's durable metadata as evidence_head= plus an optional +# one-line evidence_note=. Re-running replaces the previous +# record, so a re-measurement is recorded the same way as the first one. +# +# bin/fm-pr-merge.sh refuses to merge unless evidence_head equals the pull +# request's live head. A validation pipeline can commit after the worker's last +# measurement - a fix round, a documentation step, a rebase onto a newer base - +# which silently turns a reported suite figure or exploit result into a +# description of an earlier commit. Recording the measured commit is what makes +# that staleness detectable instead of a manual comparison someone has to +# remember to perform. +# +# Run it from the task worktree immediately after the verification run it +# describes, and run it again after every re-measurement: +# bin/fm-evidence-record.sh "$(git rev-parse HEAD)" 'full suite 4208 pass; injection exploit blocked' +# +# The note is one printable line of at most 200 characters and is quoted into +# the refusal message so a stale merge says what has to be re-measured. A note +# carrying a newline or control character is refused rather than trimmed, +# because a silently trimmed note is a silently wrong instruction. +# Usage: fm-evidence-record.sh [one-line note] +set -eu + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +FM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}" +FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}" +STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" + +usage() { + awk ' + NR == 1 { next } + /^#/ { sub(/^# ?/, ""); print; next } + { exit } + ' "$0" +} + +case "${1:-}" in + -h|--help) usage; exit 0 ;; +esac + +# shellcheck source=bin/fm-pr-lib.sh +. "$SCRIPT_DIR/fm-pr-lib.sh" +# shellcheck source=bin/fm-wake-lib.sh +. "$SCRIPT_DIR/fm-wake-lib.sh" + +if [ "$#" -lt 2 ] || [ "$#" -gt 3 ]; then + echo "error: invalid evidence record request" >&2 + echo "usage: fm-evidence-record.sh [one-line note]" >&2 + exit 2 +fi +ID=$1 +RAW_HEAD=$2 +NOTE=${3-} + +if ! fm_pr_task_id_valid "$ID"; then + echo "error: invalid evidence record request" >&2 + exit 2 +fi +HEAD_SHA=$(printf '%s' "$RAW_HEAD" | tr '[:upper:]' '[:lower:]') +if ! fm_pr_head_valid "$HEAD_SHA"; then + echo "error: '$RAW_HEAD' is not a full commit SHA; pass \"\$(git rev-parse HEAD)\"" >&2 + exit 2 +fi +if ! fm_pr_evidence_note_valid "$NOTE"; then + echo "error: the note must be one printable line of at most 200 characters" >&2 + exit 2 +fi + +# Task-derived paths are constructed only after the canonical ID validation. +META="$STATE/$ID.meta" +if [ ! -f "$META" ] || [ -L "$META" ]; then + echo "error: task metadata is unavailable" >&2 + exit 1 +fi + +if ! fm_pr_evidence_write "$META" "$HEAD_SHA" "$NOTE"; then + echo "error: could not record the evidence commit for task $ID" >&2 + exit 1 +fi + +if [ -n "$NOTE" ]; then + printf 'recorded: %s evidence measured on %s (%s)\n' "$ID" "$HEAD_SHA" "$NOTE" +else + printf 'recorded: %s evidence measured on %s\n' "$ID" "$HEAD_SHA" +fi diff --git a/bin/fm-pr-lib.sh b/bin/fm-pr-lib.sh index b70d8468894..b095313a4b9 100755 --- a/bin/fm-pr-lib.sh +++ b/bin/fm-pr-lib.sh @@ -213,6 +213,81 @@ fm_pr_head_valid() { [[ "$head" =~ ^[0-9a-f]{40}$|^[0-9a-f]{64}$ ]] } +# evidence_head= records the exact commit a task's reported verification +# evidence was measured on, and optional evidence_note= records what +# that measurement was. bin/fm-evidence-record.sh is the only writer and +# bin/fm-pr-merge.sh the only reader: a merge is refused unless evidence_head +# equals the pull request's live head, so a suite figure or exploit result +# measured before a later fix, documentation, or rebase commit can never be +# presented as the merging head's result. +FM_PR_EVIDENCE_HEAD= +FM_PR_EVIDENCE_NOTE= + +# fm_pr_evidence_note_valid : a note is one printable line, bounded, so it +# can never split the key=value record it is stored in. +fm_pr_evidence_note_valid() { + local note=${1-} + local LC_ALL=C + [ "${#note}" -le 200 ] || return 1 + [[ "$note" =~ ^[[:print:]]*$ ]] +} + +# fm_pr_evidence_read : load the task's evidence record into +# FM_PR_EVIDENCE_HEAD and FM_PR_EVIDENCE_NOTE. Returns 0 with an empty head when +# no record exists, and non-zero when the metadata is unreadable or the recorded +# head is malformed, so a corrupt record is never mistaken for a matching one. +fm_pr_evidence_read() { + local meta=${1-} head note + FM_PR_EVIDENCE_HEAD= + FM_PR_EVIDENCE_NOTE= + [ -f "$meta" ] && [ ! -L "$meta" ] || return 1 + head=$(grep '^evidence_head=' "$meta" | tail -1 || true) + [ -n "$head" ] || return 0 + head=${head#evidence_head=} + fm_pr_head_valid "$head" || return 1 + note=$(grep '^evidence_note=' "$meta" | tail -1 || true) + note=${note#evidence_note=} + fm_pr_evidence_note_valid "$note" || return 1 + FM_PR_EVIDENCE_HEAD=$head + FM_PR_EVIDENCE_NOTE=$note +} + +# fm_pr_evidence_write [note]: atomically replace the task's +# evidence record, preserving every other metadata line, and re-read the result +# so a partially written record can never be reported as recorded. Requires +# bin/fm-wake-lib.sh for the shared per-task metadata lock. +fm_pr_evidence_write() { + local meta=$1 head=$2 note=${3-} dir base tmp lock rc=0 + fm_pr_head_valid "$head" || return 1 + fm_pr_evidence_note_valid "$note" || return 1 + [ -f "$meta" ] && [ ! -L "$meta" ] || return 1 + [ "$(fm_pr_file_link_count "$meta")" = 1 ] || return 1 + dir=${meta%/*} + base=${meta##*/} + [ "$dir" != "$meta" ] || dir=. + lock=$(fm_meta_lock_path "$meta") || return 1 + fm_lock_acquire_wait "$lock" + tmp=$(mktemp "$dir/.${base}.fm-evidence.XXXXXX") || { fm_lock_release "$lock"; return 1; } + while :; do + [ -f "$meta" ] && [ ! -L "$meta" ] && [ "$(fm_pr_file_link_count "$meta")" = 1 ] || { rc=1; break; } + fm_pr_regular_destination_or_absent "$meta" || { rc=1; break; } + { grep -vE '^evidence_head=|^evidence_note=' "$meta" || true; } > "$tmp" || { rc=1; break; } + printf 'evidence_head=%s\n' "$head" >> "$tmp" || { rc=1; break; } + if [ -n "$note" ]; then + printf 'evidence_note=%s\n' "$note" >> "$tmp" || { rc=1; break; } + fi + chmod 0600 "$tmp" || { rc=1; break; } + mv -f -- "$tmp" "$meta" || { rc=1; break; } + tmp= + break + done + [ -z "$tmp" ] || rm -f -- "$tmp" + fm_lock_release "$lock" + [ "$rc" = 0 ] || return 1 + fm_pr_evidence_read "$meta" || return 1 + [ "$FM_PR_EVIDENCE_HEAD" = "$head" ] && [ "$FM_PR_EVIDENCE_NOTE" = "$note" ] +} + fm_pr_file_mode() { if [ "$(uname)" = Darwin ]; then stat -f %Lp "$1" 2>/dev/null @@ -317,6 +392,12 @@ fm_pr_metadata_identity_parse() { ;; x_request=*|x_request_ts=*|x_followups=*|x_platform=*|x_reply_max_chars=*) ;; + # A re-measurement recorded after the merge poll was armed rewrites the + # evidence lines to the end of the record, so they legitimately appear + # after pr=. Their own validity is enforced by fm_pr_evidence_read, which + # refuses the merge on a malformed record rather than passing it here. + evidence_head=*|evidence_note=*) + ;; *) [ "$seen_pr" -eq 0 ] || post_pr_invalid=1 ;; diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index 8226798a673..d8859126c04 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -7,6 +7,20 @@ # Merge method defaults to --squash when the caller passes none of --squash, # --merge, --rebase, or --method after the optional -- separator. Extra args # must not include --repo or -R because the repository comes only from the URL. +# +# The merge is refused unless the task's recorded evidence commit +# (evidence_head=, written by bin/fm-evidence-record.sh) equals the pull +# request's live head. A validation pipeline can commit after the worker's last +# measurement, which silently turns a reported suite figure or exploit result +# into a description of an earlier commit; this guard is what makes that +# staleness stop a merge instead of depending on a manual comparison. +# +# An absent record refuses too, rather than warning and merging. A guard that +# passes when nothing was recorded is defeatable by simply never recording, and +# nothing distinguishes "no claim was made" from "the claim was lost". The +# remedy is one command that records the commit the evidence was measured on, +# not a bypass flag, so a task that predates this record is never stranded and +# the guarantee is never traded away to unblock one merge. # Usage: fm-pr-merge.sh [-- ] set -eu @@ -70,6 +84,55 @@ if [ ! -f "$META" ] || [ -L "$META" ]; then exit 1 fi +# Evidence guard, before any state is recorded or any poll is armed: a refused +# merge must leave the task exactly as it found it. +if ! fm_pr_evidence_read "$META"; then + echo "error: refusing to merge $URL: the evidence record for task $ID is unreadable or malformed" >&2 + echo " fix: re-record the commit the reported verification was measured on:" >&2 + echo " $SCRIPT_DIR/fm-evidence-record.sh $ID ''" >&2 + exit 1 +fi +EVIDENCE_HEAD=$FM_PR_EVIDENCE_HEAD +EVIDENCE_NOTE=$FM_PR_EVIDENCE_NOTE + +if [ -z "$EVIDENCE_HEAD" ]; then + echo "error: refusing to merge $URL: no verification evidence commit is recorded for task $ID" >&2 + echo " expected: the commit the reported verification was measured on" >&2 + echo " found: no evidence record" >&2 + echo " fix: re-run the verification you intend to merge on, then record it:" >&2 + echo " $SCRIPT_DIR/fm-evidence-record.sh $ID ''" >&2 + exit 1 +fi + +# The live head is read here rather than taken from a recorded pr_head=, so the +# comparison is always against what would actually merge. +LIVE_HEAD= +if command -v gh >/dev/null 2>&1; then + if REMOTE_HEAD=$(gh pr view "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" --json headRefOid -q .headRefOid 2>/dev/null); then + LIVE_HEAD=$(printf '%s' "$REMOTE_HEAD" | tr -d '[:space:]' | tr '[:upper:]' '[:lower:]') + fi +fi +if ! fm_pr_head_valid "$LIVE_HEAD"; then + echo "error: refusing to merge $URL: the pull request head could not be confirmed" >&2 + echo " the recorded evidence commit for task $ID is $EVIDENCE_HEAD" >&2 + echo " without the live head there is nothing to compare it against, so the merge stops here" >&2 + echo " fix: restore GitHub access (gh auth status), then merge again" >&2 + exit 1 +fi + +if [ "$LIVE_HEAD" != "$EVIDENCE_HEAD" ]; then + echo "error: refusing to merge $URL: the reported evidence was measured on a commit that is no longer this pull request's head" >&2 + if [ -n "$EVIDENCE_NOTE" ]; then + echo " evidence measured on: $EVIDENCE_HEAD ($EVIDENCE_NOTE)" >&2 + else + echo " evidence measured on: $EVIDENCE_HEAD" >&2 + fi + echo " pull request head: $LIVE_HEAD" >&2 + echo " fix: re-run that verification on $LIVE_HEAD, then record the result:" >&2 + echo " $SCRIPT_DIR/fm-evidence-record.sh $ID $LIVE_HEAD ''" >&2 + exit 1 +fi + "$SCRIPT_DIR/fm-pr-check.sh" "$ID" "$URL" grep -qxF "pr=$URL" "$META" || { echo "error: PR metadata recording failed" >&2 diff --git a/bin/fm-test-run.sh b/bin/fm-test-run.sh index ef21cda8335..cd0b9eb8a54 100755 --- a/bin/fm-test-run.sh +++ b/bin/fm-test-run.sh @@ -205,6 +205,7 @@ family_for_basename() { fm-teardown-endpoint-safety.test.sh) printf '%s\n' backend-dispatch ;; + fm-evidence-record.test.sh|\ fm-pr-check-security.test.sh|fm-pr-merge.test.sh|fm-review-diff.test.sh|\ fm-teardown.test.sh|fm-x-mode.test.sh) printf '%s\n' pr-forge diff --git a/docs/architecture.md b/docs/architecture.md index a1d7d77753f..374425f4822 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -246,7 +246,10 @@ A ship brief records its mode as a fixed machine-readable line and the spawn ref When a selected delivery path calls for a diff, `bin/fm-review-diff.sh` refreshes the authoritative base and, when task meta records `pr=`, always fetches and compares against `refs/pull//head` by default (recorded `pr_head=` is only an offline fallback) before falling back to the local branch with a warning. Where a no-mistakes pipeline stores evidence in the repo, it publishes that PR-viewable validation evidence to an orphan evidence branch that shares no history with code branches, so it never enters the crew branch or the default branch. This repo uses that setting, and its own `.no-mistakes/` directory remains local state that stays gitignored and is rejected by CI if tracked; [`configuration.md`](configuration.md) owns the setting. -PR-based task merges go through `bin/fm-pr-merge.sh`, which records `pr=` and any available `pr_head=` through `bin/fm-pr-check.sh` before calling `gh-axi pr merge`. +PR-based task merges go through `bin/fm-pr-merge.sh`, which refuses any merge whose recorded evidence commit is not the pull request's live head, then records `pr=` and any available `pr_head=` through `bin/fm-pr-check.sh` before calling `gh-axi pr merge`. +That evidence commit is `evidence_head=` in the task's metadata, written only by [`bin/fm-evidence-record.sh`](../bin/fm-evidence-record.sh) and read only by the merge: a validation pipeline can commit after the worker's last measurement, so a reported suite figure or exploit result silently becomes a description of an earlier commit unless something compares the two. +The refusal names both commits and the command that records a re-measurement, and an absent record refuses on the same path because a guard that passes when nothing was recorded is defeatable by omission; the remedy is that one command rather than a bypass, so a task predating the record is never stranded. +PR-based ship briefs carry the matching worker contract, and [`bin/fm-pr-merge.sh`](../bin/fm-pr-merge.sh)'s header owns the guard's exact refusal conditions. The helper requires a full `https://github.com///pull/` URL, invokes `gh-axi pr merge --repo /`, defaults to `--squash`, preserves explicit merge-method flags, and rejects malformed URLs or repo override flags before recording merge state; a well-formed GitLab merge request URL (see [docs/gitlab-merge-watch.md](gitlab-merge-watch.md)) is refused too, explicitly, rather than sent to the wrong forge. Teardown is fail-closed for ship worktrees: dirty worktrees refuse, and committed work must be landed before the worktree is returned. [`bin/fm-teardown.sh`](../bin/fm-teardown.sh)'s header owns the landed-work proofs, PR-discovery fallback, and stale-lock recovery procedure. diff --git a/docs/scripts.md b/docs/scripts.md index e94ccb0e16a..5cdc0cd4cc3 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -105,7 +105,8 @@ The shared no-mistakes gate refusal for fleet lifecycle entrypoints is summarize | `fm-pr-poll.sh` | Provide the byte-static watcher program for validated PR/MR-poll sidecars | | `fm-pr-check-migrate.sh` | Quarantine older task polls without execution and rebuild only canonical polls | | `fm-pr-check.sh` | Record validated `pr=` and `pr_head=` values, then atomically arm a static merge poll | -| `fm-pr-merge.sh` | Record PR metadata, then merge a task's canonical full GitHub URL | +| `fm-pr-merge.sh` | Refuse a merge whose recorded evidence commit is not the PR head, then record PR metadata and merge | +| `fm-evidence-record.sh` | Record the commit a task's reported verification evidence was measured on | | `fm-promote.sh` | Promote a scout task in place to a protected ship task with an explicit delivery mode | | `fm-teardown.sh` | Fail-closed teardown: return landed ship worktrees, require completed scout deliverables, retire secondmate homes | | `fm-harness.sh` | Detect the running harness and resolve crew or secondmate harness, model, and effort | diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index c5ee3d00f05..15b319a1dbc 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -354,6 +354,46 @@ test_no_mistakes_dod_wording() { pass "fm-brief.sh: no-mistakes DOD keeps its apostrophe prose, now parse-safe" } +# The evidence-recording contract is what makes bin/fm-pr-merge.sh's refusal +# reachable: only the worker knows which commit its figures came from. It belongs +# to the two PR-based modes and to neither of the paths that never reach a PR +# merge, so an instruction with no enforcer is never scaffolded. +test_pr_modes_require_recording_the_measured_commit() { + local home id brief + home="$TMP_ROOT/evidence-home" + mkdir -p "$home/data" + for id in brief-evidence-nm brief-evidence-dpr; do + case "$id" in + brief-evidence-nm) FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --mode no-mistakes >/dev/null 2>&1 ;; + *) FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --mode direct-PR >/dev/null 2>&1 ;; + esac + brief="$home/data/$id/brief.md" + assert_present "$brief" "$id: brief was not scaffolded" + assert_grep "## Record the commit your evidence was measured on" "$brief" \ + "$id: PR-based brief lost the evidence-recording contract" + # The recorded form is the one the merge guard reads: task id, then the + # commit, resolved in the worktree at measurement time. + assert_grep "bin/fm-evidence-record.sh $id \"\$(git rev-parse HEAD)\"" "$brief" \ + "$id: brief must show the exact recording command, resolving the commit in the worktree" + assert_grep "Record it again after EVERY re-measurement" "$brief" \ + "$id: brief must require re-recording, which is what a final-head instruction cannot win" + assert_grep "The merge refuses when the recorded commit is not the pull request's head" "$brief" \ + "$id: brief must state the consequence that makes the record load-bearing" + done + + for id in brief-evidence-local brief-evidence-scout; do + case "$id" in + brief-evidence-local) FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --mode local-only >/dev/null 2>&1 ;; + *) FM_HOME="$home" "$ROOT/bin/fm-brief.sh" "$id" some-proj --scout >/dev/null 2>&1 ;; + esac + brief="$home/data/$id/brief.md" + assert_present "$brief" "$id: brief was not scaffolded" + assert_no_grep "fm-evidence-record.sh" "$brief" \ + "$id: a scaffold that never reaches the PR merge guard must not carry its recording contract" + done + pass "fm-brief.sh: PR-based ship briefs require recording the commit their evidence was measured on" +} + test_ship_project_memory_wording() { local home id brief home="$TMP_ROOT/project-memory-home" @@ -721,6 +761,7 @@ test_ship_mode_is_explicit_not_registry test_delivery_flags_are_refused_where_they_do_not_apply test_faster_paths_use_configured_authority_without_stacked_review test_no_mistakes_dod_wording +test_pr_modes_require_recording_the_measured_commit test_ship_project_memory_wording test_herdr_lab_contract_is_explicit_and_complete test_herdr_lab_contract_quotes_foreign_firstmate_path diff --git a/tests/fm-evidence-record.test.sh b/tests/fm-evidence-record.test.sh new file mode 100755 index 00000000000..8dbbbe3aef9 --- /dev/null +++ b/tests/fm-evidence-record.test.sh @@ -0,0 +1,232 @@ +#!/usr/bin/env bash +# Tests for bin/fm-evidence-record.sh: the only writer of the evidence record +# bin/fm-pr-merge.sh refuses stale merges against. The record has to survive +# every other metadata mutation, replace itself on re-measurement, and refuse +# anything that could put a wrong or unparsable claim into a task's metadata. +# +# Matrix: +# (a) a record is written and reported, preserving every other meta line +# (b) re-measuring replaces the record instead of stacking a second one +# (c) the note is stored verbatim and read back by the merge guard's reader +# (d) a short, uppercase, or non-hex commit is refused before any write +# (e) a note carrying a newline is refused rather than silently trimmed +# (f) a missing task metadata file is refused +# (g) an unsafe task id never constructs a path +# (h) fm-pr-check.sh's own metadata rewrite preserves the record +# (i) re-recording while a merge poll is armed keeps that poll's metadata valid +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +RECORD="$ROOT/bin/fm-evidence-record.sh" +TMP_ROOT=$(fm_test_tmproot fm-evidence-record-tests) +SHA_A=1111111111111111111111111111111111111111 +SHA_B=2222222222222222222222222222222222222222 + +make_case() { + local name=$1 case_dir + case_dir="$TMP_ROOT/$name" + mkdir -p "$case_dir/state" + fm_write_meta "$case_dir/state/task-e1.meta" \ + "window=fm-task-e1" \ + "worktree=$case_dir/wt" \ + "project=$case_dir/project" \ + "kind=ship" \ + "mode=no-mistakes" + printf '%s\n' "$case_dir" +} + +run_record() { + local case_dir=$1; shift + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + "$RECORD" "$@" +} + +# Read the record back through the same helper bin/fm-pr-merge.sh uses, so the +# test asserts the guard's view of the metadata rather than its own parse of it. +read_back() { + local meta=$1 + ( + # shellcheck source=bin/fm-pr-lib.sh + . "$ROOT/bin/fm-pr-lib.sh" + fm_pr_evidence_read "$meta" || exit 1 + printf '%s|%s\n' "$FM_PR_EVIDENCE_HEAD" "$FM_PR_EVIDENCE_NOTE" + ) +} + +test_records_and_preserves_other_meta() { + local case_dir out + case_dir=$(make_case records) + + out=$(run_record "$case_dir" task-e1 "$SHA_A" 'full suite 4208 pass') \ + || fail "records: fm-evidence-record.sh failed" + + case "$out" in + "recorded: task-e1 evidence measured on $SHA_A (full suite 4208 pass)") ;; + *) fail "records: unexpected confirmation line: $out" ;; + esac + assert_grep "evidence_head=$SHA_A" "$case_dir/state/task-e1.meta" \ + "records: evidence_head was not written" + assert_grep 'evidence_note=full suite 4208 pass' "$case_dir/state/task-e1.meta" \ + "records: evidence_note was not written" + assert_grep 'kind=ship' "$case_dir/state/task-e1.meta" \ + "records: an unrelated meta line was lost" + assert_grep 'mode=no-mistakes' "$case_dir/state/task-e1.meta" \ + "records: an unrelated meta line was lost" + pass "fm-evidence-record.sh records the measured commit and preserves the rest of the metadata" +} + +test_re_measurement_replaces_the_record() { + local case_dir count + case_dir=$(make_case re-measure) + + run_record "$case_dir" task-e1 "$SHA_A" 'full suite 4202 pass' > /dev/null \ + || fail "re-measure: first record failed" + run_record "$case_dir" task-e1 "$SHA_B" 'full suite 4208 pass' > /dev/null \ + || fail "re-measure: second record failed" + + count=$(grep -c '^evidence_head=' "$case_dir/state/task-e1.meta") + [ "$count" = 1 ] || fail "re-measure: expected exactly one evidence_head line, found $count" + count=$(grep -c '^evidence_note=' "$case_dir/state/task-e1.meta") + [ "$count" = 1 ] || fail "re-measure: expected exactly one evidence_note line, found $count" + assert_grep "evidence_head=$SHA_B" "$case_dir/state/task-e1.meta" \ + "re-measure: the re-measured commit did not replace the earlier one" + assert_no_grep "evidence_head=$SHA_A" "$case_dir/state/task-e1.meta" \ + "re-measure: the superseded commit is still recorded" + pass "fm-evidence-record.sh replaces the record on re-measurement instead of stacking claims" +} + +test_record_reads_back_through_the_guard_helper() { + local case_dir got + case_dir=$(make_case read-back) + + run_record "$case_dir" task-e1 "$SHA_A" 'injection exploit blocked' > /dev/null \ + || fail "read-back: record failed" + got=$(read_back "$case_dir/state/task-e1.meta") \ + || fail "read-back: the merge guard's reader rejected a freshly written record" + [ "$got" = "$SHA_A|injection exploit blocked" ] \ + || fail "read-back: the guard read '$got'" + + # An absent record is a readable no-record, distinct from a corrupt one. + fm_write_meta "$case_dir/state/task-e2.meta" "kind=ship" + got=$(read_back "$case_dir/state/task-e2.meta") \ + || fail "read-back: an absent record should read back cleanly as no record" + [ "$got" = "|" ] || fail "read-back: an absent record read as '$got'" + + # A corrupt record must never read back as a match. + printf 'evidence_head=not-a-sha\n' >> "$case_dir/state/task-e2.meta" + read_back "$case_dir/state/task-e2.meta" > /dev/null 2>&1 \ + && fail "read-back: a malformed evidence_head was accepted" + pass "fm-evidence-record.sh writes what the merge guard reads, and corruption never reads as a match" +} + +test_refuses_malformed_commit() { + local case_dir bad rc + case_dir=$(make_case bad-sha) + + for bad in 1111111 "$(printf '%s' "$SHA_A" | tr 'a-f' 'A-F')zz" 'g111111111111111111111111111111111111111' ''; do + set +e + run_record "$case_dir" task-e1 "$bad" > "$case_dir/out" 2> "$case_dir/err" + rc=$? + set -e + expect_code 2 "$rc" "bad-sha: '$bad' should be refused" + assert_grep 'is not a full commit SHA' "$case_dir/err" \ + "bad-sha: refusal for '$bad' did not say what a valid commit looks like" + done + assert_no_grep 'evidence_head=' "$case_dir/state/task-e1.meta" \ + "bad-sha: a refused commit still reached the metadata" + pass "fm-evidence-record.sh refuses a commit that is not a full SHA, before writing anything" +} + +test_refuses_multiline_note() { + local case_dir rc + case_dir=$(make_case bad-note) + + set +e + run_record "$case_dir" task-e1 "$SHA_A" "$(printf 'suite pass\nevidence_head=%s' "$SHA_B")" \ + > "$case_dir/out" 2> "$case_dir/err" + rc=$? + set -e + + expect_code 2 "$rc" "bad-note: a multi-line note should be refused" + assert_grep 'one printable line' "$case_dir/err" \ + "bad-note: refusal did not state the note contract" + assert_no_grep 'evidence_head=' "$case_dir/state/task-e1.meta" \ + "bad-note: a refused note still reached the metadata" + pass "fm-evidence-record.sh refuses a note that could split the record it is stored in" +} + +test_refuses_missing_and_unsafe_tasks() { + local case_dir rc + case_dir=$(make_case missing-task) + + set +e + run_record "$case_dir" task-absent "$SHA_A" > "$case_dir/out" 2> "$case_dir/err" + rc=$? + set -e + expect_code 1 "$rc" "missing-task: an unknown task should be refused" + assert_grep 'task metadata is unavailable' "$case_dir/err" \ + "missing-task: refusal did not explain the missing metadata" + + set +e + run_record "$case_dir" ../escape "$SHA_A" > "$case_dir/out" 2> "$case_dir/err" + rc=$? + set -e + expect_code 2 "$rc" "unsafe-id: a path-unsafe task id should be refused" + assert_grep 'invalid evidence record request' "$case_dir/err" \ + "unsafe-id: refusal was not the fixed, non-probing one" + assert_absent "$case_dir/state/../escape.meta" \ + "unsafe-id: an unsafe task id constructed a path" + pass "fm-evidence-record.sh refuses an unknown task and never builds a path from an unsafe id" +} + +test_record_survives_pr_metadata_rewrite() { + local case_dir fakebin + case_dir=$(make_case survives-pr-check) + fakebin="$case_dir/fakebin" + mkdir -p "$fakebin" "$case_dir/wt" + cat > "$fakebin/gh" < /dev/null \ + || fail "survives-pr-check: record failed" + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + PATH="$fakebin:$PATH" \ + "$ROOT/bin/fm-pr-check.sh" task-e1 https://github.com/example/repo/pull/7 > /dev/null \ + || fail "survives-pr-check: fm-pr-check.sh failed" + + assert_grep "evidence_head=$SHA_A" "$case_dir/state/task-e1.meta" \ + "survives-pr-check: arming the merge poll dropped the evidence record" + assert_grep 'evidence_note=full suite 4208 pass' "$case_dir/state/task-e1.meta" \ + "survives-pr-check: arming the merge poll dropped the evidence note" + + # The refused-merge remedy is to re-measure and re-record, which happens while + # the merge poll is already armed. That rewrite appends the evidence lines + # after pr=, and the watcher revalidates the armed poll against this same + # metadata, so the record has to stay a valid PR record in that arrangement. + run_record "$case_dir" task-e1 "$SHA_B" 'full suite 4212 pass' > /dev/null \ + || fail "survives-pr-check: re-recording on an armed task failed" + ( + # shellcheck source=bin/fm-pr-lib.sh + . "$ROOT/bin/fm-pr-lib.sh" + fm_pr_poll_artifacts_valid "$case_dir/state" task-e1 "$ROOT/bin/fm-pr-poll.sh" + ) || fail "survives-pr-check: re-recording evidence invalidated the armed merge poll" + pass "the evidence record survives arming a merge poll and re-recording against an armed task" +} + +test_records_and_preserves_other_meta +test_re_measurement_replaces_the_record +test_record_reads_back_through_the_guard_helper +test_refuses_malformed_commit +test_refuses_multiline_note +test_refuses_missing_and_unsafe_tasks +test_record_survives_pr_metadata_rewrite diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index 03c6ce688e3..6301f3af85b 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -116,6 +116,14 @@ write_task_meta() { "project=$dir/project" \ "kind=ship" \ "mode=no-mistakes" + # bin/fm-pr-merge.sh refuses a merge whose recorded evidence commit is not the + # PR head, so every merge fixture here needs that record. It is written through + # the real recorder rather than as a literal line, so this suite cannot drift + # from the writer's format. The commit matches the gh mock's default head. + FM_ROOT_OVERRIDE="$ROOT" FM_HOME="$dir/home" \ + "$ROOT/bin/fm-evidence-record.sh" "$id" 0123456789abcdef0123456789abcdef01234567 \ + 'fixture verification run' > /dev/null \ + || fail "write_task_meta: recording the evidence commit for $id failed" } write_poll_meta() { @@ -611,6 +619,10 @@ SH "project=$dir/project" \ 'kind=ship' \ 'mode=local-only' + FM_ROOT_OVERRIDE="$ROOT" FM_HOME="$dir/home" \ + "$ROOT/bin/fm-evidence-record.sh" "$id" 0123456789abcdef0123456789abcdef01234567 \ + 'fixture verification run' > /dev/null \ + || fail "legacy fixture: recording the evidence commit for $id failed" mkdir -p "$dir/home/state/.pr-check-quarantine" chmod 0700 "$dir/home/state/.pr-check-quarantine" printf 'reserved migration evidence\n' \ diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index a064b6919bc..0cfb2d2ca7e 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -14,6 +14,15 @@ # (f) malformed PR URL fails fast without calling gh-axi # (g) explicit merge method is not overridden by the default --squash # (h) repo override args fail fast because the repo comes from the URL +# +# The evidence-head guard is the reason a merge can be refused after every one +# of those checks passes: a worker's reported figures describe the commit they +# were measured on, and a validation pipeline can commit after that measurement. +# (i) a recorded evidence commit equal to the live head merges unchanged +# (j) a recorded evidence commit behind the live head is refused, naming both +# (k) no recorded evidence commit is refused, naming how to record one +# (l) an unconfirmable live head is refused rather than merged unverified +# (m) the refuse -> re-measure -> merge round trip works after a poll is armed set -u # shellcheck source=tests/lib.sh @@ -42,8 +51,20 @@ make_case() { printf '%s\n' "$case_dir" } +# Record a task's evidence commit through the real bin/fm-evidence-record.sh, so +# these cases exercise the same writer a crewmate uses rather than hand-writing +# the metadata line the guard reads. Args: case_dir sha [note] +record_evidence() { + local case_dir=$1 sha=$2 note=${3-} + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + "$ROOT/bin/fm-evidence-record.sh" task-x1 "$sha" "$note" > /dev/null \ + || fail "${case_dir##*/}: recording the evidence commit failed" +} + # gh-axi mock recording every invocation to a log file, and gh mock answering -# headRefOid for fm-pr-check.sh's pr_head lookup. Args: case_dir head_sha +# headRefOid for the merge guard and fm-pr-check.sh's pr_head lookup. +# Args: case_dir head_sha add_gh_mocks() { local case_dir=$1 head=$2 cat > "$case_dir/fakebin/gh-axi" <<'SH' @@ -68,7 +89,7 @@ SH # gh-axi mock that fails the merge call but succeeds everything else, so a # real merge failure is distinguishable from the recording step. add_gh_mocks_merge_fails() { - local case_dir=$1 + local case_dir=$1 head=$2 cat > "$case_dir/fakebin/gh-axi" <<'SH' #!/usr/bin/env bash printf '%s\n' "$*" >> "$FM_TEST_GH_AXI_LOG" @@ -77,13 +98,37 @@ case "${1:-} ${2:-}" in esac exit 0 SH - cat > "$case_dir/fakebin/gh" <<'SH' + cat > "$case_dir/fakebin/gh" < "$case_dir/fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +printf '%s\n' "$*" >> "$FM_TEST_GH_AXI_LOG" +exit 0 +SH + cat > "$case_dir/fakebin/gh" <<'SH' +#!/usr/bin/env bash +echo "error: could not reach GitHub" >&2 +exit 1 +SH + chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" +} + run_pr_merge() { local case_dir=$1 rc; shift FM_ROOT_OVERRIDE="$ROOT" \ @@ -104,6 +149,7 @@ test_records_pr_and_head_before_merging() { case_dir=$(make_case records-before-merge) mkdir -p "$case_dir/wt" add_gh_mocks "$case_dir" deadbeefcafefeed0000000000000000deadbeef + record_evidence "$case_dir" deadbeefcafefeed0000000000000000deadbeef 'full suite 4131 pass' : > "$case_dir/gh-axi.log" set +e @@ -126,7 +172,8 @@ test_merge_failure_propagates_after_recording() { local case_dir rc case_dir=$(make_case merge-fails) mkdir -p "$case_dir/wt" - add_gh_mocks_merge_fails "$case_dir" + add_gh_mocks_merge_fails "$case_dir" 1111111111111111111111111111111111111111 + record_evidence "$case_dir" 1111111111111111111111111111111111111111 : > "$case_dir/gh-axi.log" set +e @@ -146,6 +193,7 @@ test_extra_merge_args_forwarded() { case_dir=$(make_case extra-args) mkdir -p "$case_dir/wt" add_gh_mocks "$case_dir" 2222222222222222222222222222222222222222 + record_evidence "$case_dir" 2222222222222222222222222222222222222222 : > "$case_dir/gh-axi.log" run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/15 -- --squash --delete-branch \ @@ -261,6 +309,7 @@ test_explicit_merge_method_not_overridden() { case_dir=$(make_case explicit-merge-method) mkdir -p "$case_dir/wt" add_gh_mocks "$case_dir" 5555555555555555555555555555555555555555 + record_evidence "$case_dir" 5555555555555555555555555555555555555555 : > "$case_dir/gh-axi.log" run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/22 -- --merge \ @@ -276,6 +325,7 @@ test_method_equals_merge_method_not_overridden() { case_dir=$(make_case method-equals-merge-method) mkdir -p "$case_dir/wt" add_gh_mocks "$case_dir" 7777777777777777777777777777777777777777 + record_evidence "$case_dir" 7777777777777777777777777777777777777777 : > "$case_dir/gh-axi.log" run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/23 -- --method=merge \ @@ -291,6 +341,7 @@ test_parses_pr_url_for_gh_axi() { case_dir=$(make_case url-parsing) mkdir -p "$case_dir/wt" add_gh_mocks "$case_dir" 6666666666666666666666666666666666666666 + record_evidence "$case_dir" 6666666666666666666666666666666666666666 : > "$case_dir/gh-axi.log" run_pr_merge "$case_dir" task-x1 https://github.com/my-org/my-repo/pull/126 \ @@ -301,6 +352,124 @@ test_parses_pr_url_for_gh_axi() { pass "fm-pr-merge parses a GitHub PR URL into gh-axi number and --repo arguments" } +EVIDENCE_MEASURED=a291594aa291594aa291594aa291594aa291594a +EVIDENCE_LIVE_HEAD=44c3c63744c3c63744c3c63744c3c63744c3c637 + +test_matching_evidence_commit_merges() { + local case_dir rc + case_dir=$(make_case evidence-matches) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$EVIDENCE_LIVE_HEAD" + record_evidence "$case_dir" "$EVIDENCE_LIVE_HEAD" 'full suite 4208 pass; injection exploit blocked' + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "evidence-matches: a merge whose evidence commit is the live head should proceed" + grep -qxF 'pr merge 119 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + || fail "evidence-matches: gh-axi pr merge was not invoked for matching evidence" + assert_grep "pr=https://github.com/example/repo/pull/119" "$case_dir/state/task-x1.meta" \ + "evidence-matches: pr= was not recorded on the merging path" + assert_grep "evidence_head=$EVIDENCE_LIVE_HEAD" "$case_dir/state/task-x1.meta" \ + "evidence-matches: the evidence record was not preserved through fm-pr-check.sh" + pass "fm-pr-merge merges unchanged when the evidence commit is the pull request head" +} + +test_stale_evidence_commit_refuses_naming_both() { + local case_dir rc + case_dir=$(make_case evidence-stale) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$EVIDENCE_LIVE_HEAD" + record_evidence "$case_dir" "$EVIDENCE_MEASURED" 'full suite 4202 pass' + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "evidence-stale: fm-pr-merge should refuse evidence measured before the head" + assert_grep "evidence measured on: $EVIDENCE_MEASURED" "$case_dir/stderr" \ + "evidence-stale: the refusal did not name the commit the evidence was measured on" + assert_grep "pull request head: $EVIDENCE_LIVE_HEAD" "$case_dir/stderr" \ + "evidence-stale: the refusal did not name the live pull request head" + assert_grep 'full suite 4202 pass' "$case_dir/stderr" \ + "evidence-stale: the refusal did not say what has to be re-measured" + assert_grep "fm-evidence-record.sh task-x1 $EVIDENCE_LIVE_HEAD" "$case_dir/stderr" \ + "evidence-stale: the refusal did not name the command that records the re-measurement" + assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ + "evidence-stale: gh-axi pr merge was invoked despite stale evidence" + assert_no_grep 'pr=https://github.com/example/repo/pull/119' "$case_dir/state/task-x1.meta" \ + "evidence-stale: a refused merge still recorded PR metadata" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "evidence-stale: a refused merge still armed a merge poll" + pass "fm-pr-merge refuses stale evidence and names both the measured commit and the head" +} + +test_absent_evidence_record_refuses_actionably() { + local case_dir rc + case_dir=$(make_case evidence-absent) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$EVIDENCE_LIVE_HEAD" + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/160 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "evidence-absent: fm-pr-merge should refuse when no evidence commit is recorded" + assert_grep 'no verification evidence commit is recorded' "$case_dir/stderr" \ + "evidence-absent: the refusal did not say the record is missing" + assert_grep 'fm-evidence-record.sh task-x1' "$case_dir/stderr" \ + "evidence-absent: the refusal did not name the command that records the evidence" + assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ + "evidence-absent: gh-axi pr merge was invoked with no evidence recorded" + assert_no_grep 'pr=https://github.com/example/repo/pull/160' "$case_dir/state/task-x1.meta" \ + "evidence-absent: a refused merge still recorded PR metadata" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "evidence-absent: a refused merge still armed a merge poll" + + # The remedy is recording the measured commit, not a bypass: a task that + # predates the evidence record is unblocked by one command, so refusing here + # strands nothing. + record_evidence "$case_dir" "$EVIDENCE_LIVE_HEAD" 'full suite pass' + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/160 \ + > "$case_dir/stdout2" 2> "$case_dir/stderr2" \ + || fail "evidence-absent: recording the evidence commit did not unblock the merge" + grep -qxF 'pr merge 160 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + || fail "evidence-absent: the merge did not proceed after the evidence was recorded" + pass "fm-pr-merge refuses an absent evidence record and names the one command that clears it" +} + +test_unconfirmable_head_refuses() { + local case_dir rc + case_dir=$(make_case evidence-head-unavailable) + mkdir -p "$case_dir/wt" + add_gh_mocks_head_unavailable "$case_dir" + record_evidence "$case_dir" "$EVIDENCE_LIVE_HEAD" 'full suite pass' + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "evidence-head-unavailable: fm-pr-merge should refuse when the head cannot be confirmed" + assert_grep 'pull request head could not be confirmed' "$case_dir/stderr" \ + "evidence-head-unavailable: the refusal did not explain the unconfirmable head" + assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ + "evidence-head-unavailable: gh-axi pr merge was invoked without a confirmed head" + pass "fm-pr-merge refuses rather than merging against a head it could not confirm" +} + test_records_pr_and_head_before_merging test_merge_failure_propagates_after_recording test_extra_merge_args_forwarded @@ -311,3 +480,52 @@ test_repo_override_args_refuse_before_recording test_explicit_merge_method_not_overridden test_method_equals_merge_method_not_overridden test_parses_pr_url_for_gh_axi +# The whole point of the guard is the round trip it forces: a stale merge is +# refused, the worker re-measures on the named head and re-records, and the merge +# then proceeds. By that point the task has already armed its merge poll, so the +# re-recorded lines land after pr= in the metadata - the arrangement that must +# still parse as a valid PR record. +test_re_measured_evidence_merges_after_the_poll_is_armed() { + local case_dir rc + case_dir=$(make_case evidence-re-measured) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$EVIDENCE_LIVE_HEAD" + record_evidence "$case_dir" "$EVIDENCE_LIVE_HEAD" 'full suite 4208 pass' + : > "$case_dir/gh-axi.log" + + # PR-ready arming, exactly as bin/fm-pr-check.sh is run when the PR is reported. + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + PATH="$case_dir/fakebin:$PATH" \ + "$ROOT/bin/fm-pr-check.sh" task-x1 https://github.com/example/repo/pull/119 > /dev/null \ + || fail "evidence-re-measured: arming the merge poll failed" + + # A later pipeline commit moves the head, so the recorded evidence goes stale. + add_gh_mocks "$case_dir" 0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + expect_code 1 "$rc" "evidence-re-measured: a head moved by a later commit should refuse" + assert_grep 'pull request head: 0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f' "$case_dir/stderr" \ + "evidence-re-measured: the refusal did not name the moved head" + + # The worker re-measures on the named head and re-records; the merge proceeds. + record_evidence "$case_dir" 0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 'full suite 4212 pass' + : > "$case_dir/gh-axi.log" + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout2" 2> "$case_dir/stderr2" \ + || fail "evidence-re-measured: re-recording on the live head did not clear the refusal" + grep -qxF 'pr merge 119 --repo example/repo --squash' "$case_dir/gh-axi.log" \ + || fail "evidence-re-measured: the merge did not proceed after re-measurement" + assert_grep 'pr=https://github.com/example/repo/pull/119' "$case_dir/state/task-x1.meta" \ + "evidence-re-measured: the re-recorded metadata no longer holds a valid PR record" + pass "fm-pr-merge completes the refuse, re-measure, and merge round trip on an armed task" +} + +test_matching_evidence_commit_merges +test_stale_evidence_commit_refuses_naming_both +test_absent_evidence_record_refuses_actionably +test_unconfirmable_head_refuses +test_re_measured_evidence_merges_after_the_poll_is_armed From ed731756776afe9352fed439bf8863f39cbb6993 Mon Sep 17 00:00:00 2001 From: clca Date: Wed, 19 Aug 2026 16:49:25 +0200 Subject: [PATCH 2/5] no-mistakes(review): fix(pr): widen identity allowlist, trap evidence writes, split refusals --- bin/fm-evidence-record.sh | 7 ++ bin/fm-pr-lib.sh | 49 +++++++--- bin/fm-pr-merge.sh | 20 ++++- docs/architecture.md | 1 + tests/fm-evidence-record.test.sh | 149 ++++++++++++++++++++++++++----- tests/fm-pr-merge.test.sh | 70 ++++++++++++++- 6 files changed, 257 insertions(+), 39 deletions(-) diff --git a/bin/fm-evidence-record.sh b/bin/fm-evidence-record.sh index 2dba76ef5b8..18ed7ae7280 100755 --- a/bin/fm-evidence-record.sh +++ b/bin/fm-evidence-record.sh @@ -45,6 +45,13 @@ esac # shellcheck source=bin/fm-wake-lib.sh . "$SCRIPT_DIR/fm-wake-lib.sh" +# An interrupted write must leave the state directory exactly as it found it: +# no half-written temp beside the metadata, and no per-task lock another +# process would have to reclaim through stale-owner recovery. This mirrors +# bin/fm-pr-check.sh, the other writer of this same record. +trap fm_pr_evidence_cleanup EXIT +trap 'exit 1' HUP INT TERM + if [ "$#" -lt 2 ] || [ "$#" -gt 3 ]; then echo "error: invalid evidence record request" >&2 echo "usage: fm-evidence-record.sh [one-line note]" >&2 diff --git a/bin/fm-pr-lib.sh b/bin/fm-pr-lib.sh index b095313a4b9..44d3fa2ed30 100755 --- a/bin/fm-pr-lib.sh +++ b/bin/fm-pr-lib.sh @@ -222,6 +222,9 @@ fm_pr_head_valid() { # presented as the merging head's result. FM_PR_EVIDENCE_HEAD= FM_PR_EVIDENCE_NOTE= +FM_PR_EVIDENCE_TMP= +FM_PR_EVIDENCE_LOCK= +FM_PR_EVIDENCE_LOCK_HELD=0 # fm_pr_evidence_note_valid : a note is one printable line, bounded, so it # can never split the key=value record it is stored in. @@ -252,12 +255,27 @@ fm_pr_evidence_read() { FM_PR_EVIDENCE_NOTE=$note } +# fm_pr_evidence_cleanup: remove any in-progress evidence temp and release the +# per-task metadata lock if this process still holds it. Callers install it as +# their EXIT trap, so an interrupted write leaves neither a stray temp file in +# the state directory nor a lock another process has to reclaim through +# stale-owner recovery. Releasing is idempotent: the held flag is cleared here, +# so the normal path and the trap together release exactly once. +fm_pr_evidence_cleanup() { + [ -z "$FM_PR_EVIDENCE_TMP" ] || rm -f -- "$FM_PR_EVIDENCE_TMP" + FM_PR_EVIDENCE_TMP= + if [ "$FM_PR_EVIDENCE_LOCK_HELD" = 1 ]; then + fm_lock_release "$FM_PR_EVIDENCE_LOCK" || true + FM_PR_EVIDENCE_LOCK_HELD=0 + fi +} + # fm_pr_evidence_write [note]: atomically replace the task's # evidence record, preserving every other metadata line, and re-read the result # so a partially written record can never be reported as recorded. Requires # bin/fm-wake-lib.sh for the shared per-task metadata lock. fm_pr_evidence_write() { - local meta=$1 head=$2 note=${3-} dir base tmp lock rc=0 + local meta=$1 head=$2 note=${3-} dir base rc=0 fm_pr_head_valid "$head" || return 1 fm_pr_evidence_note_valid "$note" || return 1 [ -f "$meta" ] && [ ! -L "$meta" ] || return 1 @@ -265,24 +283,25 @@ fm_pr_evidence_write() { dir=${meta%/*} base=${meta##*/} [ "$dir" != "$meta" ] || dir=. - lock=$(fm_meta_lock_path "$meta") || return 1 - fm_lock_acquire_wait "$lock" - tmp=$(mktemp "$dir/.${base}.fm-evidence.XXXXXX") || { fm_lock_release "$lock"; return 1; } + FM_PR_EVIDENCE_LOCK=$(fm_meta_lock_path "$meta") || return 1 + fm_lock_acquire_wait "$FM_PR_EVIDENCE_LOCK" + FM_PR_EVIDENCE_LOCK_HELD=1 + FM_PR_EVIDENCE_TMP=$(mktemp "$dir/.${base}.fm-evidence.XXXXXX") \ + || { fm_pr_evidence_cleanup; return 1; } while :; do [ -f "$meta" ] && [ ! -L "$meta" ] && [ "$(fm_pr_file_link_count "$meta")" = 1 ] || { rc=1; break; } fm_pr_regular_destination_or_absent "$meta" || { rc=1; break; } - { grep -vE '^evidence_head=|^evidence_note=' "$meta" || true; } > "$tmp" || { rc=1; break; } - printf 'evidence_head=%s\n' "$head" >> "$tmp" || { rc=1; break; } + { grep -vE '^evidence_head=|^evidence_note=' "$meta" || true; } > "$FM_PR_EVIDENCE_TMP" || { rc=1; break; } + printf 'evidence_head=%s\n' "$head" >> "$FM_PR_EVIDENCE_TMP" || { rc=1; break; } if [ -n "$note" ]; then - printf 'evidence_note=%s\n' "$note" >> "$tmp" || { rc=1; break; } + printf 'evidence_note=%s\n' "$note" >> "$FM_PR_EVIDENCE_TMP" || { rc=1; break; } fi - chmod 0600 "$tmp" || { rc=1; break; } - mv -f -- "$tmp" "$meta" || { rc=1; break; } - tmp= + chmod 0600 "$FM_PR_EVIDENCE_TMP" || { rc=1; break; } + mv -f -- "$FM_PR_EVIDENCE_TMP" "$meta" || { rc=1; break; } + FM_PR_EVIDENCE_TMP= break done - [ -z "$tmp" ] || rm -f -- "$tmp" - fm_lock_release "$lock" + fm_pr_evidence_cleanup [ "$rc" = 0 ] || return 1 fm_pr_evidence_read "$meta" || return 1 [ "$FM_PR_EVIDENCE_HEAD" = "$head" ] && [ "$FM_PR_EVIDENCE_NOTE" = "$note" ] @@ -398,6 +417,12 @@ fm_pr_metadata_identity_parse() { # refuses the merge on a malformed record rather than passing it here. evidence_head=*|evidence_note=*) ;; + # bin/fm-spawn.sh appends both of these after pr= as well: + # spawn_record_traceparent strips the old traceparent= line and appends + # the new one at the end of the record, and control_relaunch_tx= is + # emitted after preserve_relaunch_meta has already re-emitted pr=. + traceparent=*|control_relaunch_tx=*) + ;; *) [ "$seen_pr" -eq 0 ] || post_pr_invalid=1 ;; diff --git a/bin/fm-pr-merge.sh b/bin/fm-pr-merge.sh index d8859126c04..c8674553d4f 100755 --- a/bin/fm-pr-merge.sh +++ b/bin/fm-pr-merge.sh @@ -15,6 +15,12 @@ # into a description of an earlier commit; this guard is what makes that # staleness stop a merge instead of depending on a manual comparison. # +# Reading that live head requires the plain GitHub CLI (gh) on PATH, which this +# path therefore depends on in addition to gh-axi: gh-axi cannot report a head +# commit at all, so a host with only gh-axi is blocked from merging here. The +# refusal names the missing binary, because the alternative to blocking is +# merging unverified, which is the exact failure this guard exists to prevent. +# # An absent record refuses too, rather than warning and merging. A guard that # passes when nothing was recorded is defeatable by simply never recording, and # nothing distinguishes "no claim was made" from "the claim was lost". The @@ -107,14 +113,20 @@ fi # The live head is read here rather than taken from a recorded pr_head=, so the # comparison is always against what would actually merge. LIVE_HEAD= -if command -v gh >/dev/null 2>&1; then - if REMOTE_HEAD=$(gh pr view "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" --json headRefOid -q .headRefOid 2>/dev/null); then - LIVE_HEAD=$(printf '%s' "$REMOTE_HEAD" | tr -d '[:space:]' | tr '[:upper:]' '[:lower:]') - fi +if ! command -v gh >/dev/null 2>&1; then + echo "error: refusing to merge $URL: the merge guard reads the pull request head with the GitHub CLI (gh), which is not on PATH" >&2 + echo " the recorded evidence commit for task $ID is $EVIDENCE_HEAD" >&2 + echo " without the live head there is nothing to compare it against, so the merge stops here" >&2 + echo " fix: install the GitHub CLI (gh), then merge again" >&2 + exit 1 +fi +if REMOTE_HEAD=$(gh pr view "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" --json headRefOid -q .headRefOid 2>/dev/null); then + LIVE_HEAD=$(printf '%s' "$REMOTE_HEAD" | tr -d '[:space:]' | tr '[:upper:]' '[:lower:]') fi if ! fm_pr_head_valid "$LIVE_HEAD"; then echo "error: refusing to merge $URL: the pull request head could not be confirmed" >&2 echo " the recorded evidence commit for task $ID is $EVIDENCE_HEAD" >&2 + echo " gh is installed but did not answer with a commit for this pull request" >&2 echo " without the live head there is nothing to compare it against, so the merge stops here" >&2 echo " fix: restore GitHub access (gh auth status), then merge again" >&2 exit 1 diff --git a/docs/architecture.md b/docs/architecture.md index 374425f4822..0035029c864 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -249,6 +249,7 @@ This repo uses that setting, and its own `.no-mistakes/` directory remains local PR-based task merges go through `bin/fm-pr-merge.sh`, which refuses any merge whose recorded evidence commit is not the pull request's live head, then records `pr=` and any available `pr_head=` through `bin/fm-pr-check.sh` before calling `gh-axi pr merge`. That evidence commit is `evidence_head=` in the task's metadata, written only by [`bin/fm-evidence-record.sh`](../bin/fm-evidence-record.sh) and read only by the merge: a validation pipeline can commit after the worker's last measurement, so a reported suite figure or exploit result silently becomes a description of an earlier commit unless something compares the two. The refusal names both commits and the command that records a re-measurement, and an absent record refuses on the same path because a guard that passes when nothing was recorded is defeatable by omission; the remedy is that one command rather than a bypass, so a task predating the record is never stranded. +The guard reads that live head with the plain GitHub CLI (`gh`), so a host that merges through this path needs `gh` on PATH in addition to `gh-axi`, which cannot report a head commit at all; a host without `gh` is refused by name rather than merged unverified. PR-based ship briefs carry the matching worker contract, and [`bin/fm-pr-merge.sh`](../bin/fm-pr-merge.sh)'s header owns the guard's exact refusal conditions. The helper requires a full `https://github.com///pull/` URL, invokes `gh-axi pr merge --repo /`, defaults to `--squash`, preserves explicit merge-method flags, and rejects malformed URLs or repo override flags before recording merge state; a well-formed GitLab merge request URL (see [docs/gitlab-merge-watch.md](gitlab-merge-watch.md)) is refused too, explicitly, rather than sent to the wrong forge. Teardown is fail-closed for ship worktrees: dirty worktrees refuse, and committed work must be landed before the worktree is returned. diff --git a/tests/fm-evidence-record.test.sh b/tests/fm-evidence-record.test.sh index 8dbbbe3aef9..e0b85999aa4 100755 --- a/tests/fm-evidence-record.test.sh +++ b/tests/fm-evidence-record.test.sh @@ -14,6 +14,10 @@ # (g) an unsafe task id never constructs a path # (h) fm-pr-check.sh's own metadata rewrite preserves the record # (i) re-recording while a merge poll is armed keeps that poll's metadata valid +# (j) the keys a relaunch appends after pr= keep that armed poll valid too, +# while an unknown key appearing there still invalidates the record +# (k) an interrupted write leaves no temp file, no held lock, and no partial +# record behind set -u # shellcheck source=tests/lib.sh @@ -56,6 +60,36 @@ read_back() { ) } +# Ask the watcher's own validator whether this task's armed merge poll still +# holds, which is the consumer that revalidates a record after it is rewritten. +poll_artifacts_valid() { + local case_dir=$1 + ( + # shellcheck source=bin/fm-pr-lib.sh + . "$ROOT/bin/fm-pr-lib.sh" + fm_pr_poll_artifacts_valid "$case_dir/state" task-e1 "$ROOT/bin/fm-pr-poll.sh" + ) +} + +# Arm a merge poll the way bin/fm-pr-check.sh is run when a PR is reported. +# Args: case_dir head_sha pr_url +arm_merge_poll() { + local case_dir=$1 head=$2 url=$3 + mkdir -p "$case_dir/fakebin" "$case_dir/wt" + cat > "$case_dir/fakebin/gh" < /dev/null +} + test_records_and_preserves_other_meta() { local case_dir out case_dir=$(make_case records) @@ -183,25 +217,12 @@ test_refuses_missing_and_unsafe_tasks() { } test_record_survives_pr_metadata_rewrite() { - local case_dir fakebin + local case_dir case_dir=$(make_case survives-pr-check) - fakebin="$case_dir/fakebin" - mkdir -p "$fakebin" "$case_dir/wt" - cat > "$fakebin/gh" < /dev/null \ || fail "survives-pr-check: record failed" - FM_ROOT_OVERRIDE="$ROOT" \ - FM_STATE_OVERRIDE="$case_dir/state" \ - PATH="$fakebin:$PATH" \ - "$ROOT/bin/fm-pr-check.sh" task-e1 https://github.com/example/repo/pull/7 > /dev/null \ + arm_merge_poll "$case_dir" "$SHA_A" https://github.com/example/repo/pull/7 \ || fail "survives-pr-check: fm-pr-check.sh failed" assert_grep "evidence_head=$SHA_A" "$case_dir/state/task-e1.meta" \ @@ -215,14 +236,100 @@ SH # metadata, so the record has to stay a valid PR record in that arrangement. run_record "$case_dir" task-e1 "$SHA_B" 'full suite 4212 pass' > /dev/null \ || fail "survives-pr-check: re-recording on an armed task failed" - ( - # shellcheck source=bin/fm-pr-lib.sh - . "$ROOT/bin/fm-pr-lib.sh" - fm_pr_poll_artifacts_valid "$case_dir/state" task-e1 "$ROOT/bin/fm-pr-poll.sh" - ) || fail "survives-pr-check: re-recording evidence invalidated the armed merge poll" + poll_artifacts_valid "$case_dir" \ + || fail "survives-pr-check: re-recording evidence invalidated the armed merge poll" pass "the evidence record survives arming a merge poll and re-recording against an armed task" } +# The evidence keys are not the only ones a legitimate writer appends after pr=. +# Relaunching a crewmate to address review comments rewrites the same record: +# bin/fm-spawn.sh strips and re-appends traceparent= at the end when trace +# context is on, and appends control_relaunch_tx= after the preserved pr= line. +# The watcher revalidates the armed poll against that metadata, so an armed +# merge poll has to survive both without the record becoming invalid. +test_relaunch_appended_keys_keep_the_armed_poll_valid() { + local case_dir meta + case_dir=$(make_case relaunch-keys) + meta="$case_dir/state/task-e1.meta" + + run_record "$case_dir" task-e1 "$SHA_A" 'full suite 4208 pass' > /dev/null \ + || fail "relaunch-keys: record failed" + arm_merge_poll "$case_dir" "$SHA_A" https://github.com/example/repo/pull/11 \ + || fail "relaunch-keys: arming the merge poll failed" + poll_artifacts_valid "$case_dir" \ + || fail "relaunch-keys: the freshly armed merge poll was already invalid" + + printf 'traceparent=00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01\n' >> "$meta" + poll_artifacts_valid "$case_dir" \ + || fail "relaunch-keys: a relaunch-recorded traceparent invalidated the armed merge poll" + printf 'control_relaunch_tx=tx-2026-08-19-1\n' >> "$meta" + poll_artifacts_valid "$case_dir" \ + || fail "relaunch-keys: a control relaunch transaction invalidated the armed merge poll" + + # The allowlist stays an allowlist: an unknown key after pr= is still refused, + # which is what catches a line injected through a forge-supplied value. + printf 'window=unexpected\n' >> "$meta" + if poll_artifacts_valid "$case_dir"; then + fail "relaunch-keys: an unknown key after pr= was accepted" + fi + pass "the keys a relaunch appends after pr= keep an armed merge poll valid, and unknown ones still do not" +} + +# bin/fm-pr-check.sh, the other writer of this record, cleans up its temp file +# and releases the per-task lock when it is interrupted. This writer must behave +# the same way, or an interrupted recording leaves a stray temp beside the +# metadata and a lock the next writer has to wait out through stale-owner +# recovery. The shimmed grep signals the recorder inside the exact window: after +# mktemp created the temp, before the record is moved into place. +test_interrupted_write_leaves_no_temp_or_held_lock() { + local case_dir meta lock real_grep leftover rc + case_dir=$(make_case interrupted) + meta="$case_dir/state/task-e1.meta" + lock="$case_dir/state/.meta-task-e1.lock" + real_grep=$(command -v grep) + mkdir -p "$case_dir/fakebin" + cat > "$case_dir/fakebin/grep" < "$case_dir/signalled" + kill -TERM "\$PPID" 2>/dev/null || true + fi + ;; +esac +exec $real_grep "\$@" +SH + chmod +x "$case_dir/fakebin/grep" + + set +e + FM_ROOT_OVERRIDE="$ROOT" \ + FM_STATE_OVERRIDE="$case_dir/state" \ + PATH="$case_dir/fakebin:$PATH" \ + "$RECORD" task-e1 "$SHA_A" 'full suite 4208 pass' > "$case_dir/out" 2> "$case_dir/err" + rc=$? + set -e + + [ "$rc" -ne 0 ] || fail "interrupted: an interrupted recording reported success" + assert_present "$case_dir/signalled" \ + "interrupted: the recorder was never signalled mid-write" + leftover=$(find "$case_dir/state" -name '.task-e1.meta.fm-evidence.*' | head -1) + [ -z "$leftover" ] || fail "interrupted: a temp file was left behind: $leftover" + { [ ! -e "$lock" ] && [ ! -L "$lock" ]; } \ + || fail "interrupted: the per-task metadata lock was left held" + assert_grep 'kind=ship' "$meta" "interrupted: the task metadata was damaged" + assert_no_grep 'evidence_head=' "$meta" \ + "interrupted: a partial record reached the metadata" + + # The lock is free, so the next recording completes without waiting on + # another process's stale-owner recovery. + run_record "$case_dir" task-e1 "$SHA_B" 'full suite 4212 pass' > /dev/null \ + || fail "interrupted: a later recording could not take the lock" + assert_grep "evidence_head=$SHA_B" "$meta" \ + "interrupted: the later recording did not land" + pass "an interrupted recording leaves no temp file, no held lock, and no partial record" +} + test_records_and_preserves_other_meta test_re_measurement_replaces_the_record test_record_reads_back_through_the_guard_helper @@ -230,3 +337,5 @@ test_refuses_malformed_commit test_refuses_multiline_note test_refuses_missing_and_unsafe_tasks test_record_survives_pr_metadata_rewrite +test_relaunch_appended_keys_keep_the_armed_poll_valid +test_interrupted_write_leaves_no_temp_or_held_lock diff --git a/tests/fm-pr-merge.test.sh b/tests/fm-pr-merge.test.sh index 0cfb2d2ca7e..594c31b6cdc 100755 --- a/tests/fm-pr-merge.test.sh +++ b/tests/fm-pr-merge.test.sh @@ -21,8 +21,11 @@ # (i) a recorded evidence commit equal to the live head merges unchanged # (j) a recorded evidence commit behind the live head is refused, naming both # (k) no recorded evidence commit is refused, naming how to record one -# (l) an unconfirmable live head is refused rather than merged unverified -# (m) the refuse -> re-measure -> merge round trip works after a poll is armed +# (l) a live head gh could not answer for is refused rather than merged +# unverified, pointing at GitHub access +# (m) a missing plain gh is refused by its own cause, naming the binary the +# guard needs rather than an auth check that cannot diagnose it +# (n) the refuse -> re-measure -> merge round trip works after a poll is armed set -u # shellcheck source=tests/lib.sh @@ -129,12 +132,25 @@ SH chmod +x "$case_dir/fakebin/gh-axi" "$case_dir/fakebin/gh" } +# PATH with every directory that provides an executable `gh` removed, so a case +# can stand in for a host that has gh-axi but no plain GitHub CLI without +# guessing where gh is installed. +path_without_gh() { + local dir out= + while IFS= read -r dir; do + [ -n "$dir" ] || continue + [ -x "$dir/gh" ] && continue + out="${out:+$out:}$dir" + done < <(printf '%s\n' "$PATH" | tr ':' '\n') + printf '%s\n' "$out" +} + run_pr_merge() { local case_dir=$1 rc; shift FM_ROOT_OVERRIDE="$ROOT" \ FM_STATE_OVERRIDE="$case_dir/state" \ FM_TEST_GH_AXI_LOG="$case_dir/gh-axi.log" \ - PATH="$case_dir/fakebin:$PATH" \ + PATH="$case_dir/fakebin:${FM_TEST_BASE_PATH:-$PATH}" \ "$PR_MERGE" "$@" rc=$? if [ "${case_dir##*/}" = unsafe-url-segment ] && [ "$rc" -eq 2 ]; then @@ -465,11 +481,58 @@ test_unconfirmable_head_refuses() { expect_code 1 "$rc" "evidence-head-unavailable: fm-pr-merge should refuse when the head cannot be confirmed" assert_grep 'pull request head could not be confirmed' "$case_dir/stderr" \ "evidence-head-unavailable: the refusal did not explain the unconfirmable head" + assert_grep 'gh is installed but did not answer' "$case_dir/stderr" \ + "evidence-head-unavailable: the refusal did not distinguish an installed gh that could not answer" + assert_grep "the recorded evidence commit for task task-x1 is $EVIDENCE_LIVE_HEAD" "$case_dir/stderr" \ + "evidence-head-unavailable: the refusal did not name the recorded evidence commit" + assert_grep 'gh auth status' "$case_dir/stderr" \ + "evidence-head-unavailable: the refusal did not point at GitHub access" assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ "evidence-head-unavailable: gh-axi pr merge was invoked without a confirmed head" pass "fm-pr-merge refuses rather than merging against a head it could not confirm" } +# A host with gh-axi but no plain gh cannot read a pull request head at all, so +# it is blocked by design. It has to be blocked by its own cause: an auth check +# diagnoses nothing when the binary is simply absent. +test_missing_gh_cli_refuses_by_its_own_cause() { + local case_dir rc + local FM_TEST_BASE_PATH + case_dir=$(make_case evidence-gh-missing) + mkdir -p "$case_dir/wt" + add_gh_mocks "$case_dir" "$EVIDENCE_LIVE_HEAD" + record_evidence "$case_dir" "$EVIDENCE_LIVE_HEAD" 'full suite pass' + rm -f "$case_dir/fakebin/gh" + FM_TEST_BASE_PATH=$(path_without_gh) + if PATH="$case_dir/fakebin:$FM_TEST_BASE_PATH" command -v gh > /dev/null 2>&1; then + fail "evidence-gh-missing: the sandbox PATH still provides gh" + fi + : > "$case_dir/gh-axi.log" + + set +e + run_pr_merge "$case_dir" task-x1 https://github.com/example/repo/pull/119 \ + > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "evidence-gh-missing: fm-pr-merge should refuse when gh is not on PATH" + assert_grep 'GitHub CLI (gh), which is not on PATH' "$case_dir/stderr" \ + "evidence-gh-missing: the refusal did not name the missing binary" + assert_grep 'fix: install the GitHub CLI (gh)' "$case_dir/stderr" \ + "evidence-gh-missing: the refusal did not name the one step that fixes it" + assert_no_grep 'gh auth status' "$case_dir/stderr" \ + "evidence-gh-missing: the refusal pointed at an auth check that cannot diagnose a missing binary" + assert_grep "the recorded evidence commit for task task-x1 is $EVIDENCE_LIVE_HEAD" "$case_dir/stderr" \ + "evidence-gh-missing: the refusal did not name the recorded evidence commit" + assert_no_grep 'pr merge' "$case_dir/gh-axi.log" \ + "evidence-gh-missing: gh-axi pr merge was invoked with no way to read the head" + assert_no_grep 'pr=https://github.com/example/repo/pull/119' "$case_dir/state/task-x1.meta" \ + "evidence-gh-missing: a refused merge still recorded PR metadata" + assert_absent "$case_dir/state/task-x1.check.sh" \ + "evidence-gh-missing: a refused merge still armed a merge poll" + pass "fm-pr-merge refuses a missing GitHub CLI by name instead of pointing at an auth check" +} + test_records_pr_and_head_before_merging test_merge_failure_propagates_after_recording test_extra_merge_args_forwarded @@ -528,4 +591,5 @@ test_matching_evidence_commit_merges test_stale_evidence_commit_refuses_naming_both test_absent_evidence_record_refuses_actionably test_unconfirmable_head_refuses +test_missing_gh_cli_refuses_by_its_own_cause test_re_measured_evidence_merges_after_the_poll_is_armed From 83cdf5a6937af4996ad649d77e354d967055147d Mon Sep 17 00:00:00 2001 From: clca Date: Wed, 19 Aug 2026 21:25:58 +0200 Subject: [PATCH 3/5] fix(pr): tolerate unknown metadata keys, reach the evidence contract 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. --- bin/fm-brief.sh | 21 ++++++---- bin/fm-pr-lib.sh | 44 +++++++++++++------- tests/fm-brief.test.sh | 27 ++++++++++++ tests/fm-evidence-record.test.sh | 67 +++++++++++++++++++++++------- tests/fm-pr-check-security.test.sh | 14 ++++++- 5 files changed, 135 insertions(+), 38 deletions(-) diff --git a/bin/fm-brief.sh b/bin/fm-brief.sh index f74cb53e81f..e16c24ddfcc 100755 --- a/bin/fm-brief.sh +++ b/bin/fm-brief.sh @@ -54,6 +54,11 @@ # on through bin/fm-evidence-record.sh, and bin/fm-pr-merge.sh refuses to merge # unless that commit is still the PR head. local-only and scout scaffolds omit it # because neither reaches that merge path. +# That section must stay ABOVE every line that tells the worker it is finished, +# and each finishing line must name recording as part of its own condition. A +# worker reads the definition of done in order and stops at the first line that +# says it may; an evidence section placed after that line is unreachable in the +# ordinary case, which is exactly how the merge guard's own input went missing. # Ship tasks include a project-memory section so durable project-intrinsic # learnings can be committed to AGENTS.md through the project's delivery path; # it carries the AGENTS.md authoring bar (widely useful knowledge only, pointers @@ -384,11 +389,12 @@ case "$MODE" in # Definition of done Delivery contract: mode=direct-PR This task ships **direct-PR**: you raise the PR yourself, without the no-mistakes pipeline. -The task is complete only when committed on your branch. -When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, then append \`done: PR {url}\` to the status file and stop. -Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome. +The task is complete only when committed on your branch AND the commit your reported verification was measured on is recorded. $EVIDENCE_SECTION + +When it is implemented and committed, push your branch and open a PR with \`gh-axi\`, record the commit your reported verification was measured on as above, then append \`done: PR {url}\` to the status file and stop. +Do NOT run /no-mistakes. The configured merge authority decides whether to merge the PR; firstmate relays the outcome. EOF ;; local-only) @@ -411,7 +417,10 @@ EOF IFS= read -r -d '' DOD <: true when a metadata line is a well-formed +# `key=value` record line, where the key is a bare identifier. Every metadata +# key firstmate writes has that shape, so the test admits a key this parser has +# never seen while still rejecting a line that is not a record line at all. +fm_pr_meta_key_line() { + local LC_ALL=C + [[ "${1-}" =~ ^[A-Za-z_][A-Za-z0-9_]*= ]] +} + +# What this parse tolerates after pr=, and what it still refuses. +# +# Tolerated: any well-formed `key=value` line whose key this parser does not +# recognise. Enumerating the permitted keys was tried and was wrong three times +# running - the evidence recorder, bin/fm-spawn.sh's traceparent and relaunch +# transaction, and bin/fm-decision-hold.sh's review record each append after +# pr=, and nothing stops a fourth writer appearing. Every such writer was a +# latent refusal of a merge on valid work, found only when a correct merge was +# wrongly blocked. Position in the record carries no meaning for keys this +# function does not read, so an unrecognised key is not evidence of corruption. +# +# Still refused, loudly: a line that is not a record line at all (no `key=` +# prefix, an empty line, or a fragment left by a truncated write), a second pr= +# line, a pr= whose URL does not parse, and a pr_head= that is not a commit. +# Those are the shapes a value carrying an embedded newline would inject, and +# they remain the reason this positional check exists. Do not relax this into +# accepting any line: the `key=value` shape is the whole remaining guard. fm_pr_metadata_identity_parse() { local file=$1 line value pr_count=0 seen_pr=0 post_pr_invalid=0 FM_PR_META_PROVIDER= @@ -409,22 +435,10 @@ fm_pr_metadata_identity_parse() { fm_pr_head_valid "$value" || post_pr_invalid=1 fi ;; - x_request=*|x_request_ts=*|x_followups=*|x_platform=*|x_reply_max_chars=*) - ;; - # A re-measurement recorded after the merge poll was armed rewrites the - # evidence lines to the end of the record, so they legitimately appear - # after pr=. Their own validity is enforced by fm_pr_evidence_read, which - # refuses the merge on a malformed record rather than passing it here. - evidence_head=*|evidence_note=*) - ;; - # bin/fm-spawn.sh appends both of these after pr= as well: - # spawn_record_traceparent strips the old traceparent= line and appends - # the new one at the end of the record, and control_relaunch_tx= is - # emitted after preserve_relaunch_meta has already re-emitted pr=. - traceparent=*|control_relaunch_tx=*) - ;; *) - [ "$seen_pr" -eq 0 ] || post_pr_invalid=1 + if [ "$seen_pr" -eq 1 ] && ! fm_pr_meta_key_line "$line"; then + post_pr_invalid=1 + fi ;; esac done < "$file" diff --git a/tests/fm-brief.test.sh b/tests/fm-brief.test.sh index 15b319a1dbc..dfa7aac2982 100755 --- a/tests/fm-brief.test.sh +++ b/tests/fm-brief.test.sh @@ -354,6 +354,25 @@ test_no_mistakes_dod_wording() { pass "fm-brief.sh: no-mistakes DOD keeps its apostrophe prose, now parse-safe" } +# assert_evidence_precedes_every_finish