diff --git a/.github/pr-bodies/39.md b/.github/pr-bodies/39.md new file mode 100644 index 00000000000..c7b513f9d92 --- /dev/null +++ b/.github/pr-bodies/39.md @@ -0,0 +1,38 @@ +## CEO overview + +- **What is changing:** Firstmate only reports a worker's pull request as merged when it has matching merge evidence. +- **Why it matters:** A finished worker could previously appear to have landed its work while its pull request was still open. +- **Customer or business impact:** The captain can distinguish work ready for review from work already merged, reducing the risk of overlooking unfinished delivery. +- **Risk and rollout:** Low risk; this delivery stops when CI is ready for review, with merging reserved for the captain's approval. + Active workers keep their current status, and safeguards still refuse to remove unlanded work. + Older runs without matching evidence may now show that their merge is unverified. + +## What changed technically + +- Passed crew runs require matching current-run PR identity, task metadata, and owned PR-poll merge evidence before displaying `PR merged/closed`. +- Missing or conflicting evidence produces `PR merge unverified`; runs without valid PR metadata retain plain `run passed`. +- Regression coverage exercises the public crew-state interface and teardown refusal, including stale evidence for a different PR on the same branch and head. +- The decision re-arm test uses changing pane output and waits for observed captures so an independent idle-pane wake cannot be mistaken for a duplicate decision. + +## Validation + +- **Checks passed:** The preceding pipeline phases passed targeted crew-state, PR-poll, teardown, shell portability, lint, and documentation checks. + CI also passed both portable parallel shards, serial shards 1, 3, and 4, Herdr, stock macOS Bash compatibility, coverage, repository invariants, and the no-mistakes requirement. + After the CI repair, the full inactive-reconciliation and PR communication suites, both PR-body checkers, targeted ShellCheck, Bash syntax, documentation audience checks, and diff checks passed locally. +- **Checks not run:** Hosted communication checks have not passed against this replacement narrative; the live PR description still uses the rejected legacy headings. + The outer executor must compose this narrative with the latest live body using `bin/fm-pr-body-compose.sh`, publish the result while preserving the machine-owned Pipeline section, and verify the resulting hosted checks. + No live forge mutation or merge was performed. +- **Evidence and limitations:** Isolated public-script fixtures reproduced the false merge claim and verified the corrected open, merged, and closed-unmerged paths, while teardown preserved unlanded work. + The live PR body reproduces all nine missing-field failures locally; replacing only its narrative with this file passes both executable body checkers while preserving the existing Pipeline suffix. + Committing this file alone does not update the live body that GitHub Actions assesses. + The [crew-state transcript](https://github.com/bingb0t5/firstmate/blob/806e500eef89f3d967310faf328a1c8a3945ecc1/.no-mistakes/evidence/fm/fm-crew-state-open-pr-truth-r1/crew-state-transcript.txt), [poll-to-watcher transcript](https://github.com/bingb0t5/firstmate/blob/806e500eef89f3d967310faf328a1c8a3945ecc1/.no-mistakes/evidence/fm/fm-crew-state-open-pr-truth-r1/poll-to-crew-transcript.txt), and [teardown transcript](https://github.com/bingb0t5/firstmate/blob/806e500eef89f3d967310faf328a1c8a3945ecc1/.no-mistakes/evidence/fm/fm-crew-state-open-pr-truth-r1/teardown-transcript.txt) use controlled forge responses rather than live GitHub state. + +## Module-boundary decision + +Current module retained: crew-state owns the displayed run verdict and reuses the existing PR identity and owned merge-evidence helpers. +No new merge authority or cleanup path is introduced. + +## Decision needed + +No implementation decision required. +Merge approval remains with the captain; this validation run does not authorize merging. diff --git a/bin/fm-crew-state.sh b/bin/fm-crew-state.sh index 16037c29305..d30a1408d97 100755 --- a/bin/fm-crew-state.sh +++ b/bin/fm-crew-state.sh @@ -12,8 +12,9 @@ # identity, else the pane busy-signature) and reconciles the possibly-stale log # against it. # -# The determinism lives entirely here - only run-step / pane / log reads plus -# fixed mapping logic, no heuristics and no LLM. Output is one stable, parseable, +# The determinism lives entirely here - only run-step / pane / log reads and +# owned PR-poll merge evidence plus fixed mapping logic, no heuristics and no LLM. +# Output is one stable, parseable, # token-tight line firstmate can read every heartbeat: # # state: · source: · @@ -42,6 +43,14 @@ # checks" from "checks green, waiting on merge" (see nm_ci_checks_state) - # a ci-step log-tail check reports checks green without terminalizing a # live run, so a green PR is never silently read as still-validating. +# A terminal passed run is done, but does not itself prove a PR merged. +# Its detail claims PR merged/closed only when the run's canonical PR +# identity matches task metadata and the owned PR-poll merge-notification +# marker validated by fm-pr-lib.sh. With valid PR metadata but missing or +# mismatched evidence, detail is "run passed: PR merge unverified"; +# without valid PR metadata, it is "run passed". This read makes no live +# forge query, and done is not permission to tear down unlanded work: +# fm-teardown.sh independently owns the landed-work test. # 3. Reconcile the status log: if its last line says needs-decision/blocked but # the run-step shows the run moved on, the log is deterministically stale and # is flagged superseded. A genuinely parked run plus a needs-decision log @@ -73,6 +82,8 @@ STATE="${FM_STATE_OVERRIDE:-$FM_HOME/state}" . "$SCRIPT_DIR/fm-busy-lib.sh" # shellcheck source=bin/fm-nm-run-lib.sh . "$SCRIPT_DIR/fm-nm-run-lib.sh" +# shellcheck source=bin/fm-pr-lib.sh +. "$SCRIPT_DIR/fm-pr-lib.sh" ID=${1:-} [ -n "$ID" ] || { echo "usage: fm-crew-state.sh " >&2; exit 2; } @@ -323,21 +334,20 @@ nm_effective_ci_step_status() { fi } -# Root cause of the PR #252 incident (2026-07): for a repo where merge is left -# to the captain, no-mistakes' ci step (and therefore top-level status/outcome) -# stays "running" for the ENTIRE CI-monitor phase, including long after GitHub -# reports every check green - it only reaches outcome=passed once the PR is -# actually merged (or failed/cancelled if closed). `axi status`'s steps[] table -# never distinguishes "still waiting on checks" from "checks green, waiting on -# merge": both read as plain `ci,running,...`. The only place that transition is -# recorded is the ci step's own log text, e.g. "all CI checks passed - still -# monitoring until merged or closed" or "no CI checks reported - still -# monitoring until merged or closed" (verified against 360+ real run logs under -# ~/.no-mistakes/logs/*/ci.log on the installed v1.32.2 binary, including the -# actual PR #252 run). Reads the ci step's log tail via `axi logs` and scans it -# for the MOST RECENT recognized marker (the log is append-only/chronological, -# so the last match is current): green with nothing red after it means CI is -# green right now, still only waiting on merge/close. +# Corroborate merge detail under the header's evidence contract. +pr_merge_observed() { + fm_pr_metadata_identity_parse "$META" || return 1 + fm_pr_url_parse "$(strip_quotes "$(nm_field pr)")" || return 1 + [ "$FM_PR_URL" = "$FM_PR_META_URL" ] || return 1 + fm_pr_poll_merge_already_notified "$STATE" "$ID" \ + "$FM_PR_META_PROVIDER" "$FM_PR_META_HOST" "$FM_PR_META_PATH" \ + "$FM_PR_META_NUMBER" +} + +# During CI monitoring, `axi status`'s steps[] table reports both pending +# checks and green checks awaiting merge as `ci,running,...`. +# Read the ci step's append-only log tail via `axi logs`; the most recent +# recognized marker wins so a later failure or re-arm supersedes earlier green. nm_ci_checks_state() { local run_id log_tail marker run_id=$(strip_quotes "$(nm_field id)") @@ -502,7 +512,16 @@ if [ "$HAVE_RUN" = 1 ]; then case "$status" in running|fixing|ci) active_status=1 ;; esac if [ -n "$outcome" ] && [ "$active_status" -eq 0 ]; then case "$outcome" in - passed) RUN_STATE="done"; RUN_DETAIL="run passed: PR merged/closed" ;; + passed) + RUN_STATE="done" + if pr_merge_observed; then + RUN_DETAIL="run passed: PR merged/closed" + elif fm_pr_metadata_identity_parse "$META"; then + RUN_DETAIL="run passed: PR merge unverified" + else + RUN_DETAIL="run passed" + fi + ;; checks-passed) RUN_STATE="done"; RUN_DETAIL="checks green: PR ready for review" ;; failed) RUN_STATE=failed; RUN_DETAIL="run failed" ;; cancelled) RUN_STATE=failed; RUN_DETAIL="run cancelled" ;; diff --git a/tests/fm-crew-state.test.sh b/tests/fm-crew-state.test.sh index a713e539f12..522b8684f89 100755 --- a/tests/fm-crew-state.test.sh +++ b/tests/fm-crew-state.test.sh @@ -31,6 +31,8 @@ set -u . "$(dirname "${BASH_SOURCE[0]}")/lib.sh" # shellcheck source=/dev/null . "$ROOT/bin/fm-classify-lib.sh" +# shellcheck source=bin/fm-pr-lib.sh +. "$ROOT/bin/fm-pr-lib.sh" CREW_STATE="$ROOT/bin/fm-crew-state.sh" TMP_ROOT=$(fm_test_tmproot fm-crew-state) @@ -272,7 +274,7 @@ run: branch: $1 status: completed head: "${FM_FAKE_RUN_HEAD:-abc1234}" - pr: "https://github.com/o/r/pull/1" + pr: "${2-https://github.com/o/r/pull/1}" findings: none outcome: passed EOF @@ -677,6 +679,97 @@ test_top_level_fixing_done_log_stays_working() { } # (d) terminal run-step is authoritative +mark_merged_poll() { # + fm_pr_poll_merge_mark_notified "$1" "$2" github github.com o/r "$3" \ + || fail "could not create merged PR poll evidence fixture" +} + +# A stopped worker can leave no-mistakes outcome=passed after CI concluded while +# the recorded PR remains open. Without the owned PR-poll marker, the state +# reader must report the run as complete but must not claim that the PR merged. +test_passed_open_pr_does_not_claim_merge() { + reset_fakes + local d; d=$(new_case passed-open-pr) + make_repo_on_branch "$d/wt" fm/feat-open-pr + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-open-pr.meta" "window=fm:fm-feat-open-pr" \ + "worktree=$d/wt" "kind=ship" "pr=https://github.com/o/r/pull/1" + printf 'done: PR https://github.com/o/r/pull/1 checks green\n' > \ + "$d/state/feat-open-pr.status" + FM_FAKE_AXI_STATUS="$(run_passed fm/feat-open-pr)" + local out; out=$(run_crew_state "$d" feat-open-pr) + assert_contains "$out" "state: done" "stopped passed run remains complete" + assert_contains "$out" "source: run-step" "passed run remains run-step sourced" + assert_contains "$out" "PR merge unverified" \ + "an open or unknown PR is reported as merge-unverified" + assert_not_contains "$out" "PR merged/closed" \ + "a passed run without forge evidence must not claim a merge" + pass "passed open PR is not reported as merged" +} + +# A matching marker is the owned poll's durable proof that its forge read saw a +# merge. It restores the terminal merged detail without requiring a live network +# call or trusting a marker for another PR. +test_passed_pr_with_owned_merge_evidence_claims_merge() { + reset_fakes + local d; d=$(new_case passed-merged-pr) + make_repo_on_branch "$d/wt" fm/feat-merged-pr + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-merged-pr.meta" "window=fm:fm-feat-merged-pr" \ + "worktree=$d/wt" "kind=ship" "pr=https://github.com/o/r/pull/1" + mark_merged_poll "$d/state" feat-merged-pr 1 + FM_FAKE_AXI_STATUS="$(run_passed fm/feat-merged-pr)" + local out; out=$(run_crew_state "$d" feat-merged-pr) + assert_contains "$out" "state: done" "merged run remains complete" + assert_contains "$out" "run passed: PR merged/closed" \ + "matching owned merge evidence restores merged detail" + pass "passed PR with matching owned merge evidence is reported as merged" +} + +# A marker for a different PR is not evidence for the currently recorded PR. +test_passed_pr_with_mismatched_merge_evidence_stays_unverified() { + reset_fakes + local d; d=$(new_case passed-mismatched-pr) + make_repo_on_branch "$d/wt" fm/feat-mismatched-pr + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-mismatched-pr.meta" "window=fm:fm-feat-mismatched-pr" \ + "worktree=$d/wt" "kind=ship" "pr=https://github.com/o/r/pull/1" + mark_merged_poll "$d/state" feat-mismatched-pr 2 + FM_FAKE_AXI_STATUS="$(run_passed fm/feat-mismatched-pr)" + local out; out=$(run_crew_state "$d" feat-mismatched-pr) + assert_contains "$out" "PR merge unverified" \ + "mismatched merge evidence stays unverified" + assert_not_contains "$out" "PR merged/closed" \ + "mismatched merge evidence cannot claim a merge" + pass "mismatched owned merge evidence is rejected" +} + +test_passed_run_with_stale_pr_metadata_stays_unverified() { + reset_fakes + local d run_pr out + d=$(new_case passed-stale-pr-metadata) + make_repo_on_branch "$d/wt" fm/feat-stale-pr + make_fakebin "$d" >/dev/null + fm_write_meta "$d/state/feat-stale-pr.meta" "window=fm:fm-feat-stale-pr" \ + "worktree=$d/wt" "kind=ship" "pr=https://github.com/o/r/pull/1" + mark_merged_poll "$d/state" feat-stale-pr 1 + FM_FAKE_TMUX_MISSING=1 + for run_pr in \ + https://github.com/o/r/pull/2 \ + https://github.com/o/other/pull/1 \ + https://gitlab.com/o/r/-/merge_requests/1 \ + https://github.com/o/r/pull/1/invalid \ + ''; do + FM_FAKE_AXI_STATUS="$(run_passed fm/feat-stale-pr "$run_pr")" + out=$(run_crew_state "$d" feat-stale-pr) + assert_contains "$out" "state: done" "stopped run remains complete: $run_pr" + assert_contains "$out" "source: run-step" "current run remains attributed: $run_pr" + assert_contains "$out" "PR merge unverified" "run PR must match merge evidence: $run_pr" + assert_not_contains "$out" "PR merged/closed" "stale metadata cannot prove this run merged: $run_pr" + done + pass "retained metadata and merge evidence cannot certify another run PR" +} + test_terminal_passed() { reset_fakes local d; d=$(new_case passed) @@ -1488,6 +1581,10 @@ test_ci_ready_done_log_relapse_stays_working test_ci_fixing_after_green_stays_working test_top_level_fixing_ci_running_after_green_stays_working test_top_level_fixing_done_log_stays_working +test_passed_open_pr_does_not_claim_merge +test_passed_pr_with_owned_merge_evidence_claims_merge +test_passed_pr_with_mismatched_merge_evidence_stays_unverified +test_passed_run_with_stale_pr_metadata_stays_unverified test_terminal_passed test_terminal_failed test_live_ci_status_outranks_stale_failure diff --git a/tests/fm-inactive-reconcile.test.sh b/tests/fm-inactive-reconcile.test.sh index 5443f476a9a..e153ef1a38a 100755 --- a/tests/fm-inactive-reconcile.test.sh +++ b/tests/fm-inactive-reconcile.test.sh @@ -842,9 +842,21 @@ test_fresh_progress_is_not_aged_from_task_creation() { } test_decision_backstop_commits_the_watcher_generation() { - local actor out pid i + local actor out pid i captures for actor in main away; do make_world "decision-generation-$actor" + # Keep pane-staleness independent of decision-generation deduplication. + # A static idle pane legitimately wakes the away watcher after two polls. + cat > "$WORLD/fakebin/tmux" <<'SH' +#!/usr/bin/env bash +case "${1:-}" in + display-message) printf '%%1\n' ;; + capture-pane) + printf 'capture\n' >> "${FM_HOME:?}/captures" + printf 'independent output %s\n> \n' "$(wc -l < "$FM_HOME/captures")" + ;; +esac +SH write_child "$MAIN" child 'needs-decision [key=api-shape]: choose the API shape' # Count completed polling opportunities instead of depending on how many # watcher cycles this machine happens to squeeze into a three-second sleep. @@ -885,18 +897,17 @@ SH FM_WATCH_HANDLING_SUCCESSOR=1 "$WATCH" > "$out" 2>&1 & pid=$! i=0 - while [ "$i" -lt 200 ] && kill -0 "$pid" 2>/dev/null; do - [ "$(wc -l < "$MAIN/captures")" -ge 4 ] && break + captures=0 + while [ "$i" -lt 100 ] && kill -0 "$pid" 2>/dev/null; do + captures=$(wc -l < "$MAIN/captures") + [ "$captures" -ge 4 ] && break sleep 0.1 i=$((i + 1)) done - if ! kill -0 "$pid" 2>/dev/null; then - wait "$pid" || true - fail "$actor re-arm duplicated the handled decision: $(cat "$out")" - fi + kill -0 "$pid" 2>/dev/null \ + || fail "$actor re-arm duplicated the handled decision: $(cat "$out")" reap "$pid" - [ "$(wc -l < "$MAIN/captures")" -ge 4 ] \ - || fail "$actor re-arm did not complete repeated polling cycles: $(cat "$out")" + [ "$captures" -ge 4 ] || fail "$actor re-arm did not reach its fourth pane capture" [ "$(wake_count "$MAIN" 'child.status')" = 0 ] \ || fail "$actor re-arm queued a duplicate handled decision" done diff --git a/tests/fm-teardown.test.sh b/tests/fm-teardown.test.sh index e56a13bea45..3eb570154fc 100755 --- a/tests/fm-teardown.test.sh +++ b/tests/fm-teardown.test.sh @@ -316,6 +316,20 @@ land_equivalent_patch_on_origin_branch() { git -C "$case_dir/project" rev-parse "refs/remotes/origin/$branch" } +# Override GitHub lookups to report the recorded PR as OPEN and unmerged. +add_gh_pr_open_for_head() { + local case_dir=$1 head=$2 + cat > "$case_dir/fakebin/gh" <&2 +exit 1 +SH + chmod +x "$case_dir/fakebin/gh" +} + # Override gh-axi so every call fails, simulating an API/network error. add_gh_axi_error() { local case_dir=$1 @@ -695,6 +709,42 @@ test_no_mistakes_origin_remote_allows() { pass "no-mistakes worktree with HEAD on origin is torn down (no regression)" } +# This is the exact false-merge fixture: no-mistakes reports a passed run and +# metadata records a PR, but the forge still reports that PR as OPEN. Teardown +# must independently refuse the unlanded work even if crew state once rendered +# the old false `PR merged/closed` detail. +test_no_mistakes_passed_open_pr_still_refuses() { + local case_dir rc head + case_dir=$(make_case nm-passed-open-pr) + write_meta "$case_dir" no-mistakes ship + wt_commit_file "$case_dir" feature.txt hello "open PR work" + append_pr_meta_url "$case_dir" + head=$(git -C "$case_dir/wt" rev-parse HEAD) + add_gh_pr_open_for_head "$case_dir" "$head" + FM_FAKE_AXI_STATUS=$(cat < "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 1 "$rc" "nm-passed-open-pr: teardown should refuse" + grep -q REFUSED "$case_dir/stderr" \ + || fail "nm-passed-open-pr: no REFUSED line in stderr" + pass "passed run with open PR is refused as unlanded" +} + test_no_mistakes_truly_unpushed_refuses() { local case_dir rc case_dir=$(make_case nm-unpushed) @@ -2619,6 +2669,7 @@ test_teardown_manual_backend_prompts_hand_edit_even_when_tasks_axi_present test_local_only_truly_unpushed_refuses test_local_only_merged_to_local_main_allows test_no_mistakes_origin_remote_allows +test_no_mistakes_passed_open_pr_still_refuses test_no_mistakes_truly_unpushed_refuses test_local_only_force_overrides_unpushed test_teardown_missing_busy_sidecar_completes