feat(reviewer): fail closed on dirty worktrees and forbidden git cleanup (#233)
Add reviewer_worktree proofs that block stash/reset/checkout -- cleanup of unrelated local files, require scratch worktree reporting when the main tree is dirty outside PR scope, and integrate the gate into final report grading and Controller Handoff review fields. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
Claude Opus 4.8
@@ -183,6 +183,24 @@ def _good_review_mutation():
|
||||
return assess_review_mutation_final_report(report, lock)
|
||||
|
||||
|
||||
def _good_worktree(**overrides):
|
||||
proof = {
|
||||
"worktree_path": "/repo/branches/review-feat-issue-224",
|
||||
"porcelain_status": "",
|
||||
"pr_scope_files": ["docs/wiki/Repositories.md"],
|
||||
"scratch_used": True,
|
||||
"scratch_path": "/repo/branches/review-feat-issue-224",
|
||||
"git_commands": [
|
||||
"git fetch prgs master feat/issue-224-wiki-proof-refresh",
|
||||
"git diff prgs/master...prgs/feat/issue-224-wiki-proof-refresh",
|
||||
],
|
||||
}
|
||||
proof.update(overrides)
|
||||
from reviewer_worktree import assess_reviewer_worktree_proof # noqa: E402
|
||||
|
||||
return assess_reviewer_worktree_proof(proof)
|
||||
|
||||
|
||||
def _good_role_boundary_179(**overrides):
|
||||
kwargs = {
|
||||
"task_role": "reviewer",
|
||||
@@ -595,6 +613,13 @@ class TestFinalReport(unittest.TestCase):
|
||||
"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,
|
||||
"scratch_path": "/repo/branches/review-feat-issue-224",
|
||||
},
|
||||
}
|
||||
kwargs.update(overrides)
|
||||
return build_final_report(**kwargs)
|
||||
@@ -899,12 +924,17 @@ class TestControllerHandoff(unittest.TestCase):
|
||||
result = assess_controller_handoff(self.BASE_HANDOFF, 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([
|
||||
"- 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: merged",
|
||||
"- Linked issue status: closed",
|
||||
@@ -1324,6 +1354,13 @@ class TestFinalReport179Bar(unittest.TestCase):
|
||||
"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,
|
||||
"scratch_path": "/repo/branches/review-feat-issue-224",
|
||||
},
|
||||
}
|
||||
kwargs.update(overrides)
|
||||
return build_final_report(**kwargs)
|
||||
|
||||
@@ -0,0 +1,166 @@
|
||||
"""Tests for reviewer worktree safety proofs (Issue #233)."""
|
||||
import sys
|
||||
import unittest
|
||||
|
||||
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
||||
|
||||
from reviewer_worktree import ( # noqa: E402
|
||||
assess_author_worktree_continuity,
|
||||
assess_reviewer_git_command_log,
|
||||
assess_reviewer_worktree_proof,
|
||||
files_outside_pr_scope,
|
||||
is_forbidden_reviewer_git_command,
|
||||
is_readonly_reviewer_git_command,
|
||||
parse_dirty_tracked_files,
|
||||
)
|
||||
|
||||
|
||||
class TestParseDirtyTrackedFiles(unittest.TestCase):
|
||||
def test_ignores_untracked_files(self):
|
||||
porcelain = "?? untracked.txt\n M tracked.py\n"
|
||||
self.assertEqual(parse_dirty_tracked_files(porcelain), ["tracked.py"])
|
||||
|
||||
def test_parses_renamed_paths(self):
|
||||
porcelain = "R old.py -> new.py\n"
|
||||
self.assertEqual(parse_dirty_tracked_files(porcelain), ["new.py"])
|
||||
|
||||
|
||||
class TestFilesOutsidePrScope(unittest.TestCase):
|
||||
def test_all_dirty_in_scope_is_clean(self):
|
||||
self.assertEqual(
|
||||
files_outside_pr_scope(
|
||||
["docs/wiki/Repositories.md"],
|
||||
["docs/wiki/Repositories.md"],
|
||||
),
|
||||
[],
|
||||
)
|
||||
|
||||
def test_unrelated_dirty_files_detected(self):
|
||||
self.assertEqual(
|
||||
files_outside_pr_scope(
|
||||
["review_proofs.py", "docs/wiki/Repositories.md"],
|
||||
["docs/wiki/Repositories.md"],
|
||||
),
|
||||
["review_proofs.py"],
|
||||
)
|
||||
|
||||
|
||||
class TestForbiddenGitCommands(unittest.TestCase):
|
||||
def test_blocks_stash_operations(self):
|
||||
self.assertTrue(is_forbidden_reviewer_git_command("git stash"))
|
||||
self.assertTrue(
|
||||
is_forbidden_reviewer_git_command(
|
||||
'git stash push -m "reviewer-temp-stash" -- tests/test_mcp_server.py'
|
||||
)
|
||||
)
|
||||
self.assertTrue(is_forbidden_reviewer_git_command("git stash pop"))
|
||||
self.assertTrue(is_forbidden_reviewer_git_command("git stash drop"))
|
||||
|
||||
def test_blocks_checkout_reset_and_clean(self):
|
||||
self.assertTrue(
|
||||
is_forbidden_reviewer_git_command(
|
||||
"git checkout -- review_proofs.py tests/test_mcp_server.py"
|
||||
)
|
||||
)
|
||||
self.assertTrue(is_forbidden_reviewer_git_command("git reset --hard"))
|
||||
self.assertTrue(is_forbidden_reviewer_git_command("git clean -fd"))
|
||||
|
||||
def test_allows_readonly_commands(self):
|
||||
for cmd in (
|
||||
"git fetch prgs master",
|
||||
"git status --porcelain",
|
||||
"git diff prgs/master...HEAD",
|
||||
"git rev-parse HEAD",
|
||||
"git -C /repo log -1",
|
||||
):
|
||||
with self.subTest(cmd=cmd):
|
||||
self.assertFalse(is_forbidden_reviewer_git_command(cmd))
|
||||
self.assertTrue(is_readonly_reviewer_git_command(cmd))
|
||||
|
||||
|
||||
class TestAssessReviewerWorktreeProof(unittest.TestCase):
|
||||
def test_clean_worktree_proceeds(self):
|
||||
result = assess_reviewer_worktree_proof({
|
||||
"worktree_path": "/repo/branches/review-pr-231",
|
||||
"porcelain_status": "",
|
||||
"pr_scope_files": ["docs/wiki/Repositories.md"],
|
||||
"scratch_used": False,
|
||||
"git_commands": ["git fetch prgs master", "git diff prgs/master...HEAD"],
|
||||
})
|
||||
self.assertTrue(result["proven"])
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
def test_dirty_unrelated_without_scratch_blocks(self):
|
||||
result = assess_reviewer_worktree_proof({
|
||||
"worktree_path": "/repo",
|
||||
"dirty_files": ["review_proofs.py", "tests/test_review_proofs.py"],
|
||||
"pr_scope_files": ["docs/wiki/Repositories.md"],
|
||||
"scratch_used": False,
|
||||
})
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(result["block"])
|
||||
self.assertIn("review_proofs.py", result["unrelated_dirty_files"][0])
|
||||
|
||||
def test_scratch_worktree_allows_dirty_main_repo(self):
|
||||
result = assess_reviewer_worktree_proof({
|
||||
"worktree_path": "/repo",
|
||||
"dirty_files": ["review_proofs.py"],
|
||||
"pr_scope_files": ["docs/wiki/Repositories.md"],
|
||||
"scratch_used": True,
|
||||
"scratch_path": "/repo/branches/review-feat-issue-224",
|
||||
})
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
def test_forbidden_command_history_blocks(self):
|
||||
result = assess_reviewer_worktree_proof({
|
||||
"worktree_path": "/repo/branches/review-pr-231",
|
||||
"porcelain_status": "",
|
||||
"git_commands": ["git stash push -m temp -- review_proofs.py"],
|
||||
})
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertTrue(result["forbidden_commands"])
|
||||
|
||||
def test_unrelated_mutations_claimed_blocks(self):
|
||||
result = assess_reviewer_worktree_proof({
|
||||
"worktree_path": "/repo",
|
||||
"porcelain_status": "",
|
||||
"unrelated_mutations_claimed": True,
|
||||
})
|
||||
self.assertFalse(result["proven"])
|
||||
|
||||
|
||||
class TestAuthorContinuity(unittest.TestCase):
|
||||
def test_author_may_keep_dirty_worktree(self):
|
||||
result = assess_author_worktree_continuity({
|
||||
"task_role": "author",
|
||||
"dirty_files": ["feat.py"],
|
||||
})
|
||||
self.assertTrue(result["allowed"])
|
||||
|
||||
def test_reviewer_dirty_worktree_uses_reviewer_gate(self):
|
||||
result = assess_author_worktree_continuity({
|
||||
"task_role": "reviewer",
|
||||
"worktree_path": "/repo",
|
||||
"dirty_files": ["other.py"],
|
||||
"pr_scope_files": ["docs/a.md"],
|
||||
"scratch_used": False,
|
||||
})
|
||||
self.assertFalse(result["proven"])
|
||||
|
||||
|
||||
class TestAssessReviewerGitCommandLog(unittest.TestCase):
|
||||
def test_empty_log_is_clean(self):
|
||||
result = assess_reviewer_git_command_log([])
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
def test_mixed_log_blocks_on_forbidden(self):
|
||||
result = assess_reviewer_git_command_log([
|
||||
"git fetch prgs",
|
||||
"git stash",
|
||||
])
|
||||
self.assertFalse(result["proven"])
|
||||
self.assertEqual(len(result["forbidden_commands"]), 1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user