fix: preserve actual reconciler role for comment_issue preflight (Closes #540)
Merge PR #541 — preserve actual reconciler role for #274/#475 exemptions during comment_issue preflight (Closes #540).
This commit was merged in pull request #541.
This commit is contained in:
@@ -0,0 +1,271 @@
|
||||
"""Regression tests for #540: comment_issue preflight must not poison the
|
||||
actual reconciler role for #274 branch-only / #475 root-checkout exemptions.
|
||||
|
||||
`resolve_task_capability("comment_issue")` stamps
|
||||
``_preflight_resolved_role = "author"`` because ``comment_issue`` has
|
||||
``required_role_kind = author``. Before the fix, ``_effective_workspace_role``
|
||||
preferred that stamp and a genuine ``prgs-reconciler`` session was treated as an
|
||||
author inside the #274 branch-only mutation guard and the #475 root checkout
|
||||
guard, blocking ``gitea_create_issue_comment`` from the control checkout.
|
||||
|
||||
The fix keys the role exemptions off the *actual profile role* as well, so the
|
||||
exemption survives the poisoned task role while author profiles stay blocked.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||
|
||||
import gitea_mcp_server as srv # noqa: E402
|
||||
import root_checkout_guard as rcg # noqa: E402
|
||||
|
||||
FAKE_AUTH = "token test"
|
||||
CONTROL_CHECKOUT_ROOT = str(Path(__file__).resolve().parents[3])
|
||||
MASTER_SHA = "a" * 40
|
||||
OTHER_SHA = "b" * 40
|
||||
|
||||
RECONCILER_PROFILE = {
|
||||
"profile_name": "prgs-reconciler",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.pr.close",
|
||||
"gitea.pr.comment",
|
||||
"gitea.issue.comment",
|
||||
],
|
||||
"forbidden_operations": [
|
||||
"gitea.pr.approve",
|
||||
"gitea.pr.merge",
|
||||
"gitea.pr.create",
|
||||
"gitea.branch.push",
|
||||
"gitea.repo.commit",
|
||||
],
|
||||
"audit_label": "prgs-reconciler",
|
||||
}
|
||||
|
||||
AUTHOR_PROFILE = {
|
||||
"profile_name": "prgs-author",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.issue.comment",
|
||||
"gitea.issue.create",
|
||||
"gitea.pr.create",
|
||||
"gitea.branch.push",
|
||||
"gitea.repo.commit",
|
||||
],
|
||||
"forbidden_operations": [
|
||||
"gitea.pr.approve",
|
||||
"gitea.pr.merge",
|
||||
],
|
||||
"audit_label": "prgs-author",
|
||||
}
|
||||
|
||||
REVIEWER_PROFILE = {
|
||||
"profile_name": "prgs-reviewer",
|
||||
"allowed_operations": ["gitea.read", "gitea.pr.approve", "gitea.pr.merge"],
|
||||
"forbidden_operations": ["gitea.pr.create", "gitea.branch.push"],
|
||||
"audit_label": "prgs-reviewer",
|
||||
}
|
||||
|
||||
|
||||
class TestActualProfileRole(unittest.TestCase):
|
||||
"""`_actual_profile_role` ignores the poisoned preflight task role (#540)."""
|
||||
|
||||
def tearDown(self):
|
||||
srv._preflight_resolved_role = None
|
||||
|
||||
@patch("gitea_mcp_server.get_profile", return_value=RECONCILER_PROFILE)
|
||||
def test_reconciler_profile_role_survives_author_task_stamp(self, _profile):
|
||||
srv._preflight_resolved_role = "author" # comment_issue poison
|
||||
self.assertEqual(srv._actual_profile_role(), "reconciler")
|
||||
# _effective_workspace_role is still poisoned to author (unchanged #510)...
|
||||
self.assertEqual(srv._effective_workspace_role(), "author")
|
||||
|
||||
@patch("gitea_mcp_server.get_profile", return_value=AUTHOR_PROFILE)
|
||||
def test_author_profile_role_is_author(self, _profile):
|
||||
srv._preflight_resolved_role = "author"
|
||||
self.assertEqual(srv._actual_profile_role(), "author")
|
||||
|
||||
@patch("gitea_mcp_server.get_profile", return_value=REVIEWER_PROFILE)
|
||||
def test_reviewer_profile_role_is_reviewer(self, _profile):
|
||||
srv._preflight_resolved_role = "author"
|
||||
self.assertEqual(srv._actual_profile_role(), "reviewer")
|
||||
|
||||
|
||||
class TestBranchesOnlyExemptionRealRole(unittest.TestCase):
|
||||
"""#274 branches-only exemption keys off the actual profile role (#540)."""
|
||||
|
||||
def tearDown(self):
|
||||
srv._preflight_resolved_role = None
|
||||
|
||||
@patch("gitea_mcp_server.get_profile", return_value=RECONCILER_PROFILE)
|
||||
def test_reconciler_exempt_despite_author_task_stamp(self, _profile):
|
||||
srv._preflight_resolved_role = "author" # poison
|
||||
# Must return without raising and without resolving an author worktree.
|
||||
with patch("gitea_mcp_server._resolve_namespace_mutation_context") as ctx:
|
||||
srv._enforce_branches_only_author_mutation()
|
||||
ctx.assert_not_called()
|
||||
|
||||
@patch("gitea_mcp_server.get_profile", return_value=AUTHOR_PROFILE)
|
||||
def test_author_not_exempt(self, _profile):
|
||||
srv._preflight_resolved_role = "author"
|
||||
sentinel = RuntimeError("author-mutation-guard-reached")
|
||||
|
||||
def _blow_up(*_a, **_k):
|
||||
raise sentinel
|
||||
|
||||
# Author is not exempt: the guard proceeds to resolve/assess the
|
||||
# workspace (proven by reaching the patched context resolver).
|
||||
with patch(
|
||||
"gitea_mcp_server._resolve_namespace_mutation_context",
|
||||
side_effect=_blow_up,
|
||||
):
|
||||
with self.assertRaises(RuntimeError) as raised:
|
||||
srv._enforce_branches_only_author_mutation()
|
||||
self.assertIs(raised.exception, sentinel)
|
||||
|
||||
|
||||
class TestRootCheckoutGuardRealRole(unittest.TestCase):
|
||||
"""#475 root guard honours the actual profile role too (#540)."""
|
||||
|
||||
def _assess(self, **kwargs):
|
||||
defaults = {
|
||||
"workspace_path": CONTROL_CHECKOUT_ROOT,
|
||||
"canonical_repo_root": CONTROL_CHECKOUT_ROOT,
|
||||
"current_branch": "feat/some-branch",
|
||||
"head_sha": OTHER_SHA,
|
||||
"porcelain_status": " M gitea_mcp_server.py\n",
|
||||
"remote_master_sha": MASTER_SHA,
|
||||
"resolved_role": "author", # poisoned task role
|
||||
}
|
||||
defaults.update(kwargs)
|
||||
return rcg.assess_root_checkout_guard(**defaults)
|
||||
|
||||
def test_actual_reconciler_exempt_despite_poisoned_resolved_author(self):
|
||||
result = self._assess(actual_role="reconciler")
|
||||
self.assertTrue(result["proven"])
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
def test_actual_author_still_blocked_on_contaminated_root(self):
|
||||
result = self._assess(actual_role="author")
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_merger_strictness_stays_keyed_on_resolved_task_role(self):
|
||||
# When the merge task resolves the merger role, the branches/ auto
|
||||
# exemption is denied and a clean control checkout is required.
|
||||
blocked = self._assess(
|
||||
workspace_path=f"{CONTROL_CHECKOUT_ROOT}/branches/review-pr-1",
|
||||
current_branch="review-pr-1",
|
||||
porcelain_status="",
|
||||
head_sha=OTHER_SHA,
|
||||
resolved_role="merger",
|
||||
)
|
||||
self.assertTrue(blocked["block"])
|
||||
|
||||
def test_actual_merger_does_not_over_tighten_non_merge_task(self):
|
||||
# A merger profile whose current task did NOT resolve to merger keeps
|
||||
# the branches/ workspace exemption (regression guard for #540): the
|
||||
# actual_role signal must not force merger strictness here.
|
||||
result = self._assess(
|
||||
workspace_path=f"{CONTROL_CHECKOUT_ROOT}/branches/review-pr-1",
|
||||
current_branch="review-pr-1",
|
||||
porcelain_status="",
|
||||
head_sha=OTHER_SHA,
|
||||
resolved_role="reviewer",
|
||||
actual_role="merger",
|
||||
)
|
||||
self.assertTrue(result["proven"])
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
def test_backward_compatible_without_actual_role(self):
|
||||
# No actual_role supplied: behaviour falls back to resolved_role.
|
||||
result = self._assess(resolved_role="reconciler")
|
||||
self.assertTrue(result["proven"])
|
||||
|
||||
|
||||
class TestReconcilerCommentThroughCanonicalPath(unittest.TestCase):
|
||||
"""Integration: reconciler comment survives the poisoned author task role."""
|
||||
|
||||
def setUp(self):
|
||||
srv._preflight_whoami_called = True
|
||||
srv._preflight_capability_called = True
|
||||
srv._preflight_whoami_violation = False
|
||||
srv._preflight_capability_violation = False
|
||||
srv._preflight_capability_baseline_porcelain = ""
|
||||
self._orig_in_test = srv._preflight_in_test_mode
|
||||
srv._preflight_in_test_mode = lambda: False
|
||||
|
||||
def tearDown(self):
|
||||
srv._preflight_in_test_mode = self._orig_in_test
|
||||
srv._preflight_resolved_role = None
|
||||
|
||||
@patch("gitea_mcp_server._get_workspace_porcelain", return_value="")
|
||||
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
|
||||
@patch("gitea_mcp_server.api_request")
|
||||
@patch(
|
||||
"gitea_mcp_server.root_checkout_guard.resolve_remote_master_sha",
|
||||
return_value=MASTER_SHA,
|
||||
)
|
||||
@patch(
|
||||
"gitea_mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value={
|
||||
"current_branch": "master",
|
||||
"head_sha": MASTER_SHA,
|
||||
"porcelain_status": "",
|
||||
},
|
||||
)
|
||||
@patch("gitea_mcp_server.get_profile", return_value=RECONCILER_PROFILE)
|
||||
def test_reconciler_comment_from_control_checkout_succeeds(
|
||||
self, _profile, _git, _remote_sha, mock_api, _auth, _porcelain
|
||||
):
|
||||
srv._preflight_resolved_role = "author" # comment_issue poison
|
||||
mock_api.return_value = {"id": 9001, "html_url": "https://x/y"}
|
||||
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
|
||||
with patch.dict(os.environ, {}, clear=False):
|
||||
os.environ.pop("GITEA_AUTHOR_WORKTREE", None)
|
||||
os.environ.pop("GITEA_ACTIVE_WORKTREE", None)
|
||||
os.environ.pop("GITEA_RECONCILER_WORKTREE", None)
|
||||
result = srv.gitea_create_issue_comment(
|
||||
515, "canonical reconciler audit", remote="prgs"
|
||||
)
|
||||
self.assertTrue(result["success"])
|
||||
self.assertEqual(result["comment_id"], 9001)
|
||||
mock_api.assert_called_once()
|
||||
|
||||
@patch("gitea_mcp_server._get_workspace_porcelain", return_value="")
|
||||
@patch("gitea_mcp_server._auth", return_value=FAKE_AUTH)
|
||||
@patch("gitea_mcp_server.api_request")
|
||||
@patch(
|
||||
"gitea_mcp_server.root_checkout_guard.resolve_remote_master_sha",
|
||||
return_value=MASTER_SHA,
|
||||
)
|
||||
@patch(
|
||||
"gitea_mcp_server.issue_lock_worktree.read_worktree_git_state",
|
||||
return_value={
|
||||
"current_branch": "master",
|
||||
"head_sha": MASTER_SHA,
|
||||
"porcelain_status": "",
|
||||
},
|
||||
)
|
||||
@patch("gitea_mcp_server.get_profile", return_value=AUTHOR_PROFILE)
|
||||
def test_author_comment_from_control_checkout_blocked(
|
||||
self, _profile, _git, _remote_sha, mock_api, _auth, _porcelain
|
||||
):
|
||||
srv._preflight_resolved_role = "author"
|
||||
with patch.object(srv, "PROJECT_ROOT", CONTROL_CHECKOUT_ROOT):
|
||||
with patch.dict(os.environ, {}, clear=False):
|
||||
os.environ.pop("GITEA_AUTHOR_WORKTREE", None)
|
||||
os.environ.pop("GITEA_ACTIVE_WORKTREE", None)
|
||||
with self.assertRaises(RuntimeError):
|
||||
srv.gitea_create_issue_comment(
|
||||
515, "author note", remote="prgs"
|
||||
)
|
||||
mock_api.assert_not_called()
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user