diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 29248f3..9cfc623 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -3338,6 +3338,8 @@ def init_review_decision_lock(remote: str | None, task: str | None, force: bool "live_mutations": [], "correction_authorized": False, "correction_reason": None, + "correction_pr_number": None, + "correction_head_sha": None, }) @@ -3422,21 +3424,30 @@ def check_review_decision_gate( if stale_review_decision_lock.prior_live_mutations_block_boundary( lock, pr_number=pr_number, expected_head_sha=ready_head ): - if prior and not lock.get("correction_authorized"): + scoped = stale_review_decision_lock.correction_applies( + lock, pr_number=pr_number, expected_head_sha=ready_head + ) + if prior and not scoped: reasons.append( "live review mutation already recorded for this PR head in this " "run; only one live review mutation is allowed per head unless " - "gitea_authorize_review_correction was invoked (fail closed, #620)" + "gitea_authorize_review_correction was invoked for this same " + "PR/head (fail closed, #620/#693)" ) elif ( action in _TERMINAL_REVIEW_ACTIONS - and any(m.get("action") in _TERMINAL_REVIEW_ACTIONS for m in prior) - and not lock.get("correction_authorized") + and any( + isinstance(m, dict) + and m.get("action") in _TERMINAL_REVIEW_ACTIONS + and m.get("pr_number") == pr_number + for m in prior + ) + and not scoped ): reasons.append( "terminal review decision already submitted for this PR head in " "this run; blocked unless an operator-approved correction was " - "authorized (fail closed, #620)" + "authorized for this same PR/head (fail closed, #620/#693)" ) return reasons @@ -3461,6 +3472,8 @@ def record_live_review_mutation(pr_number: int, action: str, review_id: int | No if lock.get("correction_authorized"): lock["correction_authorized"] = False lock["correction_reason"] = None + lock["correction_pr_number"] = None + lock["correction_head_sha"] = None _save_review_decision_lock(lock) @@ -3504,11 +3517,13 @@ def terminal_review_hard_stop_reasons( and last.get("pr_number") == pr_number ): return [] - if operation in ("mark_ready", "review", "resume") and lock.get( - "correction_authorized" + if operation in ("mark_ready", "review", "resume") and ( + stale_review_decision_lock.correction_applies( + lock, pr_number=pr_number, expected_head_sha=expected_head_sha + ) ): return [] - # #620: same PR, different reviewed head → fresh decision boundary. + # #620 same-PR new head + #693 cross-PR isolation. if stale_review_decision_lock.terminal_boundary_allows_fresh_decision( lock, pr_number=pr_number, @@ -3530,16 +3545,20 @@ def terminal_review_hard_stop_reasons( else "" ) recovery = ( - "if that PR is already merged/closed, use " - "gitea_cleanup_stale_review_decision_lock (apply=true) after live-state " - "proof (#594); if the PR is still open but the head moved, mark/submit " - "with the new expected_head_sha (#620); never delete session-state " - "files by hand" + "exact_next_action: if that PR is already merged/closed, use " + "gitea_cleanup_stale_review_decision_lock(apply=true) after live-state " + "proof (#594); if the same PR head moved, mark/submit with the new " + "expected_head_sha (#620); if the target is a different PR, " + "gitea_diagnose_review_decision_lock then mark_final on the target " + "(cross-PR isolation, #693); if same-head verdict was mistaken, " + "gitea_authorize_review_correction with matching target_pr_number " + "(not a generic unlock); never delete session-state files by hand; " + "if still blocked create/update a durable Gitea issue and stop" ) return [ "terminal review mutation already consumed in this run " f"({last.get('action')} on PR #{last.get('pr_number')}{head_note}); " - f"{guidance} (fail closed, #332); {recovery}" + f"{guidance} (fail closed, #332/#693); {recovery}" ] @@ -4121,10 +4140,33 @@ def gitea_mark_final_review_decision( "reasons": [ "review decision lock missing; resolve review_pr capability first" ], + "exact_next_action": ( + "gitea_resolve_task_capability(task='review_pr') then retry " + "mark_final" + ), + "durable_issue_handoff": { + "required": True, + "instruction": ( + "If the lock cannot be seeded, create/update a durable " + "Gitea issue and stop (#693 AC8)" + ), + }, } session_reasons = _review_decision_session_reasons(lock) if session_reasons: - return {"marked_ready": False, "reasons": session_reasons} + classification = stale_review_decision_lock.classify_review_decision_lock( + lock, + target_pr_number=pr_number, + target_head_sha=expected_head_sha, + session_blockers=session_reasons, + ) + fail = stale_review_decision_lock.build_precise_mark_final_failure( + classification + ) + return { + "marked_ready": False, + **fail, + } if remote != lock.get("remote"): return { "marked_ready": False, @@ -4132,6 +4174,7 @@ def gitea_mark_final_review_decision( f"requested remote '{remote}' does not match locked remote " f"'{lock.get('remote')}' (fail closed)" ], + "exact_next_action": "use the remote bound on the decision lock", } try: _, resolved_org, resolved_repo = _resolve(remote, None, org, repo) @@ -4166,7 +4209,26 @@ def gitea_mark_final_review_decision( pr_number, "mark_ready", expected_head_sha=expected_head_sha ) if hard_stop: - return {"marked_ready": False, "reasons": hard_stop} + classification = stale_review_decision_lock.classify_review_decision_lock( + lock, + target_pr_number=pr_number, + target_head_sha=expected_head_sha, + active_profile_identity=_decision_lock_binding().get( + "profile_identity" + ), + ) + fail = stale_review_decision_lock.build_precise_mark_final_failure( + classification + ) + # Prefer classification next_action when more precise; keep hard_stop + # text as primary reasons for operators. + return { + "marked_ready": False, + "reasons": hard_stop + list(fail.get("reasons") or []), + "classification": fail.get("classification"), + "exact_next_action": fail.get("exact_next_action"), + "durable_issue_handoff": fail.get("durable_issue_handoff"), + } if action not in _REVIEW_ACTIONS: return { "marked_ready": False, @@ -4182,17 +4244,63 @@ def gitea_mark_final_review_decision( "expected_head_sha required before marking final review " "decision (fail closed, #399)" ], + "exact_next_action": "retry mark_final with expected_head_sha pin", + } + # Safe idempotency: already ready for same PR/action/head (#693 AC4). + ready_head = stale_review_decision_lock.normalize_head_sha( + lock.get("ready_expected_head_sha") + ) + target_head = stale_review_decision_lock.normalize_head_sha(expected_head_sha) + if ( + lock.get("final_review_decision_ready") + and lock.get("ready_pr_number") == pr_number + and lock.get("ready_action") == action + and ready_head + and target_head + and stale_review_decision_lock.heads_equal(ready_head, target_head) + and lock.get("ready_remote") == remote + and lock.get("ready_org") == org + and lock.get("ready_repo") == repo + ): + return { + "marked_ready": True, + "idempotent": True, + "pr_number": pr_number, + "action": action, + "expected_head_sha": expected_head_sha, + "remote": remote, + "org": org, + "repo": repo, + "final_review_decision_ready": True, + "head_scoped": True, + "reasons": [ + "final review decision already marked ready for this PR/action/" + "head (idempotent, #693)" + ], } if stale_review_decision_lock.prior_live_mutations_block_boundary( lock, pr_number=pr_number, expected_head_sha=expected_head_sha ): + classification = stale_review_decision_lock.classify_review_decision_lock( + lock, + target_pr_number=pr_number, + target_head_sha=expected_head_sha, + ) + fail = stale_review_decision_lock.build_precise_mark_final_failure( + classification + ) return { "marked_ready": False, "reasons": [ "cannot mark final decision after a live review mutation was " "already recorded for this PR head unless an operator-approved " - "correction was authorized (fail closed, #620)" - ], + "correction was authorized for this same PR/head " + "(fail closed, #620/#693)" + ] + + list(fail.get("reasons") or []), + "classification": fail.get("classification"), + "exact_next_action": fail.get("exact_next_action"), + "durable_issue_handoff": fail.get("durable_issue_handoff"), } elig = gitea_check_pr_eligibility( pr_number=pr_number, @@ -4276,8 +4384,19 @@ def gitea_authorize_review_correction( prior_review_state: str, reason: str, operator_authorized: bool = False, + target_pr_number: int | None = None, + expected_head_sha: str | None = None, + remote: str = "dadeschools", + org: str | None = None, + repo: str | None = None, + post_audit_comment: bool = True, ) -> dict: - """Authorize one operator-approved correction after a mistaken live review.""" + """Authorize one operator-approved correction after a mistaken live review. + + #693: correction is scoped to the named prior review's **PR and head**. + It cannot unlock a different PR (no generic decision-lock bypass). + Successful authorization posts a thread-visible audit on the target PR. + """ reason = (reason or "").strip() prior_review_state = (prior_review_state or "").strip().lower() lock = _load_review_decision_lock() @@ -4303,7 +4422,20 @@ def gitea_authorize_review_correction( last_mutation = prior[-1] last_review_id = last_mutation.get("review_id") last_review_state = last_mutation.get("action") + last_pr = last_mutation.get("pr_number") + last_head = stale_review_decision_lock.mutation_head_sha(last_mutation, lock) reasons = [] + if target_pr_number is None: + reasons.append( + "target_pr_number is required so correction cannot become a " + "generic unlock (fail closed, #693)" + ) + elif last_pr is not None and int(target_pr_number) != int(last_pr): + reasons.append( + f"target_pr_number #{target_pr_number} does not match last " + f"recorded mutation PR #{last_pr}; correction cannot unlock a " + f"different PR (fail closed, #693)" + ) if last_review_id is not None and last_review_id != prior_review_id: reasons.append( f"prior review ID '{prior_review_id}' does not match last " @@ -4314,14 +4446,202 @@ def gitea_authorize_review_correction( f"prior review state '{prior_review_state}' does not match last " f"recorded review state '{last_review_state}' (fail closed)" ) + want_head = stale_review_decision_lock.normalize_head_sha(expected_head_sha) + if want_head and last_head and not stale_review_decision_lock.heads_equal( + want_head, last_head + ): + reasons.append( + f"expected_head_sha does not match last mutation head " + f"{last_head[:12]}… (fail closed, #693)" + ) if not reason: reasons.append("correction reason is required") + try: + actor = None + h, o, r = _resolve(remote, None, org, repo) + try: + actor = _authenticated_username(h) + except Exception: + actor = None + except Exception as exc: # noqa: BLE001 + return { + "authorized": False, + "reasons": [f"could not resolve remote for audit: {_redact(str(exc))}"], + } + profile = get_profile() + profile_name = (profile.get("profile_name") or "").strip() or None + audit = stale_review_decision_lock.build_correction_audit_record( + prior_review_id=prior_review_id, + prior_review_state=prior_review_state, + target_pr_number=target_pr_number if target_pr_number is not None else last_pr, + target_head_sha=want_head or last_head, + reason=reason, + actor_username=actor, + profile_name=profile_name, + authorized=False, + ) if reasons: - return {"authorized": False, "reasons": reasons} + return { + "authorized": False, + "reasons": reasons, + "audit": audit, + "exact_next_action": ( + "use same-PR correction only, or for a different PR use " + "gitea_diagnose_review_decision_lock / cross-PR isolation " + "(#693) or moot cleanup (#594)" + ), + } + scope_pr = int(target_pr_number) if target_pr_number is not None else int(last_pr) + scope_head = want_head or last_head lock["correction_authorized"] = True lock["correction_reason"] = reason + lock["correction_pr_number"] = scope_pr + lock["correction_head_sha"] = scope_head _save_review_decision_lock(lock) - return {"authorized": True, "correction_reason": reason, "reasons": []} + audit = stale_review_decision_lock.build_correction_audit_record( + prior_review_id=prior_review_id, + prior_review_state=prior_review_state, + target_pr_number=scope_pr, + target_head_sha=scope_head, + reason=reason, + actor_username=actor, + profile_name=profile_name, + authorized=True, + ) + report = { + "authorized": True, + "correction_reason": reason, + "correction_pr_number": scope_pr, + "correction_head_sha": scope_head, + "audit": audit, + "audit_comment_id": None, + "reasons": [], + "scope": "same_pr_head_only", + } + if post_audit_comment and scope_pr is not None: + comment_block = _profile_operation_gate("gitea.pr.comment") + if comment_block: + report["audit_comment_skipped"] = comment_block + else: + try: + auth = _auth(h) + body = stale_review_decision_lock.format_correction_audit_comment( + audit + ) + with _audited( + "comment_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=scope_pr, + request_metadata={"source": "authorize_review_correction"}, + ): + posted = api_request( + "POST", + f"{repo_api_url(h, o, r)}/issues/{scope_pr}/comments", + 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_diagnose_review_decision_lock( + target_pr_number: int, + expected_head_sha: str | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Read-only: classify durable review-decision lock state for a target PR (#693). + + Returns a deterministic classification, owner/evidence, exact next_action, + and whether mark_final / cleanup / scoped correction are appropriate. + Never mutates the lock and never authorizes correction. + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "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() + lock = _load_review_decision_lock() + session_blockers = _review_decision_session_reasons(lock) if lock else [] + 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 + pr_lookup_error = _redact(str(exc)) + classification = stale_review_decision_lock.classify_review_decision_lock( + lock, + target_pr_number=target_pr_number, + target_head_sha=expected_head_sha, + pr_live=pr_live, + pr_lookup_error=pr_lookup_error, + active_profile_identity=binding.get("profile_identity"), + session_blockers=session_blockers, + ) + recovery = stale_review_decision_lock.assess_open_pr_decision_lock_recovery( + lock, + target_pr_number=target_pr_number, + target_head_sha=expected_head_sha, + last_terminal_pr_live=pr_live, + pr_lookup_error=pr_lookup_error, + active_profile_identity=binding.get("profile_identity"), + session_blockers=session_blockers, + ) + try: + actor = _authenticated_username(h) + except Exception: + actor = None + return { + "success": True, + "remote": remote, + "org": o, + "repo": r, + "authenticated_username": actor, + "active_profile_identity": binding.get("profile_identity"), + "classification": classification.get("classification"), + "mark_final_allowed": classification.get("mark_final_allowed"), + "cleanup_allowed": classification.get("cleanup_allowed"), + "correction_eligible": classification.get("correction_eligible"), + "recovery_allowed": recovery.get("recovery_allowed"), + "recovery_mode": recovery.get("recovery_mode"), + "exact_next_action": classification.get("next_action"), + "owner_profile_identity": classification.get("owner_profile_identity"), + "last_terminal_pr": classification.get("last_terminal_pr"), + "last_terminal_action": classification.get("last_terminal_action"), + "locked_head_sha": classification.get("locked_head_sha"), + "target_pr_number": target_pr_number, + "target_head_sha": stale_review_decision_lock.normalize_head_sha( + expected_head_sha + ), + "lock_summary": classification.get("lock_summary"), + "evidence": classification.get("evidence"), + "reasons": list(classification.get("reasons") or []), + "manual_file_delete_forbidden": True, + "correction_as_generic_unlock_forbidden": True, + "durable_issue_handoff": stale_review_decision_lock.build_precise_mark_final_failure( + classification + ).get("durable_issue_handoff"), + } @mcp.tool() diff --git a/stale_review_decision_lock.py b/stale_review_decision_lock.py index 5f47486..0f54d3c 100644 --- a/stale_review_decision_lock.py +++ b/stale_review_decision_lock.py @@ -1,4 +1,4 @@ -"""Stale / moot #332 review-decision lock detection and cleanup policy (#594/#620). +"""Stale / moot #332 review-decision lock detection and cleanup policy (#594/#620/#693). #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 @@ -12,6 +12,11 @@ on head B of the **same open PR**. Same-head duplicates remain fail-closed. Open-PR lock *cleanup* remains forbidden unless the PR is truly merged/closed (#594); new-head re-review is allowed without deleting the durable lock. +#693 scopes the terminal boundary by **PR number**: a terminal on open PR A +must not block a fresh formal decision on open PR B on the same durable +profile lock (cross-PR isolation). Correction authorization is scoped to the +named prior review's PR/head and cannot unlock a different PR. + 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. @@ -80,17 +85,45 @@ def last_terminal_mutation(lock: dict | None) -> dict | None: return terminals[-1] if terminals else None +def correction_applies( + lock: dict | None, + *, + pr_number: int | None = None, + expected_head_sha: str | None = None, +) -> bool: + """True when operator correction is active and scoped to the target (#693). + + Global ``correction_authorized`` alone is not enough: correction must name + the same PR (and head when recorded) as the decision being re-opened. + Missing scope fields on legacy locks fail closed (do not apply). + """ + if not lock or not lock.get("correction_authorized"): + return False + corr_pr = lock.get("correction_pr_number") + if corr_pr is None: + # Legacy unscoped correction — refuse as generic unlock (#693). + return False + if pr_number is not None and int(corr_pr) != int(pr_number): + return False + corr_head = normalize_head_sha(lock.get("correction_head_sha")) + target = normalize_head_sha(expected_head_sha) + if corr_head and target and not heads_equal(corr_head, target): + return False + return True + + def prior_live_mutations_block_boundary( lock: dict | None, *, pr_number: int, expected_head_sha: str | None, ) -> bool: - """True when prior live mutations block a new decision for PR+head (#620). + """True when prior live mutations block a new decision for PR+head (#620/#693). Historical terminals on a **different head of the same PR** do not block. - Any prior mutation on another PR, the same head, or without a comparable - head SHA fails closed (blocks). + Terminals on a **different PR** do not block (#693 cross-PR isolation). + Same PR + same head (or same PR without a comparable head SHA) blocks + unless a scoped correction applies. Head resolution uses ``mutation_head_sha(m, lock)`` so pre-#620 ledgers that only stored ``ready_expected_head_sha`` still compare correctly @@ -98,7 +131,9 @@ def prior_live_mutations_block_boundary( """ if not lock: return False - if lock.get("correction_authorized"): + if correction_applies( + lock, pr_number=pr_number, expected_head_sha=expected_head_sha + ): return False prior = list(lock.get("live_mutations") or []) if not prior: @@ -108,10 +143,14 @@ def prior_live_mutations_block_boundary( if not isinstance(m, dict): return True m_pr = m.get("pr_number") + if m_pr is None: + return True # fail closed — unscoped mutation + if int(m_pr) != int(pr_number): + # #693: foreign-PR history on the shared durable lock does not block. + continue m_head = mutation_head_sha(m, lock) if ( - m_pr == pr_number - and target + target and m_head and not heads_equal(m_head, target) ): @@ -128,23 +167,34 @@ def terminal_boundary_allows_fresh_decision( expected_head_sha: str | None, operation: str, ) -> bool: - """Whether #332 hard-stop should yield for a new PR+head boundary (#620). + """Whether #332 hard-stop should yield for a new PR+head boundary (#620/#693). - Only for ``mark_ready`` / ``review`` / ``resume`` on the **same PR** when - the requested head differs from the last terminal's head. Merge is never - reopened by head change (approved head merge path is separate). + For ``mark_ready`` / ``review`` / ``resume``: + + * **same PR, different head** — allowed (#620) + * **different PR than last terminal** — allowed (#693 cross-PR isolation) + * **same PR, same head** — not allowed (unless scoped correction) + + Merge is never reopened by head change or cross-PR isolation (approved + head merge path is separate). """ if operation not in ("mark_ready", "review", "resume"): return False if not lock: return False - if lock.get("correction_authorized"): + if correction_applies( + lock, pr_number=pr_number, expected_head_sha=expected_head_sha + ): return True last = last_terminal_mutation(lock) if last is None: return True - if last.get("pr_number") != pr_number: + last_pr = last.get("pr_number") + if last_pr is None: return False + if int(last_pr) != int(pr_number): + # #693: last terminal belongs to another PR — do not hard-stop this PR. + return True locked_head = mutation_head_sha(last, lock) target = normalize_head_sha(expected_head_sha) if not locked_head or not target: @@ -438,3 +488,397 @@ def format_cleanup_audit_comment(audit: dict[str, Any]) -> str: "This path only clears a lock when the referenced PR is merged/closed.", ] return "\n".join(lines) + + +# --- #693 classification / recovery / correction audit ----------------------- + +# Deterministic classification labels for diagnostics and recovery routing. +CLASS_NO_LOCK = "no_lock" +CLASS_READY_FOR_TARGET = "ready_for_target" +CLASS_IN_PROGRESS_SAME_HEAD = "in_progress_same_head" +CLASS_TERMINAL_ACTIVE_SAME_HEAD = "terminal_active_same_head" +CLASS_STALE_SUPERSEDED_HEAD = "stale_superseded_head" +CLASS_FOREIGN_PR_TERMINAL = "foreign_pr_terminal" +CLASS_MOOT_MERGED_CLOSED = "moot_merged_closed" +CLASS_CORRECTION_ACTIVE = "correction_active" +CLASS_SESSION_OR_PROFILE_MISMATCH = "session_or_profile_mismatch" +CLASS_AMBIGUOUS = "ambiguous" + + +def classify_review_decision_lock( + lock: dict | None, + *, + target_pr_number: int | None = None, + target_head_sha: str | None = None, + pr_live: dict | None = None, + pr_lookup_error: str | None = None, + active_profile_identity: str | None = None, + session_blockers: list[str] | None = None, +) -> dict[str, Any]: + """Deterministic review-decision-lock classification (#693). + + Returns classification, exact next_action, owner/evidence fields, and + whether mark_final / cleanup / correction are appropriate. + """ + summary = lock_summary(lock) + target_head = normalize_head_sha(target_head_sha) + result: dict[str, Any] = { + "classification": CLASS_NO_LOCK, + "has_lock": lock is not None, + "lock_summary": summary, + "target_pr_number": target_pr_number, + "target_head_sha": target_head, + "last_terminal_pr": None, + "last_terminal_action": None, + "locked_head_sha": None, + "owner_profile_identity": (summary or {}).get("profile_identity"), + "session_profile": (summary or {}).get("session_profile"), + "updated_at": (summary or {}).get("updated_at"), + "correction_authorized": bool((lock or {}).get("correction_authorized")), + "correction_pr_number": (lock or {}).get("correction_pr_number"), + "correction_head_sha": normalize_head_sha( + (lock or {}).get("correction_head_sha") + ), + "mark_final_allowed": True, + "cleanup_allowed": False, + "correction_eligible": False, + "next_action": "gitea_resolve_task_capability(task='review_pr') then mark_final", + "reasons": [], + "evidence": {}, + } + + if lock is None: + result["reasons"].append("no review decision lock present") + return result + + session_blockers = list(session_blockers or []) + if session_blockers: + result["classification"] = CLASS_SESSION_OR_PROFILE_MISMATCH + result["mark_final_allowed"] = False + result["next_action"] = ( + "reconnect matching prgs-reviewer session or wait for lock TTL; " + "do not use gitea_authorize_review_correction as unlock; " + "do not delete session-state files" + ) + result["reasons"].extend(session_blockers) + return result + + 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["classification"] = CLASS_SESSION_OR_PROFILE_MISMATCH + result["mark_final_allowed"] = False + result["next_action"] = ( + f"switch to profile '{stored}' or wait for moot cleanup; " + "do not use correction authorization as a generic unlock" + ) + result["reasons"].append( + f"lock profile '{stored}' does not match active '{active}'" + ) + return result + + last = last_terminal_mutation(lock) + if last is None: + ready_pr = lock.get("ready_pr_number") + ready_head = normalize_head_sha(lock.get("ready_expected_head_sha")) + if ( + target_pr_number is not None + and ready_pr == target_pr_number + and ready_head + and target_head + and heads_equal(ready_head, target_head) + and lock.get("final_review_decision_ready") + ): + result["classification"] = CLASS_IN_PROGRESS_SAME_HEAD + result["mark_final_allowed"] = True # idempotent re-mark + result["next_action"] = ( + "gitea_submit_pr_review with final_review_decision_ready=true " + "for the ready PR/head" + ) + result["reasons"].append( + "final decision already marked ready for this PR/head (idempotent)" + ) + return result + result["classification"] = CLASS_READY_FOR_TARGET + result["next_action"] = "gitea_mark_final_review_decision then submit" + result["reasons"].append("no terminal live mutations; mark_final allowed") + return result + + last_pr = last.get("pr_number") + last_action = last.get("action") + locked_head = mutation_head_sha(last, lock) + result["last_terminal_pr"] = last_pr + result["last_terminal_action"] = last_action + result["locked_head_sha"] = locked_head + result["evidence"] = { + "last_review_id": last.get("review_id"), + "live_mutations_count": len(lock.get("live_mutations") or []), + } + + if correction_applies( + lock, pr_number=target_pr_number, expected_head_sha=target_head + ): + result["classification"] = CLASS_CORRECTION_ACTIVE + result["mark_final_allowed"] = True + result["next_action"] = ( + "gitea_mark_final_review_decision for the correction-scoped PR/head, " + "then submit once" + ) + result["reasons"].append( + f"scoped correction active for PR #{lock.get('correction_pr_number')}" + ) + return result + + # Live state of last terminal PR when provided. + live = None + if pr_lookup_error: + result["classification"] = CLASS_AMBIGUOUS + result["mark_final_allowed"] = False + result["next_action"] = ( + "retry diagnosis after live PR state is available; " + "fail closed under ambiguous evidence" + ) + result["reasons"].append(f"live PR lookup failed: {pr_lookup_error}") + return result + if pr_live is not None: + live = classify_pr_live_state(pr_live) + result["evidence"]["last_terminal_pr_state"] = live.get("pr_state") + result["evidence"]["last_terminal_pr_merged"] = live.get("pr_merged") + if live.get("pr_merged_or_closed"): + result["classification"] = CLASS_MOOT_MERGED_CLOSED + result["cleanup_allowed"] = True + result["mark_final_allowed"] = target_pr_number is not None and ( + int(last_pr) != int(target_pr_number) + if last_pr is not None and target_pr_number is not None + else True + ) + result["next_action"] = ( + "gitea_cleanup_stale_review_decision_lock(apply=true, " + f"expected_terminal_pr={last_pr}) then mark_final on the target PR" + ) + result["reasons"].append( + f"terminal on PR #{last_pr} is moot (merged/closed); cleanup allowed" + ) + return result + + if target_pr_number is None: + result["classification"] = CLASS_AMBIGUOUS + result["mark_final_allowed"] = False + result["next_action"] = "re-run diagnosis with target_pr_number" + result["reasons"].append("target_pr_number required for open-PR classification") + return result + + if last_pr is not None and int(last_pr) != int(target_pr_number): + # Cross-PR isolation: foreign terminal is history, not a block. + result["classification"] = CLASS_FOREIGN_PR_TERMINAL + result["mark_final_allowed"] = True + result["correction_eligible"] = False # must not use correction to "unlock" + result["next_action"] = ( + f"gitea_mark_final_review_decision(pr_number={target_pr_number}, ...) " + f"— foreign terminal on PR #{last_pr} does not block (#693); " + "do not call gitea_authorize_review_correction for cross-PR unlock" + ) + result["reasons"].append( + f"last terminal is {last_action} on foreign open/closed PR #{last_pr}; " + f"target PR #{target_pr_number} is isolated (#693)" + ) + return result + + # Same PR as target. + if ( + locked_head + and target_head + and not heads_equal(locked_head, target_head) + ): + result["classification"] = CLASS_STALE_SUPERSEDED_HEAD + result["mark_final_allowed"] = True + result["next_action"] = ( + f"gitea_mark_final_review_decision with expected_head_sha=" + f"{target_head} (#620 same-PR new head)" + ) + result["reasons"].append( + f"terminal {last_action} on head {locked_head[:12]}…; target head " + f"{target_head[:12]}… — fresh formal review allowed without cleanup" + ) + return result + + # Same PR + same head (or missing head comparison) — active terminal. + result["classification"] = CLASS_TERMINAL_ACTIVE_SAME_HEAD + result["mark_final_allowed"] = False + result["correction_eligible"] = True + result["next_action"] = ( + "if the prior verdict was mistaken: gitea_authorize_review_correction " + f"with prior_review_id={last.get('review_id')}, " + f"prior_review_state={last_action}, target_pr_number={target_pr_number}, " + "operator_authorized=true, then re-mark once; " + "if the prior verdict is correct: stop and hand off (do not duplicate)" + ) + result["reasons"].append( + f"terminal {last_action} already recorded for PR #{target_pr_number} " + f"at this head; same-head duplicate blocked (#332/#620)" + ) + return result + + +def build_precise_mark_final_failure( + classification: dict[str, Any], +) -> dict[str, Any]: + """One precise recovery instruction + durable issue handoff payload (#693 AC4/8).""" + cls = classification.get("classification") + next_action = classification.get("next_action") or ( + "stop and create a durable Gitea issue with lock evidence" + ) + reasons = list(classification.get("reasons") or []) + reasons.append(f"exact_next_action: {next_action}") + handoff = { + "required": cls + in ( + CLASS_AMBIGUOUS, + CLASS_SESSION_OR_PROFILE_MISMATCH, + CLASS_TERMINAL_ACTIVE_SAME_HEAD, + ) + and not classification.get("mark_final_allowed"), + "title_hint": ( + "Review-decision-lock recovery failed during formal review" + ), + "classification": cls, + "exact_next_action": next_action, + "owner_profile_identity": classification.get("owner_profile_identity"), + "last_terminal_pr": classification.get("last_terminal_pr"), + "last_terminal_action": classification.get("last_terminal_action"), + "locked_head_sha": classification.get("locked_head_sha"), + "target_pr_number": classification.get("target_pr_number"), + "target_head_sha": classification.get("target_head_sha"), + "evidence": classification.get("evidence") or {}, + "instruction": ( + "If recovery cannot complete with the exact_next_action above, " + "create or update a durable Gitea issue with this payload and stop; " + "do not delete session-state files; do not misuse correction as unlock." + ), + } + return { + "reasons": reasons, + "classification": cls, + "exact_next_action": next_action, + "durable_issue_handoff": handoff, + } + + +def build_correction_audit_record( + *, + prior_review_id: int | None, + prior_review_state: str | None, + target_pr_number: int | None, + target_head_sha: str | None, + reason: str | None, + actor_username: str | None, + profile_name: str | None, + authorized: bool, +) -> dict[str, Any]: + """Structured durable audit for review-correction authorization (#693).""" + return { + "event": "review_correction_authorization", + "issue_ref": "#693", + "authorized": bool(authorized), + "timestamp": datetime.now(timezone.utc).isoformat(), + "actor_username": actor_username, + "profile_name": profile_name, + "prior_review_id": prior_review_id, + "prior_review_state": prior_review_state, + "target_pr_number": target_pr_number, + "target_head_sha": normalize_head_sha(target_head_sha), + "reason": reason, + "scope": "same_pr_head_only", + } + + +def format_correction_audit_comment(audit: dict[str, Any]) -> str: + """Markdown body for a thread-visible correction authorization audit.""" + status = "AUTHORIZED" if audit.get("authorized") else "DENIED" + lines = [ + "## Review correction authorization audit (#693)", + "", + f"Status: **{status}**", + "", + f"- actor: `{audit.get('actor_username')}`", + f"- profile: `{audit.get('profile_name')}`", + f"- timestamp: `{audit.get('timestamp')}`", + f"- prior_review_id: `{audit.get('prior_review_id')}`", + f"- prior_review_state: `{audit.get('prior_review_state')}`", + f"- target_pr: `#{audit.get('target_pr_number')}`", + f"- target_head_sha: `{audit.get('target_head_sha')}`", + f"- reason: {audit.get('reason')}", + f"- scope: `{audit.get('scope')}` (cannot unlock a different PR)", + "", + "This is **not** a generic decision-lock unlock. Cross-PR recovery " + "uses isolation (#693) or moot cleanup (#594), never correction.", + ] + return "\n".join(lines) + + +def assess_open_pr_decision_lock_recovery( + lock: dict | None, + *, + target_pr_number: int, + target_head_sha: str | None, + last_terminal_pr_live: dict | None = None, + pr_lookup_error: str | None = None, + active_profile_identity: str | None = None, + session_blockers: list[str] | None = None, + controller_recovery_authorized: bool = False, +) -> dict[str, Any]: + """Assess guarded recovery for decision locks affecting an open target PR (#693). + + Recovery here means *workflow recovery routing*, not necessarily lock wipe. + Foreign-PR terminals are recoverable by isolation (mark_final allowed). + Moot terminals use #594 cleanup. Same-head active terminals refuse wipe. + """ + classification = classify_review_decision_lock( + lock, + target_pr_number=target_pr_number, + target_head_sha=target_head_sha, + pr_live=last_terminal_pr_live, + pr_lookup_error=pr_lookup_error, + active_profile_identity=active_profile_identity, + session_blockers=session_blockers, + ) + cls = classification["classification"] + recovery_allowed = False + recovery_mode = None + if cls == CLASS_FOREIGN_PR_TERMINAL: + recovery_allowed = True + recovery_mode = "cross_pr_isolation" + elif cls == CLASS_STALE_SUPERSEDED_HEAD: + recovery_allowed = True + recovery_mode = "same_pr_new_head" + elif cls == CLASS_MOOT_MERGED_CLOSED: + recovery_allowed = True + recovery_mode = "moot_cleanup" + elif cls == CLASS_READY_FOR_TARGET: + recovery_allowed = True + recovery_mode = "none_required" + elif cls == CLASS_CORRECTION_ACTIVE: + recovery_allowed = True + recovery_mode = "scoped_correction" + elif cls == CLASS_TERMINAL_ACTIVE_SAME_HEAD: + recovery_allowed = False + recovery_mode = "correction_only_if_mistake" + else: + recovery_allowed = False + recovery_mode = "fail_closed" + + if recovery_mode == "moot_cleanup" and not controller_recovery_authorized: + # Cleanup still needs apply + capability; assess reports path. + pass + + return { + **classification, + "recovery_allowed": recovery_allowed, + "recovery_mode": recovery_mode, + "controller_recovery_authorized": bool(controller_recovery_authorized), + "manual_file_delete_forbidden": True, + "correction_as_generic_unlock_forbidden": True, + } diff --git a/task_capability_map.py b/task_capability_map.py index b577052..17f8a4b 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -117,6 +117,23 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.pr.review", "role": "reviewer", }, + # #693: read-only classification of durable decision locks (incl. open PRs). + "diagnose_review_decision_lock": { + "permission": "gitea.read", + "role": "reviewer", + }, + "gitea_diagnose_review_decision_lock": { + "permission": "gitea.read", + "role": "reviewer", + }, + "authorize_review_correction": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, + "gitea_authorize_review_correction": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, "delete_branch": { "permission": "gitea.branch.delete", "role": "author", diff --git a/tests/test_head_scoped_review_decision_lock.py b/tests/test_head_scoped_review_decision_lock.py index c9244c7..13fdbfc 100644 --- a/tests/test_head_scoped_review_decision_lock.py +++ b/tests/test_head_scoped_review_decision_lock.py @@ -24,6 +24,10 @@ HEAD_C = "c" * 40 def _lock(mutations=None, correction=False, ready_pr=None, ready_head=None, ready_action=None): + mutations = list(mutations or []) + corr_pr = None + if correction and mutations: + corr_pr = mutations[-1].get("pr_number") return { "task": "review_pr", "remote": "prgs", @@ -40,9 +44,11 @@ def _lock(mutations=None, correction=False, ready_pr=None, ready_head=None, read "ready_remote": "prgs" if ready_pr else None, "ready_org": "Scaled-Tech-Consulting" if ready_pr else None, "ready_repo": "Gitea-Tools" if ready_pr else None, - "live_mutations": list(mutations or []), + "live_mutations": mutations, "correction_authorized": correction, - "correction_reason": None, + "correction_reason": "test" if correction else None, + "correction_pr_number": corr_pr, + "correction_head_sha": None, } @@ -174,12 +180,13 @@ class TestHardStopHeadScope(unittest.TestCase): self.assertTrue(reasons) self.assertIn("#332", reasons[0]) - def test_rc_head_a_blocks_other_pr(self): + def test_rc_head_a_allows_other_pr_via_cross_pr_isolation(self): + """#693: foreign-PR terminal must not hard-stop mark_ready on another PR.""" _seed([RC_619_HEAD_A], ready_pr=619, ready_head=HEAD_A) reasons = mcp_server.terminal_review_hard_stop_reasons( 700, "mark_ready", expected_head_sha=HEAD_B ) - self.assertTrue(reasons) + self.assertEqual(reasons, []) def test_legacy_619_ledger_allows_new_head(self): """PR #619-style durable lock without mutation head_sha.""" diff --git a/tests/test_issue_693_review_decision_lock_recovery.py b/tests/test_issue_693_review_decision_lock_recovery.py new file mode 100644 index 0000000..b996340 --- /dev/null +++ b/tests/test_issue_693_review_decision_lock_recovery.py @@ -0,0 +1,427 @@ +"""Review-decision-lock recovery / cross-PR isolation (#693). + +Reproduces the PR #688 → PR #692 formal-review sequence: + + * durable lock holds REQUEST_CHANGES on open PR #688 @ head A (review_id 425) + * reviewer leases PR #692 @ head B (6d86db…) + * mark_final for #692 must succeed without correction unlock + * cleanup of open #688 terminal remains forbidden (#594) + * authorize_review_correction for #688 cannot unlock #692 + * eventual #692 APPROVE is unique and head-bound (review_id 426) + * mark_final is idempotent for same PR/action/head + * diagnosis returns one exact next_action + durable handoff payload +""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_server +import review_workflow_load +import stale_review_decision_lock as srdl + +# PR #688 / #692 incident fixtures (sanitized reconstruction from issue #693) +HEAD_688 = "c7a444eb4b41cf916fdbd20a4999ffd78af496d0" +HEAD_692 = "6d86db793bbdfaed16bd54d93171f1dd66f234ca" +PR_A = 688 +PR_B = 692 +REVIEW_ID_A = 425 +REVIEW_ID_B = 426 + +RC_688 = { + "pr_number": PR_A, + "action": "request_changes", + "review_id": REVIEW_ID_A, + "review_state": "request_changes", + "head_sha": HEAD_688, +} +APPROVE_692 = { + "pr_number": PR_B, + "action": "approve", + "review_id": REVIEW_ID_B, + "review_state": "approve", + "head_sha": HEAD_692, +} + + +def _lock(mutations=None, **kwargs): + from datetime import datetime, timezone + + now = datetime.now(timezone.utc).isoformat() + base = { + "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": False, + "correction_reason": None, + "correction_pr_number": None, + "correction_head_sha": None, + "updated_at": now, + "recorded_at": now, + "writer_pid": os.getpid(), + } + base.update(kwargs) + # Ensure TTL fields stay fresh unless the caller is testing expiry. + if "recorded_at" not in kwargs: + base["recorded_at"] = now + if "updated_at" not in kwargs: + base["updated_at"] = now + return base + + +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"], +} + + +def _seed(mutations=None, **kwargs): + review_workflow_load.record_review_workflow_load(mcp_server.PROJECT_ROOT) + mcp_server._save_review_decision_lock(_lock(mutations, **kwargs)) + mcp_server.gitea_load_review_workflow() + + +def _no_lease(): + return {"block": False, "reasons": [], "mutation_allowed": True} + + +def _open_pr(number, head): + return { + "number": number, + "state": "open", + "merged": False, + "merged_at": None, + "head": {"sha": head}, + } + + +class TestCrossPrIsolationPure(unittest.TestCase): + def test_foreign_terminal_does_not_block_mark_boundary(self): + lock = _lock([RC_688]) + self.assertFalse( + srdl.prior_live_mutations_block_boundary( + lock, pr_number=PR_B, expected_head_sha=HEAD_692 + ) + ) + self.assertTrue( + srdl.prior_live_mutations_block_boundary( + lock, pr_number=PR_A, expected_head_sha=HEAD_688 + ) + ) + + def test_terminal_boundary_allows_fresh_decision_on_other_pr(self): + lock = _lock([RC_688]) + self.assertTrue( + srdl.terminal_boundary_allows_fresh_decision( + lock, + pr_number=PR_B, + expected_head_sha=HEAD_692, + operation="mark_ready", + ) + ) + self.assertFalse( + srdl.terminal_boundary_allows_fresh_decision( + lock, + pr_number=PR_A, + expected_head_sha=HEAD_688, + operation="mark_ready", + ) + ) + + def test_unscoped_correction_does_not_apply(self): + lock = _lock([RC_688], correction_authorized=True, correction_reason="x") + # Missing correction_pr_number → fail closed (#693) + self.assertFalse( + srdl.correction_applies(lock, pr_number=PR_A, expected_head_sha=HEAD_688) + ) + + def test_scoped_correction_only_for_named_pr(self): + lock = _lock( + [RC_688], + correction_authorized=True, + correction_reason="mistake", + correction_pr_number=PR_A, + correction_head_sha=HEAD_688, + ) + self.assertTrue( + srdl.correction_applies(lock, pr_number=PR_A, expected_head_sha=HEAD_688) + ) + self.assertFalse( + srdl.correction_applies(lock, pr_number=PR_B, expected_head_sha=HEAD_692) + ) + + +class TestClassification(unittest.TestCase): + def test_foreign_pr_terminal_classification(self): + lock = _lock([RC_688]) + c = srdl.classify_review_decision_lock( + lock, + target_pr_number=PR_B, + target_head_sha=HEAD_692, + pr_live=_open_pr(PR_A, HEAD_688), + active_profile_identity="prgs-reviewer", + ) + self.assertEqual(c["classification"], srdl.CLASS_FOREIGN_PR_TERMINAL) + self.assertTrue(c["mark_final_allowed"]) + self.assertFalse(c["correction_eligible"]) + self.assertIn("do not call gitea_authorize_review_correction", c["next_action"]) + + def test_same_head_terminal_classification(self): + lock = _lock([RC_688]) + c = srdl.classify_review_decision_lock( + lock, + target_pr_number=PR_A, + target_head_sha=HEAD_688, + pr_live=_open_pr(PR_A, HEAD_688), + ) + self.assertEqual(c["classification"], srdl.CLASS_TERMINAL_ACTIVE_SAME_HEAD) + self.assertFalse(c["mark_final_allowed"]) + self.assertTrue(c["correction_eligible"]) + + def test_precise_failure_includes_durable_handoff(self): + lock = _lock([RC_688]) + c = srdl.classify_review_decision_lock( + lock, target_pr_number=PR_A, target_head_sha=HEAD_688 + ) + fail = srdl.build_precise_mark_final_failure(c) + self.assertIn("exact_next_action", fail) + self.assertIn("durable_issue_handoff", fail) + self.assertTrue(fail["durable_issue_handoff"]["required"]) + + +class TestPr692Sequence(unittest.TestCase): + def setUp(self): + self._env = patch.dict(os.environ, REVIEWER_ENV, clear=False) + self._env.start() + self._prof = patch.object( + mcp_server, "get_profile", return_value=REVIEWER_PROFILE + ) + self._prof.start() + + def tearDown(self): + self._prof.stop() + self._env.stop() + mcp_server._save_review_decision_lock(None) + review_workflow_load.clear_review_workflow_load() + + def test_mark_final_692_succeeds_with_open_688_terminal(self): + _seed([RC_688], writer_pid=25883, session_pid=60615) + with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch( + "mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease() + ), patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={"head_sha": HEAD_692, "eligible": True}, + ): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=PR_B, + action="approve", + expected_head_sha=HEAD_692, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(result["marked_ready"], result.get("reasons")) + lock = mcp_server._load_review_decision_lock() + self.assertEqual(lock["ready_pr_number"], PR_B) + self.assertEqual( + srdl.normalize_head_sha(lock["ready_expected_head_sha"]), HEAD_692 + ) + # Foreign terminal history preserved (non-destructive isolation). + self.assertEqual(lock["live_mutations"][0]["pr_number"], PR_A) + + def test_mark_final_idempotent_same_binding(self): + _seed( + [RC_688], + final_review_decision_ready=True, + ready_pr_number=PR_B, + ready_action="approve", + ready_expected_head_sha=HEAD_692, + ready_remote="prgs", + ready_org="Scaled-Tech-Consulting", + ready_repo="Gitea-Tools", + ) + with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch( + "mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease() + ), patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={"head_sha": HEAD_692, "eligible": True}, + ): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=PR_B, + action="approve", + expected_head_sha=HEAD_692, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(result["marked_ready"]) + self.assertTrue(result.get("idempotent")) + + def test_cleanup_still_forbidden_while_688_open(self): + _seed([RC_688]) + assessment = srdl.assess_stale_review_decision_lock( + mcp_server._load_review_decision_lock(), + pr_live=_open_pr(PR_A, HEAD_688), + active_profile_identity="prgs-reviewer", + ) + self.assertFalse(assessment["cleanup_allowed"]) + self.assertFalse(assessment["is_moot"]) + + def test_correction_cannot_unlock_different_pr(self): + _seed([RC_688]) + r = mcp_server.gitea_authorize_review_correction( + prior_review_id=REVIEW_ID_A, + prior_review_state="request_changes", + reason="try unlock 692", + operator_authorized=True, + target_pr_number=PR_B, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + post_audit_comment=False, + ) + self.assertFalse(r["authorized"]) + self.assertTrue(any("different PR" in x for x in r["reasons"])) + + def test_scoped_correction_for_same_pr_posts_audit(self): + _seed([RC_688]) + with patch( + "mcp_server._authenticated_username", return_value="sysadmin" + ), patch( + "mcp_server.get_auth_header", return_value="Basic dGVzdDp0ZXN0" + ), patch( + "mcp_server._auth", return_value="Basic dGVzdDp0ZXN0" + ), patch("mcp_server.api_request", return_value={"id": 99901}): + r = mcp_server.gitea_authorize_review_correction( + prior_review_id=REVIEW_ID_A, + prior_review_state="request_changes", + reason="mistaken RC wording", + operator_authorized=True, + target_pr_number=PR_A, + expected_head_sha=HEAD_688, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + post_audit_comment=True, + ) + self.assertTrue(r["authorized"], r.get("reasons")) + self.assertEqual(r.get("correction_pr_number"), PR_A) + self.assertEqual( + r.get("audit_comment_id"), + 99901, + r.get("audit_comment_skipped") or r.get("audit_comment_error") or r, + ) + + def test_diagnose_foreign_terminal_next_action(self): + _seed([RC_688]) + with patch( + "mcp_server._authenticated_username", return_value="sysadmin" + ), patch( + "mcp_server.get_auth_header", return_value="Basic dGVzdDp0ZXN0" + ), patch( + "mcp_server._auth", return_value="Basic dGVzdDp0ZXN0" + ), patch("mcp_server.api_request", return_value=_open_pr(PR_A, HEAD_688)): + d = mcp_server.gitea_diagnose_review_decision_lock( + target_pr_number=PR_B, + expected_head_sha=HEAD_692, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(d["success"], d.get("reasons")) + self.assertEqual(d["classification"], srdl.CLASS_FOREIGN_PR_TERMINAL) + self.assertTrue(d["mark_final_allowed"]) + self.assertIn("mark_final", d["exact_next_action"]) + self.assertTrue(d["correction_as_generic_unlock_forbidden"]) + + def test_approval_ledger_unique_and_bound_after_sequence(self): + """After mark_final + record mutation, ledger binds unique approve to 692 head.""" + _seed([RC_688]) + with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch( + "mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease() + ), patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={"head_sha": HEAD_692, "eligible": True}, + ): + marked = mcp_server.gitea_mark_final_review_decision( + pr_number=PR_B, + action="approve", + expected_head_sha=HEAD_692, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(marked["marked_ready"]) + mcp_server.record_live_review_mutation(PR_B, "approve", REVIEW_ID_B) + lock = mcp_server._load_review_decision_lock() + terminals = [ + m + for m in lock["live_mutations"] + if m.get("action") in srdl.TERMINAL_REVIEW_ACTIONS + and m.get("pr_number") == PR_B + ] + self.assertEqual(len(terminals), 1) + self.assertEqual(terminals[0]["review_id"], REVIEW_ID_B) + self.assertEqual( + srdl.normalize_head_sha(terminals[0].get("head_sha")), HEAD_692 + ) + # Same-head duplicate still blocked. + self.assertTrue( + srdl.prior_live_mutations_block_boundary( + lock, pr_number=PR_B, expected_head_sha=HEAD_692 + ) + ) + + +class TestSessionRestartWriterPid(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + review_workflow_load.clear_review_workflow_load() + + def test_writer_pid_change_does_not_inherit_foreign_block(self): + # session_pid 60615 origin, writer later 25883 — still foreign isolation. + _seed([RC_688], session_pid=60615, writer_pid=25883) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons( + PR_B, "mark_ready", expected_head_sha=HEAD_692 + ), + [], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 3ac2905..6ab5660 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -2540,15 +2540,18 @@ class TestSubmitPrReview(unittest.TestCase): lock = _load_review_decision_lock() or {} lock["live_mutations"] = [{"pr_number": 8, "action": "approve", "review_id": 42, "review_state": "approve"}] _save_review_decision_lock(lock) - r = gitea_authorize_review_correction( - prior_review_id=42, - prior_review_state="approve", - reason="typo", - operator_authorized=True, - ) + with patch("mcp_server.api_request", return_value={"id": 9001}): + r = gitea_authorize_review_correction( + prior_review_id=42, + prior_review_state="approve", + reason="typo", + operator_authorized=True, + target_pr_number=8, + ) self.assertTrue(r["authorized"]) lock = _load_review_decision_lock() self.assertTrue(lock["correction_authorized"]) + self.assertEqual(lock["correction_pr_number"], 8) def test_authorize_review_correction_mismatch(self): from mcp_server import _load_review_decision_lock, _save_review_decision_lock @@ -2562,6 +2565,7 @@ class TestSubmitPrReview(unittest.TestCase): prior_review_state="approve", reason="typo", operator_authorized=True, + target_pr_number=8, ) self.assertFalse(r["authorized"]) self.assertTrue(any("prior review ID" in x for x in r["reasons"])) @@ -2572,10 +2576,22 @@ class TestSubmitPrReview(unittest.TestCase): prior_review_state="request_changes", reason="typo", operator_authorized=True, + target_pr_number=8, ) self.assertFalse(r["authorized"]) self.assertTrue(any("prior review state" in x for x in r["reasons"])) + # Cross-PR unlock refused (#693) + r = gitea_authorize_review_correction( + prior_review_id=42, + prior_review_state="approve", + reason="unlock other pr", + operator_authorized=True, + target_pr_number=692, + ) + self.assertFalse(r["authorized"]) + self.assertTrue(any("different PR" in x for x in r["reasons"])) + def test_authorize_review_correction_requires_operator_auth(self): from mcp_server import _load_review_decision_lock, _save_review_decision_lock lock = _load_review_decision_lock() or {} @@ -2588,6 +2604,7 @@ class TestSubmitPrReview(unittest.TestCase): prior_review_state="approve", reason="typo", operator_authorized=False, + target_pr_number=8, ) self.assertFalse(r["authorized"]) self.assertTrue(any("operator authorization" in x for x in r["reasons"])) @@ -2645,6 +2662,7 @@ class TestSubmitPrReview(unittest.TestCase): prior_review_state="approve", reason="operator approved correcting mistaken approve", operator_authorized=True, + target_pr_number=8, ) _mark_request_changes_ready(remote="prgs") second = gitea_submit_pr_review( diff --git a/tests/test_stale_review_decision_lock_cleanup.py b/tests/test_stale_review_decision_lock_cleanup.py index 1425644..89fed00 100644 --- a/tests/test_stale_review_decision_lock_cleanup.py +++ b/tests/test_stale_review_decision_lock_cleanup.py @@ -188,20 +188,32 @@ class TestActiveHardStopPreserved(unittest.TestCase): 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_open_approve_blocks_same_pr_not_other_pr_mark_ready(self): + _seed([APPROVED_OPEN]) # approve on PR #700 + # Same PR still hard-stopped for mark_ready. + reasons_same = mcp_server.terminal_review_hard_stop_reasons( + 700, "mark_ready" + ) + self.assertTrue(reasons_same) + self.assertIn("#332", reasons_same[0]) + # #693: other PR may mark_ready without cleanup. + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready"), + [], + ) - def test_request_changes_still_blocks_everything(self): - _seed([RC_OPEN]) + def test_request_changes_still_blocks_same_pr(self): + _seed([RC_OPEN]) # request_changes on PR #701 for op in ("merge", "mark_ready", "review"): self.assertTrue( - mcp_server.terminal_review_hard_stop_reasons(592, op), + mcp_server.terminal_review_hard_stop_reasons(701, op), msg=op, ) + # Foreign PR review path is open (#693). + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready"), + [], + ) # --------------------------------------------------------------------------- # diff --git a/tests/test_terminal_review_hard_stop.py b/tests/test_terminal_review_hard_stop.py index 07edd98..b2cb171 100644 --- a/tests/test_terminal_review_hard_stop.py +++ b/tests/test_terminal_review_hard_stop.py @@ -1,10 +1,10 @@ -"""Tests for the session hard-stop after a terminal review mutation (#332). +"""Tests for the session hard-stop after a terminal review mutation (#332/#693). Extends the single-terminal-decision machinery (#211): after a live -REQUEST_CHANGES the run must stop; after an APPROVE only the merge sequence -for that same PR may continue; a different PR can never be reviewed or -merged in the same run; duplicate REQUEST_CHANGES at an unchanged head is -suppressed. +REQUEST_CHANGES on a PR head the same-head path must stop; after an APPROVE +only the merge sequence for that same PR may continue; #693 allows a +*different* PR to be mark_ready/review in the same durable lock (cross-PR +isolation); duplicate REQUEST_CHANGES at an unchanged head is suppressed. """ import os import unittest @@ -15,7 +15,10 @@ import mcp_server HEAD_SHA = "a" * 40 -def _lock(mutations=None, correction=False): +def _lock(mutations=None, correction=False, correction_pr=None): + mutations = list(mutations or []) + if correction and correction_pr is None and mutations: + correction_pr = mutations[-1].get("pr_number") return { "task": "review_pr", "remote": "prgs", @@ -29,9 +32,11 @@ def _lock(mutations=None, correction=False): "ready_remote": None, "ready_org": None, "ready_repo": None, - "live_mutations": list(mutations or []), + "live_mutations": mutations, "correction_authorized": correction, - "correction_reason": None, + "correction_reason": "test" if correction else None, + "correction_pr_number": correction_pr if correction else None, + "correction_head_sha": None, } @@ -64,25 +69,35 @@ class TestTerminalHardStopReasons(unittest.TestCase): self.assertEqual( mcp_server.terminal_review_hard_stop_reasons(5, "merge"), []) - def test_request_changes_blocks_everything(self): + def test_request_changes_blocks_same_pr_allows_other_pr_review(self): _seed([RC_A]) + # Same PR: still hard-stopped for mark_ready/review/merge. for op in ("merge", "mark_ready", "review"): - for pr in (5, 6): - reasons = mcp_server.terminal_review_hard_stop_reasons(pr, op) - self.assertTrue(reasons, f"{op}/{pr}") - self.assertIn("request_changes on PR #5", reasons[0]) - self.assertIn("#332", reasons[0]) + reasons = mcp_server.terminal_review_hard_stop_reasons(5, op) + self.assertTrue(reasons, f"{op}/5") + self.assertIn("request_changes on PR #5", reasons[0]) + self.assertIn("#332", reasons[0]) + # #693: different PR may mark_ready / review (not merge via hard-stop alone). + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(6, "mark_ready"), []) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(6, "review"), []) + # Merge of an unapproved other PR remains blocked by hard-stop path + # (last terminal is not approve of PR 6). + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(6, "merge")) - def test_approve_allows_same_pr_merge_only(self): + def test_approve_allows_same_pr_merge_and_other_pr_review(self): _seed([APPROVED_A]) self.assertEqual( mcp_server.terminal_review_hard_stop_reasons(5, "merge"), []) self.assertTrue( mcp_server.terminal_review_hard_stop_reasons(6, "merge")) - self.assertTrue( - mcp_server.terminal_review_hard_stop_reasons(6, "mark_ready")) - self.assertTrue( - mcp_server.terminal_review_hard_stop_reasons(6, "review")) + # #693 cross-PR isolation: other PR formal review path is open. + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(6, "mark_ready"), []) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(6, "review"), []) def test_correction_reopens_review_path_only(self): _seed([RC_A], correction=True) @@ -90,9 +105,11 @@ class TestTerminalHardStopReasons(unittest.TestCase): mcp_server.terminal_review_hard_stop_reasons(5, "mark_ready"), []) self.assertEqual( mcp_server.terminal_review_hard_stop_reasons(5, "review"), []) - # correction never opens a cross-PR merge + # scoped correction never opens a cross-PR merge self.assertTrue( mcp_server.terminal_review_hard_stop_reasons(6, "merge")) + # unscoped/cross-PR correction must not lift PR 6 via correction alone + # (PR 6 is allowed by isolation, not by correction scope) class TestMergeHardStopWiring(unittest.TestCase): @@ -126,14 +143,29 @@ class TestMarkFinalHardStopWiring(unittest.TestCase): mcp_server._save_review_decision_lock(None) mcp_server.review_workflow_load.clear_review_workflow_load() - def test_mark_ready_blocked_after_terminal_mutation(self): + def test_mark_ready_same_pr_blocked_after_terminal_mutation(self): _seed([RC_A]) result = mcp_server.gitea_mark_final_review_decision( - pr_number=6, action="approve", remote="prgs") + pr_number=5, action="approve", remote="prgs", + expected_head_sha=HEAD_SHA) self.assertFalse(result["marked_ready"]) self.assertTrue( any("#332" in r for r in result["reasons"]), result["reasons"]) + def test_mark_ready_other_pr_allowed_after_foreign_terminal(self): + """#693: foreign open-PR terminal must not block mark_final on PR B.""" + _seed([RC_A]) + no_lease = {"block": False, "reasons": [], "mutation_allowed": True} + with patch("mcp_server._list_pr_lease_comments", return_value=[]), \ + patch("mcp_server._pr_work_lease_reviewer_block", + return_value=no_lease), \ + patch.object(mcp_server, "gitea_check_pr_eligibility", + return_value={"head_sha": HEAD_SHA, "eligible": True}): + result = mcp_server.gitea_mark_final_review_decision( + pr_number=6, action="approve", remote="prgs", + expected_head_sha=HEAD_SHA) + self.assertTrue(result["marked_ready"], result.get("reasons")) + def _feedback(blocking, stale=False, success=True): return {