feat: reject stale already-landed controller handoff fields (Closes #299) #371
@@ -1704,6 +1704,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
if report_text
|
if report_text
|
||||||
else {"proven": True, "block": False, "reasons": [], "violations": []}
|
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": []}
|
||||||
|
)
|
||||||
inventory_worktree = (
|
inventory_worktree = (
|
||||||
assess_inventory_worktree_report(report_text)
|
assess_inventory_worktree_report(report_text)
|
||||||
if report_text
|
if report_text
|
||||||
@@ -1870,6 +1875,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
"queue-status report violates loaded workflow proof gates (#339)"
|
"queue-status report violates loaded workflow proof gates (#339)"
|
||||||
)
|
)
|
||||||
downgrade_reasons.extend(queue_status_report.get("reasons", []))
|
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 inventory_worktree.get("proven"):
|
if not inventory_worktree.get("proven"):
|
||||||
downgrade_reasons.append(
|
downgrade_reasons.append(
|
||||||
"inventory pagination or worktree fields inconsistent (#293)"
|
"inventory pagination or worktree fields inconsistent (#293)"
|
||||||
@@ -1968,6 +1978,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination,
|
|||||||
"queue_status_violations": list(
|
"queue_status_violations": list(
|
||||||
queue_status_report.get("violations") or []
|
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 []
|
||||||
|
),
|
||||||
"inventory_worktree_proven": bool(inventory_worktree.get("proven")),
|
"inventory_worktree_proven": bool(inventory_worktree.get("proven")),
|
||||||
"inventory_worktree_violations": list(
|
"inventory_worktree_violations": list(
|
||||||
inventory_worktree.get("reasons") or []
|
inventory_worktree.get("reasons") or []
|
||||||
@@ -5209,6 +5223,13 @@ def assess_validation_worktree_edit_report(report_text, **kwargs):
|
|||||||
return _assess(report_text, **kwargs)
|
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)
|
||||||
|
|
||||||
|
|
||||||
def assess_inventory_worktree_report(report_text, **kwargs):
|
def assess_inventory_worktree_report(report_text, **kwargs):
|
||||||
"""#293: prove inventory pagination and consistent worktree reporting."""
|
"""#293: prove inventory pagination and consistent worktree reporting."""
|
||||||
from reviewer_inventory_worktree import assess_inventory_worktree_report as _assess
|
from reviewer_inventory_worktree import assess_inventory_worktree_report as _assess
|
||||||
|
|||||||
@@ -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"
|
||||||
|
),
|
||||||
|
}
|
||||||
@@ -172,6 +172,12 @@ def test_validation_worktree_edit_verifier_exported():
|
|||||||
assert callable(assess_validation_worktree_edit_report)
|
assert callable(assess_validation_worktree_edit_report)
|
||||||
|
|
||||||
|
|
||||||
|
def test_already_landed_handoff_verifier_exported():
|
||||||
|
from review_proofs import assess_already_landed_handoff_report
|
||||||
|
|
||||||
|
assert callable(assess_already_landed_handoff_report)
|
||||||
|
|
||||||
|
|
||||||
def test_inventory_worktree_verifier_exported():
|
def test_inventory_worktree_verifier_exported():
|
||||||
from review_proofs import assess_inventory_worktree_report
|
from review_proofs import assess_inventory_worktree_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