diff --git a/review_proofs.py b/review_proofs.py index b9a6646..fd77282 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1593,6 +1593,20 @@ HANDOFF_BASE_FIELDS = ( ("Safety", ("safety",)), ) +# Issue #320: reviewer handoffs replace the legacy ambiguous +# "Workspace mutations" field with these precise mutation categories. +HANDOFF_REVIEW_MUTATION_FIELDS = ( + ("File edits by reviewer", ("file edits by reviewer",)), + ("Worktree/index mutations", ("worktree/index mutations",)), + ("Git ref mutations", ("git ref mutations",)), + ("MCP/Gitea mutations", ("mcp/gitea mutations",)), + ("Review mutations", ("review mutations",)), + ("Merge mutations", ("merge mutations",)), + ("Cleanup mutations", ("cleanup mutations",)), + ("External-state mutations", ("external-state mutations",)), + ("Read-only diagnostics", ("read-only diagnostics",)), +) + HANDOFF_ROLE_FIELDS = { "review": ( ("Selected PR", ("selected pr",)), @@ -1608,7 +1622,7 @@ HANDOFF_ROLE_FIELDS = { ("Merge result", ("merge result",)), ("Linked issue status", ("linked issue status", "linked issue")), ("Cleanup status", ("cleanup status", "cleanup")), - ), + ) + HANDOFF_REVIEW_MUTATION_FIELDS, "author": ( ("Selected issue", ("selected issue",)), ("Issue lock proof", ("issue lock proof", "lock before diff")), @@ -1759,6 +1773,25 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): field for field in required if field[0] not in ("Workspace mutations", "Mutations") ] + if role == "review": + # Issue #320: reviewer handoffs use the precise mutation categories + # in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous + # "Workspace mutations" field, which is rejected below. + required = [ + field for field in required + if field[0] != "Workspace mutations" + ] + if any(label.startswith("workspace mutations") for label in labels): + return { + "verdict": "incomplete", + "downgraded": True, + "missing_fields": [], + "reasons": [ + "review handoff must not include legacy " + "'Workspace mutations' field; report the precise " + "mutation categories instead (issue #320)" + ], + } required.extend(HANDOFF_ROLE_FIELDS.get(role or "", ())) missing = [] diff --git a/tests/test_review_proofs.py b/tests/test_review_proofs.py index b4683dc..025d16f 100644 --- a/tests/test_review_proofs.py +++ b/tests/test_review_proofs.py @@ -939,13 +939,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", @@ -957,6 +962,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") @@ -1079,6 +1093,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."""