Governor review 10539 findings, finding by finding (issue-29 branch, PR #32 retained):
1. Recovery must never attribute a DIFFERENT head's merge to a reviewed-head plan.
recover_incomplete() adopted any live record with merged is True for the PR
number without comparing heads: if plan A was persisted and then head B was
merged externally, the retry row for A would debit/label/close/comment as
though reviewed head A had been delivered. Crash-window adoption now
requires BOTH positive merged proof AND live head_sha == the persisted
planned/reviewed head_sha. A merged DIFFERENT head fails closed with a
structured HEAD_MISMATCH result (persisted attempt + result entry), and
performs NO debit, NO label removal, NO issue close, NO success comment;
the row is left unadopted. The same-head crash-window success case is
preserved.
2. A candidate with no reviewed head_sha must never reach the merge client.
Passing expected_head_sha=None to merge_pr would arm the production
client's backward-compatible live-head fallback and could merge a head the
policy never reviewed. _apply_policy() now rejects a missing/empty
head_sha BEFORE the plan write (review_state.head_sha is NOT NULL, so the
DB would reject it anyway — now with the correct semantic failure) and
_safe_merge() independently refuses to call the client without a pinned
reviewed head. Zero merge attempts and zero downstream side effects.
New regressions (tests/test_review_policy.py):
- test_recovery_merged_different_head_fails_closed — plan at sha-A, live
merged at sha-B: HEAD_MISMATCH, zero side effects, merged_step unmarked.
- test_missing_reviewed_head_never_reaches_merge_client — candidate with no
head_sha: structured failure, no merge_pr call, no debit/label/close/comment.
Preserved: reviewed-SHA merge pin, production normalized merged/state shape,
closed-unmerged negative gate (state==closed never counts), durable recovery
independent of the open-PR scan, checkpoint/idempotency coverage, and the
same-head crash-window adoption.
Evidence: python3 -m pytest -q -> 203 passed, 1 skipped (was 201 passed, 1
skipped; +2 new tests). git diff --check clean. Files: gitea_saas/review_policy.py,
tests/test_review_policy.py.
Governor review #2 findings on PR #32:
1. _safe_merge now passes the REVIEWED head sha to
gitea.merge_pr(..., expected_head_sha=...) — the production client
(issue #28) fails closed before any POST when the branch moved after
review, so an unreviewed SHA can never be merged. Integration
regression: SHA A reviewed / live SHA B -> no merge POST, no debit,
no close; the moved head is re-reviewed and merged next poll.
2. Production client shape coordinated: GiteaClient._normalize_pr now
carries the raw 'merged' boolean and 'state' in every normalized PR
(list_open_prs and get_pr), so recover_incomplete can positively
identify a merged PR. Tests exercise the production-shaped fake
(merged/state fields, open-only list_open_prs, live-record mutation
on merge) instead of a richer test-only dict. Crash-window adoption
regression: merged_pr checkpoint missed, live record reports
merged=True -> recovery finishes debit/label/close/comment without a
second merge request.
3. state == 'closed' is no longer treated as merged anywhere: _safe_merge
and recover_incomplete adopt a merge only on 'merged is True'.
Negative regression: a closed-but-unmerged PR with a pending outcome
plan produces no debit, no label removal, no issue close, no success
comment.
Evidence: 201 passed, 1 skipped (full suite); git diff --check clean.
A head SHA is now recorded as completed (reviews table) only after its
required policy action fully succeeded; any failure — verdict generation
or a label/comment/merge/close/debit boundary — is persisted in the new
review_state table (attempts, last_error, merged_step/closed_step
checkpoints) and reported as a structured outcome=failed result, leaving
the same unchanged head eligible for retry on the next poll.
Retries resume from checkpoints so a partial side effect can never
double-merge a PR or double-close an issue; the delivery debit keeps its
billing-core idempotency key (<repo>#<pr>@<head_sha>). Legacy reviews
rows are preserved untouched (completed history); no migration needed.
Closes#29
