From a863928661e3d83d450e5e9c57519feeb3e78f0f Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 02:12:26 -0400 Subject: [PATCH 1/2] Clarify and enforce reviewer-vs-merger role boundaries (#483) --- docs/llm-workflow-runbooks.md | 10 +- final_report_validator.py | 15 +++ gitea_mcp_server.py | 80 +++++++++++----- review_proofs.py | 18 +++- .../templates/merge-pr.md | 11 ++- .../templates/review-pr.md | 7 +- task_capability_map.py | 2 +- tests/test_final_report_validator.py | 92 +++++++++++++++++++ tests/test_mcp_server.py | 25 +++-- tests/test_operator_guide.py | 20 +++- tests/test_resolve_task_capability.py | 19 +++- tests/test_runtime_clarity.py | 50 +++++++++- 12 files changed, 294 insertions(+), 55 deletions(-) diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 634e22c..97bacdd 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -601,10 +601,10 @@ loop and do **not** substitute WebFetch/Playwright/manual base64. ### Merge a PR - **Profile:** merger (allowed to merge; must **not** be the PR author). -- **Steps:** confirm eligibility; require explicit confirmation - (`MERGE PR `); optionally pin head SHA / changed-file set; merge only when - Gitea reports the PR mergeable (branch-protection checks satisfied). No force, - no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). +- **Steps:** + - Merger workflow starts only after formal approval at current head. Reviewer workflow ends with review decision and separate merger handoff. + - Confirm eligibility; require explicit confirmation (`MERGE PR `); optionally pin head SHA / changed-file set; merge only when Gitea reports the PR mergeable (branch-protection checks satisfied). No force, no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). + - Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - **Prompt:** `Use any eligible merger profile to merge PR #N if checks pass and it is mergeable. Confirm with "MERGE PR N". Do not force-merge.` @@ -645,7 +645,7 @@ an author. |---|---|---|---|---| | Review PR (`review_pr`) | reviewer (e.g. `sysadmin` / `prgs-reviewer`) | read, gated review verdicts | commits, pushes, file edits, author comments, merge without eligibility | active profile is an author profile — stop immediately; do **not** switch to author-side fixes unless the operator explicitly re-tasks | | Address PR change requests (`address_pr_change_requests`) | author (e.g. `jcwalker3` / `prgs-author`) | commit/push fixes to the PR branch, PR comment summarizing fixes | review verdicts, approve, request-changes, merge | active profile lacks branch push | -| Merge PR (`merge_pr`) | reviewer/merger | gated merge after eligibility + approval | merging own PR, merging without pinned head match | active profile is an author profile, or any merge gate fails | +| Merge PR (`merge_pr`) | merger (e.g. `sysadmin` / `prgs-merger`) | gated merge after eligibility + approval | merging own PR, merging without pinned head match, reviewing PRs | active profile is an author or reviewer-only profile, or any merge gate fails | | Comment on issue discussion (`comment_issue`) | any profile with `gitea.issue.comment` | issue thread comments | review verdicts, closing via comment | permission missing (`gitea.pr.comment` does **not** imply it) | | Comment on PR (`comment_pr`) | any profile with `gitea.pr.comment` | PR thread comments | review verdicts | permission missing | | Author implementation (`create_branch`/`push_branch`/`create_pr`) | author | branch, commit, push, open PR | self-review, self-merge | profile lacks the author permissions | diff --git a/final_report_validator.py b/final_report_validator.py index 26c4ba0..718ae04 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -25,6 +25,7 @@ from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ "review_pr", + "merge_pr", "reconcile_already_landed", "author_issue", "work_issue", @@ -36,6 +37,8 @@ FINAL_REPORT_TASK_KINDS = frozenset({ _TASK_KIND_ALIASES = { "review": "review_pr", "review-merge-pr": "review_pr", + "merge": "merge_pr", + "merge_pr": "merge_pr", "reconcile-landed-pr": "reconcile_already_landed", "reconcile_already_landed": "reconcile_already_landed", "work-issue": "work_issue", @@ -44,6 +47,7 @@ _TASK_KIND_ALIASES = { _HANDOFF_ROLE_BY_TASK = { "review_pr": "review", + "merge_pr": "merger", "reconcile_already_landed": None, "author_issue": "author", "work_issue": "author", @@ -1036,6 +1040,17 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_review_mutation, _rule_reviewer_stale_head_proof, ], + "merge_pr": [ + _rule_shared_controller_handoff, + _rule_shared_email_disclosure, + *_SHARED_ISSUE_LOCK_RULES, + _rule_reviewer_legacy_workspace_mutations, + _rule_reviewer_vague_mutations_none, + _rule_reviewer_mutation_categories, + _rule_reviewer_git_fetch_readonly, + _rule_reviewer_linked_issue, + _rule_reviewer_stale_head_proof, + ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, _rule_shared_email_disclosure, diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 9ea9050..a1339e5 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -137,13 +137,13 @@ def verify_mutation_authority(remote: str | None, host: str | None = None, ) # Reviewer/author role pivot boundary: only an authorized pivot - # (recorded by gitea_activate_profile) may cross author → reviewer. - if (required_role == "reviewer" + # (recorded by gitea_activate_profile) may cross author -> reviewer/merger. + if (required_role in ("reviewer", "merger") and "author" in str(data.get("initial_profile")).lower() - and "reviewer" in str(active_profile).lower()): + and ("reviewer" in str(active_profile).lower() or "merger" in str(active_profile).lower())): if not data.get("role_pivot_authorized"): raise RuntimeError( - "Attempted reviewer mutation from author session without " + f"Attempted {required_role} mutation from author session without " "authorized role pivot (fail closed)" ) @@ -418,7 +418,7 @@ def record_preflight_check(type_name: str, resolved_role: str | None = None): def _enforce_branches_only_author_mutation(worktree_path: str | None = None) -> None: """#274: author mutations must run from a branches/ session worktree.""" - if _preflight_resolved_role == "reviewer": + if _preflight_resolved_role in ("reviewer", "reconciler", "merger"): return ctx = _resolve_author_mutation_context(worktree_path) workspace = ctx["workspace_path"] @@ -1872,13 +1872,23 @@ def gitea_check_pr_eligibility( # ("gitea.pr.merge") always match each other and never cross services. allowed = profile["allowed_operations"] forbidden = profile["forbidden_operations"] - op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + active_role = _role_kind(allowed, forbidden) + if action == "merge" and active_role == "reviewer": + op_ok = False + op_reason = "reviewer-cannot-merge" + else: + op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + if not op_ok: if op_reason == "no-allowed-operations": reasons.append( "profile has no configured allowed operations (fail closed)") elif op_reason == "forbidden": reasons.append(f"profile forbids '{action}'") + elif op_reason == "reviewer-cannot-merge": + reasons.append( + "Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head." + ) elif op_reason == "invalid-forbidden-entry": reasons.append( "profile has an unrecognized forbidden operation entry " @@ -1902,21 +1912,23 @@ def gitea_check_pr_eligibility( missing_perm) req_profile = None + role_noun = "reviewer" if action in ("approve", "request_changes", "review"): - req_profile = "A profile with reviewer role permissions (allowing approve/merge/review, and forbidding author operations)" + req_profile = "A profile with reviewer role permissions (allowing approve/review, and forbidding author operations)" elif action == "merge": - req_profile = "A profile with reviewer role permissions and explicit merge permission" + req_profile = "A profile with merger role permissions (allowing merge, and forbidding author operations)" + role_noun = "merger" result["required_profile"] = req_profile switching_supported = gitea_config.is_runtime_switching_enabled() if switching_supported: result["fixable_by_profile_switch"] = True result["requires_different_namespace"] = False - safe_step = f"Switch to a reviewer profile by calling gitea_activate_profile with a profile that allows {action}." + safe_step = f"Switch to a {role_noun} profile by calling gitea_activate_profile with a profile that allows {action}." else: result["fixable_by_profile_switch"] = False result["requires_different_namespace"] = True - safe_step = "Switch to the reviewer MCP session (e.g. gitea-reviewer) which has reviewer permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a reviewer profile." + safe_step = f"Switch to the {role_noun} MCP session (e.g. gitea-{role_noun}) which has {role_noun} permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a {role_noun} profile." result["safe_next_step"] = safe_step return result @@ -3419,6 +3431,12 @@ def gitea_merge_pr( available. Never secrets. """ verify_preflight_purity(remote) + allowed = get_profile()["allowed_operations"] + forbidden = get_profile()["forbidden_operations"] + active_role = _role_kind(allowed, forbidden) + if active_role == "reviewer": + raise RuntimeError("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.") + do = (do or "").strip().lower() result = { "performed": False, @@ -3434,6 +3452,10 @@ def gitea_merge_pr( "merge_result": None, "merge_commit": None, "reasons": [], + "active_profile": get_profile()["profile_name"], + "role_kind": active_role, + "merge_capability_source": "profile" if "gitea.pr.merge" in allowed else "config", + "explicit_operator_auth": confirmation, } reasons = result["reasons"] @@ -3599,7 +3621,7 @@ def gitea_merge_pr( # A profile/identity flip or side-channel override between preflight # and merge fails closed here. try: - verify_mutation_authority(remote, host, required_role="reviewer", + verify_mutation_authority(remote, host, required_role="merger", active_identity=auth_user) except RuntimeError as e: reasons.append(str(e)) @@ -5214,26 +5236,28 @@ def _role_kind(allowed, forbidden) -> str: """Classify the active profile from its normalized operations. 'reconciler' can close already-landed PRs without review/author powers; - 'author' can create PRs / push branches; 'reviewer' can approve/merge; - 'reconciler' can close already-landed PRs via ``gitea.pr.close`` without - review/author powers; 'mixed' can do both (a config smell — the - reviewer-deadlock invariant forbids it in v2 configs); 'limited' can do - neither. + 'author' can create PRs / push branches; 'reviewer' can approve; + 'merger' can merge PRs; + 'mixed' can do multiple (a config smell); 'limited' can do neither. """ if reconciler_profile.is_reconciler_profile(allowed, forbidden): return "reconciler" def can(op): return gitea_config.check_operation(op, allowed, forbidden)[0] - review = can("gitea.pr.approve") or can("gitea.pr.merge") + can_merge = can("gitea.pr.merge") + can_approve = can("gitea.pr.approve") author = can("gitea.pr.create") or can("gitea.branch.push") reconciler = ( can("gitea.pr.close") - and not review + and not can_approve + and not can_merge and not author ) - if review and author: + if (can_approve or can_merge) and author: return "mixed" - if review: + if can_merge: + return "merger" + if can_approve: return "reviewer" if reconciler: return "reconciler" @@ -5749,12 +5773,20 @@ def mcp_get_control_plane_guide( "gitea.issue.comment (separate from gitea.pr.comment).") elif role == "reviewer": guidance.append( - "Reviewer profile: review/approve/merge may proceed ONLY after " + "Reviewer profile: review/approve may proceed ONLY after " "gitea_check_pr_eligibility passes and the PR head SHA is " "pinned (expected_head_sha); the PR author must be a different " - "user, and merging additionally requires explicit operator " - "authorization plus the 'MERGE PR ' confirmation. " - "PR comments do not imply issue comments.") + "user. Reviewer profiles cannot merge PRs. " + "PR comments do not imply issue comments. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") + elif role == "merger": + guidance.append( + "Merger profile: merge may proceed ONLY after formal approval at " + "current head, gitea_check_pr_eligibility passes, and the PR head " + "SHA is pinned (expected_head_sha); the PR author must be a different " + "user, and merging requires explicit operator authorization plus the " + "'MERGE PR ' confirmation. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") elif role == "mixed": guidance.append( "WARNING: this profile allows both authoring and " diff --git a/review_proofs.py b/review_proofs.py index b74ebb8..4f725df 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -2243,6 +2243,18 @@ HANDOFF_ROLE_FIELDS = { ("Linked issue status", ("linked issue status", "linked issue")), ("Cleanup status", ("cleanup status", "cleanup")), ) + HANDOFF_REVIEW_MUTATION_FIELDS, + "merger": ( + ("Selected PR", ("selected pr",)), + ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), + ("Active profile", ("active profile",)), + ("Role kind", ("role kind",)), + ("Merge capability source", ("merge capability source",)), + ("Explicit operator authorization", ("explicit operator authorization", "operator authorization", "authorization")), + ("Expected head SHA", ("expected head sha", "expected head")), + ("Approval at current head", ("approval at current head", "approval")), + ("Merge result", ("merge result",)), + ("Cleanup status", ("cleanup status", "cleanup")), + ) + HANDOFF_REVIEW_MUTATION_FIELDS, "author": ( ("Selected issue", ("selected issue",)), ("Issue lock proof", ("issue lock proof", "lock before diff")), @@ -2417,8 +2429,8 @@ 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 + if role in ("review", "merger"): + # Issue #320: reviewer and merger handoffs use the precise mutation categories # in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous # "Workspace mutations" field, which is rejected below. required = [ @@ -2431,7 +2443,7 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): "downgraded": True, "missing_fields": [], "reasons": [ - "review handoff must not include legacy " + "review or merger handoff must not include legacy " "'Workspace mutations' field; report the precise " "mutation categories instead (issue #320)" ], diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index ae99544..e4e7a65 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -1,4 +1,4 @@ -# Template: merge a PR (eligible reviewer only) +# Template: merge a PR (eligible merger only) Copy, fill the `<...>` fields, and paste as the task prompt. @@ -10,8 +10,9 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): -- Only an eligible, NON-author reviewer merges. If authenticated user == PR +- Only an eligible, NON-author merger merges. If authenticated user == PR author → STOP. +- Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - Do not merge unless the PR is open, mergeable, and its checks/review pass. - No force-merge, no bypassing branch protections. - If the PR is closed but `merged=false`, STOP and run reconciliation. Do not clean up. @@ -51,10 +52,12 @@ Then run the cleanup template (worktree-cleanup.md): - fetch/prune; confirm main checkout is clean and current (0 0). Handoff: end with a section titled exactly `Controller Handoff` per SKILL.md -§K (long form — a merge is always high-risk), including the review/merge role -fields (Selected PR, Reviewer eligibility, Pinned reviewed head, Review +§K (long form — a merge is always high-risk), including the merger role +fields (Selected PR, Merger eligibility, Pinned reviewed head, Review decision, Merge result, Linked issue status, Cleanup status) plus: merge commit, PR metadata state/merged flag/hash, remote master hash, and the post-merge verification method used & verification results. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. ``` diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 391f962..c24d3d8 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -105,8 +105,9 @@ Steps: - base branch unchanged - no undismissed REQUEST_CHANGES / blocking review state left unaccounted If anything moved → STOP, re-pin, re-validate before any verdict. -10. Post the review verdict: approve only if scope is clean and checks pass; - otherwise request changes with specifics. Never merge from this review step. + 10. Post the review verdict: approve only if scope is clean and checks pass; + otherwise request changes with specifics. Never merge from this review step. + Review and merge are separate workflow roles. A reviewer approval is not merge authorization. Include a "Review Metadata" block (attribution only — docs/llm-agent-sha.md): Review Metadata: @@ -122,4 +123,6 @@ including the review/merge role fields: Selected PR, Reviewer eligibility, Pinned reviewed head, Review decision, Merge result, Linked issue status, Cleanup status. If you could not merge, name the exact gate. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. ``` diff --git a/task_capability_map.py b/task_capability_map.py index 1d40a78..d563736 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -66,7 +66,7 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { }, "merge_pr": { "permission": "gitea.pr.merge", - "role": "reviewer", + "role": "merger", }, "blind_pr_queue_review": { "permission": "gitea.pr.review", diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 88c5b23..88499b3 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -513,8 +513,100 @@ class TestEntryPoint(unittest.TestCase): def test_supported_task_kinds_include_review_and_reconcile(self): self.assertIn("review_pr", FINAL_REPORT_TASK_KINDS) + self.assertIn("merge_pr", FINAL_REPORT_TASK_KINDS) self.assertIn("reconcile_already_landed", FINAL_REPORT_TASK_KINDS) +class TestMergePrRules(unittest.TestCase): + def test_clean_merger_report_passes(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + "- Active profile: prgs-merger", + "- Role kind: merger", + "- Merge capability source: profile", + "- Explicit operator authorization: MERGE PR 203", + "- Expected head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Approval at current head: yes", + "- Merge result: PR merged successfully", + "- Cleanup status: complete", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertEqual(result["grade"], "A") + self.assertFalse(result["blocked"]) + + def test_missing_merger_fields_blocks(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertTrue(result["downgraded"]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 2b8efb4..64617cd 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -848,6 +848,19 @@ class TestMergePR(unittest.TestCase): # -- identity / profile / eligibility fail-closed ------------------------- + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_reviewer_profile_cannot_merge(self, _auth, mock_api): + env = {"GITEA_PROFILE_NAME": "prgs-reviewer", + "GITEA_ALLOWED_OPERATIONS": "read,approve"} + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs" + ) + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) + mock_api.assert_not_called() + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_self_author_cannot_merge(self, _auth, mock_api): @@ -889,14 +902,13 @@ class TestMergePR(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_profile_without_merge_permission_blocks(self, _auth, mock_api): - mock_api.side_effect = [{"login": "merger-bot"}, self._pr("author-bot")] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") - self.assertFalse(r["performed"]) - self.assertIn("profile is not allowed to merge", r["reasons"]) + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs") + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) self._assert_no_merge_call(mock_api) # -- PR state / mergeability ---------------------------------------------- @@ -1049,7 +1061,8 @@ class TestMergePR(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha=new_sha, remote="prgs") self.assertFalse(r["performed"]) self.assertTrue(r.get("approval_visible")) self.assertFalse(r.get("approval_at_current_head")) diff --git a/tests/test_operator_guide.py b/tests/test_operator_guide.py index a5159c6..147a6b2 100644 --- a/tests/test_operator_guide.py +++ b/tests/test_operator_guide.py @@ -33,8 +33,15 @@ REVIEWER_ENV = { "GITEA_PROFILE_NAME": "reviewer-test", "GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.pr.review,gitea.pr.comment,gitea.pr.approve," - "gitea.pr.request_changes,gitea.pr.merge", - "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push", + "gitea.pr.request_changes", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.merge", +} + +MERGER_ENV = { + "GITEA_PROFILE_NAME": "merger-test", + "GITEA_ALLOWED_OPERATIONS": + "gitea.read,gitea.pr.comment,gitea.pr.merge", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.approve", } EXPECTED_SKILLS = [ @@ -88,6 +95,15 @@ class TestControlPlaneGuide(GuideTestBase): self.assertIn("eligibility", blob) self.assertIn("pinned", blob) + @patch("mcp_server.api_request", return_value={"login": "merger-bot"}) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_merger_profile_guidance(self, _auth, _api): + with patch.dict(os.environ, MERGER_ENV, clear=True): + g = mcp_get_control_plane_guide(remote="prgs") + self.assertEqual(g["profile"]["role_kind"], "merger") + blob = " ".join(g["guidance"]).lower() + self.assertIn("merge", blob) + @patch("mcp_server.get_auth_header", return_value=None) def test_unresolved_identity_instructs_stop(self, _auth): with patch.dict(os.environ, AUTHOR_ENV, clear=True): diff --git a/tests/test_resolve_task_capability.py b/tests/test_resolve_task_capability.py index a34456d..aa25ced 100644 --- a/tests/test_resolve_task_capability.py +++ b/tests/test_resolve_task_capability.py @@ -44,9 +44,19 @@ CONFIG_RESOLVER = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.pr.merge", "gitea.issue.comment"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -83,6 +93,7 @@ class TestResolveTaskCapability(unittest.TestCase): "GITEA_MCP_PROFILE": profile, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } @patch("mcp_server.api_request", return_value={"login": "author-user"}) @@ -231,9 +242,9 @@ class TestResolveTaskCapability(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile")): res = mcp_server.gitea_resolve_task_capability(task="merge_pr", remote="prgs") self.assertFalse(res["allowed_in_current_session"]) - self.assertIn("reviewer", res["exact_safe_next_action"].lower()) + self.assertIn("merger", res["exact_safe_next_action"].lower()) self.assertEqual(res["required_operation_permission"], "gitea.pr.merge") - self.assertEqual(res["required_role_kind"], "reviewer") + self.assertEqual(res["required_role_kind"], "merger") @patch("mcp_server.api_request") def test_self_review_blocked_structured(self, mock_api): diff --git a/tests/test_runtime_clarity.py b/tests/test_runtime_clarity.py index e5098e0..1e94ada 100644 --- a/tests/test_runtime_clarity.py +++ b/tests/test_runtime_clarity.py @@ -43,9 +43,19 @@ CONFIG_SWITCHING_DISABLED = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.approve", "gitea.pr.merge"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -90,6 +100,7 @@ class TestRuntimeClarity(unittest.TestCase): "GITEA_MCP_REVEAL_ENDPOINTS": reveal, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } # ------------------------------------------------------------------------- @@ -125,7 +136,7 @@ class TestRuntimeClarity(unittest.TestCase): t for t in caps["task_capabilities"] if t["task"] == "merge_pr" ) self.assertFalse(merge_entry["allowed_in_current_session"]) - self.assertIn("reviewer-profile", merge_entry["matching_configured_profiles"]) + self.assertIn("merger-profile", merge_entry["matching_configured_profiles"]) @patch( "mcp_server.assess_preflight_status", @@ -144,6 +155,30 @@ class TestRuntimeClarity(unittest.TestCase): caps = ctx["session_capabilities"] self.assertFalse(caps["can_author_prs"]) self.assertFalse(caps["can_create_issues"]) + self.assertTrue(caps["can_review_prs"]) + self.assertFalse(caps["can_merge_prs"]) + merge_entry = next( + t for t in caps["task_capabilities"] if t["task"] == "merge_pr" + ) + self.assertFalse(merge_entry["allowed_in_current_session"]) + + @patch( + "mcp_server.assess_preflight_status", + return_value={"preflight_ready": True, "preflight_block_reasons": []}, + ) + @patch("mcp_server.api_request", return_value={"login": "merger-user"}) + @patch("mcp_server.get_auth_header", return_value="token merger-pass") + def test_get_runtime_context_merger(self, _auth, _api, _preflight): + with patch.dict(os.environ, self._env("merger-profile"), clear=True): + ctx = mcp_server.gitea_get_runtime_context(remote="dadeschools") + self.assertEqual(ctx["active_profile"], "merger-profile") + self.assertEqual(ctx["authenticated_username"], "merger-user") + self.assertTrue(ctx["review_merge_allowed"]) + self.assertEqual(ctx["suggested_fix"], "none") + self.assertEqual(ctx["safe_next_action"], "None; ready for operations.") + caps = ctx["session_capabilities"] + self.assertFalse(caps["can_author_prs"]) + self.assertFalse(caps["can_create_issues"]) self.assertFalse(caps["can_review_prs"]) self.assertTrue(caps["can_merge_prs"]) merge_entry = next( @@ -160,7 +195,7 @@ class TestRuntimeClarity(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile", reveal="0"), clear=True): res = mcp_server.gitea_list_profiles() profiles = res["profiles"] - self.assertEqual(len(profiles), 2) + self.assertEqual(len(profiles), 3) author_prof = next(p for p in profiles if p["name"] == "author-profile") self.assertTrue(author_prof["is_active"]) @@ -176,6 +211,13 @@ class TestRuntimeClarity(unittest.TestCase): self.assertEqual(reviewer_prof["base_url"], "") self.assertEqual(reviewer_prof["identity_status"], "credentials present") + merger_prof = next(p for p in profiles if p["name"] == "merger-profile") + self.assertFalse(merger_prof["is_active"]) + self.assertEqual(merger_prof["role_kind"], "merger") + self.assertEqual(merger_prof["auth"]["name"], "") + self.assertEqual(merger_prof["base_url"], "") + self.assertEqual(merger_prof["identity_status"], "credentials present") + @patch("mcp_server.api_request", return_value={"login": "author-user"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_list_profiles_revealed_under_opt_in(self, _auth, _api): From 76d1417c284172621e42a3bbdf54e319d8a5a6a1 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 02:19:37 -0400 Subject: [PATCH 2/2] fix: align example config with reviewer merger separation --- docs/gitea-dual-namespace-deployment.md | 13 +++++++++- docs/gitea-execution-profiles.md | 3 ++- docs/wiki/Identity-and-Profiles.md | 15 ++++++++--- gitea-mcp.v2-contexts.example.json | 18 ++++++++++--- tests/test_config_v2_contexts.py | 34 +++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 8 deletions(-) diff --git a/docs/gitea-dual-namespace-deployment.md b/docs/gitea-dual-namespace-deployment.md index 6ee7e8e..f25f09f 100644 --- a/docs/gitea-dual-namespace-deployment.md +++ b/docs/gitea-dual-namespace-deployment.md @@ -22,9 +22,12 @@ launched with exactly one static execution profile: | Namespace (MCP server name) | Profile (role) | Typical use | |-----------------------------|----------------|-------------| | `gitea-author` | an author profile | implement issues, push branches, open PRs, comment | -| `gitea-reviewer` | a reviewer profile | review, approve/request changes, merge | +| `gitea-reviewer` | a reviewer profile | review, approve/request changes | +| `gitea-merger` | a merger profile | merge PRs after approval and verification | | `gitea-reconciler` | a reconciler profile | close already-landed open PRs after ancestry proof (#304 profile; #310 close tool) | +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + Properties: - **One process, one credential.** Each namespace authenticates as exactly @@ -106,6 +109,14 @@ syntax to the client): "GITEA_MCP_CONFIG": "", "GITEA_MCP_PROFILE": "" } + }, + "gitea-merger": { + "command": "/venv/bin/python3", + "args": ["/mcp_server.py"], + "env": { + "GITEA_MCP_CONFIG": "", + "GITEA_MCP_PROFILE": "" + } } } } diff --git a/docs/gitea-execution-profiles.md b/docs/gitea-execution-profiles.md index 3d9958f..970f5d4 100644 --- a/docs/gitea-execution-profiles.md +++ b/docs/gitea-execution-profiles.md @@ -311,7 +311,8 @@ To make Gitea MCP profile activation and runtime identity state explicit, the fo ### 2. Dual MCP Namespaces Recommendation For security-sensitive or high-risk tasks, the preferred safety model uses separate, isolated MCP server instances (namespaces/sessions) launched with static profiles: - `gitea-author`: Exposes tools configured with author permissions; cannot perform approvals or merges. -- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews and merges. +- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews. Review and merge are separate workflow roles. A reviewer approval is not merge authorization. +- `gitea-merger`: Exposes tools configured with merger permissions; used for PR merges. This layout maintains physical separation of credentials and prevents privilege escalation within a single session. This is the model accepted in #139; deployment details, rationale, and client setup live in diff --git a/docs/wiki/Identity-and-Profiles.md b/docs/wiki/Identity-and-Profiles.md index 763a3bb..fb6000d 100644 --- a/docs/wiki/Identity-and-Profiles.md +++ b/docs/wiki/Identity-and-Profiles.md @@ -16,12 +16,21 @@ bind an authenticated Gitea identity to an allowed operation set. - **Allowed:** branch create/push, PR create, issue comment/create/close, repo commit, read. - **Forbidden:** PR approve, merge, request_changes. -### Reviewer / merger +### Reviewer - **Profile:** `prgs-reviewer` - **Typical identity:** `sysadmin` -- **Allowed:** PR review/approve/merge/request_changes, issue comment, read. -- **Forbidden:** branch push, PR create, repo commit. +- **Allowed:** PR review/approve/request_changes, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR merge. + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + +### Merger + +- **Profile:** `prgs-merger` +- **Typical identity:** `sysadmin` +- **Allowed:** PR merge, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR approve, PR review, PR request_changes. ## Configuration diff --git a/gitea-mcp.v2-contexts.example.json b/gitea-mcp.v2-contexts.example.json index 4bdad56..bd5b1ee 100644 --- a/gitea-mcp.v2-contexts.example.json +++ b/gitea-mcp.v2-contexts.example.json @@ -52,8 +52,19 @@ "execution_profile": "example-reviewer", "audit_label": "example-reviewer", "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, - "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes", "merge"], - "forbidden_operations": ["branch", "commit", "push", "open_pr"] + "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "merge"] + }, + "example-merger": { + "enabled": true, + "context": "example-context", + "role": "merger", + "username": "reviewer-user", + "execution_profile": "example-merger", + "audit_label": "example-merger", + "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, + "allowed_operations": ["read", "comment", "issue.comment", "merge"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "approve", "review", "request_changes"] } }, "projects": { @@ -63,7 +74,8 @@ "default_owner": "Example-Org", "default_repo": "Example-Repo", "default_author_profile": "example-author", - "default_reviewer_profile": "example-reviewer" + "default_reviewer_profile": "example-reviewer", + "default_merger_profile": "example-merger" } }, "rules": { diff --git a/tests/test_config_v2_contexts.py b/tests/test_config_v2_contexts.py index 71d3a7b..feb9a80 100644 --- a/tests/test_config_v2_contexts.py +++ b/tests/test_config_v2_contexts.py @@ -435,6 +435,40 @@ class ValidateConfigTests(unittest.TestCase): with open(example, encoding="utf-8") as fh: self.assertEqual(gitea_config.validate_config(json.load(fh)), []) + def test_repo_example_file_reviewer_vs_merger_boundaries(self): + example_path = __import__("pathlib").Path(__file__).resolve().parent.parent \ + / "gitea-mcp.v2-contexts.example.json" + with open(example_path, encoding="utf-8") as fh: + cfg = json.load(fh) + + # 1. Config includes separate reviewer and merger examples + self.assertIn("example-reviewer", cfg["profiles"]) + self.assertIn("example-merger", cfg["profiles"]) + + reviewer = cfg["profiles"]["example-reviewer"] + merger = cfg["profiles"]["example-merger"] + + # 2. Example reviewer config does not include merge capability + reviewer_allowed = reviewer.get("allowed_operations") or [] + reviewer_forbidden = reviewer.get("forbidden_operations") or [] + + self.assertNotIn("merge", reviewer_allowed) + self.assertNotIn("gitea.pr.merge", reviewer_allowed) + self.assertIn("merge", reviewer_forbidden) + + # 3. Example reviewer can resolve review task, cannot resolve merge task + # 4. Example merger can resolve merge task + rev_review_ok, _ = gitea_config.check_operation("gitea.pr.review", reviewer_allowed, reviewer_forbidden) + rev_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", reviewer_allowed, reviewer_forbidden) + + merg_allowed = merger.get("allowed_operations") or [] + merg_forbidden = merger.get("forbidden_operations") or [] + merg_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", merg_allowed, merg_forbidden) + + self.assertTrue(rev_review_ok, "example-reviewer must allow review") + self.assertFalse(rev_merge_ok, "example-reviewer must not allow merge") + self.assertTrue(merg_merge_ok, "example-merger must allow merge") + def test_broken_contexts_config_reports_problems(self): data = contexts_config() data["profiles"]["prgs-author"]["context"] = "nope"