feat: reject stale already-landed controller handoff fields (#299)
Add assess_already_landed_handoff_report verifier to block Pinned reviewed head, Scratch worktree used, legacy Mutations/Workspace mutations None when git fetch or other mutations occurred, and narrative/handoff drift after the already-landed gate fires. Wire into build_final_report and review_proofs. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
Claude Opus 4.8
@@ -1405,6 +1405,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
||||
if report_text
|
||||
else {"proven": True, "block": False, "reasons": [], "violations": []}
|
||||
)
|
||||
already_landed_handoff = (
|
||||
assess_already_landed_handoff_report(report_text)
|
||||
if report_text
|
||||
else {"proven": True, "block": False, "reasons": []}
|
||||
)
|
||||
if baseline_validation is None and report_text:
|
||||
baseline_validation = assess_reviewer_baseline_validation_proof(
|
||||
report_text=report_text,
|
||||
@@ -1561,6 +1566,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
||||
"queue-status report violates loaded workflow proof gates (#339)"
|
||||
)
|
||||
downgrade_reasons.extend(queue_status_report.get("reasons", []))
|
||||
if not already_landed_handoff.get("proven"):
|
||||
downgrade_reasons.append(
|
||||
"already-landed controller handoff has stale or inconsistent fields (#299)"
|
||||
)
|
||||
downgrade_reasons.extend(already_landed_handoff.get("reasons", []))
|
||||
if not baseline_validation.get("proven"):
|
||||
downgrade_reasons.append(
|
||||
"reviewer baseline validation proof missing or failed (#325)"
|
||||
@@ -1646,6 +1656,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
||||
"queue_status_violations": list(
|
||||
queue_status_report.get("violations") or []
|
||||
),
|
||||
"already_landed_handoff_proven": bool(already_landed_handoff.get("proven")),
|
||||
"already_landed_handoff_violations": list(
|
||||
already_landed_handoff.get("reasons") or []
|
||||
),
|
||||
"baseline_validation_proven": bool(baseline_validation.get("proven")),
|
||||
"baseline_validation_violations": list(
|
||||
baseline_validation.get("violations") or []
|
||||
@@ -4064,3 +4078,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs):
|
||||
from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess
|
||||
|
||||
return _assess(report_text, **kwargs)
|
||||
|
||||
|
||||
def assess_already_landed_handoff_report(report_text, **kwargs):
|
||||
"""#299: reject stale fields after the already-landed gate fires."""
|
||||
from reviewer_already_landed_handoff import assess_already_landed_handoff_report as _assess
|
||||
|
||||
return _assess(report_text, **kwargs)
|
||||
|
||||
@@ -0,0 +1,216 @@
|
||||
"""Already-landed controller handoff consistency verifier (#299)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from typing import Any
|
||||
|
||||
from review_proofs import git_ref_mutating_commands
|
||||
|
||||
ALREADY_LANDED_STATE = "ALREADY_LANDED_RECONCILE_REQUIRED"
|
||||
|
||||
_HANDOFF_SECTION_RE = re.compile(r"^##\s*Controller Handoff\s*$", re.I | re.M)
|
||||
|
||||
_STALE_HANDOFF_FIELDS = (
|
||||
"pinned reviewed head",
|
||||
"scratch worktree used",
|
||||
)
|
||||
|
||||
_LEGACY_MUTATIONS_NONE_RE = re.compile(
|
||||
r"^\s*[-*]?\s*mutations\s*:\s*none\s*$",
|
||||
re.I | re.M,
|
||||
)
|
||||
_LEGACY_WORKSPACE_NONE_RE = re.compile(
|
||||
r"^\s*[-*]?\s*workspace\s+mutations\s*:\s*none\s*$",
|
||||
re.I | re.M,
|
||||
)
|
||||
|
||||
_FORBIDDEN_REVIEW_STATES_RE = re.compile(
|
||||
r"review decision\s*:\s*approved?\b|"
|
||||
r"merge result\s*:\s*merged\b|"
|
||||
r"\bready[_ ]to[_ ]merge\b",
|
||||
re.I,
|
||||
)
|
||||
|
||||
_NARRATIVE_ELIGIBILITY_RE = re.compile(
|
||||
r"eligibility class\s*:\s*([^\n]+)",
|
||||
re.I,
|
||||
)
|
||||
_NARRATIVE_REVIEWED_SHA_RE = re.compile(
|
||||
r"reviewed head sha\s*:\s*([^\n]+)",
|
||||
re.I,
|
||||
)
|
||||
_NARRATIVE_WORKTREE_RE = re.compile(
|
||||
r"review worktree used\s*:\s*([^\n]+)",
|
||||
re.I,
|
||||
)
|
||||
|
||||
|
||||
def _handoff_field_map(report_text: str) -> dict[str, str]:
|
||||
text = report_text or ""
|
||||
match = _HANDOFF_SECTION_RE.search(text)
|
||||
if not match:
|
||||
return {}
|
||||
fields: dict[str, str] = {}
|
||||
for line in text[match.end() :].splitlines():
|
||||
stripped = line.strip().lstrip("-*").strip()
|
||||
if ":" not in stripped:
|
||||
continue
|
||||
key, value = stripped.split(":", 1)
|
||||
fields[key.strip().lower()] = value.strip()
|
||||
return fields
|
||||
|
||||
|
||||
def _narrative_before_handoff(report_text: str) -> str:
|
||||
text = report_text or ""
|
||||
match = _HANDOFF_SECTION_RE.search(text)
|
||||
if not match:
|
||||
return text
|
||||
return text[: match.start()]
|
||||
|
||||
|
||||
def _is_truthy(value: str) -> bool:
|
||||
lowered = (value or "").strip().lower()
|
||||
return lowered not in {"", "none", "n/a", "not applicable", "false", "no", "—", "-"}
|
||||
|
||||
|
||||
def _gate_active(text: str, fields: dict[str, str], session: dict) -> bool:
|
||||
if session.get("gate_fired") or session.get("already_landed_gate_fired"):
|
||||
return True
|
||||
eligibility = (fields.get("eligibility class") or "").upper()
|
||||
if ALREADY_LANDED_STATE in eligibility:
|
||||
return True
|
||||
return ALREADY_LANDED_STATE.lower() in text.lower()
|
||||
|
||||
|
||||
def assess_already_landed_handoff_report(
|
||||
report_text: str,
|
||||
*,
|
||||
handoff_session: dict | None = None,
|
||||
command_log: list | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""Reject stale handoff fields and narrative/handoff drift after the gate (#299)."""
|
||||
text = report_text or ""
|
||||
session = dict(handoff_session or {})
|
||||
fields = _handoff_field_map(text)
|
||||
reasons: list[str] = []
|
||||
|
||||
if not _gate_active(text, fields, session):
|
||||
return {
|
||||
"proven": True,
|
||||
"block": False,
|
||||
"reasons": [],
|
||||
"gate_active": False,
|
||||
"safe_next_action": "proceed",
|
||||
}
|
||||
|
||||
for stale_field in _STALE_HANDOFF_FIELDS:
|
||||
if stale_field in fields:
|
||||
reasons.append(
|
||||
f"already-landed handoff must not include stale field "
|
||||
f"'{stale_field.title()}' (#299)"
|
||||
)
|
||||
|
||||
if _LEGACY_WORKSPACE_NONE_RE.search(text):
|
||||
reasons.append(
|
||||
"already-landed handoff must not include legacy "
|
||||
"'Workspace mutations: None' (#299)"
|
||||
)
|
||||
|
||||
ref_commands = git_ref_mutating_commands(command_log or session.get("command_log"))
|
||||
mutations_observed = bool(
|
||||
ref_commands
|
||||
or session.get("mutations_observed")
|
||||
or session.get("mcp_mutations")
|
||||
or session.get("review_mutations")
|
||||
)
|
||||
if mutations_observed and _LEGACY_MUTATIONS_NONE_RE.search(text):
|
||||
reasons.append(
|
||||
"already-landed handoff must not claim 'Mutations: None' when "
|
||||
"git ref, MCP, review, merge, or cleanup mutations occurred (#299)"
|
||||
)
|
||||
|
||||
git_ref_value = fields.get("git ref mutations", "").strip().lower()
|
||||
if ref_commands and (not git_ref_value or git_ref_value == "none"):
|
||||
reasons.append(
|
||||
"git fetch/ref updates must be reported under Git ref mutations (#299)"
|
||||
)
|
||||
|
||||
reviewed_sha = fields.get("reviewed head sha", "")
|
||||
if reviewed_sha and _is_truthy(reviewed_sha):
|
||||
reasons.append(
|
||||
"already-landed handoff must use 'Reviewed head SHA: none' (#299)"
|
||||
)
|
||||
|
||||
worktree_used = fields.get("review worktree used", "")
|
||||
if worktree_used and _is_truthy(worktree_used):
|
||||
reasons.append(
|
||||
"already-landed handoff must set Review worktree used: false (#299)"
|
||||
)
|
||||
|
||||
candidate_sha = fields.get("candidate head sha", "")
|
||||
if not candidate_sha or candidate_sha.lower() in {"none", "n/a", "unknown"}:
|
||||
reasons.append(
|
||||
"already-landed handoff must include Candidate head SHA (#299)"
|
||||
)
|
||||
|
||||
if _FORBIDDEN_REVIEW_STATES_RE.search(text):
|
||||
reasons.append(
|
||||
"already-landed handoff must not claim approved/merged/ready-to-merge (#299)"
|
||||
)
|
||||
|
||||
narrative = _narrative_before_handoff(text)
|
||||
handoff_eligibility = (fields.get("eligibility class") or "").strip()
|
||||
narrative_eligibility = (
|
||||
_NARRATIVE_ELIGIBILITY_RE.search(narrative).group(1).strip()
|
||||
if _NARRATIVE_ELIGIBILITY_RE.search(narrative)
|
||||
else ""
|
||||
)
|
||||
if handoff_eligibility and narrative_eligibility:
|
||||
if handoff_eligibility.upper() != narrative_eligibility.upper():
|
||||
reasons.append(
|
||||
"narrative Eligibility class disagrees with controller handoff (#299)"
|
||||
)
|
||||
|
||||
narrative_reviewed = (
|
||||
_NARRATIVE_REVIEWED_SHA_RE.search(narrative).group(1).strip()
|
||||
if _NARRATIVE_REVIEWED_SHA_RE.search(narrative)
|
||||
else ""
|
||||
)
|
||||
if narrative_reviewed and reviewed_sha:
|
||||
if narrative_reviewed.lower() != reviewed_sha.lower():
|
||||
reasons.append(
|
||||
"narrative Reviewed head SHA disagrees with controller handoff (#299)"
|
||||
)
|
||||
|
||||
narrative_worktree = (
|
||||
_NARRATIVE_WORKTREE_RE.search(narrative).group(1).strip()
|
||||
if _NARRATIVE_WORKTREE_RE.search(narrative)
|
||||
else ""
|
||||
)
|
||||
if narrative_worktree and worktree_used:
|
||||
if narrative_worktree.lower() != worktree_used.lower():
|
||||
reasons.append(
|
||||
"narrative Review worktree used disagrees with controller handoff (#299)"
|
||||
)
|
||||
|
||||
git_ref_narrative = "git ref mutations" in narrative.lower()
|
||||
if git_ref_narrative and git_ref_value:
|
||||
if "none" in git_ref_value and ref_commands:
|
||||
reasons.append(
|
||||
"narrative and handoff disagree on git ref mutation reporting (#299)"
|
||||
)
|
||||
|
||||
proven = not reasons
|
||||
return {
|
||||
"proven": proven,
|
||||
"block": not proven,
|
||||
"reasons": list(dict.fromkeys(reasons)),
|
||||
"gate_active": True,
|
||||
"safe_next_action": (
|
||||
"emit canonical ALREADY_LANDED_RECONCILE_REQUIRED handoff without "
|
||||
"stale review/merge fields; align narrative and controller handoff"
|
||||
if reasons
|
||||
else "proceed"
|
||||
),
|
||||
}
|
||||
@@ -140,3 +140,9 @@ def test_non_mergeable_skip_verifier_exported():
|
||||
from review_proofs import assess_non_mergeable_skip_proof
|
||||
|
||||
assert callable(assess_non_mergeable_skip_proof)
|
||||
|
||||
|
||||
def test_already_landed_handoff_verifier_exported():
|
||||
from review_proofs import assess_already_landed_handoff_report
|
||||
|
||||
assert callable(assess_already_landed_handoff_report)
|
||||
|
||||
@@ -0,0 +1,131 @@
|
||||
"""Tests for already-landed handoff verifier (#299)."""
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||
|
||||
from reviewer_already_landed_handoff import ( # noqa: E402
|
||||
ALREADY_LANDED_STATE,
|
||||
assess_already_landed_handoff_report,
|
||||
)
|
||||
|
||||
|
||||
def _good_report() -> str:
|
||||
return "\n".join([
|
||||
"## PR reconciliation summary",
|
||||
f"Eligibility class: {ALREADY_LANDED_STATE}",
|
||||
"Candidate head SHA: 2a544f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||
"Reviewed head SHA: none",
|
||||
"Review worktree used: false",
|
||||
"Ancestor proof: PR head is ancestor of prgs/master",
|
||||
"",
|
||||
"## Controller Handoff",
|
||||
"- Selected PR: #278",
|
||||
f"- Eligibility class: {ALREADY_LANDED_STATE}",
|
||||
"- Candidate head SHA: 2a544f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||
"- Reviewed head SHA: none",
|
||||
"- Target branch: master",
|
||||
"- Target branch SHA: abc123def4567890abcdef1234567890abcdef12",
|
||||
"- Review worktree used: false",
|
||||
"- Git ref mutations: git fetch prgs master",
|
||||
"- Review decision: none",
|
||||
"- Merge result: none",
|
||||
"- Linked issue status: open",
|
||||
"- Safe next action: reconcile open PR and linked issue",
|
||||
])
|
||||
|
||||
|
||||
class TestAlreadyLandedHandoff(unittest.TestCase):
|
||||
def test_clean_handoff_passes(self):
|
||||
result = assess_already_landed_handoff_report(_good_report())
|
||||
self.assertTrue(result["proven"], result["reasons"])
|
||||
self.assertTrue(result["gate_active"])
|
||||
|
||||
def test_inactive_gate_skips_checks(self):
|
||||
report = "## Controller Handoff\n- Selected PR: #12\n- Review decision: approve"
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertTrue(result["proven"])
|
||||
self.assertFalse(result["gate_active"])
|
||||
|
||||
def test_rejects_stale_pinned_reviewed_head(self):
|
||||
report = _good_report().replace(
|
||||
"- Candidate head SHA:",
|
||||
"- Pinned reviewed head: 2a544f582026b72a229d59a172c0a63ac4aaeaf9\n"
|
||||
"- Candidate head SHA:",
|
||||
)
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("pinned reviewed head" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_rejects_scratch_worktree_used(self):
|
||||
report = _good_report() + "\n- Scratch worktree used: false"
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("scratch worktree" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_rejects_mutations_none_with_git_fetch(self):
|
||||
report = _good_report().replace(
|
||||
"- Git ref mutations: git fetch prgs master",
|
||||
"- Git ref mutations: none\n- Mutations: None",
|
||||
)
|
||||
result = assess_already_landed_handoff_report(
|
||||
report,
|
||||
command_log=[{"command": "git fetch prgs master", "performed": True}],
|
||||
)
|
||||
self.assertFalse(result["proven"])
|
||||
joined = " ".join(result["reasons"]).lower()
|
||||
self.assertIn("mutations", joined)
|
||||
self.assertIn("git ref", joined)
|
||||
|
||||
def test_rejects_workspace_mutations_none(self):
|
||||
report = _good_report() + "\n- Workspace mutations: None"
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("workspace mutations" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_rejects_reviewed_head_sha_when_gate_fired(self):
|
||||
report = _good_report().replace("- Reviewed head SHA: none", "- Reviewed head SHA: abc123")
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("reviewed head sha" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_rejects_narrative_handoff_eligibility_mismatch(self):
|
||||
report = _good_report().replace(
|
||||
f"Eligibility class: {ALREADY_LANDED_STATE}",
|
||||
"Eligibility class: NORMAL_REVIEW_CANDIDATE",
|
||||
1,
|
||||
)
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("eligibility class" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_session_gate_fired_without_text_marker(self):
|
||||
report = "## Controller Handoff\n- Selected PR: #1\n- Pinned reviewed head: abc"
|
||||
result = assess_already_landed_handoff_report(
|
||||
report,
|
||||
handoff_session={"gate_fired": True},
|
||||
)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(result["gate_active"])
|
||||
|
||||
def test_infra_stop_style_normal_handoff_not_blocked(self):
|
||||
report = "\n".join([
|
||||
"## Controller Handoff",
|
||||
"- Selected PR: #12",
|
||||
"- Review decision: request_changes",
|
||||
"- Pinned reviewed head: abc123def4567890abcdef1234567890abcdef12",
|
||||
])
|
||||
result = assess_already_landed_handoff_report(report)
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
|
||||
class TestExport(unittest.TestCase):
|
||||
def test_review_proofs_reexport(self):
|
||||
from review_proofs import assess_already_landed_handoff_report as exported
|
||||
|
||||
self.assertTrue(callable(exported))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user