diff --git a/.env.example b/.env.example index 5463787..4777fac 100644 --- a/.env.example +++ b/.env.example @@ -46,3 +46,12 @@ GITEA_TOKEN_SOURCE=GITEA_TOKEN # profile's values. Leave unset for pure env-based configuration. GITEA_MCP_CONFIG=/Users/jasonwalker/.config/gitea-tools/profiles.json GITEA_MCP_PROFILE=prgs + +# Namespace-scoped active task workspaces (#510). Each MCP namespace uses only +# its own role env var; foreign bindings (e.g. GITEA_AUTHOR_WORKTREE in a +# merger process) are ignored. +# GITEA_AUTHOR_WORKTREE=/path/to/repo/branches/issue-123-work +# GITEA_REVIEWER_WORKTREE=/path/to/repo/branches/review-pr456 +# GITEA_MERGER_WORKTREE=/path/to/repo/branches/merge-pr456 +# GITEA_RECONCILER_WORKTREE=/path/to/repo/branches/reconcile-pr456 +# GITEA_ACTIVE_WORKTREE=/path/to/repo/branches/session-override diff --git a/author_mutation_worktree.py b/author_mutation_worktree.py index 29dc05d..dccb94a 100644 --- a/author_mutation_worktree.py +++ b/author_mutation_worktree.py @@ -12,6 +12,8 @@ import subprocess BASE_BRANCHES = frozenset({"master", "main", "dev"}) ACTIVE_WORKTREE_ENV = "GITEA_ACTIVE_WORKTREE" AUTHOR_WORKTREE_ENV = "GITEA_AUTHOR_WORKTREE" +# Author-only: reviewer/merger/reconciler namespaces use role-specific env vars +# via namespace_workspace_binding (#510). def _normalize_path(path: str) -> str: diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d67769d..cb904c4 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -823,6 +823,45 @@ scripts/release-tag v0.4.0 --notes-file /tmp/release-notes.md scripts/release-tag v0.4.0 --notes-file /tmp/release-notes.md --push ``` +## Namespace workspace binding (#510) + +Each MCP namespace resolves its **own** active task workspace. Foreign role +worktree environment variables must not poison another namespace's purity +checks. + +| Namespace | Workspace env vars (in priority under `GITEA_ACTIVE_WORKTREE`) | Allowed roots | +|-----------|------------------------------------------------------------------|---------------| +| author | `GITEA_AUTHOR_WORKTREE` | `branches/` worktree only (#274) | +| reviewer | `GITEA_REVIEWER_WORKTREE` | clean `branches/` worktree | +| merger | `GITEA_MERGER_WORKTREE` | clean `branches/` worktree **or** clean control checkout | +| reconciler | `GITEA_RECONCILER_WORKTREE` | clean `branches/` worktree **or** clean control checkout | + +`GITEA_AUTHOR_WORKTREE` is **author-only**. Reviewer, merger, and reconciler +MCP processes ignore it even when it points at a dirty author WIP tree. + +### Safe reconnect / rebind procedure + +When a mutation blocks on workspace binding: + +1. Read the error — it names the **resolved workspace path**, **role + namespace**, and **binding source** (tool arg, env var, or process root). +2. Reconnect or relaunch the correct namespace MCP server from the intended + workspace (or set the role-specific env var before launch). +3. Pass `worktree_path` on reviewer/merger mutation tools when the active + branches/ worktree differs from the MCP process root. +4. **Do not** clean, reset, or discard foreign role worktrees to unblock your + own namespace — that destroys another agent's WIP. + +### CTH guidance for workspace binding blockers + +When posting a Canonical Thread Handoff after a binding blocker: + +- State which namespace was active (author / reviewer / merger / reconciler). +- Quote the resolved workspace path and binding source from the error. +- Name the safe reconnect action (relaunch MCP from `branches/...`, set + `GITEA_*_WORKTREE`, or pass `worktree_path`). +- Explicitly note that foreign worktrees must not be cleaned to unblock. + ## Safety notes - Never place raw tokens or passwords in any LLM MCP config; reference secrets diff --git a/final_report_validator.py b/final_report_validator.py index 352a3ff..85d7b4e 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -12,6 +12,7 @@ import re from typing import Any, Callable import issue_lock_provenance +from post_merge_cleanup_proof import assess_post_merge_cleanup_proof from review_proofs import ( HANDOFF_HEADING, assess_controller_handoff, @@ -1042,6 +1043,20 @@ def _rule_reviewer_review_mutation( ) +def _rule_reviewer_post_merge_cleanup_proof(report_text: str) -> list[dict[str, str]]: + result = assess_post_merge_cleanup_proof(report_text) + if not result.get("block"): + return [] + return _findings_from_reasons( + "reviewer.post_merge_cleanup_proof", + result.get("reasons") or [], + field="Cleanup status", + severity="block", + safe_next_action=result.get("safe_next_action") + or "report CLEANUP_SKIPPED with blocker or full cleanup checklist", + ) + + _SHARED_ISSUE_LOCK_RULES = ( _rule_shared_issue_lock_external_state, _rule_shared_manual_lock_pr_override, @@ -1071,6 +1086,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_target_branch_freshness, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, + _rule_reviewer_post_merge_cleanup_proof, _rule_reviewer_stale_head_proof, ], "reconcile_already_landed": [ diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index c650778..d9608b0 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -175,12 +175,76 @@ _preflight_reviewer_violation_files: list[str] = [] ACTIVE_WORKTREE_ENV = "GITEA_ACTIVE_WORKTREE" AUTHOR_WORKTREE_ENV = "GITEA_AUTHOR_WORKTREE" +REVIEWER_WORKTREE_ENV = "GITEA_REVIEWER_WORKTREE" +MERGER_WORKTREE_ENV = "GITEA_MERGER_WORKTREE" +RECONCILER_WORKTREE_ENV = "GITEA_RECONCILER_WORKTREE" + +import namespace_workspace_binding as nwb # noqa: E402 def _preflight_in_test_mode() -> bool: return "pytest" in sys.modules or "unittest" in sys.modules +def _reviewer_session_worktree() -> str | None: + import reviewer_pr_lease as _reviewer_pr_lease + + session = _reviewer_pr_lease.get_session_lease() + if not session: + return None + worktree = (session.get("worktree") or "").strip() + return worktree or None + + +def _effective_workspace_role() -> str: + """Resolve the namespace key used for workspace binding (#510).""" + profile = get_profile() + role = _preflight_resolved_role + if not role: + role = _role_kind( + profile.get("allowed_operations") or [], + profile.get("forbidden_operations") or [], + ) + return nwb.normalize_role_kind( + role, + profile_name=profile.get("profile_name"), + ) + + +def _resolve_preflight_workspace_path(worktree_path: str | None = None) -> str: + """Resolve the namespace-scoped workspace root inspected by pre-flight guards.""" + role = _effective_workspace_role() + workspace, _source = nwb.resolve_namespace_workspace( + role_kind=role, + worktree_path=worktree_path, + process_project_root=PROJECT_ROOT, + session_lease_worktree=( + _reviewer_session_worktree() if role in {"reviewer", "merger"} else None + ), + profile_name=get_profile().get("profile_name"), + ) + return workspace + + +def _resolve_namespace_mutation_context(worktree_path: str | None = None) -> dict: + """Canonical namespace workspace + repository root for guards (#460/#510).""" + role = _effective_workspace_role() + return nwb.resolve_namespace_mutation_context( + role_kind=role, + worktree_path=worktree_path, + process_project_root=PROJECT_ROOT, + session_lease_worktree=( + _reviewer_session_worktree() if role in {"reviewer", "merger"} else None + ), + profile_name=get_profile().get("profile_name"), + ) + + +def _resolve_author_mutation_context(worktree_path: str | None = None) -> dict: + """Backward-compatible alias for namespace workspace context.""" + return _resolve_namespace_mutation_context(worktree_path) + + def _ensure_process_start_porcelain() -> str: """Capture the shared-worktree baseline once per MCP process (#252).""" global _process_start_porcelain @@ -189,91 +253,6 @@ def _ensure_process_start_porcelain() -> str: return _process_start_porcelain -def _resolve_preflight_workspace_path(worktree_path: str | None = None) -> str: - """Resolve the workspace root inspected by pre-flight guards.""" - return author_mutation_worktree.resolve_mutation_workspace( - worktree_path, - PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), - ) - - -def _resolve_author_mutation_context(worktree_path: str | None = None) -> dict: - """Canonical workspace + repository root for runtime_context and guards (#460).""" - return author_mutation_worktree.resolve_author_mutation_context( - worktree_path, - PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - author_worktree_env=os.environ.get(AUTHOR_WORKTREE_ENV), - ) - - -def _reviewer_session_worktree() -> str | None: - session = reviewer_pr_lease.get_session_lease() - if not session: - return None - worktree = (session.get("worktree") or "").strip() - return worktree or None - - -def _resolve_reviewer_mutation_worktree_path( - worktree_path: str | None = None, - worktree: str | None = None, -) -> str: - """Resolve the workspace inspected before reviewer mutations (#503).""" - return reviewer_mutation_workspace.resolve_reviewer_mutation_workspace( - worktree_path=worktree_path, - worktree=worktree, - process_project_root=PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - session_lease_worktree=_reviewer_session_worktree(), - ) - - -def _assess_reviewer_mutation_workspace( - worktree_path: str | None = None, - worktree: str | None = None, -) -> dict: - git_state = issue_lock_worktree.read_worktree_git_state( - _resolve_reviewer_mutation_worktree_path(worktree_path, worktree) - ) - return reviewer_mutation_workspace.assess_reviewer_mutation_workspace( - worktree_path=worktree_path, - worktree=worktree, - process_project_root=PROJECT_ROOT, - active_worktree_env=os.environ.get(ACTIVE_WORKTREE_ENV), - session_lease_worktree=_reviewer_session_worktree(), - current_branch=git_state.get("current_branch"), - ) - - -def _verify_reviewer_mutation_workspace( - remote: str | None = None, - worktree_path: str | None = None, - worktree: str | None = None, -) -> str: - """Bind reviewer mutations to the active branches/ worktree (#503).""" - assessment = _assess_reviewer_mutation_workspace(worktree_path, worktree) - if assessment["block"]: - raise RuntimeError( - reviewer_mutation_workspace.format_stale_mcp_workspace_binding_error( - process_project_root=assessment["process_project_root"], - mutation_workspace=assessment["mutation_workspace"], - declared_worktree_path=assessment.get("declared_worktree_path"), - metadata_only=assessment.get("metadata_only", False), - ) - + ( - f" Details: {'; '.join(assessment['reasons'])}" - if assessment.get("reasons") - else "" - ) - ) - resolved = assessment["mutation_workspace"] - verify_preflight_purity(remote, worktree_path=resolved, task="review_pr") - return resolved - - def _get_git_root(path: str) -> str | None: try: res = subprocess.run( @@ -341,7 +320,7 @@ def _format_preflight_files(files: list[str]) -> str: def _preflight_workspace_details(worktree_path: str | None, dirty_files: list[str]) -> dict: - ctx = _resolve_author_mutation_context(worktree_path) + ctx = _resolve_namespace_mutation_context(worktree_path) workspace = ctx["workspace_path"] inspected_root = _get_git_root(workspace) process_root = ctx["process_project_root"] @@ -359,6 +338,9 @@ def _preflight_workspace_details(worktree_path: str | None, dirty_files: list[st "dirty_files": list(dirty_files), "dirty_scope": dirty_scope, "workspace_roots_aligned": ctx["roots_aligned"], + "workspace_role_kind": ctx.get("workspace_role_kind"), + "workspace_binding_source": ctx.get("workspace_binding_source"), + "ignored_bindings": list(ctx.get("ignored_bindings") or []), } if not ctx["roots_aligned"]: details["workspace_root_mismatch"] = ( @@ -369,13 +351,19 @@ def _preflight_workspace_details(worktree_path: str | None, dirty_files: list[st def _format_preflight_workspace_details(details: dict) -> str: - return ( - f"MCP server process root: {details.get('mcp_server_process_root')}; " - f"active task workspace root: {details.get('active_task_workspace_root')}; " - f"inspected git root: {details.get('inspected_git_root')}; " - f"dirty files: {_format_preflight_files(details.get('dirty_files') or [])}; " - f"dirty scope: {details.get('dirty_scope')}" - ) + parts = [ + f"MCP server process root: {details.get('mcp_server_process_root')}", + f"active task workspace root: {details.get('active_task_workspace_root')}", + f"workspace role: {details.get('workspace_role_kind')}", + f"binding source: {details.get('workspace_binding_source')}", + f"inspected git root: {details.get('inspected_git_root')}", + f"dirty files: {_format_preflight_files(details.get('dirty_files') or [])}", + f"dirty scope: {details.get('dirty_scope')}", + ] + ignored = details.get("ignored_bindings") or [] + if ignored: + parts.append(f"ignored foreign bindings: {'; '.join(ignored)}") + return "; ".join(parts) def assess_preflight_status(worktree_path: str | None = None) -> dict: @@ -398,20 +386,23 @@ def assess_preflight_status(worktree_path: str | None = None) -> dict: "Active task workspace has tracked file edits before mutation " f"({_format_preflight_workspace_details(workspace_details)})" ) - if _preflight_resolved_role == "reviewer": - binding = reviewer_mutation_workspace.assess_metadata_only_worktree_binding( + role = _effective_workspace_role() + if role in nwb.NON_AUTHOR_ROLES: + binding = nwb.assess_metadata_only_worktree_binding( + role_kind=role, declared_worktree_path=worktree_path, - mutation_workspace=_resolve_reviewer_mutation_worktree_path(), + mutation_workspace=_resolve_preflight_workspace_path(worktree_path), process_project_root=PROJECT_ROOT, + profile_name=get_profile().get("profile_name"), ) if binding.get("block"): reasons.append(binding["reasons"][0]) reasons.append( - reviewer_mutation_workspace.format_stale_mcp_workspace_binding_error( - process_project_root=PROJECT_ROOT, - mutation_workspace=binding["mutation_workspace"], - declared_worktree_path=binding.get("declared_worktree_path"), - metadata_only=True, + nwb.format_namespace_workspace_binding_error( + role_kind=role, + workspace_path=binding["mutation_workspace"], + binding_source="MCP server process root (default)", + reasons=binding.get("reasons"), ) ) return { @@ -547,13 +538,14 @@ def record_preflight_check( def _enforce_branches_only_author_mutation(worktree_path: str | None = None) -> None: """#274: author file/branch mutations must run from a branches/ worktree. - Reviewer and reconciler roles are exempt: reconciler ``close_pr`` is a - Gitea metadata mutation and must not require ``GITEA_AUTHOR_WORKTREE`` - (#468). + Reviewer, merger, and reconciler roles are exempt: reconciler ``close_pr`` + is a Gitea metadata mutation and must not require ``GITEA_AUTHOR_WORKTREE`` + (#468). Non-author namespaces use dedicated workspace env vars (#510). """ - if _preflight_resolved_role in ("reviewer", "reconciler"): + role = _effective_workspace_role() + if role in nwb.NON_AUTHOR_ROLES: return - ctx = _resolve_author_mutation_context(worktree_path) + ctx = _resolve_namespace_mutation_context(worktree_path) workspace = ctx["workspace_path"] git_state = issue_lock_worktree.read_worktree_git_state(workspace) assessment = author_mutation_worktree.assess_author_mutation_worktree( @@ -601,11 +593,12 @@ def verify_preflight_purity( f"'{task}' (fail closed)" ) - ctx = _resolve_author_mutation_context(worktree_path) + ctx = _resolve_namespace_mutation_context(worktree_path) workspace = ctx["workspace_path"] canonical_root = ctx["canonical_repo_root"] process_root = ctx["process_project_root"] real_workspace = os.path.realpath(workspace) + role = ctx.get("workspace_role_kind") or _effective_workspace_role() if real_workspace != process_root: if not _preflight_in_test_mode(): @@ -622,11 +615,15 @@ def verify_preflight_purity( dirty_files = sorted(_parse_porcelain_entries(_get_workspace_porcelain(workspace))) if dirty_files: - details = _preflight_workspace_details(workspace, dirty_files) raise RuntimeError( - "Pre-flight order violation: Active task workspace has tracked " - "file edits before mutation (fail closed). " - f"{_format_preflight_workspace_details(details)}" + nwb.format_namespace_workspace_binding_error( + role_kind=role, + workspace_path=workspace, + binding_source=ctx.get("workspace_binding_source") + or "unknown binding source", + dirty_files=dirty_files, + ignored_bindings=ctx.get("ignored_bindings"), + ) ) else: if _preflight_whoami_violation: @@ -642,14 +639,14 @@ def verify_preflight_purity( f"{_format_preflight_files(_preflight_capability_violation_files)}" ) - if _preflight_resolved_role == "reviewer": + if role in {"reviewer", "merger"}: current = _get_workspace_porcelain() baseline = _preflight_capability_baseline_porcelain or "" reviewer_delta = _new_tracked_changes_since(baseline, current) _preflight_reviewer_violation_files = reviewer_delta if reviewer_delta: raise RuntimeError( - "Reviewer role violation: Reviewer profile is forbidden from modifying " + f"{role.title()} role violation: profile is forbidden from modifying " "tracked workspace files (fail closed). Offending files: " f"{_format_preflight_files(reviewer_delta)}" ) @@ -657,6 +654,45 @@ def verify_preflight_purity( _enforce_branches_only_author_mutation(worktree_path) _clear_preflight_capability_state() + +def _verify_role_mutation_workspace( + remote: str | None = None, + *, + worktree_path: str | None = None, + worktree: str | None = None, + task: str | None = None, +) -> str: + """Bind reviewer/merger mutations to the active namespace workspace (#510).""" + role = _effective_workspace_role() + git_state = issue_lock_worktree.read_worktree_git_state( + _resolve_preflight_workspace_path(worktree_path) + ) + assessment = nwb.assess_namespace_mutation_workspace( + role_kind=role, + worktree_path=worktree_path, + worktree=worktree, + process_project_root=PROJECT_ROOT, + session_lease_worktree=( + _reviewer_session_worktree() if role in {"reviewer", "merger"} else None + ), + profile_name=get_profile().get("profile_name"), + current_branch=git_state.get("current_branch"), + ) + if assessment["block"]: + raise RuntimeError( + nwb.format_namespace_workspace_binding_error( + role_kind=role, + workspace_path=assessment["mutation_workspace"], + binding_source=assessment.get("workspace_binding_source") + or "unknown binding source", + reasons=assessment.get("reasons"), + ignored_bindings=assessment.get("ignored_bindings"), + ) + ) + resolved = assessment["mutation_workspace"] + verify_preflight_purity(remote, worktree_path=resolved, task=task) + return resolved + from mcp.server.fastmcp import FastMCP # noqa: E402 from gitea_auth import ( # noqa: E402 @@ -690,7 +726,6 @@ import author_mutation_worktree # noqa: E402 import issue_claim_heartbeat # noqa: E402 import issue_work_duplicate_gate # noqa: E402 import reviewer_pr_lease # noqa: E402 -import reviewer_mutation_workspace # noqa: E402 import merged_cleanup_reconcile # noqa: E402 import reconciler_profile # noqa: E402 import reconciliation_workflow # noqa: E402 @@ -2796,25 +2831,9 @@ def _evaluate_pr_review_submission( worktree_path: str | None = None, ) -> dict: """Shared gate chain for live submit and dry-run review tools.""" - try: - _verify_reviewer_mutation_workspace( - remote, worktree_path=worktree_path - ) - except RuntimeError as exc: - return { - "requested_action": (action or "").strip().lower(), - "performed": False, - "dry_run": not live, - "would_perform": False, - "authenticated_user": None, - "profile_name": get_profile()["profile_name"], - "pr_author": None, - "pr_number": pr_number, - "head_sha": None, - "expected_head_sha": expected_head_sha, - "remote": remote if remote in REMOTES else None, - "reasons": [str(exc)], - } + _verify_role_mutation_workspace( + remote, worktree_path=worktree_path, task="review_pr" + ) action = (action or "").strip().lower() result = { "requested_action": action, @@ -3005,16 +3024,9 @@ def gitea_mark_final_review_decision( remote: str = "dadeschools", org: str | None = None, repo: str | None = None, - worktree_path: str | None = None, ) -> dict: """Mark validation complete; the final review decision is ready to submit.""" action = (action or "").strip().lower() - try: - _verify_reviewer_mutation_workspace( - remote, worktree_path=worktree_path - ) - except RuntimeError as exc: - return {"marked_ready": False, "reasons": [str(exc)]} lock = _load_review_decision_lock() if lock is None: return { @@ -3630,6 +3642,7 @@ def gitea_merge_pr( host: str | None = None, org: str | None = None, repo: str | None = None, + worktree_path: str | None = None, ) -> dict: """Gated merge of a Gitea pull request (#16). @@ -3676,6 +3689,10 @@ def gitea_merge_pr( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + worktree_path: Merger workspace path under ``branches/`` or clean + control checkout; defaults to ``GITEA_MERGER_WORKTREE``, + ``GITEA_ACTIVE_WORKTREE``, or the MCP server process root. Ignores + foreign ``GITEA_AUTHOR_WORKTREE`` bindings (#510). Returns: dict describing the attempt: performed, authenticated user, profile @@ -3683,7 +3700,9 @@ def gitea_merge_pr( reasons/gates passed or blocked, and merge result / merge commit if available. Never secrets. """ - verify_preflight_purity(remote, task="merge_pr") + _verify_role_mutation_workspace( + remote, worktree_path=worktree_path, task="merge_pr" + ) do = (do or "").strip().lower() result = { "performed": False, @@ -5166,17 +5185,7 @@ def gitea_acquire_reviewer_pr_lease( "permission_report": _permission_block_report("gitea.pr.comment"), } - try: - resolved_worktree = _verify_reviewer_mutation_workspace( - remote, worktree=worktree - ) - except RuntimeError as exc: - return { - "success": False, - "acquired": False, - "reasons": [str(exc)], - "permission_report": None, - } + _verify_role_mutation_workspace(remote, worktree=worktree, task="review_pr") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) profile = get_profile() @@ -5194,7 +5203,7 @@ def gitea_acquire_reviewer_pr_lease( session_id=sid, repo=repo_label, issue_number=issue_number, - worktree=resolved_worktree, + worktree=worktree, candidate_head=candidate_head, target_branch=target_branch, target_branch_sha=target_branch_sha, @@ -5226,7 +5235,7 @@ def gitea_acquire_reviewer_pr_lease( "session_id": sid, "reviewer_identity": identity, "profile": profile.get("profile_name"), - "worktree": resolved_worktree, + "worktree": worktree, "phase": "claimed", "candidate_head": candidate_head, "target_branch": target_branch, @@ -5277,14 +5286,7 @@ def gitea_heartbeat_reviewer_pr_lease( ], } - try: - _verify_reviewer_mutation_workspace( - remote, - worktree=worktree, - worktree_path=session.get("worktree"), - ) - except RuntimeError as exc: - return {"success": False, "posted": False, "reasons": [str(exc)]} + verify_preflight_purity(remote, task="review_pr") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) body = reviewer_pr_lease.format_lease_body( diff --git a/namespace_workspace_binding.py b/namespace_workspace_binding.py new file mode 100644 index 0000000..4831907 --- /dev/null +++ b/namespace_workspace_binding.py @@ -0,0 +1,307 @@ +"""Namespace-scoped MCP workspace binding (#510). + +Each role namespace (author, reviewer, merger, reconciler) resolves its own +active task workspace. Foreign role worktree environment variables must not +poison workspace purity checks in another namespace. +""" + +from __future__ import annotations + +import os + +import author_mutation_worktree as amw + +ACTIVE_WORKTREE_ENV = amw.ACTIVE_WORKTREE_ENV +AUTHOR_WORKTREE_ENV = amw.AUTHOR_WORKTREE_ENV +REVIEWER_WORKTREE_ENV = "GITEA_REVIEWER_WORKTREE" +MERGER_WORKTREE_ENV = "GITEA_MERGER_WORKTREE" +RECONCILER_WORKTREE_ENV = "GITEA_RECONCILER_WORKTREE" + +ROLE_WORKTREE_ENVS: dict[str, str] = { + "author": AUTHOR_WORKTREE_ENV, + "reviewer": REVIEWER_WORKTREE_ENV, + "merger": MERGER_WORKTREE_ENV, + "reconciler": RECONCILER_WORKTREE_ENV, +} + +NON_AUTHOR_ROLES = frozenset({"reviewer", "merger", "reconciler"}) + + +def normalize_role_kind( + role_kind: str | None, + *, + profile_name: str | None = None, +) -> str: + """Map profile/task role to a workspace namespace key.""" + role = (role_kind or "author").strip().lower() + profile = (profile_name or "").strip().lower() + if role == "reviewer" and "merger" in profile: + return "merger" + if role in ROLE_WORKTREE_ENVS: + return role + return "author" + + +def _env_value(env: dict[str, str] | os._Environ, key: str) -> str | None: + text = (env.get(key) or "").strip() + return text or None + + +def resolve_namespace_workspace( + *, + role_kind: str, + worktree_path: str | None = None, + worktree: str | None = None, + process_project_root: str, + env: dict[str, str] | os._Environ | None = None, + session_lease_worktree: str | None = None, + profile_name: str | None = None, +) -> tuple[str, str]: + """Return ``(resolved_path, binding_source)`` for *role_kind*.""" + env_map = env if env is not None else os.environ + role = normalize_role_kind(role_kind, profile_name=profile_name) + role_env_key = ROLE_WORKTREE_ENVS[role] + + for candidate, source in ( + (worktree_path, "worktree_path argument"), + (worktree, "worktree argument"), + (_env_value(env_map, ACTIVE_WORKTREE_ENV), f"{ACTIVE_WORKTREE_ENV} environment variable"), + (_env_value(env_map, role_env_key), f"{role_env_key} environment variable"), + (session_lease_worktree if role in {"reviewer", "merger"} else None, + "reviewer PR lease worktree"), + ): + text = (candidate or "").strip() + if text: + return os.path.realpath(os.path.abspath(text)), source + + return os.path.realpath(process_project_root), "MCP server process root (default)" + + +def resolve_namespace_mutation_context( + *, + role_kind: str, + worktree_path: str | None, + process_project_root: str, + env: dict[str, str] | os._Environ | None = None, + session_lease_worktree: str | None = None, + worktree: str | None = None, + profile_name: str | None = None, +) -> dict: + """Shared workspace resolution for runtime_context and mutation guards.""" + workspace, binding_source = resolve_namespace_workspace( + role_kind=role_kind, + worktree_path=worktree_path, + worktree=worktree, + process_project_root=process_project_root, + env=env, + session_lease_worktree=session_lease_worktree, + profile_name=profile_name, + ) + process_root = os.path.realpath(process_project_root) + role = normalize_role_kind(role_kind, profile_name=profile_name) + pollution = assess_foreign_role_worktree_pollution( + role_kind=role, + resolved_workspace=workspace, + binding_source=binding_source, + env=env, + profile_name=profile_name, + ) + canonical_root = amw.resolve_canonical_repo_root(process_root, process_root) + return { + "workspace_path": workspace, + "workspace_binding_source": binding_source, + "workspace_role_kind": role, + "ignored_bindings": pollution.get("ignored_bindings") or [], + "process_project_root": process_root, + "canonical_repo_root": canonical_root, + "roots_aligned": canonical_root == process_root, + } + + +def assess_foreign_role_worktree_pollution( + *, + role_kind: str, + resolved_workspace: str, + binding_source: str, + env: dict[str, str] | os._Environ | None = None, + profile_name: str | None = None, +) -> dict: + """Detect when a foreign role env would have hijacked workspace binding.""" + env_map = env if env is not None else os.environ + role = normalize_role_kind(role_kind, profile_name=profile_name) + if role == "author": + return {"would_pollute": False, "ignored_bindings": []} + + ignored: list[str] = [] + author_path = _env_value(env_map, AUTHOR_WORKTREE_ENV) + if author_path: + author_real = os.path.realpath(os.path.abspath(author_path)) + resolved_real = os.path.realpath(resolved_workspace) + if author_real != resolved_real and binding_source != f"{AUTHOR_WORKTREE_ENV} environment variable": + ignored.append( + f"{AUTHOR_WORKTREE_ENV}={author_real} (ignored for {role} namespace)" + ) + return { + "would_pollute": bool(ignored), + "ignored_bindings": ignored, + } + + +def assess_metadata_only_worktree_binding( + *, + role_kind: str, + declared_worktree_path: str | None, + mutation_workspace: str, + process_project_root: str, + profile_name: str | None = None, +) -> dict: + """Fail closed when declared worktree_path would not redirect mutations.""" + declared = (declared_worktree_path or "").strip() + process_root = os.path.realpath(process_project_root) + mutation_root = os.path.realpath(mutation_workspace) + role = normalize_role_kind(role_kind, profile_name=profile_name) + if not declared: + return {"block": False, "reasons": [], "metadata_only": False} + + declared_root = os.path.realpath(os.path.abspath(declared)) + if declared_root == mutation_root: + return {"block": False, "reasons": [], "metadata_only": False} + + if declared_root != process_root and mutation_root == process_root: + return { + "block": True, + "metadata_only": True, + "reasons": [ + f"worktree_path is metadata-only for {role} mutations: preflight " + f"inspected '{declared_root}' but mutation tools would still " + f"validate MCP server process root '{process_root}'" + ], + "declared_worktree_path": declared_root, + "mutation_workspace": mutation_root, + "process_project_root": process_root, + } + + return {"block": False, "reasons": [], "metadata_only": False} + + +def format_namespace_workspace_binding_error( + *, + role_kind: str, + workspace_path: str, + binding_source: str, + reasons: list[str] | None = None, + ignored_bindings: list[str] | None = None, + dirty_files: list[str] | None = None, +) -> str: + """Canonical error when namespace workspace binding blocks mutations.""" + role = normalize_role_kind(role_kind) + workspace = os.path.realpath(workspace_path) + parts = [ + f"Namespace workspace binding blocked ({role} namespace, #510): " + f"resolved workspace '{workspace}' via {binding_source}." + ] + if ignored_bindings: + parts.append( + "Foreign role bindings ignored: " + "; ".join(ignored_bindings) + "." + ) + if dirty_files: + parts.append( + "Dirty tracked files in active task workspace: " + + ", ".join(dirty_files) + + "." + ) + if reasons: + parts.append("Details: " + "; ".join(reasons) + ".") + parts.append( + "Remediation: reconnect or relaunch the MCP server from a clean dedicated " + f"branches/ {role} worktree, set " + f"{ROLE_WORKTREE_ENVS.get(role, ACTIVE_WORKTREE_ENV)} or {ACTIVE_WORKTREE_ENV} " + "to that path, or pass worktree_path on mutation tools. Do not clean or " + "reset foreign role worktrees to unblock this namespace." + ) + return " ".join(parts) + + +def assess_namespace_mutation_workspace( + *, + role_kind: str, + worktree_path: str | None, + worktree: str | None, + process_project_root: str, + env: dict[str, str] | os._Environ | None = None, + session_lease_worktree: str | None = None, + profile_name: str | None = None, + current_branch: str | None = None, +) -> dict: + """Evaluate namespace workspace binding before preflight/mutation.""" + ctx = resolve_namespace_mutation_context( + role_kind=role_kind, + worktree_path=worktree_path, + worktree=worktree, + process_project_root=process_project_root, + env=env, + session_lease_worktree=session_lease_worktree, + profile_name=profile_name, + ) + mutation_workspace = ctx["workspace_path"] + binding_source = ctx["workspace_binding_source"] + role = ctx["workspace_role_kind"] + process_root = ctx["process_project_root"] + + metadata = assess_metadata_only_worktree_binding( + role_kind=role, + declared_worktree_path=worktree_path, + mutation_workspace=mutation_workspace, + process_project_root=process_root, + profile_name=profile_name, + ) + pollution = assess_foreign_role_worktree_pollution( + role_kind=role, + resolved_workspace=mutation_workspace, + binding_source=binding_source, + env=env, + profile_name=profile_name, + ) + + reasons = list(metadata.get("reasons") or []) + if role == "author": + branches = amw.assess_author_mutation_worktree( + workspace_path=mutation_workspace, + project_root=ctx["canonical_repo_root"], + current_branch=current_branch, + ) + if branches["block"]: + reasons.extend(branches["reasons"]) + elif ( + role == "reviewer" + and mutation_workspace == process_root + and not amw.is_path_under_branches(mutation_workspace, ctx["canonical_repo_root"]) + ): + reasons.append( + f"{role} mutation blocked: workspace is the stable control checkout; " + f"create or reconnect to a session-owned worktree under branches/ " + f"or set {ROLE_WORKTREE_ENVS[role]} / {ACTIVE_WORKTREE_ENV}" + ) + elif ( + role in {"reviewer", "merger"} + and mutation_workspace != process_root + and not amw.is_path_under_branches(mutation_workspace, ctx["canonical_repo_root"]) + ): + reasons.append( + f"{role} mutation blocked: workspace '{mutation_workspace}' is not under " + f"'{ctx['canonical_repo_root']}/branches/'" + ) + + block = bool(reasons) + return { + "block": block, + "reasons": reasons, + "mutation_workspace": mutation_workspace, + "workspace_binding_source": binding_source, + "workspace_role_kind": role, + "process_project_root": process_root, + "canonical_repo_root": ctx["canonical_repo_root"], + "metadata_only": metadata.get("metadata_only", False), + "declared_worktree_path": metadata.get("declared_worktree_path"), + "ignored_bindings": pollution.get("ignored_bindings") or [], + } \ No newline at end of file diff --git a/post_merge_cleanup_proof.py b/post_merge_cleanup_proof.py new file mode 100644 index 0000000..6562a04 --- /dev/null +++ b/post_merge_cleanup_proof.py @@ -0,0 +1,239 @@ +"""Post-merge cleanup proof verifier for reviewer final reports (#402).""" + +from __future__ import annotations + +import re +from typing import Any + +CLEANUP_SKIPPED = "CLEANUP_SKIPPED" +CLEANUP_PERFORMED = "CLEANUP_PERFORMED" + +_CLEANUP_SECTION_HINT = re.compile( + r"(?:cleanup (?:status|result|mutations)|post-merge cleanup|" + r"gitea_delete_branch|remote branch.*deleted|worktree remove)", + re.IGNORECASE, +) +_CLEANUP_SKIPPED_RE = re.compile(r"\bCLEANUP_SKIPPED\b", re.IGNORECASE) +_CLEANUP_BLOCKER_RE = re.compile( + r"(?:cleanup blocker|cleanup skip(?:ped)? reason)\s*:\s*(.+)", + re.IGNORECASE, +) +_REMOTE_DELETE_CLAIM_RE = re.compile( + r"(?:gitea_delete_branch|remote (?:head )?branch (?:was )?deleted|" + r"deleted remote branch|delete_branch)", + re.IGNORECASE, +) +_WORKTREE_REMOVE_CLAIM_RE = re.compile( + r"(?:git worktree remove|worktree (?:was )?removed|removed (?:local )?worktree|" + r"worktree cleanup performed)", + re.IGNORECASE, +) +_DELETE_CAPABILITY_RE = re.compile( + r"(?:delete[- ]branch capability resolved|gitea\.branch\.delete)\s*:\s*" + r".*(?:gitea\.branch\.delete|delete_branch).*(?:resolved|allowed|proven)|" + r"gitea\.branch\.delete\s+(?:resolved|allowed|proven)", + re.IGNORECASE, +) +_DELETE_TASK_RE = re.compile( + r"(?:delete_branch|cleanup_branch|reconcile_merged_cleanups)", + re.IGNORECASE, +) +_MERGE_RESULT_RE = re.compile( + r"merge result\s*:\s*(?:merged|success|performed)", + re.IGNORECASE, +) +_MERGE_COMMIT_SHA_RE = re.compile( + r"(?:merge commit sha|merged commit sha|merge commit)\s*:\s*([0-9a-f]{7,40})", + re.IGNORECASE, +) +_PR_HEAD_BRANCH_RE = re.compile( + r"(?:merged pr head branch|pr head branch|deleted branch)\s*:\s*(\S+)", + re.IGNORECASE, +) +_BRANCH_NOT_PROTECTED_RE = re.compile( + r"branch (?:is )?not protected|branch protection\s*:\s*(?:none|false|no)", + re.IGNORECASE, +) +_OPEN_PR_INVENTORY_RE = re.compile( + r"(?:no other open pr(?:\s+references)?(?:\s+\S+)?|open pr inventory proof|" + r"open pr references).*(?:none|zero|0|clear|inventory complete)|" + r"no other open pr references branch", + re.IGNORECASE, +) +_ACTIVE_CLAIM_LEASE_RE = re.compile( + r"(?:no active (?:heartbeat|claim|lease)|" + r"(?:active )?(?:heartbeat|claim|lease)(?:/(?:claim|lease))*\s*:\s*none)", + re.IGNORECASE, +) +_SESSION_OWNED_WORKTREE_RE = re.compile( + r"(?:removed worktree path|cleanup worktree path|session-owned worktree)\s*:\s*(\S+)", + re.IGNORECASE, +) +_BRANCHES_PATH_RE = re.compile(r"\bbranches/", re.IGNORECASE) +_CLEAN_TRACKED_RE = re.compile( + r"(?:pre-removal tracked state|tracked state before removal)\s*:\s*clean", + re.IGNORECASE, +) +_CLEAN_UNTRACKED_RE = re.compile( + r"(?:pre-removal untracked state|untracked state before removal)\s*:\s*clean", + re.IGNORECASE, +) +_WORKTREE_LIST_AFTER_RE = re.compile( + r"(?:git worktree list after|post-removal worktree list|worktree list after)", + re.IGNORECASE, +) +_WRONG_BRANCH_RE = re.compile( + r"deleted branch (?:does not match|!=|differs from) (?:merged )?pr head", + re.IGNORECASE, +) + + +def _claims_remote_delete(text: str) -> bool: + return bool(_REMOTE_DELETE_CLAIM_RE.search(text)) + + +def _claims_worktree_remove(text: str) -> bool: + return bool(_WORKTREE_REMOVE_CLAIM_RE.search(text)) + + +def _branch_safety_fields_present(text: str) -> list[str]: + missing: list[str] = [] + if not _DELETE_CAPABILITY_RE.search(text): + missing.append("delete-branch capability resolved (gitea.branch.delete)") + if not _DELETE_TASK_RE.search(text): + missing.append("delete-branch task named (delete_branch or cleanup)") + if not _MERGE_RESULT_RE.search(text): + missing.append("merge result: merged") + if not _MERGE_COMMIT_SHA_RE.search(text): + missing.append("merge commit SHA") + if not _PR_HEAD_BRANCH_RE.search(text): + missing.append("merged PR head branch / deleted branch name") + if not _BRANCH_NOT_PROTECTED_RE.search(text): + missing.append("branch not protected proof") + if not _OPEN_PR_INVENTORY_RE.search(text): + missing.append("open PR inventory proof (no other PR references branch)") + if not _ACTIVE_CLAIM_LEASE_RE.search(text): + missing.append("no active heartbeat/claim/lease proof") + return missing + + +def _worktree_cleanup_fields_present(text: str) -> list[str]: + missing: list[str] = [] + match = _SESSION_OWNED_WORKTREE_RE.search(text) + path = match.group(1).strip() if match else "" + if not path: + missing.append("session-owned worktree path") + elif not _BRANCHES_PATH_RE.search(path.replace("\\", "/")): + missing.append("worktree path under branches/") + if not _CLEAN_TRACKED_RE.search(text): + missing.append("pre-removal tracked state: clean") + if not _CLEAN_UNTRACKED_RE.search(text): + missing.append("pre-removal untracked state: clean") + if not _WORKTREE_LIST_AFTER_RE.search(text): + missing.append("git worktree list after removal") + return missing + + +def assess_post_merge_cleanup_proof( + report_text: str, + *, + cleanup_session: dict | None = None, +) -> dict[str, Any]: + """Validate post-merge cleanup claims carry safety-gate proof (#402).""" + text = report_text or "" + session = dict(cleanup_session or {}) + reasons: list[str] = [] + + if _CLEANUP_SKIPPED_RE.search(text) or session.get("outcome") == CLEANUP_SKIPPED: + blocker = (session.get("blocker") or "").strip() + if not blocker: + match = _CLEANUP_BLOCKER_RE.search(text) + blocker = match.group(1).strip() if match else "" + if blocker.upper() == CLEANUP_SKIPPED: + blocker = "" + if not blocker: + reasons.append( + "CLEANUP_SKIPPED requires exact cleanup blocker reason (#402)" + ) + return { + "block": bool(reasons), + "proven": not reasons, + "outcome": CLEANUP_SKIPPED, + "remote_delete_claimed": False, + "worktree_remove_claimed": False, + "reasons": reasons, + "safe_next_action": ( + "report CLEANUP_SKIPPED with exact blocker; do not claim performed cleanup" + if reasons + else "proceed" + ), + } + + if not _CLEANUP_SECTION_HINT.search(text) and not session.get("cleanup_claimed"): + return { + "block": False, + "proven": True, + "outcome": None, + "remote_delete_claimed": False, + "worktree_remove_claimed": False, + "reasons": [], + "safe_next_action": "proceed", + } + + remote_delete = bool( + session.get("remote_delete_claimed") or _claims_remote_delete(text) + ) + worktree_remove = bool( + session.get("worktree_remove_claimed") or _claims_worktree_remove(text) + ) + + if _WRONG_BRANCH_RE.search(text): + reasons.append( + "cleanup report claims deleted branch that is not the merged PR head branch" + ) + + if remote_delete: + reasons.extend( + f"remote branch deletion missing {field}" + for field in _branch_safety_fields_present(text) + ) + + if worktree_remove: + reasons.extend( + f"worktree removal missing {field}" + for field in _worktree_cleanup_fields_present(text) + ) + + if (remote_delete or worktree_remove) and not (remote_delete or worktree_remove): + pass + + if not remote_delete and not worktree_remove: + cleanup_mutations = re.search( + r"cleanup mutations\s*:\s*(?!none\b)\S", + text, + re.IGNORECASE, + ) + if cleanup_mutations: + reasons.append( + "cleanup mutations reported without post-merge cleanup proof checklist" + ) + + outcome = CLEANUP_PERFORMED if (remote_delete or worktree_remove) and not reasons else None + if remote_delete or worktree_remove: + outcome = CLEANUP_PERFORMED if not reasons else "CLEANUP_CLAIMED_UNPROVEN" + + block = bool(reasons) + return { + "block": block, + "proven": not block, + "outcome": outcome, + "remote_delete_claimed": remote_delete, + "worktree_remove_claimed": worktree_remove, + "reasons": reasons, + "safe_next_action": ( + "report CLEANUP_SKIPPED with exact blocker or include the full cleanup " + "checklist before claiming remote delete or worktree removal" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/reviewer_mutation_workspace.py b/reviewer_mutation_workspace.py deleted file mode 100644 index 4e6b365..0000000 --- a/reviewer_mutation_workspace.py +++ /dev/null @@ -1,157 +0,0 @@ -"""Reviewer mutation workspace binding (#503). - -Reviewer mutation tools must validate the same branches/ worktree that -read-only preflight and runtime_context inspect — not only the MCP server -process root. -""" - -from __future__ import annotations - -import os - -import author_mutation_worktree as amw - -ACTIVE_WORKTREE_ENV = amw.ACTIVE_WORKTREE_ENV - - -def resolve_reviewer_mutation_workspace( - *, - worktree_path: str | None, - worktree: str | None, - process_project_root: str, - active_worktree_env: str | None = None, - session_lease_worktree: str | None = None, -) -> str: - """Resolve the workspace inspected before reviewer mutations.""" - for candidate in ( - worktree_path, - worktree, - active_worktree_env, - session_lease_worktree, - ): - text = (candidate or "").strip() - if text: - return os.path.realpath(os.path.abspath(text)) - return os.path.realpath(process_project_root) - - -def assess_metadata_only_worktree_binding( - *, - declared_worktree_path: str | None, - mutation_workspace: str, - process_project_root: str, -) -> dict: - """Fail closed when declared worktree_path would not redirect mutations.""" - declared = (declared_worktree_path or "").strip() - process_root = os.path.realpath(process_project_root) - mutation_root = os.path.realpath(mutation_workspace) - if not declared: - return {"block": False, "reasons": [], "metadata_only": False} - - declared_root = os.path.realpath(os.path.abspath(declared)) - if declared_root == mutation_root: - return {"block": False, "reasons": [], "metadata_only": False} - - if declared_root != process_root and mutation_root == process_root: - return { - "block": True, - "metadata_only": True, - "reasons": [ - "worktree_path is metadata-only for reviewer mutations: preflight " - f"inspected '{declared_root}' but mutation tools would still " - f"validate MCP server process root '{process_root}'" - ], - "declared_worktree_path": declared_root, - "mutation_workspace": mutation_root, - "process_project_root": process_root, - } - - return {"block": False, "reasons": [], "metadata_only": False} - - -def format_stale_mcp_workspace_binding_error( - *, - process_project_root: str, - mutation_workspace: str, - declared_worktree_path: str | None = None, - metadata_only: bool = False, -) -> str: - """Canonical handoff when reviewer mutations bind to the wrong workspace.""" - process_root = os.path.realpath(process_project_root) - mutation_root = os.path.realpath(mutation_workspace) - declared = (declared_worktree_path or "").strip() - declared_root = ( - os.path.realpath(os.path.abspath(declared)) if declared else None - ) - - if metadata_only and declared_root: - lead = ( - "Reviewer mutation workspace mismatch (#503): worktree_path affected " - f"read-only preflight only ('{declared_root}') while mutation tools " - f"still inspect MCP server process root '{process_root}'." - ) - elif mutation_root == process_root: - lead = ( - "Reviewer mutation workspace mismatch (#503): mutation tools are " - f"bound to the stable control checkout '{process_root}' instead of " - "an active branches/ review worktree." - ) - else: - lead = ( - "Reviewer mutation workspace mismatch (#503): mutation workspace " - f"'{mutation_root}' does not match the declared active task worktree." - ) - - fixes = ( - "Fix: relaunch or reconnect the MCP server from the branches/ review " - "worktree, set GITEA_ACTIVE_WORKTREE to that path, pass worktree_path " - "to reviewer mutation tools, or acquire a reviewer PR lease from the " - "same branches/ worktree so mutations inherit the validated workspace." - ) - return f"{lead} {fixes}" - - -def assess_reviewer_mutation_workspace( - *, - worktree_path: str | None, - worktree: str | None, - process_project_root: str, - active_worktree_env: str | None = None, - session_lease_worktree: str | None = None, - current_branch: str | None = None, -) -> dict: - """Evaluate reviewer mutation workspace binding before preflight/mutation.""" - mutation_workspace = resolve_reviewer_mutation_workspace( - worktree_path=worktree_path, - worktree=worktree, - process_project_root=process_project_root, - active_worktree_env=active_worktree_env, - session_lease_worktree=session_lease_worktree, - ) - process_root = os.path.realpath(process_project_root) - metadata = assess_metadata_only_worktree_binding( - declared_worktree_path=worktree_path, - mutation_workspace=mutation_workspace, - process_project_root=process_root, - ) - reasons = list(metadata.get("reasons") or []) - canonical_root = amw.resolve_canonical_repo_root(process_root, process_root) - branches = amw.assess_author_mutation_worktree( - workspace_path=mutation_workspace, - project_root=canonical_root, - current_branch=current_branch, - ) - if branches["block"]: - reasons.extend(branches["reasons"]) - - block = bool(reasons) - return { - "block": block, - "reasons": reasons, - "mutation_workspace": mutation_workspace, - "process_project_root": process_root, - "canonical_repo_root": canonical_root, - "metadata_only": metadata.get("metadata_only", False), - "declared_worktree_path": metadata.get("declared_worktree_path"), - "under_branches": branches.get("under_branches", False), - } \ No newline at end of file diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 1c4e9be..76a7000 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -881,6 +881,40 @@ Do not update the main checkout if merge failed, was blocked, or produced reconc If any local artifact is created after final cleanup, run and report a new final status check. +## 28A. Post-merge cleanup proof checklist (#402) + +Successful tool execution is not proof that cleanup was authorized. Before claiming remote branch deletion or local worktree removal, the final report must carry the full safety checklist below. If any gate is missing, report `CLEANUP_SKIPPED` with the exact blocker — never perform cleanup and never claim it was performed. + +### Remote branch deletion checklist + +When `gitea_delete_branch` (or equivalent) deletes the merged PR head branch, report: + +* Delete-branch capability resolved: name the task (`delete_branch` / `cleanup_branch` / `reconcile_merged_cleanups`) and permission (`gitea.branch.delete`) with resolver proof before the delete call +* Merge result: merged +* Merge commit SHA: full 40-character SHA +* Merged PR head branch / deleted branch: exact branch name (must match) +* Branch protection: none / branch is not protected +* Open PR inventory proof: no other open PR references the branch +* Active heartbeat/claim/lease: none + +### Local worktree removal checklist + +When removing session-owned review/simulation worktrees under `branches/`, report: + +* Session-owned worktree path: exact path under `branches/` +* Pre-removal tracked state: clean +* Pre-removal untracked state: clean +* Git worktree list after removal: command output or equivalent proof + +### Skipped cleanup + +If any gate fails, report: + +* Cleanup outcome: `CLEANUP_SKIPPED` +* Cleanup blocker: exact missing gate (for example `gitea.branch.delete capability not resolved`) + +Skipped cleanup with an exact blocker passes validation. Performed-cleanup claims without the checklist fail validation. + ## 29. Recovery handoff rules If blocked, produce a recovery handoff with: diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 651b253..179159d 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3879,7 +3879,7 @@ class TestPreflightVerification(unittest.TestCase): os.environ["GITEA_TEST_PORCELAIN"] = " M reviewer_edit.py\n" with self.assertRaises(RuntimeError) as ctx: mcp_server.verify_preflight_purity() - self.assertIn("Reviewer profile is forbidden from modifying tracked workspace files", str(ctx.exception)) + self.assertIn("forbidden from modifying tracked workspace files", str(ctx.exception)) self.assertIn("reviewer_edit.py", str(ctx.exception)) # Foreign pre-existing dirty state does not block when unchanged. @@ -3945,7 +3945,8 @@ class TestPreflightVerification(unittest.TestCase): with self.assertRaises(RuntimeError) as ctx: mcp_server.verify_preflight_purity(worktree_path=worktree) msg = str(ctx.exception) - self.assertIn("active task workspace root", msg) - self.assertIn("inspected git root", msg) - self.assertIn("dirty files: task_file.py", msg) - self.assertIn("dirty scope:", msg) + self.assertIn("resolved workspace", msg) + self.assertIn(worktree, msg) + self.assertIn("worktree_path argument", msg) + self.assertIn("task_file.py", msg) + self.assertIn("author namespace", msg) diff --git a/tests/test_namespace_workspace_binding.py b/tests/test_namespace_workspace_binding.py new file mode 100644 index 0000000..b9c270c --- /dev/null +++ b/tests/test_namespace_workspace_binding.py @@ -0,0 +1,240 @@ +"""Tests for namespace-scoped MCP workspace binding (#510).""" + +from __future__ import annotations + +import os +import sys +import unittest +from pathlib import Path +from unittest import mock +from unittest.mock import MagicMock + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import gitea_mcp_server as srv # noqa: E402 +import namespace_workspace_binding as nwb # noqa: E402 + +REPO_ROOT = Path(__file__).resolve().parent.parent +if REPO_ROOT.parent.name == "branches": + CONTROL_ROOT = str(REPO_ROOT.parent.parent) +else: + CONTROL_ROOT = str(REPO_ROOT) + +AUTHOR_DIRTY = f"{CONTROL_ROOT}/branches/mcp-author-worktree" +MERGER_CLEAN = f"{CONTROL_ROOT}/branches/merge-pr487-submit" +REVIEWER_CLEAN = f"{CONTROL_ROOT}/branches/review-pr487-submit" +RECONCILER_CLEAN = f"{CONTROL_ROOT}/branches/reconcile-pr487" +MCP_PROCESS_ROOT = CONTROL_ROOT + + +class TestNamespaceWorkspaceModule(unittest.TestCase): + def test_author_env_ignored_for_merger_namespace(self): + workspace, source = nwb.resolve_namespace_workspace( + role_kind="merger", + worktree_path=None, + process_project_root=MCP_PROCESS_ROOT, + env={ + nwb.AUTHOR_WORKTREE_ENV: AUTHOR_DIRTY, + nwb.MERGER_WORKTREE_ENV: MERGER_CLEAN, + }, + profile_name="gitea-merger", + ) + self.assertEqual(workspace, os.path.realpath(MERGER_CLEAN)) + self.assertEqual(source, f"{nwb.MERGER_WORKTREE_ENV} environment variable") + + def test_author_env_ignored_for_reviewer_namespace(self): + workspace, source = nwb.resolve_namespace_workspace( + role_kind="reviewer", + worktree_path=None, + process_project_root=MCP_PROCESS_ROOT, + env={ + nwb.AUTHOR_WORKTREE_ENV: AUTHOR_DIRTY, + nwb.REVIEWER_WORKTREE_ENV: REVIEWER_CLEAN, + }, + ) + self.assertEqual(workspace, os.path.realpath(REVIEWER_CLEAN)) + self.assertEqual(source, f"{nwb.REVIEWER_WORKTREE_ENV} environment variable") + + def test_author_env_ignored_for_reconciler_namespace(self): + workspace, source = nwb.resolve_namespace_workspace( + role_kind="reconciler", + worktree_path=None, + process_project_root=MCP_PROCESS_ROOT, + env={ + nwb.AUTHOR_WORKTREE_ENV: AUTHOR_DIRTY, + nwb.RECONCILER_WORKTREE_ENV: RECONCILER_CLEAN, + }, + ) + self.assertEqual(workspace, os.path.realpath(RECONCILER_CLEAN)) + self.assertEqual(source, f"{nwb.RECONCILER_WORKTREE_ENV} environment variable") + + def test_merger_profile_maps_to_merger_namespace(self): + role = nwb.normalize_role_kind("reviewer", profile_name="gitea-merger") + self.assertEqual(role, "merger") + + def test_error_message_includes_path_and_binding_source(self): + msg = nwb.format_namespace_workspace_binding_error( + role_kind="merger", + workspace_path=AUTHOR_DIRTY, + binding_source=f"{nwb.AUTHOR_WORKTREE_ENV} environment variable", + dirty_files=["gitea_mcp_server.py"], + ignored_bindings=[f"{nwb.AUTHOR_WORKTREE_ENV}={AUTHOR_DIRTY} (ignored for merger namespace)"], + ) + self.assertIn(AUTHOR_DIRTY, msg) + self.assertIn("via", msg.lower()) + self.assertIn(nwb.AUTHOR_WORKTREE_ENV, msg) + self.assertIn("Do not clean or reset foreign role worktrees", msg) + + +class TestNamespaceWorkspaceIntegration(unittest.TestCase): + def setUp(self): + self._saved = { + "whoami_called": srv._preflight_whoami_called, + "capability_called": srv._preflight_capability_called, + "resolved_role": srv._preflight_resolved_role, + "whoami_violation": srv._preflight_whoami_violation, + "capability_violation": srv._preflight_capability_violation, + "in_test": srv._preflight_in_test_mode, + } + srv._preflight_whoami_called = True + srv._preflight_capability_called = True + srv._preflight_whoami_violation = False + srv._preflight_capability_violation = False + srv._preflight_in_test_mode = lambda: False + self._env_patch = mock.patch.dict(os.environ, {"GITEA_TEST_PORCELAIN": ""}, clear=False) + self._env_patch.start() + + def tearDown(self): + srv._preflight_whoami_called = self._saved["whoami_called"] + srv._preflight_capability_called = self._saved["capability_called"] + srv._preflight_resolved_role = self._saved["resolved_role"] + srv._preflight_whoami_violation = self._saved["whoami_violation"] + srv._preflight_capability_violation = self._saved["capability_violation"] + srv._preflight_in_test_mode = self._saved["in_test"] + self._env_patch.stop() + for key in ( + nwb.AUTHOR_WORKTREE_ENV, + nwb.MERGER_WORKTREE_ENV, + nwb.REVIEWER_WORKTREE_ENV, + nwb.RECONCILER_WORKTREE_ENV, + nwb.ACTIVE_WORKTREE_ENV, + ): + os.environ.pop(key, None) + + def _merger_profile(self): + return { + "profile_name": "gitea-merger", + "allowed_operations": ["gitea.pr.merge", "gitea.read"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + } + + def _reviewer_profile(self): + return { + "profile_name": "prgs-reviewer", + "allowed_operations": ["gitea.pr.approve", "gitea.pr.review", "gitea.read"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + } + + def _reconciler_profile(self): + return { + "profile_name": "prgs-reconciler", + "allowed_operations": ["gitea.pr.close", "gitea.read"], + "forbidden_operations": [ + "gitea.pr.approve", + "gitea.pr.merge", + "gitea.pr.create", + ], + } + + def _author_profile(self): + return { + "profile_name": "prgs-author", + "allowed_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.read"], + "forbidden_operations": ["gitea.pr.merge", "gitea.pr.approve"], + } + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_dirty_author_worktree_does_not_block_merger_with_clean_workspace( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock(returncode=0, stdout=f"{CONTROL_ROOT}/.git\n") + os.environ[nwb.AUTHOR_WORKTREE_ENV] = AUTHOR_DIRTY + os.environ[nwb.MERGER_WORKTREE_ENV] = MERGER_CLEAN + srv._preflight_resolved_role = "reviewer" + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._merger_profile()): + srv.verify_preflight_purity("prgs", worktree_path=MERGER_CLEAN) + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_dirty_author_worktree_does_not_block_reviewer_with_clean_workspace( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock(returncode=0, stdout=f"{CONTROL_ROOT}/.git\n") + os.environ[nwb.AUTHOR_WORKTREE_ENV] = AUTHOR_DIRTY + os.environ[nwb.REVIEWER_WORKTREE_ENV] = REVIEWER_CLEAN + srv._preflight_resolved_role = "reviewer" + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._reviewer_profile()): + srv.verify_preflight_purity("prgs", worktree_path=REVIEWER_CLEAN) + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_dirty_author_worktree_does_not_block_reconciler_with_clean_workspace( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock(returncode=0, stdout=f"{CONTROL_ROOT}/.git\n") + os.environ[nwb.AUTHOR_WORKTREE_ENV] = AUTHOR_DIRTY + os.environ[nwb.RECONCILER_WORKTREE_ENV] = RECONCILER_CLEAN + srv._preflight_resolved_role = "reconciler" + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._reconciler_profile()): + srv.verify_preflight_purity("prgs", worktree_path=RECONCILER_CLEAN) + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_dirty_active_task_workspace_still_blocks_mutations( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock(returncode=0, stdout=f"{CONTROL_ROOT}/.git\n") + os.environ[nwb.MERGER_WORKTREE_ENV] = MERGER_CLEAN + srv._preflight_resolved_role = "reviewer" + dirty = " M namespace_workspace_binding.py\n" + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._merger_profile()): + with mock.patch("gitea_mcp_server._get_workspace_porcelain", return_value=dirty): + with self.assertRaises(RuntimeError) as ctx: + srv.verify_preflight_purity("prgs", worktree_path=MERGER_CLEAN) + self.assertIn(MERGER_CLEAN, str(ctx.exception)) + self.assertIn("binding", str(ctx.exception).lower()) + + def test_root_workspace_mutation_still_blocked_for_author(self): + srv._preflight_resolved_role = "author" + with mock.patch.object(srv, "PROJECT_ROOT", CONTROL_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._author_profile()): + with mock.patch( + "gitea_mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={"current_branch": "master"}, + ): + with self.assertRaises(RuntimeError) as ctx: + srv.verify_preflight_purity("prgs") + self.assertIn("stable control checkout", str(ctx.exception)) + + @mock.patch("subprocess.run") + @mock.patch("os.path.isdir", return_value=True) + @mock.patch("os.path.exists", return_value=True) + def test_pr487_style_merge_binds_clean_merger_workspace( + self, _exists, _isdir, mock_run + ): + mock_run.return_value = MagicMock(returncode=0, stdout=f"{CONTROL_ROOT}/.git\n") + os.environ[nwb.AUTHOR_WORKTREE_ENV] = AUTHOR_DIRTY + srv._preflight_resolved_role = "reviewer" + with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): + with mock.patch("gitea_mcp_server.get_profile", return_value=self._merger_profile()): + resolved = srv._verify_role_mutation_workspace("prgs") + self.assertEqual(resolved, os.path.realpath(MCP_PROCESS_ROOT)) \ No newline at end of file diff --git a/tests/test_post_merge_cleanup_proof.py b/tests/test_post_merge_cleanup_proof.py new file mode 100644 index 0000000..10e5bf6 --- /dev/null +++ b/tests/test_post_merge_cleanup_proof.py @@ -0,0 +1,169 @@ +"""Tests for post-merge cleanup proof enforcement (#402).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from final_report_validator import assess_final_report_validator # noqa: E402 +from post_merge_cleanup_proof import ( # noqa: E402 + CLEANUP_SKIPPED, + assess_post_merge_cleanup_proof, +) + +MERGE_SHA = "a" * 40 +HEAD_BRANCH = "feat/issue-274-branches-only-worktrees" +WORKTREE = "branches/review-pr374-conflicts" + + +def _full_remote_cleanup_report(**overrides): + fields = { + "Task": "review PR #374", + "Merge result": "merged", + "Merge commit SHA": MERGE_SHA, + "Cleanup status": "remote branch deleted", + "Delete-branch capability resolved": "gitea.branch.delete allowed via delete_branch task", + "Merged PR head branch": HEAD_BRANCH, + "Deleted branch": HEAD_BRANCH, + "Branch protection": "none", + "Open PR inventory proof": "no other open PR references branch (inventory complete)", + "Active heartbeat/claim/lease": "none", + "Cleanup mutations": "gitea_delete_branch on remote head branch", + } + fields.update(overrides) + lines = ["## Controller Handoff", ""] + lines.extend(f"- {key}: {value}" for key, value in fields.items()) + return "\n".join(lines) + + +def _full_worktree_cleanup_report(**overrides): + fields = { + "Task": "review PR #374", + "Merge result": "merged", + "Cleanup status": "local worktree removed", + "Removed worktree path": WORKTREE, + "Pre-removal tracked state": "clean", + "Pre-removal untracked state": "clean", + "Git worktree list after removal": "only main checkout listed", + "Cleanup mutations": "git worktree remove on session-owned review worktree", + } + fields.update(overrides) + lines = ["## Controller Handoff", ""] + lines.extend(f"- {key}: {value}" for key, value in fields.items()) + return "\n".join(lines) + + +class TestPostMergeCleanupProof(unittest.TestCase): + def test_no_cleanup_claim_passes(self): + report = "## Controller Handoff\n- Task: review PR #1\n- Merge result: merged" + result = assess_post_merge_cleanup_proof(report) + self.assertFalse(result["block"]) + + def test_missing_capability_proof_blocked(self): + report = _full_remote_cleanup_report( + **{"Delete-branch capability resolved": "delete succeeded"} + ) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + self.assertTrue(any("capability" in r.lower() for r in result["reasons"])) + + def test_unmerged_pr_blocked(self): + report = _full_remote_cleanup_report(**{"Merge result": "not merged"}) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + self.assertTrue(any("merge result" in r.lower() for r in result["reasons"])) + + def test_wrong_branch_blocked(self): + report = _full_remote_cleanup_report( + **{"Deleted branch": "feat/other-branch"} + ) + report += "\nDeleted branch does not match merged PR head branch" + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_protected_branch_blocked(self): + report = _full_remote_cleanup_report(**{"Branch protection": "enabled"}) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_open_pr_reference_blocked(self): + report = _full_remote_cleanup_report( + **{"Open PR inventory proof": "PR #999 still references branch"} + ) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_active_claim_blocked(self): + report = _full_remote_cleanup_report( + **{"Active heartbeat/claim/lease": "status:in-progress on linked issue"} + ) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_dirty_worktree_blocked(self): + report = _full_worktree_cleanup_report( + **{"Pre-removal tracked state": "dirty"} + ) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + self.assertTrue(any("tracked" in r.lower() for r in result["reasons"])) + + def test_foreign_worktree_blocked(self): + report = _full_worktree_cleanup_report( + **{"Removed worktree path": "/tmp/foreign-worktree"} + ) + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + self.assertTrue(any("branches/" in r.lower() for r in result["reasons"])) + + def test_cleanup_skipped_with_blocker_passes(self): + report = "\n".join([ + "## Controller Handoff", + "- Cleanup outcome: CLEANUP_SKIPPED", + "- Cleanup blocker: gitea.branch.delete capability not resolved in active profile", + ]) + result = assess_post_merge_cleanup_proof(report) + self.assertFalse(result["block"]) + self.assertEqual(result["outcome"], CLEANUP_SKIPPED) + + def test_cleanup_skipped_without_blocker_blocked(self): + report = "## Controller Handoff\n- Cleanup outcome: CLEANUP_SKIPPED" + result = assess_post_merge_cleanup_proof(report) + self.assertTrue(result["block"]) + + def test_full_remote_cleanup_passes(self): + result = assess_post_merge_cleanup_proof(_full_remote_cleanup_report()) + self.assertFalse(result["block"]) + self.assertTrue(result["remote_delete_claimed"]) + + def test_full_worktree_cleanup_passes(self): + result = assess_post_merge_cleanup_proof(_full_worktree_cleanup_report()) + self.assertFalse(result["block"]) + self.assertTrue(result["worktree_remove_claimed"]) + + def test_validator_integration_blocks_unproven_delete(self): + report = ( + "## Controller Handoff\n" + "- Cleanup mutations: gitea_delete_branch deleted remote branch\n" + ) + result = assess_final_report_validator(report, task_kind="review_pr") + self.assertTrue(result["blocked"]) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.post_merge_cleanup_proof", rule_ids) + + def test_validator_integration_allows_skipped(self): + report = "\n".join([ + "## Controller Handoff", + "- Cleanup outcome: CLEANUP_SKIPPED", + "- Cleanup blocker: branch still referenced by open PR #414", + ]) + result = assess_final_report_validator(report, task_kind="review_pr") + cleanup_findings = [ + f for f in result["findings"] + if f["rule_id"] == "reviewer.post_merge_cleanup_proof" + ] + self.assertEqual(cleanup_findings, []) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_reviewer_mutation_workspace.py b/tests/test_reviewer_mutation_workspace.py deleted file mode 100644 index 666b1c1..0000000 --- a/tests/test_reviewer_mutation_workspace.py +++ /dev/null @@ -1,217 +0,0 @@ -"""Tests for reviewer mutation workspace binding (#503).""" - -from __future__ import annotations - -import os -import sys -import unittest -from pathlib import Path -from unittest import mock -from unittest.mock import MagicMock - -sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) - -import gitea_mcp_server as srv # noqa: E402 -import reviewer_mutation_workspace as rmw # noqa: E402 -import reviewer_pr_lease # noqa: E402 - -REPO_ROOT = Path(__file__).resolve().parent.parent -if REPO_ROOT.parent.name == "branches": - CONTROL_ROOT = str(REPO_ROOT.parent.parent) -else: - CONTROL_ROOT = str(REPO_ROOT) -BRANCHES_WORKTREE = f"{CONTROL_ROOT}/branches/review-pr503-submit" -MCP_PROCESS_ROOT = CONTROL_ROOT - - -class TestReviewerMutationWorkspaceModule(unittest.TestCase): - def test_resolve_prefers_explicit_worktree_path(self): - resolved = rmw.resolve_reviewer_mutation_workspace( - worktree_path=BRANCHES_WORKTREE, - worktree="/ignored", - process_project_root=MCP_PROCESS_ROOT, - active_worktree_env="/env/path", - session_lease_worktree="/lease/path", - ) - self.assertEqual(resolved, os.path.realpath(BRANCHES_WORKTREE)) - - def test_resolve_falls_back_to_session_lease(self): - resolved = rmw.resolve_reviewer_mutation_workspace( - worktree_path=None, - worktree=None, - process_project_root=MCP_PROCESS_ROOT, - session_lease_worktree=BRANCHES_WORKTREE, - ) - self.assertEqual(resolved, os.path.realpath(BRANCHES_WORKTREE)) - - def test_metadata_only_binding_detected(self): - assessment = rmw.assess_metadata_only_worktree_binding( - declared_worktree_path=BRANCHES_WORKTREE, - mutation_workspace=MCP_PROCESS_ROOT, - process_project_root=MCP_PROCESS_ROOT, - ) - self.assertTrue(assessment["block"]) - self.assertTrue(assessment["metadata_only"]) - self.assertIn("metadata-only", assessment["reasons"][0]) - - def test_canonical_error_mentions_fix_paths(self): - msg = rmw.format_stale_mcp_workspace_binding_error( - process_project_root=MCP_PROCESS_ROOT, - mutation_workspace=MCP_PROCESS_ROOT, - declared_worktree_path=BRANCHES_WORKTREE, - metadata_only=True, - ) - self.assertIn("GITEA_ACTIVE_WORKTREE", msg) - self.assertIn("worktree_path", msg) - self.assertIn("branches/", msg) - - -class TestReviewerMutationWorkspaceIntegration(unittest.TestCase): - def setUp(self): - reviewer_pr_lease.clear_session_lease() - self._saved = { - "whoami_called": srv._preflight_whoami_called, - "capability_called": srv._preflight_capability_called, - "resolved_role": srv._preflight_resolved_role, - "whoami_violation": srv._preflight_whoami_violation, - "whoami_files": list(srv._preflight_whoami_violation_files), - "capability_violation": srv._preflight_capability_violation, - "capability_files": list(srv._preflight_capability_violation_files), - "reviewer_files": list(srv._preflight_reviewer_violation_files), - "in_test": srv._preflight_in_test_mode, - } - srv._preflight_whoami_called = True - srv._preflight_capability_called = True - srv._preflight_resolved_role = "reviewer" - srv._preflight_whoami_violation = True - srv._preflight_whoami_violation_files = ["gitea_mcp_server.py"] - srv._preflight_capability_violation = False - srv._preflight_capability_violation_files = [] - srv._preflight_reviewer_violation_files = [] - srv._preflight_in_test_mode = lambda: False - self._env_patch = mock.patch.dict(os.environ, {"GITEA_TEST_PORCELAIN": ""}, clear=False) - self._env_patch.start() - os.environ.pop("GITEA_ACTIVE_WORKTREE", None) - os.environ.pop("GITEA_AUTHOR_WORKTREE", None) - - def tearDown(self): - reviewer_pr_lease.clear_session_lease() - srv._preflight_whoami_called = self._saved["whoami_called"] - srv._preflight_capability_called = self._saved["capability_called"] - srv._preflight_resolved_role = self._saved["resolved_role"] - srv._preflight_whoami_violation = self._saved["whoami_violation"] - srv._preflight_whoami_violation_files = self._saved["whoami_files"] - srv._preflight_capability_violation = self._saved["capability_violation"] - srv._preflight_capability_violation_files = self._saved["capability_files"] - srv._preflight_reviewer_violation_files = self._saved["reviewer_files"] - srv._preflight_in_test_mode = self._saved["in_test"] - self._env_patch.stop() - - @mock.patch("subprocess.run") - @mock.patch("os.path.isdir", return_value=True) - @mock.patch("os.path.exists", return_value=True) - def test_reviewer_mutation_uses_declared_branches_worktree( - self, _exists, _isdir, mock_run - ): - mock_run.return_value = MagicMock( - returncode=0, - stdout=f"{CONTROL_ROOT}/.git\n", - ) - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - resolved = srv._verify_reviewer_mutation_workspace( - "prgs", worktree_path=BRANCHES_WORKTREE - ) - self.assertEqual(resolved, os.path.realpath(BRANCHES_WORKTREE)) - - def test_control_checkout_blocks_reviewer_mutations(self): - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - with self.assertRaises(RuntimeError) as ctx: - srv._verify_reviewer_mutation_workspace("prgs") - self.assertIn("stable control checkout", str(ctx.exception)) - self.assertIn("GITEA_ACTIVE_WORKTREE", str(ctx.exception)) - - def test_preflight_and_mutation_agree_on_workspace(self): - os.environ["GITEA_ACTIVE_WORKTREE"] = BRANCHES_WORKTREE - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - with mock.patch.object( - srv, - "_get_git_root", - return_value=BRANCHES_WORKTREE, - ): - status = srv.assess_preflight_status(worktree_path=BRANCHES_WORKTREE) - resolved = srv._resolve_reviewer_mutation_worktree_path( - worktree_path=BRANCHES_WORKTREE - ) - self.assertTrue(status["preflight_ready"]) - self.assertEqual( - status["preflight_workspace"]["active_task_workspace_root"], - os.path.realpath(BRANCHES_WORKTREE), - ) - self.assertEqual(resolved, os.path.realpath(BRANCHES_WORKTREE)) - - def test_metadata_only_worktree_path_blocks_preflight_ready(self): - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - status = srv.assess_preflight_status(worktree_path=BRANCHES_WORKTREE) - self.assertFalse(status["preflight_ready"]) - self.assertTrue( - any("metadata-only" in reason for reason in status["preflight_block_reasons"]) - ) - - @mock.patch("subprocess.run") - @mock.patch("os.path.isdir", return_value=True) - @mock.patch("os.path.exists", return_value=True) - def test_dry_run_inherits_session_lease_worktree( - self, _exists, _isdir, mock_run - ): - mock_run.return_value = MagicMock( - returncode=0, - stdout=f"{CONTROL_ROOT}/.git\n", - ) - reviewer_pr_lease.record_session_lease({ - "pr_number": 503, - "session_id": "sess-503", - "worktree": BRANCHES_WORKTREE, - }) - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - with mock.patch.object( - srv, - "gitea_check_pr_eligibility", - return_value={ - "eligible": False, - "reasons": ["stub"], - "authenticated_user": "reviewer", - "profile_name": "prgs-reviewer", - "pr_author": "author", - "head_sha": "abc123", - }, - ): - result = srv.gitea_dry_run_pr_review( - pr_number=503, - action="comment", - remote="prgs", - ) - self.assertNotIn("stable control checkout", " ".join(result.get("reasons") or [])) - self.assertNotIn("metadata-only", " ".join(result.get("reasons") or [])) - - @mock.patch("subprocess.run") - @mock.patch("os.path.isdir", return_value=True) - @mock.patch("os.path.exists", return_value=True) - def test_dry_run_fails_closed_from_control_checkout_without_binding( - self, _exists, _isdir, mock_run - ): - mock_run.return_value = MagicMock( - returncode=0, - stdout=f"{CONTROL_ROOT}/.git\n", - ) - with mock.patch.object(srv, "PROJECT_ROOT", MCP_PROCESS_ROOT): - result = srv.gitea_dry_run_pr_review( - pr_number=503, - action="comment", - remote="prgs", - ) - reasons = " ".join(result.get("reasons") or []) - self.assertIn("stable control checkout", reasons) - - -if __name__ == "__main__": - unittest.main()