GH-5266: fix(autopilot): review-merge hold is blind after re-adoption and cleared by COMMENTED reviews — harden the CHANGES_REQUESTED backstop

[alekspetrov]

Summary

Automated PR created by Pilot for task GH-5266.

Closes #5266

Changes

GitHub Issue GH-5266: fix(autopilot): review-merge hold is blind after re-adoption and cleared by COMMENTED reviews — harden the CHANGES_REQUESTED backstop

Problem

Follow-up from the PR#5264 post-merge review (notes N1/N2 — both pre-existing in the reused hasChangesRequested logic in internal/autopilot/controller.go, now load-bearing for the merge hold):

1. Re-adoption blinds the review hold. hasChangesRequested ignores reviews submitted before prState.CreatedAt, and both PR registration paths (fresh registration and reconciler re-adoption) set CreatedAt to now. After a daemon restart or orphan re-adoption, a standing CHANGES_REQUESTED review older than the adoption timestamp is invisible — the PR auto-merges over the reviewer's objection. This is exactly the race/revision-spawn-failure scenario fix(autopilot): merging stage attempts to merge draft PRs and ignores CHANGES_REQUESTED reviews — burns attempts, trips breaker, and only the draft flag prevents merging over a reviewer #5263 was filed to close; the protection currently holds only within a single tracked lifetime.
2. COMMENTED supersedes CHANGES_REQUESTED. The latest-review-per-user map overwrites with whatever review is chronologically last, so a later COMMENTED review from the same reviewer clears the hold. GitHub's own model does not treat COMMENTED as superseding a change request. Fail-open.

Fix shape

• For (1): on re-adoption, derive the review-hold cutoff from something durable (e.g. the PR's own creation time from the GitHub API, or the head commit's timestamp) instead of prState.CreatedAt=now — the CreatedAt filter's original purpose (avoid permanently parking on ancient stale reviews) is preserved by cutting off at PR creation, not at adoption.
• For (2): in the per-user latest map, let only APPROVE or an explicit dismissal (state DISMISSED) supersede a CHANGES_REQUESTED; COMMENTED keeps the standing request.

Acceptance

• Table-driven tests: (a) PR with CHANGES_REQUESTED → daemon restart/re-adoption simulated (fresh prState) → merge still held; (b) CHANGES_REQUESTED then COMMENTED (same user) → held; then APPROVE → unparked; then a DISMISSED case → unparked.
• The no-permanent-park property is preserved: a PR whose only change-request predates the PR's own creation (cross-linked/moved review edge) or is dismissed does not park.
• Genuine-failure accounting untouched.

Refs

• PR#5264 review notes N1/N2 · fix(autopilot): merging stage attempts to merge draft PRs and ignores CHANGES_REQUESTED reviews — burns attempts, trips breaker, and only the draft flag prevents merging over a reviewer #5263 (parent) · the revision-flow interception (ReviewFeedback poller) remains the primary path; this hardens the backstop.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/autopilot/controller.go	93.33%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[alekspetrov]

Post-merge review — APPROVE (w-notes)

Verified against merged main (547dd83d):

1. N1 fix confirmed. Review-hold cutoff now anchors on ghPR.CreatedAt (durable GitHub creation time) instead of prState.CreatedAt (reset to now by both registration paths). Both call sites checked: handleMerging passes the fresh ghPRForHold (nil on fail-open fetch error → documented fallback); processAllPRs reuses its loop-fetched ghPR, guaranteed non-nil (loop continues on fetch error). Fallback to prState.CreatedAt only when ghPR is nil or CreatedAt unparseable — no-permanent-park intent preserved.
2. N2 fix confirmed. Once a user's latest state is CHANGES_REQUESTED, only APPROVED/DISMISSED overwrite; COMMENTED ignored. SDK constants verified to match GitHub API strings exactly.
3. Tests are real killers. All 3 new tests + 2 updated ones re-run locally against merged main in a detached worktree — pass. The e2e test additionally pins the hold semantics: StageMerging retained, MergeAttempts=0, per-PR failure count 0 (a hold must not feed the breaker) — exactly the fix(autopilot): merging stage attempts to merge draft PRs and ignores CHANGES_REQUESTED reviews — burns attempts, trips breaker, and only the draft flag prevents merging over a reviewer #5263 accounting requirement.
4. Acceptance met: re-adoption hold (a), COMMENTED→held / APPROVE→unparked / DISMISSED→unparked (b), cross-linked-review edge not parked, genuine-failure accounting untouched.

Notes (no action required):

• N1 The supersede map now load-bearingly assumes ListPullRequestReviews returns ascending chronological order (GitHub's documented behavior; the pre-existing latest-wins map assumed it too). A descending order would fail closed (spurious hold), not open.
• N2 Degraded case remains by design: re-adoption + GetPullRequest failure in handleMerging → fallback cutoff = adoption time → blind again for that tick. Documented in the code comment; acceptable fail-open, self-heals next tick.