fix: resolve conflicts for PR #368 against current master
This commit is contained in:
+425
-2
@@ -26,6 +26,8 @@ from review_proofs import ( # noqa: E402
|
||||
assess_already_landed_report_state,
|
||||
assess_already_landed_review_gate,
|
||||
assess_author_pr_report,
|
||||
assess_full_suite_failure_approval_gate,
|
||||
assess_queue_ordering_proof,
|
||||
assess_reviewer_baseline_validation_proof,
|
||||
assess_capability_evidence,
|
||||
assess_capability_proof,
|
||||
@@ -942,13 +944,18 @@ class TestControllerHandoff(unittest.TestCase):
|
||||
self.assertIn("Safety", result["missing_fields"])
|
||||
|
||||
def test_review_role_requires_review_fields(self):
|
||||
result = assess_controller_handoff(self.BASE_HANDOFF, role="review")
|
||||
# Issue #320: review handoffs must not carry the legacy
|
||||
# 'Workspace mutations' line, so strip it from the shared base.
|
||||
review_base = "\n".join(
|
||||
line for line in self.BASE_HANDOFF.splitlines()
|
||||
if not line.startswith("- Workspace mutations:"))
|
||||
result = assess_controller_handoff(review_base, role="review")
|
||||
self.assertEqual(result["verdict"], "incomplete")
|
||||
self.assertIn("Pinned reviewed head", result["missing_fields"])
|
||||
self.assertIn("Worktree path", result["missing_fields"])
|
||||
self.assertIn("Merge result", result["missing_fields"])
|
||||
|
||||
complete = self.BASE_HANDOFF + "\n" + "\n".join([
|
||||
complete = review_base + "\n" + "\n".join([
|
||||
"- Selected PR: #999",
|
||||
"- Reviewer eligibility: passed",
|
||||
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||
@@ -960,6 +967,15 @@ class TestControllerHandoff(unittest.TestCase):
|
||||
"- Merge result: merged",
|
||||
"- Linked issue status: closed",
|
||||
"- Cleanup status: branch deleted",
|
||||
"- File edits by reviewer: none",
|
||||
"- Worktree/index mutations: created and removed review worktree",
|
||||
"- Git ref mutations: git fetch prgs",
|
||||
"- MCP/Gitea mutations: none",
|
||||
"- Review mutations: one approve on #999",
|
||||
"- Merge mutations: PR #999 merged",
|
||||
"- Cleanup mutations: removed review worktree",
|
||||
"- External-state mutations: none",
|
||||
"- Read-only diagnostics: git log, git diff",
|
||||
])
|
||||
result = assess_controller_handoff(complete, role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
@@ -1082,6 +1098,144 @@ class TestControllerHandoff(unittest.TestCase):
|
||||
self.assertEqual(res2["verdict"], "complete")
|
||||
|
||||
|
||||
class TestReviewHandoffPreciseMutationCategories(unittest.TestCase):
|
||||
"""Issue #320: reviewer handoffs drop legacy 'Workspace mutations'."""
|
||||
|
||||
REVIEW_BASE = "\n".join([
|
||||
"## Controller Handoff",
|
||||
"",
|
||||
"- Task: review PR #999",
|
||||
"- Repo: Scaled-Tech-Consulting/Gitea-Tools",
|
||||
"- Role: reviewer",
|
||||
"- Identity: jcwalker3 / prgs-reviewer",
|
||||
"- Issue/PR: PR #999",
|
||||
"- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||
"- Files changed: review_proofs.py",
|
||||
"- Validation: 700 passed, 6 skipped",
|
||||
"- Mutations: one review submitted",
|
||||
"- Current status: review complete",
|
||||
"- Blockers: none",
|
||||
"- Next: controller decides",
|
||||
"- Safety: no self-review; no self-merge; no secrets",
|
||||
"- Selected PR: #999",
|
||||
"- Reviewer eligibility: passed",
|
||||
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
|
||||
"- Worktree path: /repo/branches/review-pr-999",
|
||||
"- Worktree dirty: no",
|
||||
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
|
||||
"- Unrelated local mutations: none",
|
||||
"- Review decision: approve",
|
||||
"- Merge result: none",
|
||||
"- Linked issue status: open",
|
||||
"- Cleanup status: worktree removed",
|
||||
])
|
||||
|
||||
PRECISE_CATEGORIES = "\n".join([
|
||||
"- File edits by reviewer: none",
|
||||
"- Worktree/index mutations: created and removed review worktree",
|
||||
"- Git ref mutations: git fetch prgs",
|
||||
"- MCP/Gitea mutations: none",
|
||||
"- Review mutations: one approve on #999",
|
||||
"- Merge mutations: none",
|
||||
"- Cleanup mutations: removed review worktree",
|
||||
"- External-state mutations: none",
|
||||
"- Read-only diagnostics: git log, git diff",
|
||||
])
|
||||
|
||||
def _complete_handoff(self, **overrides):
|
||||
text = self.REVIEW_BASE + "\n" + self.PRECISE_CATEGORIES
|
||||
for old, new in overrides.items():
|
||||
text = text.replace(old, new)
|
||||
return text
|
||||
|
||||
def test_review_handoff_with_precise_categories_is_complete(self):
|
||||
result = assess_controller_handoff(
|
||||
self._complete_handoff(), role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
self.assertFalse(result["downgraded"])
|
||||
|
||||
def test_review_handoff_rejects_legacy_workspace_mutations_none(self):
|
||||
text = self._complete_handoff() + "\n- Workspace mutations: none"
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "incomplete")
|
||||
self.assertTrue(result["downgraded"])
|
||||
self.assertTrue(any(
|
||||
"workspace mutations" in reason.lower()
|
||||
for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_review_handoff_rejects_legacy_workspace_mutations_any_value(self):
|
||||
text = (self._complete_handoff()
|
||||
+ "\n- Workspace mutations: edited review_proofs.py")
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "incomplete")
|
||||
self.assertTrue(any(
|
||||
"workspace mutations" in reason.lower()
|
||||
for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_review_handoff_missing_precise_categories_is_incomplete(self):
|
||||
result = assess_controller_handoff(self.REVIEW_BASE, role="review")
|
||||
self.assertEqual(result["verdict"], "incomplete")
|
||||
self.assertIn("File edits by reviewer", result["missing_fields"])
|
||||
self.assertIn("Worktree/index mutations", result["missing_fields"])
|
||||
self.assertIn("Read-only diagnostics", result["missing_fields"])
|
||||
|
||||
def test_review_handoff_does_not_require_workspace_mutations(self):
|
||||
result = assess_controller_handoff(
|
||||
self._complete_handoff(), role="review")
|
||||
self.assertNotIn("Workspace mutations", result["missing_fields"])
|
||||
|
||||
def test_request_changes_handoff_with_precise_categories_is_complete(self):
|
||||
text = self._complete_handoff(**{
|
||||
"- Review decision: approve": "- Review decision: request-changes",
|
||||
"- Review mutations: one approve on #999":
|
||||
"- Review mutations: one request_changes on #999",
|
||||
})
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
|
||||
def test_merge_handoff_with_precise_categories_is_complete(self):
|
||||
text = self._complete_handoff(**{
|
||||
"- Merge result: none": "- Merge result: merged",
|
||||
"- Merge mutations: none": "- Merge mutations: PR #999 merged",
|
||||
})
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
|
||||
def test_fetch_only_handoff_with_precise_categories_is_complete(self):
|
||||
text = self._complete_handoff(**{
|
||||
"- Review decision: approve": "- Review decision: none",
|
||||
"- Review mutations: one approve on #999": "- Review mutations: none",
|
||||
"- Worktree/index mutations: created and removed review worktree":
|
||||
"- Worktree/index mutations: none",
|
||||
"- Cleanup mutations: removed review worktree":
|
||||
"- Cleanup mutations: none",
|
||||
"- Mutations: one review submitted": "- Mutations: fetch only",
|
||||
})
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
|
||||
def test_blocked_handoff_with_precise_categories_is_complete(self):
|
||||
text = self._complete_handoff(**{
|
||||
"- Blockers: none": "- Blockers: infra_stop (registry unreachable)",
|
||||
"- Review decision: approve": "- Review decision: none",
|
||||
"- Review mutations: one approve on #999": "- Review mutations: none",
|
||||
})
|
||||
result = assess_controller_handoff(text, role="review")
|
||||
self.assertEqual(result["verdict"], "complete")
|
||||
|
||||
def test_author_role_still_requires_workspace_mutations(self):
|
||||
# Non-review roles keep the legacy contract until their own issues
|
||||
# migrate them.
|
||||
author = TestControllerHandoff.BASE_HANDOFF
|
||||
stripped = "\n".join(
|
||||
line for line in author.splitlines()
|
||||
if not line.startswith("- Workspace mutations:"))
|
||||
result = assess_controller_handoff(stripped, role="author")
|
||||
self.assertEqual(result["verdict"], "incomplete")
|
||||
self.assertIn("Workspace mutations", result["missing_fields"])
|
||||
|
||||
|
||||
class TestReviewMutationFinalReport(unittest.TestCase):
|
||||
"""Final reports must list exactly one live review mutation."""
|
||||
@@ -2163,6 +2317,111 @@ class TestCreateIssueFinalReport(unittest.TestCase):
|
||||
self.assertTrue(result["downgraded"])
|
||||
|
||||
|
||||
class TestQueueOrderingProof(unittest.TestCase):
|
||||
"""Issue #321: reviewer queue selection must prove ordering."""
|
||||
|
||||
def test_next_eligible_claim_without_policy_fails(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text="Selected the next eligible PR: PR #286.",
|
||||
)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(result["claims_selection"])
|
||||
|
||||
def test_next_eligible_claim_with_policy_passes(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text=(
|
||||
"Queue ordering policy: oldest PR number first\n"
|
||||
"Selected the next eligible PR: PR #286."
|
||||
),
|
||||
)
|
||||
self.assertTrue(result["proven"])
|
||||
self.assertEqual(result["policy"], "oldest PR number first")
|
||||
|
||||
def test_newest_first_api_with_oldest_first_selection_passes(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text=(
|
||||
"Queue ordering policy: oldest PR number first\n"
|
||||
"API order was newest-first (PR #291 before PR #286).\n"
|
||||
"Selected the next eligible PR: PR #286."
|
||||
),
|
||||
queue_ordering={
|
||||
"policy": "oldest PR number first",
|
||||
"selected_pr_number": 286,
|
||||
"api_pr_numbers": [291, 286],
|
||||
},
|
||||
)
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
def test_earlier_actionable_pr_without_skip_reason_fails(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text=(
|
||||
"Queue ordering policy: oldest PR number first\n"
|
||||
"Selected the next eligible PR: PR #300."
|
||||
),
|
||||
queue_ordering={
|
||||
"policy": "oldest PR number first",
|
||||
"selected_pr_number": 300,
|
||||
"api_pr_numbers": [300, 286],
|
||||
},
|
||||
)
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_priority_override_requires_explanation(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text="Selected the next eligible PR: PR #300.",
|
||||
queue_ordering={
|
||||
"policy": "priority label",
|
||||
"selected_pr_number": 300,
|
||||
"api_pr_numbers": [286, 300],
|
||||
},
|
||||
)
|
||||
self.assertFalse(result["proven"])
|
||||
|
||||
def test_priority_override_with_explanation_passes(self):
|
||||
result = assess_queue_ordering_proof(
|
||||
report_text=(
|
||||
"Queue ordering policy: priority label\n"
|
||||
"Priority override: security label on PR #300.\n"
|
||||
"Selected the next eligible PR: PR #300."
|
||||
),
|
||||
queue_ordering={
|
||||
"policy": "priority label",
|
||||
"selected_pr_number": 300,
|
||||
"api_pr_numbers": [286, 300],
|
||||
"priority_override": "security label on PR #300",
|
||||
},
|
||||
)
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
def test_build_final_report_downgrades_missing_ordering_proof(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="Selected the next eligible PR: PR #286.",
|
||||
)
|
||||
self.assertNotEqual(report["grade"], "A")
|
||||
self.assertFalse(report["queue_ordering_proven"])
|
||||
|
||||
class TestReviewerBaselineValidationProof(unittest.TestCase):
|
||||
"""Issue #325: baseline validation must use branches/ worktrees."""
|
||||
|
||||
@@ -2414,6 +2673,170 @@ class TestAlreadyLandedReportState(unittest.TestCase):
|
||||
"Review decision: APPROVED\nMerge result: MERGED\n",
|
||||
gate_fired=False)
|
||||
self.assertTrue(result["complete"])
|
||||
|
||||
|
||||
class TestFullSuiteFailureApprovalGate(unittest.TestCase):
|
||||
"""Issue #323: full-suite failure blocks approval without baseline proof."""
|
||||
|
||||
ROOT = "/repo/Gitea-Tools"
|
||||
|
||||
def _good_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,
|
||||
"pr_suite_command": "venv/bin/pytest tests/",
|
||||
"baseline_suite_command": "venv/bin/pytest tests/",
|
||||
"new_tests_passed": True,
|
||||
"unrelated_explanation": (
|
||||
"failures touch agent_temp_artifacts only; PR changes "
|
||||
"review_proofs handoff fields"
|
||||
),
|
||||
}
|
||||
proof.update(overrides)
|
||||
return proof
|
||||
|
||||
def _good_report(self):
|
||||
return (
|
||||
"Full suite failed on PR worktree and on baseline.\n"
|
||||
"- Validation command: venv/bin/pytest tests/\n"
|
||||
f"- Working directory: {self.ROOT}/branches/review-pr-280\n"
|
||||
"Failures are pre-existing on master.\n"
|
||||
)
|
||||
|
||||
def test_full_suite_pass_allows_approval_without_baseline(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=True,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertTrue(result["approve_allowed"])
|
||||
self.assertFalse(result["block"])
|
||||
self.assertEqual(result["default_action"], "proceed")
|
||||
|
||||
def test_full_suite_failure_defaults_to_request_changes(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="request_changes",
|
||||
full_suite_passed=False,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["block"])
|
||||
self.assertEqual(result["default_action"], "request_changes")
|
||||
|
||||
def test_approval_with_failure_and_no_proof_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
self.assertEqual(result["default_action"], "request_changes")
|
||||
|
||||
def test_approval_with_matching_baseline_proof_allowed(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertTrue(result["approve_allowed"])
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
def test_mismatched_failure_signatures_block_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(
|
||||
failure_signatures_match=False,
|
||||
pr_failures=["tests/test_other.py::test_other"],
|
||||
),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_main_checkout_baseline_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(worktree_path=self.ROOT),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_missing_new_tests_proof_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(new_tests_passed=None),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(any(
|
||||
"new" in reason.lower() and "test" in reason.lower()
|
||||
for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_missing_unrelated_explanation_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(unrelated_explanation=""),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
|
||||
def test_missing_suite_commands_block_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(
|
||||
pr_suite_command="", baseline_suite_command=""),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(any(
|
||||
"command" in reason.lower() for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_unknown_suite_result_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=None,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_vague_same_as_master_claim_without_proof_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
report_text="Validation: failures same as master; approving.",
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_non_approve_decision_never_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="comment",
|
||||
full_suite_passed=None,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user