fix: harden review resolution and merged poll retirement

[eyevanovich]

Intent

Port the two captain-approved upstream review-lifecycle correctness fixes onto the Firstmate fork at PR #12's landed head. Review diffs must prefer freshly fetched GitHub or GitLab review heads stored under private refs, use reachable recorded pr_head only as an offline fallback, refuse forge-origin identity mismatches, and warn before final local-branch fallback. Merged-review polling must retire exactly once only after its notification is durable, use provider-neutral identity-bound crash-recovery receipts, preserve GitLab's hash-registered custom-check route including trusted self-hosted forges, retain task and persistent-secondmate lifecycle state, preserve X-mode and non-executing migration security, and safely recover through watcher, migration, rearm, replacement, tamper, and teardown races. Preserve all fork-specific GitLab, direct signing, no-mistakes observer, local-only, secondmate, away/X-mode, and five-backend behavior; add comprehensive counterfactual tests; do not merge.

What Changed

• Prefer freshly fetched GitHub or GitLab review heads in private refs, with validated offline and warned local-branch fallbacks.
• Retire merged review polls through provider-neutral, identity-bound recovery receipts that preserve replacement polls and lifecycle state across races and restarts.
• Extend migration, teardown, security, and counterfactual coverage for GitHub and GitLab review lifecycles; the full test suite and targeted lifecycle tests pass.

Risk Assessment

🚨 High: Captain, the change should not merge until the crash window that can duplicate a merged-review notification is explicitly resolved or approved.

Testing

The full baseline was already green; focused end-to-end CLI tests then exercised review-head selection, identity and fallback handling, exactly-once durable retirement and recovery races, migration/X-mode security, and GitLab lifecycle behavior, all successfully. A reviewer-visible CLI transcript was captured; no screenshot was applicable because this change has no rendered UI surface.

Evidence: Review lifecycle end-to-end transcript

$ bash tests/fm-review-diff.test.sh
ok - fresh GitHub review heads defeat stale reachable recorded heads
ok - fresh GitLab review heads defeat stale reachable recorded heads
ok - trusted self-hosted GitLab identity can fetch the current review head
ok - remote unavailability falls back to a reachable recorded review head
ok - invalid and multiline recorded review heads are refused
ok - tasks without a recorded review retain the local branch diff
ok - the local branch is a warned final fallback when neither review head is usable
ok - remote review identity mismatches refuse rather than using recorded metadata
$ bash tests/fm-pr-check-security.test.sh
ok - raw-byte parser accepts canonical URLs and rejects the complete adversarial matrix
ok - merged GitHub polls notify once and retirement preserves lifecycle metadata
ok - nonterminal, failed, tampered, and replacement poll states retain the right authority
ok - retirement recovers across notification, migration, and teardown crash boundaries
ok - PR and teardown entrypoints reject invalid arguments before every side effect
ok - valid direct and merge flows record exact metadata and reject multiline head metadata
ok - rejected metacharacter bytes remain inert at generation and watcher time
ok - static poll is silent except for one merged line and remains watcher-bounded
ok - interrupted atomic preparation cleans private temporaries and publishes nothing
ok - concurrent watchers observe only complete private poll publications
ok - post-rename poll validation faults revoke both names and allow a clean retry
ok - migration creates and validates private state before watcher exclusion
ok - migration pauses older watchers and acquires exclusion before its first scan or marker
ok - poll, marker, diagnostic, and quarantine paths refuse symlinks and directories
ok - marker and diagnostic rename errors and signals fail closed and recover durably on retry
ok - post-rename marker, diagnostic, and obligation faults are revoked and reconstructed on retry
ok - quarantine type and mode faults fail closed and recover only when a retry can validate them
ok - canonical and ambiguous failure obligations block every retry until all task artifacts are repaired
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
●  WATCHER DOWN - SUPERVISION IS OFF
●  1 task(s) in flight, but no watcher has a fresh beacon (last beat: never, grace 300s).
●  Trust the emitted supervision protocol for this harness; do not use shell & for watcher repair.
●  This is a supervision warning only; the guarded operation WILL still run.
●  resume supervision according to the session-start block for this harness; do not use shell &.
●━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
ok - ambiguous migration recovery accepts an explicitly validated replacement poll
ok - ambiguous repair rejects copied, metadata- or task-mismatched, forged, and partial poll publications
ok - all live, marker, diagnostic, X, custom-check, obligation, and teardown boundaries require single-link files
ok - canonical publication failure remains incomplete until a later clean retry rebuilds the poll
ok - legacy reserved obligations and delimiter-bearing task IDs retry without ambiguity
ok - migration never executes legacy checks, preserves X mode, quarantines ambiguity, and is idempotent
ok - historical X shims migrate only from the exact single-link mode-0755 identity
ok - direct registration refreshes authenticated v1 X shims across marker states
ok - bootstrap runs the non-executing migration at the locked session boundary
ok - bootstrap isolates incomplete poll migration from unrelated recovery sweeps
ok - watcher signals promptly stop custom checks and clean private state
ok - returned custom check descendants are drained on installed and fallback timeout paths
ok - teardown removes safe poll artifacts and refuses quarantine-directory symlinks without traversal
$ bash tests/fm-gitlab-review-lifecycle.test.sh
ok - GitLab review metadata and authenticated merge poll are recorded
ok - GitLab merged polls retire once while trust tampering preserves evidence
ok - pushed GitLab tasks require canonical merge proof for cleanup
ok - GitLab cleanup proof covers task state and identity failures
ok - the guarded merge path delegates GitLab to the narrow adapter
ok - an explicit GitLab merge method is not overridden
ok - merged GitLab work is proven landed before cleanup

- Outcome: 🔧 1 issue found → auto-fixed (2) ✅ across 3 runs (1h12m32s)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 error

• 🚨 bin/fm-watch.sh:827 - The required criterion says, “Merged-review polling must retire exactly once only after its notification is durable” and must recover safely through watcher races. The wake is durably appended here before the retirement receipt is published. A crash between lines 827 and 829 leaves the poll armed with no recovery receipt, so the next watcher can append a duplicate merged notification. Couple the durable notification to an identity-bound retirement intent that recovery can correlate before permitting another poll execution.

🔧 **Test** - 1 issue found → auto-fixed (2) ✅

• 🚨 tests failed with exit code 1
• command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"

🔧 Fix: Supply teardown fixtures’ new check-library dependency
1 error still open:

• 🚨 tests failed with exit code 1
• command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"

🔧 Fix: Supply legacy teardown fixture’s check-library dependency
✅ Re-checked - no issues remain.

• command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"
• Baseline previously completed: command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"
• bash tests/fm-review-diff.test.sh
• bash tests/fm-pr-check-security.test.sh
• bash tests/fm-gitlab-review-lifecycle.test.sh
• Captured combined behavioral output with tee /var/folders/j_/g4b3__rs5j95fq6xf9fb17sc0000gn/T/no-mistakes-evidence/01KYMW7NF1APVGVDT8G2J1RRVJ/review-lifecycle-e2e-transcript.txt
• Verified git status --short remained empty after testing

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.