fix: resolve conflicts for issue 324
This commit is contained in:
@@ -1764,6 +1764,51 @@ def record_live_review_mutation(pr_number: int, action: str, review_id: int | No
|
|||||||
_save_review_decision_lock(lock)
|
_save_review_decision_lock(lock)
|
||||||
|
|
||||||
|
|
||||||
|
def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[str]:
|
||||||
|
"""Session hard-stop after a terminal live review mutation (#332).
|
||||||
|
|
||||||
|
After a terminal verdict is consumed in this run, the only permitted
|
||||||
|
continuation is the merge sequence for the same PR that was approved.
|
||||||
|
A REQUEST_CHANGES (or an approval of a different PR) blocks every
|
||||||
|
further review/mark-ready/merge mutation. An operator-approved
|
||||||
|
correction (#211) re-opens the review path only — never a cross-PR
|
||||||
|
merge.
|
||||||
|
|
||||||
|
*operation* is one of 'merge', 'mark_ready', or 'review'.
|
||||||
|
Returns [] when the operation may proceed.
|
||||||
|
"""
|
||||||
|
lock = _load_review_decision_lock()
|
||||||
|
if lock is None:
|
||||||
|
return []
|
||||||
|
terminals = [
|
||||||
|
m for m in (lock.get("live_mutations") or [])
|
||||||
|
if m.get("action") in _TERMINAL_REVIEW_ACTIONS
|
||||||
|
]
|
||||||
|
if not terminals:
|
||||||
|
return []
|
||||||
|
last = terminals[-1]
|
||||||
|
if (
|
||||||
|
operation == "merge"
|
||||||
|
and last.get("action") == "approve"
|
||||||
|
and last.get("pr_number") == pr_number
|
||||||
|
):
|
||||||
|
return []
|
||||||
|
if operation in ("mark_ready", "review") and lock.get("correction_authorized"):
|
||||||
|
return []
|
||||||
|
if last.get("action") == "approve":
|
||||||
|
guidance = (
|
||||||
|
f"only the merge sequence for approved PR "
|
||||||
|
f"#{last.get('pr_number')} may continue"
|
||||||
|
)
|
||||||
|
else:
|
||||||
|
guidance = "the session must stop and produce a final report"
|
||||||
|
return [
|
||||||
|
"terminal review mutation already consumed in this run "
|
||||||
|
f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} "
|
||||||
|
"(fail closed, #332)"
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
# Patterns scrubbed from any surfaced error text so a credential can never leak.
|
# Patterns scrubbed from any surfaced error text so a credential can never leak.
|
||||||
_SECRET_PREFIXES = ("token ", "Basic ")
|
_SECRET_PREFIXES = ("token ", "Basic ")
|
||||||
|
|
||||||
@@ -2179,6 +2224,9 @@ def gitea_mark_final_review_decision(
|
|||||||
}
|
}
|
||||||
org = resolved_org
|
org = resolved_org
|
||||||
repo = resolved_repo
|
repo = resolved_repo
|
||||||
|
hard_stop = terminal_review_hard_stop_reasons(pr_number, "mark_ready")
|
||||||
|
if hard_stop:
|
||||||
|
return {"marked_ready": False, "reasons": hard_stop}
|
||||||
if lock.get("live_mutations") and not lock.get("correction_authorized"):
|
if lock.get("live_mutations") and not lock.get("correction_authorized"):
|
||||||
return {
|
return {
|
||||||
"marked_ready": False,
|
"marked_ready": False,
|
||||||
@@ -2196,6 +2244,33 @@ def gitea_mark_final_review_decision(
|
|||||||
f"{sorted(_REVIEW_ACTIONS)}"
|
f"{sorted(_REVIEW_ACTIONS)}"
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
|
if action == "request_changes":
|
||||||
|
# Duplicate request-changes suppression (#332): an unresolved
|
||||||
|
# REQUEST_CHANGES at the current head must not be duplicated.
|
||||||
|
feedback = gitea_get_pr_review_feedback(
|
||||||
|
pr_number, remote=remote, org=org, repo=repo)
|
||||||
|
if not feedback.get("success"):
|
||||||
|
return {
|
||||||
|
"marked_ready": False,
|
||||||
|
"reasons": [
|
||||||
|
"could not verify existing request-changes state for "
|
||||||
|
f"PR #{pr_number}; refusing to submit a possibly "
|
||||||
|
"duplicate REQUEST_CHANGES (fail closed, #332)"
|
||||||
|
],
|
||||||
|
}
|
||||||
|
if (
|
||||||
|
feedback.get("has_blocking_change_requests")
|
||||||
|
and not feedback.get("review_feedback_stale")
|
||||||
|
):
|
||||||
|
return {
|
||||||
|
"marked_ready": False,
|
||||||
|
"reasons": [
|
||||||
|
f"PR #{pr_number} already has an unresolved "
|
||||||
|
"REQUEST_CHANGES at the current head SHA; do not "
|
||||||
|
"duplicate the request-changes review (fail closed, "
|
||||||
|
"#332)"
|
||||||
|
],
|
||||||
|
}
|
||||||
lock["final_review_decision_ready"] = True
|
lock["final_review_decision_ready"] = True
|
||||||
lock["ready_pr_number"] = pr_number
|
lock["ready_pr_number"] = pr_number
|
||||||
lock["ready_action"] = action
|
lock["ready_action"] = action
|
||||||
@@ -2765,6 +2840,14 @@ def gitea_merge_pr(
|
|||||||
)
|
)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
|
# Gate 2b — session hard-stop after a terminal review mutation (#332):
|
||||||
|
# merge may only continue for the same PR that was just approved.
|
||||||
|
# Local in-process check; zero API calls.
|
||||||
|
hard_stop = terminal_review_hard_stop_reasons(pr_number, "merge")
|
||||||
|
if hard_stop:
|
||||||
|
reasons.extend(hard_stop)
|
||||||
|
return result
|
||||||
|
|
||||||
# Gate 3 — reuse #14 eligibility (identity + profile + merge-allowed +
|
# Gate 3 — reuse #14 eligibility (identity + profile + merge-allowed +
|
||||||
# author + self-merge block + open + mergeable/unknown fail-closed).
|
# author + self-merge block + open + mergeable/unknown fail-closed).
|
||||||
# Read-only GETs only.
|
# Read-only GETs only.
|
||||||
|
|||||||
+322
-1
@@ -1231,7 +1231,8 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
role_boundary=None, review_mutation=None,
|
role_boundary=None, review_mutation=None,
|
||||||
report_text=None, review_decision_lock=None,
|
report_text=None, review_decision_lock=None,
|
||||||
controller_handoff=None, capability_proof=None,
|
controller_handoff=None, capability_proof=None,
|
||||||
sweep_proof=None, worktree_proof=None):
|
sweep_proof=None, worktree_proof=None,
|
||||||
|
baseline_validation=None, project_root=None):
|
||||||
"""Required behavior 6 + acceptance criteria: one report, distinct proofs.
|
"""Required behavior 6 + acceptance criteria: one report, distinct proofs.
|
||||||
|
|
||||||
Combines the individual proof verdicts into the final-report fields the
|
Combines the individual proof verdicts into the final-report fields the
|
||||||
@@ -1258,6 +1259,18 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
)
|
)
|
||||||
|
|
||||||
empty_queue_report = assess_empty_queue_report(report_text)
|
empty_queue_report = assess_empty_queue_report(report_text)
|
||||||
|
if baseline_validation is None and report_text:
|
||||||
|
baseline_validation = assess_reviewer_baseline_validation_proof(
|
||||||
|
report_text=report_text,
|
||||||
|
project_root=project_root,
|
||||||
|
)
|
||||||
|
elif baseline_validation is None:
|
||||||
|
baseline_validation = {
|
||||||
|
"proven": True,
|
||||||
|
"block": False,
|
||||||
|
"reasons": [],
|
||||||
|
"violations": [],
|
||||||
|
}
|
||||||
|
|
||||||
contamination_status = contamination.get("status", "unknown")
|
contamination_status = contamination.get("status", "unknown")
|
||||||
checkout_proven = bool(checkout_proof.get("proven"))
|
checkout_proven = bool(checkout_proof.get("proven"))
|
||||||
@@ -1387,6 +1400,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
"empty-queue report missing or failed trust-gate proof (#198)"
|
"empty-queue report missing or failed trust-gate proof (#198)"
|
||||||
)
|
)
|
||||||
downgrade_reasons.extend(empty_queue_report.get("reasons", []))
|
downgrade_reasons.extend(empty_queue_report.get("reasons", []))
|
||||||
|
if not baseline_validation.get("proven"):
|
||||||
|
downgrade_reasons.append(
|
||||||
|
"reviewer baseline validation proof missing or failed (#325)"
|
||||||
|
)
|
||||||
|
downgrade_reasons.extend(baseline_validation.get("reasons", []))
|
||||||
|
|
||||||
merge_allowed = (
|
merge_allowed = (
|
||||||
identity_eligible
|
identity_eligible
|
||||||
@@ -1398,9 +1416,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
# #179: no merge without a proven final live-state recheck.
|
# #179: no merge without a proven final live-state recheck.
|
||||||
and live_state_proven
|
and live_state_proven
|
||||||
and worktree_proven
|
and worktree_proven
|
||||||
|
and baseline_validation.get("proven")
|
||||||
)
|
)
|
||||||
|
|
||||||
violations = []
|
violations = []
|
||||||
|
violations.extend(baseline_validation.get("violations", []))
|
||||||
if merge_performed and not merge_allowed:
|
if merge_performed and not merge_allowed:
|
||||||
violations.append(
|
violations.append(
|
||||||
"merge was performed/claimed although the proofs did not allow "
|
"merge was performed/claimed although the proofs did not allow "
|
||||||
@@ -1452,6 +1472,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
else True
|
else True
|
||||||
),
|
),
|
||||||
"empty_queue_trust_gate_status": empty_queue_report.get("status"),
|
"empty_queue_trust_gate_status": empty_queue_report.get("status"),
|
||||||
|
"baseline_validation_proven": bool(baseline_validation.get("proven")),
|
||||||
|
"baseline_validation_violations": list(
|
||||||
|
baseline_validation.get("violations") or []
|
||||||
|
),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@@ -3020,6 +3044,180 @@ def build_review_mutation_proof(run_log: list[dict]) -> dict:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Reviewer baseline validation (#325)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
_TEST_VALIDATION_COMMAND_RE = re.compile(
|
||||||
|
r"(?:^|\s)(?:venv/)?(?:bin/)?(?:python\s+-m\s+)?"
|
||||||
|
r"(?:pytest|make\s+test|npm\s+test|cargo\s+test|go\s+test\b)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
|
||||||
|
_PREEXISTING_FAILURE_CLAIM_RE = re.compile(
|
||||||
|
r"(?:\bsame\s+as\s+master\b|"
|
||||||
|
r"\bpre-?existing(?:\s+(?:on\s+)?master)?\b|"
|
||||||
|
r"\bfailures?\s+(?:are|is)\s+pre-?existing\b|"
|
||||||
|
r"\bmaster\s+also\s+fails?\b|"
|
||||||
|
r"\bfull-?suite\s+failures?\s+(?:are|is)\s+pre-?existing\b)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
|
||||||
|
_REPORT_VALIDATION_CWD_RE = re.compile(
|
||||||
|
r"(?:working\s+directory|cwd|directory)\s*:\s*(\S+)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _normalize_path(path: str) -> str:
|
||||||
|
return (path or "").replace("\\", "/").rstrip("/")
|
||||||
|
|
||||||
|
|
||||||
|
def _path_under_branches(path: str, project_root: str | None = None) -> bool:
|
||||||
|
"""True when *path* is inside the project's ``branches/`` directory."""
|
||||||
|
normalized = _normalize_path(path)
|
||||||
|
if not normalized:
|
||||||
|
return False
|
||||||
|
if "/branches/" in f"{normalized}/":
|
||||||
|
return True
|
||||||
|
if normalized.endswith("/branches"):
|
||||||
|
return True
|
||||||
|
if project_root:
|
||||||
|
root = _normalize_path(project_root)
|
||||||
|
if normalized.startswith(f"{root}/"):
|
||||||
|
rel = normalized[len(root) + 1 :]
|
||||||
|
return rel == "branches" or rel.startswith("branches/")
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def _is_test_validation_command(command: str) -> bool:
|
||||||
|
return bool(_TEST_VALIDATION_COMMAND_RE.search((command or "").strip()))
|
||||||
|
|
||||||
|
|
||||||
|
def _is_full_sha(value: str | None) -> bool:
|
||||||
|
return bool(value and _FULL_SHA.match((value or "").strip()))
|
||||||
|
|
||||||
|
|
||||||
|
def assess_reviewer_baseline_validation_proof(
|
||||||
|
*,
|
||||||
|
validation_runs: list[dict] | None = None,
|
||||||
|
baseline_proof: dict | None = None,
|
||||||
|
report_text: str | None = None,
|
||||||
|
project_root: str | None = None,
|
||||||
|
) -> dict:
|
||||||
|
"""#325: reviewer validation and baseline comparison must use ``branches/``.
|
||||||
|
|
||||||
|
*validation_runs* entries: ``command``, ``working_directory`` (or ``cwd``),
|
||||||
|
optional ``project_root``.
|
||||||
|
|
||||||
|
*baseline_proof* keys when claiming pre-existing master failures:
|
||||||
|
``worktree_path``, ``baseline_target_sha``, ``pr_head_sha``,
|
||||||
|
``baseline_failures``, ``pr_failures``, ``failure_signatures_match``,
|
||||||
|
``clean_before``, ``clean_after``.
|
||||||
|
"""
|
||||||
|
reasons: list[str] = []
|
||||||
|
violations: list[str] = []
|
||||||
|
text = report_text or ""
|
||||||
|
|
||||||
|
runs = list(validation_runs or [])
|
||||||
|
pending_command = None
|
||||||
|
for raw_line in text.splitlines():
|
||||||
|
line = raw_line.strip().lstrip("-*").strip()
|
||||||
|
lower = line.lower()
|
||||||
|
if lower.startswith("validation command:"):
|
||||||
|
pending_command = line.split(":", 1)[1].strip()
|
||||||
|
continue
|
||||||
|
cwd_match = _REPORT_VALIDATION_CWD_RE.search(line)
|
||||||
|
if cwd_match:
|
||||||
|
cwd = cwd_match.group(1).strip().rstrip(",.;")
|
||||||
|
command = pending_command or line
|
||||||
|
if _is_test_validation_command(command):
|
||||||
|
runs.append(
|
||||||
|
{
|
||||||
|
"command": command,
|
||||||
|
"working_directory": cwd,
|
||||||
|
"project_root": project_root,
|
||||||
|
}
|
||||||
|
)
|
||||||
|
pending_command = None
|
||||||
|
|
||||||
|
for run in runs:
|
||||||
|
command = (run.get("command") or "").strip()
|
||||||
|
if not command or not _is_test_validation_command(command):
|
||||||
|
continue
|
||||||
|
cwd = (run.get("working_directory") or run.get("cwd") or "").strip()
|
||||||
|
root = run.get("project_root") or project_root
|
||||||
|
if not cwd:
|
||||||
|
reasons.append(
|
||||||
|
"test validation command stated without working directory; "
|
||||||
|
"cannot prove branches-only execution (#325)"
|
||||||
|
)
|
||||||
|
continue
|
||||||
|
if not _path_under_branches(cwd, root):
|
||||||
|
violations.append(
|
||||||
|
f"test validation ran outside branches/ worktree: cwd={cwd!r}"
|
||||||
|
)
|
||||||
|
reasons.append(
|
||||||
|
"reviewer workflow must not run test suites in the main "
|
||||||
|
f"checkout; cwd {cwd!r} is not under branches/ (#325)"
|
||||||
|
)
|
||||||
|
|
||||||
|
claims_preexisting = bool(_PREEXISTING_FAILURE_CLAIM_RE.search(text))
|
||||||
|
if claims_preexisting:
|
||||||
|
proof = baseline_proof or {}
|
||||||
|
worktree = (proof.get("worktree_path") or "").strip()
|
||||||
|
if not worktree or not _path_under_branches(worktree, project_root):
|
||||||
|
reasons.append(
|
||||||
|
"pre-existing master failure claimed without a baseline "
|
||||||
|
"worktree path under branches/ (#325)"
|
||||||
|
)
|
||||||
|
if not _is_full_sha(proof.get("baseline_target_sha")):
|
||||||
|
reasons.append(
|
||||||
|
"pre-existing master failure claimed without "
|
||||||
|
"baseline_target_sha proof (#325)"
|
||||||
|
)
|
||||||
|
if not _is_full_sha(proof.get("pr_head_sha")):
|
||||||
|
reasons.append(
|
||||||
|
"pre-existing master failure claimed without pr_head_sha "
|
||||||
|
"proof (#325)"
|
||||||
|
)
|
||||||
|
baseline_failures = proof.get("baseline_failures")
|
||||||
|
pr_failures = proof.get("pr_failures")
|
||||||
|
if baseline_failures is None or pr_failures is None:
|
||||||
|
reasons.append(
|
||||||
|
"pre-existing master failure claimed without baseline and "
|
||||||
|
"PR failure listings (#325)"
|
||||||
|
)
|
||||||
|
if proof.get("failure_signatures_match") is not True:
|
||||||
|
reasons.append(
|
||||||
|
"pre-existing master failure claimed without proven matching "
|
||||||
|
"failure signatures (#325)"
|
||||||
|
)
|
||||||
|
if proof.get("clean_before") is not True:
|
||||||
|
reasons.append(
|
||||||
|
"baseline worktree clean-before validation not proven (#325)"
|
||||||
|
)
|
||||||
|
if proof.get("clean_after") is not True:
|
||||||
|
reasons.append(
|
||||||
|
"baseline worktree clean-after validation not proven (#325)"
|
||||||
|
)
|
||||||
|
|
||||||
|
proven = not reasons and not violations
|
||||||
|
return {
|
||||||
|
"proven": proven,
|
||||||
|
"block": bool(violations),
|
||||||
|
"claims_preexisting": claims_preexisting,
|
||||||
|
"reasons": reasons,
|
||||||
|
"violations": violations,
|
||||||
|
"safe_next_action": (
|
||||||
|
"create a clean baseline worktree under branches/, e.g. "
|
||||||
|
"branches/baseline-master-pr<N>, and rerun validation there"
|
||||||
|
if not proven
|
||||||
|
else "proceed"
|
||||||
|
),
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Identity disclosure (#305)
|
# Identity disclosure (#305)
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -3106,3 +3304,126 @@ def assess_reviewer_fallback_report(report_text, **kwargs):
|
|||||||
from reviewer_fallback import assess_reviewer_fallback_report as _assess
|
from reviewer_fallback import assess_reviewer_fallback_report as _assess
|
||||||
|
|
||||||
return _assess(report_text, **kwargs)
|
return _assess(report_text, **kwargs)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Git ref mutations (#297)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
_GIT_REF_MUTATIONS_FIELD_RE = re.compile(
|
||||||
|
r"^\s*git ref mutations\s*:\s*(.+?)\s*$", re.I | re.M)
|
||||||
|
_GIT_WORKTREE_MUTATIONS_NONE_RE = re.compile(
|
||||||
|
r"^\s*git/worktree mutations\s*:\s*none\b", re.I | re.M)
|
||||||
|
|
||||||
|
_GIT_REF_MUTATING_FIRST_WORDS = frozenset(
|
||||||
|
{"fetch", "pull", "push", "merge", "update-ref"})
|
||||||
|
_GIT_REF_MUTATING_PREFIXES = ("remote update", "branch -d", "branch -D",
|
||||||
|
"reset --hard")
|
||||||
|
|
||||||
|
|
||||||
|
def _command_text(entry):
|
||||||
|
if isinstance(entry, dict):
|
||||||
|
return str(entry.get("command") or "").strip()
|
||||||
|
return str(entry or "").strip()
|
||||||
|
|
||||||
|
|
||||||
|
def _git_subcommand_line(command):
|
||||||
|
tokens = command.split()
|
||||||
|
if not tokens or tokens[0] != "git":
|
||||||
|
return None
|
||||||
|
return " ".join(tokens[1:])
|
||||||
|
|
||||||
|
|
||||||
|
def git_ref_mutating_commands(command_log):
|
||||||
|
"""Return logged commands that update git refs (#297).
|
||||||
|
|
||||||
|
``git fetch`` and friends edit no files but move refs; they are never
|
||||||
|
read-only diagnostics.
|
||||||
|
"""
|
||||||
|
out = []
|
||||||
|
for entry in command_log or []:
|
||||||
|
command = _command_text(entry)
|
||||||
|
rest = _git_subcommand_line(command)
|
||||||
|
if rest is None:
|
||||||
|
continue
|
||||||
|
first = rest.split()[0] if rest.split() else ""
|
||||||
|
if first in _GIT_REF_MUTATING_FIRST_WORDS:
|
||||||
|
out.append(command)
|
||||||
|
continue
|
||||||
|
if any(rest.startswith(prefix) for prefix in _GIT_REF_MUTATING_PREFIXES):
|
||||||
|
out.append(command)
|
||||||
|
return out
|
||||||
|
|
||||||
|
|
||||||
|
def _fetch_targets(ref_commands):
|
||||||
|
"""Extract ``<remote>/<branch>`` targets from git fetch commands."""
|
||||||
|
targets = []
|
||||||
|
for command in ref_commands:
|
||||||
|
rest = _git_subcommand_line(command) or ""
|
||||||
|
tokens = rest.split()
|
||||||
|
if not tokens or tokens[0] != "fetch":
|
||||||
|
continue
|
||||||
|
args = [t for t in tokens[1:] if not t.startswith("-")]
|
||||||
|
if len(args) >= 2:
|
||||||
|
targets.append(f"{args[0]}/{args[1]}")
|
||||||
|
return targets
|
||||||
|
|
||||||
|
|
||||||
|
def assess_git_ref_mutation_report(report_text, *, command_log=None):
|
||||||
|
"""#297: ref updates are Git ref mutations, not read-only diagnostics.
|
||||||
|
|
||||||
|
When the command log holds ref-updating commands, the report must carry
|
||||||
|
a non-none ``Git ref mutations`` entry (naming ``fetched
|
||||||
|
<remote>/<branch>`` for targeted fetches), may not claim ``Git/worktree
|
||||||
|
mutations: None``, and may not list the command as read-only.
|
||||||
|
"""
|
||||||
|
text = report_text or ""
|
||||||
|
lower = text.lower()
|
||||||
|
reasons = []
|
||||||
|
ref_commands = git_ref_mutating_commands(command_log)
|
||||||
|
fetch_targets = _fetch_targets(ref_commands)
|
||||||
|
|
||||||
|
if ref_commands:
|
||||||
|
field = _GIT_REF_MUTATIONS_FIELD_RE.search(text)
|
||||||
|
value = field.group(1).strip().lower() if field else None
|
||||||
|
if not value or value == "none":
|
||||||
|
reasons.append(
|
||||||
|
"ref-updating commands ran but the report has no "
|
||||||
|
"'Git ref mutations' entry"
|
||||||
|
)
|
||||||
|
for target in fetch_targets:
|
||||||
|
if "fetched" not in lower or target.lower() not in lower:
|
||||||
|
reasons.append(
|
||||||
|
"git fetch ran; report must state "
|
||||||
|
f"'Git ref mutations: fetched {target}'"
|
||||||
|
)
|
||||||
|
if _GIT_WORKTREE_MUTATIONS_NONE_RE.search(text):
|
||||||
|
reasons.append(
|
||||||
|
"'Git/worktree mutations: None' rejected: ref-updating "
|
||||||
|
"commands ran"
|
||||||
|
)
|
||||||
|
for line in text.splitlines():
|
||||||
|
if "read-only" not in line.lower():
|
||||||
|
continue
|
||||||
|
for command in ref_commands:
|
||||||
|
if command.lower() in line.lower():
|
||||||
|
reasons.append(
|
||||||
|
"read-only diagnostics may not include ref-updating "
|
||||||
|
f"command '{command}'"
|
||||||
|
)
|
||||||
|
|
||||||
|
proven = not reasons
|
||||||
|
return {
|
||||||
|
"proven": proven,
|
||||||
|
"block": not proven,
|
||||||
|
"reasons": reasons,
|
||||||
|
"ref_mutating_commands": ref_commands,
|
||||||
|
"fetch_targets": fetch_targets,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def assess_non_mergeable_skip_proof(report_text, **kwargs):
|
||||||
|
"""#322: require conflict proof when skipping non-mergeable PRs."""
|
||||||
|
from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess
|
||||||
|
|
||||||
|
return _assess(report_text, **kwargs)
|
||||||
|
|||||||
@@ -0,0 +1,159 @@
|
|||||||
|
"""Non-mergeable PR skip conflict-proof verifier for reviewer reports (#322)."""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
|
from typing import Any
|
||||||
|
|
||||||
|
_FULL_SHA = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE)
|
||||||
|
_SHORT_SHA = re.compile(r"\b[0-9a-f]{7,39}\b", re.IGNORECASE)
|
||||||
|
_PR_NUMBER_RE = re.compile(r"(?:\bPR\s*#?|#)(\d+)\b", re.IGNORECASE)
|
||||||
|
_MERGEABILITY_FALSE_RE = re.compile(
|
||||||
|
r"(?:mergeable\s*:\s*false|not mergeable|non[- ]mergeable|mergeability\s*:\s*false|"
|
||||||
|
r"mergeability result\s*:\s*false)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
_CONFLICT_PROOF_RE = re.compile(
|
||||||
|
r"(?:merge-tree|git merge-tree|gitea_view_pr|gitea_check_pr_eligibility|"
|
||||||
|
r"conflict proof|mergeability tool|merge simulation|conflicting files?)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
_CONFLICTING_FILES_RE = re.compile(
|
||||||
|
r"(?:conflicting files?|conflict files?)\s*:\s*(.+)$",
|
||||||
|
re.IGNORECASE | re.MULTILINE,
|
||||||
|
)
|
||||||
|
_HEAD_CHANGED_RE = re.compile(
|
||||||
|
r"(?:head (?:changed|unchanged)|head sha (?:changed|unchanged)|"
|
||||||
|
r"head changed since|head unchanged since)",
|
||||||
|
re.IGNORECASE,
|
||||||
|
)
|
||||||
|
_MERGEABILITY_UNVERIFIED_RE = re.compile(
|
||||||
|
r"\bMERGEABILITY_UNVERIFIED\b",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _pr_section(text: str, pr_number: int) -> str:
|
||||||
|
"""Return report lines likely describing one skipped PR."""
|
||||||
|
lines = (text or "").splitlines()
|
||||||
|
chunks: list[str] = []
|
||||||
|
capture = False
|
||||||
|
token = f"#{pr_number}"
|
||||||
|
for line in lines:
|
||||||
|
lower = line.lower()
|
||||||
|
if token in lower or f"pr {pr_number}" in lower or f"pr#{pr_number}" in lower.replace(" ", ""):
|
||||||
|
capture = True
|
||||||
|
chunks.append(line)
|
||||||
|
continue
|
||||||
|
if capture:
|
||||||
|
if _PR_NUMBER_RE.search(line) and token not in line.lower():
|
||||||
|
break
|
||||||
|
if line.strip() == "" and len(chunks) > 3:
|
||||||
|
break
|
||||||
|
chunks.append(line)
|
||||||
|
if chunks:
|
||||||
|
return "\n".join(chunks)
|
||||||
|
return text or ""
|
||||||
|
|
||||||
|
|
||||||
|
def _has_head_sha(text: str, head_sha: str | None) -> bool:
|
||||||
|
if not head_sha:
|
||||||
|
return bool(_FULL_SHA.search(text) or _SHORT_SHA.search(text))
|
||||||
|
head = head_sha.strip().lower()
|
||||||
|
if head in text.lower():
|
||||||
|
return True
|
||||||
|
if len(head) >= 7 and head[:7] in text.lower():
|
||||||
|
return True
|
||||||
|
return bool(_FULL_SHA.search(text))
|
||||||
|
|
||||||
|
|
||||||
|
def assess_non_mergeable_skip_proof(
|
||||||
|
report_text: str,
|
||||||
|
*,
|
||||||
|
skipped_prs: list[dict] | None = None,
|
||||||
|
) -> dict[str, Any]:
|
||||||
|
"""Validate skipped non-mergeable PR documentation in reviewer reports (#322)."""
|
||||||
|
text = report_text or ""
|
||||||
|
reasons: list[str] = []
|
||||||
|
assessments: list[dict[str, Any]] = []
|
||||||
|
|
||||||
|
for entry in skipped_prs or []:
|
||||||
|
pr_number = entry.get("pr_number")
|
||||||
|
if pr_number is None:
|
||||||
|
reasons.append("skipped PR entry missing pr_number")
|
||||||
|
continue
|
||||||
|
pr_number = int(pr_number)
|
||||||
|
section = _pr_section(text, pr_number)
|
||||||
|
mergeable = entry.get("mergeable")
|
||||||
|
verified = entry.get("mergeability_verified")
|
||||||
|
classification = (entry.get("classification") or "").strip().upper()
|
||||||
|
head_sha = (entry.get("head_sha") or "").strip() or None
|
||||||
|
|
||||||
|
item_reasons: list[str] = []
|
||||||
|
|
||||||
|
if f"#{pr_number}" not in text.lower() and f"pr {pr_number}" not in text.lower():
|
||||||
|
item_reasons.append(f"skipped PR #{pr_number} not documented in final report")
|
||||||
|
|
||||||
|
if mergeable is False or verified is False:
|
||||||
|
if not _MERGEABILITY_UNVERIFIED_RE.search(section) and verified is False:
|
||||||
|
if "MERGEABILITY_UNVERIFIED" not in classification:
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} conflict proof unavailable; report must classify "
|
||||||
|
"MERGEABILITY_UNVERIFIED"
|
||||||
|
)
|
||||||
|
elif verified is not False:
|
||||||
|
if not _MERGEABILITY_FALSE_RE.search(section):
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} skip missing mergeability:false proof"
|
||||||
|
)
|
||||||
|
if not _has_head_sha(section, head_sha):
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} skip missing current head SHA"
|
||||||
|
)
|
||||||
|
if not _CONFLICT_PROOF_RE.search(section):
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} skip missing conflict proof command/tool"
|
||||||
|
)
|
||||||
|
if entry.get("head_changed_since_prior_blocker") is not None:
|
||||||
|
if not _HEAD_CHANGED_RE.search(section):
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} skip missing head-changed-since-blocker proof"
|
||||||
|
)
|
||||||
|
|
||||||
|
if (
|
||||||
|
entry.get("conflicting_files")
|
||||||
|
and not _CONFLICTING_FILES_RE.search(section)
|
||||||
|
and not any(
|
||||||
|
path.lower() in section.lower()
|
||||||
|
for path in (entry.get("conflicting_files") or [])
|
||||||
|
)
|
||||||
|
):
|
||||||
|
item_reasons.append(
|
||||||
|
f"PR #{pr_number} has conflicting files in session proof but "
|
||||||
|
"report omits file-level conflict proof"
|
||||||
|
)
|
||||||
|
|
||||||
|
proven = not item_reasons
|
||||||
|
assessments.append({
|
||||||
|
"pr_number": pr_number,
|
||||||
|
"proven": proven,
|
||||||
|
"classification": classification or (
|
||||||
|
"MERGEABILITY_UNVERIFIED" if verified is False else "NON_MERGEABLE_SKIPPED"
|
||||||
|
),
|
||||||
|
"reasons": item_reasons,
|
||||||
|
})
|
||||||
|
reasons.extend(item_reasons)
|
||||||
|
|
||||||
|
proven = not reasons
|
||||||
|
return {
|
||||||
|
"proven": proven,
|
||||||
|
"block": not proven,
|
||||||
|
"downgraded": False,
|
||||||
|
"assessments": assessments,
|
||||||
|
"reasons": reasons,
|
||||||
|
"safe_next_action": (
|
||||||
|
"document each skipped non-mergeable PR with head SHA, mergeability result, "
|
||||||
|
"conflict proof command, conflicting files, and head-change status"
|
||||||
|
if reasons
|
||||||
|
else "proceed"
|
||||||
|
),
|
||||||
|
}
|
||||||
@@ -0,0 +1,101 @@
|
|||||||
|
"""Git ref mutations must be reported, never hidden as diagnostics (#297).
|
||||||
|
|
||||||
|
``git fetch`` updates remote-tracking refs even though it edits no files.
|
||||||
|
Reports that ran fetch (or any ref-updating command) must carry a
|
||||||
|
``Git ref mutations`` entry and may not claim ``Git/worktree mutations:
|
||||||
|
None`` or classify the fetch as read-only diagnostics.
|
||||||
|
"""
|
||||||
|
import sys
|
||||||
|
import unittest
|
||||||
|
|
||||||
|
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
import review_proofs
|
||||||
|
|
||||||
|
|
||||||
|
def _assess(report, commands):
|
||||||
|
return review_proofs.assess_git_ref_mutation_report(
|
||||||
|
report, command_log=commands)
|
||||||
|
|
||||||
|
|
||||||
|
class TestGitRefMutationDetection(unittest.TestCase):
|
||||||
|
def test_fetch_only_review_with_proper_ledger_passes(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Git ref mutations: fetched prgs/master",
|
||||||
|
"Review decision: APPROVE",
|
||||||
|
])
|
||||||
|
res = _assess(report, ["git fetch prgs master"])
|
||||||
|
self.assertTrue(res["proven"], res["reasons"])
|
||||||
|
self.assertEqual(res["ref_mutating_commands"], ["git fetch prgs master"])
|
||||||
|
|
||||||
|
def test_fetch_without_ref_mutation_line_is_blocked(self):
|
||||||
|
report = "Review decision: APPROVE\nMutations: None\n"
|
||||||
|
res = _assess(report, ["git fetch prgs master"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("git ref mutations" in r.lower() for r in res["reasons"]))
|
||||||
|
|
||||||
|
def test_fetch_with_remote_branch_requires_fetched_target_in_report(self):
|
||||||
|
report = "Git ref mutations: some refs updated\n"
|
||||||
|
res = _assess(report, ["git fetch prgs master"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("prgs/master" in r for r in res["reasons"]))
|
||||||
|
|
||||||
|
def test_git_worktree_mutations_none_rejected_when_fetch_occurred(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Git ref mutations: fetched prgs/master",
|
||||||
|
"Git/worktree mutations: None",
|
||||||
|
])
|
||||||
|
res = _assess(report, ["git fetch prgs master"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("git/worktree mutations" in r.lower() for r in res["reasons"]))
|
||||||
|
|
||||||
|
def test_fetch_classified_as_read_only_diagnostics_rejected(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Git ref mutations: fetched prgs/master",
|
||||||
|
"Read-only diagnostics: git fetch prgs master, git status",
|
||||||
|
])
|
||||||
|
res = _assess(report, ["git fetch prgs master"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("read-only" in r.lower() for r in res["reasons"]))
|
||||||
|
|
||||||
|
def test_worktree_creation_is_not_a_ref_mutation(self):
|
||||||
|
report = "Worktree mutations: created branches/review-pr9\n"
|
||||||
|
res = _assess(report, ["git worktree add branches/review-pr9 prgs/master"])
|
||||||
|
self.assertTrue(res["proven"], res["reasons"])
|
||||||
|
self.assertEqual(res["ref_mutating_commands"], [])
|
||||||
|
|
||||||
|
def test_merge_command_counts_as_ref_mutation(self):
|
||||||
|
report = "Review decision: APPROVE\n"
|
||||||
|
res = _assess(report, ["git merge --no-ff feat/x"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertIn("git merge --no-ff feat/x", res["ref_mutating_commands"])
|
||||||
|
|
||||||
|
def test_cleanup_branch_delete_counts_as_ref_mutation(self):
|
||||||
|
report = "Cleanup mutations: deleted branch feat/x\n"
|
||||||
|
res = _assess(report, ["git branch -d feat/x"])
|
||||||
|
self.assertFalse(res["proven"])
|
||||||
|
self.assertIn("git branch -d feat/x", res["ref_mutating_commands"])
|
||||||
|
|
||||||
|
def test_no_mutation_blocked_handoff_passes(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Git/worktree mutations: None",
|
||||||
|
"Current status: blocked handoff, no mutations performed",
|
||||||
|
])
|
||||||
|
res = _assess(report, ["git status --porcelain", "git log --oneline -1"])
|
||||||
|
self.assertTrue(res["proven"], res["reasons"])
|
||||||
|
self.assertEqual(res["ref_mutating_commands"], [])
|
||||||
|
|
||||||
|
def test_command_log_dict_entries_supported(self):
|
||||||
|
report = "Git ref mutations: fetched prgs/master\n"
|
||||||
|
res = _assess(report, [{"command": "git fetch prgs master"}])
|
||||||
|
self.assertTrue(res["proven"], res["reasons"])
|
||||||
|
self.assertEqual(
|
||||||
|
res["ref_mutating_commands"], ["git fetch prgs master"])
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
@@ -127,4 +127,10 @@ def test_reviewer_fallback_verifier_exported():
|
|||||||
def test_create_issue_final_report_verifier_exported():
|
def test_create_issue_final_report_verifier_exported():
|
||||||
from review_proofs import assess_create_issue_final_report
|
from review_proofs import assess_create_issue_final_report
|
||||||
|
|
||||||
assert callable(assess_create_issue_final_report)
|
assert callable(assess_create_issue_final_report)
|
||||||
|
|
||||||
|
|
||||||
|
def test_non_mergeable_skip_verifier_exported():
|
||||||
|
from review_proofs import assess_non_mergeable_skip_proof
|
||||||
|
|
||||||
|
assert callable(assess_non_mergeable_skip_proof)
|
||||||
@@ -48,6 +48,21 @@ import mcp_server
|
|||||||
|
|
||||||
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
||||||
|
|
||||||
|
_NO_BLOCKER_FEEDBACK = {
|
||||||
|
"success": True,
|
||||||
|
"has_blocking_change_requests": False,
|
||||||
|
"review_feedback_stale": False,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def _mark_request_changes_ready(pr_number=8, **kwargs):
|
||||||
|
"""Mark a request_changes decision ready with the #332 duplicate-
|
||||||
|
suppression feedback fetch stubbed to 'no existing blocker'."""
|
||||||
|
with patch("mcp_server.gitea_get_pr_review_feedback",
|
||||||
|
return_value=dict(_NO_BLOCKER_FEEDBACK)):
|
||||||
|
return gitea_mark_final_review_decision(
|
||||||
|
pr_number, "request_changes", **kwargs)
|
||||||
|
|
||||||
|
|
||||||
def _formal_review(reviewer, verdict, sha="abc123", review_id=1):
|
def _formal_review(reviewer, verdict, sha="abc123", review_id=1):
|
||||||
return {
|
return {
|
||||||
@@ -1839,7 +1854,7 @@ class TestSubmitPrReview(unittest.TestCase):
|
|||||||
@patch("mcp_server.api_request")
|
@patch("mcp_server.api_request")
|
||||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||||
def test_request_changes_succeeds_when_eligible(self, _auth, mock_api):
|
def test_request_changes_succeeds_when_eligible(self, _auth, mock_api):
|
||||||
gitea_mark_final_review_decision(8, "request_changes", remote="prgs")
|
_mark_request_changes_ready(remote="prgs")
|
||||||
mock_api.side_effect = [
|
mock_api.side_effect = [
|
||||||
{"login": "reviewer-bot"}, self._pr("author-bot"),
|
{"login": "reviewer-bot"}, self._pr("author-bot"),
|
||||||
{"id": 9, "state": "REQUEST_CHANGES"},
|
{"id": 9, "state": "REQUEST_CHANGES"},
|
||||||
@@ -1862,7 +1877,7 @@ class TestSubmitPrReview(unittest.TestCase):
|
|||||||
self.assertEqual(post_calls[0].args[3]["event"], "REQUEST_CHANGES")
|
self.assertEqual(post_calls[0].args[3]["event"], "REQUEST_CHANGES")
|
||||||
|
|
||||||
def test_request_changes_blocked_without_eligibility(self):
|
def test_request_changes_blocked_without_eligibility(self):
|
||||||
gitea_mark_final_review_decision(8, "request_changes", remote="prgs")
|
_mark_request_changes_ready(remote="prgs")
|
||||||
with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) as _a, \
|
with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) as _a, \
|
||||||
patch("mcp_server.api_request") as mock_api:
|
patch("mcp_server.api_request") as mock_api:
|
||||||
mock_api.side_effect = [{"login": "reviewer-bot"}, self._pr("author-bot")]
|
mock_api.side_effect = [{"login": "reviewer-bot"}, self._pr("author-bot")]
|
||||||
@@ -2144,7 +2159,7 @@ class TestSubmitPrReview(unittest.TestCase):
|
|||||||
reason="operator approved correcting mistaken approve",
|
reason="operator approved correcting mistaken approve",
|
||||||
operator_authorized=True,
|
operator_authorized=True,
|
||||||
)
|
)
|
||||||
gitea_mark_final_review_decision(8, "request_changes", remote="prgs")
|
_mark_request_changes_ready(remote="prgs")
|
||||||
second = gitea_submit_pr_review(
|
second = gitea_submit_pr_review(
|
||||||
pr_number=8, action="request_changes", remote="prgs",
|
pr_number=8, action="request_changes", remote="prgs",
|
||||||
final_review_decision_ready=True,
|
final_review_decision_ready=True,
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ from review_proofs import ( # noqa: E402
|
|||||||
ISSUE_SELECTION_CONTINUATION_EXPLICIT,
|
ISSUE_SELECTION_CONTINUATION_EXPLICIT,
|
||||||
ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR,
|
ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR,
|
||||||
assess_author_pr_report,
|
assess_author_pr_report,
|
||||||
|
assess_reviewer_baseline_validation_proof,
|
||||||
assess_capability_evidence,
|
assess_capability_evidence,
|
||||||
assess_capability_proof,
|
assess_capability_proof,
|
||||||
assess_contradictory_no_pr_claim,
|
assess_contradictory_no_pr_claim,
|
||||||
@@ -2160,5 +2161,125 @@ class TestCreateIssueFinalReport(unittest.TestCase):
|
|||||||
self.assertTrue(result["downgraded"])
|
self.assertTrue(result["downgraded"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestReviewerBaselineValidationProof(unittest.TestCase):
|
||||||
|
"""Issue #325: baseline validation must use branches/ worktrees."""
|
||||||
|
|
||||||
|
ROOT = "/repo/Gitea-Tools"
|
||||||
|
|
||||||
|
def _good_baseline_proof(self, **overrides):
|
||||||
|
proof = {
|
||||||
|
"worktree_path": f"{self.ROOT}/branches/baseline-master-pr280",
|
||||||
|
"baseline_target_sha": PINNED,
|
||||||
|
"pr_head_sha": OTHER,
|
||||||
|
"baseline_failures": ["tests/test_foo.py::test_bar"],
|
||||||
|
"pr_failures": ["tests/test_foo.py::test_bar"],
|
||||||
|
"failure_signatures_match": True,
|
||||||
|
"clean_before": True,
|
||||||
|
"clean_after": True,
|
||||||
|
}
|
||||||
|
proof.update(overrides)
|
||||||
|
return proof
|
||||||
|
|
||||||
|
def test_main_checkout_test_run_blocks(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
validation_runs=[
|
||||||
|
{
|
||||||
|
"command": "venv/bin/pytest tests/",
|
||||||
|
"working_directory": self.ROOT,
|
||||||
|
"project_root": self.ROOT,
|
||||||
|
}
|
||||||
|
],
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["block"])
|
||||||
|
self.assertTrue(result["violations"])
|
||||||
|
|
||||||
|
def test_branches_worktree_test_run_passes(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
validation_runs=[
|
||||||
|
{
|
||||||
|
"command": "venv/bin/pytest tests/",
|
||||||
|
"working_directory": f"{self.ROOT}/branches/review-pr-280",
|
||||||
|
"project_root": self.ROOT,
|
||||||
|
}
|
||||||
|
],
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_preexisting_claim_without_baseline_proof_fails(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
report_text=(
|
||||||
|
"Validation: full-suite failures are pre-existing on master."
|
||||||
|
),
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["claims_preexisting"])
|
||||||
|
|
||||||
|
def test_preexisting_claim_with_complete_baseline_proof_passes(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
report_text="Failures are pre-existing on master.",
|
||||||
|
baseline_proof=self._good_baseline_proof(),
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_mismatched_failures_block_preexisting_claim(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
report_text="Same as master.",
|
||||||
|
baseline_proof=self._good_baseline_proof(
|
||||||
|
failure_signatures_match=False,
|
||||||
|
pr_failures=["tests/test_other.py::test_other"],
|
||||||
|
),
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_report_parsed_main_checkout_cwd_blocks(self):
|
||||||
|
result = assess_reviewer_baseline_validation_proof(
|
||||||
|
report_text=(
|
||||||
|
"- Validation command: venv/bin/pytest tests/\n"
|
||||||
|
f"- Working directory: {self.ROOT}\n"
|
||||||
|
),
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["block"])
|
||||||
|
|
||||||
|
def test_build_final_report_downgrades_main_checkout_validation(self):
|
||||||
|
report = build_final_report(
|
||||||
|
checkout_proof=_good_checkout(),
|
||||||
|
inventory=_good_inventory(),
|
||||||
|
validation=_good_validation(),
|
||||||
|
contamination=_good_contamination(),
|
||||||
|
identity_eligible=True,
|
||||||
|
merge_performed=False,
|
||||||
|
issue_status_verified=True,
|
||||||
|
capability_evidence=_good_capability_evidence(),
|
||||||
|
sweep=_good_sweep(),
|
||||||
|
live_state=_good_live_state(),
|
||||||
|
role_boundary=_good_role_boundary(),
|
||||||
|
review_mutation=_good_review_mutation(),
|
||||||
|
controller_handoff=_good_handoff(),
|
||||||
|
capability_proof=_good_capability_proof(),
|
||||||
|
sweep_proof=_good_secret_sweep(),
|
||||||
|
worktree_proof={
|
||||||
|
"worktree_path": "/repo/branches/review-feat-issue-224",
|
||||||
|
"porcelain_status": "",
|
||||||
|
"pr_scope_files": ["docs/wiki/Repositories.md"],
|
||||||
|
"scratch_used": True,
|
||||||
|
},
|
||||||
|
report_text=(
|
||||||
|
"- Validation command: venv/bin/pytest tests/\n"
|
||||||
|
f"- Working directory: {self.ROOT}\n"
|
||||||
|
),
|
||||||
|
project_root=self.ROOT,
|
||||||
|
)
|
||||||
|
self.assertNotEqual(report["grade"], "A")
|
||||||
|
self.assertFalse(report["baseline_validation_proven"])
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
@@ -0,0 +1,150 @@
|
|||||||
|
"""Tests for non-mergeable skip conflict-proof verifier (#322)."""
|
||||||
|
import sys
|
||||||
|
import unittest
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
from reviewer_mergeability_skip import assess_non_mergeable_skip_proof # noqa: E402
|
||||||
|
|
||||||
|
HEAD_A = "0fdc8f582026b72a229d59a172c0a63ac4aaeaf9"
|
||||||
|
HEAD_B = "1111111111111111111111111111111111111111"
|
||||||
|
|
||||||
|
|
||||||
|
def _good_skip_report(pr_number: int, head: str, files: str = "review_proofs.py") -> str:
|
||||||
|
return "\n".join([
|
||||||
|
f"Skipped PR #{pr_number} (non-mergeable).",
|
||||||
|
f"- PR number: #{pr_number}",
|
||||||
|
f"- Current head SHA: {head}",
|
||||||
|
"- Mergeability result: false",
|
||||||
|
"- Conflict proof command: git merge-tree $(git merge-base prgs/master prgs/feat) prgs/master prgs/feat",
|
||||||
|
f"- Conflicting files: {files}",
|
||||||
|
"- Head unchanged since prior blocker",
|
||||||
|
"- Blocking category: merge conflict",
|
||||||
|
])
|
||||||
|
|
||||||
|
|
||||||
|
class TestNonMergeableSkipProof(unittest.TestCase):
|
||||||
|
def test_no_skips_passes(self):
|
||||||
|
report = "Selected PR #203 for review. No earlier PRs skipped."
|
||||||
|
result = assess_non_mergeable_skip_proof(report, skipped_prs=[])
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
self.assertFalse(result["block"])
|
||||||
|
|
||||||
|
def test_documented_non_mergeable_skip_passes(self):
|
||||||
|
report = _good_skip_report(282, HEAD_A)
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 282,
|
||||||
|
"mergeable": False,
|
||||||
|
"head_sha": HEAD_A,
|
||||||
|
"mergeability_verified": True,
|
||||||
|
"conflicting_files": ["review_proofs.py"],
|
||||||
|
"head_changed_since_prior_blocker": False,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_skip_without_conflict_proof_blocks(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Skipped PR #284 because it was not mergeable.",
|
||||||
|
f"Head SHA: {HEAD_B}",
|
||||||
|
])
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 284,
|
||||||
|
"mergeable": False,
|
||||||
|
"head_sha": HEAD_B,
|
||||||
|
"mergeability_verified": True,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(result["block"])
|
||||||
|
self.assertTrue(any("conflict proof" in r for r in result["reasons"]))
|
||||||
|
|
||||||
|
def test_missing_head_sha_blocks(self):
|
||||||
|
report = _good_skip_report(282, HEAD_A).replace(HEAD_A, "")
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 282,
|
||||||
|
"mergeable": False,
|
||||||
|
"head_sha": HEAD_A,
|
||||||
|
"mergeability_verified": True,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
|
||||||
|
def test_mergeability_unverified_classification_passes(self):
|
||||||
|
report = "\n".join([
|
||||||
|
"Skipped PR #282.",
|
||||||
|
"Classification: MERGEABILITY_UNVERIFIED",
|
||||||
|
"Conflict proof could not be retrieved in this session.",
|
||||||
|
])
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 282,
|
||||||
|
"mergeable": False,
|
||||||
|
"mergeability_verified": False,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_unverified_without_classification_blocks(self):
|
||||||
|
report = "Skipped PR #282 due to conflicts."
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 282,
|
||||||
|
"mergeable": False,
|
||||||
|
"mergeability_verified": False,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(any("MERGEABILITY_UNVERIFIED" in r for r in result["reasons"]))
|
||||||
|
|
||||||
|
def test_non_mergeable_without_file_details_still_passes_with_command(self):
|
||||||
|
report = _good_skip_report(284, HEAD_B, files="not available")
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 284,
|
||||||
|
"mergeable": False,
|
||||||
|
"head_sha": HEAD_B,
|
||||||
|
"mergeability_verified": True,
|
||||||
|
"conflicting_files": [],
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertTrue(result["proven"])
|
||||||
|
|
||||||
|
def test_stale_head_requires_head_change_field(self):
|
||||||
|
report = _good_skip_report(282, HEAD_A).replace(
|
||||||
|
"- Head unchanged since prior blocker",
|
||||||
|
"",
|
||||||
|
)
|
||||||
|
result = assess_non_mergeable_skip_proof(
|
||||||
|
report,
|
||||||
|
skipped_prs=[{
|
||||||
|
"pr_number": 282,
|
||||||
|
"mergeable": False,
|
||||||
|
"head_sha": HEAD_A,
|
||||||
|
"mergeability_verified": True,
|
||||||
|
"head_changed_since_prior_blocker": True,
|
||||||
|
}],
|
||||||
|
)
|
||||||
|
self.assertFalse(result["proven"])
|
||||||
|
self.assertTrue(any("head-changed" in r for r in result["reasons"]))
|
||||||
|
|
||||||
|
|
||||||
|
class TestExport(unittest.TestCase):
|
||||||
|
def test_review_proofs_reexport(self):
|
||||||
|
from review_proofs import assess_non_mergeable_skip_proof as exported
|
||||||
|
|
||||||
|
self.assertTrue(callable(exported))
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
@@ -0,0 +1,187 @@
|
|||||||
|
"""Tests for the session hard-stop after a terminal review mutation (#332).
|
||||||
|
|
||||||
|
Extends the single-terminal-decision machinery (#211): after a live
|
||||||
|
REQUEST_CHANGES the run must stop; after an APPROVE only the merge sequence
|
||||||
|
for that same PR may continue; a different PR can never be reviewed or
|
||||||
|
merged in the same run; duplicate REQUEST_CHANGES at an unchanged head is
|
||||||
|
suppressed.
|
||||||
|
"""
|
||||||
|
import os
|
||||||
|
import unittest
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
import mcp_server
|
||||||
|
|
||||||
|
|
||||||
|
def _lock(mutations=None, correction=False):
|
||||||
|
return {
|
||||||
|
"task": "review_pr",
|
||||||
|
"remote": "prgs",
|
||||||
|
"session_pid": os.getpid(),
|
||||||
|
"session_profile": "prgs-reviewer",
|
||||||
|
"session_profile_lock": "",
|
||||||
|
"final_review_decision_ready": False,
|
||||||
|
"ready_pr_number": None,
|
||||||
|
"ready_action": None,
|
||||||
|
"ready_expected_head_sha": None,
|
||||||
|
"ready_remote": None,
|
||||||
|
"ready_org": None,
|
||||||
|
"ready_repo": None,
|
||||||
|
"live_mutations": list(mutations or []),
|
||||||
|
"correction_authorized": correction,
|
||||||
|
"correction_reason": None,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def _seed(mutations=None, correction=False):
|
||||||
|
mcp_server._save_review_decision_lock(_lock(mutations, correction))
|
||||||
|
|
||||||
|
|
||||||
|
APPROVED_A = {"pr_number": 5, "action": "approve", "review_id": 1,
|
||||||
|
"review_state": "approve"}
|
||||||
|
RC_A = {"pr_number": 5, "action": "request_changes", "review_id": 2,
|
||||||
|
"review_state": "request_changes"}
|
||||||
|
|
||||||
|
|
||||||
|
class TestTerminalHardStopReasons(unittest.TestCase):
|
||||||
|
def tearDown(self):
|
||||||
|
mcp_server._save_review_decision_lock(None)
|
||||||
|
|
||||||
|
def test_no_lock_no_reasons(self):
|
||||||
|
mcp_server._save_review_decision_lock(None)
|
||||||
|
self.assertEqual(
|
||||||
|
mcp_server.terminal_review_hard_stop_reasons(5, "merge"), [])
|
||||||
|
|
||||||
|
def test_no_terminal_mutation_no_reasons(self):
|
||||||
|
_seed()
|
||||||
|
self.assertEqual(
|
||||||
|
mcp_server.terminal_review_hard_stop_reasons(5, "merge"), [])
|
||||||
|
|
||||||
|
def test_request_changes_blocks_everything(self):
|
||||||
|
_seed([RC_A])
|
||||||
|
for op in ("merge", "mark_ready", "review"):
|
||||||
|
for pr in (5, 6):
|
||||||
|
reasons = mcp_server.terminal_review_hard_stop_reasons(pr, op)
|
||||||
|
self.assertTrue(reasons, f"{op}/{pr}")
|
||||||
|
self.assertIn("request_changes on PR #5", reasons[0])
|
||||||
|
self.assertIn("#332", reasons[0])
|
||||||
|
|
||||||
|
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"))
|
||||||
|
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)
|
||||||
|
self.assertEqual(
|
||||||
|
mcp_server.terminal_review_hard_stop_reasons(5, "mark_ready"), [])
|
||||||
|
self.assertEqual(
|
||||||
|
mcp_server.terminal_review_hard_stop_reasons(5, "review"), [])
|
||||||
|
# correction never opens a cross-PR merge
|
||||||
|
self.assertTrue(
|
||||||
|
mcp_server.terminal_review_hard_stop_reasons(6, "merge"))
|
||||||
|
|
||||||
|
|
||||||
|
class TestMergeHardStopWiring(unittest.TestCase):
|
||||||
|
def tearDown(self):
|
||||||
|
mcp_server._save_review_decision_lock(None)
|
||||||
|
|
||||||
|
def test_merge_blocked_after_request_changes(self):
|
||||||
|
_seed([RC_A])
|
||||||
|
result = mcp_server.gitea_merge_pr(
|
||||||
|
pr_number=6, confirmation="MERGE PR 6", remote="prgs")
|
||||||
|
self.assertFalse(result["performed"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("#332" in r for r in result["reasons"]), result["reasons"])
|
||||||
|
|
||||||
|
def test_merge_of_other_pr_blocked_after_approve(self):
|
||||||
|
_seed([APPROVED_A])
|
||||||
|
result = mcp_server.gitea_merge_pr(
|
||||||
|
pr_number=6, confirmation="MERGE PR 6", remote="prgs")
|
||||||
|
self.assertFalse(result["performed"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("#332" in r for r in result["reasons"]), result["reasons"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestMarkFinalHardStopWiring(unittest.TestCase):
|
||||||
|
def tearDown(self):
|
||||||
|
mcp_server._save_review_decision_lock(None)
|
||||||
|
|
||||||
|
def test_mark_ready_blocked_after_terminal_mutation(self):
|
||||||
|
_seed([RC_A])
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
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 _feedback(blocking, stale=False, success=True):
|
||||||
|
return {
|
||||||
|
"success": success,
|
||||||
|
"has_blocking_change_requests": blocking,
|
||||||
|
"review_feedback_stale": stale,
|
||||||
|
"current_head_sha": "abc123",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
class TestDuplicateRequestChangesSuppression(unittest.TestCase):
|
||||||
|
def tearDown(self):
|
||||||
|
mcp_server._save_review_decision_lock(None)
|
||||||
|
|
||||||
|
def test_duplicate_request_changes_blocked_at_same_head(self):
|
||||||
|
_seed()
|
||||||
|
with patch.object(mcp_server, "gitea_get_pr_review_feedback",
|
||||||
|
return_value=_feedback(blocking=True, stale=False)):
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
pr_number=6, action="request_changes", remote="prgs")
|
||||||
|
self.assertFalse(result["marked_ready"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("duplicate" in r for r in result["reasons"]),
|
||||||
|
result["reasons"])
|
||||||
|
|
||||||
|
def test_request_changes_allowed_when_prior_blocker_stale(self):
|
||||||
|
_seed()
|
||||||
|
with patch.object(mcp_server, "gitea_get_pr_review_feedback",
|
||||||
|
return_value=_feedback(blocking=True, stale=True)):
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
pr_number=6, action="request_changes", remote="prgs")
|
||||||
|
self.assertTrue(result["marked_ready"], result.get("reasons"))
|
||||||
|
|
||||||
|
def test_request_changes_allowed_when_no_blocker(self):
|
||||||
|
_seed()
|
||||||
|
with patch.object(mcp_server, "gitea_get_pr_review_feedback",
|
||||||
|
return_value=_feedback(blocking=False)):
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
pr_number=6, action="request_changes", remote="prgs")
|
||||||
|
self.assertTrue(result["marked_ready"], result.get("reasons"))
|
||||||
|
|
||||||
|
def test_request_changes_fails_closed_when_feedback_unavailable(self):
|
||||||
|
_seed()
|
||||||
|
with patch.object(mcp_server, "gitea_get_pr_review_feedback",
|
||||||
|
return_value=_feedback(blocking=False,
|
||||||
|
success=False)):
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
pr_number=6, action="request_changes", remote="prgs")
|
||||||
|
self.assertFalse(result["marked_ready"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("could not verify" in r for r in result["reasons"]),
|
||||||
|
result["reasons"])
|
||||||
|
|
||||||
|
def test_approve_does_not_fetch_feedback(self):
|
||||||
|
_seed()
|
||||||
|
with patch.object(mcp_server, "gitea_get_pr_review_feedback",
|
||||||
|
side_effect=AssertionError("must not be called")):
|
||||||
|
result = mcp_server.gitea_mark_final_review_decision(
|
||||||
|
pr_number=6, action="approve", remote="prgs")
|
||||||
|
self.assertTrue(result["marked_ready"], result.get("reasons"))
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
Reference in New Issue
Block a user