Compare commits

..
Author SHA1 Message Date
sysadmin a1e5c33bfc feat(guard): native MCP transport binding and contaminated-review quarantine (Closes #695)
Bind mutation/credential paths to a process-local native MCP runtime so env
spoofing, direct imports, and offline helpers cannot reconstruct session gates
after native transport failure. Quarantine contaminated formal reviews under
controller authority and honor quarantine in review feedback, merge eligibility,
merge mutation, and canonical handoff validation. Add regression coverage for
the second (PR #694 / review 427) incident class.
2026-07-13 05:57:42 -04:00
21 changed files with 1613 additions and 1438 deletions
+21
View File
@@ -386,6 +386,27 @@ def assess_canonical_comment(
if not related or related.lower() in {"none", "n/a", "-"}:
missing.append("RELATED_PRS")
# #695 AC7: untrusted / offline approval claims cannot certify merger handoff.
try:
import review_quarantine as _rq
for claim_reason in _rq.assess_untrusted_canonical_approval_claim(text):
extra.append(claim_reason)
except Exception:
# Fail closed on import/runtime errors for approval-shaped claims only.
lower = text.lower()
if (
"state:\napproved" in lower
or "who_is_next:\nmerger" in lower
or "merge_ready: true" in lower
or "merge_ready:\ntrue" in lower
or "ready-to-merge" in lower
):
extra.append(
"canonical approval/merge-ready claim could not be verified "
"for native review proof (#695 AC7; fail closed)"
)
allowed = not missing and not vague and not extra
correction = ""
if not allowed:
+65 -11
View File
@@ -1,23 +1,77 @@
# MCP daemon import and keychain guard (#558)
# MCP daemon import and native-transport guard (#558 / #695)
## Problem
During deadlock debugging, agents imported `gitea_mcp_server` / ran credential
helpers from a raw shell, bypassing preflight purity and role gates.
During deadlock debugging and the PR #694 incident (#695), agents imported
`gitea_mcp_server` or ran credential helpers from a raw shell / offline helper,
bypassing native MCP transport, preflight purity, and role gates. Contaminated
formal reviews then looked identical to native approvals.
## Rule
Mutation auth and keychain fill require a **sanctioned MCP daemon** process.
Mutation auth, keychain fill, and controller quarantine require a **native MCP
transport runtime** established only by the official entrypoint.
| Context | Allowed |
|---------|---------|
| Official MCP entrypoint (`mcp_server.py` / `gitea_mcp_server` `__main__`) sets `GITEA_MCP_SANCTIONED_DAEMON=1` | yes |
| pytest | yes |
| `GITEA_ALLOW_DIRECT_MCP_IMPORT=1` (operator/tests only) | yes |
| bare `python -c 'import gitea_auth; get_auth_header(...)'` | **no** |
| keychain fill without daemon | **no** unless `GITEA_ALLOW_KEYCHAIN_CLI=1` |
| Official MCP entrypoint (`mcp_server.py` / `gitea_mcp_server` `__main__`) calls `mark_sanctioned_daemon()` and holds a process-local runtime token | yes |
| pytest (hermetic unit tests) | yes |
| `GITEA_MCP_SANCTIONED_DAEMON=1` alone (no process-local native runtime) | **no** (#695) |
| `GITEA_ALLOW_DIRECT_MCP_IMPORT=1` in LLM sessions | **no** — never set in agent sessions |
| `GITEA_ALLOW_KEYCHAIN_CLI=1` in LLM sessions | **no** — human operator only |
| bare `python -c 'import gitea_mcp_server; …'` or offline runners | **no** |
| keychain fill outside native/pytest | **no** |
Native runtime is **process-local**: a random token bound to the daemon PID.
It is never reconstructed from environment variables, session-state files, or
importing internals in a fresh Python process.
## Contaminated review quarantine (#695 AC8)
Controller/reconciler/merger profiles may call
`gitea_quarantine_contaminated_review` with explicit confirmation:
```text
QUARANTINE CONTAMINATED REVIEW <review_id> PR <pr_number>
```
Quarantine records are durable under the MCP session-state root and are
**honored** by:
- `gitea_get_pr_review_feedback` (quarantined approvals do not authorize merge)
- `gitea_check_pr_eligibility` action=`merge`
- `gitea_merge_pr` (mutation)
- merger lease adoption paths that read `approval_at_current_head`
Forensic Gitea reviews and historical comments are **never deleted**.
## STOP after native MCP failure (AC10)
If the native MCP namespace dies (EOF, capability disconnect, session death):
1. **STOP.** State BLOCKED + DIAGNOSE.
2. Do **not** import `gitea_mcp_server` from a standalone process.
3. Do **not** run `offline_mcp_helper.py`, `offline_mcp_runner.py`,
`run_quarantine.py`, or any offline mutation helper.
4. Do **not** set direct-import, keychain-bypass, or raw-token environment
variables.
5. Reconnect / restart the official MCP daemon; resume only via native tools.
Any further native MCP failure is a hard stop. Do not construct another fallback.
## Canonical approval claims (AC7)
Comments that claim `approved` / `ready-to-merge` / `WHO_IS_NEXT: merger` /
`MERGE_READY: true` must include:
```text
NATIVE_REVIEW_PROOF: transport=native_mcp; …
```
Claims that cite offline/import helpers are rejected even if a proof line is
present.
## Operator note
LLM sessions must never set the allow-direct-import or allow-keychain-cli
overrides. Those are human-only escape hatches.
LLM sessions must never set allow-direct-import, allow-keychain-cli, or raw
token overrides. Those are human-only escape hatches outside agent workflows.
+405 -351
View File
@@ -1109,6 +1109,8 @@ import issue_lock_store # noqa: E402
import issue_lock_adoption # noqa: E402
import stacked_pr_support # noqa: E402
import merge_approval_gate # noqa: E402
import review_quarantine # noqa: E402 # #695 contaminated formal-review quarantine
import mcp_daemon_guard # noqa: E402 # #695 native transport provenance
import already_landed_reconcile # noqa: E402
import author_mutation_worktree # noqa: E402
import root_checkout_guard # noqa: E402
@@ -3055,6 +3057,51 @@ def gitea_check_pr_eligibility(
elif result["mergeable"] is None:
reasons.append("PR mergeability unknown")
# #695: merge eligibility must honor quarantine-aware formal review feedback.
# Contaminated approvals (e.g. review 427 on PR #694) must not make merge
# eligible even when Gitea still shows APPROVED / mergeable=true.
if action == "merge" and not reasons:
try:
feedback = gitea_get_pr_review_feedback(
pr_number=pr_number, remote=remote, host=host, org=org, repo=repo,
)
except Exception as exc: # noqa: BLE001 — fail closed, never leak secrets
feedback = {
"success": False,
"reasons": [
"PR review feedback unavailable for merge eligibility "
f"(fail closed, #695): {_redact(str(exc))}"
],
}
result["approval_visible"] = feedback.get("approval_visible")
result["approval_at_current_head"] = feedback.get("approval_at_current_head")
result["quarantined_approvals_at_current_head"] = feedback.get(
"quarantined_approvals_at_current_head"
)
result["stale_approval_block_reason"] = feedback.get(
"stale_approval_block_reason"
)
result["has_blocking_change_requests"] = feedback.get(
"has_blocking_change_requests"
)
if not feedback.get("success"):
reasons.append(
"PR review feedback unavailable for merge eligibility (fail closed, #695)"
)
reasons.extend(feedback.get("reasons") or [])
elif feedback.get("has_blocking_change_requests"):
reasons.append(
"undismissed REQUEST_CHANGES review blocks merge eligibility (fail closed)"
)
elif not feedback.get("approval_at_current_head"):
reasons.append(
feedback.get("stale_approval_block_reason")
or (
"no non-quarantined APPROVED review at current head; "
"merge eligibility denied (#695)"
)
)
result["eligible"] = len(reasons) == 0
if result["eligible"]:
reasons.append("all eligibility checks passed")
@@ -3338,8 +3385,6 @@ 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,
})
@@ -3424,30 +3469,21 @@ 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
):
scoped = stale_review_decision_lock.correction_applies(
lock, pr_number=pr_number, expected_head_sha=ready_head
)
if prior and not scoped:
if prior and not lock.get("correction_authorized"):
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 for this same "
"PR/head (fail closed, #620/#693)"
"gitea_authorize_review_correction was invoked (fail closed, #620)"
)
elif (
action in _TERMINAL_REVIEW_ACTIONS
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
and any(m.get("action") in _TERMINAL_REVIEW_ACTIONS for m in prior)
and not lock.get("correction_authorized")
):
reasons.append(
"terminal review decision already submitted for this PR head in "
"this run; blocked unless an operator-approved correction was "
"authorized for this same PR/head (fail closed, #620/#693)"
"authorized (fail closed, #620)"
)
return reasons
@@ -3464,6 +3500,10 @@ def record_live_review_mutation(pr_number: int, action: str, review_id: int | No
"action": action,
"review_id": review_id,
"review_state": action,
# #695 AC6: audit records expose native session/transport provenance.
**mcp_daemon_guard.mutation_provenance_fields(),
"writer_pid": os.getpid(),
"session_pid": lock.get("session_pid") or os.getpid(),
}
if head_sha:
entry["head_sha"] = head_sha
@@ -3472,8 +3512,6 @@ 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)
@@ -3517,13 +3555,11 @@ def terminal_review_hard_stop_reasons(
and last.get("pr_number") == pr_number
):
return []
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
)
if operation in ("mark_ready", "review", "resume") and lock.get(
"correction_authorized"
):
return []
# #620 same-PR new head + #693 cross-PR isolation.
# #620: same PR, different reviewed head → fresh decision boundary.
if stale_review_decision_lock.terminal_boundary_allows_fresh_decision(
lock,
pr_number=pr_number,
@@ -3545,20 +3581,16 @@ def terminal_review_hard_stop_reasons(
else ""
)
recovery = (
"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"
"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"
)
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/#693); {recovery}"
f"{guidance} (fail closed, #332); {recovery}"
]
@@ -3697,30 +3729,68 @@ def gitea_get_pr_review_feedback(
base = f"{repo_api_url(h, o, r)}/pulls/{pr_number}"
pr = api_request("GET", base, auth) or {}
raw_reviews = api_request("GET", f"{base}/reviews", auth) or []
if not isinstance(pr, dict):
return {
"success": False,
"pr_number": pr_number,
"feedback_not_attempted": True,
"reasons": ["PR payload unavailable for review feedback (fail closed)"],
}
if not isinstance(raw_reviews, list):
raw_reviews = []
current_head = (pr.get("head") or {}).get("sha")
reveal = _reveal_endpoints()
ordered = sorted(
raw_reviews,
(rv for rv in raw_reviews if isinstance(rv, dict)),
key=lambda rv: ((rv.get("submitted_at") or ""), rv.get("id") or 0),
)
reviews = []
latest_by_reviewer = {}
latest_reviewed_head = None
quarantined_review_ids: set[int] = set()
quarantined_at_head = 0
for rv in ordered:
state = (rv.get("state") or "").upper()
reviewer = (rv.get("user") or {}).get("login", "")
commit_id = rv.get("commit_id")
rid = rv.get("id")
try:
rid_int = int(rid) if rid is not None else None
except (TypeError, ValueError):
rid_int = None
q = review_quarantine.is_review_quarantined(
remote=remote,
org=o,
repo=r,
pr_number=pr_number,
review_id=rid_int,
reviewed_head_sha=commit_id or current_head,
)
entry = {
"reviewer": reviewer,
"verdict": state,
"body": _redact(rv.get("body") or ""),
"submitted_at": rv.get("submitted_at"),
"reviewed_head_sha": commit_id,
"review_id": rid_int,
"dismissed": bool(rv.get("dismissed")),
"stale": bool(rv.get("stale")) or bool(
commit_id and current_head and commit_id != current_head),
"quarantined": bool(q.get("quarantined")),
}
if q.get("quarantined"):
entry["quarantine_reasons"] = q.get("reasons") or []
if rid_int is not None:
quarantined_review_ids.add(rid_int)
if (
state == "APPROVED"
and not entry["dismissed"]
and current_head
and commit_id
and commit_id == current_head
):
quarantined_at_head += 1
if reveal:
entry["url"] = rv.get("html_url")
reviews.append(entry)
@@ -3728,7 +3798,11 @@ def gitea_get_pr_review_feedback(
# per-reviewer verdict — otherwise a drive-by comment on the
# current head would mask the staleness of an older undismissed
# REQUEST_CHANGES.
# #695: quarantined formal reviews never authorize merge and must not
# become the reviewer's latest active verdict for eligibility/merge.
if state in _VERDICT_STATES and state != "COMMENT":
if entry.get("quarantined"):
continue
latest_reviewed_head = commit_id or latest_reviewed_head
if reviewer:
latest_by_reviewer[reviewer] = entry
@@ -3744,7 +3818,21 @@ def gitea_get_pr_review_feedback(
approval_head = merge_approval_gate.assess_merge_approval_head(
current_head_sha=current_head,
latest_by_reviewer=latest_by_reviewer,
quarantined_review_ids=quarantined_review_ids,
)
stale_reason = approval_head["stale_approval_block_reason"]
# When the only approvals at head are quarantined, they were excluded from
# latest_by_reviewer; surface an explicit #695 void reason for eligibility/merge.
if (
not approval_head["approval_at_current_head"]
and quarantined_at_head
and not stale_reason
):
stale_reason = (
"contaminated/quarantined approval at current head is void for "
"merge authorization (#695); required next action: fresh native "
"MCP re-review after controller quarantine evidence is recorded"
)
return {
"success": True,
"pr_number": pr_number,
@@ -3757,7 +3845,14 @@ def gitea_get_pr_review_feedback(
"approval_visible": bool(approvals),
"approval_at_current_head": approval_head["approval_at_current_head"],
"latest_approved_head_sha": approval_head["latest_approved_head_sha"],
"stale_approval_block_reason": approval_head["stale_approval_block_reason"],
"stale_approval_block_reason": stale_reason,
"quarantined_review_ids": sorted(quarantined_review_ids),
"quarantined_approvals_at_current_head": (
max(
approval_head.get("quarantined_approvals_at_current_head") or 0,
quarantined_at_head,
)
),
"latest_reviewed_head_sha": latest_reviewed_head,
"review_feedback_stale": bool(
latest_reviewed_head and current_head
@@ -3766,6 +3861,7 @@ def gitea_get_pr_review_feedback(
e["reviewed_head_sha"] and current_head
and e["reviewed_head_sha"] != current_head
for e in blocking),
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
@@ -4140,33 +4236,10 @@ 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:
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,
}
return {"marked_ready": False, "reasons": session_reasons}
if remote != lock.get("remote"):
return {
"marked_ready": False,
@@ -4174,7 +4247,6 @@ 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)
@@ -4209,26 +4281,7 @@ def gitea_mark_final_review_decision(
pr_number, "mark_ready", expected_head_sha=expected_head_sha
)
if 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"),
}
return {"marked_ready": False, "reasons": hard_stop}
if action not in _REVIEW_ACTIONS:
return {
"marked_ready": False,
@@ -4244,63 +4297,17 @@ 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 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"),
"correction was authorized (fail closed, #620)"
],
}
elig = gitea_check_pr_eligibility(
pr_number=pr_number,
@@ -4384,19 +4391,8 @@ 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.
#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.
"""
"""Authorize one operator-approved correction after a mistaken live review."""
reason = (reason or "").strip()
prior_review_state = (prior_review_state or "").strip().lower()
lock = _load_review_decision_lock()
@@ -4422,20 +4418,7 @@ 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 "
@@ -4446,202 +4429,14 @@ 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,
"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
return {"authorized": False, "reasons": reasons}
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)
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"),
}
return {"authorized": True, "correction_reason": reason, "reasons": []}
@mcp.tool()
@@ -5734,6 +5529,16 @@ def gitea_merge_pr(
result["pr_author"] = elig.get("pr_author")
result["head_sha"] = elig.get("head_sha")
result["mergeable"] = elig.get("mergeable")
# Surface #695 approval/quarantine fields from eligibility even on deny.
for _k in (
"approval_visible",
"approval_at_current_head",
"quarantined_approvals_at_current_head",
"stale_approval_block_reason",
"has_blocking_change_requests",
):
if _k in elig:
result[_k] = elig.get(_k)
if not elig.get("eligible"):
reasons.append("eligibility check for 'merge' failed (fail closed)")
reasons.extend(elig.get("reasons", []))
@@ -5841,16 +5646,31 @@ def gitea_merge_pr(
result["review_feedback_stale"] = feedback.get("review_feedback_stale")
result["has_blocking_change_requests"] = feedback.get(
"has_blocking_change_requests")
result["quarantined_review_ids"] = feedback.get("quarantined_review_ids") or []
result["quarantined_approvals_at_current_head"] = feedback.get(
"quarantined_approvals_at_current_head"
)
if feedback.get("has_blocking_change_requests"):
reasons.append(
"undismissed REQUEST_CHANGES review blocks merge (fail closed)"
)
return result
if not feedback.get("approval_visible"):
reasons.append(
"no visible APPROVED review on PR; verify review submission "
"completed before merge (fail closed)"
)
# #695: quarantined APPROVED reviews are not "visible" for merge auth.
if feedback.get("quarantined_approvals_at_current_head"):
reasons.append(
feedback.get("stale_approval_block_reason")
or (
"only contaminated/quarantined approval(s) at current head "
"(#695); merge authorization is void — fresh native MCP "
"re-review required after controller quarantine evidence"
)
)
else:
reasons.append(
"no visible APPROVED review on PR; verify review submission "
"completed before merge (fail closed)"
)
return result
if not feedback.get("approval_at_current_head"):
reasons.append(
@@ -12993,13 +12813,247 @@ def gitea_reclaim_expired_workflow_lease(
}
@mcp.tool()
def gitea_quarantine_contaminated_review(
pr_number: int,
review_id: int,
confirmation: str,
reason: str,
reviewed_head_sha: str,
incident_issue: int = 695,
forensic_comment_ids: list[int] | None = None,
remote: str = "dadeschools",
host: str | None = None,
org: str | None = None,
repo: str | None = None,
post_audit_comment: bool = True,
) -> dict:
"""Controller quarantine of a contaminated formal review (#695 AC8).
Writes a durable quarantine record that live feedback / eligibility /
merge gates honor by ``review_id``. Forensic Gitea review objects and
historical comments are retained (never deleted).
**Native transport only.** Untrusted local imports / offline scripts
cannot create quarantine records. Confirmation must equal exactly
``QUARANTINE CONTAMINATED REVIEW <review_id> PR <pr_number>``.
Restricted to reconciler / merger / controller profiles (not author or
the contaminated reviewer acting alone). Does not auto-apply to review
427 until an independent adversarial reviewer and controller deployment
of this tooling have completed.
"""
# Fail closed outside native MCP before any mutation assessment.
try:
mcp_daemon_guard.assert_sanctioned_mutation_runtime(
"gitea_quarantine_contaminated_review"
)
except mcp_daemon_guard.UnsanctionedRuntimeError as exc:
return {
"success": False,
"quarantined": False,
"reasons": [str(exc)],
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
assessment = review_quarantine.assess_quarantine_write(
confirmation=confirmation,
pr_number=pr_number,
review_id=review_id,
reason=reason,
native_required=True,
)
if not assessment.get("allowed"):
return {
"success": False,
"quarantined": False,
"reasons": assessment.get("reasons") or [],
"expected_confirmation": assessment.get("expected_confirmation"),
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
profile = get_profile()
profile_name = (profile.get("profile_name") or "").strip()
role = _actual_profile_role()
allowed_roles = {"reconciler", "merger"}
controller_named = "controller" in profile_name.lower()
if role not in allowed_roles and not controller_named:
return {
"success": False,
"quarantined": False,
"reasons": [
f"quarantine requires reconciler/merger/controller profile "
f"(active role={role!r}, profile={profile_name!r}); "
"author/reviewer-only sessions cannot quarantine (#695)"
],
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
comment_block = _profile_operation_gate("gitea.pr.comment")
if comment_block and post_audit_comment:
return {
"success": False,
"quarantined": False,
"reasons": comment_block,
"permission_report": _permission_block_report("gitea.pr.comment"),
}
read_block = _profile_operation_gate("gitea.read")
if read_block:
return {
"success": False,
"quarantined": False,
"reasons": read_block,
"permission_report": _permission_block_report("gitea.read"),
}
h, o, r = _resolve(remote, host, org, repo)
auth = _auth(h)
try:
identity = _authenticated_username(h) or profile.get("username") or ""
except Exception:
identity = profile.get("username") or ""
# Verify review exists (forensic retention — do not dismiss via Gitea API).
try:
reviews = (
api_request(
"GET",
f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews",
auth,
)
or []
)
except Exception as exc: # noqa: BLE001
return {
"success": False,
"quarantined": False,
"reasons": [
f"could not list PR reviews before quarantine (fail closed): "
f"{_redact(str(exc))}"
],
}
match = None
for rv in reviews:
if int(rv.get("id") or 0) == int(review_id):
match = rv
break
if match is None:
return {
"success": False,
"quarantined": False,
"reasons": [
f"review_id {review_id} not found on PR #{pr_number} "
f"(fail closed; refuse quarantine of missing review)"
],
}
live_head = (match.get("commit_id") or "").strip().lower()
want_head = (reviewed_head_sha or "").strip().lower()
if want_head and live_head and want_head != live_head:
return {
"success": False,
"quarantined": False,
"reasons": [
f"reviewed_head_sha {want_head[:12]}… does not match review "
f"commit_id {live_head[:12]}… (fail closed, #695)"
],
}
record = review_quarantine.build_quarantine_record(
remote=remote,
org=o,
repo=r,
pr_number=pr_number,
review_id=review_id,
reviewed_head_sha=want_head or live_head,
reason=reason,
actor_username=identity,
profile_name=profile_name,
incident_issue=incident_issue,
forensic_comment_ids=forensic_comment_ids,
)
try:
written = review_quarantine.write_quarantine_record(record)
except mcp_daemon_guard.UnsanctionedRuntimeError as exc:
return {
"success": False,
"quarantined": False,
"reasons": [str(exc)],
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
except OSError as exc:
return {
"success": False,
"quarantined": False,
"reasons": [f"quarantine persist failed: {_redact(str(exc))}"],
}
audit_comment_id = None
if post_audit_comment:
body = review_quarantine.format_quarantine_audit_comment(record)
try:
with _audited(
"comment_pr",
host=h,
remote=remote,
org=o,
repo=r,
pr_number=pr_number,
request_metadata={"source": "quarantine_contaminated_review"},
):
posted = api_request(
"POST",
f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments",
auth,
{"body": body},
)
if isinstance(posted, dict):
audit_comment_id = posted.get("id")
except Exception as exc: # noqa: BLE001
return {
"success": True,
"quarantined": True,
"review_id": review_id,
"pr_number": pr_number,
"path": written.get("path"),
"audit_comment_id": None,
"warnings": [
f"quarantine written but audit comment failed: "
f"{_redact(str(exc))}"
],
"record": {
k: v
for k, v in record.items()
if k != "native_provenance"
} | {
"native_provenance": record.get("native_provenance"),
},
"native_runtime": mcp_daemon_guard.native_runtime_status(),
}
return {
"success": True,
"quarantined": True,
"review_id": review_id,
"pr_number": pr_number,
"path": written.get("path"),
"audit_comment_id": audit_comment_id,
"retain_forensic_evidence": True,
"merge_authorization": "void",
"record": record,
"native_runtime": mcp_daemon_guard.native_runtime_status(),
"reasons": [
f"review_id {review_id} quarantined for PR #{pr_number}; "
"merge authorization void; forensic evidence retained (#695)"
],
}
# ── Entry point ───────────────────────────────────────────────────────────────
if __name__ == "__main__":
# #558: mark this process as the official MCP daemon before any tool
# dispatch so direct shell imports cannot reuse mutation/auth paths.
import mcp_daemon_guard
# #558 / #695: mark this process as the official native MCP daemon before
# any tool dispatch. Env vars alone cannot reconstruct native transport;
# offline imports / standalone scripts fail closed on mutations.
mcp_daemon_guard.mark_sanctioned_daemon()
# Lock this session's launch profile into the environment so child CLI
# processes (e.g. review_pr.py) can detect and refuse profile
+157 -34
View File
@@ -1,67 +1,153 @@
"""Sanctioned MCP daemon guards for imports and credential access (#558).
"""Sanctioned MCP daemon guards for imports and credential access (#558 / #695).
Direct ``import gitea_mcp_server`` / ``import gitea_auth`` from a shell, plus
raw keychain dumps, bypass preflight purity and role gates. Mutation helpers
and keychain fallbacks therefore require an explicit sanctioned runtime.
Direct ``import gitea_mcp_server`` from a shell bypasses native MCP transport.
#558 introduced a daemon marker; #695 hardens it so:
Sanctioned contexts (any one):
- ``GITEA_MCP_SANCTIONED_DAEMON=1`` (set by the official MCP entrypoint)
- pytest (``PYTEST_CURRENT_TEST`` present)
- explicit operator opt-in ``GITEA_ALLOW_DIRECT_MCP_IMPORT=1`` (tests/tools only)
- Environment variables alone cannot reconstruct a native session
(``GITEA_MCP_SANCTIONED_DAEMON=1`` / ``GITEA_ALLOW_DIRECT_MCP_IMPORT=1`` are
insufficient for mutation gates).
- A process-local runtime record is created only by the official entrypoint
(``mcp_server.py`` calling ``mark_sanctioned_daemon``), bound to PID and a
random secret that never leaves process memory.
- Offline scripts that import internals fail closed on mutations.
- Pytest remains allowed (hermetic tests); optional force flags exist for
provenance regression tests.
Credential keychain fill additionally allows:
- ``GITEA_ALLOW_KEYCHAIN_CLI=1`` for operator-only non-MCP scripts that must
use git-credential (never the default for LLM shells).
Manual deletion of session-state files is never a recovery path.
"""
from __future__ import annotations
import hashlib
import inspect
import os
import secrets
import time
from typing import Any
SANCTIONED_DAEMON_ENV = "GITEA_MCP_SANCTIONED_DAEMON"
ALLOW_DIRECT_IMPORT_ENV = "GITEA_ALLOW_DIRECT_MCP_IMPORT"
ALLOW_KEYCHAIN_CLI_ENV = "GITEA_ALLOW_KEYCHAIN_CLI"
# Test-only: force provenance failure even under pytest (#695 regressions).
FORCE_PROVENANCE_FAIL_ENV = "GITEA_TEST_FORCE_UNSANCTIONED"
# Process-local native runtime (never persisted, never read from env alone).
_NATIVE_RUNTIME: dict[str, Any] | None = None
class UnsanctionedRuntimeError(RuntimeError):
"""Raised when mutation/credential code runs outside a sanctioned MCP daemon."""
"""Raised when mutation/credential code runs outside a native MCP daemon."""
def is_pytest_runtime() -> bool:
if os.environ.get("GITEA_TEST_FORCE_UNSANCTIONED") == "1":
if (os.environ.get(FORCE_PROVENANCE_FAIL_ENV) or "").strip() in {
"1",
"true",
"yes",
}:
return False
import sys
if "pytest" in sys.modules:
return True
return bool((os.environ.get("PYTEST_CURRENT_TEST") or "").strip())
def is_sanctioned_mcp_daemon() -> bool:
if (os.environ.get(SANCTIONED_DAEMON_ENV) or "").strip() in {"1", "true", "yes"}:
return True
if (os.environ.get(ALLOW_DIRECT_IMPORT_ENV) or "").strip() in {"1", "true", "yes"}:
return True
if is_pytest_runtime():
return True
def _caller_is_official_entrypoint() -> bool:
"""True when mark_sanctioned_daemon is invoked from mcp_server.py."""
for frame in inspect.stack()[1:12]:
path = (frame.filename or "").replace("\\", "/")
base = path.rsplit("/", 1)[-1]
if base == "mcp_server.py":
return True
return False
def mark_sanctioned_daemon() -> None:
"""Call from the official MCP server entrypoint before serving tools."""
def mark_sanctioned_daemon(*, allow_test_bootstrap: bool = False) -> dict[str, Any]:
"""Mark this process as the official native MCP daemon (#695).
Only the official ``mcp_server.py`` entrypoint (or pytest test bootstrap)
may establish native transport. Setting env vars alone is insufficient.
"""
global _NATIVE_RUNTIME
if not is_pytest_runtime() and not allow_test_bootstrap:
if not _caller_is_official_entrypoint():
raise UnsanctionedRuntimeError(
"mark_sanctioned_daemon rejected: not called from official "
"mcp_server.py entrypoint (#695). Offline import / standalone "
"scripts cannot reconstruct native transport. Stop after native "
"MCP failure; do not run offline mutation helpers."
)
token = secrets.token_hex(32)
_NATIVE_RUNTIME = {
"token": token,
"token_fingerprint": hashlib.sha256(token.encode()).hexdigest()[:16],
"pid": os.getpid(),
"started_at": time.time(),
"entrypoint": "mcp_server",
}
# Legacy signal for older probes; alone does not authorize mutations.
os.environ[SANCTIONED_DAEMON_ENV] = "1"
return native_runtime_status()
def clear_native_runtime_for_tests() -> None:
"""Test helper: drop native runtime (does not clear env)."""
global _NATIVE_RUNTIME
_NATIVE_RUNTIME = None
def is_native_mcp_transport() -> bool:
"""True when this process holds a live native MCP runtime record (#695)."""
if (os.environ.get(FORCE_PROVENANCE_FAIL_ENV) or "").strip() in {
"1",
"true",
"yes",
}:
return False
if _NATIVE_RUNTIME is None:
return False
if int(_NATIVE_RUNTIME.get("pid") or -1) != os.getpid():
return False
if not (_NATIVE_RUNTIME.get("token") or "").strip():
return False
return True
def is_sanctioned_mcp_daemon() -> bool:
"""Backward-compatible name; #695 requires native transport, not env alone."""
if is_native_mcp_transport():
return True
if is_pytest_runtime():
return True
# Explicit direct-import override is for non-LLM operator/test tools only.
# It is deliberately ignored when a native runtime is expected for mutations
# under LLM sessions (tests use pytest path). Env alone never grants native.
return False
def assert_sanctioned_mutation_runtime(context: str = "mutation") -> None:
"""Fail closed when server mutation code is used outside the MCP daemon."""
"""Fail closed when mutation code runs outside native MCP transport (#695)."""
if is_sanctioned_mcp_daemon():
return
env_spoof = (os.environ.get(SANCTIONED_DAEMON_ENV) or "").strip() in {
"1",
"true",
"yes",
}
extra = ""
if env_spoof:
extra = (
f" Note: {SANCTIONED_DAEMON_ENV} alone is not sufficient (#695); "
"native transport requires the official MCP entrypoint."
)
raise UnsanctionedRuntimeError(
f"Unsanctioned runtime blocked {context} (#558). "
"Do not import gitea_mcp_server / call mutation helpers from a raw "
"shell or ad-hoc script. Use the official MCP daemon entrypoint "
f"(sets {SANCTIONED_DAEMON_ENV}=1), or run under pytest. "
f"Operator-only override: {ALLOW_DIRECT_IMPORT_ENV}=1 (not for LLM sessions)."
f"Unsanctioned / non-native runtime blocked {context} (#695). "
"Do not import gitea_mcp_server or call mutation helpers from a raw "
"shell, offline runner, or ad-hoc script after native MCP failure. "
"Stop and reconnect the official MCP daemon (mcp_server.py). "
f"Do not set {ALLOW_DIRECT_IMPORT_ENV} or raw token env vars in LLM "
f"sessions.{extra}"
)
@@ -69,21 +155,58 @@ def assert_keychain_access_allowed() -> None:
"""Fail closed for git-credential keychain fill outside sanctioned contexts."""
if is_sanctioned_mcp_daemon():
return
# Operator-only keychain CLI remains available outside LLM mutation path.
if (os.environ.get(ALLOW_KEYCHAIN_CLI_ENV) or "").strip() in {"1", "true", "yes"}:
return
if not is_pytest_runtime():
# Still block pure env spoof of SANCTIONED_DAEMON for keychain when
# FORCE is set for tests.
if (os.environ.get(FORCE_PROVENANCE_FAIL_ENV) or "").strip() in {
"1",
"true",
"yes",
}:
pass
else:
return
raise UnsanctionedRuntimeError(
"Unsanctioned keychain/credential fill blocked (#558). "
"Unsanctioned keychain/credential fill blocked (#558/#695). "
"Token extraction via git-credential is only allowed inside the "
f"official MCP daemon ({SANCTIONED_DAEMON_ENV}=1), pytest, or with "
f"explicit operator opt-in {ALLOW_KEYCHAIN_CLI_ENV}=1."
f"official native MCP daemon or with explicit operator opt-in "
f"{ALLOW_KEYCHAIN_CLI_ENV}=1 (never for offline mutation runners)."
)
def runtime_status() -> dict[str, Any]:
def native_runtime_status() -> dict[str, Any]:
"""LLM-safe native runtime status (no raw token)."""
rt = _NATIVE_RUNTIME or {}
return {
"sanctioned_daemon": is_sanctioned_mcp_daemon(),
"native_mcp_transport": is_native_mcp_transport(),
"pytest": is_pytest_runtime(),
"pid": rt.get("pid"),
"token_fingerprint": rt.get("token_fingerprint"),
"started_at": rt.get("started_at"),
"entrypoint": rt.get("entrypoint"),
"env_sanctioned_alone_insufficient": True,
"sanctioned_env": SANCTIONED_DAEMON_ENV,
"allow_direct_import_env": ALLOW_DIRECT_IMPORT_ENV,
"allow_keychain_cli_env": ALLOW_KEYCHAIN_CLI_ENV,
}
def runtime_status() -> dict[str, Any]:
"""Backward-compatible status payload."""
status = native_runtime_status()
status["sanctioned_daemon"] = is_sanctioned_mcp_daemon()
return status
def mutation_provenance_fields() -> dict[str, Any]:
"""Fields to attach to live mutation / review audit records (#695 AC6)."""
st = native_runtime_status()
return {
"transport": "native_mcp" if st["native_mcp_transport"] else "untrusted",
"native_mcp_transport": bool(st["native_mcp_transport"]),
"native_runtime_pid": st.get("pid"),
"native_token_fingerprint": st.get("token_fingerprint"),
"entrypoint": st.get("entrypoint"),
}
+43 -9
View File
@@ -1,7 +1,8 @@
"""Merge approval must pin the current PR head SHA (#471).
"""Merge approval must pin the current PR head SHA (#471 / #695).
Formal APPROVED reviews that predate the live PR head must not satisfy
``gitea_merge_pr`` eligibility. Pure assessment helpers are isolated here
``gitea_merge_pr`` eligibility. Contaminated / quarantined approvals (#695)
must not authorize merge either. Pure assessment helpers are isolated here
for hermetic unit tests apart from MCP HTTP calls.
"""
@@ -12,6 +13,7 @@ def assess_merge_approval_head(
*,
current_head_sha: str | None,
latest_by_reviewer: dict,
quarantined_review_ids: set | None = None,
) -> dict:
"""Return whether a visible approval applies to the live PR head.
@@ -19,18 +21,43 @@ def assess_merge_approval_head(
current_head_sha: Current PR head commit SHA.
latest_by_reviewer: Map of reviewer login → review entry dicts with
``verdict``, ``dismissed``, and ``reviewed_head_sha`` keys.
Optional ``review_id`` / ``id`` used for quarantine checks (#695).
quarantined_review_ids: Optional set of review IDs that must not count
toward merge authorization (#695).
Returns:
dict with ``approval_at_current_head``, ``latest_approved_head_sha``,
and ``stale_approval_block_reason`` (set when merge must fail closed).
"""
current = (current_head_sha or "").strip()
approved_entries = [
entry
for entry in (latest_by_reviewer or {}).values()
if (entry.get("verdict") or "").upper() == "APPROVED"
and not entry.get("dismissed")
]
blocked_ids = {int(x) for x in (quarantined_review_ids or set()) if x is not None}
def _rid(entry: dict):
raw = entry.get("review_id", entry.get("id"))
try:
return int(raw) if raw is not None else None
except (TypeError, ValueError):
return None
approved_entries = []
quarantined_at_head = []
for entry in (latest_by_reviewer or {}).values():
if (entry.get("verdict") or "").upper() != "APPROVED":
continue
if entry.get("dismissed"):
continue
rid = _rid(entry)
head = (entry.get("reviewed_head_sha") or "").strip()
if rid is not None and rid in blocked_ids:
if current and head == current:
quarantined_at_head.append(entry)
continue
if entry.get("quarantined"):
if current and head == current:
quarantined_at_head.append(entry)
continue
approved_entries.append(entry)
at_current = any(
(entry.get("reviewed_head_sha") or "").strip() == current
for entry in approved_entries
@@ -47,7 +74,13 @@ def assess_merge_approval_head(
)[-1]
latest_approved = (latest_entry.get("reviewed_head_sha") or "").strip() or None
reason = None
if approved_entries and not at_current:
if quarantined_at_head and not at_current:
reason = (
"contaminated/quarantined approval at current head is void for "
"merge authorization (#695); required next action: fresh native "
"MCP re-review after controller quarantine evidence is recorded"
)
elif approved_entries and not at_current:
reason = (
f"stale approval: approved SHA '{latest_approved}' does not match "
f"current live PR head SHA '{current or '(unknown)'}' (fail closed); "
@@ -58,4 +91,5 @@ def assess_merge_approval_head(
"approval_at_current_head": at_current,
"latest_approved_head_sha": latest_approved,
"stale_approval_block_reason": reason,
"quarantined_approvals_at_current_head": len(quarantined_at_head),
}
+351
View File
@@ -0,0 +1,351 @@
"""Controller quarantine of contaminated formal reviews (#695).
Quarantine records are durable under the MCP session-state root and are only
writable when the caller holds a **native** MCP transport runtime. Untrusted
local scripts that import this module cannot establish a valid quarantine
(writes fail closed). Live server-side gates (eligibility, merge, feedback)
honor quarantine by review_id + PR + head.
Forensic evidence (Gitea review objects, lease comments) is never deleted.
"""
from __future__ import annotations
import json
import os
from datetime import datetime, timezone
from typing import Any
import mcp_daemon_guard
import mcp_session_state
KIND_QUARANTINE = "review_quarantine"
CONFIRMATION_PREFIX = "QUARANTINE CONTAMINATED REVIEW"
def _now() -> str:
return datetime.now(timezone.utc).isoformat()
def quarantine_state_path(
*,
remote: str,
org: str,
repo: str,
pr_number: int,
review_id: int,
) -> str:
# Dedicated subdir under session state root (mode 0o700).
root = os.path.join(mcp_session_state.default_state_dir(), "quarantine")
os.makedirs(root, mode=0o700, exist_ok=True)
# Explicit multi-segment key so remote/org/repo/pr/review cannot collide.
segs = [
KIND_QUARANTINE,
mcp_session_state._sanitize_segment(remote),
mcp_session_state._sanitize_segment(org),
mcp_session_state._sanitize_segment(repo),
f"pr{int(pr_number)}",
f"rev{int(review_id)}",
]
return os.path.join(root, "-".join(segs) + ".json")
def build_quarantine_record(
*,
remote: str,
org: str,
repo: str,
pr_number: int,
review_id: int,
reviewed_head_sha: str,
reason: str,
actor_username: str | None,
profile_name: str | None,
incident_issue: int | None = None,
forensic_comment_ids: list[int] | None = None,
) -> dict[str, Any]:
provenance = mcp_daemon_guard.mutation_provenance_fields()
return {
"kind": KIND_QUARANTINE,
"issue_ref": "#695",
"remote": remote,
"org": org,
"repo": repo,
"pr_number": int(pr_number),
"review_id": int(review_id),
"reviewed_head_sha": (reviewed_head_sha or "").strip().lower(),
"reason": (reason or "").strip(),
"actor_username": actor_username,
"profile_name": profile_name,
"incident_issue": incident_issue,
"forensic_comment_ids": list(forensic_comment_ids or []),
"created_at": _now(),
"native_provenance": provenance,
"merge_authorization": "void",
"retain_forensic_evidence": True,
}
def assess_quarantine_write(
*,
confirmation: str,
pr_number: int,
review_id: int,
reason: str,
native_required: bool = True,
) -> dict[str, Any]:
"""Pure assessment of whether a quarantine write may proceed."""
reasons: list[str] = []
expected = f"{CONFIRMATION_PREFIX} {int(review_id)} PR {int(pr_number)}"
if (confirmation or "").strip() != expected:
reasons.append(
f"confirmation must equal exactly '{expected}' (fail closed, #695)"
)
if not (reason or "").strip():
reasons.append("quarantine reason is required (fail closed, #695)")
if native_required and not mcp_daemon_guard.is_native_mcp_transport():
if not mcp_daemon_guard.is_pytest_runtime():
reasons.append(
"quarantine write requires native MCP transport; untrusted "
"local code cannot create quarantine records (#695)"
)
return {
"allowed": not reasons,
"expected_confirmation": expected,
"reasons": reasons,
}
def write_quarantine_record(record: dict[str, Any]) -> dict[str, Any]:
"""Persist quarantine record; fail closed outside native/pytest runtime."""
if not mcp_daemon_guard.is_native_mcp_transport():
if not mcp_daemon_guard.is_pytest_runtime():
raise mcp_daemon_guard.UnsanctionedRuntimeError(
"quarantine write blocked: non-native runtime (#695)"
)
path = quarantine_state_path(
remote=str(record["remote"]),
org=str(record["org"]),
repo=str(record["repo"]),
pr_number=int(record["pr_number"]),
review_id=int(record["review_id"]),
)
# Refuse overwriting with weaker provenance from untrusted caller by
# requiring native fields present.
if not (record.get("native_provenance") or {}).get("native_mcp_transport"):
if not mcp_daemon_guard.is_pytest_runtime():
raise mcp_daemon_guard.UnsanctionedRuntimeError(
"quarantine record missing native provenance (#695)"
)
tmp = path + ".tmp"
data = json.dumps(record, indent=2, sort_keys=True)
with open(tmp, "w", encoding="utf-8") as fh:
fh.write(data)
fh.flush()
os.fsync(fh.fileno())
os.replace(tmp, path)
os.chmod(path, 0o600)
return {"path": path, "written": True, "record": record}
def load_quarantine_record(
*,
remote: str,
org: str,
repo: str,
pr_number: int,
review_id: int,
) -> dict[str, Any] | None:
path = quarantine_state_path(
remote=remote,
org=org,
repo=repo,
pr_number=pr_number,
review_id=review_id,
)
if not os.path.isfile(path):
return None
try:
with open(path, encoding="utf-8") as fh:
data = json.load(fh)
except (OSError, json.JSONDecodeError):
return None
if not isinstance(data, dict):
return None
return data
def is_review_quarantined(
*,
remote: str,
org: str,
repo: str,
pr_number: int,
review_id: int | None,
reviewed_head_sha: str | None = None,
) -> dict[str, Any]:
"""Whether a formal review is quarantined for merge authorization (#695)."""
if review_id is None:
return {"quarantined": False, "record": None, "reasons": []}
rec = load_quarantine_record(
remote=remote,
org=org,
repo=repo,
pr_number=pr_number,
review_id=int(review_id),
)
if not rec:
return {"quarantined": False, "record": None, "reasons": []}
reasons = [
f"formal review_id {review_id} on PR #{pr_number} is quarantined "
f"(#695; incident issue {rec.get('incident_issue')}); "
"merge authorization is void; forensic evidence retained"
]
want_head = (reviewed_head_sha or "").strip().lower()
rec_head = (rec.get("reviewed_head_sha") or "").strip().lower()
if want_head and rec_head and want_head != rec_head:
# Still quarantined by review_id; note head mismatch for operators.
reasons.append(
f"quarantine head {rec_head[:12]}… differs from assessed head "
f"{want_head[:12]}… (still void by review_id)"
)
return {"quarantined": True, "record": rec, "reasons": reasons}
def filter_approvals_for_merge(
*,
remote: str,
org: str,
repo: str,
pr_number: int,
current_head_sha: str | None,
reviews: list[dict],
) -> dict[str, Any]:
"""Split reviews into merge-usable vs quarantined contaminated approvals."""
current = (current_head_sha or "").strip().lower()
usable: list[dict] = []
quarantined: list[dict] = []
for rev in reviews or []:
if not isinstance(rev, dict):
continue
verdict = (rev.get("verdict") or rev.get("state") or "").upper()
if verdict not in {"APPROVED", "APPROVE"}:
continue
if rev.get("dismissed"):
continue
rid = rev.get("review_id") or rev.get("id")
head = (
rev.get("reviewed_head_sha")
or rev.get("commit_id")
or rev.get("head_sha")
or ""
).strip().lower()
q = is_review_quarantined(
remote=remote,
org=org,
repo=repo,
pr_number=pr_number,
review_id=int(rid) if rid is not None else None,
reviewed_head_sha=head or current,
)
entry = {**rev, "review_id": rid, "reviewed_head_sha": head}
if q["quarantined"]:
entry["quarantined"] = True
entry["quarantine_reasons"] = q["reasons"]
quarantined.append(entry)
else:
entry["quarantined"] = False
usable.append(entry)
usable_at_head = [
e
for e in usable
if current and (e.get("reviewed_head_sha") or "") == current
]
return {
"usable_approvals": usable,
"usable_approvals_at_current_head": usable_at_head,
"quarantined_approvals": quarantined,
"approval_visible_for_merge": bool(usable_at_head),
"has_quarantined_approval_at_head": any(
current
and (e.get("reviewed_head_sha") or "") == current
for e in quarantined
),
}
def format_quarantine_audit_comment(record: dict[str, Any]) -> str:
"""Append-only forensic audit comment body (does not delete evidence)."""
lines = [
"## Contaminated formal review quarantine (#695)",
"",
"Status: **QUARANTINED — merge authorization VOID**",
"",
f"- review_id: `{record.get('review_id')}`",
f"- pr: `#{record.get('pr_number')}`",
f"- reviewed_head_sha: `{record.get('reviewed_head_sha')}`",
f"- actor: `{record.get('actor_username')}`",
f"- profile: `{record.get('profile_name')}`",
f"- incident_issue: `#{record.get('incident_issue')}`",
f"- reason: {record.get('reason')}",
f"- created_at: `{record.get('created_at')}`",
f"- native_transport: "
f"`{(record.get('native_provenance') or {}).get('native_mcp_transport')}`",
f"- native_token_fingerprint: "
f"`{(record.get('native_provenance') or {}).get('native_token_fingerprint')}`",
f"- forensic_comment_ids retained: "
f"`{record.get('forensic_comment_ids')}`",
"",
"This record does **not** delete Gitea reviews or historical comments. "
"Fresh native-MCP re-review is required before merge.",
]
return "\n".join(lines)
def assess_untrusted_canonical_approval_claim(body: str) -> list[str]:
"""Reasons to reject canonical comments that claim approved/merge-ready.
Used when the comment asserts approval without native review proof (#695 AC7).
False "official workflow" claims that cite offline/import paths are always
rejected even when a NATIVE_REVIEW_PROOF line is present.
"""
text = body or ""
lower = text.lower()
claims_approved = (
"state:\napproved" in lower
or "state: approved" in lower
or "who_is_next:\nmerger" in lower
or "who_is_next: merger" in lower
or "merge_ready: true" in lower
or "merge_ready:\ntrue" in lower
or "ready-to-merge" in lower
)
if not claims_approved:
return []
# Reject explicit offline/import "official workflow" spoofing (#695 AC9).
offline_spoof = (
"offline_mcp" in lower
or "offline import" in lower
or "direct import" in lower
or "import gitea_mcp_server" in lower
or "run_quarantine.py" in lower
or "offline_mcp_helper" in lower
or "offline_mcp_runner" in lower
)
has_proof = (
"NATIVE_REVIEW_PROOF:" in text
or "native_review_proof:" in lower
)
if offline_spoof:
return [
"canonical approval claim cites offline/import/helper path; "
"NATIVE_REVIEW_PROOF rejected (#695 AC7/AC9); stop after native "
"MCP failure — do not construct offline mutation fallbacks"
]
if has_proof:
return []
return [
"canonical comment claims approved/merge-ready state without "
"NATIVE_REVIEW_PROOF from a native MCP review mutation (#695 AC7); "
"contaminated or untrusted approvals cannot certify merger handoff"
]
@@ -14,6 +14,8 @@ before any PR mutation. Final report schema:
**BLOCKED + DIAGNOSE (universal default):** If at any point you cannot perform a required step (skill not available, terminal broken, capability missing, wrong profile, guard blocks, worktree misbound, instructions unavailable, preflight fails, etc.), STOP. Clearly state BLOCKED. Use the standard [`../templates/blocked-diagnose-report.md`](../templates/blocked-diagnose-report.md) template. Only safe non-mutating recovery. Report using the template. Do not continue or use any fallback (temp scripts, direct API, etc.) unless controller authorizes. All blocker classes listed in llm-project-workflow/SKILL.md must trigger this.
**Native MCP failure is a hard stop (#695):** If the native MCP namespace dies (EOF, capability disconnect, session death), STOP. Do **not** import `gitea_mcp_server` from a standalone process, do **not** run offline helpers (`offline_mcp_helper.py`, `offline_mcp_runner.py`, `run_quarantine.py`), and do **not** set direct-import / keychain-bypass / raw-token environment variables. Reconnect the official MCP daemon only. Contaminated approvals must be controller-quarantined; they never authorize merge.
**Default task prompt:**
> Review the next eligible open PR in this project. Merge it only if every
@@ -14,6 +14,8 @@ before any issue implementation mutation. Final report schema:
**BLOCKED + DIAGNOSE (universal default):** If at any point you cannot perform a required step (skill not available, terminal broken, capability missing, wrong profile, guard blocks, worktree misbound, instructions unavailable, preflight fails, etc.), STOP. Clearly state BLOCKED. Use the standard [`../templates/blocked-diagnose-report.md`](../templates/blocked-diagnose-report.md) template. Only safe non-mutating recovery. Report using the template. Do not continue or use any fallback (temp scripts, direct API, etc.) unless controller authorizes. All blocker classes listed in llm-project-workflow/SKILL.md must trigger this.
**Native MCP failure is a hard stop (#695):** Do not import MCP server internals, run offline mutation helpers, or set credential-bypass env vars after a native transport failure. Reconnect the official daemon only.
**Default task prompt:**
> Find the next eligible issue in this project, work on it only if all gates
+13 -457
View File
@@ -1,4 +1,4 @@
"""Stale / moot #332 review-decision lock detection and cleanup policy (#594/#620/#693).
"""Stale / moot #332 review-decision lock detection and cleanup policy (#594/#620).
#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,11 +12,6 @@ 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.
@@ -85,45 +80,17 @@ 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/#693).
"""True when prior live mutations block a new decision for PR+head (#620).
Historical terminals on a **different head of the same PR** do not block.
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.
Any prior mutation on another PR, the same head, or without a comparable
head SHA fails closed (blocks).
Head resolution uses ``mutation_head_sha(m, lock)`` so pre-#620 ledgers
that only stored ``ready_expected_head_sha`` still compare correctly
@@ -131,9 +98,7 @@ def prior_live_mutations_block_boundary(
"""
if not lock:
return False
if correction_applies(
lock, pr_number=pr_number, expected_head_sha=expected_head_sha
):
if lock.get("correction_authorized"):
return False
prior = list(lock.get("live_mutations") or [])
if not prior:
@@ -143,14 +108,10 @@ 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 (
target
m_pr == pr_number
and target
and m_head
and not heads_equal(m_head, target)
):
@@ -167,34 +128,23 @@ 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/#693).
"""Whether #332 hard-stop should yield for a new PR+head boundary (#620).
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).
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).
"""
if operation not in ("mark_ready", "review", "resume"):
return False
if not lock:
return False
if correction_applies(
lock, pr_number=pr_number, expected_head_sha=expected_head_sha
):
if lock.get("correction_authorized"):
return True
last = last_terminal_mutation(lock)
if last is None:
return True
last_pr = last.get("pr_number")
if last_pr is None:
if last.get("pr_number") != pr_number:
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:
@@ -488,397 +438,3 @@ 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,
}
+10 -17
View File
@@ -72,6 +72,16 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = {
"permission": "gitea.pr.merge",
"role": "merger",
},
# #695 AC8: controller quarantine of contaminated formal reviews.
# Apply path posts an append-only forensic audit comment (pr.comment).
"quarantine_contaminated_review": {
"permission": "gitea.pr.comment",
"role": "reconciler",
},
"gitea_quarantine_contaminated_review": {
"permission": "gitea.pr.comment",
"role": "reconciler",
},
"adopt_merger_pr_lease": {
"permission": "gitea.pr.comment",
"role": "reviewer",
@@ -117,23 +127,6 @@ 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",
+9 -5
View File
@@ -329,13 +329,17 @@ class TestGatedToolAudit(_AuditWiringBase):
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_merge_success_audited(self, _auth, mock_api):
# user, pr, feedback pr+reviews, merge POST, readback.
# user, pr, eligibility feedback pr+reviews (#695), gate-7 feedback
# pr+reviews, merge POST, readback.
approval = [{
"id": 1, "user": {"login": "reviewer-bot"}, "state": "APPROVED",
"commit_id": "abc123", "submitted_at": "2026-07-06T10:00:00Z",
"dismissed": False,
}]
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
self._pr("author-bot"),
[{"id": 1, "user": {"login": "reviewer-bot"}, "state": "APPROVED",
"commit_id": "abc123", "submitted_at": "2026-07-06T10:00:00Z",
"dismissed": False}],
self._pr("author-bot"), approval, # eligibility merge feedback
self._pr("author-bot"), approval, # gate 7 feedback
{}, {"merged_commit_sha": "c1"},
]
env = self._env(GITEA_PROFILE_NAME="gitea-merger",
@@ -94,6 +94,7 @@ REVIEW_STATUS: approved / approval_at_current_head
MERGE_READY: true
BLOCKERS: none
VALIDATION: pytest passed; reviewer approved at head {FULL_SHA}
NATIVE_REVIEW_PROOF: transport=native_mcp; entrypoint=mcp_server; token_fingerprint=testharmless
LAST_UPDATED_BY: prgs-reviewer
"""
+4 -11
View File
@@ -24,10 +24,6 @@ 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",
@@ -44,11 +40,9 @@ 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": mutations,
"live_mutations": list(mutations or []),
"correction_authorized": correction,
"correction_reason": "test" if correction else None,
"correction_pr_number": corr_pr,
"correction_head_sha": None,
"correction_reason": None,
}
@@ -180,13 +174,12 @@ class TestHardStopHeadScope(unittest.TestCase):
self.assertTrue(reasons)
self.assertIn("#332", reasons[0])
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."""
def test_rc_head_a_blocks_other_pr(self):
_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.assertEqual(reasons, [])
self.assertTrue(reasons)
def test_legacy_619_ledger_allows_new_head(self):
"""PR #619-style durable lock without mutation head_sha."""
@@ -1,427 +0,0 @@
"""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()
@@ -0,0 +1,400 @@
"""Regression tests for Issue #695 — second incident (PR #694 / review 427).
Reproduces offline import, env-only runtime spoof, exposed-token invocation,
direct imports, locally generated runtime keys, standalone quarantine attempts,
and false “official workflow” canonical claims. Gates must fail closed.
"""
from __future__ import annotations
import os
import tempfile
import unittest
from pathlib import Path
from unittest.mock import patch
import mcp_daemon_guard
import merge_approval_gate
import review_quarantine
import canonical_comment_validator as ccv
HEAD_694 = "1844e298809373be19a526fd39b7d8b0669eb5bd"
HEAD_OTHER = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
class TestNativeTransportBinding(unittest.TestCase):
def tearDown(self) -> None:
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ.pop(mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV, None)
os.environ.pop(mcp_daemon_guard.SANCTIONED_DAEMON_ENV, None)
os.environ.pop(mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, None)
def test_env_alone_does_not_establish_native_transport(self):
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ[mcp_daemon_guard.SANCTIONED_DAEMON_ENV] = "1"
os.environ[mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV] = "1"
os.environ[mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV] = "1"
self.assertFalse(mcp_daemon_guard.is_native_mcp_transport())
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError) as ctx:
mcp_daemon_guard.assert_sanctioned_mutation_runtime("offline_import")
msg = str(ctx.exception)
self.assertIn("#695", msg)
self.assertIn("not sufficient", msg.lower() + " " + msg)
def test_direct_import_mark_rejected_outside_entrypoint(self):
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ[mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV] = "1"
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError) as ctx:
mcp_daemon_guard.mark_sanctioned_daemon()
self.assertIn("mcp_server.py", str(ctx.exception))
self.assertFalse(mcp_daemon_guard.is_native_mcp_transport())
def test_locally_generated_runtime_key_without_entrypoint_rejected(self):
"""Spoofing process-local fields via mark outside entrypoint fails."""
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ[mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV] = "1"
# Even if a caller tries allow_test_bootstrap under force-unsanctioned
# pytest path is also forced off — only real entrypoint may mark.
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError):
mcp_daemon_guard.mark_sanctioned_daemon(allow_test_bootstrap=False)
def test_test_bootstrap_establishes_native_for_hermetic_tests(self):
mcp_daemon_guard.clear_native_runtime_for_tests()
st = mcp_daemon_guard.mark_sanctioned_daemon(allow_test_bootstrap=True)
self.assertTrue(st["native_mcp_transport"])
self.assertTrue(mcp_daemon_guard.is_native_mcp_transport())
mcp_daemon_guard.assert_sanctioned_mutation_runtime("test-bootstrap")
fields = mcp_daemon_guard.mutation_provenance_fields()
self.assertEqual(fields["transport"], "native_mcp")
self.assertTrue(fields["native_mcp_transport"])
self.assertIsNotNone(fields["native_token_fingerprint"])
def test_exposed_token_env_never_grants_native(self):
"""Raw / exposed token env vars must never reconstruct native transport."""
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ[mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV] = "1"
for key in (
"GITEA_TOKEN",
"GITEA_ACCESS_TOKEN",
"GITHUB_TOKEN",
"GITEA_MCP_TOKEN",
"GITEA_RAW_TOKEN",
):
os.environ[key] = "exposed-token-value-must-not-authorize"
try:
self.assertFalse(mcp_daemon_guard.is_native_mcp_transport())
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError):
mcp_daemon_guard.assert_sanctioned_mutation_runtime("exposed-token")
finally:
for key in (
"GITEA_TOKEN",
"GITEA_ACCESS_TOKEN",
"GITHUB_TOKEN",
"GITEA_MCP_TOKEN",
"GITEA_RAW_TOKEN",
):
os.environ.pop(key, None)
class TestQuarantineWriteNativeOnly(unittest.TestCase):
def setUp(self) -> None:
self._tmp = tempfile.TemporaryDirectory()
self.addCleanup(self._tmp.cleanup)
mcp_daemon_guard.clear_native_runtime_for_tests()
def tearDown(self) -> None:
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ.pop(mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV, None)
def test_standalone_quarantine_write_blocked_when_unsanctioned(self):
os.environ[mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV] = "1"
assessment = review_quarantine.assess_quarantine_write(
confirmation="QUARANTINE CONTAMINATED REVIEW 427 PR 694",
pr_number=694,
review_id=427,
reason="contaminated offline approval",
native_required=True,
)
self.assertFalse(assessment["allowed"])
self.assertTrue(
any("native MCP transport" in r for r in assessment["reasons"])
)
record = review_quarantine.build_quarantine_record(
remote="prgs",
org="Scaled-Tech-Consulting",
repo="Gitea-Tools",
pr_number=694,
review_id=427,
reviewed_head_sha=HEAD_694,
reason="contaminated",
actor_username="sysadmin",
profile_name="prgs-merger",
incident_issue=695,
forensic_comment_ids=[10883, 10886],
)
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError):
review_quarantine.write_quarantine_record(record)
def test_confirmation_must_match_exactly(self):
assessment = review_quarantine.assess_quarantine_write(
confirmation="quarantine 427",
pr_number=694,
review_id=427,
reason="x",
native_required=False,
)
self.assertFalse(assessment["allowed"])
self.assertIn("confirmation must equal exactly", assessment["reasons"][0])
def test_quarantine_honored_by_merge_approval_gate(self):
"""Contaminated review 427 at head must not authorize merge (#695)."""
result = merge_approval_gate.assess_merge_approval_head(
current_head_sha=HEAD_694,
latest_by_reviewer={
"sysadmin": {
"verdict": "APPROVED",
"dismissed": False,
"reviewed_head_sha": HEAD_694,
"review_id": 427,
"submitted_at": "2026-07-13T07:20:00Z",
}
},
quarantined_review_ids={427},
)
self.assertFalse(result["approval_at_current_head"])
self.assertIn("quarantined", result["stale_approval_block_reason"])
self.assertEqual(result["quarantined_approvals_at_current_head"], 1)
def test_filter_approvals_for_merge_splits_quarantined(self):
with patch(
"review_quarantine.mcp_session_state.default_state_dir",
return_value=self._tmp.name,
):
mcp_daemon_guard.mark_sanctioned_daemon(allow_test_bootstrap=True)
record = review_quarantine.build_quarantine_record(
remote="prgs",
org="org",
repo="repo",
pr_number=694,
review_id=427,
reviewed_head_sha=HEAD_694,
reason="offline import contaminated approval",
actor_username="sysadmin",
profile_name="prgs-merger",
incident_issue=695,
)
# Force native provenance bit for write path under test bootstrap.
record["native_provenance"] = {
**record["native_provenance"],
"native_mcp_transport": True,
}
review_quarantine.write_quarantine_record(record)
filtered = review_quarantine.filter_approvals_for_merge(
remote="prgs",
org="org",
repo="repo",
pr_number=694,
current_head_sha=HEAD_694,
reviews=[
{
"verdict": "APPROVED",
"review_id": 427,
"reviewed_head_sha": HEAD_694,
"reviewer": "sysadmin",
},
{
"verdict": "APPROVED",
"review_id": 999,
"reviewed_head_sha": HEAD_694,
"reviewer": "fresh-reviewer",
},
],
)
self.assertTrue(filtered["has_quarantined_approval_at_head"])
self.assertEqual(len(filtered["quarantined_approvals"]), 1)
self.assertEqual(filtered["quarantined_approvals"][0]["review_id"], 427)
self.assertEqual(len(filtered["usable_approvals_at_current_head"]), 1)
self.assertEqual(
filtered["usable_approvals_at_current_head"][0]["review_id"], 999
)
self.assertTrue(filtered["approval_visible_for_merge"])
class TestCanonicalHandoffValidation(unittest.TestCase):
def test_false_official_workflow_offline_claim_rejected(self):
body = f"""## Canonical PR State
STATE: approved
WHO_IS_NEXT: merger
NEXT_ACTION: Merge PR #694 immediately after offline helper success
NEXT_PROMPT:
```text
Merger: land PR #694; offline_mcp_runner completed official workflow.
```
WHAT_HAPPENED: offline import of gitea_mcp_server submitted APPROVED review 427
WHY: claimed official workflow via direct import after native EOF
ISSUE: #693
HEAD_SHA: {HEAD_694}
REVIEW_STATUS: approved / approval_at_current_head
MERGE_READY: true
BLOCKERS: none
VALIDATION: offline_mcp_helper.py + offline_mcp_runner.py; import gitea_mcp_server
NATIVE_REVIEW_PROOF: transport=offline_mcp; spoofed
LAST_UPDATED_BY: contaminated-session
"""
result = ccv.assess_canonical_comment(body, context="pr_comment")
self.assertFalse(result["allowed"])
joined = " ".join(result.get("extra_reasons") or [])
self.assertIn("#695", joined)
self.assertIn("offline", joined.lower())
def test_merge_ready_without_native_proof_rejected(self):
body = f"""## Canonical PR State
STATE: ready-to-merge
WHO_IS_NEXT: merger
NEXT_ACTION: Merge PR after confirming approval_at_current_head
NEXT_PROMPT:
```text
Merge PR #694 for issue #693 after live mergeable check passes.
```
WHAT_HAPPENED: Reviewer approved at current head
WHY: All gates passed and head SHA is current
ISSUE: #693
HEAD_SHA: {HEAD_694}
REVIEW_STATUS: approved / approval_at_current_head
MERGE_READY: true
BLOCKERS: none
VALIDATION: pytest passed; reviewer approved at head {HEAD_694}
LAST_UPDATED_BY: prgs-reviewer
"""
result = ccv.assess_canonical_comment(body, context="pr_comment")
self.assertFalse(result["allowed"])
joined = " ".join(result.get("extra_reasons") or [])
self.assertIn("NATIVE_REVIEW_PROOF", joined)
def test_merge_ready_with_native_proof_allowed(self):
body = f"""## Canonical PR State
STATE: ready-to-merge
WHO_IS_NEXT: merger
NEXT_ACTION: Merge PR after confirming approval_at_current_head
NEXT_PROMPT:
```text
Merge PR #500 for issue #496 after live mergeable check passes.
```
WHAT_HAPPENED: Reviewer approved at current head via native MCP
WHY: All gates passed and head SHA is current
ISSUE: #496
HEAD_SHA: {HEAD_694}
REVIEW_STATUS: approved / approval_at_current_head
MERGE_READY: true
BLOCKERS: none
VALIDATION: pytest passed; reviewer approved at head {HEAD_694}
NATIVE_REVIEW_PROOF: transport=native_mcp; entrypoint=mcp_server; token_fingerprint=abc123
LAST_UPDATED_BY: prgs-reviewer
"""
result = ccv.assess_canonical_comment(body, context="pr_comment")
self.assertTrue(result["allowed"], result)
class TestFeedbackQuarantineIntegration(unittest.TestCase):
"""gitea_get_pr_review_feedback must void quarantined review 427 at head."""
def setUp(self) -> None:
self._tmp = tempfile.TemporaryDirectory()
self.addCleanup(self._tmp.cleanup)
mcp_daemon_guard.clear_native_runtime_for_tests()
mcp_daemon_guard.mark_sanctioned_daemon(allow_test_bootstrap=True)
def tearDown(self) -> None:
mcp_daemon_guard.clear_native_runtime_for_tests()
def test_feedback_excludes_quarantined_approval_from_merge_auth(self):
import mcp_server
with patch(
"review_quarantine.mcp_session_state.default_state_dir",
return_value=self._tmp.name,
):
record = review_quarantine.build_quarantine_record(
remote="prgs",
org="Scaled-Tech-Consulting",
repo="Gitea-Tools",
pr_number=694,
review_id=427,
reviewed_head_sha=HEAD_694,
reason="contaminated offline approval (incident #695)",
actor_username="controller",
profile_name="prgs-merger",
incident_issue=695,
forensic_comment_ids=[10883, 10886],
)
record["native_provenance"]["native_mcp_transport"] = True
review_quarantine.write_quarantine_record(record)
def _api(method, url, auth=None, payload=None):
if url.endswith("/pulls/694") and method == "GET":
return {
"number": 694,
"state": "open",
"head": {"sha": HEAD_694},
"user": {"login": "jcwalker3"},
}
if url.endswith("/reviews"):
return [
{
"id": 427,
"user": {"login": "sysadmin"},
"state": "APPROVED",
"body": "contaminated",
"submitted_at": "2026-07-13T07:20:00Z",
"commit_id": HEAD_694,
"dismissed": False,
"stale": False,
}
]
return {}
with patch("mcp_server.api_request", side_effect=_api):
with patch(
"mcp_server.get_auth_header", return_value="Basic dGVzdA=="
):
with patch(
"mcp_server.get_profile",
return_value={
"profile_name": "prgs-merger",
"allowed_operations": ["gitea.read", "gitea.pr.merge"],
"forbidden_operations": [],
"base_url": None,
},
):
result = mcp_server.gitea_get_pr_review_feedback(
pr_number=694,
remote="prgs",
org="Scaled-Tech-Consulting",
repo="Gitea-Tools",
)
self.assertTrue(result.get("success"), result)
self.assertFalse(result.get("approval_at_current_head"))
self.assertFalse(result.get("approval_visible"))
self.assertIn(427, result.get("quarantined_review_ids") or [])
self.assertGreaterEqual(
result.get("quarantined_approvals_at_current_head") or 0, 1
)
self.assertIn("quarantined", (result.get("stale_approval_block_reason") or ""))
class TestDocsStopAfterNativeFailure(unittest.TestCase):
def test_daemon_guard_doc_requires_stop(self):
root = Path(__file__).resolve().parent.parent
doc = (root / "docs" / "mcp-daemon-import-guard.md").read_text(encoding="utf-8")
self.assertIn("#695", doc)
self.assertIn("STOP after native MCP failure", doc)
self.assertIn("offline_mcp_runner", doc)
self.assertIn("run_quarantine.py", doc)
self.assertIn("GITEA_ALLOW_DIRECT_MCP_IMPORT", doc)
if __name__ == "__main__":
unittest.main()
+26 -4
View File
@@ -1,4 +1,4 @@
"""Tests for sanctioned MCP daemon guards (#558)."""
"""Tests for sanctioned MCP daemon guards (#558 / #695)."""
from __future__ import annotations
@@ -11,6 +11,10 @@ import gitea_auth
class TestMcpDaemonGuard(unittest.TestCase):
def tearDown(self) -> None:
mcp_daemon_guard.clear_native_runtime_for_tests()
os.environ.pop(mcp_daemon_guard.FORCE_PROVENANCE_FAIL_ENV, None)
def test_unsanctioned_blocks_mutation_runtime(self):
env = {k: v for k, v in os.environ.items() if k not in {
mcp_daemon_guard.SANCTIONED_DAEMON_ENV,
@@ -26,11 +30,29 @@ class TestMcpDaemonGuard(unittest.TestCase):
# Running under pytest already sets PYTEST_CURRENT_TEST.
mcp_daemon_guard.assert_sanctioned_mutation_runtime("pytest")
def test_mark_sanctioned_allows(self):
env = {k: v for k, v in os.environ.items() if k != "PYTEST_CURRENT_TEST"}
def test_mark_sanctioned_bootstrap_allows(self):
mcp_daemon_guard.clear_native_runtime_for_tests()
mcp_daemon_guard.mark_sanctioned_daemon(allow_test_bootstrap=True)
mcp_daemon_guard.assert_sanctioned_mutation_runtime("daemon")
self.assertTrue(mcp_daemon_guard.is_native_mcp_transport())
def test_env_alone_insufficient_when_force_unsanctioned(self):
mcp_daemon_guard.clear_native_runtime_for_tests()
env = {
k: v
for k, v in os.environ.items()
if k not in {
mcp_daemon_guard.SANCTIONED_DAEMON_ENV,
mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV,
"PYTEST_CURRENT_TEST",
}
}
env[mcp_daemon_guard.SANCTIONED_DAEMON_ENV] = "1"
env["GITEA_TEST_FORCE_UNSANCTIONED"] = "1"
with patch.dict(os.environ, env, clear=True):
mcp_daemon_guard.assert_sanctioned_mutation_runtime("daemon")
with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError) as ctx:
mcp_daemon_guard.assert_sanctioned_mutation_runtime("env-spoof")
self.assertIn("not sufficient", str(ctx.exception))
def test_keychain_blocked_without_sanction(self):
env = {
+39 -36
View File
@@ -836,16 +836,24 @@ class TestMergePR(unittest.TestCase):
)
def _feedback_reads(self, author="author-bot", sha="abc123"):
"""PR + reviews GETs for gitea_get_pr_review_feedback during merge."""
"""PR + reviews GETs for one gitea_get_pr_review_feedback call."""
return [self._pr(author, sha=sha), _visible_approval_reviews(sha=sha)]
def _eligibility_merge_reads(self, author="author-bot", sha="abc123"):
"""Eligibility user/PR plus #695 merge-approval feedback PR+reviews."""
return [
{"login": "merger-bot"},
self._pr(author, sha=sha),
*self._feedback_reads(author=author, sha=sha),
]
# -- success --------------------------------------------------------------
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_merge_succeeds_when_all_gates_pass(self, _auth, mock_api):
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
*self._feedback_reads(),
{}, # merge POST
{"merged_commit_sha": "mergecommit99"}, # read-back
@@ -864,7 +872,7 @@ class TestMergePR(unittest.TestCase):
self.assertEqual(r["merge_method"], "squash")
self.assertEqual(r["merge_commit"], "mergecommit99")
# 5th call is the merge POST with the requested method/title/message.
merge_call = mock_api.call_args_list[4]
merge_call = mock_api.call_args_list[6]
self.assertEqual(merge_call.args[0], "POST")
self.assertTrue(merge_call.args[1].endswith("/pulls/8/merge"))
payload = merge_call.args[3]
@@ -877,7 +885,7 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_expected_changed_files_match_allows(self, _auth, mock_api):
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
[{"filename": "a.py"}, {"filename": "b.py"}], # files
*self._feedback_reads(),
{}, # merge POST
@@ -899,7 +907,7 @@ class TestMergePR(unittest.TestCase):
def test_readback_failure_reports_skipped_cleanup(self, _auth, mock_api):
"""Merge OK + read-back GET failure => explicit cleanup skip, not silence."""
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
*self._feedback_reads(),
{}, # merge POST
RuntimeError("HTTP 502: Gitea upstream unavailable"), # read-back fails
@@ -919,7 +927,7 @@ class TestMergePR(unittest.TestCase):
self.assertEqual(r["cleanup_status"], "skipped (merge read-back failed)")
# No tracker-cleanup API traffic after the failed read-back:
# user, PR (eligibility), feedback PR+reviews, merge POST, read-back.
self.assertEqual(mock_api.call_count, 6)
self.assertEqual(mock_api.call_count, 8)
for c in mock_api.call_args_list:
self.assertNotEqual(c.args[0], "DELETE")
@@ -930,7 +938,7 @@ class TestMergePR(unittest.TestCase):
def test_cleanup_exception_surfaced_and_redacted(self, _auth, mock_api, _cleanup):
"""Unexpected cleanup exception => merge still succeeds; error surfaced redacted."""
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
*self._feedback_reads(),
{}, # merge POST
{"merged_commit_sha": "c9"}, # read-back OK
@@ -1142,8 +1150,10 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_head_sha_mismatch_blocks(self, _auth, mock_api):
# Eligibility (#695) needs approval feedback before head-gate runs.
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot", sha="abc123")]
*self._eligibility_merge_reads(sha="abc123"),
]
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
@@ -1162,7 +1172,7 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_changed_files_mismatch_blocks(self, _auth, mock_api):
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
[{"filename": "a.py"}, {"filename": "c.py"}], # actual files
]
env = {"GITEA_PROFILE_NAME": "gitea-merger",
@@ -1193,7 +1203,7 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_output_redacts_secrets(self, _auth, mock_api):
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
*self._feedback_reads(),
{}, {"merged_commit_sha": "c1"},
]
@@ -1213,7 +1223,7 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_merge_error_message_redacts_credential(self, _auth, mock_api):
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
*self._eligibility_merge_reads(),
*self._feedback_reads(),
RuntimeError("HTTP 500: token abc-secret-xyz rejected"),
]
@@ -1234,6 +1244,7 @@ class TestMergePR(unittest.TestCase):
def test_merge_blocked_on_stale_approval_head(self, _auth, mock_api):
old_sha = "8b61c4b41f1b49b271ed3b99657431cf06eeda3e"
new_sha = "3e4b721d60e97147ba0704773cf57cd0d42cbe31"
# #695: eligibility itself now fails closed on stale approval head.
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot", sha=new_sha),
self._pr("author-bot", sha=new_sha),
@@ -1256,6 +1267,7 @@ class TestMergePR(unittest.TestCase):
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_merge_blocked_without_visible_approval(self, _auth, mock_api):
# #695: eligibility denies merge when no non-quarantined APPROVED review.
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
self._pr("author-bot"),
@@ -1270,12 +1282,21 @@ class TestMergePR(unittest.TestCase):
)
self.assertFalse(r["performed"])
self.assertFalse(r.get("approval_visible"))
self.assertTrue(any("no visible APPROVED review" in x for x in r["reasons"]))
self.assertTrue(
any(
"no non-quarantined APPROVED review" in x
or "no visible APPROVED review" in x
or "merge eligibility denied" in x
for x in r["reasons"]
),
msg=r["reasons"],
)
self._assert_no_merge_call(mock_api)
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_merge_blocked_on_request_changes(self, _auth, mock_api):
# #695: eligibility denies merge on undismissed REQUEST_CHANGES.
mock_api.side_effect = [
{"login": "merger-bot"}, self._pr("author-bot"),
self._pr("author-bot"),
@@ -2540,18 +2561,15 @@ 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)
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,
)
r = gitea_authorize_review_correction(
prior_review_id=42,
prior_review_state="approve",
reason="typo",
operator_authorized=True,
)
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
@@ -2565,7 +2583,6 @@ 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"]))
@@ -2576,22 +2593,10 @@ 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 {}
@@ -2604,7 +2609,6 @@ 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"]))
@@ -2662,7 +2666,6 @@ 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(
+20
View File
@@ -54,6 +54,26 @@ class TestMergeApprovalGate(unittest.TestCase):
self.assertFalse(result["approval_at_current_head"])
self.assertIsNone(result["latest_approved_head_sha"])
def test_quarantined_approval_void_for_merge(self):
"""#695: contaminated formal review at head must not authorize merge."""
result = assess_merge_approval_head(
current_head_sha=HEAD_NEW,
latest_by_reviewer={
"sysadmin": {
"verdict": "APPROVED",
"dismissed": False,
"reviewed_head_sha": HEAD_NEW,
"review_id": 427,
"submitted_at": "2026-07-13T07:20:00Z",
}
},
quarantined_review_ids={427},
)
self.assertFalse(result["approval_at_current_head"])
self.assertEqual(result["quarantined_approvals_at_current_head"], 1)
self.assertIn("quarantined", result["stale_approval_block_reason"])
self.assertIn("#695", result["stale_approval_block_reason"])
if __name__ == "__main__":
unittest.main()
+14 -1
View File
@@ -206,7 +206,20 @@ class TestEligibilityNormalizesOperations(unittest.TestCase):
def test_namespaced_profile_ops_allow_legacy_action(self, _auth, mock_api):
# JSON-config profiles carry canonical namespaced ops; the raw action
# "merge" must still match them after normalization.
mock_api.side_effect = [{"login": "merger-bot"}, self._pr("author-bot")]
# #695: merge eligibility also loads quarantine-aware review feedback.
mock_api.side_effect = [
{"login": "merger-bot"},
self._pr("author-bot"),
self._pr("author-bot"),
[{
"id": 1,
"user": {"login": "reviewer-bot"},
"state": "APPROVED",
"commit_id": "abc123",
"submitted_at": "2026-07-06T10:00:00Z",
"dismissed": False,
}],
]
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.pr.merge"}
with patch.dict(os.environ, env, clear=True):
@@ -188,32 +188,20 @@ class TestActiveHardStopPreserved(unittest.TestCase):
mcp_server._save_review_decision_lock(None)
mcp_server.review_workflow_load.clear_review_workflow_load()
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_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_same_pr(self):
_seed([RC_OPEN]) # request_changes on PR #701
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(701, op),
mcp_server.terminal_review_hard_stop_reasons(592, op),
msg=op,
)
# Foreign PR review path is open (#693).
self.assertEqual(
mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready"),
[],
)
# --------------------------------------------------------------------------- #
+22 -54
View File
@@ -1,10 +1,10 @@
"""Tests for the session hard-stop after a terminal review mutation (#332/#693).
"""Tests for the session hard-stop after a terminal review mutation (#332).
Extends the single-terminal-decision machinery (#211): after a live
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.
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.
"""
import os
import unittest
@@ -15,10 +15,7 @@ import mcp_server
HEAD_SHA = "a" * 40
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")
def _lock(mutations=None, correction=False):
return {
"task": "review_pr",
"remote": "prgs",
@@ -32,11 +29,9 @@ def _lock(mutations=None, correction=False, correction_pr=None):
"ready_remote": None,
"ready_org": None,
"ready_repo": None,
"live_mutations": mutations,
"live_mutations": list(mutations or []),
"correction_authorized": correction,
"correction_reason": "test" if correction else None,
"correction_pr_number": correction_pr if correction else None,
"correction_head_sha": None,
"correction_reason": None,
}
@@ -69,35 +64,25 @@ class TestTerminalHardStopReasons(unittest.TestCase):
self.assertEqual(
mcp_server.terminal_review_hard_stop_reasons(5, "merge"), [])
def test_request_changes_blocks_same_pr_allows_other_pr_review(self):
def test_request_changes_blocks_everything(self):
_seed([RC_A])
# Same PR: still hard-stopped for mark_ready/review/merge.
for op in ("merge", "mark_ready", "review"):
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"))
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])
def test_approve_allows_same_pr_merge_and_other_pr_review(self):
def test_approve_allows_same_pr_merge_only(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"))
# #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"), [])
self.assertTrue(
mcp_server.terminal_review_hard_stop_reasons(6, "mark_ready"))
self.assertTrue(
mcp_server.terminal_review_hard_stop_reasons(6, "review"))
def test_correction_reopens_review_path_only(self):
_seed([RC_A], correction=True)
@@ -105,11 +90,9 @@ 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"), [])
# scoped correction never opens a cross-PR merge
# 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):
@@ -143,29 +126,14 @@ class TestMarkFinalHardStopWiring(unittest.TestCase):
mcp_server._save_review_decision_lock(None)
mcp_server.review_workflow_load.clear_review_workflow_load()
def test_mark_ready_same_pr_blocked_after_terminal_mutation(self):
def test_mark_ready_blocked_after_terminal_mutation(self):
_seed([RC_A])
result = mcp_server.gitea_mark_final_review_decision(
pr_number=5, action="approve", remote="prgs",
expected_head_sha=HEAD_SHA)
pr_number=6, action="approve", remote="prgs")
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 {