fix(profiles): canonical reconciler ops and guard raw branch delete (#687)
Address REQUEST_CHANGES on PR #688: - migrate_profiles: emit fully canonical reconciler/author/reviewer defaults; canonicalize explicit ops; fail if reconciler loses required pr.close/read - gitea_delete_branch: deny reconciler (and non-author roles); refuse preservation/protected branches before any API call - gitea_cleanup_merged_pr_branch: require reconciler role only; block preservation/evidence branches via branch_cleanup_guard - docs: end-to-end operator runbook for profile migrate/apply/reconnect/cleanup - tests: migration canonicalization, idempotency, raw-delete denial, guarded cleanup success and unmerged/preserve rejections Closes #687.
This commit is contained in:
@@ -8,10 +8,33 @@ import branch_cleanup_guard as guard # noqa: E402
|
||||
import mcp_server # noqa: E402
|
||||
import task_capability_map # noqa: E402
|
||||
from final_report_validator import assess_final_report_validator # noqa: E402
|
||||
from mcp_server import gitea_cleanup_merged_pr_branch # noqa: E402
|
||||
from mcp_server import gitea_cleanup_merged_pr_branch, gitea_delete_branch # noqa: E402
|
||||
|
||||
FAKE_AUTH = "token fake"
|
||||
|
||||
# Reconciler-shaped profile that holds branch.delete (recommended) plus
|
||||
# required pr.close/read so _role_kind classifies as reconciler.
|
||||
RECONCILER_WITH_DELETE = {
|
||||
"profile_name": "prgs-reconciler",
|
||||
"role": "reconciler",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.pr.close",
|
||||
"gitea.pr.comment",
|
||||
"gitea.issue.comment",
|
||||
"gitea.issue.close",
|
||||
"gitea.branch.delete",
|
||||
],
|
||||
"forbidden_operations": [
|
||||
"gitea.pr.approve",
|
||||
"gitea.pr.merge",
|
||||
"gitea.pr.review",
|
||||
"gitea.pr.create",
|
||||
"gitea.branch.push",
|
||||
"gitea.repo.commit",
|
||||
],
|
||||
}
|
||||
|
||||
|
||||
class TestRawBranchDeleteGuard(unittest.TestCase):
|
||||
def test_detects_local_and_remote_raw_git_delete_commands(self):
|
||||
@@ -134,11 +157,7 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
|
||||
def test_root_checkout_cleanup_fails_closed(self):
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value={
|
||||
"profile_name": "branch-cleanup",
|
||||
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
res = gitea_cleanup_merged_pr_branch(
|
||||
pr_number=487,
|
||||
@@ -155,11 +174,7 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
|
||||
branch = "feat/issue-485-lease-comments-non-list-guard"
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value={
|
||||
"profile_name": "branch-cleanup",
|
||||
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
self.mock_api.side_effect = [
|
||||
{
|
||||
@@ -190,11 +205,7 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
|
||||
branch = "feat/issue-485-lease-comments-non-list-guard"
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value={
|
||||
"profile_name": "branch-cleanup",
|
||||
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
self.mock_api.side_effect = [
|
||||
{
|
||||
@@ -220,6 +231,168 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
|
||||
]
|
||||
self.assertFalse(delete_calls)
|
||||
|
||||
def test_reconciler_with_branch_delete_cannot_raw_delete(self):
|
||||
"""#687: reconciler + gitea.branch.delete still cannot call raw delete."""
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
res = gitea_delete_branch(
|
||||
branch="fix/issue-683-workflow-guard-hardening",
|
||||
remote="prgs",
|
||||
)
|
||||
self.assertFalse(res.get("success", True))
|
||||
self.assertFalse(res.get("performed", True))
|
||||
reasons = " ".join(res.get("reasons") or [])
|
||||
self.assertIn("raw gitea_delete_branch", reasons)
|
||||
self.assertIn("cleanup_merged_pr_branch", reasons)
|
||||
self.mock_api.assert_not_called()
|
||||
|
||||
def test_reconciler_raw_delete_denies_preservation_branch(self):
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
res = gitea_delete_branch(
|
||||
branch="chore/issue-681-preserve-review-session-wip",
|
||||
remote="prgs",
|
||||
)
|
||||
self.assertFalse(res.get("performed", True))
|
||||
self.mock_api.assert_not_called()
|
||||
|
||||
def test_author_with_branch_delete_role_ok_but_preserve_blocked(self):
|
||||
"""Author role may use raw delete path when permitted; preserve fails closed."""
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-author",
|
||||
"role": "author",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.pr.create",
|
||||
"gitea.branch.push",
|
||||
"gitea.branch.delete",
|
||||
],
|
||||
"forbidden_operations": ["gitea.pr.approve", "gitea.pr.merge"],
|
||||
},
|
||||
).start()
|
||||
res = gitea_delete_branch(
|
||||
branch="chore/issue-681-preserve-review-session-wip",
|
||||
remote="prgs",
|
||||
)
|
||||
self.assertFalse(res.get("performed", True))
|
||||
self.assertIn("preservation", " ".join(res.get("reasons") or []))
|
||||
self.mock_api.assert_not_called()
|
||||
|
||||
def test_unmerged_branch_cleanup_rejected(self):
|
||||
branch = "feat/unmerged-work"
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
self.mock_api.side_effect = [
|
||||
{
|
||||
"number": 999,
|
||||
"merged": False,
|
||||
"merged_at": None,
|
||||
"head": {"ref": branch, "sha": "b" * 40},
|
||||
"base": {"ref": "master"},
|
||||
},
|
||||
{},
|
||||
]
|
||||
res = gitea_cleanup_merged_pr_branch(
|
||||
pr_number=999,
|
||||
confirmation=f"CLEANUP MERGED PR 999 BRANCH {branch}",
|
||||
branch=branch,
|
||||
remote="prgs",
|
||||
worktree_path="/tmp/repo/branches/cleanup",
|
||||
)
|
||||
self.assertFalse(res["performed"])
|
||||
self.assertTrue(
|
||||
any("not merged" in r for r in (res.get("reasons") or []))
|
||||
)
|
||||
delete_calls = [
|
||||
call for call in self.mock_api.call_args_list if call.args[0] == "DELETE"
|
||||
]
|
||||
self.assertFalse(delete_calls)
|
||||
|
||||
def test_preservation_branch_cleanup_rejected(self):
|
||||
branch = "chore/issue-681-preserve-review-session-wip"
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value=dict(RECONCILER_WITH_DELETE),
|
||||
).start()
|
||||
self.mock_api.side_effect = [
|
||||
{
|
||||
"number": 681,
|
||||
"merged": True,
|
||||
"merged_at": "2026-07-08T01:00:00Z",
|
||||
"head": {"ref": branch, "sha": "c" * 40},
|
||||
"base": {"ref": "master"},
|
||||
},
|
||||
{},
|
||||
]
|
||||
res = gitea_cleanup_merged_pr_branch(
|
||||
pr_number=681,
|
||||
confirmation=f"CLEANUP MERGED PR 681 BRANCH {branch}",
|
||||
branch=branch,
|
||||
remote="prgs",
|
||||
worktree_path="/tmp/repo/branches/cleanup",
|
||||
)
|
||||
self.assertFalse(res["performed"])
|
||||
self.assertTrue(
|
||||
any("preservation" in r for r in (res.get("reasons") or []))
|
||||
)
|
||||
delete_calls = [
|
||||
call for call in self.mock_api.call_args_list if call.args[0] == "DELETE"
|
||||
]
|
||||
self.assertFalse(delete_calls)
|
||||
|
||||
def test_non_reconciler_with_delete_denied_cleanup(self):
|
||||
patch(
|
||||
"mcp_server.get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-author",
|
||||
"role": "author",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.pr.create",
|
||||
"gitea.branch.push",
|
||||
"gitea.branch.delete",
|
||||
],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
).start()
|
||||
res = gitea_cleanup_merged_pr_branch(
|
||||
pr_number=487,
|
||||
confirmation="CLEANUP MERGED PR 487 BRANCH feat/branch",
|
||||
branch="feat/branch",
|
||||
remote="prgs",
|
||||
worktree_path="/tmp/repo/branches/cleanup",
|
||||
)
|
||||
self.assertFalse(res["performed"])
|
||||
self.assertEqual(res.get("required_role_kind"), "reconciler")
|
||||
self.mock_api.assert_not_called()
|
||||
|
||||
def test_assess_guard_rejects_preservation_branch(self):
|
||||
assessment = guard.assess_merged_pr_branch_cleanup(
|
||||
pr_number=681,
|
||||
head_branch="chore/issue-681-preserve-review-session-wip",
|
||||
merged=True,
|
||||
remote_branch_exists=True,
|
||||
open_pr_heads=set(),
|
||||
head_on_target=True,
|
||||
delete_capability_allowed=True,
|
||||
confirmation=(
|
||||
"CLEANUP MERGED PR 681 BRANCH "
|
||||
"chore/issue-681-preserve-review-session-wip"
|
||||
),
|
||||
)
|
||||
self.assertFalse(assessment["safe_to_delete"])
|
||||
self.assertTrue(
|
||||
any("preservation" in r for r in assessment["block_reasons"])
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user