feat: reject contradictory reviewer handoff ledgers (Closes #501) #506
@@ -929,6 +929,7 @@ When posting a Canonical Thread Handoff after a binding blocker:
|
||||
|
||||
## Related documents
|
||||
|
||||
- [`reviewer-handoff-consistency.md`](reviewer-handoff-consistency.md) — reject contradictory reviewer handoffs (#501).
|
||||
- [`issue-acceptance-gate.md`](issue-acceptance-gate.md) — controller issue-acceptance audit after PR merge (#500).
|
||||
- [`../skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md) — portable cross-project LLM workflow skill.
|
||||
- [`gitea-execution-profiles.md`](gitea-execution-profiles.md) — the profile model.
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
# Reviewer Handoff Consistency
|
||||
|
||||
Reviewer and final-review controller handoffs must not contradict themselves.
|
||||
A narrative that says a merge happened, a review was blocked, or a terminal
|
||||
mutation budget was consumed must match the mutation ledger fields in the same
|
||||
handoff.
|
||||
|
||||
## What gets validated
|
||||
|
||||
`reviewer_handoff_consistency.assess_reviewer_handoff_consistency()` checks:
|
||||
|
||||
- merge claims appear under `Merge mutations` or `MCP/Gitea mutations`
|
||||
- terminal-mutation-budget claims name the exact prior mutation in the ledger
|
||||
- blocked review submission is not paired with "final decision marked"
|
||||
- reviewer lease acquisition includes a `Review decision`
|
||||
- blocked/rejected mutations include proof fields:
|
||||
- tool called
|
||||
- mutation attempted
|
||||
- mutation rejected
|
||||
- no server-side state changed
|
||||
|
||||
`final_report_validator` applies this as `reviewer.handoff_consistency` on
|
||||
`review_pr` reports and fails closed.
|
||||
|
||||
## Blocked review template
|
||||
|
||||
When `gitea_submit_pr_review` fails closed, use
|
||||
`reviewer_handoff_consistency.render_blocked_review_handoff_template()` or the
|
||||
copy in
|
||||
[`skills/llm-project-workflow/templates/blocked-review-handoff.md`](../skills/llm-project-workflow/templates/blocked-review-handoff.md).
|
||||
|
||||
## Related
|
||||
|
||||
- #331 — file-mutation ledger alignment
|
||||
- #501 — contradictory narrative vs ledger detection
|
||||
@@ -13,6 +13,7 @@ from typing import Any, Callable
|
||||
|
||||
import issue_acceptance_gate
|
||||
import issue_lock_provenance
|
||||
import reviewer_handoff_consistency
|
||||
from mcp_native_cleanup_proof import assess_mcp_native_cleanup_proof
|
||||
from post_merge_cleanup_proof import assess_post_merge_cleanup_proof
|
||||
from review_proofs import (
|
||||
@@ -1059,6 +1060,25 @@ def _rule_reviewer_validation_structured(
|
||||
)
|
||||
|
||||
|
||||
def _rule_reviewer_handoff_consistency(report_text: str) -> list[dict[str, str]]:
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(
|
||||
report_text
|
||||
)
|
||||
if result.get("proven"):
|
||||
return []
|
||||
return _findings_from_reasons(
|
||||
"reviewer.handoff_consistency",
|
||||
result.get("reasons") or [],
|
||||
field="Review mutations",
|
||||
severity="block",
|
||||
safe_next_action=(
|
||||
"rewrite reviewer handoff so narrative claims match the mutation "
|
||||
"ledger; use the blocked-review handoff template when submission "
|
||||
"was rejected"
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
def _rule_reviewer_mutation_ledger(
|
||||
report_text: str,
|
||||
*,
|
||||
@@ -1300,6 +1320,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = {
|
||||
_rule_reviewer_already_landed_eligible,
|
||||
_rule_reviewer_already_landed_state,
|
||||
_rule_reviewer_target_branch_freshness,
|
||||
_rule_reviewer_handoff_consistency,
|
||||
_rule_reviewer_workflow_load_boundary,
|
||||
_rule_reviewer_mutation_ledger,
|
||||
_rule_reviewer_review_mutation,
|
||||
|
||||
@@ -0,0 +1,186 @@
|
||||
"""Reviewer handoff consistency validation (#501).
|
||||
|
||||
Detects contradictions between narrative claims and mutation ledger fields in
|
||||
reviewer/final-review controller handoffs. Pure validation only — does not post
|
||||
comments or mutate Gitea state.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
|
||||
_HANDOFF_FIELD_RE = re.compile(
|
||||
r"^\s*[-*]\s*([^:]+):\s*(.*)$",
|
||||
re.MULTILINE | re.IGNORECASE,
|
||||
)
|
||||
|
||||
_MERGE_NARRATIVE_RE = re.compile(
|
||||
r"\b(?:merged\s+pr\s*#|pr\s+#\d+\s+(?:was\s+)?merged|"
|
||||
r"merge\s+result\s*:\s*merged|successfully\s+merged|"
|
||||
r"terminal\s+mutation\s+budget.{0,80}\bmerge)\b",
|
||||
re.IGNORECASE | re.DOTALL,
|
||||
)
|
||||
_REVIEW_SUBMIT_BLOCKED_RE = re.compile(
|
||||
r"\b(?:review\s+submission\s+blocked|submit_pr_review\s+(?:failed|blocked)|"
|
||||
r"gitea_submit_pr_review\s+(?:failed|blocked|not\s+(?:attempted|performed))|"
|
||||
r"review\s+not\s+submitted|could\s+not\s+submit\s+review)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_FINAL_DECISION_MARKED_RE = re.compile(
|
||||
r"\b(?:gitea_mark_final_review_decision|final\s+review\s+decision\s+marked|"
|
||||
r"server-side\s+final\s+decision\s+marked|marked\s+final\s+review\s+decision)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_TERMINAL_BUDGET_CONSUMED_RE = re.compile(
|
||||
r"\b(?:terminal\s+mutation\s+budget\s+(?:already\s+)?consumed|"
|
||||
r"single[- ]terminal\s+mutation\s+(?:already\s+)?consumed|"
|
||||
r"mutation\s+budget\s+exhausted)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_LEASE_NARRATIVE_RE = re.compile(
|
||||
r"\b(?:reviewer\s+lease\s+acquired|acquired\s+reviewer\s+pr\s+lease|"
|
||||
r"gitea_acquire_reviewer_pr_lease)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_REJECTED_MUTATION_RE = re.compile(
|
||||
r"\b(?:mutation\s+rejected|tool\s+failed\s+closed|blocked\s+before\s+mutation|"
|
||||
r"no\s+server-side\s+state\s+changed)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_MERGE_IN_LEDGER_RE = re.compile(r"\bmerge\b", re.IGNORECASE)
|
||||
_REVIEW_SUBMITTED_IN_LEDGER_RE = re.compile(
|
||||
r"\b(?:submitted|approve|request_changes|review\s+submitted)\b",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_NONE_VALUE_RE = re.compile(r"^\s*(?:none|not\s+attempted|n/a)\s*$", re.IGNORECASE)
|
||||
|
||||
_BLOCKED_REVIEW_PROOF_FIELDS = (
|
||||
"tool called",
|
||||
"mutation attempted",
|
||||
"mutation rejected",
|
||||
"no server-side state changed",
|
||||
)
|
||||
|
||||
|
||||
def render_blocked_review_handoff_template() -> str:
|
||||
"""Return a corrected handoff template for blocked review submissions."""
|
||||
return """## Blocked Review Submission Handoff
|
||||
|
||||
- Tool called: gitea_submit_pr_review
|
||||
- Mutation attempted: yes
|
||||
- Mutation rejected: yes
|
||||
- No server-side state changed: confirmed
|
||||
- Proof/source: <paste exact tool error or gate reason>
|
||||
- Prior terminal mutation that consumed budget: <name exact prior mutation or none>
|
||||
- Review decision (local intent): <approve | request_changes | comment only>
|
||||
- Server-side final decision marked: <yes with tool proof | no>
|
||||
- Gitea review submitted: no
|
||||
- Gitea review blocked because: <exact gate/error>
|
||||
- Review mutations: none (submission blocked)
|
||||
- MCP/Gitea mutations: <list only mutations that actually occurred>
|
||||
- Safe next action: rewrite handoff with consistent ledger before posting
|
||||
"""
|
||||
|
||||
|
||||
def _handoff_fields(text: str | None) -> dict[str, str]:
|
||||
fields: dict[str, str] = {}
|
||||
for match in _HANDOFF_FIELD_RE.finditer(text or ""):
|
||||
key = match.group(1).strip().lower()
|
||||
value = match.group(2).strip()
|
||||
fields[key] = value
|
||||
return fields
|
||||
|
||||
|
||||
def _is_none_value(value: str | None) -> bool:
|
||||
return bool(_NONE_VALUE_RE.match(value or ""))
|
||||
|
||||
|
||||
def _ledger_mentions_merge(*values: str | None) -> bool:
|
||||
blob = " ".join(v for v in values if v)
|
||||
return bool(blob) and bool(_MERGE_IN_LEDGER_RE.search(blob))
|
||||
|
||||
|
||||
def _ledger_mentions_review_submission(*values: str | None) -> bool:
|
||||
blob = " ".join(v for v in values if v)
|
||||
return bool(blob) and bool(_REVIEW_SUBMITTED_IN_LEDGER_RE.search(blob))
|
||||
|
||||
|
||||
def assess_reviewer_handoff_consistency(report_text: str | None) -> dict:
|
||||
"""Validate reviewer handoff narrative against mutation ledger fields."""
|
||||
text = report_text or ""
|
||||
fields = _handoff_fields(text)
|
||||
reasons: list[str] = []
|
||||
|
||||
review_mutations = fields.get("review mutations", "")
|
||||
merge_mutations = fields.get("merge mutations", "")
|
||||
mcp_mutations = fields.get("mcp/gitea mutations", "")
|
||||
review_decision = fields.get("review decision", "")
|
||||
|
||||
if _MERGE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge(
|
||||
merge_mutations, mcp_mutations, review_mutations
|
||||
):
|
||||
reasons.append(
|
||||
"narrative claims a merge occurred but mutation ledger omits merge"
|
||||
)
|
||||
|
||||
if _TERMINAL_BUDGET_CONSUMED_RE.search(text):
|
||||
if _ledger_mentions_merge(merge_mutations, mcp_mutations):
|
||||
pass
|
||||
elif _LEASE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge(
|
||||
merge_mutations, mcp_mutations
|
||||
):
|
||||
reasons.append(
|
||||
"terminal mutation budget attributed to merge but ledger only "
|
||||
"shows lease or non-merge activity"
|
||||
)
|
||||
elif not _ledger_mentions_merge(merge_mutations, mcp_mutations):
|
||||
reasons.append(
|
||||
"terminal mutation budget consumed claim must identify the "
|
||||
"exact prior mutation in the ledger"
|
||||
)
|
||||
|
||||
if _REVIEW_SUBMIT_BLOCKED_RE.search(text) and _FINAL_DECISION_MARKED_RE.search(text):
|
||||
reasons.append(
|
||||
"handoff claims review submission blocked but also claims "
|
||||
"server-side final decision was marked"
|
||||
)
|
||||
|
||||
lease_claimed = _LEASE_NARRATIVE_RE.search(text) or (
|
||||
not _is_none_value(mcp_mutations)
|
||||
and "lease" in mcp_mutations.lower()
|
||||
)
|
||||
if lease_claimed and _is_none_value(review_decision):
|
||||
reasons.append(
|
||||
"reviewer lease acquired but Review decision field is missing or none"
|
||||
)
|
||||
|
||||
if _REVIEW_SUBMIT_BLOCKED_RE.search(text) or _REJECTED_MUTATION_RE.search(text):
|
||||
lower = text.lower()
|
||||
missing_proof = [
|
||||
field
|
||||
for field in _BLOCKED_REVIEW_PROOF_FIELDS
|
||||
if field not in lower
|
||||
]
|
||||
if missing_proof:
|
||||
reasons.append(
|
||||
"blocked/rejected mutation claim missing proof fields: "
|
||||
+ ", ".join(missing_proof)
|
||||
)
|
||||
|
||||
if (
|
||||
_FINAL_DECISION_MARKED_RE.search(text)
|
||||
and _is_none_value(review_mutations)
|
||||
and not _ledger_mentions_review_submission(mcp_mutations)
|
||||
and not _REVIEW_SUBMIT_BLOCKED_RE.search(text)
|
||||
):
|
||||
reasons.append(
|
||||
"final review decision marked but Review mutations ledger is empty"
|
||||
)
|
||||
|
||||
proven = not reasons
|
||||
return {
|
||||
"proven": proven,
|
||||
"block": not proven,
|
||||
"reasons": reasons,
|
||||
"fields": fields,
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
# Blocked review submission handoff
|
||||
|
||||
Use when `gitea_submit_pr_review` or the terminal mutation gate blocks review
|
||||
submission.
|
||||
|
||||
```text
|
||||
## Blocked Review Submission Handoff
|
||||
|
||||
- Tool called: gitea_submit_pr_review
|
||||
- Mutation attempted: yes
|
||||
- Mutation rejected: yes
|
||||
- No server-side state changed: confirmed
|
||||
- Proof/source: <paste exact tool error or gate reason>
|
||||
- Prior terminal mutation that consumed budget: <exact prior mutation or none>
|
||||
- Review decision (local intent): <approve | request_changes | comment only>
|
||||
- Server-side final decision marked: <yes with tool proof | no>
|
||||
- Gitea review submitted: no
|
||||
- Gitea review blocked because: <exact gate/error>
|
||||
- Review mutations: none (submission blocked)
|
||||
- MCP/Gitea mutations: <only mutations that actually occurred>
|
||||
- Safe next action: rewrite handoff with consistent ledger before posting
|
||||
```
|
||||
@@ -0,0 +1,120 @@
|
||||
"""Tests for reviewer handoff consistency validation (#501)."""
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||
|
||||
import reviewer_handoff_consistency # noqa: E402
|
||||
from final_report_validator import assess_final_report_validator # noqa: E402
|
||||
|
||||
|
||||
def _base_handoff(**overrides):
|
||||
lines = [
|
||||
"## Controller Handoff",
|
||||
"",
|
||||
"- Task: review PR #472",
|
||||
"- Review decision: request_changes",
|
||||
"- Review mutations: submitted request_changes review",
|
||||
"- Merge mutations: none",
|
||||
"- MCP/Gitea mutations: reviewer lease acquired",
|
||||
]
|
||||
text = "\n".join(lines)
|
||||
for key, value in overrides.items():
|
||||
token = f"- {key}:"
|
||||
if token in text:
|
||||
before, _, after = text.partition(token)
|
||||
rest = after.split("\n", 1)
|
||||
suffix = rest[1] if len(rest) > 1 else ""
|
||||
text = f"{before}{token} {value}" + (f"\n{suffix}" if suffix else "")
|
||||
else:
|
||||
text += f"\n- {key}: {value}"
|
||||
return text
|
||||
|
||||
|
||||
class TestReviewerHandoffConsistency(unittest.TestCase):
|
||||
def test_consistent_handoff_passes(self):
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(
|
||||
_base_handoff()
|
||||
)
|
||||
self.assertTrue(result["proven"], result["reasons"])
|
||||
|
||||
def test_claimed_merge_missing_from_ledger(self):
|
||||
report = _base_handoff(
|
||||
**{
|
||||
"Current status": "Merged PR #411 earlier; reviewing PR #472",
|
||||
"Merge mutations": "none",
|
||||
"MCP/Gitea mutations": "reviewer lease acquired",
|
||||
}
|
||||
)
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("merge" in r.lower() for r in result["reasons"]))
|
||||
|
||||
def test_terminal_budget_consumed_without_merge_in_ledger(self):
|
||||
report = _base_handoff(
|
||||
**{
|
||||
"Current status": "Terminal mutation budget already consumed by merge",
|
||||
"Merge mutations": "none",
|
||||
"MCP/Gitea mutations": "reviewer lease acquired",
|
||||
}
|
||||
)
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("terminal mutation budget" in r for r in result["reasons"]))
|
||||
|
||||
def test_review_blocked_but_final_decision_marked(self):
|
||||
report = _base_handoff(
|
||||
**{
|
||||
"Current status": "gitea_submit_pr_review blocked",
|
||||
"Review mutations": "none",
|
||||
}
|
||||
)
|
||||
report += "\nServer-side final decision marked via gitea_mark_final_review_decision."
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("blocked" in r and "final decision" in r for r in result["reasons"]))
|
||||
|
||||
def test_lease_without_review_decision(self):
|
||||
report = _base_handoff(**{"Review decision": "none"})
|
||||
report += "\nReviewer lease acquired for PR #472."
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("Review decision" in r for r in result["reasons"]))
|
||||
|
||||
def test_rejected_mutation_without_proof(self):
|
||||
report = _base_handoff(
|
||||
**{
|
||||
"Review mutations": "none",
|
||||
"MCP/Gitea mutations": "none",
|
||||
}
|
||||
)
|
||||
report += "\nReview submission blocked before mutation."
|
||||
result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(any("proof fields" in r for r in result["reasons"]))
|
||||
|
||||
def test_blocked_template_includes_required_fields(self):
|
||||
template = reviewer_handoff_consistency.render_blocked_review_handoff_template()
|
||||
for field in reviewer_handoff_consistency._BLOCKED_REVIEW_PROOF_FIELDS:
|
||||
self.assertIn(field, template.lower())
|
||||
|
||||
def test_final_report_validator_integration(self):
|
||||
report = _base_handoff(
|
||||
**{
|
||||
"Current status": "Merged PR #411; terminal mutation budget consumed",
|
||||
"Merge mutations": "none",
|
||||
}
|
||||
)
|
||||
result = assess_final_report_validator(report, "review_pr")
|
||||
self.assertTrue(
|
||||
any(
|
||||
f["rule_id"] == "reviewer.handoff_consistency"
|
||||
for f in result["findings"]
|
||||
),
|
||||
result["findings"],
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user