diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index effb9f7..5fc591b 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -1049,6 +1049,72 @@ Special states: Final reports must not claim a comment was posted when `canonical_comment_validation.allowed` is false (#496 AC14). +## Stale #332 review-decision lock cleanup (#594) + +#332 hard-stops a reviewer session after a terminal live review mutation +(`approve` / `request_changes`). After #559 those locks are **durable** under +`~/.cache/gitea-tools/session-state/review_decision_lock-.json`, so they +can outlive the PR they protected. + +### When cleanup is **allowed** + +Use `gitea_cleanup_stale_review_decision_lock` from a **reviewer** profile when: + +1. A durable review-decision lock has a terminal live mutation, **and** +2. Live Gitea state shows that terminal PR is already **merged** or **closed** + (so no same-PR merge sequence remains), **and** +3. The active profile identity matches the lock, **and** +4. Authenticated identity can be verified. + +Workflow: + +```text +# 1) Assess only (default) +gitea_cleanup_stale_review_decision_lock(apply=false, remote=prgs, ...) + +# 2) Apply after is_moot=true and cleanup_allowed=true +gitea_cleanup_stale_review_decision_lock( + apply=true, + expected_terminal_pr=, + remote=prgs, + ... +) +``` + +Successful apply: + +* clears in-memory + durable decision lock for that profile +* returns a structured `audit` payload +* posts an audit comment on the mooted PR when `gitea.pr.comment` is allowed + (`post_audit_comment=true` default) + +Same-profile auto-expire: if `gitea_merge_pr` succeeds on a PR that this same +profile just approved, the decision lock is cleared after merge. Cross-profile +locks (reviewer vs merger) still require the cleanup tool. + +### When cleanup is **forbidden** + +Do **not** clear when: + +* the last terminal PR is still **open** (active #332 hard-stop — continue merge + for that approve, or stop after request_changes) +* live PR state cannot be fetched (fail closed) +* profile identity does not match the lock +* identity cannot be verified +* `expected_terminal_pr` does not match the last terminal PR + +**Never** use manual deletion of session-state files as the normal workflow. +**Never** use cleanup to skip review of an open PR or to bypass eligibility / +mark-ready / submit gates on active work. + +### Related tools + +| Tool | Purpose | +|------|---------| +| `gitea_cleanup_stale_review_decision_lock` | Moot decision-lock cleanup (#594) | +| `gitea_authorize_review_correction` | Operator-approved correction after a **mistaken** live review on still-active work (#211) | +| `gitea_cleanup_post_merge_moot_lease` | Moot **lease** cleanup after merge (#515) — different object | + ## Fail-closed behavior Before any mutating action the workflow verifies identity, active profile, diff --git a/docs/wiki/Safety-and-Gates.md b/docs/wiki/Safety-and-Gates.md index 25c21fb..d8182a9 100644 --- a/docs/wiki/Safety-and-Gates.md +++ b/docs/wiki/Safety-and-Gates.md @@ -15,5 +15,7 @@ 5. **Explicit merge confirmation** — `gitea_merge_pr` requires `confirmation="MERGE PR "`. 6. **No self-review / self-merge** — Authenticated user must differ from PR author for approve/merge. 7. **Review decision lock** — Live review mutations require validation-phase dry-run and `gitea_mark_final_review_decision`. -8. **Redaction** — Tokens, passwords, and keychain material never appear in tool output. -9. **Wiki publication (#224)** — `docs/wiki/` and the sync helper are prerequisites only. Closing a wiki issue requires live Gitea Wiki proof on the repo Wiki tab. See [Runbooks](Runbooks.md#wiki-publication-readiness-gate-224). \ No newline at end of file +8. **Terminal review hard-stop (#332)** — After a terminal live review mutation, only same-PR merge after `approve` may continue. Durable locks (#559) must not be deleted by hand. +9. **Stale decision-lock cleanup (#594)** — `gitea_cleanup_stale_review_decision_lock` may clear a durable #332 lock **only** when the last terminal mutation's PR is live-state **merged or closed**, identity/profile gates pass, and apply uses a reviewer-capable profile. Open/ambiguous locks stay fail-closed. Successful cleanup records a durable audit trail. +10. **Redaction** — Tokens, passwords, and keychain material never appear in tool output. +11. **Wiki publication (#224)** — `docs/wiki/` and the sync helper are prerequisites only. Closing a wiki issue requires live Gitea Wiki proof on the repo Wiki tab. See [Runbooks](Runbooks.md#wiki-publication-readiness-gate-224). \ No newline at end of file diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd50942..18b9d03 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -807,6 +807,7 @@ import review_proofs # noqa: E402 import review_workflow_boundary # noqa: E402 import review_workflow_load # noqa: E402 import mcp_session_state # noqa: E402 +import stale_review_decision_lock # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import issue_lock_provenance # noqa: E402 @@ -3096,10 +3097,15 @@ def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[st ) else: guidance = "the session must stop and produce a final report" + recovery = ( + "if that PR is already merged/closed, use " + "gitea_cleanup_stale_review_decision_lock (apply=true) after live-state " + "proof (#594); never delete session-state files by hand" + ) return [ "terminal review mutation already consumed in this run " f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} " - "(fail closed, #332)" + f"(fail closed, #332); {recovery}" ] @@ -3872,6 +3878,205 @@ def gitea_authorize_review_correction( return {"authorized": True, "correction_reason": reason, "reasons": []} +@mcp.tool() +def gitea_cleanup_stale_review_decision_lock( + apply: bool = False, + expected_terminal_pr: int | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, + post_audit_comment: bool = True, +) -> dict: + """Clear a durable #332 review-decision lock only when it is moot (#594). + + Read-first by default (``apply=False``). A lock is moot when its last + terminal live mutation references a PR that is already **merged** or + **closed** on live Gitea — so no same-PR merge sequence remains. + + Fail-closed when: + - no lock / no terminal mutation + - live PR is still open (active #332 hard-stop preserved) + - live PR state cannot be fetched + - active profile identity does not match the lock + - apply requested without reviewer capability (``gitea.pr.review``) + + Never weakens #332 for open PRs. Never instructs manual session-state + file deletion. On successful apply, clears memory + durable store and + returns a durable audit record (optionally posted as a PR comment). + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "cleanup_performed": False, + "is_moot": False, + "cleanup_allowed": False, + "mode": "apply" if apply else "read_only", + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + binding = _decision_lock_binding() + active_identity = binding.get("profile_identity") + lock = _load_review_decision_lock() + last = stale_review_decision_lock.last_terminal_mutation(lock) + pr_live = None + pr_lookup_error = None + last_pr = last.get("pr_number") if last else None + + if last_pr is not None: + try: + pr_live = api_request( + "GET", f"{repo_api_url(h, o, r)}/pulls/{last_pr}", auth + ) or {} + if not pr_live: + pr_lookup_error = "empty PR payload" + except Exception as exc: # noqa: BLE001 — surface redacted + pr_lookup_error = _redact(str(exc)) + + assessment = stale_review_decision_lock.assess_stale_review_decision_lock( + lock, + pr_live=pr_live, + pr_lookup_error=pr_lookup_error, + active_profile_identity=active_identity, + ) + + # Optional pin: refuse apply against a different terminal PR than expected. + if ( + expected_terminal_pr is not None + and assessment.get("last_terminal_pr") is not None + and int(expected_terminal_pr) != int(assessment["last_terminal_pr"]) + ): + assessment = dict(assessment) + assessment["cleanup_allowed"] = False + assessment["reasons"] = list(assessment.get("reasons") or []) + [ + f"expected_terminal_pr #{expected_terminal_pr} does not match " + f"last terminal PR #{assessment.get('last_terminal_pr')} " + "(fail closed, #594)" + ] + + try: + actor = _authenticated_username(h) + except Exception: + actor = None + profile = get_profile() + profile_name = (profile.get("profile_name") or "").strip() or None + + audit = stale_review_decision_lock.build_cleanup_audit_record( + assessment=assessment, + actor_username=actor, + profile_name=profile_name, + applied=False, + ) + + report = { + "success": True, + "cleanup_performed": False, + "mode": "apply" if apply else "read_only", + "remote": remote, + "org": o, + "repo": r, + "active_profile_identity": active_identity, + "authenticated_username": actor, + "has_lock": assessment.get("has_lock"), + "is_moot": assessment.get("is_moot"), + "cleanup_allowed": assessment.get("cleanup_allowed"), + "last_terminal_pr": assessment.get("last_terminal_pr"), + "last_terminal_action": assessment.get("last_terminal_action"), + "pr_state": assessment.get("pr_state"), + "pr_merged": assessment.get("pr_merged"), + "pr_merged_or_closed": assessment.get("pr_merged_or_closed"), + "merge_commit_sha": assessment.get("merge_commit_sha"), + "lock_summary": assessment.get("lock_summary"), + "audit": audit, + "audit_comment_id": None, + "reasons": list(assessment.get("reasons") or []), + "manual_file_delete_forbidden": True, + } + + if not apply: + return report + + if not assessment.get("cleanup_allowed"): + report["success"] = False + report["cleanup_skipped_reason"] = list(assessment.get("reasons") or []) + return report + + if not actor: + report["success"] = False + report["reasons"] = [ + "authenticated identity could not be verified; refuse lock " + "cleanup (fail closed, #594)" + ] + return report + + review_block = _profile_operation_gate("gitea.pr.review") + if review_block: + report["success"] = False + report["reasons"] = review_block + report["permission_report"] = _permission_block_report("gitea.pr.review") + return report + + session_reasons = _review_decision_session_reasons(lock) + if session_reasons: + report["success"] = False + report["reasons"] = session_reasons + return report + + # Clear durable + in-memory lock only after all gates pass. + prior_summary = assessment.get("lock_summary") + _save_review_decision_lock(None) + audit = stale_review_decision_lock.build_cleanup_audit_record( + assessment=assessment, + actor_username=actor, + profile_name=profile_name, + applied=True, + ) + audit["prior_lock_summary"] = prior_summary + report["cleanup_performed"] = True + report["audit"] = audit + report["reasons"] = list(assessment.get("reasons") or []) + [ + f"cleared durable review decision lock for moot terminal mutation " + f"on PR #{assessment.get('last_terminal_pr')} (#594)" + ] + + if post_audit_comment and assessment.get("last_terminal_pr") is not None: + comment_block = _profile_operation_gate("gitea.pr.comment") + if comment_block: + report["audit_comment_skipped"] = comment_block + else: + try: + body = stale_review_decision_lock.format_cleanup_audit_comment( + audit + ) + comment_url = ( + f"{repo_api_url(h, o, r)}/issues/" + f"{assessment['last_terminal_pr']}/comments" + ) + with _audited( + "comment_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=assessment["last_terminal_pr"], + request_metadata={ + "source": "cleanup_stale_review_decision_lock" + }, + ): + posted = api_request( + "POST", comment_url, auth, {"body": body} + ) + report["audit_comment_id"] = (posted or {}).get("id") + except Exception as exc: # noqa: BLE001 + report["audit_comment_error"] = _redact(str(exc)) + + return report + + @mcp.tool() def gitea_dry_run_pr_review( pr_number: int, @@ -4609,6 +4814,29 @@ def gitea_merge_pr( reviewer_pr_lease.get_session_lease() ) ) + # Same-profile auto-expire (#594): if *this* profile's decision lock ends + # with an approve of the PR just merged, clear it so durable state cannot + # block later unrelated reviews. Cross-profile locks (e.g. reviewer vs + # merger) still require gitea_cleanup_stale_review_decision_lock. + try: + lock_after = _load_review_decision_lock() + last_term = stale_review_decision_lock.last_terminal_mutation(lock_after) + if ( + last_term + and last_term.get("action") == "approve" + and last_term.get("pr_number") == pr_number + ): + _save_review_decision_lock(None) + result["review_decision_lock_cleared_after_merge"] = True + reasons.append( + f"cleared same-profile review decision lock after merge of " + f"approved PR #{pr_number} (#594)" + ) + else: + result["review_decision_lock_cleared_after_merge"] = False + except Exception as clear_exc: # noqa: BLE001 — never fail the merge + result["review_decision_lock_cleared_after_merge"] = False + result["review_decision_lock_clear_error"] = _redact(str(clear_exc)) reasons.append(f"all gates passed; merged PR #{pr_number} via '{do}'") return result diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 6c35ce9..e0b3996 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -826,6 +826,75 @@ The final report must identify: * whether same-PR merge continuation was allowed * whether the run stopped as required +## 26A-1. Stale durable review-decision lock cleanup (#594) + +After #559, the review-decision lock is durable under +`~/.cache/gitea-tools/session-state/` (for example +`review_decision_lock-prgs-reviewer.json`). That durability can outlive the work +it protects: a terminal `approve` / `request_changes` on a PR that is already +**merged or closed** still hard-stops later unrelated reviews under #332. + +**Do not** delete session-state files by hand. Use the canonical MCP path. + +### When cleanup is allowed + +Use `gitea_cleanup_stale_review_decision_lock` when: + +1. `gitea_mark_final_review_decision` / live review is blocked by + `terminal review mutation already consumed ... (#332)`. +2. Live Gitea state for the **last terminal mutation's PR** is **merged** or + **closed** (no same-PR merge sequence remains). +3. Active profile identity matches the durable lock (reviewer profile with + `gitea.pr.review` for `apply=true`). +4. Optional pin: pass `expected_terminal_pr` to the prior terminal PR number + (for example `586` when resuming after a merged #586 lock). + +Recommended sequence: + +```text +gitea_whoami +gitea_resolve_task_capability(task="cleanup_stale_review_decision_lock") +gitea_cleanup_stale_review_decision_lock(apply=false, remote=..., org=..., repo=...) +# inspect is_moot / cleanup_allowed / last_terminal_pr +gitea_cleanup_stale_review_decision_lock( + apply=true, + expected_terminal_pr=, + remote=..., org=..., repo=..., +) +gitea_resolve_task_capability(task="review_pr") +gitea_load_review_workflow() +# continue formal mark + submit for the *new* PR +``` + +Successful `apply=true` clears memory + durable store and returns an audit +record (and posts a durable PR comment on the terminal PR when +`post_audit_comment` is true and comment permission allows). + +Same-profile auto-expire: after `gitea_merge_pr` succeeds for the PR that this +profile just approved, the decision lock for that profile is cleared +automatically. Cross-profile locks (for example reviewer approve, merger merge) +still require `gitea_cleanup_stale_review_decision_lock`. + +### When cleanup is forbidden + +Refuse / fail-closed — keep #332 hard-stop — when: + +* the last terminal PR is still **open** (active review or merge sequence may + still apply) +* live PR state cannot be fetched (ambiguous) +* no lock or no terminal live mutation exists +* profile identity does not match the lock +* apply requested without authenticated identity or `gitea.pr.review` +* `expected_terminal_pr` does not match the last terminal PR + +Do **not** use cleanup to: + +* bypass #332 on an open PR +* replace `gitea_authorize_review_correction` for a mistaken review on a still + active PR (#211) +* auto-approve or auto-merge any PR +* justify `rm` of session-state files + ## 26B. Per-PR reviewer lease (#407) Parallel reviewer sessions are allowed only when each session holds a distinct, @@ -1043,6 +1112,8 @@ Blocked handoffs must say to rerun the full workflow after the blocker clears. If the blocker is a terminal review mutation already consumed in the current run, the handoff must say that the next run must start from the beginning and must not reuse stale ready/approved state. +If the referenced terminal PR is already **merged or closed** on live Gitea, the durable #332 decision lock is **moot**. Do **not** delete session-state files by hand. Use `gitea_cleanup_stale_review_decision_lock` (assess with `apply=false`, then `apply=true` with `expected_terminal_pr` set) from the reviewer profile after identity/profile checks (#594). Cleanup is **forbidden** while that PR is still open. + ## 30. Final report must be precise Include: diff --git a/stale_review_decision_lock.py b/stale_review_decision_lock.py new file mode 100644 index 0000000..28b6fc9 --- /dev/null +++ b/stale_review_decision_lock.py @@ -0,0 +1,252 @@ +"""Stale / moot #332 review-decision lock detection and cleanup policy (#594). + +#332 correctly hard-stops a reviewer session after a terminal live review +mutation. After #559 those locks are durable on disk, so they can outlive the +work they protect: when the referenced PR is already merged or closed, no +same-PR merge sequence remains, yet the durable ledger still blocks unrelated +new terminal reviews. + +This module is pure policy (no Gitea I/O). The MCP tool +``gitea_cleanup_stale_review_decision_lock`` fetches live PR state, calls these +helpers, and only then clears durable state when cleanup is allowed. + +Manual deletion of ``~/.cache/gitea-tools/session-state/*`` is never the +canonical path. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from typing import Any + +# Keep in sync with gitea_mcp_server._TERMINAL_REVIEW_ACTIONS. +TERMINAL_REVIEW_ACTIONS = frozenset({"approve", "request_changes"}) + + +def last_terminal_mutation(lock: dict | None) -> dict | None: + """Return the last terminal live mutation on *lock*, or None.""" + if not lock: + return None + terminals = [ + m + for m in (lock.get("live_mutations") or []) + if isinstance(m, dict) and m.get("action") in TERMINAL_REVIEW_ACTIONS + ] + return terminals[-1] if terminals else None + + +def classify_pr_live_state(pr_live: dict | None) -> dict[str, Any]: + """Normalize a Gitea PR payload into merge/closed flags.""" + pr = pr_live or {} + merged = bool(pr.get("merged") or pr.get("merged_at")) + state = (pr.get("state") or "").strip().lower() or None + closed = state == "closed" + return { + "pr_state": state, + "pr_merged": merged, + "pr_merged_or_closed": merged or closed, + "merge_commit_sha": pr.get("merge_commit_sha") + or pr.get("merged_commit_sha"), + } + + +def lock_summary(lock: dict | None) -> dict[str, Any] | None: + """LLM-safe summary of a decision lock (no secrets).""" + if lock is None: + return None + last = last_terminal_mutation(lock) + mutations = list(lock.get("live_mutations") or []) + return { + "task": lock.get("task"), + "remote": lock.get("remote"), + "org": lock.get("org") or lock.get("ready_org"), + "repo": lock.get("repo") or lock.get("ready_repo"), + "profile_identity": lock.get("profile_identity") + or lock.get("session_profile_lock") + or lock.get("session_profile"), + "session_profile": lock.get("session_profile"), + "ready_pr_number": lock.get("ready_pr_number"), + "ready_action": lock.get("ready_action"), + "live_mutations_count": len(mutations), + "last_terminal": ( + { + "pr_number": last.get("pr_number"), + "action": last.get("action"), + "review_id": last.get("review_id"), + } + if last + else None + ), + "correction_authorized": bool(lock.get("correction_authorized")), + "updated_at": lock.get("updated_at") or lock.get("recorded_at"), + } + + +def assess_stale_review_decision_lock( + lock: dict | None, + *, + pr_live: dict | None = None, + pr_lookup_error: str | None = None, + active_profile_identity: str | None = None, +) -> dict[str, Any]: + """Assess whether a durable review-decision lock is stale/moot (#594). + + *pr_live* must be the live Gitea payload for the **last terminal + mutation's PR**. When no lock / no terminal mutation exists, cleanup is + not applicable. When the PR is still open (or lookup fails), cleanup is + forbidden and #332 hard-stop remains in force. + """ + summary = lock_summary(lock) + result: dict[str, Any] = { + "has_lock": lock is not None, + "lock_summary": summary, + "last_terminal_pr": None, + "last_terminal_action": None, + "is_moot": False, + "cleanup_allowed": False, + "profile_match": True, + "pr_state": None, + "pr_merged": False, + "pr_merged_or_closed": False, + "merge_commit_sha": None, + "reasons": [], + "recovery_tool": "gitea_cleanup_stale_review_decision_lock", + } + + if lock is None: + result["reasons"].append("no review decision lock present") + return result + + # Profile identity gate (caller may re-check with env; pure assess still + # reports mismatch when active identity is supplied). + stored = ( + (lock.get("profile_identity") or lock.get("session_profile_lock") or "") + .strip() + ) + active = (active_profile_identity or "").strip() + if active and stored and active != stored: + result["profile_match"] = False + result["reasons"].append( + "review decision lock profile identity does not match active " + f"profile '{active}' (fail closed, #594)" + ) + return result + + last = last_terminal_mutation(lock) + if last is None: + result["reasons"].append( + "review decision lock has no terminal live mutations; nothing to clear" + ) + return result + + pr_number = last.get("pr_number") + action = last.get("action") + result["last_terminal_pr"] = pr_number + result["last_terminal_action"] = action + + if pr_number is None: + result["reasons"].append( + "last terminal mutation missing pr_number; refuse cleanup (fail closed)" + ) + return result + + if pr_lookup_error: + result["reasons"].append( + f"could not load live state for PR #{pr_number}: {pr_lookup_error} " + "(fail closed, #594)" + ) + return result + + if pr_live is None: + result["reasons"].append( + f"live PR state required for terminal PR #{pr_number} before cleanup " + "(fail closed, #594)" + ) + return result + + live = classify_pr_live_state(pr_live) + result.update( + { + "pr_state": live["pr_state"], + "pr_merged": live["pr_merged"], + "pr_merged_or_closed": live["pr_merged_or_closed"], + "merge_commit_sha": live["merge_commit_sha"], + } + ) + + if not live["pr_merged_or_closed"]: + result["reasons"].append( + f"terminal review mutation on open PR #{pr_number} is still active " + f"({action}); #332 hard-stop remains — cleanup forbidden (#594)" + ) + return result + + # Merged or closed: lock is moot. Same-PR merge continuation is impossible. + result["is_moot"] = True + result["cleanup_allowed"] = True + result["reasons"].append( + f"terminal mutation ({action} on PR #{pr_number}) is moot: PR is " + f"{'merged' if live['pr_merged'] else 'closed'}; canonical cleanup allowed (#594)" + ) + return result + + +def build_cleanup_audit_record( + *, + assessment: dict[str, Any], + actor_username: str | None, + profile_name: str | None, + applied: bool, +) -> dict[str, Any]: + """Structured durable audit payload for a cleanup attempt.""" + last = (assessment.get("lock_summary") or {}).get("last_terminal") or {} + return { + "event": "stale_review_decision_lock_cleanup", + "issue_ref": "#594", + "applied": bool(applied), + "timestamp": datetime.now(timezone.utc).isoformat(), + "actor_username": actor_username, + "profile_name": profile_name, + "last_terminal_pr": assessment.get("last_terminal_pr") or last.get("pr_number"), + "last_terminal_action": assessment.get("last_terminal_action") + or last.get("action"), + "pr_state": assessment.get("pr_state"), + "pr_merged": assessment.get("pr_merged"), + "pr_merged_or_closed": assessment.get("pr_merged_or_closed"), + "merge_commit_sha": assessment.get("merge_commit_sha"), + "prior_live_mutations_count": ( + (assessment.get("lock_summary") or {}).get("live_mutations_count") + ), + "prior_profile_identity": ( + (assessment.get("lock_summary") or {}).get("profile_identity") + ), + "cleanup_allowed": assessment.get("cleanup_allowed"), + "is_moot": assessment.get("is_moot"), + "reasons": list(assessment.get("reasons") or []), + } + + +def format_cleanup_audit_comment(audit: dict[str, Any]) -> str: + """Markdown body for a durable Gitea audit comment after cleanup.""" + status = "APPLIED" if audit.get("applied") else "ASSESSED (not applied)" + lines = [ + "## Stale #332 review-decision lock cleanup (#594)", + "", + f"Status: **{status}**", + "", + f"- actor: `{audit.get('actor_username')}`", + f"- profile: `{audit.get('profile_name')}`", + f"- timestamp: `{audit.get('timestamp')}`", + f"- last terminal: `{audit.get('last_terminal_action')}` on " + f"PR #{audit.get('last_terminal_pr')}", + f"- PR state: `{audit.get('pr_state')}` " + f"(merged={audit.get('pr_merged')})", + f"- merge_commit_sha: `{audit.get('merge_commit_sha')}`", + f"- prior live_mutations_count: " + f"`{audit.get('prior_live_mutations_count')}`", + f"- prior profile_identity: `{audit.get('prior_profile_identity')}`", + "", + "Manual deletion of session-state files is **not** the workflow.", + "This path only clears a lock when the referenced PR is merged/closed.", + ] + return "\n".join(lines) diff --git a/task_capability_map.py b/task_capability_map.py index 14f3c70..3842c2b 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -96,6 +96,17 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.pr.approve", "role": "reviewer", }, + # #594: clear durable #332 decision lock only when last terminal PR is + # already merged/closed (moot). Apply path requires reviewer review + # permission; assessment itself uses gitea.read inside the tool. + "cleanup_stale_review_decision_lock": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, + "gitea_cleanup_stale_review_decision_lock": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, "delete_branch": { "permission": "gitea.branch.delete", "role": "author", diff --git a/tests/conftest.py b/tests/conftest.py index f7c9e8c..1873223 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ """Shared pytest fixtures for the Gitea-Tools test suite.""" +import os import sys import pytest @@ -16,13 +17,41 @@ def _reset_mutation_authority(monkeypatch): the next test. It must never replace verify_mutation_authority with a no-op: individual tests that need a specific authority state set it up explicitly. + + Session-state isolation (#559 / #590 / #594): many tests use + ``patch.dict(os.environ, ..., clear=True)``, which drops + ``GITEA_MCP_SESSION_STATE_DIR``. Pin ``default_state_dir`` / + ``DEFAULT_STATE_DIR`` to a per-test temp dir so durable load/save never + touches host state even after env clears. """ monkeypatch.delenv("GITEA_SESSION_PROFILE_LOCK", raising=False) # Isolate durable session-state files so tests never share host cache (#559). import tempfile _state_tmp = tempfile.TemporaryDirectory(prefix="gitea-session-state-") - monkeypatch.setenv("GITEA_MCP_SESSION_STATE_DIR", _state_tmp.name) + state_dir = _state_tmp.name + monkeypatch.setenv("GITEA_MCP_SESSION_STATE_DIR", state_dir) + try: + import mcp_session_state + except Exception: + mcp_session_state = None + if mcp_session_state is not None: + monkeypatch.setattr( + mcp_session_state, "DEFAULT_STATE_DIR", state_dir, raising=False + ) + + def _isolated_default_state_dir( + _fallback: str = state_dir, + _env_key: str = mcp_session_state.STATE_DIR_ENV, + ) -> str: + raw = (os.environ.get(_env_key) or "").strip() + return raw or _fallback + + monkeypatch.setattr( + mcp_session_state, + "default_state_dir", + _isolated_default_state_dir, + ) try: import mcp_server except Exception: @@ -40,6 +69,11 @@ def _reset_mutation_authority(monkeypatch): monkeypatch.setattr(mcp_server, "_preflight_capability_baseline_porcelain", None) monkeypatch.setattr(mcp_server, "_preflight_resolved_task", None) monkeypatch.setattr(mcp_server, "_preflight_reviewer_violation_files", []) + monkeypatch.setattr( + mcp_server, + "_check_mcp_runtimes_diagnostics", + lambda *args, **kwargs: [], + ) try: import review_workflow_load review_workflow_load._REVIEW_WORKFLOW_LOAD = None diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index d52263b..a793e15 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -774,6 +774,9 @@ class TestMergePR(unittest.TestCase): def setUp(self): import reviewer_pr_lease + # Clean ledger each merge test so residual/host durable #332 state + # cannot short-circuit eligibility gates (#590 / #594). + mcp_server._save_review_decision_lock(None) mcp_server.gitea_load_review_workflow() self._lease_patch = _install_owned_reviewer_lease(8) self._lease_patch.start() diff --git a/tests/test_stale_review_decision_lock_cleanup.py b/tests/test_stale_review_decision_lock_cleanup.py new file mode 100644 index 0000000..1425644 --- /dev/null +++ b/tests/test_stale_review_decision_lock_cleanup.py @@ -0,0 +1,378 @@ +"""Stale #332 review-decision lock cleanup (#594). + +Proves: + a. active terminal locks still block unrelated terminal reviews + b. merged/closed PR locks can be cleared canonically + c. cleanup cannot bypass review gates on open PRs + d. after moot cleanup, mark_ready for a different PR can proceed + (PR #592-style after PR #586-style stale approve) +""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_server +import stale_review_decision_lock as srdl +from mcp_server import ( + gitea_cleanup_stale_review_decision_lock, + gitea_mark_final_review_decision, +) + +HEAD = "a" * 40 +FAKE_AUTH = "Basic dGVzdDp0ZXN0" + +REVIEWER_ENV = { + "GITEA_PROFILE_NAME": "prgs-reviewer", + "GITEA_ALLOWED_OPERATIONS": ( + "gitea.read,gitea.pr.review,gitea.pr.approve," + "gitea.pr.request_changes,gitea.pr.comment" + ), + "GITEA_SESSION_PROFILE_LOCK": "prgs-reviewer", +} + +REVIEWER_PROFILE = { + "profile_name": "prgs-reviewer", + "allowed_operations": [ + "gitea.read", + "gitea.pr.review", + "gitea.pr.approve", + "gitea.pr.request_changes", + "gitea.pr.comment", + ], + "forbidden_operations": [ + "gitea.pr.merge", + "gitea.pr.create", + "gitea.branch.push", + ], +} + + +def _reviewer_profile_patch(): + return patch.object(mcp_server, "get_profile", return_value=REVIEWER_PROFILE) + + +def _lock(mutations=None, correction=False): + return { + "task": "review_pr", + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": "Gitea-Tools", + "session_pid": os.getpid(), + "session_profile": "prgs-reviewer", + "session_profile_lock": "prgs-reviewer", + "profile_identity": "prgs-reviewer", + "final_review_decision_ready": False, + "ready_pr_number": None, + "ready_action": None, + "ready_expected_head_sha": None, + "ready_remote": None, + "ready_org": None, + "ready_repo": None, + "live_mutations": list(mutations or []), + "correction_authorized": correction, + "correction_reason": None, + } + + +APPROVED_586 = { + "pr_number": 586, + "action": "approve", + "review_id": 395, + "review_state": "approve", +} +APPROVED_OPEN = { + "pr_number": 700, + "action": "approve", + "review_id": 1, + "review_state": "approve", +} +RC_OPEN = { + "pr_number": 701, + "action": "request_changes", + "review_id": 2, + "review_state": "request_changes", +} + + +def _seed(mutations=None, correction=False): + import review_workflow_load + + review_workflow_load.record_review_workflow_load(mcp_server.PROJECT_ROOT) + mcp_server._save_review_decision_lock(_lock(mutations, correction)) + mcp_server.gitea_load_review_workflow() + + +def _merged_pr(pr_number=586, sha="6913ac9" + "0" * 33): + return { + "number": pr_number, + "state": "closed", + "merged": True, + "merged_at": "2026-07-09T18:20:03Z", + "merge_commit_sha": sha, + } + + +def _open_pr(pr_number=700): + return { + "number": pr_number, + "state": "open", + "merged": False, + "merged_at": None, + "merge_commit_sha": None, + } + + +# --------------------------------------------------------------------------- # +# Pure assessment +# --------------------------------------------------------------------------- # +class TestAssessStaleLock(unittest.TestCase): + def test_no_lock(self): + a = srdl.assess_stale_review_decision_lock(None) + self.assertFalse(a["cleanup_allowed"]) + self.assertFalse(a["is_moot"]) + + def test_no_terminal_mutations(self): + a = srdl.assess_stale_review_decision_lock(_lock([])) + self.assertFalse(a["cleanup_allowed"]) + + def test_open_pr_not_cleanable(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_OPEN]), pr_live=_open_pr(700) + ) + self.assertFalse(a["is_moot"]) + self.assertFalse(a["cleanup_allowed"]) + self.assertTrue(any("open PR" in r for r in a["reasons"])) + + def test_merged_pr_is_moot_and_cleanable(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), pr_live=_merged_pr(586) + ) + self.assertTrue(a["is_moot"]) + self.assertTrue(a["cleanup_allowed"]) + self.assertEqual(a["last_terminal_pr"], 586) + + def test_closed_unmerged_is_moot(self): + a = srdl.assess_stale_review_decision_lock( + _lock([RC_OPEN]), + pr_live={"number": 701, "state": "closed", "merged": False}, + ) + self.assertTrue(a["is_moot"]) + self.assertTrue(a["cleanup_allowed"]) + + def test_profile_mismatch_blocks(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), + pr_live=_merged_pr(586), + active_profile_identity="other-profile", + ) + self.assertFalse(a["cleanup_allowed"]) + self.assertFalse(a["profile_match"]) + + def test_lookup_error_fail_closed(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), + pr_lookup_error="timeout", + ) + self.assertFalse(a["cleanup_allowed"]) + self.assertTrue(any("timeout" in r for r in a["reasons"])) + + +# --------------------------------------------------------------------------- # +# Hard-stop still blocks active locks +# --------------------------------------------------------------------------- # +class TestActiveHardStopPreserved(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def test_open_approve_still_blocks_other_pr_mark_ready(self): + _seed([APPROVED_OPEN]) + reasons = mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready") + self.assertTrue(reasons) + self.assertIn("#332", reasons[0]) + self.assertIn("gitea_cleanup_stale_review_decision_lock", reasons[0]) + + def test_request_changes_still_blocks_everything(self): + _seed([RC_OPEN]) + for op in ("merge", "mark_ready", "review"): + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(592, op), + msg=op, + ) + + +# --------------------------------------------------------------------------- # +# MCP cleanup tool +# --------------------------------------------------------------------------- # +class TestCleanupTool(unittest.TestCase): + def setUp(self): + self._env = patch.dict(os.environ, REVIEWER_ENV, clear=False) + self._env.start() + + def tearDown(self): + self._env.stop() + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def test_read_only_reports_moot_without_clearing(self): + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=False, remote="prgs", + org="Scaled-Tech-Consulting", repo="Gitea-Tools", + ) + self.assertTrue(r["success"]) + self.assertTrue(r["is_moot"]) + self.assertTrue(r["cleanup_allowed"]) + self.assertFalse(r["cleanup_performed"]) + self.assertIsNotNone(mcp_server._load_review_decision_lock()) + + def test_apply_clears_moot_lock(self): + _seed([APPROVED_586]) + posts = [] + + def _api(method, url, auth, payload=None): + if method == "GET" and "/pulls/586" in url: + return _merged_pr() + if method == "POST" and "/comments" in url: + posts.append(payload) + return {"id": 9999} + return {} + + with patch.object(mcp_server, "api_request", side_effect=_api), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]), \ + patch.object( + mcp_server, "_audited", + side_effect=lambda *a, **k: __import__( + "contextlib" + ).nullcontext(), + ): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=586, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(r["cleanup_performed"], r) + self.assertTrue(r["audit"]["applied"]) + self.assertIsNone(mcp_server._load_review_decision_lock()) + self.assertEqual(r.get("audit_comment_id"), 9999) + self.assertTrue(posts) + + def test_apply_refuses_open_pr(self): + _seed([APPROVED_OPEN]) + with patch.object(mcp_server, "api_request", return_value=_open_pr(700)), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, remote="prgs", + org="Scaled-Tech-Consulting", repo="Gitea-Tools", + ) + self.assertFalse(r["cleanup_performed"]) + self.assertFalse(r["cleanup_allowed"]) + self.assertIsNotNone(mcp_server._load_review_decision_lock()) + + def test_expected_terminal_pr_pin(self): + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=999, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertFalse(r["cleanup_performed"]) + self.assertTrue(any("expected_terminal_pr" in x for x in r["reasons"])) + + def test_after_moot_cleanup_other_pr_mark_ready_unblocked(self): + """PR #592-style: after clearing merged #586 lock, new mark_ready works.""" + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]), \ + patch.object( + mcp_server, "_audited", + side_effect=lambda *a, **k: __import__( + "contextlib" + ).nullcontext(), + ): + cleaned = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=586, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + post_audit_comment=False, + ) + self.assertTrue(cleaned["cleanup_performed"], cleaned) + + # Hard-stop gone; re-init clean lock like a fresh review_pr resolve. + mcp_server.init_review_decision_lock(remote="prgs", task="review_pr") + mcp_server.gitea_load_review_workflow() + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready"), + [], + ) + + no_lease = {"block": False, "reasons": [], "mutation_allowed": True} + with patch.object(mcp_server, "_list_pr_lease_comments", return_value=[]), \ + patch.object( + mcp_server, "_pr_work_lease_reviewer_block", return_value=no_lease + ), \ + patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={ + "eligible": True, + "reasons": [], + "authenticated_user": "sysadmin", + "pr_author": "jcwalker3", + "self_author": False, + }, + ), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ): + marked = gitea_mark_final_review_decision( + pr_number=592, + action="approve", + expected_head_sha=HEAD, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(marked.get("marked_ready"), marked) + + +if __name__ == "__main__": + unittest.main()