feat: hard-stop review mutations on already-landed PR heads (Closes #292) #368

2026-07-07T04:39:44-05:00

jcwalker3 commented

2026-07-07 04:39:44 -05:00

Summary

Closes #292.

A reviewer workflow approved PR #278 and called the merge API although the PR head was already an ancestor of master, ending in a Gitea HTTP 405 and manual reconciliation. The #327 validator (merged as PR #348) rejects bad wording in reports that admit the landed state; what was missing is the pre-mutation gate itself and a gate-aware report check. This PR adds both.

Changes

review_proofs.py
- assess_already_landed_review_gate(pr_number, candidate_head_sha, target_branch, target_branch_sha, head_is_ancestor_of_target, live_head_sha=None, real_blocker=False, mergeable=None) — runs after PR selection and head pinning, before any review mutation:
  - Ancestor of target → state ALREADY_LANDED_RECONCILE_REQUIRED: approval blocked, merge API blocked, request-changes allowed only with a real blocker, reconciliation handoff required with the full field list (PR number/title, candidate head SHA, target branch, target branch SHA, ancestor proof, linked issue status, recommended reconciliation action, capability proof for close/comment mutations).
  - Not an ancestor → NORMAL_REVIEW_CANDIDATE; mergeability/conflicts stay a separate gate.
  - Fail closed as GATE_NOT_PROVEN (all mutations blocked) when the ancestry check was not run, the candidate or target SHA is not a full 40-hex SHA, the target branch is missing, or the live head no longer matches the pinned candidate head.
- assess_already_landed_report_state(report_text, gate_fired) — when the session's own gate fired, rejects APPROVED/MERGED/READY_TO_MERGE wording and requires the ALREADY_LANDED_RECONCILE_REQUIRED state. Complements the #327 text rules, which only trigger when the report itself mentions the landed state.
- New exported constants ALREADY_LANDED_STATE and ALREADY_LANDED_HANDOFF_FIELDS.
tests/test_review_proofs.py — TestAlreadyLandedReviewGate (8 tests) and TestAlreadyLandedReportState (6 tests): already-landed, normal mergeable, non-mergeable normal, changed head since pin, unchecked ancestry, invalid SHAs, real-blocker request-changes, handoff field list, and all report-state verdicts including gate-not-fired passthrough.

Acceptance criteria mapping (#292)

A PR whose head SHA is already ancestor of the target branch cannot be approved.
A PR whose head SHA is already ancestor of the target branch cannot trigger the merge API.
The workflow stops before review mutation and emits a reconciliation handoff (required fields returned by the gate).
Tests cover already-landed PR, normal mergeable PR, changed-head PR, and non-mergeable PR.
Final-report validation rejects APPROVED when the already-landed gate fired.

Validation

python -m pytest tests/ in the issue worktree (base 634d8a1): 1213 passed, 1 failed, 6 skipped, 20 subtests passed.

The single failure (tests/test_agent_temp_artifacts.py::TestIssueLockArtifactWarning::test_lock_success_includes_artifact_warning) is pre-existing on the clean base with this change stashed and is tracked by issue #359 (in progress). One transient failure of tests/test_worktrees.py::TestWorktreeStart::test_accepts_issue_linked_impl_branches appeared in a single run while a concurrent session mutated live worktrees; it passes in isolation, in its module, and in the final full-suite rerun.

Overlap note: #294 asks for the identical gate plus target-SHA reporting (both included here); #295/#298 wording rules already landed via #327.

🤖 Generated with Claude Code

## Summary Closes #292. A reviewer workflow approved PR #278 and called the merge API although the PR head was already an ancestor of `master`, ending in a Gitea HTTP 405 and manual reconciliation. The #327 validator (merged as PR #348) rejects bad wording in reports that admit the landed state; what was missing is the pre-mutation gate itself and a gate-aware report check. This PR adds both. ## Changes - `review_proofs.py` - `assess_already_landed_review_gate(pr_number, candidate_head_sha, target_branch, target_branch_sha, head_is_ancestor_of_target, live_head_sha=None, real_blocker=False, mergeable=None)` — runs after PR selection and head pinning, before any review mutation: - Ancestor of target → state `ALREADY_LANDED_RECONCILE_REQUIRED`: approval blocked, merge API blocked, request-changes allowed only with a real blocker, reconciliation handoff required with the full field list (PR number/title, candidate head SHA, target branch, target branch SHA, ancestor proof, linked issue status, recommended reconciliation action, capability proof for close/comment mutations). - Not an ancestor → `NORMAL_REVIEW_CANDIDATE`; mergeability/conflicts stay a separate gate. - Fail closed as `GATE_NOT_PROVEN` (all mutations blocked) when the ancestry check was not run, the candidate or target SHA is not a full 40-hex SHA, the target branch is missing, or the live head no longer matches the pinned candidate head. - `assess_already_landed_report_state(report_text, gate_fired)` — when the session's own gate fired, rejects `APPROVED`/`MERGED`/`READY_TO_MERGE` wording and requires the `ALREADY_LANDED_RECONCILE_REQUIRED` state. Complements the #327 text rules, which only trigger when the report itself mentions the landed state. - New exported constants `ALREADY_LANDED_STATE` and `ALREADY_LANDED_HANDOFF_FIELDS`. - `tests/test_review_proofs.py` — `TestAlreadyLandedReviewGate` (8 tests) and `TestAlreadyLandedReportState` (6 tests): already-landed, normal mergeable, non-mergeable normal, changed head since pin, unchecked ancestry, invalid SHAs, real-blocker request-changes, handoff field list, and all report-state verdicts including gate-not-fired passthrough. ## Acceptance criteria mapping (#292) - [x] A PR whose head SHA is already ancestor of the target branch cannot be approved. - [x] A PR whose head SHA is already ancestor of the target branch cannot trigger the merge API. - [x] The workflow stops before review mutation and emits a reconciliation handoff (required fields returned by the gate). - [x] Tests cover already-landed PR, normal mergeable PR, changed-head PR, and non-mergeable PR. - [x] Final-report validation rejects `APPROVED` when the already-landed gate fired. ## Validation `python -m pytest tests/` in the issue worktree (base 634d8a1): **1213 passed, 1 failed, 6 skipped, 20 subtests passed**. The single failure (`tests/test_agent_temp_artifacts.py::TestIssueLockArtifactWarning::test_lock_success_includes_artifact_warning`) is pre-existing on the clean base with this change stashed and is tracked by issue #359 (in progress). One transient failure of `tests/test_worktrees.py::TestWorktreeStart::test_accepts_issue_linked_impl_branches` appeared in a single run while a concurrent session mutated live worktrees; it passes in isolation, in its module, and in the final full-suite rerun. Overlap note: #294 asks for the identical gate plus target-SHA reporting (both included here); #295/#298 wording rules already landed via #327. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jcwalker3 added 1 commit 2026-07-07 04:39:44 -05:00

feat: hard-stop review mutations on already-landed PR heads (Closes #292 ) 876f1ee3a3

A reviewer workflow approved and attempted to merge PR #278 although its
head commit was already an ancestor of master, ending in a Gitea 405 and
manual reconciliation. Add the missing pre-mutation gate:

- assess_already_landed_review_gate classifies the pinned candidate
  head against the fetched target branch: NORMAL_REVIEW_CANDIDATE,
  ALREADY_LANDED_RECONCILE_REQUIRED, or GATE_NOT_PROVEN (fail closed).
  Already-landed PRs block approval and the merge API, allow
  request-changes only for a real blocker, and require a reconciliation
  handoff (PR number/title, candidate head SHA, target branch + SHA,
  ancestor proof, linked issue status, recommended action, capability
  proof for close/comment mutations). Unchecked ancestry, invalid or
  missing SHAs, and a head changed since pinning all fail closed.
  Mergeability stays a separate gate.

- assess_already_landed_report_state rejects APPROVED / MERGED /
  READY_TO_MERGE wording and missing ALREADY_LANDED_RECONCILE_REQUIRED
  state when the session's gate fired, complementing the text-marker
  rules from #327 which need the report to admit the landed state.

Tests cover already-landed, normal, non-mergeable, changed-head,
unchecked-ancestry, and invalid-SHA gate paths plus all report-state
verdicts.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

jcwalker3 referenced this pull request

2026-07-07 04:40:00 -05:00

Reviewer workflow must stop before approval when PR head is already landed #292

jcwalker3 referenced this issue from a commit

2026-07-07 08:15:32 -05:00

fix: resolve conflicts for PR #368 against current master

jcwalker3 added 1 commit 2026-07-07 08:15:33 -05:00

fix: resolve conflicts for PR #368 against current master 5131fc595a

sysadmin approved these changes 2026-07-07 08:27:55 -05:00

sysadmin left a comment

All tests pass successfully after merging master (1314 passed, 1 failed [pre-existing]). Reviewed the implementation of assess_already_landed_review_gate and assess_already_landed_report_state in review_proofs.py and the test coverage in tests/test_review_proofs.py. It correctly blocks approvals and merges on already-landed PR heads, while requiring a proper reconciliation handoff. Verified that the remaining failure is pre-existing on master.

All tests pass successfully after merging master (1314 passed, 1 failed [pre-existing]). Reviewed the implementation of `assess_already_landed_review_gate` and `assess_already_landed_report_state` in `review_proofs.py` and the test coverage in `tests/test_review_proofs.py`. It correctly blocks approvals and merges on already-landed PR heads, while requiring a proper reconciliation handoff. Verified that the remaining failure is pre-existing on master.

sysadmin merged commit 7b71113b4d into master

2026-07-07 08:28:05 -05:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Scaled-Tech-Consulting/Gitea-Tools#368