fix(reviewer): isolate final-decision locks by PR and exact head #896

New Issue

2026-07-24T21:39:26-05:00

jcwalker3 commented

2026-07-24 21:39:26 -05:00

Summary

gitea_mark_final_review_decision fail-closes on a target PR because of a terminal review decision recorded on a different PR, while gitea_diagnose_review_decision_lock simultaneously reports that cross-PR isolation permits the very same operation. The two tools contradict each other, and both prescribe retrying the identical failing call, so the prescribed recovery is a loop.

The consequence is not merely a stuck workflow — it is a fail-open. A reviewer's blocking verdict could not be recorded at all, so the PR kept a stale approval and stayed merge-eligible.

Proven behavior

Observed live on 2026-07-25. Profile prgs-reviewer, identity sysadmin, remote prgs, repo Scaled-Tech-Consulting/Gitea-Tools. Runtime parity was green throughout (mutation_safe: true, restart_required: false), so this is not a stale-runtime artifact.

Target: PR #882 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e.
A terminal approval on foreign PR #885 (review 580, head dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885) prevented mark_final_review_decision for #882.

`gitea_mark_final_review_decision(pr_number=882, action=request_changes, expected_head_sha=1cbbde00...)`

marked_ready: false
classification: "foreign_pr_terminal"
reasons:
  - "cannot mark final decision after a live review mutation was already
     recorded for this PR head unless an operator-approved correction was
     authorized for this same PR/head (fail closed, #620/#693)"
  - "last terminal is approve on foreign open/closed PR #885; target PR #882
     is isolated (#693)"
exact_next_action: "gitea_mark_final_review_decision(pr_number=882, ...) —
                    foreign terminal on PR #885 does not block (#693); do not
                    call gitea_authorize_review_correction for cross-PR unlock"
evidence: { last_review_id: 580, live_mutations_count: 3 }
locked_head_sha: "dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885"
target_head_sha: "1cbbde00895dce1545290a660bc31c4ac9210b3e"

Identical output on two attempts.

`gitea_diagnose_review_decision_lock(target_pr_number=882, expected_head_sha=1cbbde00...)`

classification: "foreign_pr_terminal"
mark_final_allowed: true
recovery_allowed: true
recovery_mode: "cross_pr_isolation"
correction_eligible: false
cleanup_allowed: false
last_terminal_pr: 885
last_terminal_action: "approve"
lock_summary.ready_pr_number: 885
lock_summary.ready_expected_head_sha: "dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885"
reasons: ["last terminal is approve on foreign open/closed PR #885; target
          PR #882 is isolated (#693)"]

mark_final_allowed: true and marked_ready: false were true at the same moment, for the same PR and the same head.

The prescribed retry loop

Both tools return the same exact_next_action: call gitea_mark_final_review_decision for PR #882 again — exactly the call that just failed. Following the prescribed recovery loops indefinitely. It was attempted twice, produced byte-identical output, and was then stopped deliberately.

Even a non-terminal review comment was blocked

The comment path is not an escape hatch. gitea_submit_pr_review(pr_number=882, action=comment) returned:

"final_review_decision_ready must be true; validation-phase live review
 mutations are forbidden — use gitea_dry_run_pr_review instead (fail closed)"

while gitea_dry_run_pr_review reported would_perform: true for both request_changes and comment. The only thing between a validated reviewer and a recorded verdict is a flag that mark_final refuses to set for cross-PR reasons. The reviewer had to fall back to an issue-thread comment (issue #661 comment 16232) to record the blocking finding at all.

The state is head-scoped but leaks across PRs

In the same session, after PR #885's head moved off dac40ab9...:

gitea_mark_final_review_decision(pr_number=885, action=approve,
    expected_head_sha=95e4aae287902e629b879a39887042a56f2f0757)
-> marked_ready: true, head_scoped: true

Moving #885 to a new head cleared the lock for #885. Terminal state is therefore already tracked per head — it is simply not isolated per PR. A single global "last terminal" slot (lock_summary.ready_pr_number) is consulted for every target PR.

Safety impact

PR #882 carried a reviewer-reproduced fail-open in drain_proof.py: with counts.sessions_live_other = 3 and the acknowledgement key absent, acks_or_timeout passed with detail "no other live sessions required to acknowledge", the proof minted clean: True, and gate_apply_restart returned verdict: allow / allow: True — authorizing a restart against three live sessions with zero acknowledgement evidence.

Because the blocking verdict could not be recorded, the undismissed approval 576 at that exact head remained in force and PR #882 stayed merge-eligible despite the reproduced fail-open. A merger running normal preflight would have seen approval_at_current_head: true and no blocking change request.

This guard defect converts a reviewer's blocking finding into no server-side state change at all: a fail-open in the control plane guarding a fail-open in the product.

Required behavior

Key terminal review state by both PR number and exact head SHA. A terminal decision on PR #885 must have no effect on a mark_final for PR #882 at any head.
Make the mark-final and diagnostic paths use the same authorization decision. gitea_mark_final_review_decision and gitea_diagnose_review_decision_lock must never disagree for the same (pr_number, head_sha). One must be the single source of truth and the other must derive from it.
Never prescribe the identical call that just failed as recovery. When a refusal is not self-recoverable, exact_next_action must name a different, actually-available action, or declare the state terminal and require operator involvement.
An unrecordable blocking decision must not silently leave a PR merge-eligible. Either the prior approval is invalidated, or the merge path is held, so a blocked REQUEST_CHANGES cannot degrade to "approved" by default.
Add regression coverage for a terminal decision on PR A followed by a decision on unrelated PR B: the PR B decision must succeed, and diagnose must agree with mark_final in both directions.

References

#693 — cross-PR isolation, cited by both tools in their own reasons
#620 — post-mutation correction gate, cited in the refusal
Issue #661 comment 16232 — the reviewer's blocking finding for PR #882, recorded on the issue thread because every PR review path was blocked
PR #882 (feat/issue-661-drain-proof-hard-gate), PR #885 (issue-640)

Scope note

This issue tracks the reviewer decision-lock guard only. The drain_proof.py acknowledgement fail-open that exposed it is remediated separately on PR #882 / issue #661 and is not in scope here.

## Summary `gitea_mark_final_review_decision` fail-closes on a target PR because of a terminal review decision recorded on a **different** PR, while `gitea_diagnose_review_decision_lock` simultaneously reports that cross-PR isolation permits the very same operation. The two tools contradict each other, and both prescribe retrying the identical failing call, so the prescribed recovery is a loop. The consequence is not merely a stuck workflow — it is a fail-open. A reviewer's blocking verdict could not be recorded at all, so the PR kept a stale approval and stayed merge-eligible. ## Proven behavior Observed live on 2026-07-25. Profile `prgs-reviewer`, identity `sysadmin`, remote `prgs`, repo `Scaled-Tech-Consulting/Gitea-Tools`. Runtime parity was green throughout (`mutation_safe: true`, `restart_required: false`), so this is not a stale-runtime artifact. - **Target:** PR #882 at head `1cbbde00895dce1545290a660bc31c4ac9210b3e`. - A terminal approval on **foreign PR #885** (review `580`, head `dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885`) prevented `mark_final_review_decision` for #882. ### `gitea_mark_final_review_decision(pr_number=882, action=request_changes, expected_head_sha=1cbbde00...)` ``` marked_ready: false classification: "foreign_pr_terminal" reasons: - "cannot mark final decision after a live review mutation was already recorded for this PR head unless an operator-approved correction was authorized for this same PR/head (fail closed, #620/#693)" - "last terminal is approve on foreign open/closed PR #885; target PR #882 is isolated (#693)" exact_next_action: "gitea_mark_final_review_decision(pr_number=882, ...) — foreign terminal on PR #885 does not block (#693); do not call gitea_authorize_review_correction for cross-PR unlock" evidence: { last_review_id: 580, live_mutations_count: 3 } locked_head_sha: "dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885" target_head_sha: "1cbbde00895dce1545290a660bc31c4ac9210b3e" ``` Identical output on two attempts. ### `gitea_diagnose_review_decision_lock(target_pr_number=882, expected_head_sha=1cbbde00...)` ``` classification: "foreign_pr_terminal" mark_final_allowed: true recovery_allowed: true recovery_mode: "cross_pr_isolation" correction_eligible: false cleanup_allowed: false last_terminal_pr: 885 last_terminal_action: "approve" lock_summary.ready_pr_number: 885 lock_summary.ready_expected_head_sha: "dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885" reasons: ["last terminal is approve on foreign open/closed PR #885; target PR #882 is isolated (#693)"] ``` `mark_final_allowed: true` and `marked_ready: false` were true at the same moment, for the same PR and the same head. ### The prescribed retry loop Both tools return the same `exact_next_action`: call `gitea_mark_final_review_decision` for PR #882 again — exactly the call that just failed. Following the prescribed recovery loops indefinitely. It was attempted twice, produced byte-identical output, and was then stopped deliberately. ### Even a non-terminal review comment was blocked The comment path is not an escape hatch. `gitea_submit_pr_review(pr_number=882, action=comment)` returned: ``` "final_review_decision_ready must be true; validation-phase live review mutations are forbidden — use gitea_dry_run_pr_review instead (fail closed)" ``` while `gitea_dry_run_pr_review` reported `would_perform: true` for both `request_changes` and `comment`. The only thing between a validated reviewer and a recorded verdict is a flag that `mark_final` refuses to set for cross-PR reasons. The reviewer had to fall back to an issue-thread comment (issue #661 comment `16232`) to record the blocking finding at all. ### The state is head-scoped but leaks across PRs In the same session, after PR #885's head moved off `dac40ab9...`: ``` gitea_mark_final_review_decision(pr_number=885, action=approve, expected_head_sha=95e4aae287902e629b879a39887042a56f2f0757) -> marked_ready: true, head_scoped: true ``` Moving #885 to a new head cleared the lock for #885. Terminal state is therefore already tracked per head — it is simply not isolated per PR. A single global "last terminal" slot (`lock_summary.ready_pr_number`) is consulted for every target PR. ## Safety impact PR #882 carried a reviewer-reproduced fail-open in `drain_proof.py`: with `counts.sessions_live_other = 3` and the acknowledgement key absent, `acks_or_timeout` passed with detail "no other live sessions required to acknowledge", the proof minted `clean: True`, and `gate_apply_restart` returned `verdict: allow` / `allow: True` — authorizing a restart against three live sessions with zero acknowledgement evidence. Because the blocking verdict could not be recorded, the undismissed approval `576` at that exact head remained in force and **PR #882 stayed merge-eligible despite the reproduced fail-open**. A merger running normal preflight would have seen `approval_at_current_head: true` and no blocking change request. This guard defect converts a reviewer's blocking finding into no server-side state change at all: a fail-open in the control plane guarding a fail-open in the product. ## Required behavior 1. **Key terminal review state by both PR number and exact head SHA.** A terminal decision on PR #885 must have no effect on a `mark_final` for PR #882 at any head. 2. **Make the mark-final and diagnostic paths use the same authorization decision.** `gitea_mark_final_review_decision` and `gitea_diagnose_review_decision_lock` must never disagree for the same `(pr_number, head_sha)`. One must be the single source of truth and the other must derive from it. 3. **Never prescribe the identical call that just failed as recovery.** When a refusal is not self-recoverable, `exact_next_action` must name a different, actually-available action, or declare the state terminal and require operator involvement. 4. **An unrecordable blocking decision must not silently leave a PR merge-eligible.** Either the prior approval is invalidated, or the merge path is held, so a blocked REQUEST_CHANGES cannot degrade to "approved" by default. 5. **Add regression coverage** for a terminal decision on PR A followed by a decision on unrelated PR B: the PR B decision must succeed, and `diagnose` must agree with `mark_final` in both directions. ## References - #693 — cross-PR isolation, cited by both tools in their own reasons - #620 — post-mutation correction gate, cited in the refusal - Issue #661 comment `16232` — the reviewer's blocking finding for PR #882, recorded on the issue thread because every PR review path was blocked - PR #882 (`feat/issue-661-drain-proof-hard-gate`), PR #885 (`issue-640`) ## Scope note This issue tracks the reviewer decision-lock guard only. The `drain_proof.py` acknowledgement fail-open that exposed it is remediated separately on PR #882 / issue #661 and is not in scope here.

jcwalker3 added the status:ready workflow-hardening terminal-lock safety type:bug labels 2026-07-24 21:39:26 -05:00

sysadmin referenced this issue

2026-07-24 22:34:07 -05:00

feat(restart): pre-restart drain proof and hard gate (Closes #661) #882

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

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