pbwraith/gitea-saas-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

gitea-saas-core#29: transactional + retryable review outcome application #32

Merged
pbwraith merged 5 commits from issue-29 into main 2026-08-29 10:23:28 +02:00
Conversation 9 Commits 5 Files Changed 3 +1055 -36
pbwraith commented 2026-08-28 20:44:24 +02:00
Owner
Copy Link

Transactional and retryable review outcome application (#29)

ReviewPolicy.poll_and_review() previously recorded (repo, PR, head SHA) as
reviewed before the policy action succeeded, so a transient failure at any
boundary stranded the PR forever.

This PR makes outcome application transactional + retryable in
gitea_saas/review_policy.py:

  • A head SHA is recorded as completed (reviews table) only after its
    required policy action fully succeeded (criterion 1).
  • Failed/incomplete attempts are persisted in a new review_state table:
    attempt count, last error, and merged_step/closed_step idempotency
    checkpoints (criterion 2).
  • Verdict-generation or policy-action exceptions produce a structured
    outcome=failed result (error + attempts); the same unchanged head
    stays eligible for retry on the next poll (criterion 3).
  • Completed heads are skipped on later polls; new head SHAs are reviewed
    normally (criterion 4).
  • Retries resume from checkpoints — 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> (criterion 5).
  • Focused coverage in tests/test_review_policy.py injects one-shot
    failures after verdict creation and at label, comment, merge, close and
    debit boundaries, proving a later poll completes the same head exactly
    once (criterion 6). Legacy reviews rows are preserved untouched
    (criterion 7).
  • python -m pytest -q passes (169 passed, 1 skipped) and
    git diff --check is clean (criterion 8).

Out of scope as filed: gitea_client.py HTTP method implementation, live
Gitea mutation tests, auth/dashboard PRs.

Closes #29

Transactional and retryable review outcome application (#29) `ReviewPolicy.poll_and_review()` previously recorded `(repo, PR, head SHA)` as reviewed *before* the policy action succeeded, so a transient failure at any boundary stranded the PR forever. This PR makes outcome application transactional + retryable in `gitea_saas/review_policy.py`: - A head SHA is recorded as completed (`reviews` table) **only after** its required policy action fully succeeded (criterion 1). - Failed/incomplete attempts are persisted in a new `review_state` table: attempt count, last error, and `merged_step`/`closed_step` idempotency checkpoints (criterion 2). - Verdict-generation or policy-action exceptions produce a structured `outcome=failed` result (`error` + `attempts`); the same unchanged head stays eligible for retry on the next poll (criterion 3). - Completed heads are skipped on later polls; new head SHAs are reviewed normally (criterion 4). - Retries resume from checkpoints — 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>` (criterion 5). - Focused coverage in `tests/test_review_policy.py` injects one-shot failures after verdict creation and at label, comment, merge, close and debit boundaries, proving a later poll completes the same head exactly once (criterion 6). Legacy `reviews` rows are preserved untouched (criterion 7). - `python -m pytest -q` passes (169 passed, 1 skipped) and `git diff --check` is clean (criterion 8). Out of scope as filed: `gitea_client.py` HTTP method implementation, live Gitea mutation tests, auth/dashboard PRs. Closes #29
pbwraith added 1 commit 2026-08-28 20:44:26 +02:00 
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
pbwraith commented 2026-08-28 20:45:23 +02:00
Author
Owner
Copy Link

[lane2] gitea-saas-core#29 implemented — commit 6cff159 on branch issue-29, PR #32.

Evidence:

  • python3 -m pytest -q: full suite 169 passed, 1 skipped (11.1s); tests/test_review_policy.py: 26 passed including the new transactional/retry coverage.
  • git diff --check: clean.
  • Failure-boundary tests inject one-shot failures at verdict generation, label (add/remove), comment, merge, close, and debit boundaries; each proves a later poll completes the same unchanged head exactly once, with no double merge / double close / double debit (billing idempotency key <repo>#<pr>@<head_sha> preserved).
  • Legacy reviews rows are preserved untouched; retry metadata lives in the new review_state table (attempts, last_error, merged_step/closed_step checkpoints).

Ready for manual review.

[lane2] gitea-saas-core#29 implemented — commit `6cff159` on branch `issue-29`, PR #32. Evidence: - `python3 -m pytest -q`: full suite **169 passed, 1 skipped** (11.1s); `tests/test_review_policy.py`: **26 passed** including the new transactional/retry coverage. - `git diff --check`: clean. - Failure-boundary tests inject one-shot failures at verdict generation, label (add/remove), comment, merge, close, and debit boundaries; each proves a later poll completes the same unchanged head exactly once, with no double merge / double close / double debit (billing idempotency key `<repo>#<pr>@<head_sha>` preserved). - Legacy `reviews` rows are preserved untouched; retry metadata lives in the new `review_state` table (`attempts`, `last_error`, `merged_step`/`closed_step` checkpoints). Ready for manual review.
pbwraith commented 2026-08-28 21:02:43 +02:00
Author
Owner
Copy Link

Blocking: retry recovery begins by calling list_open_prs(). Once a real merge succeeds, that PR is no longer open. If debit, label removal, issue closure, or a comment fails after the merge, the persisted retry row can never be reached on a later poll. The tests miss this because the fake client continues returning the merged PR from list_open_prs().

Please persist the remaining post-merge work before merging and drive incomplete recovery independently of the open-PR scan (or use an equivalent durable delivery/retry queue). Add a test where the merged PR disappears from list_open_prs() before the next retry. Also cover a crash/failure after a comment or label side effect succeeds so replay does not duplicate externally visible actions. Re-run the full suite on the updated branch.

Blocking: retry recovery begins by calling `list_open_prs()`. Once a real merge succeeds, that PR is no longer open. If debit, label removal, issue closure, or a comment fails after the merge, the persisted retry row can never be reached on a later poll. The tests miss this because the fake client continues returning the merged PR from `list_open_prs()`. Please persist the remaining post-merge work before merging and drive incomplete recovery independently of the open-PR scan (or use an equivalent durable delivery/retry queue). Add a test where the merged PR disappears from `list_open_prs()` before the next retry. Also cover a crash/failure after a comment or label side effect succeeds so replay does not duplicate externally visible actions. Re-run the full suite on the updated branch.
pbwraith added the
bounced
 label 2026-08-28 21:02:44 +02:00
pbwraith added 1 commit 2026-08-28 21:28:43 +02:00
pbwraith removed the
bounced
 label 2026-08-28 21:28:57 +02:00
pbwraith commented 2026-08-28 21:29:21 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 1]

Review finding — fixed on the EXACT existing head branch issue-29 (no new branch, no duplicate PR). New commit c3915d2 (fast-forward 6cff159..c3915d2).

Finding: retry recovery began with list_open_prs(). Once a real merge succeeds the PR is no longer open, so the persisted review_state row could never be reached on a later poll — debit, label removal, issue close, or comment failures after the merge were stranded forever. The tests missed it because the fake kept returning the merged PR from list_open_prs(). Additionally, post-merge label removal and the success comment had no checkpoints, so replay after a crashed side effect could duplicate visible actions.

Fix (finding-by-finding):

  1. Persist remaining post-merge work before merging — _apply_policy now calls _ensure_plan() BEFORE _safe_merge(), writing verdict/tier/linked-issue into review_state so the row alone is sufficient to finish the job (gitea_saas/review_policy.py:594-600; method :233-257).
  2. Drive incomplete recovery independently of the open-PR scan — new recover_incomplete() walks review_state rows directly, re-fetches each PR via get_pr() (works for merged/closed PRs), and resumes the remaining checkpointed steps debit → label → close → comment, with crash-window adoption when the live PR already reports merged/closed (:385-470). poll_and_review() now runs recovery FIRST, before the open scan (:348-352).
  3. No duplication of externally visible actions — every post-merge step is checkpointed (debit_step, label_step, comment_step added alongside merged_step/closed_step; new columns ALTER-added idempotently for pre-existing DBs via _ensure_review_state_columns, :170-180). A replay skips already-applied steps: one merge, one debit, one label removal, one close, one success comment.
  4. Merged-PR-disappears test — test_merged_pr_gone_from_open_scan_recovered_after_post_merge_failure removes the merged PR from list_open_prs() exactly as real Gitea does; the next poll still completes the outcome exactly once via recovery.
  5. Crash-after-side-effect tests — test_crash_after_label_removed_no_duplicate_visible_actions (comment fails after label removal succeeded) and test_crash_after_success_comment_before_completion_record (process dies after the success comment, before the completion row) both prove replay performs no duplicate visible action.

Evidence:

  • python3 -m pytest -q: 172 passed, 1 skipped (was 169 passed, 1 skipped; +3 new recovery tests). tests/test_review_policy.py: 29 passed.
  • git diff --check: clean.
  • Only gitea_saas/review_policy.py + tests/test_review_policy.py touched (the issue's owned files).

PR left open for manual review; not merged by worker.

[worker-bounce-fix attempt 1] Review finding — fixed on the EXACT existing head branch `issue-29` (no new branch, no duplicate PR). New commit `c3915d2` (fast-forward `6cff159..c3915d2`). **Finding:** retry recovery began with `list_open_prs()`. Once a real merge succeeds the PR is no longer open, so the persisted `review_state` row could never be reached on a later poll — debit, label removal, issue close, or comment failures after the merge were stranded forever. The tests missed it because the fake kept returning the merged PR from `list_open_prs()`. Additionally, post-merge label removal and the success comment had no checkpoints, so replay after a crashed side effect could duplicate visible actions. **Fix (finding-by-finding):** 1. *Persist remaining post-merge work before merging* — `_apply_policy` now calls `_ensure_plan()` BEFORE `_safe_merge()`, writing verdict/tier/linked-issue into `review_state` so the row alone is sufficient to finish the job (`gitea_saas/review_policy.py:594-600`; method `:233-257`). 2. *Drive incomplete recovery independently of the open-PR scan* — new `recover_incomplete()` walks `review_state` rows directly, re-fetches each PR via `get_pr()` (works for merged/closed PRs), and resumes the remaining checkpointed steps debit → label → close → comment, with crash-window adoption when the live PR already reports merged/closed (`:385-470`). `poll_and_review()` now runs recovery FIRST, before the open scan (`:348-352`). 3. *No duplication of externally visible actions* — every post-merge step is checkpointed (`debit_step`, `label_step`, `comment_step` added alongside `merged_step`/`closed_step`; new columns ALTER-added idempotently for pre-existing DBs via `_ensure_review_state_columns`, `:170-180`). A replay skips already-applied steps: one merge, one debit, one label removal, one close, one success comment. 4. *Merged-PR-disappears test* — `test_merged_pr_gone_from_open_scan_recovered_after_post_merge_failure` removes the merged PR from `list_open_prs()` exactly as real Gitea does; the next poll still completes the outcome exactly once via recovery. 5. *Crash-after-side-effect tests* — `test_crash_after_label_removed_no_duplicate_visible_actions` (comment fails after label removal succeeded) and `test_crash_after_success_comment_before_completion_record` (process dies after the success comment, before the completion row) both prove replay performs no duplicate visible action. **Evidence:** - `python3 -m pytest -q`: **172 passed, 1 skipped** (was 169 passed, 1 skipped; +3 new recovery tests). `tests/test_review_policy.py`: **29 passed**. - `git diff --check`: clean. - Only `gitea_saas/review_policy.py` + `tests/test_review_policy.py` touched (the issue's owned files). PR left open for manual review; not merged by worker.
pbwraith commented 2026-08-29 09:50:30 +02:00
Author
Owner
Copy Link

[governor-review] BOUNCED — production recovery still cannot safely identify the reviewed merge

Reviewed head c3915d25331f223083048432fcb20b03065537b4 against base ccccfbf879bbd07512e86757abcf0c494f5b49f2 in a fresh synthetic merge. Full suite: 172 passed, 1 skipped; diff check clean. The new retry table and post-merge checkpoints address most of the prior failure path, but three production blockers remain:

  1. _safe_merge() still calls self.gitea.merge_pr(repo, pr_number) without the reviewed pr["head_sha"]. The repaired production client in PR #31 only closes the review-to-merge TOCTOU gate when expected_head_sha is supplied. Pass the exact reviewed head and add an integration regression proving SHA A reviewed/current SHA B causes no merge.
  2. recover_incomplete() relies on get_pr() returning merged or state, but the production GiteaClient.get_pr() shape from PR #31 returns neither. A real merge that succeeds just before the merged_step checkpoint will disappear from the open scan and recovery will continue to skip it forever. Coordinate the production client shape (include the raw merged boolean and state, or add an equivalent explicit operation) and test with the production-shaped fake, not a richer test-only PR dict.
  3. state == "closed" is not proof of a merge. A PR can be closed without merging; the current recovery path would adopt that as merged_step, debit the client, remove labels, and close the linked issue. Require positive merged is True. Add a closed-but-unmerged negative regression proving no billing or issue close occurs.

Keep the durable recovery independent of list_open_prs() and the current idempotency/checkpoint coverage. Repair this same branch and PR.

[governor-review] BOUNCED — production recovery still cannot safely identify the reviewed merge Reviewed head `c3915d25331f223083048432fcb20b03065537b4` against base `ccccfbf879bbd07512e86757abcf0c494f5b49f2` in a fresh synthetic merge. Full suite: 172 passed, 1 skipped; diff check clean. The new retry table and post-merge checkpoints address most of the prior failure path, but three production blockers remain: 1. `_safe_merge()` still calls `self.gitea.merge_pr(repo, pr_number)` without the reviewed `pr["head_sha"]`. The repaired production client in PR #31 only closes the review-to-merge TOCTOU gate when `expected_head_sha` is supplied. Pass the exact reviewed head and add an integration regression proving SHA A reviewed/current SHA B causes no merge. 2. `recover_incomplete()` relies on `get_pr()` returning `merged` or `state`, but the production `GiteaClient.get_pr()` shape from PR #31 returns neither. A real merge that succeeds just before the `merged_step` checkpoint will disappear from the open scan and recovery will continue to skip it forever. Coordinate the production client shape (include the raw `merged` boolean and `state`, or add an equivalent explicit operation) and test with the production-shaped fake, not a richer test-only PR dict. 3. `state == "closed"` is not proof of a merge. A PR can be closed without merging; the current recovery path would adopt that as `merged_step`, debit the client, remove labels, and close the linked issue. Require positive `merged is True`. Add a closed-but-unmerged negative regression proving no billing or issue close occurs. Keep the durable recovery independent of `list_open_prs()` and the current idempotency/checkpoint coverage. Repair this same branch and PR.
pbwraith added the
bounced
 label 2026-08-29 09:50:30 +02:00
pbwraith added 2 commits 2026-08-29 10:00:16 +02:00 
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.
pbwraith removed the
bounced
 label 2026-08-29 10:00:46 +02:00
pbwraith commented 2026-08-29 10:00:48 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 2]

Governor review (2026-08-29) findings — fixed on the EXACT existing head branch issue-29 (no new branch, no duplicate PR). New commits: d522ea2 (merge of current main, bringing in the merged PR #31 client contract) + 30e71d4 (bounce-fix). Head now 30e71d4; PR re-verified: state=open, merged=false, head issue-29, mergeable=true, 3 changed files.

Finding 1 — merge not pinned to the reviewed head. _safe_merge() called merge_pr(repo, n) with no expected head, so the production client's review-to-merge TOCTOU gate (PR #31) was never armed from the policy call site.
Fix: _safe_merge now passes expected_head_sha=pr["head_sha"] — the exact reviewed SHA — through to gitea.merge_pr(...) (review_policy.py:_safe_merge). The client (already on main via the merge) compares the pre-merge live head to it and fails closed BEFORE any POST; head_commit_id is the reviewed SHA.
Regression: test_reviewed_head_pinned_so_moved_branch_is_never_merged — SHA A reviewed, branch moves to SHA B before the merge attempt: no merge POST, no debit, no close, no success comment; next poll re-reviews SHA B and merges exactly once.

Finding 2 — recovery relied on merged/state keys the production client shape does not return. recover_incomplete() read live["merged"]/live["state"], but the normalized GiteaClient.get_pr() shape (PR #31) carried neither, so a real merge succeeding just before the merged_step checkpoint would vanish from the open scan and stay unrecovered forever.
Fix (coordinated client shape, as suggested): GiteaClient._normalize_pr now includes the raw merged boolean and state in every normalized PR (list_open_prs and get_pr), so recovery positively identifies merges from the production shape (gitea_client.py:_normalize_pr).
Tests use the production-shaped fake — merged/state fields on every PR, list_open_prs returns open-unmerged only, merge_pr mutates the live record to merged/closed exactly like the API would — not a richer test-only dict (tests/test_review_policy.py:FakeGitea).
Regression: test_crash_window_adopts_merged_pr_from_production_shape — plan persisted, checkpoint write missed, live record reports merged=True: recovery adopts it and finishes debit → label → close → comment without a second merge request. The existing test_merged_pr_gone_from_open_scan_recovered_after_post_merge_failure (PR disappears from the open scan) is retained and passes against the production-shaped fake.

Finding 3 — state == "closed" treated as proof of a merge. A closed-but-unmerged PR was adopted as merged, triggering a debit, label removal, issue close, and success comment on a merge that never happened.
Fix: both adoption sites (_safe_merge and recover_incomplete) now require positive merged is True; state == "closed" alone never counts.
Negative regression: test_closed_unmerged_pr_recovery_never_debits_or_closes — pending outcome plan + live PR closed/unmerged: zero side effects (no debit, no label removal, no issue close, no success comment), row left unrecovered.

Evidence:

  • python3 -m pytest -q: 201 passed, 1 skipped (was 172 passed, 1 skipped; +3 new recovery regressions, and the merged PR #31 client tests now run in-suite). tests/test_review_policy.py: 32 passed.
  • git diff --check: clean.
  • Files touched in 30e71d4 only: gitea_saas/review_policy.py, tests/test_review_policy.py, plus the sanctioned client-shape coordination in gitea_saas/gitea_client.py (2 lines + docstrings). No other files, no scope expansion.

PR left open for manual review; not merged by worker.

[worker-bounce-fix attempt 2] Governor review (2026-08-29) findings — fixed on the EXACT existing head branch `issue-29` (no new branch, no duplicate PR). New commits: `d522ea2` (merge of current `main`, bringing in the merged PR #31 client contract) + `30e71d4` (bounce-fix). Head now `30e71d4`; PR re-verified: state=open, merged=false, head `issue-29`, mergeable=true, 3 changed files. **Finding 1 — merge not pinned to the reviewed head.** `_safe_merge()` called `merge_pr(repo, n)` with no expected head, so the production client's review-to-merge TOCTOU gate (PR #31) was never armed from the policy call site. *Fix:* `_safe_merge` now passes `expected_head_sha=pr["head_sha"]` — the exact reviewed SHA — through to `gitea.merge_pr(...)` (`review_policy.py:_safe_merge`). The client (already on main via the merge) compares the pre-merge live head to it and fails closed BEFORE any POST; `head_commit_id` is the reviewed SHA. *Regression:* `test_reviewed_head_pinned_so_moved_branch_is_never_merged` — SHA A reviewed, branch moves to SHA B before the merge attempt: no merge POST, no debit, no close, no success comment; next poll re-reviews SHA B and merges exactly once. **Finding 2 — recovery relied on `merged`/`state` keys the production client shape does not return.** `recover_incomplete()` read `live["merged"]`/`live["state"]`, but the normalized `GiteaClient.get_pr()` shape (PR #31) carried neither, so a real merge succeeding just before the `merged_step` checkpoint would vanish from the open scan and stay unrecovered forever. *Fix (coordinated client shape, as suggested):* `GiteaClient._normalize_pr` now includes the raw `merged` boolean and `state` in every normalized PR (`list_open_prs` and `get_pr`), so recovery positively identifies merges from the production shape (`gitea_client.py:_normalize_pr`). *Tests use the production-shaped fake* — `merged`/`state` fields on every PR, `list_open_prs` returns open-unmerged only, `merge_pr` mutates the live record to merged/closed exactly like the API would — not a richer test-only dict (`tests/test_review_policy.py:FakeGitea`). *Regression:* `test_crash_window_adopts_merged_pr_from_production_shape` — plan persisted, checkpoint write missed, live record reports `merged=True`: recovery adopts it and finishes debit → label → close → comment without a second merge request. The existing `test_merged_pr_gone_from_open_scan_recovered_after_post_merge_failure` (PR disappears from the open scan) is retained and passes against the production-shaped fake. **Finding 3 — `state == "closed"` treated as proof of a merge.** A closed-but-unmerged PR was adopted as merged, triggering a debit, label removal, issue close, and success comment on a merge that never happened. *Fix:* both adoption sites (`_safe_merge` and `recover_incomplete`) now require positive `merged is True`; `state == "closed"` alone never counts. *Negative regression:* `test_closed_unmerged_pr_recovery_never_debits_or_closes` — pending outcome plan + live PR closed/unmerged: zero side effects (no debit, no label removal, no issue close, no success comment), row left unrecovered. **Evidence:** - `python3 -m pytest -q`: **201 passed, 1 skipped** (was 172 passed, 1 skipped; +3 new recovery regressions, and the merged PR #31 client tests now run in-suite). `tests/test_review_policy.py`: **32 passed**. - `git diff --check`: clean. - Files touched in `30e71d4` only: `gitea_saas/review_policy.py`, `tests/test_review_policy.py`, plus the sanctioned client-shape coordination in `gitea_saas/gitea_client.py` (2 lines + docstrings). No other files, no scope expansion. PR left open for manual review; not merged by worker.
pbwraith commented 2026-08-29 10:04:11 +02:00
Author
Owner
Copy Link

[governor-review] BOUNCED — recovery can still attribute the wrong merged head

Reviewed repaired head 30e71d44bafc0bccd35a36c920bd29165d47959e against current base fc55c41dd0d85100ce3b9896331bdb7ee9872ffc (including merged PR #31) in a fresh synthetic merge. Full suite: 201 passed, 1 skipped; diff check clean. The reviewed-SHA call site, production merged/state shape, and closed-unmerged guard are fixed.

Two head-integrity gaps remain:

  1. recover_incomplete() adopts any get_pr() record with merged is True for the same PR number, without verifying live["head_sha"] == review_state.head_sha. If plan A was persisted, then the branch moved and head B was merged externally, the retry row for A will debit/close/comment as though reviewed head A was the delivered merge. Require both positive merge proof and exact planned-head equality. A merged different head must fail closed and surface a structured mismatch; it must not debit, remove labels, close the linked issue, or post a success comment. Add that negative regression.
  2. _safe_merge() passes expected_head_sha=pr.get("head_sha"); when the candidate lacks a head SHA this is None, which invokes the production client's backward-compatible live-head fallback and can merge a head the policy never reviewed. Reject a missing/empty reviewed SHA before calling the client and prove no merge attempt or downstream side effect occurs.

Preserve all current durable recovery/checkpoint coverage and the same-head crash-window success case. Repair this same branch and PR.

[governor-review] BOUNCED — recovery can still attribute the wrong merged head Reviewed repaired head `30e71d44bafc0bccd35a36c920bd29165d47959e` against current base `fc55c41dd0d85100ce3b9896331bdb7ee9872ffc` (including merged PR #31) in a fresh synthetic merge. Full suite: 201 passed, 1 skipped; diff check clean. The reviewed-SHA call site, production `merged/state` shape, and closed-unmerged guard are fixed. Two head-integrity gaps remain: 1. `recover_incomplete()` adopts any `get_pr()` record with `merged is True` for the same PR number, without verifying `live["head_sha"] == review_state.head_sha`. If plan A was persisted, then the branch moved and head B was merged externally, the retry row for A will debit/close/comment as though reviewed head A was the delivered merge. Require both positive merge proof and exact planned-head equality. A merged different head must fail closed and surface a structured mismatch; it must not debit, remove labels, close the linked issue, or post a success comment. Add that negative regression. 2. `_safe_merge()` passes `expected_head_sha=pr.get("head_sha")`; when the candidate lacks a head SHA this is `None`, which invokes the production client's backward-compatible live-head fallback and can merge a head the policy never reviewed. Reject a missing/empty reviewed SHA before calling the client and prove no merge attempt or downstream side effect occurs. Preserve all current durable recovery/checkpoint coverage and the same-head crash-window success case. Repair this same branch and PR.
pbwraith added the
bounced
 label 2026-08-29 10:04:12 +02:00
pbwraith commented 2026-08-29 10:04:28 +02:00
Author
Owner
Copy Link

[owner-guidance]

Authorize another repair iteration on this same issue-29 branch and PR #32. The latest instruction to work through the PR queue authorizes closing these newly observed head-integrity gaps; do not accept them as follow-up work and do not relax the review bar.

Implement governor comment 10539 exactly: recovery may adopt a merged PR only when merged is True and the live merged head equals the persisted reviewed head; a different merged head must produce a visible fail-closed mismatch with no billing/label/issue/comment side effects. Also reject a missing reviewed head before calling the merge client, proving no live-head fallback or downstream action occurs.

Preserve the reviewed-SHA merge pin, production normalized merge fields, closed-unmerged negative gate, durable recovery independent of open-PR scans, and current checkpoint/idempotency coverage. Repair this PR; do not open a replacement.

This authorization supersedes the retry-budget gate for this finding.

[owner-guidance] Authorize another repair iteration on this same `issue-29` branch and PR #32. The latest instruction to work through the PR queue authorizes closing these newly observed head-integrity gaps; do not accept them as follow-up work and do not relax the review bar. Implement governor comment `10539` exactly: recovery may adopt a merged PR only when `merged is True` and the live merged head equals the persisted reviewed head; a different merged head must produce a visible fail-closed mismatch with no billing/label/issue/comment side effects. Also reject a missing reviewed head before calling the merge client, proving no live-head fallback or downstream action occurs. Preserve the reviewed-SHA merge pin, production normalized merge fields, closed-unmerged negative gate, durable recovery independent of open-PR scans, and current checkpoint/idempotency coverage. Repair this PR; do not open a replacement. This authorization supersedes the retry-budget gate for this finding.
pbwraith added 1 commit 2026-08-29 10:21:05 +02:00 
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.
pbwraith removed the
bounced
 label 2026-08-29 10:21:21 +02:00
pbwraith commented 2026-08-29 10:21:42 +02:00
Author
Owner
Copy Link

[worker-bounce-fix attempt 3]

Commit: b46c356 on branch issue-29 (PR #32 retained, head 30e71d4 -> b46c356). Files: gitea_saas/review_policy.py, tests/test_review_policy.py. Full suite: python3 -m pytest -q -> 203 passed, 1 skipped (was 201 passed, 1 skipped; +2 new regressions). git diff --check clean.

Implements the owner decision (2026-08-29T10:04, for governor review 10539) — the two head-integrity gaps, finding by finding:

  1. Recovery must verify BOTH positive merge proof AND exact planned-head equality. recover_incomplete() crash-window adoption previously accepted any live record with merged is True for the PR number without comparing heads — if plan A was persisted and head B was merged externally, the retry row for A would debit/close/comment as though reviewed head A had been delivered. Adoption now requires live["head_sha"] == review_state.head_sha. A merged DIFFERENT head fails closed with a structured HEAD_MISMATCH result (persisted attempt with the mismatch message + result entry), performs NO debit, NO label removal, NO issue close, NO success comment, leaves merged_step unmarked, and leaves the row for the operator. The same-head crash-window success case is preserved.
    Negative regression: test_recovery_merged_different_head_fails_closed — plan at sha-A, live merged at sha-B: outcome failed, code HEAD_MISMATCH, message names both heads, all side-effect lists empty, merged_step unmarked, head not recorded as reviewed.

  2. Missing/empty reviewed head rejected before the merge client. expected_head_sha=None would invoke the production client's backward-compatible live-head fallback and could merge a head the policy never reviewed. Two-layered rejection: _apply_policy() now refuses a missing/empty head_sha BEFORE the plan write (with the semantic merge refused: candidate PR carries no reviewed head_sha — ... refusing to fall back to the live head failure; note review_state.head_sha is NOT NULL, so the DB would have rejected the plan anyway — now with the correct meaning), and _safe_merge() independently refuses to call merge_pr without a pinned reviewed head (expected_head_sha is now the guard-verified reviewed_head, never a bare .get()). Zero merge attempts, zero downstream side effects.
    Negative regression: test_missing_reviewed_head_never_reaches_merge_client — candidate with head_sha=None under an approve+auto-merge gold tier: structured failure naming the missing reviewed head, gitea.merged == [], no debit/label/close/comment, head not recorded as reviewed.

Preserved per the owner directive: the 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, and the full checkpoint/idempotency coverage (one merge, one debit, one label removal, one close, one success comment).

Evidence: python3 -m pytest -q -> 203 passed, 1 skipped; git diff --check clean; PR #32 verified after push: state=open, merged=false, head issue-29 @ b46c356, mergeable=true, changed files = 3 with non-empty diff. bounced removed; issue #29 stays in-review. Ready for manual re-review.

[worker-bounce-fix attempt 3] Commit: `b46c356` on branch `issue-29` (PR #32 retained, head 30e71d4 -> b46c356). Files: `gitea_saas/review_policy.py`, `tests/test_review_policy.py`. Full suite: `python3 -m pytest -q` -> 203 passed, 1 skipped (was 201 passed, 1 skipped; +2 new regressions). `git diff --check` clean. Implements the owner decision (2026-08-29T10:04, for governor review 10539) — the two head-integrity gaps, finding by finding: 1. Recovery must verify BOTH positive merge proof AND exact planned-head equality. `recover_incomplete()` crash-window adoption previously accepted any live record with `merged is True` for the PR number without comparing heads — if plan A was persisted and head B was merged externally, the retry row for A would debit/close/comment as though reviewed head A had been delivered. Adoption now requires `live["head_sha"] == review_state.head_sha`. A merged DIFFERENT head fails closed with a structured `HEAD_MISMATCH` result (persisted attempt with the mismatch message + result entry), performs NO debit, NO label removal, NO issue close, NO success comment, leaves `merged_step` unmarked, and leaves the row for the operator. The same-head crash-window success case is preserved. Negative regression: `test_recovery_merged_different_head_fails_closed` — plan at sha-A, live merged at sha-B: outcome failed, code HEAD_MISMATCH, message names both heads, all side-effect lists empty, merged_step unmarked, head not recorded as reviewed. 2. Missing/empty reviewed head rejected before the merge client. `expected_head_sha=None` would invoke the production client's backward-compatible live-head fallback and could merge a head the policy never reviewed. Two-layered rejection: `_apply_policy()` now refuses a missing/empty `head_sha` BEFORE the plan write (with the semantic `merge refused: candidate PR carries no reviewed head_sha — ... refusing to fall back to the live head` failure; note `review_state.head_sha` is NOT NULL, so the DB would have rejected the plan anyway — now with the correct meaning), and `_safe_merge()` independently refuses to call `merge_pr` without a pinned reviewed head (expected_head_sha is now the guard-verified `reviewed_head`, never a bare `.get()`). Zero merge attempts, zero downstream side effects. Negative regression: `test_missing_reviewed_head_never_reaches_merge_client` — candidate with `head_sha=None` under an approve+auto-merge gold tier: structured failure naming the missing reviewed head, `gitea.merged == []`, no debit/label/close/comment, head not recorded as reviewed. Preserved per the owner directive: the 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, and the full checkpoint/idempotency coverage (one merge, one debit, one label removal, one close, one success comment). Evidence: `python3 -m pytest -q` -> 203 passed, 1 skipped; `git diff --check` clean; PR #32 verified after push: state=open, merged=false, head `issue-29` @ b46c356, mergeable=true, changed files = 3 with non-empty diff. `bounced` removed; issue #29 stays `in-review`. Ready for manual re-review.
pbwraith merged commit 8588033d73 into main 2026-08-29 10:23:28 +02:00
pbwraith commented 2026-08-29 10:23:32 +02:00
Author
Owner
Copy Link

[governor-review] MERGED — exact head b46c356d64f1891e32024f3107937d35c7cc3e9a reviewed against base fc55c41dd0d85100ce3b9896331bdb7ee9872ffc.

Evidence:

  • The synthetic merge was clean and git diff --check passed.
  • Full suite: 203 passed, 1 skipped.
  • Recovery adopts an externally completed merge only when the live merged SHA exactly equals the persisted reviewed SHA; a different SHA returns structured HEAD_MISMATCH with no debit, label removal, issue close, or success comment.
  • A candidate with no reviewed head_sha now fails before any plan write or merge-client call, so the production client's live-head compatibility fallback cannot merge an unreviewed commit.
  • Closed-but-unmerged PRs remain ineligible for recovery.

Linked issue #29 is closed.

[governor-review] MERGED — exact head `b46c356d64f1891e32024f3107937d35c7cc3e9a` reviewed against base `fc55c41dd0d85100ce3b9896331bdb7ee9872ffc`. Evidence: - The synthetic merge was clean and `git diff --check` passed. - Full suite: 203 passed, 1 skipped. - Recovery adopts an externally completed merge only when the live merged SHA exactly equals the persisted reviewed SHA; a different SHA returns structured `HEAD_MISMATCH` with no debit, label removal, issue close, or success comment. - A candidate with no reviewed `head_sha` now fails before any plan write or merge-client call, so the production client's live-head compatibility fallback cannot merge an unreviewed commit. - Closed-but-unmerged PRs remain ineligible for recovery. Linked issue #29 is closed.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked

   
bounced

   
eval-task

   
in-progress

   
in-review

   
needs-guidance

   
p1

   
p2

   
p3

   
tracking

   
unfunded
No Label
blocked
bounced
eval-task
in-progress
in-review
needs-guidance
p1
p2
p3
tracking
unfunded
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: pbwraith/gitea-saas-core#32
No description provided.
Delete Branch "issue-29"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?