Allow canonical cleanup or override of stale #332 review decision locks after merged PRs #594

New Issue

2026-07-09T14:08:51-05:00

jcwalker3 commented

2026-07-09 14:08:51 -05:00

Problem

#332 correctly hard-stops a reviewer session after a terminal live review mutation (approve / request_changes). That protection must remain for active, unmerged, or ambiguous terminal review state.

After #559, review-decision locks are durable on disk under ~/.cache/gitea-tools/session-state/ (e.g. review_decision_lock-prgs-reviewer.json). That durability can outlive the work the lock was meant to protect: if the referenced PR is already merged or closed, no merge sequence remains for that PR, yet the durable ledger still blocks unrelated new terminal reviews forever.

Manual deletion of session-state files is not an acceptable normal workflow.

Live evidence (PR #592 review blocked by merged PR #586)

A reviewer completed technical review of PR #592 (Issue #590) and intended formal APPROVE, but formal submission was blocked:

terminal review mutation already consumed in this run (approve on PR #586);
only the merge sequence for approved PR #586 may continue (fail closed, #332)

Facts at block time:

Durable lock file: ~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json
Ledger live_mutations referenced prior terminal approvals including PR #586
PR #586 was already merged at commit 6913ac9 (merge_commit_sha / master tip)
Reviewer lease for PR #592 was acquired and released cleanly
Technical validation for PR #592 was green:
- git diff --check clean
- tests/test_mcp_server.py: 207 passed
- isolation / session-state / hard-stop / workflow suite: 32 passed
- key regressions: 4 passed
Blocker was stale session ledger state, not PR #592 code quality
Durable diagnosis recorded on PR #592 comment 8458 and Issue #590 comment 8465

Related (not duplicates)

Issue	Why related	Why not the same work
#332 (closed)	Defines the terminal hard-stop	Does not define post-merge stale-lock cleanup
#211 (closed)	Operator-authorized correction after mistaken live review	Correction re-opens review path for a mistake; not moot-after-merge cleanup
#559 (closed)	Made decision locks durable across daemon processes	Persistence without expiry created this outlive problem
#515 (closed)	Post-merge moot lease cleanup pattern	Leases (#407), not review-decision locks
#590 (open)	Test isolation so host ledger cannot poison pytest	Test-only; does not add production MCP cleanup/override

Required behavior

Detect moot locks. When a durable review-decision lock's last terminal mutation references a PR that is live-state merged or closed (and therefore no same-PR merge sequence is possible), classify the lock as stale/moot rather than active hard-stop.
Canonical MCP-safe cleanup/reset path. Provide a gated MCP tool (or gated extension of an existing review-state tool) that can clear, expire, or override a stale review-decision lock only when prior locked work is terminal/moot.
Preserve #332 for active work. If the locked PR is still open, or state is ambiguous, or merge of an approved open PR is still a valid continuation, hard-stop behavior remains fail-closed.
Identity / profile gates. Cleanup requires authenticated identity + profile permission checks (reviewer/operator-capable profile as designed). Fail closed if identity cannot be verified.
Durable audit trail. Every successful clear/override records who/what/when/why (profile, identity, prior lock payload summary, referenced PR number + live state proof, result). Prefer Gitea-visible audit (issue/PR comment or structured tool result that can be posted) plus local durable record as needed.
No file-delete workflow. Docs and tooling must not instruct operators to rm session-state files as the normal path.

Non-goals

Weakening #332 for open PRs or same-run multi-PR terminal mutations
Replacing #211 correction flow for mistaken live reviews on still-active PRs
Hand-editing or bypassing production hard-stop code from LLM sessions
Auto-approving or auto-merging any PR as a side effect of lock cleanup

Acceptance criteria

Detect when a durable review-decision lock references a PR that is already merged/closed (live Gitea state).
Allow a canonical cleanup/reset path only when the prior locked work is terminal/moot.
Preserve #332 hard-stop for active, unmerged, or ambiguous terminal review state.
Require identity/profile checks before clearing any lock.
Record a durable audit trail when a stale lock is cleared or overridden.
Tests prove:
- active review locks still block unrelated terminal reviews
- merged/closed PR locks can be cleared canonically
- stale lock cleanup cannot be used to bypass review gates on open PRs
- PR #592-style approval can proceed after PR #586-style stale lock cleanup
Workflow docs explain when stale lock cleanup is allowed vs forbidden.

Suggested implementation notes (non-binding)

Mirror the spirit of #515 moot-lease cleanup: read live PR state first; mutate only when moot.
Consider auto-expire on successful merge of the approved PR (merger path) and an explicit operator/reviewer recovery tool when auto-expire was skipped or another session wrote the lock.
Keep #211 gitea_authorize_review_correction for mistaken reviews; do not overload it as the only path for post-merge stale locks unless semantics stay fail-closed and clearly documented.

Source pointers

PR #592 review block: formal gitea_mark_final_review_decision fail-closed under #332 after prior approve of merged #586
PR #592 technical review comments: 8458 (diagnosis), lease 8454 / release 8470
Issue #590 comment 8465
Durable path: ~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json
Code: terminal_review_hard_stop_reasons / _load_review_decision_lock / _save_review_decision_lock in gitea_mcp_server.py; durable store in mcp_session_state.py

## Problem #332 correctly hard-stops a reviewer session after a terminal live review mutation (`approve` / `request_changes`). That protection must remain for **active**, **unmerged**, or **ambiguous** terminal review state. After #559, review-decision locks are **durable** on disk under `~/.cache/gitea-tools/session-state/` (e.g. `review_decision_lock-prgs-reviewer.json`). That durability can outlive the work the lock was meant to protect: if the referenced PR is already **merged or closed**, no merge sequence remains for that PR, yet the durable ledger still blocks unrelated new terminal reviews forever. Manual deletion of session-state files is **not** an acceptable normal workflow. ## Live evidence (PR #592 review blocked by merged PR #586) A reviewer completed technical review of PR #592 (Issue #590) and intended formal **APPROVE**, but formal submission was blocked: ```text terminal review mutation already consumed in this run (approve on PR #586); only the merge sequence for approved PR #586 may continue (fail closed, #332) ``` Facts at block time: * Durable lock file: `~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json` * Ledger `live_mutations` referenced prior terminal approvals including PR **#586** * PR **#586** was already **merged** at commit `6913ac9` (merge_commit_sha / master tip) * Reviewer lease for PR #592 was acquired and released cleanly * Technical validation for PR #592 was green: * `git diff --check` clean * `tests/test_mcp_server.py`: 207 passed * isolation / session-state / hard-stop / workflow suite: 32 passed * key regressions: 4 passed * Blocker was **stale session ledger state**, not PR #592 code quality * Durable diagnosis recorded on PR #592 comment 8458 and Issue #590 comment 8465 ## Related (not duplicates) | Issue | Why related | Why not the same work | |-------|-------------|------------------------| | #332 (closed) | Defines the terminal hard-stop | Does not define post-merge stale-lock cleanup | | #211 (closed) | Operator-authorized **correction** after mistaken live review | Correction re-opens review path for a mistake; not moot-after-merge cleanup | | #559 (closed) | Made decision locks durable across daemon processes | Persistence without expiry created this outlive problem | | #515 (closed) | Post-merge **moot lease** cleanup pattern | Leases (#407), not review-decision locks | | #590 (open) | Test isolation so host ledger cannot poison pytest | Test-only; does not add production MCP cleanup/override | ## Required behavior 1. **Detect moot locks.** When a durable review-decision lock's last terminal mutation references a PR that is live-state **merged** or **closed** (and therefore no same-PR merge sequence is possible), classify the lock as **stale/moot** rather than active hard-stop. 2. **Canonical MCP-safe cleanup/reset path.** Provide a gated MCP tool (or gated extension of an existing review-state tool) that can clear, expire, or override a **stale** review-decision lock **only** when prior locked work is terminal/moot. 3. **Preserve #332 for active work.** If the locked PR is still open, or state is ambiguous, or merge of an approved open PR is still a valid continuation, hard-stop behavior remains fail-closed. 4. **Identity / profile gates.** Cleanup requires authenticated identity + profile permission checks (reviewer/operator-capable profile as designed). Fail closed if identity cannot be verified. 5. **Durable audit trail.** Every successful clear/override records who/what/when/why (profile, identity, prior lock payload summary, referenced PR number + live state proof, result). Prefer Gitea-visible audit (issue/PR comment or structured tool result that can be posted) plus local durable record as needed. 6. **No file-delete workflow.** Docs and tooling must not instruct operators to `rm` session-state files as the normal path. ## Non-goals * Weakening #332 for open PRs or same-run multi-PR terminal mutations * Replacing #211 correction flow for mistaken live reviews on still-active PRs * Hand-editing or bypassing production hard-stop code from LLM sessions * Auto-approving or auto-merging any PR as a side effect of lock cleanup ## Acceptance criteria - [ ] Detect when a durable review-decision lock references a PR that is already merged/closed (live Gitea state). - [ ] Allow a canonical cleanup/reset path **only** when the prior locked work is terminal/moot. - [ ] Preserve #332 hard-stop for active, unmerged, or ambiguous terminal review state. - [ ] Require identity/profile checks before clearing any lock. - [ ] Record a durable audit trail when a stale lock is cleared or overridden. - [ ] Tests prove: - [ ] active review locks still block unrelated terminal reviews - [ ] merged/closed PR locks can be cleared canonically - [ ] stale lock cleanup cannot be used to bypass review gates on open PRs - [ ] PR #592-style approval can proceed after PR #586-style stale lock cleanup - [ ] Workflow docs explain when stale lock cleanup is **allowed** vs **forbidden**. ## Suggested implementation notes (non-binding) * Mirror the spirit of #515 moot-lease cleanup: read live PR state first; mutate only when moot. * Consider auto-expire on successful merge of the approved PR (merger path) **and** an explicit operator/reviewer recovery tool when auto-expire was skipped or another session wrote the lock. * Keep #211 `gitea_authorize_review_correction` for mistaken reviews; do not overload it as the only path for post-merge stale locks unless semantics stay fail-closed and clearly documented. ## Source pointers * PR #592 review block: formal `gitea_mark_final_review_decision` fail-closed under #332 after prior approve of merged #586 * PR #592 technical review comments: 8458 (diagnosis), lease 8454 / release 8470 * Issue #590 comment 8465 * Durable path: `~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json` * Code: `terminal_review_hard_stop_reasons` / `_load_review_decision_lock` / `_save_review_decision_lock` in `gitea_mcp_server.py`; durable store in `mcp_session_state.py`

jcwalker3 added the bug mcp gitea reliability workflow labels 2026-07-09 14:08:52 -05:00

sysadmin commented

2026-07-09 14:12:05 -05:00

Canonical Issue State

STATE: OPEN
WHO_IS_NEXT: author
NEXT_ACTION: Implement canonical MCP-safe stale review-decision lock cleanup/reset for merged or closed prior PRs without weakening active #332 hard-stop.
NEXT_PROMPT:

Gitea author task. Invoke gitea-workflow first.

Implement Issue #594: Allow canonical cleanup of stale reviewer decision locks after merged PRs.

Stay out of Timesheet. Do not mutate the root checkout. Use an isolated branches/ author worktree.
Do not weaken production #332 hard-stop for open/active/ambiguous review locks.

Deliver:
1) Detect durable review_decision_lock whose last terminal mutation targets a live-state merged or closed PR.
2) Gated MCP cleanup/reset tool requiring identity/profile/capability proof and live Gitea terminal verification.
3) Durable audit comment/trail on successful clear.
4) Tests: active locks still block; merged/closed locks clearable; cleanup cannot bypass open-PR gates; PR #592-style review proceeds after PR #586-style stale lock cleanup.
5) Workflow docs for allowed vs forbidden cleanup.

Related: #332, #559, #590 / PR #592.

WHAT_HAPPENED: Duplicate search confirmed this issue already tracks the work. No new issue created. This comment appends the latest PR #592 formal-review block evidence from a second independent reviewer pass.
WHY: Keep one durable tracker (#594) for stale #332+#559 decision-lock cleanup; avoid duplicate issues for the same root cause.
RELATED_PRS: #592 (blocked formal APPROVE), #590 (isolation fix under review), prior terminal mutations referenced in durable lock for #589 / #585 / #586
BLOCKERS: none for this issue body update; formal APPROVE of PR #592 remains blocked until this issue lands or operator clears the stale lock manually
VALIDATION:

Duplicate search: open issues scanned; exact match #594 title/body; related non-duplicates #332 (closed hard-stop), #559 (closed durable state), #590 (test isolation only), #515 (moot lease cleanup pattern, not decision locks)
Closed-related: #211 (correction path, not post-merge cleanup)
Fresh evidence from PR #592 technical review (sysadmin / prgs-reviewer, author jcwalker3, self_author=false)
LAST_UPDATED_BY: sysadmin (prgs-reviewer)

Additional PR #592 evidence (second pass)

Live PR #592

Head: b810e512fd9f50b7c0106305914ad79553faee12
State: open, mergeable=true
Author: jcwalker3
Reviewer: sysadmin / prgs-reviewer (not self)
Changed files (tests only):
- tests/conftest.py
- tests/test_mcp_server.py
- tests/test_session_state_isolation.py
Production code: 0 files changed

Validation (isolated branches/review-pr-592-issue-590)

git diff --check prgs/master...HEAD: clean
python3 -m pytest tests/test_mcp_server.py -q: 207 passed
isolation / session-state / hard-stop / workflow / stale-runtime suites: 33 passed
targeted test_unknown_profile_blocks + env-clear regression: 2 passed

Formal review block

gitea_mark_final_review_decision fail-closed:

terminal review mutation already consumed in this run (approve on PR #586);
only the merge sequence for approved PR #586 may continue (fail closed, #332)

Durable lock path: ~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json
Lock live_mutations included terminal approves for PRs #589, #585, #586 (last terminal: #586)
This is not a PR #592 code defect; it is stale durable session-state cleanup

Durable tracking comments from that blocked review

PR #592 comment #8472
Issue #590 comment #8479
Earlier diagnosis comments also referenced in issue body: #8458 / #8465

Required fix reminder (unchanged scope)
Canonical MCP-safe cleanup/override only when prior lock target is terminal (merged/closed/no merge sequence remains). Must preserve #332 for active/unmerged/ambiguous locks. Require identity/profile/capability proof + live Gitea verification + durable audit trail. Tests and workflow docs as listed in the issue acceptance criteria.

## Canonical Issue State STATE: OPEN WHO_IS_NEXT: author NEXT_ACTION: Implement canonical MCP-safe stale review-decision lock cleanup/reset for merged or closed prior PRs without weakening active #332 hard-stop. NEXT_PROMPT: ```text Gitea author task. Invoke gitea-workflow first. Implement Issue #594: Allow canonical cleanup of stale reviewer decision locks after merged PRs. Stay out of Timesheet. Do not mutate the root checkout. Use an isolated branches/ author worktree. Do not weaken production #332 hard-stop for open/active/ambiguous review locks. Deliver: 1) Detect durable review_decision_lock whose last terminal mutation targets a live-state merged or closed PR. 2) Gated MCP cleanup/reset tool requiring identity/profile/capability proof and live Gitea terminal verification. 3) Durable audit comment/trail on successful clear. 4) Tests: active locks still block; merged/closed locks clearable; cleanup cannot bypass open-PR gates; PR #592-style review proceeds after PR #586-style stale lock cleanup. 5) Workflow docs for allowed vs forbidden cleanup. Related: #332, #559, #590 / PR #592. ``` WHAT_HAPPENED: Duplicate search confirmed this issue already tracks the work. No new issue created. This comment appends the latest PR #592 formal-review block evidence from a second independent reviewer pass. WHY: Keep one durable tracker (#594) for stale #332+#559 decision-lock cleanup; avoid duplicate issues for the same root cause. RELATED_PRS: #592 (blocked formal APPROVE), #590 (isolation fix under review), prior terminal mutations referenced in durable lock for #589 / #585 / #586 BLOCKERS: none for this issue body update; formal APPROVE of PR #592 remains blocked until this issue lands or operator clears the stale lock manually VALIDATION: - Duplicate search: open issues scanned; exact match #594 title/body; related non-duplicates #332 (closed hard-stop), #559 (closed durable state), #590 (test isolation only), #515 (moot lease cleanup pattern, not decision locks) - Closed-related: #211 (correction path, not post-merge cleanup) - Fresh evidence from PR #592 technical review (sysadmin / prgs-reviewer, author jcwalker3, self_author=false) LAST_UPDATED_BY: sysadmin (prgs-reviewer) --- ### Additional PR #592 evidence (second pass) **Live PR #592** - Head: `b810e512fd9f50b7c0106305914ad79553faee12` - State: open, mergeable=true - Author: `jcwalker3` - Reviewer: `sysadmin` / `prgs-reviewer` (not self) - Changed files (tests only): - `tests/conftest.py` - `tests/test_mcp_server.py` - `tests/test_session_state_isolation.py` - Production code: 0 files changed **Validation (isolated `branches/review-pr-592-issue-590`)** - `git diff --check prgs/master...HEAD`: clean - `python3 -m pytest tests/test_mcp_server.py -q`: **207 passed** - isolation / session-state / hard-stop / workflow / stale-runtime suites: **33 passed** - targeted `test_unknown_profile_blocks` + env-clear regression: **2 passed** **Formal review block** - `gitea_mark_final_review_decision` fail-closed: ```text terminal review mutation already consumed in this run (approve on PR #586); only the merge sequence for approved PR #586 may continue (fail closed, #332) ``` - Durable lock path: `~/.cache/gitea-tools/session-state/review_decision_lock-prgs-reviewer.json` - Lock `live_mutations` included terminal approves for PRs **#589, #585, #586** (last terminal: #586) - This is **not** a PR #592 code defect; it is stale durable session-state cleanup **Durable tracking comments from that blocked review** - PR #592 comment `#8472` - Issue #590 comment `#8479` - Earlier diagnosis comments also referenced in issue body: #8458 / #8465 **Required fix reminder (unchanged scope)** Canonical MCP-safe cleanup/override only when prior lock target is terminal (merged/closed/no merge sequence remains). Must preserve #332 for active/unmerged/ambiguous locks. Require identity/profile/capability proof + live Gitea verification + durable audit trail. Tests and workflow docs as listed in the issue acceptance criteria.

jcwalker3 added the status:in-progress label 2026-07-09 14:13:21 -05:00

jcwalker3 commented

2026-07-09 14:13:21 -05:00

Issue claim heartbeat

kind: claim
issue: Allow canonical cleanup or override of stale #332 review decision locks after merged PRs (#594)
branch: feat/issue-594-stale-decision-lock-cleanup
phase: claimed
profile: prgs-author
pr: none
blocker: none
next_action: create worktree and begin implementation

**Issue claim heartbeat** - kind: claim - issue: #594 - branch: feat/issue-594-stale-decision-lock-cleanup - phase: claimed - profile: prgs-author - pr: none - blocker: none - next_action: create worktree and begin implementation

jcwalker3 referenced this issue from a commit

2026-07-09 14:19:12 -05:00

feat: canonical cleanup for stale #332 review decision locks (#594)

jcwalker3 referenced this issue from a commit

2026-07-09 14:21:27 -05:00

docs: expand #594 stale decision-lock cleanup guidance

jcwalker3 referenced this issue from a commit

2026-07-09 14:25:11 -05:00

test: harden session-state isolation for #594 decision-lock suite

jcwalker3 referenced a pull request that will close this issue

2026-07-09 14:28:37 -05:00

feat: canonical cleanup for stale #332 review decision locks (Closes #594) #595