Compare commits

..
Author SHA1 Message Date
sysadmin 6b675f5c83 fix: harden structured auth MCP errors against reviewer findings (#699)
Address PR #701 request-changes-class defects:

1. Fixed messages only — never embed HTTP bodies, Keychain, or exception
   text in tool results or daemon logs; sanitization fails closed.
2. Narrow Tool.run boundary wraps original success path; re-raises
   UrlElicitationRequiredError; install is idempotent.
3. No RuntimeError substring heuristics; only typed client failures
   are classified as auth/authz/network/config.
4. Central classify_http_status — every HTTP 403 is GiteaAuthzError.

Regressions cover adversarial secrets, stdio survival, elicitation,
parser RuntimeError, generic 403, repeated install, profiles, provenance.

Closes #699
2026-07-13 13:40:38 -04:00
sysadmin b4d0cb22e4 fix: map Gitea auth failures to structured MCP tool errors (Closes #699)
HTTP 401/scope-403/network failures become typed client errors with stable
reason codes. The FastMCP Tool.run boundary returns CallToolResult isError
payloads so stdio transport survives; daemon logs use sanitized reason codes
only. Regression tests cover author/reconciler profiles, transport survival,
and non-misclassification of unexpected exceptions.

Cross-links: #685, #695, #697, #698, PR #696 (scopes not absorbed).
2026-07-13 13:10:00 -04:00
sysadmin 237656702f Merge pull request 'feat(lease): guarded cleanup for obsolete reviewer comment leases (Closes #691)' (#692) from fix/issue-691-obsolete-reviewer-lease-cleanup into master 2026-07-12 22:28:59 -05:00
sysadmin 6d86db793b feat(lease): guarded cleanup for obsolete reviewer comment leases (Closes #691)
Add fail-closed assessment and MCP tool for expired/superseded-head
comment-backed reviewer leases that lack a control-plane lease_id.

- assess_obsolete_reviewer_comment_lease_cleanup eligibility matrix
- diagnose classifications: foreign_active_current_head,
  foreign_expired_current_head, foreign_completed_superseded_head,
  foreign_expired_superseded_head, orphaned_owner_missing,
  ambiguous_conflicting_evidence
- gitea_cleanup_obsolete_reviewer_comment_lease with confirmation
  CLEANUP OBSOLETE REVIEWER LEASE <n> and controller_recovery_authorized
- preserve audit history via append-only phase=released marker
- never repoint, transfer validation, use PID ownership, or steal
  active current-head foreign leases
- regression for PR #688 lease comment 10749 after recorded expiry
2026-07-12 23:09:20 -04:00
19 changed files with 2713 additions and 951 deletions
-18
View File
@@ -7,19 +7,6 @@ from typing import Any
PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) PROTECTED_BRANCHES = frozenset({"master", "main", "dev"})
# Evidence / preservation branches must never be removed by cleanup tools
# (e.g. chore/issue-681-preserve-review-session-wip).
_PRESERVATION_MARKERS = ("preserve", "preservation", "evidence")
def is_preservation_or_evidence_branch(branch: str | None) -> bool:
"""Return True when *branch* is a preservation/evidence ref that must stay."""
if not branch:
return False
name = str(branch).lower()
return any(marker in name for marker in _PRESERVATION_MARKERS)
_RAW_BRANCH_DELETE_PATTERNS = ( _RAW_BRANCH_DELETE_PATTERNS = (
re.compile(r"\bgit(?:\s+-C\s+\S+)?\s+branch\s+-[dD]\b[^\n\r]*", re.I), re.compile(r"\bgit(?:\s+-C\s+\S+)?\s+branch\s+-[dD]\b[^\n\r]*", re.I),
re.compile(r"\bgit(?:\s+-C\s+\S+)?\s+push\b[^\n\r]*\s--delete\b[^\n\r]*", re.I), re.compile(r"\bgit(?:\s+-C\s+\S+)?\s+push\b[^\n\r]*\s--delete\b[^\n\r]*", re.I),
@@ -85,11 +72,6 @@ def assess_merged_pr_branch_cleanup(
reasons.append("PR head branch is missing") reasons.append("PR head branch is missing")
if head_branch in protected: if head_branch in protected:
reasons.append(f"branch '{head_branch}' is protected") reasons.append(f"branch '{head_branch}' is protected")
if is_preservation_or_evidence_branch(head_branch):
reasons.append(
f"branch '{head_branch}' is a preservation/evidence branch and "
"cannot be deleted through merged-PR cleanup"
)
if head_branch in open_pr_heads: if head_branch in open_pr_heads:
reasons.append("an open PR still references this head branch") reasons.append("an open PR still references this head branch")
if head_on_target is False: if head_on_target is False:
-185
View File
@@ -238,43 +238,11 @@ narrow operation set:
- `gitea.issue.comment` - `gitea.issue.comment`
- `gitea.issue.close` - `gitea.issue.close`
- `gitea.pr.close` - `gitea.pr.close`
- `gitea.branch.delete` (merged-branch cleanup only — see below)
Forbidden on reconciler profiles: `gitea.pr.approve`, `gitea.pr.merge`, Forbidden on reconciler profiles: `gitea.pr.approve`, `gitea.pr.merge`,
`gitea.pr.review`, `gitea.pr.create`, `gitea.branch.push`, and `gitea.pr.review`, `gitea.pr.create`, `gitea.branch.push`, and
`gitea.repo.commit`. `gitea.repo.commit`.
### Merged-branch cleanup ownership (`gitea.branch.delete`)
The reconciler is the repository-supported owner of merged-PR source-branch
cleanup: `task_capability_map` maps `cleanup_merged_pr_branch` (and
`reconciliation_cleanup`) to role `reconciler` with permission
`gitea.branch.delete`. Post-merge branch lifecycle is reconciliation work —
it happens after the author, reviewer, and merger roles have completed, and
it must not be reachable from those roles.
Least-privilege constraints:
- `gitea.branch.delete` is granted **only** to reconciler profiles. Author,
reviewer, and merger profiles must never hold it; `gitea_delete_branch`
and `gitea_cleanup_merged_pr_branch` fail closed on any profile without
the permission.
- Even with the permission, reconciler deletion is only supported through the
guarded `gitea_cleanup_merged_pr_branch` path (#514 / #687): the PR must be
merged, the head an ancestor of the target, the branch not protected
(`master`/`main`/`dev`), the branch not a preservation/evidence ref (e.g.
`chore/issue-681-preserve-review-session-wip`), no open PR may still use the
head, and an explicit `CLEANUP MERGED PR <n> BRANCH <branch>` confirmation is
required. Raw `gitea_delete_branch` is **denied** to reconciler even when
`gitea.branch.delete` is present.
- Raw `git branch -d` / `git push --delete` cleanup remains blocked by
`branch_cleanup_guard` and the final-report validator regardless of
profile permissions.
- `gitea.branch.delete` has no short alias in `GITEA_OPERATION_ALIASES`;
write it fully qualified in `allowed_operations`. Migration must emit
canonical names such as `gitea.pr.close` (never bare `pr.close` /
`issue.close`, which the production normalizer rejects or drops).
Launch a static `gitea-reconciler` MCP namespace with Launch a static `gitea-reconciler` MCP namespace with
`GITEA_MCP_PROFILE=prgs-reconciler`. Profile shape is validated by `GITEA_MCP_PROFILE=prgs-reconciler`. Profile shape is validated by
`reconciler_profile.assess_reconciler_profile` (#304). Use the `reconciler_profile.assess_reconciler_profile` (#304). Use the
@@ -283,159 +251,6 @@ Launch a static `gitea-reconciler` MCP namespace with
fresh target-branch fetch, recorded target SHA, and ancestor proof. PRs whose fresh target-branch fetch, recorded target SHA, and ancestor proof. PRs whose
heads are not already landed cannot be closed through this path. heads are not already landed cannot be closed through this path.
### Operational runbook: grant reconciler `gitea.branch.delete` (#687)
Merging a code PR that updates `migrate_profiles.py` / `reconciler_profile.py`
**does not** change the live operator profile on disk. Apply the profile
change deliberately, then reconnect the client-managed namespace.
1. **Approved migration / profile-update command** (from the repo root, using
the project venv if present):
```bash
# Dry-run first (default): validates v2 output, writes nothing
python3 migrate_profiles.py -i ~/.config/gitea-tools/profiles.json
# Apply: creates backup then writes migrated v2 config
python3 migrate_profiles.py -i ~/.config/gitea-tools/profiles.json -w
# Optional explicit paths:
# python3 migrate_profiles.py -i ~/.config/gitea-tools/profiles.json \
# -o ~/.config/gitea-tools/profiles.json \
# --backup ~/.config/gitea-tools/profiles.json.bak -w
```
If the live file is already v2, edit the reconciler identitys
`allowed_operations` / `forbidden_operations` under
`environments.<env>.services.gitea.identities.reconciler` (or the
`prgs-reconciler` alias target) so allowed includes the canonical set
below — then re-validate with a load of the config (see step 3).
2. **Inspect the generated (or edited) profile** — confirm the reconciler
identity, for example:
```bash
python3 - <<'PY'
import json
from pathlib import Path
cfg = json.loads(Path.home().joinpath(".config/gitea-tools/profiles.json").read_text())
# v2 environments shape:
ident = cfg["environments"]["prgs"]["services"]["gitea"]["identities"]["reconciler"]
print("role:", ident.get("role"))
print("allowed:", ident.get("allowed_operations"))
print("forbidden:", ident.get("forbidden_operations"))
PY
```
3. **Validate canonical operation names and least privilege**
Expected canonical **allowed** (defaults after migration):
- `gitea.read`
- `gitea.pr.close` (required)
- `gitea.pr.comment`
- `gitea.issue.comment`
- `gitea.issue.close`
- `gitea.branch.delete` (recommended; cleanup only)
Expected **forbidden** includes at least: `gitea.pr.approve`,
`gitea.pr.merge`, `gitea.pr.review`, `gitea.pr.create`,
`gitea.branch.push`, `gitea.repo.commit`.
No shorthand (`pr.close`, `issue.close`, `pr.comment`) may remain.
Validate with the production loader:
```bash
python3 - <<'PY'
import gitea_config, reconciler_profile
from pathlib import Path
path = str(Path.home() / ".config/gitea-tools/profiles.json")
gitea_config.load_config(path) # fails closed on invalid config
# Or assess the reconciler lists directly after extracting them:
# print(reconciler_profile.assess_reconciler_profile(allowed, forbidden))
PY
```
4. **Merging PR #688 (or any code PR) does not update the live profile.**
Code changes only the migration helper, schema, docs, and tests. The
operator must still run `migrate_profiles.py -w` or an equivalent
authorized edit of `~/.config/gitea-tools/profiles.json`.
5. **Supported apply method:** `python3 migrate_profiles.py … -w` (backup
created automatically) **or** operator-authorized edit of the live
profiles file after backup. Unsupported: silent mtime tricks, manual
process kill to “reload”, or undocumented env overrides.
6. **Backup and validation:** `-w` copies the input to
`<input_path>.bak` (or `--backup PATH`) before writing. Re-run
`load_config` / `assess_reconciler_profile` after write. Keep the
`.bak` until live whoami/capability checks pass.
7. **Client-managed namespace reconnect/reload:** reconnect or reload the
IDE MCP client so `gitea-reconciler` restarts from current `master` and
the updated `GITEA_MCP_PROFILE=prgs-reconciler` config. Do not hand-launch
`mcp_server.py` / `gitea_mcp_server.py` with ad hoc `GITEA_*` env
(see #686 / #630).
8. **Live reverification** (through the client-managed `gitea-reconciler`
namespace only):
- `gitea_whoami` → identity + profile `prgs-reconciler`
- `gitea_assess_master_parity` → `stale=false`, `restart_required=false`
- `gitea_resolve_task_capability(task="cleanup_merged_pr_branch")` →
`allowed_in_current_session=true` only when permission and role match
- `gitea_resolve_task_capability(task="delete_branch")` →
**not** allowed for reconciler (role denial must be enforced)
9. **Guarded cleanup usage** (example for a merged PR whose source branch
remains on the remote):
```text
gitea_cleanup_merged_pr_branch(
pr_number=<N>,
branch=<exact PR head branch>,
confirmation="CLEANUP MERGED PR <N> BRANCH <exact PR head branch>",
remote="prgs",
org="Scaled-Tech-Consulting",
repo="Gitea-Tools",
worktree_path="<path under branches/>",
)
```
The tool refuses unmerged PRs, protected branches, preservation/evidence
branches, open-PR heads, mismatched branch names, and wrong confirmation.
10. **Prohibitions**
- No raw `git push --delete`, `git branch -d` / `-D`, or delete refspecs
- No arbitrary `gitea_delete_branch` from reconciler
- No unsupported profile switching mid-run without full re-preflight
- No ad hoc hand-edits of live profiles **unless** operator-authorized,
backed up, and revalidated as above
Canonical migrated reconciler example:
```json
{
"role": "reconciler",
"allowed_operations": [
"gitea.read",
"gitea.pr.close",
"gitea.pr.comment",
"gitea.issue.comment",
"gitea.issue.close",
"gitea.branch.delete"
],
"forbidden_operations": [
"gitea.pr.approve",
"gitea.pr.merge",
"gitea.pr.review",
"gitea.pr.create",
"gitea.branch.push",
"gitea.repo.commit"
]
}
```
## Identity and fail-closed rules ## Identity and fail-closed rules
Before **any** mutating action, a workflow must know both: Before **any** mutating action, a workflow must know both:
+207 -19
View File
@@ -243,6 +243,192 @@ def _redact(text):
return str(text) return str(text)
# ── Classified client failures (#699) ─────────────────────────────────────────
# Subclasses of RuntimeError preserve existing ``except RuntimeError`` call
# sites. Exception *messages* are fixed constants only — HTTP response bodies,
# Keychain material, and arbitrary exception text are never stored on the
# exception or re-emitted to tool results / daemon logs.
# Fixed messages (must match mcp_tool_error_boundary.FIXED_MESSAGES keys used here).
_MSG_AUTH_INVALID = "Gitea authentication failed: invalid or revoked credentials"
_MSG_AUTH_FAILED = "Gitea authentication failed"
_MSG_AUTHZ_SCOPE = "Gitea authorization failed: insufficient token scope"
_MSG_AUTHZ_DENIED = "Gitea authorization failed: access denied"
_MSG_NETWORK = "Network error contacting Gitea"
_MSG_CONFIG = "Gitea configuration or credential resolution failed"
_MSG_UPSTREAM = "Gitea upstream unavailable"
_MSG_HTTP = "Gitea HTTP request failed"
class GiteaClientError(RuntimeError):
"""Base for known Gitea client failures with stable reason_code metadata."""
reason_code = "client_error"
error_class = "client"
http_status = None
def __init__(self, message=None, *, reason_code=None, http_status=None):
if reason_code is not None:
self.reason_code = reason_code
if http_status is not None:
self.http_status = http_status
# Message is always a fixed constant; callers cannot inject bodies.
fixed = message if message is not None else _MSG_HTTP
super().__init__(fixed)
class GiteaAuthError(GiteaClientError):
"""Authentication failure (invalid/revoked credentials → typically HTTP 401)."""
reason_code = "auth_invalid_token"
error_class = "authentication"
http_status = 401
def __init__(self, message=None, *, reason_code=None, http_status=None):
super().__init__(
message if message is not None else _MSG_AUTH_INVALID,
reason_code=reason_code or "auth_invalid_token",
http_status=http_status if http_status is not None else 401,
)
class GiteaAuthzError(GiteaClientError):
"""Authorization failure (HTTP 403 — scope deficiency or access denied)."""
reason_code = "authz_denied"
error_class = "authorization"
http_status = 403
def __init__(self, message=None, *, reason_code=None, http_status=None):
code = reason_code or "authz_denied"
if code == "authz_insufficient_scope":
fixed = _MSG_AUTHZ_SCOPE
else:
fixed = _MSG_AUTHZ_DENIED
code = "authz_denied"
super().__init__(
message if message is not None else fixed,
reason_code=code,
http_status=http_status if http_status is not None else 403,
)
class GiteaNetworkError(GiteaClientError):
"""Transport / DNS / timeout failure contacting Gitea."""
reason_code = "network_error"
error_class = "network"
http_status = None
def __init__(self, message=None, *, reason_code=None, http_status=None):
super().__init__(
message if message is not None else _MSG_NETWORK,
reason_code=reason_code or "network_error",
http_status=http_status,
)
class GiteaConfigError(GiteaClientError):
"""Local configuration / credential resolution failure (not HTTP auth)."""
reason_code = "config_error"
error_class = "configuration"
http_status = None
def __init__(self, message=None, *, reason_code=None, http_status=None):
super().__init__(
message if message is not None else _MSG_CONFIG,
reason_code=reason_code or "config_error",
http_status=http_status,
)
class GiteaHttpError(GiteaClientError):
"""Non-auth HTTP failure with fixed message (no response body)."""
reason_code = "http_error"
error_class = "client"
http_status = None
def __init__(self, message=None, *, reason_code=None, http_status=None):
code = reason_code or "http_error"
if code == "upstream_unavailable":
fixed = _MSG_UPSTREAM
else:
fixed = _MSG_HTTP
code = "http_error"
super().__init__(
message if message is not None else fixed,
reason_code=code,
http_status=http_status,
)
def _looks_like_insufficient_scope(detail: str) -> bool:
"""Internal: inspect redacted body *only* to refine 403 reason_code.
The body is never stored on the exception or returned to callers.
"""
lower = (detail or "").lower()
markers = (
"insufficient scope",
"required scope",
"does not have at least one of required scope",
"token does not have",
"missing scope",
"scope(s)",
)
return any(m in lower for m in markers)
def classify_http_status(code: int, *, body_hint: str = "") -> tuple[type, str, int]:
"""Central HTTP status → (exception_class, reason_code, http_status).
Every HTTP 403 becomes authorization-class. Body text is used only as a
local hint for scope vs denied reason_code and is never returned.
"""
if code == 401:
return (GiteaAuthError, "auth_invalid_token", 401)
if code == 403:
if _looks_like_insufficient_scope(body_hint or ""):
return (GiteaAuthzError, "authz_insufficient_scope", 403)
return (GiteaAuthzError, "authz_denied", 403)
if code in (502, 503, 504):
return (GiteaHttpError, "upstream_unavailable", code)
return (GiteaHttpError, "http_error", code)
def raise_for_http_status(code: int, body: str = "") -> None:
"""Raise a typed client error for *code* without embedding *body*.
*body* may be inspected only to choose scope vs denied for 403; it is
never placed on the exception message.
"""
# Redact before any inspection; discard after classification.
try:
hint = _redact(body or "").strip()
except Exception:
hint = ""
exc_cls, reason, status = classify_http_status(code, body_hint=hint)
# Explicitly construct without passing body/hint into message.
if exc_cls is GiteaAuthError:
raise GiteaAuthError(reason_code=reason, http_status=status)
if exc_cls is GiteaAuthzError:
raise GiteaAuthzError(reason_code=reason, http_status=status)
if reason == "upstream_unavailable":
raise GiteaHttpError(
reason_code="upstream_unavailable",
http_status=status,
)
raise GiteaHttpError(reason_code="http_error", http_status=status)
def _raise_http_error(code: int, detail: str = "") -> None:
"""Backward-compatible alias — *detail* is never embedded in the error."""
raise_for_http_status(code, detail)
def _add_query(url, **params): def _add_query(url, **params):
"""Return *url* with the given query parameters added or overridden. """Return *url* with the given query parameters added or overridden.
@@ -315,23 +501,24 @@ def api_request(method, url, auth_header, payload=None, *,
"""Make an authenticated JSON request to the Gitea API. """Make an authenticated JSON request to the Gitea API.
Returns parsed JSON on success (or ``None`` for an empty body), and raises Returns parsed JSON on success (or ``None`` for an empty body), and raises
``RuntimeError`` on failure. a classified client error on failure.
On HTTP 429 the request is retried up to *max_retries* times: honoring a On HTTP 429 the request is retried up to *max_retries* times: honoring a
valid ``Retry-After`` header (seconds or HTTP-date) when present, otherwise valid ``Retry-After`` header (seconds or HTTP-date) when present, otherwise
using capped jittered exponential backoff. Successful responses are using capped jittered exponential backoff. Successful responses are
unchanged. unchanged.
All failures are converted to a ``RuntimeError`` with a clear, secret Failures raise typed exceptions with **fixed messages only** (#699). HTTP
-redacted message (no raw stack traces or credential material): response bodies are read solely for local 403 reason refinement and are
never stored on exceptions or returned to callers:
- Non-429 HTTP errors surface the status code and a redacted response body. - HTTP 401 → :class:`GiteaAuthError` (``auth_invalid_token``)
502/503/504 upstream errors get an explicit "Gitea upstream unavailable" - HTTP 403 → :class:`GiteaAuthzError` (scope or denied)
message. - 502/503/504 → :class:`GiteaHttpError` (``upstream_unavailable``)
- Timeouts and network/DNS failures (``URLError`` / ``TimeoutError``) surface - Other non-429 HTTP → :class:`GiteaHttpError` (``http_error``)
a generic "network error contacting Gitea" message. - Timeouts / DNS / ``URLError`` → :class:`GiteaNetworkError`
- A malformed (non-JSON) success body surfaces a "malformed JSON response" - Malformed success JSON → plain ``RuntimeError`` (programming/protocol;
message rather than a raw decode error. not reclassified as authentication)
The ``*_func`` parameters and ``timeout`` are injection points for The ``*_func`` parameters and ``timeout`` are injection points for
deterministic testing. deterministic testing.
@@ -369,22 +556,23 @@ def api_request(method, url, auth_header, payload=None, *,
error_body = e.read().decode("utf-8", errors="replace") error_body = e.read().decode("utf-8", errors="replace")
except Exception: except Exception:
error_body = "" error_body = ""
detail = _redact(error_body).strip() # Classify from status (+ local body hint). Body is not embedded.
if e.code in (502, 503, 504): try:
msg = f"HTTP {e.code}: Gitea upstream unavailable" raise_for_http_status(e.code, error_body)
raise RuntimeError(f"{msg}: {detail}" if detail else msg) from e except GiteaClientError:
raise RuntimeError(f"HTTP {e.code}: {detail}") from e raise
# Defensive: raise_for_http_status always raises.
raise GiteaHttpError(http_status=e.code) from e # pragma: no cover
except (urllib.error.URLError, TimeoutError) as e: except (urllib.error.URLError, TimeoutError) as e:
reason = getattr(e, "reason", e) # Fixed message only — do not embed URLError reason (may leak paths).
raise RuntimeError( raise GiteaNetworkError(reason_code="network_error") from e
f"network error contacting Gitea: {_redact(reason)}"
) from e
if not body: if not body:
return None return None
try: try:
return json.loads(body) return json.loads(body)
except ValueError as e: except ValueError as e:
# Programming/protocol failure — not authentication.
raise RuntimeError("malformed JSON response from Gitea") from e raise RuntimeError("malformed JSON response from Gitea") from e
+298 -80
View File
@@ -1084,7 +1084,9 @@ from gitea_auth import ( # noqa: E402
repo_api_url, repo_api_url,
get_profile, get_profile,
gitea_url, gitea_url,
GiteaConfigError,
) )
import mcp_tool_error_boundary # noqa: E402
import gitea_audit # noqa: E402 import gitea_audit # noqa: E402
import gitea_config # noqa: E402 import gitea_config # noqa: E402
import capability_stop_terminal # noqa: E402 import capability_stop_terminal # noqa: E402
@@ -1464,6 +1466,12 @@ def _with_optional_url(result: dict, url: str | None) -> dict:
result["url"] = url result["url"] = url
return result return result
# #699: known auth/authz/network/config failures → structured CallToolResult
# isError; stdio transport must survive (no unhandled raise / process exit).
from mcp.server.fastmcp.tools.base import Tool as _FastMCPTool # noqa: E402
mcp_tool_error_boundary.install_tool_run_boundary(_FastMCPTool)
mcp = FastMCP("gitea-tools", instructions=( mcp = FastMCP("gitea-tools", instructions=(
"Gitea issue tracker and PR management for dadeschools and prgs instances. " "Gitea issue tracker and PR management for dadeschools and prgs instances. "
"Use the gitea_ prefixed tools to create issues, PRs, list issues, etc." "Use the gitea_ prefixed tools to create issues, PRs, list issues, etc."
@@ -1801,13 +1809,16 @@ def _enforce_remote_repo_guard(
def _auth(host: str) -> str: def _auth(host: str) -> str:
"""Get auth header, raise if unavailable.""" """Get auth header, raise if unavailable.
Missing credentials are a configuration failure, not a silent internal
crash. Typed as :class:`gitea_auth.GiteaConfigError` so the tool-error
boundary (#699) maps them to a structured isError result without EOF.
The exception message is a fixed constant (no host/token material).
"""
header = get_auth_header(host) header = get_auth_header(host)
if header is None: if header is None:
raise RuntimeError( raise GiteaConfigError(reason_code="config_error")
f"No credentials for {host}. "
"Ensure you've logged in via HTTPS at least once."
)
return header return header
@@ -5925,65 +5936,6 @@ def gitea_delete_branch(
"permission_report": _permission_block_report("gitea.branch.delete"), "permission_report": _permission_block_report("gitea.branch.delete"),
} }
# Possessing gitea.branch.delete alone is not enough for arbitrary deletion.
# task_capability_map maps delete_branch → author; reconciler must use the
# guarded cleanup_merged_pr_branch path only (#687 / #514).
profile = get_profile()
active_role = _profile_role_kind(profile)
required_role = task_capability_map.required_role("delete_branch")
if active_role == "reconciler":
return {
"success": False,
"performed": False,
"required_permission": "gitea.branch.delete",
"required_role_kind": required_role,
"active_role_kind": active_role,
"reasons": [
"reconciler profile cannot use raw gitea_delete_branch; "
"use gitea_cleanup_merged_pr_branch for a fully merged PR "
"source branch only (fail closed)"
],
"exact_next_action": (
"Call gitea_cleanup_merged_pr_branch with pr_number, the "
"exact PR head branch, and confirmation "
"'CLEANUP MERGED PR <n> BRANCH <branch>' after capability "
"resolve for cleanup_merged_pr_branch."
),
"permission_report": _permission_block_report("gitea.branch.delete"),
}
if active_role != required_role:
return {
"success": False,
"performed": False,
"required_permission": "gitea.branch.delete",
"required_role_kind": required_role,
"active_role_kind": active_role,
"reasons": [
f"Active profile role '{active_role}' cannot perform "
f"{required_role} task 'delete_branch' even when "
"gitea.branch.delete is present (fail closed)"
],
"permission_report": _permission_block_report("gitea.branch.delete"),
}
if branch_cleanup_guard.is_preservation_or_evidence_branch(branch):
return {
"success": False,
"performed": False,
"required_permission": "gitea.branch.delete",
"reasons": [
f"branch '{branch}' is a preservation/evidence branch and "
"cannot be deleted (fail closed)"
],
}
if branch in branch_cleanup_guard.PROTECTED_BRANCHES:
return {
"success": False,
"performed": False,
"required_permission": "gitea.branch.delete",
"reasons": [f"branch '{branch}' is protected (fail closed)"],
}
audit_allowed, audit_reasons = ( audit_allowed, audit_reasons = (
audit_reconciliation_mode.check_audit_mutation_allowed("delete_branch") audit_reconciliation_mode.check_audit_mutation_allowed("delete_branch")
) )
@@ -6035,20 +5987,18 @@ def gitea_cleanup_merged_pr_branch(
} }
profile = get_profile() profile = get_profile()
active_role = _profile_role_kind(profile) active_role = _role_kind(
# cleanup_merged_pr_branch is reconciler-owned (task_capability_map). profile.get("allowed_operations", []),
# Author/reviewer/merger must not reach this path even if they somehow profile.get("forbidden_operations", []),
# hold gitea.branch.delete. )
if active_role != "reconciler": if active_role == "reviewer":
return { return {
"success": False, "success": False,
"performed": False, "performed": False,
"required_permission": "gitea.branch.delete", "required_permission": "gitea.branch.delete",
"required_role_kind": "reconciler",
"active_role_kind": active_role,
"reasons": [ "reasons": [
f"profile role '{active_role}' is not authorized for merged " "reviewer profile is not authorized for merged branch cleanup "
"branch cleanup; required role is reconciler (fail closed)" "(fail closed)"
], ],
"permission_report": _permission_block_report("gitea.branch.delete"), "permission_report": _permission_block_report("gitea.branch.delete"),
} }
@@ -8025,11 +7975,12 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
org: str | None = None, org: str | None = None,
repo: str | None = None, repo: str | None = None,
) -> dict: ) -> dict:
"""Read-only: diagnose open-PR reviewer lease handoff and emit next action (#599). """Read-only: diagnose open-PR reviewer lease handoff and emit next action (#599, #691).
Classifies no-lease / own / foreign / instructed-lease-missing-with-replacement Classifies no-lease / own / foreign / superseded-head / expired / orphan /
and worktree binding mismatch. Returns a canonical ``next_action`` without instructed-lease-missing-with-replacement and worktree binding mismatch.
mutating Gitea state. Never steals foreign leases. Returns a canonical ``next_action`` (including guarded obsolete-lease cleanup)
without mutating Gitea state. Never steals foreign leases.
""" """
read_block = _profile_operation_gate("gitea.read") read_block = _profile_operation_gate("gitea.read")
if read_block: if read_block:
@@ -8040,7 +7991,7 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
} }
comments = _fetch_pr_comments( comments = _fetch_pr_comments(
pr_number, remote=remote, host=host, org=org, repo=repo) pr_number, remote=remote, host=host, org=org, repo=repo)
h, _o, _r = _resolve(remote, host, org, repo) h, o, r = _resolve(remote, host, org, repo)
try: try:
username = _authenticated_username(h) username = _authenticated_username(h)
except Exception: except Exception:
@@ -8054,6 +8005,64 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
# Prefer in-session lease id when present; otherwise diagnose as a fresh # Prefer in-session lease id when present; otherwise diagnose as a fresh
# caller without inventing ownership of an on-thread lease. # caller without inventing ownership of an on-thread lease.
session_id = (session_lease.get("session_id") or "").strip() or None session_id = (session_lease.get("session_id") or "").strip() or None
# Live PR head + formal reviews for #691 superseded/completed classification.
current_head_sha = None
formal_reviews: list[dict] = []
try:
auth = _auth(h)
pr_live = api_request(
"GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth
) or {}
head = pr_live.get("head") or {}
current_head_sha = head.get("sha") or pr_live.get("head_commit_sha")
try:
reviews = api_request(
"GET",
f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews",
auth,
) or []
for rev in reviews if isinstance(reviews, list) else []:
formal_reviews.append({
"verdict": rev.get("state") or rev.get("verdict"),
"reviewed_head_sha": rev.get("commit_id") or rev.get(
"reviewed_head_sha"
),
"dismissed": bool(rev.get("dismissed")),
"reviewer": ((rev.get("user") or {}).get("login")),
"submitted_at": rev.get("submitted_at"),
})
except Exception:
formal_reviews = []
except Exception:
current_head_sha = None
lease_wt = None
active = reviewer_pr_lease.find_newest_nonterminal_lease(
comments, pr_number=pr_number, include_expired=True
)
if active:
lease_wt = (active.get("worktree") or "").strip() or None
worktree_exists = None
worktree_clean = None
if lease_wt:
worktree_exists = os.path.isdir(lease_wt)
if worktree_exists:
try:
import subprocess
st = subprocess.run(
["git", "-C", lease_wt, "status", "--porcelain"],
capture_output=True,
text=True,
timeout=30,
check=False,
)
if st.returncode == 0:
worktree_clean = not bool((st.stdout or "").strip())
except Exception:
worktree_clean = None
diagnosis = reviewer_pr_lease.diagnose_reviewer_pr_lease_handoff( diagnosis = reviewer_pr_lease.diagnose_reviewer_pr_lease_handoff(
comments, comments,
pr_number=pr_number, pr_number=pr_number,
@@ -8063,6 +8072,11 @@ def gitea_diagnose_reviewer_pr_lease_handoff(
env_bound_worktree=env_wt, env_bound_worktree=env_wt,
instructed_session_id=instructed_session_id, instructed_session_id=instructed_session_id,
instructed_comment_id=instructed_comment_id, instructed_comment_id=instructed_comment_id,
current_head_sha=current_head_sha,
formal_reviews=formal_reviews,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
owner_process_alive=None, # PID never proves ownership; leave unset
) )
diagnosis["success"] = True diagnosis["success"] = True
diagnosis["remote"] = remote diagnosis["remote"] = remote
@@ -8190,6 +8204,212 @@ def gitea_cleanup_post_merge_moot_lease(
return report return report
@mcp.tool()
def gitea_cleanup_obsolete_reviewer_comment_lease(
pr_number: int,
confirmation: str = "",
apply: bool = False,
controller_recovery_authorized: bool = False,
expected_lease_comment_id: int | None = None,
expected_session_id: str | None = None,
expected_leased_head: str | None = None,
owner_process_alive: bool | None = None,
current_head_review_in_progress: bool = False,
remote: str = "dadeschools",
host: str | None = None,
org: str | None = None,
repo: str | None = None,
) -> dict:
"""Guarded cleanup for obsolete comment-backed reviewer leases on open PRs (#691).
Neutralises an expired and/or superseded-head foreign lease by posting a
terminal ``phase: released`` lease marker (append-only audit). Never deletes
comments, never repoints the lease to the new head, never transfers
validation/decision/workflow state, never uses PID equality as ownership,
and never steals a genuinely active foreign lease on the current head.
Read-first when ``apply`` is false. Apply requires:
- ``controller_recovery_authorized=True`` (explicit operator/controller
recovery capability not implied by profile alone)
- ``confirmation`` exactly ``CLEANUP OBSOLETE REVIEWER LEASE <pr_number>``
- full eligibility evidence from
``reviewer_pr_lease.assess_obsolete_reviewer_comment_lease_cleanup``
Comment-backed leases need no control-plane DB ``lease_id``.
"""
read_block = _profile_operation_gate("gitea.read")
if read_block:
return {
"success": False,
"cleanup_performed": False,
"no_steal_or_adoption": True,
"reasons": read_block,
"permission_report": _permission_block_report("gitea.read"),
}
h, o, r = _resolve(remote, host, org, repo)
auth = _auth(h)
expected_repo = f"{o}/{r}"
comments = _fetch_pr_comments(
pr_number, remote=remote, host=host, org=org, repo=repo
)
pr_live = api_request(
"GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth
) or {}
head = pr_live.get("head") or {}
current_head_sha = head.get("sha") or pr_live.get("head_commit_sha")
formal_reviews: list[dict] = []
try:
reviews = api_request(
"GET",
f"{repo_api_url(h, o, r)}/pulls/{pr_number}/reviews",
auth,
) or []
for rev in reviews if isinstance(reviews, list) else []:
formal_reviews.append({
"verdict": rev.get("state") or rev.get("verdict"),
"reviewed_head_sha": rev.get("commit_id")
or rev.get("reviewed_head_sha"),
"dismissed": bool(rev.get("dismissed")),
"reviewer": ((rev.get("user") or {}).get("login")),
"submitted_at": rev.get("submitted_at"),
})
except Exception:
formal_reviews = []
lease = reviewer_pr_lease.find_newest_nonterminal_lease(
comments, pr_number=pr_number, include_expired=True
)
lease_wt = ((lease or {}).get("worktree") or "").strip() or None
worktree_exists = None
worktree_clean = None
worktree_has_unpreserved_work = None
if lease_wt:
worktree_exists = os.path.isdir(lease_wt)
if worktree_exists:
try:
import subprocess
st = subprocess.run(
["git", "-C", lease_wt, "status", "--porcelain"],
capture_output=True,
text=True,
timeout=30,
check=False,
)
if st.returncode == 0:
dirty = bool((st.stdout or "").strip())
worktree_clean = not dirty
worktree_has_unpreserved_work = dirty
except Exception:
worktree_clean = None
session = reviewer_pr_lease.get_session_lease() or {}
assessment = reviewer_pr_lease.assess_obsolete_reviewer_comment_lease_cleanup(
comments,
pr_number=pr_number,
current_head_sha=current_head_sha,
formal_reviews=formal_reviews,
repo=expected_repo,
expected_repo=expected_repo,
requesting_session_id=(session.get("session_id") or "").strip() or None,
controller_recovery_authorized=controller_recovery_authorized,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
worktree_has_unpreserved_work=worktree_has_unpreserved_work,
owner_process_alive=owner_process_alive,
owner_pid_observed=None,
requesting_pid=os.getpid(),
current_head_review_in_progress=current_head_review_in_progress,
expected_lease_comment_id=expected_lease_comment_id,
expected_session_id=expected_session_id,
expected_leased_head=expected_leased_head,
confirmation=confirmation,
apply=apply,
)
report = {
"success": True,
"pr_number": pr_number,
"pr_state": pr_live.get("state"),
"cleanup_allowed": assessment.get("cleanup_allowed"),
"cleanup_performed": False,
"classification": assessment.get("classification"),
"blocker_kind": assessment.get("blocker_kind"),
"exact_next_action": assessment.get("exact_next_action"),
"mutation_eligibility": assessment.get("mutation_eligibility"),
"leased_head": assessment.get("leased_head"),
"current_head": assessment.get("current_head"),
"expires_at": assessment.get("expires_at"),
"terminal_review": assessment.get("terminal_review"),
"worktree_state": assessment.get("worktree_state"),
"owner_session_evidence": assessment.get("owner_session_evidence"),
"cleanup_tool": assessment.get("cleanup_tool"),
"required_confirmation": assessment.get("required_confirmation"),
"active_lease": assessment.get("active_lease"),
"no_steal_or_adoption": True,
"no_repoint": True,
"no_validation_transfer": True,
"mode": "apply" if apply else "read_only",
"reasons": assessment.get("reasons") or [],
"forbidden": assessment.get("forbidden") or [],
}
if not apply:
return report
if not assessment.get("cleanup_allowed"):
report["success"] = False
report["cleanup_skipped_reason"] = (
assessment.get("fail_closed_reasons")
or assessment.get("reasons")
or ["cleanup not allowed"]
)
return report
comment_block = _profile_operation_gate("gitea.pr.comment")
if comment_block:
report["success"] = False
report["reasons"] = comment_block
report["permission_report"] = _permission_block_report("gitea.pr.comment")
return report
verify_preflight_purity(remote)
# Terminal lease marker (preserves history; does not delete comment 10749-style
# entries — newest released marker neutralises the ledger).
release_body = assessment.get("release_body") or ""
audit_body = assessment.get("audit_comment_body") or ""
comment_url = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments"
with _audited(
"comment_pr",
host=h,
remote=remote,
org=o,
repo=r,
pr_number=pr_number,
request_metadata={
"source": "cleanup_obsolete_reviewer_comment_lease",
"classification": assessment.get("classification"),
},
):
posted_release = api_request(
"POST", comment_url, auth, {"body": release_body}
)
posted_audit = None
if audit_body.strip():
posted_audit = api_request(
"POST", comment_url, auth, {"body": audit_body}
)
# Never seed session lease from cleaned foreign lease.
report["cleanup_performed"] = True
report["released_comment_id"] = (posted_release or {}).get("id")
report["audit_comment_id"] = (posted_audit or {}).get("id")
report["session_lease_seeded"] = False
return report
@mcp.tool() @mcp.tool()
def gitea_release_reviewer_pr_lease( def gitea_release_reviewer_pr_lease(
pr_number: int, pr_number: int,
@@ -11330,8 +11550,6 @@ def gitea_resolve_task_capability(
"gitea_commit_files", "gitea_commit_files",
"address_pr_change_requests", "address_pr_change_requests",
"delete_branch", "delete_branch",
"cleanup_merged_pr_branch",
"reconciliation_cleanup",
"work_issue", "work_issue",
"work-issue", "work-issue",
} }
+1
View File
@@ -13,6 +13,7 @@ from typing import Any
AUTHORIZED_CLEANUP_TOOLS = frozenset({ AUTHORIZED_CLEANUP_TOOLS = frozenset({
"gitea_cleanup_post_merge_moot_lease", "gitea_cleanup_post_merge_moot_lease",
"gitea_cleanup_obsolete_reviewer_comment_lease",
"gitea_reconcile_merged_cleanups", "gitea_reconcile_merged_cleanups",
"gitea_delete_branch", "gitea_delete_branch",
"gitea_cleanup_merged_pr_branch", "gitea_cleanup_merged_pr_branch",
+451
View File
@@ -0,0 +1,451 @@
"""MCP tool-boundary error mapping for known Gitea client failures (#699).
Known authentication / authorization / network / configuration failures leave
the tool boundary as a sanitized structured ``CallToolResult`` with
``isError=True``. Stdio transport remains connected.
Design constraints (reviewer-ratified, #699 / PR #701):
1. **No secret material** in tool results or daemon logs. Messages are fixed
constants keyed by ``reason_code``; HTTP bodies / exception text never
surface. Sanitization failure fails closed to ``internal_error``.
2. **Narrow boundary** wraps the original FastMCP ``Tool.run`` success path;
re-raises framework control-flow exceptions (``UrlElicitationRequiredError``);
maps only typed client failures. Installation is idempotent.
3. **No RuntimeError substring heuristics.** Only explicit typed
authentication / authorization / network / configuration exceptions
receive those labels.
4. **HTTP classification** lives in ``gitea_auth.classify_http_status``;
every 403 is authorization-class.
"""
from __future__ import annotations
import json
import logging
import sys
from typing import Any
logger = logging.getLogger("gitea_mcp.tool_error_boundary")
# Stable reason codes (#699).
REASON_AUTH_FAILED = "auth_failed"
REASON_AUTH_INVALID_TOKEN = "auth_invalid_token"
REASON_AUTHZ_INSUFFICIENT_SCOPE = "authz_insufficient_scope"
REASON_AUTHZ_DENIED = "authz_denied"
REASON_NETWORK_ERROR = "network_error"
REASON_CONFIG_ERROR = "config_error"
REASON_INTERNAL_ERROR = "internal_error"
REASON_UPSTREAM_UNAVAILABLE = "upstream_unavailable"
REASON_HTTP_ERROR = "http_error"
ERROR_CLASS_AUTHENTICATION = "authentication"
ERROR_CLASS_AUTHORIZATION = "authorization"
ERROR_CLASS_NETWORK = "network"
ERROR_CLASS_CONFIGURATION = "configuration"
ERROR_CLASS_INTERNAL = "internal"
ERROR_CLASS_UPSTREAM = "upstream"
# Fixed, secret-free operator messages. Never interpolate HTTP bodies,
# Keychain contents, tokens, or arbitrary exception text.
FIXED_MESSAGES: dict[str, str] = {
REASON_AUTH_FAILED: "Gitea authentication failed",
REASON_AUTH_INVALID_TOKEN: (
"Gitea authentication failed: invalid or revoked credentials"
),
REASON_AUTHZ_INSUFFICIENT_SCOPE: (
"Gitea authorization failed: insufficient token scope"
),
REASON_AUTHZ_DENIED: "Gitea authorization failed: access denied",
REASON_NETWORK_ERROR: "Network error contacting Gitea",
REASON_CONFIG_ERROR: "Gitea configuration or credential resolution failed",
REASON_INTERNAL_ERROR: "Internal tool error",
REASON_UPSTREAM_UNAVAILABLE: "Gitea upstream unavailable",
REASON_HTTP_ERROR: "Gitea HTTP request failed",
}
_INSTALL_FLAG = "_gitea_auth_boundary_installed"
_ORIGINAL_ATTR = "_gitea_auth_boundary_original"
def fixed_message(reason_code: str) -> str:
"""Return the fixed sanitized message for *reason_code* (fail closed)."""
return FIXED_MESSAGES.get(reason_code, FIXED_MESSAGES[REASON_INTERNAL_ERROR])
def _safe_profile_name() -> str | None:
try:
from gitea_auth import get_profile
name = (get_profile() or {}).get("profile_name")
if name is None:
return None
text = str(name).strip()
# Profile names are non-secret identifiers; still bound length.
return text[:80] if text else None
except Exception:
return None
def _is_framework_control_flow(exc: BaseException) -> bool:
"""True for exceptions the MCP framework must re-raise unchanged."""
try:
from mcp.shared.exceptions import UrlElicitationRequiredError
if isinstance(exc, UrlElicitationRequiredError):
return True
except Exception:
pass
# BaseException subclasses that must never become tool isError payloads.
if isinstance(exc, (KeyboardInterrupt, SystemExit, GeneratorExit)):
return True
return False
def is_known_client_failure(exc: BaseException) -> bool:
"""True only for explicit typed Gitea client / config failures.
Never true for arbitrary ``RuntimeError`` (reviewer finding #3).
"""
try:
import gitea_auth
if isinstance(
exc,
(
gitea_auth.GiteaAuthError,
gitea_auth.GiteaAuthzError,
gitea_auth.GiteaNetworkError,
gitea_auth.GiteaConfigError,
gitea_auth.GiteaHttpError,
),
):
return True
except Exception:
return False
try:
import gitea_config
if isinstance(exc, gitea_config.ConfigError):
return True
except Exception:
pass
return False
def classify_exception(exc: BaseException) -> dict[str, Any]:
"""Return structured classification with **fixed** messages only.
Only typed authentication / authorization / network / configuration
failures receive those labels. Unexpected programming failures are
``internal_error``. HTTP bodies and exception text are never copied
into ``message``.
"""
try:
return _classify_exception_impl(exc)
except Exception:
# Fail closed: sanitization / classification failure must not leak.
return {
"reason_code": REASON_INTERNAL_ERROR,
"error_class": ERROR_CLASS_INTERNAL,
"http_status": None,
"message": fixed_message(REASON_INTERNAL_ERROR),
"transport_survives": True,
}
def _classify_exception_impl(exc: BaseException) -> dict[str, Any]:
import gitea_auth
if isinstance(exc, gitea_auth.GiteaAuthError):
code = getattr(exc, "reason_code", None) or REASON_AUTH_INVALID_TOKEN
if code not in (
REASON_AUTH_FAILED,
REASON_AUTH_INVALID_TOKEN,
):
code = REASON_AUTH_INVALID_TOKEN
return {
"reason_code": code,
"error_class": ERROR_CLASS_AUTHENTICATION,
"http_status": getattr(exc, "http_status", None) or 401,
"message": fixed_message(code),
"transport_survives": True,
}
if isinstance(exc, gitea_auth.GiteaAuthzError):
code = getattr(exc, "reason_code", None) or REASON_AUTHZ_DENIED
if code not in (REASON_AUTHZ_DENIED, REASON_AUTHZ_INSUFFICIENT_SCOPE):
code = REASON_AUTHZ_DENIED
return {
"reason_code": code,
"error_class": ERROR_CLASS_AUTHORIZATION,
"http_status": getattr(exc, "http_status", None) or 403,
"message": fixed_message(code),
"transport_survives": True,
}
if isinstance(exc, gitea_auth.GiteaNetworkError):
return {
"reason_code": REASON_NETWORK_ERROR,
"error_class": ERROR_CLASS_NETWORK,
"http_status": getattr(exc, "http_status", None),
"message": fixed_message(REASON_NETWORK_ERROR),
"transport_survives": True,
}
if isinstance(exc, gitea_auth.GiteaConfigError):
return {
"reason_code": REASON_CONFIG_ERROR,
"error_class": ERROR_CLASS_CONFIGURATION,
"http_status": getattr(exc, "http_status", None),
"message": fixed_message(REASON_CONFIG_ERROR),
"transport_survives": True,
}
if isinstance(exc, gitea_auth.GiteaHttpError):
code = getattr(exc, "reason_code", None) or REASON_HTTP_ERROR
if code == REASON_UPSTREAM_UNAVAILABLE:
error_class = ERROR_CLASS_UPSTREAM
else:
error_class = ERROR_CLASS_INTERNAL
code = REASON_HTTP_ERROR
return {
"reason_code": code,
"error_class": error_class,
"http_status": getattr(exc, "http_status", None),
"message": fixed_message(code),
"transport_survives": True,
}
try:
import gitea_config
if isinstance(exc, gitea_config.ConfigError):
return {
"reason_code": REASON_CONFIG_ERROR,
"error_class": ERROR_CLASS_CONFIGURATION,
"http_status": None,
"message": fixed_message(REASON_CONFIG_ERROR),
"transport_survives": True,
}
except Exception:
pass
# Unwrap FastMCP ToolError cause when the original was a typed failure.
cause = getattr(exc, "__cause__", None)
if cause is not None and cause is not exc and is_known_client_failure(cause):
return _classify_exception_impl(cause)
# No message-substring authentication heuristics (reviewer finding #3).
return {
"reason_code": REASON_INTERNAL_ERROR,
"error_class": ERROR_CLASS_INTERNAL,
"http_status": None,
"message": fixed_message(REASON_INTERNAL_ERROR),
"transport_survives": True,
}
def build_structured_error_payload(
classification: dict[str, Any],
*,
tool_name: str | None = None,
profile_name: str | None = None,
) -> dict[str, Any]:
"""LLM-safe structured payload — fixed message + typed metadata only."""
try:
reason = str(classification.get("reason_code") or REASON_INTERNAL_ERROR)
message = fixed_message(reason)
# Refuse to emit any classification message that is not the fixed constant.
if classification.get("message") != message:
message = fixed_message(reason)
payload: dict[str, Any] = {
"success": False,
"isError": True,
"reason_code": reason if reason in FIXED_MESSAGES else REASON_INTERNAL_ERROR,
"error_class": classification.get("error_class") or ERROR_CLASS_INTERNAL,
"message": message,
"transport_survives": True,
"retryable": classification.get("error_class")
in {
ERROR_CLASS_AUTHENTICATION,
ERROR_CLASS_NETWORK,
ERROR_CLASS_CONFIGURATION,
},
}
status = classification.get("http_status")
if isinstance(status, int):
payload["http_status"] = status
if tool_name and isinstance(tool_name, str):
# Tool names are identifiers, not secrets; bound length.
payload["tool"] = tool_name[:120]
if profile_name and isinstance(profile_name, str):
payload["profile"] = profile_name[:80]
return payload
except Exception:
return {
"success": False,
"isError": True,
"reason_code": REASON_INTERNAL_ERROR,
"error_class": ERROR_CLASS_INTERNAL,
"message": fixed_message(REASON_INTERNAL_ERROR),
"transport_survives": True,
"retryable": False,
}
def log_sanitized_daemon_reason(
classification: dict[str, Any],
*,
tool_name: str | None = None,
stream=None,
) -> None:
"""Daemon log: reason codes only — never exception text or response bodies."""
stream = stream if stream is not None else sys.stderr
try:
reason = classification.get("reason_code") or REASON_INTERNAL_ERROR
error_class = classification.get("error_class") or ERROR_CLASS_INTERNAL
# Only emit known tokens; never classification['message'] from callers
# that might have been poisoned.
if reason not in FIXED_MESSAGES:
reason = REASON_INTERNAL_ERROR
error_class = ERROR_CLASS_INTERNAL
parts = [
"mcp_tool_error",
f"reason_code={reason}",
f"error_class={error_class}",
]
if tool_name and isinstance(tool_name, str):
parts.append(f"tool={tool_name[:120]}")
status = classification.get("http_status")
if isinstance(status, int):
parts.append(f"http_status={status}")
# Intentionally no detail= / message= field — secrets lived there.
line = " ".join(parts)
stream.write(line + "\n")
if hasattr(stream, "flush"):
stream.flush()
logger.warning(line)
except Exception:
# Fail closed: never fall back to logging the exception.
try:
stream.write(
"mcp_tool_error reason_code=internal_error "
"error_class=internal\n"
)
if hasattr(stream, "flush"):
stream.flush()
except Exception:
pass
def to_call_tool_result(
exc: BaseException,
*,
tool_name: str | None = None,
profile_name: str | None = None,
log: bool = True,
) -> Any:
"""Build a FastMCP ``CallToolResult`` with ``isError=True`` for *exc*."""
from mcp.types import CallToolResult, TextContent
try:
classification = classify_exception(exc)
if log:
log_sanitized_daemon_reason(classification, tool_name=tool_name)
payload = build_structured_error_payload(
classification, tool_name=tool_name, profile_name=profile_name
)
text = json.dumps(payload, indent=2, sort_keys=True)
return CallToolResult(
content=[TextContent(type="text", text=text)],
structuredContent=payload,
isError=True,
)
except Exception:
# Absolute fail-closed path — no exception text.
fallback = {
"success": False,
"isError": True,
"reason_code": REASON_INTERNAL_ERROR,
"error_class": ERROR_CLASS_INTERNAL,
"message": fixed_message(REASON_INTERNAL_ERROR),
"transport_survives": True,
"retryable": False,
}
return CallToolResult(
content=[
TextContent(
type="text",
text=json.dumps(fallback, indent=2, sort_keys=True),
)
],
structuredContent=fallback,
isError=True,
)
def install_tool_run_boundary(Tool) -> bool:
"""Install a **narrow** error boundary around FastMCP ``Tool.run``.
Strategy (preserves framework semantics):
* Call the **original** ``Tool.run`` for the success path (async, return
types, convert_result, protocol behavior unchanged).
* Re-raise ``UrlElicitationRequiredError`` and other control-flow
exceptions without mapping to ``internal_error``.
* Map typed Gitea client failures (and ToolError whose ``__cause__`` is
typed) to structured ``CallToolResult(isError=True)``.
* Map remaining unexpected tool failures to fixed ``internal_error``
isError results (transport survival) without secret-bearing text.
* Idempotent: second install is a no-op and returns ``False``.
"""
if getattr(Tool.run, _INSTALL_FLAG, False):
return False
from mcp.server.fastmcp.exceptions import ToolError
from mcp.shared.exceptions import UrlElicitationRequiredError
original_run = Tool.run
async def run_boundary(
self,
arguments: dict[str, Any],
context=None,
convert_result: bool = False,
) -> Any:
try:
return await original_run(
self,
arguments,
context=context,
convert_result=convert_result,
)
except UrlElicitationRequiredError:
# Framework control-flow — must not become internal_error.
raise
except BaseException as exc:
if _is_framework_control_flow(exc):
raise
if not isinstance(exc, Exception):
raise
# Prefer typed cause under FastMCP ToolError wrappers.
target: BaseException = exc
if isinstance(exc, ToolError) and exc.__cause__ is not None:
target = exc.__cause__
# Known client failures → structured isError with reason codes.
# Unexpected failures → fixed internal_error isError (no secrets).
profile_name = _safe_profile_name()
return to_call_tool_result(
target,
tool_name=getattr(self, "name", None),
profile_name=profile_name,
)
setattr(run_boundary, _INSTALL_FLAG, True)
setattr(run_boundary, _ORIGINAL_ATTR, original_run)
Tool.run = run_boundary # type: ignore[method-assign]
return True
def boundary_is_installed(Tool) -> bool:
"""True when the #699 boundary is active on *Tool.run*."""
return bool(getattr(Tool.run, _INSTALL_FLAG, False))
+7 -126
View File
@@ -20,114 +20,12 @@ if PROJECT_ROOT not in sys.path:
import gitea_config import gitea_config
# Defaults emit *canonical* operation names only. Shorthand that is not in AUTHOR_DEFAULT_ALLOWED = ["read", "branch", "commit", "push", "open_pr", "comment"]
# gitea_config.GITEA_OPERATION_ALIASES (e.g. ``pr.close``, ``issue.close``) AUTHOR_DEFAULT_FORBIDDEN = ["approve", "request_changes", "merge"]
# is silently dropped by the production loader and must never appear here.
AUTHOR_DEFAULT_ALLOWED = [
"gitea.read",
"gitea.branch.create",
"gitea.repo.commit",
"gitea.branch.push",
"gitea.pr.create",
"gitea.pr.comment",
]
AUTHOR_DEFAULT_FORBIDDEN = [
"gitea.pr.approve",
"gitea.pr.request_changes",
"gitea.pr.merge",
]
REVIEWER_DEFAULT_ALLOWED = [ REVIEWER_DEFAULT_ALLOWED = [
"gitea.read", "read", "review", "comment", "approve", "request_changes", "merge"
"gitea.pr.review",
"gitea.pr.comment",
"gitea.pr.approve",
"gitea.pr.request_changes",
"gitea.pr.merge",
] ]
REVIEWER_DEFAULT_FORBIDDEN = [ REVIEWER_DEFAULT_FORBIDDEN = ["branch", "commit", "push", "open_pr"]
"gitea.branch.create",
"gitea.repo.commit",
"gitea.branch.push",
"gitea.pr.create",
]
# Required reconciler ops (read + pr.close) plus recommended comment/close and
# branch.delete for guarded merged-PR cleanup. All names must normalize via
# gitea_config.normalize_operation without being dropped.
RECONCILER_DEFAULT_ALLOWED = [
"gitea.read",
"gitea.pr.close",
"gitea.pr.comment",
"gitea.issue.comment",
"gitea.issue.close",
"gitea.branch.delete",
]
RECONCILER_DEFAULT_FORBIDDEN = [
"gitea.pr.approve",
"gitea.pr.merge",
"gitea.pr.review",
"gitea.pr.create",
"gitea.branch.push",
"gitea.repo.commit",
]
# Migration-only expansions for common shorthands that are *not* in
# GITEA_OPERATION_ALIASES. Emitted output is always the canonical form so a
# second canonicalize pass is a no-op (idempotent).
_MIGRATION_ONLY_ALIASES = {
"pr.close": "gitea.pr.close",
"pr.comment": "gitea.pr.comment",
"issue.close": "gitea.issue.close",
"branch.delete": "gitea.branch.delete",
}
# Reconciler required ops that must survive migration (from reconciler_profile).
RECONCILER_REQUIRED_CANONICAL = ("gitea.read", "gitea.pr.close")
def canonicalize_operation(op: str) -> str:
"""Return a canonical operation name accepted by the production loader.
Fail closed on unknown/ambiguous spellings so required permissions cannot
be silently dropped by ``check_operation`` later.
"""
if not isinstance(op, str) or not op.strip():
raise ValueError("operation must be a non-empty string (fail closed)")
op = op.strip()
try:
return gitea_config.normalize_operation(op)
except gitea_config.ConfigError:
pass
if op in _MIGRATION_ONLY_ALIASES:
return _MIGRATION_ONLY_ALIASES[op]
raise ValueError(
f"operation {op!r} cannot be canonicalized for migration "
"(unknown/ambiguous; fail closed — production loader would drop it)"
)
def canonicalize_operations(ops, *, context: str = "operations") -> list[str]:
"""Canonicalize a list of operations; preserve order, drop duplicates."""
if not isinstance(ops, list):
raise ValueError(f"{context} must be a list (fail closed)")
out: list[str] = []
seen: set[str] = set()
for entry in ops:
canon = canonicalize_operation(entry)
if canon not in seen:
seen.add(canon)
out.append(canon)
return out
def _assert_reconciler_required_survive(allowed: list[str], profile_name: str) -> None:
"""Fail visibly when migration would leave a reconciler without required ops."""
missing = [op for op in RECONCILER_REQUIRED_CANONICAL if op not in set(allowed)]
if missing:
raise ValueError(
f"Profile '{profile_name}' (reconciler) is missing required "
f"operation(s) after migration: {missing}. Refusing to emit a "
"profile that would silently fail pr.close / read (fail closed)."
)
def infer_role(name, execution_profile): def infer_role(name, execution_profile):
@@ -192,11 +90,9 @@ def migrate_v1_to_v2(v1_data):
ident_name = "reviewer" ident_name = "reviewer"
elif role == "author": elif role == "author":
ident_name = "author" ident_name = "author"
elif role == "reconciler":
ident_name = "reconciler"
else: else:
role = prof.get("role") role = prof.get("role")
if role not in (None, "author", "reviewer", "reconciler"): if role not in (None, "author", "reviewer"):
raise ValueError( raise ValueError(
f"Profile '{name}' has unsupported role {role!r}" f"Profile '{name}' has unsupported role {role!r}"
) )
@@ -228,35 +124,20 @@ def migrate_v1_to_v2(v1_data):
raise ValueError( raise ValueError(
f"Profile '{name}' operation fields must be lists" f"Profile '{name}' operation fields must be lists"
) )
try: identity_data["allowed_operations"] = list(allowed)
identity_data["allowed_operations"] = canonicalize_operations( identity_data["forbidden_operations"] = list(forbidden)
allowed, context=f"profile '{name}' allowed_operations"
)
identity_data["forbidden_operations"] = canonicalize_operations(
forbidden, context=f"profile '{name}' forbidden_operations"
)
except ValueError as exc:
raise ValueError(f"Profile '{name}': {exc}") from exc
elif role == "author": elif role == "author":
identity_data["allowed_operations"] = list(AUTHOR_DEFAULT_ALLOWED) identity_data["allowed_operations"] = list(AUTHOR_DEFAULT_ALLOWED)
identity_data["forbidden_operations"] = list(AUTHOR_DEFAULT_FORBIDDEN) identity_data["forbidden_operations"] = list(AUTHOR_DEFAULT_FORBIDDEN)
elif role == "reviewer": elif role == "reviewer":
identity_data["allowed_operations"] = list(REVIEWER_DEFAULT_ALLOWED) identity_data["allowed_operations"] = list(REVIEWER_DEFAULT_ALLOWED)
identity_data["forbidden_operations"] = list(REVIEWER_DEFAULT_FORBIDDEN) identity_data["forbidden_operations"] = list(REVIEWER_DEFAULT_FORBIDDEN)
elif role == "reconciler":
identity_data["allowed_operations"] = list(RECONCILER_DEFAULT_ALLOWED)
identity_data["forbidden_operations"] = list(RECONCILER_DEFAULT_FORBIDDEN)
else: else:
raise ValueError( raise ValueError(
f"Profile '{name}' has no explicit operation lists and no " f"Profile '{name}' has no explicit operation lists and no "
"unambiguous author/reviewer role marker (fail closed)" "unambiguous author/reviewer role marker (fail closed)"
) )
if role == "reconciler":
_assert_reconciler_required_survive(
identity_data["allowed_operations"], name
)
# Nest inside environments/services structure # Nest inside environments/services structure
env = environments.setdefault(env_name, {}) env = environments.setdefault(env_name, {})
services = env.setdefault("services", {}) services = env.setdefault("services", {})
-5
View File
@@ -18,11 +18,6 @@ RECONCILER_RECOMMENDED_OPERATIONS = (
"gitea.pr.comment", "gitea.pr.comment",
"gitea.issue.comment", "gitea.issue.comment",
"gitea.issue.close", "gitea.issue.close",
# Merged-branch cleanup is reconciler-owned (task_capability_map maps
# cleanup_merged_pr_branch -> reconciler). The permission is only
# exercisable through the guarded gitea_cleanup_merged_pr_branch path
# (#514): merged proof, protected-branch refusal, explicit confirmation.
"gitea.branch.delete",
) )
RECONCILER_FORBIDDEN_OPERATIONS = ( RECONCILER_FORBIDDEN_OPERATIONS = (
+657 -43
View File
@@ -524,13 +524,26 @@ def assess_lease_inventory(
"reclaimable_review_leases": reclaimable, "reclaimable_review_leases": reclaimable,
} }
# Canonical next-action vocabulary for reviewer lease handoff (#599). # Canonical next-action vocabulary for reviewer lease handoff (#599, #691).
NEXT_ACTION_ACQUIRE = "acquire" NEXT_ACTION_ACQUIRE = "acquire"
NEXT_ACTION_WAIT = "wait" NEXT_ACTION_WAIT = "wait"
NEXT_ACTION_RESUME_EXACT_OWNER_SESSION = "resume_exact_owner_session" NEXT_ACTION_RESUME_EXACT_OWNER_SESSION = "resume_exact_owner_session"
NEXT_ACTION_RELEASE_EXPIRED_LEASE = "release_expired_lease" NEXT_ACTION_RELEASE_EXPIRED_LEASE = "release_expired_lease"
NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP = "operator_authorized_cleanup" NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP = "operator_authorized_cleanup"
NEXT_ACTION_REPAIR_WORKTREE_BINDING = "repair_worktree_binding" NEXT_ACTION_REPAIR_WORKTREE_BINDING = "repair_worktree_binding"
NEXT_ACTION_CLEANUP_OBSOLETE_LEASE = "cleanup_obsolete_reviewer_comment_lease"
CLEANUP_OBSOLETE_LEASE_TOOL = "gitea_cleanup_obsolete_reviewer_comment_lease"
CLEANUP_OBSOLETE_LEASE_CONFIRMATION_PREFIX = "CLEANUP OBSOLETE REVIEWER LEASE "
# Formal terminal review verdicts that complete a leased-head workflow (#691).
_TERMINAL_REVIEW_VERDICTS = frozenset({
"APPROVED",
"REQUEST_CHANGES",
"approved",
"request_changes",
"REQUESTCHANGES",
})
_HANDOFF_CLASSIFICATIONS = frozenset({ _HANDOFF_CLASSIFICATIONS = frozenset({
"no_lease", "no_lease",
@@ -539,6 +552,12 @@ _HANDOFF_CLASSIFICATIONS = frozenset({
"foreign_active", "foreign_active",
"foreign_reclaimable", "foreign_reclaimable",
"foreign_expired", "foreign_expired",
"foreign_active_current_head",
"foreign_expired_current_head",
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"orphaned_owner_missing",
"ambiguous_conflicting_evidence",
"instructed_lease_missing_with_replacement", "instructed_lease_missing_with_replacement",
"worktree_binding_mismatch", "worktree_binding_mismatch",
}) })
@@ -551,6 +570,476 @@ def _norm_path(value: str | None) -> str:
return os.path.normpath(text.rstrip("/")) return os.path.normpath(text.rstrip("/"))
def _normalize_verdict(value: str | None) -> str:
text = (value or "").strip().upper().replace("-", "_").replace(" ", "_")
if text in {"REQUESTCHANGES", "REQUEST_CHANGES", "CHANGES_REQUESTED"}:
return "REQUEST_CHANGES"
if text in {"APPROVED", "APPROVE"}:
return "APPROVED"
return text
def formal_terminal_review_for_head(
formal_reviews: list[dict] | None,
*,
leased_head: str | None,
) -> dict[str, Any] | None:
"""Return the latest undismissed terminal formal review for *leased_head*."""
head = _normalize_sha(leased_head)
if not head:
return None
matches: list[dict[str, Any]] = []
for review in formal_reviews or []:
if review.get("dismissed"):
continue
verdict = _normalize_verdict(
review.get("verdict") or review.get("state") or review.get("body_state")
)
if verdict not in {"APPROVED", "REQUEST_CHANGES"}:
continue
rhead = _normalize_sha(
review.get("reviewed_head_sha")
or review.get("commit_id")
or review.get("head_sha")
)
if rhead != head:
continue
matches.append({**review, "verdict": verdict, "reviewed_head_sha": rhead})
if not matches:
return None
return matches[-1]
def find_newest_nonterminal_lease(
comments: list[dict],
*,
pr_number: int,
now: datetime | None = None,
include_expired: bool = True,
) -> dict[str, Any] | None:
"""Newest non-terminal lease marker, optionally including expired ones (#691).
Unlike ``find_active_reviewer_lease``, this retains expired non-terminal
markers so diagnosis/cleanup can still name the obsolete lease after
``expires_at`` (comment-backed ledger; no control-plane ``lease_id``).
"""
now = now or datetime.now(timezone.utc)
entries = list(reversed(_lease_entries(comments, pr_number=pr_number)))
for entry in entries:
phase = (entry.get("phase") or "").strip().lower()
if phase in _TERMINAL_PHASES:
# Newest terminal ends the ledger chain for active acquisition, but
# an older non-terminal is not "active". Stop at newest marker.
return None
expired = _lease_expired(entry, now=now)
if expired and not include_expired:
return None
lease = dict(entry)
lease["freshness"] = classify_lease_freshness(lease, now=now)
lease["expired"] = expired
return lease
return None
def cleanup_confirmation_for_pr(pr_number: int) -> str:
return f"{CLEANUP_OBSOLETE_LEASE_CONFIRMATION_PREFIX}{int(pr_number)}"
def assess_obsolete_reviewer_comment_lease_cleanup(
comments: list[dict],
*,
pr_number: int,
current_head_sha: str | None,
formal_reviews: list[dict] | None = None,
repo: str | None = None,
expected_repo: str | None = None,
requesting_session_id: str | None = None,
controller_recovery_authorized: bool = False,
worktree_exists: bool | None = None,
worktree_clean: bool | None = None,
worktree_has_unpreserved_work: bool | None = None,
owner_process_alive: bool | None = None,
owner_pid_observed: int | None = None,
requesting_pid: int | None = None,
current_head_review_in_progress: bool = False,
expected_lease_comment_id: int | None = None,
expected_session_id: str | None = None,
expected_leased_head: str | None = None,
confirmation: str | None = None,
apply: bool = False,
now: datetime | None = None,
) -> dict[str, Any]:
"""Guarded cleanup eligibility for obsolete comment-backed reviewer leases (#691).
Cleanup is never transfer/adoption/repoint and never uses PID equality as
ownership. Eligible only when evidence proves the lease is past expiry and/or
pinned to a superseded head with a completed formal terminal review, and
every safety boundary holds.
"""
now = now or datetime.now(timezone.utc)
reasons: list[str] = []
fail_closed_reasons: list[str] = []
current_head = _normalize_sha(current_head_sha)
req_session = (requesting_session_id or "").strip()
lease = find_newest_nonterminal_lease(
comments, pr_number=pr_number, now=now, include_expired=True
)
if not lease:
# Fall back to active finder (unexpired only) for report consistency.
lease = find_active_reviewer_lease(comments, pr_number=pr_number, now=now)
classification = "no_lease"
cleanup_allowed = False
release_body: str | None = None
audit_comment_body: str | None = None
terminal_review = None
leased_head = None
head_superseded = False
past_expiry = False
expires_parseable = True
if not lease:
fail_closed_reasons.append(
f"PR #{pr_number}: no non-terminal comment-backed reviewer lease to clean"
)
classification = "no_lease"
else:
leased_head = _normalize_sha(lease.get("candidate_head"))
expires_raw = lease.get("expires_at")
expires_at = _parse_timestamp(expires_raw)
if expires_raw and expires_at is None:
expires_parseable = False
fail_closed_reasons.append(
"lease expires_at cannot be parsed or trusted (fail closed)"
)
past_expiry = bool(expires_at and expires_at <= now)
freshness = lease.get("freshness") or classify_lease_freshness(lease, now=now)
owner_session = (lease.get("session_id") or "").strip()
is_requesting_owner = bool(
req_session and owner_session and req_session == owner_session
)
# Identity pins (repo / PR / session / head / comment) must match when
# the caller supplies expected evidence.
lease_repo = (lease.get("repo") or "").strip()
exp_repo = (expected_repo or repo or "").strip()
if exp_repo and lease_repo and exp_repo != lease_repo:
fail_closed_reasons.append(
f"repository identity mismatch: lease repo={lease_repo!r} "
f"expected={exp_repo!r}"
)
if lease.get("pr_number") not in (None, pr_number):
fail_closed_reasons.append(
f"PR identity mismatch: lease pr={lease.get('pr_number')} "
f"requested={pr_number}"
)
if expected_lease_comment_id is not None:
cid = lease.get("comment_id")
if cid is None or int(cid) != int(expected_lease_comment_id):
fail_closed_reasons.append(
"lease comment_id does not match expected evidence "
f"(lease={cid}, expected={expected_lease_comment_id})"
)
if expected_session_id:
if owner_session != expected_session_id.strip():
fail_closed_reasons.append(
"lease session_id does not match expected evidence "
f"(lease={owner_session}, expected={expected_session_id})"
)
if expected_leased_head:
exp_head = _normalize_sha(expected_leased_head)
if not exp_head or exp_head != leased_head:
fail_closed_reasons.append(
"leased head does not match expected evidence "
f"(lease={leased_head}, expected={exp_head})"
)
# PID equality is never ownership proof (#691).
if (
owner_pid_observed is not None
and requesting_pid is not None
and int(owner_pid_observed) == int(requesting_pid)
):
reasons.append(
"observed PID equals requesting PID but PID equality is NOT "
"ownership proof; ignored for authorization"
)
if is_requesting_owner:
fail_closed_reasons.append(
"requesting session owns the lease; use "
"gitea_release_reviewer_pr_lease (owner path), not non-owner cleanup"
)
if current_head_review_in_progress:
fail_closed_reasons.append(
"a current-head review submission is in progress; cleanup denied"
)
if not current_head:
fail_closed_reasons.append(
"current PR head SHA missing or unparseable (fail closed)"
)
if not leased_head:
fail_closed_reasons.append(
"lease candidate_head missing or unparseable (fail closed)"
)
head_superseded = bool(
current_head and leased_head and current_head != leased_head
)
head_matches_current = bool(
current_head and leased_head and current_head == leased_head
)
terminal_review = formal_terminal_review_for_head(
formal_reviews, leased_head=leased_head
)
has_terminal = terminal_review is not None
# Worktree safety.
if worktree_has_unpreserved_work is True or worktree_clean is False:
fail_closed_reasons.append(
"lease worktree is dirty or has unpreserved work; cleanup denied"
)
if (
worktree_exists is True
and worktree_clean is None
and worktree_has_unpreserved_work is None
):
# Unknown cleanliness when path exists — fail closed.
fail_closed_reasons.append(
"lease worktree exists but cleanliness is unknown (fail closed)"
)
# Classification matrix (#691).
if head_matches_current and freshness in {"active", "stale_warning"}:
classification = "foreign_active_current_head"
fail_closed_reasons.append(
"genuinely active foreign lease on the current PR head; "
"cleanup denied (fail closed)"
)
elif head_matches_current and past_expiry:
classification = "foreign_expired_current_head"
# Expired on current head: owner-missing / orphan path may apply.
if owner_process_alive is True:
fail_closed_reasons.append(
"lease expired on current head but owner process still alive "
"with plausible activity; cleanup denied"
)
elif worktree_clean is False:
fail_closed_reasons.append(
"owner process absent but worktree dirty; cleanup denied"
)
elif owner_process_alive is False and (
worktree_clean is True or worktree_exists is False
):
if controller_recovery_authorized:
classification = "orphaned_owner_missing"
else:
fail_closed_reasons.append(
"controller/recovery capability not authorized for orphan cleanup"
)
else:
fail_closed_reasons.append(
"insufficient orphan evidence for expired current-head lease "
"(need owner_process_alive=false and clean/absent worktree)"
)
elif head_superseded and has_terminal and past_expiry:
classification = "foreign_expired_superseded_head"
elif head_superseded and has_terminal and not past_expiry:
classification = "foreign_completed_superseded_head"
elif head_superseded and not has_terminal:
classification = "ambiguous_conflicting_evidence"
fail_closed_reasons.append(
"lease head is superseded but no formal terminal review exists "
"for the leased head (fail closed)"
)
elif not head_superseded and not past_expiry and freshness in {
"active", "stale_warning"
}:
classification = "foreign_active_current_head"
fail_closed_reasons.append(
"foreign lease still active on current head; wait or use owner release"
)
elif past_expiry and not head_superseded:
classification = "foreign_expired_current_head"
else:
classification = "ambiguous_conflicting_evidence"
fail_closed_reasons.append(
"lease evidence is ambiguous; cleanup denied (fail closed)"
)
# Recent owner progress on a non-superseded active lease blocks cleanup.
minutes = _minutes_since_activity(lease, now=now)
if (
head_matches_current
and freshness == "active"
and minutes is not None
and minutes < STALE_WARNING_MINUTES
):
fail_closed_reasons.append(
"owner session has recent authenticated progress on current head; "
"cleanup denied"
)
# Authority + confirmation for apply.
if not controller_recovery_authorized:
if classification in {
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"foreign_expired_current_head",
"orphaned_owner_missing",
}:
fail_closed_reasons.append(
"controller/recovery capability required "
"(controller_recovery_authorized=true)"
)
expected_conf = cleanup_confirmation_for_pr(pr_number)
if apply:
if (confirmation or "").strip() != expected_conf:
fail_closed_reasons.append(
f"confirmation must equal exactly {expected_conf!r}"
)
# Superseded + terminal path does not require past_expiry (AC1).
eligible_class = classification in {
"foreign_completed_superseded_head",
"foreign_expired_superseded_head",
"orphaned_owner_missing",
"foreign_expired_current_head",
}
# foreign_expired_current_head needs orphan/clean worktree + authority.
if classification == "foreign_expired_current_head":
if worktree_clean is not True and worktree_exists is not False:
if "worktree" not in " ".join(fail_closed_reasons):
fail_closed_reasons.append(
"expired current-head cleanup requires proven-clean "
"worktree or absent worktree"
)
if owner_process_alive is True:
fail_closed_reasons.append(
"owner process still alive on expired current-head lease"
)
cleanup_allowed = (
eligible_class
and expires_parseable
and not fail_closed_reasons
and not is_requesting_owner
)
if cleanup_allowed:
release_body = format_lease_body(
repo=lease.get("repo") or exp_repo or "",
pr_number=pr_number,
issue_number=lease.get("issue_number"),
reviewer_identity=lease.get("reviewer_identity") or "",
profile=lease.get("profile") or "unknown",
session_id=owner_session or "unknown",
worktree=lease.get("worktree") or "",
phase="released",
candidate_head=leased_head,
target_branch=lease.get("target_branch") or "master",
target_branch_sha=lease.get("target_branch_sha"),
last_activity=now,
blocker="obsolete-superseded-or-expired-lease",
)
audit_comment_body = (
"## Canonical obsolete reviewer lease cleanup (#691)\n\n"
f"- pr: #{pr_number}\n"
f"- lease_comment_id: {lease.get('comment_id')}\n"
f"- session_id: {owner_session}\n"
f"- leased_head: {leased_head}\n"
f"- current_head: {current_head}\n"
f"- expires_at: {lease.get('expires_at')}\n"
f"- classification: {classification}\n"
f"- terminal_review_verdict: "
f"{(terminal_review or {}).get('verdict')}\n"
f"- tool: {CLEANUP_OBSOLETE_LEASE_TOOL}\n"
"- action: posted terminal phase=released lease marker; "
"did not transfer validation, decision, or workflow proof; "
"did not repoint lease to the new head; did not delete history\n"
)
reasons.append(
f"cleanup eligible ({classification}); post terminal released "
"marker via sanctioned tool"
)
else:
reasons.extend(fail_closed_reasons)
return {
"pr_number": pr_number,
"classification": classification,
"blocker_kind": classification if not cleanup_allowed else "none",
"cleanup_allowed": cleanup_allowed,
"mutation_allowed": False, # never grants review mutation
"mutation_eligibility": "prohibited" if not cleanup_allowed else "cleanup_only",
"exact_next_action": (
NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
if cleanup_allowed
else (
NEXT_ACTION_WAIT
if classification
in {
"foreign_active_current_head",
"ambiguous_conflicting_evidence",
}
else NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP
)
),
"cleanup_tool": CLEANUP_OBSOLETE_LEASE_TOOL,
"required_confirmation": cleanup_confirmation_for_pr(pr_number),
"leased_head": leased_head,
"current_head": current_head,
"head_superseded": head_superseded,
"past_expiry": past_expiry,
"expires_at": (lease or {}).get("expires_at") if lease else None,
"expires_parseable": expires_parseable,
"terminal_review": terminal_review,
"terminal_review_present": terminal_review is not None,
"worktree_state": {
"path": (lease or {}).get("worktree") if lease else None,
"exists": worktree_exists,
"clean": worktree_clean,
"has_unpreserved_work": worktree_has_unpreserved_work,
},
"owner_session_evidence": {
"session_id": (lease or {}).get("session_id") if lease else None,
"reviewer_identity": (
(lease or {}).get("reviewer_identity") if lease else None
),
"process_alive": owner_process_alive,
"pid_observed": owner_pid_observed,
"pid_is_not_ownership_proof": True,
"requesting_session_is_owner": bool(
lease
and req_session
and (lease.get("session_id") or "").strip() == req_session
),
},
"active_lease": lease,
"release_body": release_body,
"audit_comment_body": audit_comment_body,
"controller_recovery_authorized": controller_recovery_authorized,
"reasons": reasons if reasons else fail_closed_reasons,
"fail_closed_reasons": fail_closed_reasons,
"forbidden": [
"manual comment deletion",
"database edits",
"mtime manipulation",
"PID-based ownership",
"direct session-state seeding",
"lease stealing or adoption",
"repointing old lease to new head",
"transfer of validation or decision state",
"worktree reuse by replacement reviewer",
],
}
def diagnose_reviewer_pr_lease_handoff( def diagnose_reviewer_pr_lease_handoff(
comments: list[dict], comments: list[dict],
*, *,
@@ -561,40 +1050,50 @@ def diagnose_reviewer_pr_lease_handoff(
env_bound_worktree: str | None = None, env_bound_worktree: str | None = None,
instructed_session_id: str | None = None, instructed_session_id: str | None = None,
instructed_comment_id: int | None = None, instructed_comment_id: int | None = None,
current_head_sha: str | None = None,
formal_reviews: list[dict] | None = None,
worktree_exists: bool | None = None,
worktree_clean: bool | None = None,
owner_process_alive: bool | None = None,
now: datetime | None = None, now: datetime | None = None,
) -> dict[str, Any]: ) -> dict[str, Any]:
"""Classify open-PR reviewer lease handoff and emit a canonical next action (#599). """Classify open-PR reviewer lease handoff and emit a canonical next action (#599, #691).
Read-only diagnosis. Never steals, releases, or adopts a foreign lease. Read-only diagnosis. Never steals, releases, or adopts a foreign lease.
Fail-closed acquisition rules for active foreign leases remain intact. Fail-closed acquisition rules for active foreign leases remain intact.
Returns a structured diagnosis with: Returns a structured diagnosis with:
- classification - classification (including #691 superseded/expired distinctions)
- next_action (one of wait / resume_exact_owner_session / - next_action (including cleanup_obsolete_reviewer_comment_lease)
release_expired_lease / operator_authorized_cleanup /
repair_worktree_binding / acquire)
- active_lease identity fields when present - active_lease identity fields when present
- worktree_binding match result - worktree_binding match result
- instructed-lease mismatch flags - instructed-lease mismatch flags
- cleanup tool/confirmation when cleanup-eligible
""" """
now = now or datetime.now(timezone.utc) now = now or datetime.now(timezone.utc)
session_id = (current_session_id or "").strip() session_id = (current_session_id or "").strip()
identity = (current_reviewer_identity or "").strip() identity = (current_reviewer_identity or "").strip()
instructed_sid = (instructed_session_id or "").strip() instructed_sid = (instructed_session_id or "").strip()
reasons: list[str] = [] reasons: list[str] = []
current_head = _normalize_sha(current_head_sha)
active = find_active_reviewer_lease(comments, pr_number=pr_number, now=now) active = find_active_reviewer_lease(comments, pr_number=pr_number, now=now)
# Also surface expired non-terminal newest marker for #691 diagnosis.
newest_nt = find_newest_nonterminal_lease(
comments, pr_number=pr_number, now=now, include_expired=True
)
lease_for_class = active or newest_nt
session_lease = get_session_lease() session_lease = get_session_lease()
# Worktree binding: env-bound vs proposed vs active lease worktree. # Worktree binding: env-bound vs proposed vs active lease worktree.
env_wt = _norm_path(env_bound_worktree) env_wt = _norm_path(env_bound_worktree)
prop_wt = _norm_path(proposed_worktree) prop_wt = _norm_path(proposed_worktree)
lease_wt = _norm_path((active or {}).get("worktree") if active else None) lease_wt = _norm_path((lease_for_class or {}).get("worktree") if lease_for_class else None)
binding_mismatch = False binding_mismatch = False
binding_details: dict[str, Any] = { binding_details: dict[str, Any] = {
"env_bound_worktree": env_bound_worktree or None, "env_bound_worktree": env_bound_worktree or None,
"proposed_worktree": proposed_worktree or None, "proposed_worktree": proposed_worktree or None,
"lease_worktree": (active or {}).get("worktree") if active else None, "lease_worktree": (lease_for_class or {}).get("worktree") if lease_for_class else None,
"match": True, "match": True,
} }
paths = [p for p in (env_wt, prop_wt, lease_wt) if p] paths = [p for p in (env_wt, prop_wt, lease_wt) if p]
@@ -608,13 +1107,13 @@ def diagnose_reviewer_pr_lease_handoff(
# Instructed lease gone while a different lease is active (PR #592-style). # Instructed lease gone while a different lease is active (PR #592-style).
instructed_missing_with_replacement = False instructed_missing_with_replacement = False
if instructed_sid or instructed_comment_id is not None: if instructed_sid or instructed_comment_id is not None:
if not active: if not lease_for_class:
reasons.append( reasons.append(
"instructed lease is gone and no active replacement lease remains" "instructed lease is gone and no active replacement lease remains"
) )
else: else:
owner = (active.get("session_id") or "").strip() owner = (lease_for_class.get("session_id") or "").strip()
cid = active.get("comment_id") cid = lease_for_class.get("comment_id")
sid_mismatch = bool(instructed_sid and owner and owner != instructed_sid) sid_mismatch = bool(instructed_sid and owner and owner != instructed_sid)
cid_mismatch = ( cid_mismatch = (
instructed_comment_id is not None instructed_comment_id is not None
@@ -631,35 +1130,94 @@ def diagnose_reviewer_pr_lease_handoff(
# Classification + next_action. # Classification + next_action.
classification = "no_lease" classification = "no_lease"
next_action = NEXT_ACTION_ACQUIRE next_action = NEXT_ACTION_ACQUIRE
cleanup_hint: dict[str, Any] | None = None
if active: if lease_for_class:
owner = (active.get("session_id") or "").strip() owner = (lease_for_class.get("session_id") or "").strip()
freshness = active.get("freshness") or classify_lease_freshness( freshness = lease_for_class.get("freshness") or classify_lease_freshness(
active, now=now lease_for_class, now=now
) )
owner_identity = (active.get("reviewer_identity") or "").strip() owner_identity = (lease_for_class.get("reviewer_identity") or "").strip()
is_own = bool(session_id and owner and owner == session_id) is_own = bool(session_id and owner and owner == session_id)
# Same identity alone is NOT ownership for resume; session_id must match. # Same identity alone is NOT ownership for resume; session_id must match.
same_identity = bool( same_identity = bool(
identity and owner_identity and identity == owner_identity identity and owner_identity and identity == owner_identity
) )
leased_head = _normalize_sha(lease_for_class.get("candidate_head"))
head_superseded = bool(
current_head and leased_head and current_head != leased_head
)
head_current = bool(
current_head and leased_head and current_head == leased_head
)
past_expiry = bool(lease_for_class.get("expired")) or freshness == "expired"
if not past_expiry:
past_expiry = _lease_expired(lease_for_class, now=now)
terminal = formal_terminal_review_for_head(
formal_reviews, leased_head=leased_head
)
if is_own and freshness in {"active", "stale_warning"}: if is_own and freshness in {"active", "stale_warning"} and not past_expiry:
classification = "own_active" classification = "own_active"
next_action = NEXT_ACTION_RESUME_EXACT_OWNER_SESSION next_action = NEXT_ACTION_RESUME_EXACT_OWNER_SESSION
elif is_own and freshness in {"reclaimable", "expired"}: elif is_own and (freshness in {"reclaimable", "expired"} or past_expiry):
classification = "own_expired" classification = "own_expired"
next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE
reasons.append( reasons.append(
f"own lease freshness is '{freshness}'; release via " f"own lease freshness is '{freshness}'; release via "
"gitea_release_reviewer_pr_lease then re-acquire" "gitea_release_reviewer_pr_lease then re-acquire"
) )
elif not is_own and freshness in {"active", "stale_warning"}: elif not is_own and head_superseded and terminal and past_expiry:
classification = "foreign_active" classification = "foreign_expired_superseded_head"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign lease expired and pinned to superseded head with "
"formal terminal review; use guarded obsolete-lease cleanup"
)
elif not is_own and head_superseded and terminal and not past_expiry:
classification = "foreign_completed_superseded_head"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign lease pinned to superseded head with completed formal "
"review; use guarded obsolete-lease cleanup (do not wait indefinitely)"
)
elif not is_own and head_superseded and not terminal:
classification = "ambiguous_conflicting_evidence"
next_action = NEXT_ACTION_WAIT
reasons.append(
"lease head superseded but no formal terminal review for leased "
"head; fail closed / wait (no indefinite steal)"
)
elif not is_own and head_current and past_expiry:
classification = "foreign_expired_current_head"
if owner_process_alive is False and worktree_clean is True:
classification = "orphaned_owner_missing"
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign expired lease on current head; owner process absent "
"and worktree clean — guarded orphan cleanup eligible"
)
elif owner_process_alive is False and worktree_clean is False:
classification = "ambiguous_conflicting_evidence"
next_action = NEXT_ACTION_WAIT
reasons.append(
"owner process absent but worktree dirty; cleanup denied"
)
else:
next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
reasons.append(
"foreign expired lease on current head; use guarded cleanup "
"when worktree/process evidence permits"
)
elif not is_own and freshness in {"active", "stale_warning"} and not past_expiry:
classification = (
"foreign_active_current_head" if head_current or not current_head
else "foreign_active"
)
next_action = NEXT_ACTION_WAIT next_action = NEXT_ACTION_WAIT
reasons.append( reasons.append(
f"foreign active reviewer lease (session_id={owner}, " f"foreign active reviewer lease (session_id={owner}, "
f"phase={active.get('phase')}, freshness={freshness}); " f"phase={lease_for_class.get('phase')}, freshness={freshness}); "
"do not submit; do not steal" "do not submit; do not steal"
) )
if same_identity: if same_identity:
@@ -674,11 +1232,12 @@ def diagnose_reviewer_pr_lease_handoff(
f"foreign reclaimable lease (session_id={owner}); clear only via " f"foreign reclaimable lease (session_id={owner}); clear only via "
"sanctioned gitea_release_reviewer_pr_lease when reclaimable" "sanctioned gitea_release_reviewer_pr_lease when reclaimable"
) )
elif not is_own and freshness == "expired": elif not is_own and (freshness == "expired" or past_expiry):
classification = "foreign_expired" classification = "foreign_expired"
next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE
reasons.append( reasons.append(
f"foreign expired lease (session_id={owner}); use sanctioned release" f"foreign expired lease (session_id={owner}); use sanctioned release "
f"or {CLEANUP_OBSOLETE_LEASE_TOOL}"
) )
else: else:
classification = "foreign_active" classification = "foreign_active"
@@ -691,13 +1250,26 @@ def diagnose_reviewer_pr_lease_handoff(
if instructed_missing_with_replacement and classification.startswith( if instructed_missing_with_replacement and classification.startswith(
"foreign" "foreign"
): ):
classification = "instructed_lease_missing_with_replacement" # Preserve #691 cleanup path when superseded/expired; otherwise
# Foreign active still means wait; reclaimable still release. # keep the PR #592 instructed-missing label.
if next_action == NEXT_ACTION_WAIT: if next_action != NEXT_ACTION_CLEANUP_OBSOLETE_LEASE:
reasons.append( classification = "instructed_lease_missing_with_replacement"
"replacement foreign lease is active — wait; " if next_action == NEXT_ACTION_WAIT:
"operator_authorized_cleanup only with explicit operator authority" reasons.append(
) "replacement foreign lease is active — wait; "
"operator_authorized_cleanup only with explicit operator authority"
)
if next_action == NEXT_ACTION_CLEANUP_OBSOLETE_LEASE:
cleanup_hint = {
"cleanup_tool": CLEANUP_OBSOLETE_LEASE_TOOL,
"required_confirmation": cleanup_confirmation_for_pr(pr_number),
"controller_recovery_authorized_required": True,
"leased_head": leased_head,
"current_head": current_head,
"expires_at": lease_for_class.get("expires_at"),
"terminal_review_verdict": (terminal or {}).get("verdict"),
}
else: else:
classification = "no_lease" classification = "no_lease"
next_action = NEXT_ACTION_ACQUIRE next_action = NEXT_ACTION_ACQUIRE
@@ -720,29 +1292,55 @@ def diagnose_reviewer_pr_lease_handoff(
) )
lease_summary = None lease_summary = None
if active: if lease_for_class:
lease_summary = { lease_summary = {
"comment_id": active.get("comment_id"), "comment_id": lease_for_class.get("comment_id"),
"session_id": active.get("session_id"), "session_id": lease_for_class.get("session_id"),
"phase": active.get("phase"), "phase": lease_for_class.get("phase"),
"candidate_head": active.get("candidate_head"), "candidate_head": lease_for_class.get("candidate_head"),
"expires_at": active.get("expires_at"), "expires_at": lease_for_class.get("expires_at"),
"last_activity": active.get("last_activity"), "last_activity": lease_for_class.get("last_activity"),
"freshness": active.get("freshness") "freshness": lease_for_class.get("freshness")
or classify_lease_freshness(active, now=now), or classify_lease_freshness(lease_for_class, now=now),
"reviewer_identity": active.get("reviewer_identity"), "reviewer_identity": lease_for_class.get("reviewer_identity"),
"profile": active.get("profile"), "profile": lease_for_class.get("profile"),
"worktree": active.get("worktree"), "worktree": lease_for_class.get("worktree"),
"blocker": active.get("blocker"), "blocker": lease_for_class.get("blocker"),
} }
return { return {
"pr_number": pr_number, "pr_number": pr_number,
"classification": classification, "classification": classification,
"blocker_kind": classification,
"next_action": next_action, "next_action": next_action,
"exact_next_action": next_action,
"active_lease": lease_summary, "active_lease": lease_summary,
"session_lease": session_lease, "session_lease": session_lease,
"worktree_binding": binding_details, "worktree_binding": binding_details,
"leased_head": (lease_summary or {}).get("candidate_head"),
"current_head": current_head,
"expires_at": (lease_summary or {}).get("expires_at"),
"terminal_review_state": (
formal_terminal_review_for_head(
formal_reviews,
leased_head=(lease_summary or {}).get("candidate_head"),
)
if lease_summary
else None
),
"worktree_state": {
"exists": worktree_exists,
"clean": worktree_clean,
"path": (lease_summary or {}).get("worktree"),
},
"owner_session_evidence": {
"session_id": (lease_summary or {}).get("session_id"),
"process_alive": owner_process_alive,
"pid_is_not_ownership_proof": True,
},
"cleanup": cleanup_hint,
"cleanup_tool": (cleanup_hint or {}).get("cleanup_tool"),
"required_confirmation": (cleanup_hint or {}).get("required_confirmation"),
"instructed_session_id": instructed_session_id, "instructed_session_id": instructed_session_id,
"instructed_comment_id": instructed_comment_id, "instructed_comment_id": instructed_comment_id,
"instructed_lease_missing_with_replacement": instructed_missing_with_replacement, "instructed_lease_missing_with_replacement": instructed_missing_with_replacement,
@@ -751,6 +1349,19 @@ def diagnose_reviewer_pr_lease_handoff(
and not binding_mismatch and not binding_mismatch
and bool(session_lease) and bool(session_lease)
), ),
"mutation_eligibility": (
"allowed"
if (
next_action == NEXT_ACTION_RESUME_EXACT_OWNER_SESSION
and not binding_mismatch
and bool(session_lease)
)
else (
"cleanup_only"
if next_action == NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
else "prohibited"
)
),
"reasons": reasons, "reasons": reasons,
"forbidden": [ "forbidden": [
"manual lock deletion", "manual lock deletion",
@@ -758,5 +1369,8 @@ def diagnose_reviewer_pr_lease_handoff(
"mtime manipulation", "mtime manipulation",
"direct _SESSION_LEASE seeding", "direct _SESSION_LEASE seeding",
"silent foreign lease steal", "silent foreign lease steal",
"PID-based ownership",
"repointing old lease to new head",
"transfer of validation or decision state",
], ],
} }
+10
View File
@@ -76,6 +76,16 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = {
"permission": "gitea.pr.comment", "permission": "gitea.pr.comment",
"role": "reviewer", "role": "reviewer",
}, },
# #691: guarded non-owner cleanup of obsolete comment-backed reviewer leases.
# Apply path posts lease release + audit comments (gitea.pr.comment).
"cleanup_obsolete_reviewer_comment_lease": {
"permission": "gitea.pr.comment",
"role": "reviewer",
},
"gitea_cleanup_obsolete_reviewer_comment_lease": {
"permission": "gitea.pr.comment",
"role": "reviewer",
},
"blind_pr_queue_review": { "blind_pr_queue_review": {
"permission": "gitea.pr.review", "permission": "gitea.pr.review",
"role": "reviewer", "role": "reviewer",
+22 -16
View File
@@ -79,41 +79,46 @@ class TestApiRequestFailures(unittest.TestCase):
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_timeout_converted_to_runtimeerror(self, mock_open): def test_timeout_converted_to_runtimeerror(self, mock_open):
mock_open.side_effect = TimeoutError("timed out") mock_open.side_effect = TimeoutError("timed out")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaNetworkError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertIn("network error contacting Gitea", str(ctx.exception)) # Fixed message only (#699) — still a RuntimeError subclass.
self.assertIsInstance(ctx.exception, RuntimeError)
self.assertEqual(str(ctx.exception), "Network error contacting Gitea")
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_dns_network_failure_converted(self, mock_open): def test_dns_network_failure_converted(self, mock_open):
mock_open.side_effect = urllib.error.URLError("Name or service not known") mock_open.side_effect = urllib.error.URLError("Name or service not known")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaNetworkError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertIn("network error contacting Gitea", str(ctx.exception)) self.assertEqual(str(ctx.exception), "Network error contacting Gitea")
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_502_upstream_message(self, mock_open): def test_502_upstream_message(self, mock_open):
mock_open.side_effect = http_error(502, "bad gateway") mock_open.side_effect = http_error(502, "bad gateway")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
msg = str(ctx.exception) msg = str(ctx.exception)
self.assertIn("HTTP 502", msg) self.assertEqual(msg, "Gitea upstream unavailable")
self.assertIn("upstream unavailable", msg) self.assertEqual(ctx.exception.reason_code, "upstream_unavailable")
self.assertEqual(ctx.exception.http_status, 502)
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_503_upstream_message(self, mock_open): def test_503_upstream_message(self, mock_open):
mock_open.side_effect = http_error(503, "") mock_open.side_effect = http_error(503, "")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertIn("HTTP 503", str(ctx.exception)) self.assertEqual(str(ctx.exception), "Gitea upstream unavailable")
self.assertIn("upstream unavailable", str(ctx.exception)) self.assertEqual(ctx.exception.http_status, 503)
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_malformed_error_payload_does_not_crash(self, mock_open): def test_malformed_error_payload_does_not_crash(self, mock_open):
# Non-JSON garbage error body must still yield a clean RuntimeError. # Non-JSON garbage error body must still yield a clean typed error
# with a fixed message (no body echo — #699).
mock_open.side_effect = http_error(500, "<html>garbage</html>") mock_open.side_effect = http_error(500, "<html>garbage</html>")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertIn("HTTP 500", str(ctx.exception)) self.assertEqual(str(ctx.exception), "Gitea HTTP request failed")
self.assertNotIn("<html>", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_malformed_success_json_raises_clean_error(self, mock_open): def test_malformed_success_json_raises_clean_error(self, mock_open):
@@ -126,16 +131,17 @@ class TestApiRequestFailures(unittest.TestCase):
def test_no_secret_leak_in_error_body(self, mock_open): def test_no_secret_leak_in_error_body(self, mock_open):
mock_open.side_effect = http_error( mock_open.side_effect = http_error(
400, "failed: token supersecret123 rejected") 400, "failed: token supersecret123 rejected")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
msg = str(ctx.exception) msg = str(ctx.exception)
self.assertNotIn("supersecret123", msg) self.assertNotIn("supersecret123", msg)
self.assertIn(gitea_audit.REDACTED, msg) # Fixed message — body never appears (stronger than redaction).
self.assertEqual(msg, "Gitea HTTP request failed")
@patch("gitea_auth.urllib.request.urlopen") @patch("gitea_auth.urllib.request.urlopen")
def test_auth_header_never_in_error(self, mock_open): def test_auth_header_never_in_error(self, mock_open):
mock_open.side_effect = http_error(400, "bad request") mock_open.side_effect = http_error(400, "bad request")
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH) gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertNotIn(FAKE_AUTH, str(ctx.exception)) self.assertNotIn(FAKE_AUTH, str(ctx.exception))
+1 -7
View File
@@ -27,13 +27,7 @@ from task_capability_map import required_permission, required_role
DELETE_PROFILE = { DELETE_PROFILE = {
"profile_name": "prgs-author-delete", "profile_name": "prgs-author-delete",
"role": "author", "allowed_operations": ["gitea.read", "gitea.branch.delete"],
"allowed_operations": [
"gitea.read",
"gitea.pr.create",
"gitea.branch.push",
"gitea.branch.delete",
],
"forbidden_operations": [], "forbidden_operations": [],
"audit_label": "prgs-author-delete", "audit_label": "prgs-author-delete",
} }
+16 -227
View File
@@ -8,33 +8,10 @@ import branch_cleanup_guard as guard # noqa: E402
import mcp_server # noqa: E402 import mcp_server # noqa: E402
import task_capability_map # noqa: E402 import task_capability_map # noqa: E402
from final_report_validator import assess_final_report_validator # noqa: E402 from final_report_validator import assess_final_report_validator # noqa: E402
from mcp_server import gitea_cleanup_merged_pr_branch, gitea_delete_branch # noqa: E402 from mcp_server import gitea_cleanup_merged_pr_branch # noqa: E402
FAKE_AUTH = "token fake" FAKE_AUTH = "token fake"
# Reconciler-shaped profile that holds branch.delete (recommended) plus
# required pr.close/read so _role_kind classifies as reconciler.
RECONCILER_WITH_DELETE = {
"profile_name": "prgs-reconciler",
"role": "reconciler",
"allowed_operations": [
"gitea.read",
"gitea.pr.close",
"gitea.pr.comment",
"gitea.issue.comment",
"gitea.issue.close",
"gitea.branch.delete",
],
"forbidden_operations": [
"gitea.pr.approve",
"gitea.pr.merge",
"gitea.pr.review",
"gitea.pr.create",
"gitea.branch.push",
"gitea.repo.commit",
],
}
class TestRawBranchDeleteGuard(unittest.TestCase): class TestRawBranchDeleteGuard(unittest.TestCase):
def test_detects_local_and_remote_raw_git_delete_commands(self): def test_detects_local_and_remote_raw_git_delete_commands(self):
@@ -116,48 +93,14 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
self.assertEqual(res["required_permission"], "gitea.branch.delete") self.assertEqual(res["required_permission"], "gitea.branch.delete")
self.mock_api.assert_not_called() self.mock_api.assert_not_called()
def test_author_and_merger_without_delete_authority_fail_closed(self):
role_profiles = {
"author": [
"gitea.read",
"gitea.branch.create",
"gitea.branch.push",
"gitea.repo.commit",
"gitea.pr.create",
],
"merger": ["gitea.read", "gitea.pr.merge"],
}
for name, allowed in role_profiles.items():
with self.subTest(role=name):
profile_patch = patch(
"mcp_server.get_profile",
return_value={
"profile_name": name,
"allowed_operations": allowed,
"forbidden_operations": [],
},
)
profile_patch.start()
try:
res = gitea_cleanup_merged_pr_branch(
pr_number=487,
confirmation="CLEANUP MERGED PR 487 BRANCH feat/branch",
branch="feat/branch",
remote="prgs",
worktree_path="/tmp/repo/branches/cleanup",
)
finally:
profile_patch.stop()
self.assertFalse(res["performed"])
self.assertEqual(
res["required_permission"], "gitea.branch.delete"
)
self.mock_api.assert_not_called()
def test_root_checkout_cleanup_fails_closed(self): def test_root_checkout_cleanup_fails_closed(self):
patch( patch(
"mcp_server.get_profile", "mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE), return_value={
"profile_name": "branch-cleanup",
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
"forbidden_operations": [],
},
).start() ).start()
res = gitea_cleanup_merged_pr_branch( res = gitea_cleanup_merged_pr_branch(
pr_number=487, pr_number=487,
@@ -174,7 +117,11 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
branch = "feat/issue-485-lease-comments-non-list-guard" branch = "feat/issue-485-lease-comments-non-list-guard"
patch( patch(
"mcp_server.get_profile", "mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE), return_value={
"profile_name": "branch-cleanup",
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
"forbidden_operations": [],
},
).start() ).start()
self.mock_api.side_effect = [ self.mock_api.side_effect = [
{ {
@@ -205,7 +152,11 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
branch = "feat/issue-485-lease-comments-non-list-guard" branch = "feat/issue-485-lease-comments-non-list-guard"
patch( patch(
"mcp_server.get_profile", "mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE), return_value={
"profile_name": "branch-cleanup",
"allowed_operations": ["gitea.read", "gitea.branch.delete"],
"forbidden_operations": [],
},
).start() ).start()
self.mock_api.side_effect = [ self.mock_api.side_effect = [
{ {
@@ -231,168 +182,6 @@ class TestMergedPrBranchCleanupTool(unittest.TestCase):
] ]
self.assertFalse(delete_calls) self.assertFalse(delete_calls)
def test_reconciler_with_branch_delete_cannot_raw_delete(self):
"""#687: reconciler + gitea.branch.delete still cannot call raw delete."""
patch(
"mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE),
).start()
res = gitea_delete_branch(
branch="fix/issue-683-workflow-guard-hardening",
remote="prgs",
)
self.assertFalse(res.get("success", True))
self.assertFalse(res.get("performed", True))
reasons = " ".join(res.get("reasons") or [])
self.assertIn("raw gitea_delete_branch", reasons)
self.assertIn("cleanup_merged_pr_branch", reasons)
self.mock_api.assert_not_called()
def test_reconciler_raw_delete_denies_preservation_branch(self):
patch(
"mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE),
).start()
res = gitea_delete_branch(
branch="chore/issue-681-preserve-review-session-wip",
remote="prgs",
)
self.assertFalse(res.get("performed", True))
self.mock_api.assert_not_called()
def test_author_with_branch_delete_role_ok_but_preserve_blocked(self):
"""Author role may use raw delete path when permitted; preserve fails closed."""
patch(
"mcp_server.get_profile",
return_value={
"profile_name": "prgs-author",
"role": "author",
"allowed_operations": [
"gitea.read",
"gitea.pr.create",
"gitea.branch.push",
"gitea.branch.delete",
],
"forbidden_operations": ["gitea.pr.approve", "gitea.pr.merge"],
},
).start()
res = gitea_delete_branch(
branch="chore/issue-681-preserve-review-session-wip",
remote="prgs",
)
self.assertFalse(res.get("performed", True))
self.assertIn("preservation", " ".join(res.get("reasons") or []))
self.mock_api.assert_not_called()
def test_unmerged_branch_cleanup_rejected(self):
branch = "feat/unmerged-work"
patch(
"mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE),
).start()
self.mock_api.side_effect = [
{
"number": 999,
"merged": False,
"merged_at": None,
"head": {"ref": branch, "sha": "b" * 40},
"base": {"ref": "master"},
},
{},
]
res = gitea_cleanup_merged_pr_branch(
pr_number=999,
confirmation=f"CLEANUP MERGED PR 999 BRANCH {branch}",
branch=branch,
remote="prgs",
worktree_path="/tmp/repo/branches/cleanup",
)
self.assertFalse(res["performed"])
self.assertTrue(
any("not merged" in r for r in (res.get("reasons") or []))
)
delete_calls = [
call for call in self.mock_api.call_args_list if call.args[0] == "DELETE"
]
self.assertFalse(delete_calls)
def test_preservation_branch_cleanup_rejected(self):
branch = "chore/issue-681-preserve-review-session-wip"
patch(
"mcp_server.get_profile",
return_value=dict(RECONCILER_WITH_DELETE),
).start()
self.mock_api.side_effect = [
{
"number": 681,
"merged": True,
"merged_at": "2026-07-08T01:00:00Z",
"head": {"ref": branch, "sha": "c" * 40},
"base": {"ref": "master"},
},
{},
]
res = gitea_cleanup_merged_pr_branch(
pr_number=681,
confirmation=f"CLEANUP MERGED PR 681 BRANCH {branch}",
branch=branch,
remote="prgs",
worktree_path="/tmp/repo/branches/cleanup",
)
self.assertFalse(res["performed"])
self.assertTrue(
any("preservation" in r for r in (res.get("reasons") or []))
)
delete_calls = [
call for call in self.mock_api.call_args_list if call.args[0] == "DELETE"
]
self.assertFalse(delete_calls)
def test_non_reconciler_with_delete_denied_cleanup(self):
patch(
"mcp_server.get_profile",
return_value={
"profile_name": "prgs-author",
"role": "author",
"allowed_operations": [
"gitea.read",
"gitea.pr.create",
"gitea.branch.push",
"gitea.branch.delete",
],
"forbidden_operations": [],
},
).start()
res = gitea_cleanup_merged_pr_branch(
pr_number=487,
confirmation="CLEANUP MERGED PR 487 BRANCH feat/branch",
branch="feat/branch",
remote="prgs",
worktree_path="/tmp/repo/branches/cleanup",
)
self.assertFalse(res["performed"])
self.assertEqual(res.get("required_role_kind"), "reconciler")
self.mock_api.assert_not_called()
def test_assess_guard_rejects_preservation_branch(self):
assessment = guard.assess_merged_pr_branch_cleanup(
pr_number=681,
head_branch="chore/issue-681-preserve-review-session-wip",
merged=True,
remote_branch_exists=True,
open_pr_heads=set(),
head_on_target=True,
delete_capability_allowed=True,
confirmation=(
"CLEANUP MERGED PR 681 BRANCH "
"chore/issue-681-preserve-review-session-wip"
),
)
self.assertFalse(assessment["safe_to_delete"])
self.assertTrue(
any("preservation" in r for r in assessment["block_reasons"])
)
if __name__ == "__main__": if __name__ == "__main__":
unittest.main() unittest.main()
@@ -0,0 +1,599 @@
"""Guarded cleanup for obsolete comment-backed reviewer leases (#691).
Reproduces PR #688 lease comment 10749 class of defect: completed
REQUEST_CHANGES on head A, author pushes head B, foreign lease remains
pinned to A (and after expiry still has no non-owner cleanup path that
diagnosis can prescribe beyond indefinite wait).
"""
from __future__ import annotations
import sys
import unittest
from datetime import datetime, timedelta, timezone
from pathlib import Path
from unittest.mock import patch
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import merger_lease_adoption as mla # noqa: E402
import reviewer_pr_lease as leases # noqa: E402
# PR #688 / comment 10749 reproduction fixtures
HEAD_A = "c7a444eb4b41cf916fdbd20a4999ffd78af496d0"
HEAD_B = "4a6357800364718a27a36ebc73578d4b929ff4aa"
SESSION_FOREIGN = "25883-7d4c6e6ebd53"
COMMENT_10749 = 10749
REPO = "Scaled-Tech-Consulting/Gitea-Tools"
PR = 688
ISSUE = 687
EXPIRES_10749 = "2026-07-13T04:23:00Z"
def _utc(dt: datetime) -> datetime:
return dt if dt.tzinfo else dt.replace(tzinfo=timezone.utc)
def _lease_comment(
*,
pr_number: int = PR,
session_id: str = SESSION_FOREIGN,
phase: str = "claimed",
candidate_head: str = HEAD_A,
comment_id: int = COMMENT_10749,
last_activity: datetime | None = None,
expires_at: datetime | None = None,
worktree: str = "branches/review-pr688-feat-issue-687-reconciler-branch-delete",
repo: str = REPO,
) -> dict:
now = last_activity or datetime.now(timezone.utc)
body = leases.format_lease_body(
repo=repo,
pr_number=pr_number,
issue_number=ISSUE,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id=session_id,
worktree=worktree,
phase=phase,
candidate_head=candidate_head,
target_branch="master",
target_branch_sha="b" * 40,
last_activity=now,
expires_at=expires_at,
)
return {
"id": comment_id,
"body": body,
"user": {"login": "sysadmin"},
"author": "sysadmin",
"created_at": now.isoformat(),
"updated_at": now.isoformat(),
}
def _reviews_for_head(head: str, verdict: str = "REQUEST_CHANGES") -> list[dict]:
return [
{
"verdict": verdict,
"reviewed_head_sha": head,
"dismissed": False,
"reviewer": "sysadmin",
}
]
def _assess(
comments,
*,
current_head=HEAD_B,
formal_reviews=None,
controller=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
requesting_session="fresh-controller",
apply=False,
confirmation=None,
**kwargs,
):
return leases.assess_obsolete_reviewer_comment_lease_cleanup(
comments,
pr_number=PR,
current_head_sha=current_head,
formal_reviews=formal_reviews if formal_reviews is not None else _reviews_for_head(HEAD_A),
repo=REPO,
expected_repo=REPO,
requesting_session_id=requesting_session,
controller_recovery_authorized=controller,
worktree_exists=worktree_exists,
worktree_clean=worktree_clean,
worktree_has_unpreserved_work=not worktree_clean if worktree_exists else False,
owner_process_alive=owner_process_alive,
owner_pid_observed=kwargs.get("owner_pid_observed"),
requesting_pid=kwargs.get("requesting_pid", 99999),
current_head_review_in_progress=kwargs.get(
"current_head_review_in_progress", False
),
expected_lease_comment_id=kwargs.get("expected_lease_comment_id"),
expected_session_id=kwargs.get("expected_session_id"),
expected_leased_head=kwargs.get("expected_leased_head"),
confirmation=confirmation
if confirmation is not None
else (leases.cleanup_confirmation_for_pr(PR) if apply else ""),
apply=apply,
now=kwargs.get("now"),
)
class TestIssue691SupersededHeadCleanup(unittest.TestCase):
def setUp(self):
leases.clear_session_lease()
def test_1_request_changes_then_push_expired_cleanup_succeeds(self):
"""Completed REQUEST_CHANGES on A, push B, lease A expires → cleanup OK."""
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
last_activity=expires - timedelta(hours=2),
expires_at=expires,
)
result = _assess(
[claim],
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
now=now,
apply=True,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "foreign_expired_superseded_head")
self.assertEqual(
result["exact_next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
self.assertIn("phase: released", result["release_body"])
self.assertIn("obsolete-superseded-or-expired-lease", result["release_body"])
self.assertIsNotNone(result["audit_comment_body"])
self.assertEqual(result["cleanup_tool"], leases.CLEANUP_OBSOLETE_LEASE_TOOL)
def test_2_approve_then_push_cleanup_policy(self):
"""Completed APPROVE on A, push B → cleanup eligible (completed superseded)."""
now = datetime(2026, 7, 13, 1, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc) # not yet expired
claim = _lease_comment(
last_activity=now - timedelta(minutes=10),
expires_at=expires,
)
result = _assess(
[claim],
formal_reviews=_reviews_for_head(HEAD_A, "APPROVED"),
now=now,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "foreign_completed_superseded_head")
def test_3_active_current_head_cleanup_denied(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=5),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_B, "REQUEST_CHANGES"),
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertEqual(result["classification"], "foreign_active_current_head")
def test_4_foreign_owner_recent_progress_denied(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=2),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
owner_process_alive=True,
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(
any("recent authenticated progress" in r for r in result["reasons"])
or any("current PR head" in r for r in result["reasons"])
)
def test_5_owner_absent_worktree_dirty_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=expires - timedelta(hours=1),
expires_at=expires,
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
worktree_clean=False,
owner_process_alive=False,
now=now,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("dirty" in r for r in result["reasons"]))
def test_6_owner_absent_worktree_clean_orphan_ok(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=expires - timedelta(hours=1),
expires_at=expires,
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=[],
worktree_clean=True,
owner_process_alive=False,
now=now,
)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["classification"], "orphaned_owner_missing")
def test_7_pid_reuse_not_ownership(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim],
now=now,
owner_pid_observed=4242,
requesting_pid=4242,
)
self.assertTrue(
any("PID equality" in r or "NOT ownership" in r for r in result["reasons"])
or result["owner_session_evidence"]["pid_is_not_ownership_proof"]
)
# Still eligible via superseded+terminal+expired despite matching PID.
self.assertTrue(result["cleanup_allowed"], result["reasons"])
def test_8_comment_backed_no_db_lease_id(self):
"""Cleanup uses comment ledger only — no control-plane lease_id required."""
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
# Explicitly no lease_id field anywhere.
self.assertNotIn("lease_id", claim)
result = _assess([claim], now=now)
self.assertTrue(result["cleanup_allowed"], result["reasons"])
self.assertEqual(result["active_lease"]["comment_id"], COMMENT_10749)
self.assertIsNone(result["active_lease"].get("lease_id"))
def test_9_cleanup_posts_durable_audit_bodies(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], now=now, apply=True)
self.assertTrue(result["cleanup_allowed"])
self.assertIn("#691", result["audit_comment_body"])
self.assertIn(str(COMMENT_10749), result["audit_comment_body"])
self.assertIn(HEAD_A, result["audit_comment_body"])
self.assertIn(HEAD_B, result["audit_comment_body"])
def test_10_fresh_acquire_after_cleanup_marker(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(
expires_at=expires,
last_activity=expires - timedelta(hours=1),
comment_id=COMMENT_10749,
)
assessed = _assess([claim], now=now, apply=True)
self.assertTrue(assessed["cleanup_allowed"])
release = {
"id": 99999,
"body": assessed["release_body"],
"user": {"login": "sysadmin"},
}
# Newest terminal marker ends active lease.
active = leases.find_active_reviewer_lease(
[claim, release], pr_number=PR, now=now
)
self.assertIsNone(active)
acq = leases.assess_acquire_lease(
[claim, release],
pr_number=PR,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id="fresh-reviewer-1",
repo=REPO,
issue_number=ISSUE,
worktree="branches/review-pr-688-fresh",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=now,
)
self.assertTrue(acq["acquire_allowed"], acq["reasons"])
def test_11_no_validation_state_transfer(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], now=now, apply=True)
self.assertTrue(result["cleanup_allowed"])
# Release body keeps old candidate_head (no repoint).
self.assertIn(f"candidate_head: {HEAD_A}", result["release_body"])
self.assertNotIn(HEAD_B, result["release_body"].split("candidate_head:")[1].split("\n")[0])
self.assertIn("did not transfer validation", result["audit_comment_body"])
self.assertIn("did not repoint", result["audit_comment_body"])
# Session lease must remain unset by assessment (tool never seeds).
self.assertIsNone(leases.get_session_lease())
def test_12_repeated_cleanup_idempotent(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
first = _assess([claim], now=now, apply=True)
self.assertTrue(first["cleanup_allowed"])
release = {"id": 100001, "body": first["release_body"], "user": {"login": "x"}}
second = _assess([claim, release], now=now, apply=True)
self.assertFalse(second["cleanup_allowed"])
self.assertEqual(second["classification"], "no_lease")
def test_13_malformed_expiry_fails_closed(self):
claim = _lease_comment()
# Corrupt expires_at in the body.
claim["body"] = claim["body"].replace(
claim["body"].split("expires_at:")[1].split("\n")[0].strip(),
"not-a-timestamp",
)
result = _assess([claim], formal_reviews=_reviews_for_head(HEAD_A))
self.assertFalse(result["cleanup_allowed"])
self.assertFalse(result["expires_parseable"])
self.assertTrue(any("expires_at" in r for r in result["reasons"]))
def test_14_wrong_identity_evidence_fails_closed(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
bad_repo = _assess(
[claim],
now=now,
# force repo mismatch via expected_repo
)
# Patch via direct call with wrong expected_repo
bad = leases.assess_obsolete_reviewer_comment_lease_cleanup(
[claim],
pr_number=PR,
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A),
expected_repo="Other-Org/Other-Repo",
requesting_session_id="fresh",
controller_recovery_authorized=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
now=now,
)
self.assertFalse(bad["cleanup_allowed"])
self.assertTrue(any("repository identity" in r for r in bad["reasons"]))
bad_pr = leases.assess_obsolete_reviewer_comment_lease_cleanup(
[claim],
pr_number=999,
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A),
expected_repo=REPO,
requesting_session_id="fresh",
controller_recovery_authorized=True,
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
expected_lease_comment_id=COMMENT_10749,
now=now,
)
self.assertFalse(bad_pr["cleanup_allowed"])
bad_head = _assess(
[claim],
now=now,
expected_leased_head="0" * 40,
)
self.assertFalse(bad_head["cleanup_allowed"])
bad_session = _assess(
[claim],
now=now,
expected_session_id="wrong-session",
)
self.assertFalse(bad_session["cleanup_allowed"])
def test_15_current_head_active_cannot_be_displaced(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=1),
expires_at=now + timedelta(hours=2),
)
result = _assess(
[claim],
current_head=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_B),
now=now,
current_head_review_in_progress=True,
)
self.assertFalse(result["cleanup_allowed"])
# Acquire also blocked by foreign active lease.
acq = leases.assess_acquire_lease(
[claim],
pr_number=PR,
reviewer_identity="other",
profile="prgs-reviewer",
session_id="thief",
repo=REPO,
issue_number=ISSUE,
worktree="branches/steal",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=now,
)
self.assertFalse(acq["acquire_allowed"])
def test_regression_pr688_comment_10749_after_expiry(self):
"""Exact 10749 reproduction after recorded expires_at 2026-07-13T04:23:00Z."""
now = datetime(2026, 7, 13, 4, 30, 0, tzinfo=timezone.utc)
expires = datetime.fromisoformat(EXPIRES_10749.replace("Z", "+00:00"))
claim = _lease_comment(
session_id=SESSION_FOREIGN,
candidate_head=HEAD_A,
comment_id=COMMENT_10749,
last_activity=expires - timedelta(hours=2),
expires_at=expires,
)
# Diagnosis must not prescribe indefinite wait-only for this case.
diag = leases.diagnose_reviewer_pr_lease_handoff(
[claim],
pr_number=PR,
current_session_id="fresh-reviewer",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
worktree_exists=True,
worktree_clean=True,
owner_process_alive=False,
instructed_session_id=SESSION_FOREIGN,
instructed_comment_id=COMMENT_10749,
now=now,
)
self.assertEqual(diag["classification"], "foreign_expired_superseded_head")
self.assertEqual(
diag["next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
self.assertEqual(diag["cleanup_tool"], leases.CLEANUP_OBSOLETE_LEASE_TOOL)
self.assertIn("CLEANUP OBSOLETE REVIEWER LEASE 688", diag["required_confirmation"])
self.assertEqual(diag["mutation_eligibility"], "cleanup_only")
self.assertFalse(diag["mutation_allowed"])
# Assessment still returns the lease as active/stale class of block for acquire
# when unexpired path... here it's expired so find_active is None; acquire free
# only after cleanup if somehow still active. With expired, find_active is None:
active = leases.find_active_reviewer_lease([claim], pr_number=PR, now=now)
self.assertIsNone(active)
# But superseded unexpired still blocks acquire and diagnosis points to cleanup:
unexpired_now = datetime(2026, 7, 13, 1, 0, 0, tzinfo=timezone.utc)
claim2 = _lease_comment(
session_id=SESSION_FOREIGN,
candidate_head=HEAD_A,
comment_id=COMMENT_10749,
last_activity=unexpired_now - timedelta(minutes=45),
expires_at=expires,
)
active2 = leases.find_active_reviewer_lease(
[claim2], pr_number=PR, now=unexpired_now
)
self.assertIsNotNone(active2)
self.assertEqual(active2["freshness"], "stale_warning")
diag2 = leases.diagnose_reviewer_pr_lease_handoff(
[claim2],
pr_number=PR,
current_session_id="fresh-reviewer",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=_reviews_for_head(HEAD_A, "REQUEST_CHANGES"),
worktree_exists=True,
worktree_clean=True,
now=unexpired_now,
)
self.assertEqual(diag2["classification"], "foreign_completed_superseded_head")
self.assertEqual(
diag2["next_action"], leases.NEXT_ACTION_CLEANUP_OBSOLETE_LEASE
)
acq = leases.assess_acquire_lease(
[claim2],
pr_number=PR,
reviewer_identity="sysadmin",
profile="prgs-reviewer",
session_id="fresh-reviewer",
repo=REPO,
issue_number=ISSUE,
worktree="branches/review-pr-688-new",
candidate_head=HEAD_B,
target_branch="master",
target_branch_sha="b" * 40,
now=unexpired_now,
)
self.assertFalse(acq["acquire_allowed"])
def test_missing_controller_auth_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], controller=False, now=now)
self.assertFalse(result["cleanup_allowed"])
def test_wrong_confirmation_denied_on_apply(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim], now=now, apply=True, confirmation="MERGE PR 688"
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("confirmation" in r for r in result["reasons"]))
def test_owner_session_must_use_owner_release(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess(
[claim],
now=now,
requesting_session=SESSION_FOREIGN,
)
self.assertFalse(result["cleanup_allowed"])
self.assertTrue(any("owns the lease" in r for r in result["reasons"]))
def test_superseded_without_terminal_review_denied(self):
now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc)
expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc)
claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1))
result = _assess([claim], formal_reviews=[], now=now)
self.assertFalse(result["cleanup_allowed"])
self.assertEqual(result["classification"], "ambiguous_conflicting_evidence")
class TestIssue691DiagnoseClassifications(unittest.TestCase):
def setUp(self):
leases.clear_session_lease()
def test_foreign_active_current_head_still_wait(self):
now = datetime.now(timezone.utc)
claim = _lease_comment(
candidate_head=HEAD_B,
last_activity=now - timedelta(minutes=5),
expires_at=now + timedelta(hours=1),
)
claim["id"] = 1
result = leases.diagnose_reviewer_pr_lease_handoff(
[claim],
pr_number=PR,
current_session_id="other",
current_reviewer_identity="sysadmin",
current_head_sha=HEAD_B,
formal_reviews=[],
now=now,
)
self.assertEqual(result["classification"], "foreign_active_current_head")
self.assertEqual(result["next_action"], leases.NEXT_ACTION_WAIT)
self.assertFalse(result["mutation_allowed"])
if __name__ == "__main__":
unittest.main()
+3 -9
View File
@@ -1425,16 +1425,10 @@ class TestReviewPR(unittest.TestCase):
class TestDeleteBranch(unittest.TestCase): class TestDeleteBranch(unittest.TestCase):
DELETE_PROFILE = { DELETE_PROFILE = {
"profile_name": "test-author-deleter", "profile_name": "test-deleter",
"role": "author", "allowed_operations": ["gitea.read", "gitea.branch.delete"],
"allowed_operations": [
"gitea.read",
"gitea.pr.create",
"gitea.branch.push",
"gitea.branch.delete",
],
"forbidden_operations": [], "forbidden_operations": [],
"audit_label": "test-author-deleter", "audit_label": "test-deleter",
} }
@patch("mcp_server.get_profile", return_value=DELETE_PROFILE) @patch("mcp_server.get_profile", return_value=DELETE_PROFILE)
+6 -176
View File
@@ -92,19 +92,14 @@ class TestMigrateProfiles(unittest.TestCase):
author = prgs_gitea["identities"]["author"] author = prgs_gitea["identities"]["author"]
self.assertEqual(author["username"], "jcwalker3") self.assertEqual(author["username"], "jcwalker3")
self.assertEqual(author["auth"]["id"], "redacted-author-ref") self.assertEqual(author["auth"]["id"], "redacted-author-ref")
self.assertEqual( self.assertEqual(author["allowed_operations"], ["read", "comment"])
author["allowed_operations"], ["gitea.read", "gitea.pr.comment"] self.assertEqual(author["forbidden_operations"], ["approve", "merge"])
)
self.assertEqual(
author["forbidden_operations"],
["gitea.pr.approve", "gitea.pr.merge"],
)
reviewer = prgs_gitea["identities"]["reviewer"] reviewer = prgs_gitea["identities"]["reviewer"]
self.assertEqual(reviewer["role"], "reviewer") self.assertEqual(reviewer["role"], "reviewer")
self.assertEqual(reviewer["username"], "sysadmin") self.assertEqual(reviewer["username"], "sysadmin")
self.assertEqual(reviewer["auth"]["id"], "redacted-reviewer-ref") self.assertEqual(reviewer["auth"]["id"], "redacted-reviewer-ref")
self.assertIn("gitea.pr.merge", reviewer["allowed_operations"]) self.assertIn("merge", reviewer["allowed_operations"])
def test_alias_generation(self): def test_alias_generation(self):
"""Test that aliases are correctly generated to support old profile names.""" """Test that aliases are correctly generated to support old profile names."""
@@ -193,7 +188,7 @@ class TestMigrateProfiles(unittest.TestCase):
self.assertNotIn("token", stdout_output.lower()) self.assertNotIn("token", stdout_output.lower())
def test_explicit_operations_are_preserved(self): def test_explicit_operations_are_preserved(self):
"""Explicit v1 permissions are canonicalized, not replaced by role defaults.""" """Explicit v1 permissions must not be replaced by role defaults."""
v1_data = json.loads(json.dumps(self.v1_content)) v1_data = json.loads(json.dumps(self.v1_content))
v1_data["profiles"]["prgs-reviewer"]["allowed_operations"] = ["read"] v1_data["profiles"]["prgs-reviewer"]["allowed_operations"] = ["read"]
v1_data["profiles"]["prgs-reviewer"]["forbidden_operations"] = ["merge"] v1_data["profiles"]["prgs-reviewer"]["forbidden_operations"] = ["merge"]
@@ -203,8 +198,8 @@ class TestMigrateProfiles(unittest.TestCase):
v2_data["environments"]["prgs"]["services"]["gitea"] v2_data["environments"]["prgs"]["services"]["gitea"]
["identities"]["reviewer"] ["identities"]["reviewer"]
) )
self.assertEqual(reviewer["allowed_operations"], ["gitea.read"]) self.assertEqual(reviewer["allowed_operations"], ["read"])
self.assertEqual(reviewer["forbidden_operations"], ["gitea.pr.merge"]) self.assertEqual(reviewer["forbidden_operations"], ["merge"])
def test_inferred_role_defaults_only_when_unambiguous(self): def test_inferred_role_defaults_only_when_unambiguous(self):
"""Role defaults are allowed only for clear author/reviewer profiles.""" """Role defaults are allowed only for clear author/reviewer profiles."""
@@ -311,171 +306,6 @@ class TestMigrateProfiles(unittest.TestCase):
migrate_profiles.main() migrate_profiles.main()
self.assertEqual(cm.exception.code, 1) self.assertEqual(cm.exception.code, 1)
def test_reconciler_profile_migration(self):
"""Legacy reconciler shorthands migrate to valid canonical operations."""
import gitea_config
import reconciler_profile
v1_data = {
"version": 1,
"profiles": {
"prgs-reconciler": {
"base_url": "redacted-prgs-service",
"username": "reconciler-agent",
"auth": {"type": "keychain", "id": "reconciler-ref"},
"execution_profile": "prgs-reconciler",
"allowed_operations": [
"read",
"pr.close",
"pr.comment",
"issue.comment",
"issue.close",
"gitea.branch.delete",
],
"forbidden_operations": [
"merge",
"approve",
"review",
"pr.create",
"branch.push",
"commit",
],
}
}
}
v2_data = migrate_profiles.migrate_v1_to_v2(v1_data)
reconciler = (
v2_data["environments"]["prgs"]["services"]["gitea"]
["identities"]["reconciler"]
)
self.assertEqual(reconciler["role"], "reconciler")
allowed = reconciler["allowed_operations"]
forbidden = reconciler["forbidden_operations"]
# No invalid shorthand remains
for bad in ("pr.close", "pr.comment", "issue.close", "read", "merge"):
self.assertNotIn(bad, allowed)
self.assertNotIn(bad, forbidden)
for required in (
"gitea.read",
"gitea.pr.close",
"gitea.pr.comment",
"gitea.issue.comment",
"gitea.issue.close",
"gitea.branch.delete",
):
self.assertIn(required, allowed)
# Production loader accepts every allowed op
self.assertEqual(
gitea_config.normalize_operation(required), required
)
self.assertEqual(v2_data["aliases"]["prgs-reconciler"], "prgs.gitea.reconciler")
assessment = reconciler_profile.assess_reconciler_profile(allowed, forbidden)
self.assertTrue(assessment["valid"])
self.assertTrue(migrate_profiles.validate_v2_data(v2_data))
def test_reconciler_profile_defaults(self):
"""Reconciler defaults are fully canonical and loader-valid."""
import gitea_config
import reconciler_profile
v1_data = {
"version": 1,
"profiles": {
"prgs-reconciler": {
"base_url": "redacted-prgs-service",
"username": "reconciler-agent",
"auth": {"type": "keychain", "id": "reconciler-ref"},
"execution_profile": "prgs-reconciler",
}
}
}
v2_data = migrate_profiles.migrate_v1_to_v2(v1_data)
reconciler = (
v2_data["environments"]["prgs"]["services"]["gitea"]
["identities"]["reconciler"]
)
self.assertEqual(reconciler["role"], "reconciler")
self.assertEqual(
reconciler["allowed_operations"],
migrate_profiles.RECONCILER_DEFAULT_ALLOWED,
)
self.assertEqual(
reconciler["forbidden_operations"],
migrate_profiles.RECONCILER_DEFAULT_FORBIDDEN,
)
for op in reconciler["allowed_operations"]:
self.assertEqual(gitea_config.normalize_operation(op), op)
self.assertTrue(op.startswith("gitea."))
assessment = reconciler_profile.assess_reconciler_profile(
reconciler["allowed_operations"],
reconciler["forbidden_operations"],
)
self.assertTrue(assessment["valid"])
self.assertNotIn(
"gitea.branch.delete", assessment["missing_recommended_operations"]
)
def test_reconciler_migration_idempotent_canonicalize(self):
"""Second canonicalize of already-canonical ops is a no-op."""
first = migrate_profiles.canonicalize_operations(
list(migrate_profiles.RECONCILER_DEFAULT_ALLOWED)
)
second = migrate_profiles.canonicalize_operations(first)
self.assertEqual(first, second)
self.assertEqual(first, list(migrate_profiles.RECONCILER_DEFAULT_ALLOWED))
def test_reconciler_missing_required_fails_visibly(self):
"""Missing gitea.pr.close after migration fails closed (not silent drop)."""
v1_data = {
"version": 1,
"profiles": {
"prgs-reconciler": {
"base_url": "redacted-prgs-service",
"username": "reconciler-agent",
"auth": {"type": "keychain", "id": "reconciler-ref"},
"execution_profile": "prgs-reconciler",
"allowed_operations": ["read", "gitea.branch.delete"],
"forbidden_operations": ["merge"],
}
},
}
with self.assertRaisesRegex(ValueError, "missing required"):
migrate_profiles.migrate_v1_to_v2(v1_data)
def test_unknown_operation_fails_visibly(self):
v1_data = {
"version": 1,
"profiles": {
"prgs-author": {
"base_url": "redacted-prgs-service",
"username": "jcwalker3",
"auth": {"type": "keychain", "id": "hidden-author-ref"},
"execution_profile": "prgs-author",
"allowed_operations": ["read", "not.a.real.op"],
"forbidden_operations": ["merge"],
}
},
}
with self.assertRaisesRegex(ValueError, "cannot be canonicalized"):
migrate_profiles.migrate_v1_to_v2(v1_data)
def test_role_inference_author_reviewer_merger_reconciler(self):
self.assertEqual(
migrate_profiles.infer_role("prgs-author", "prgs-author"), "author"
)
self.assertEqual(
migrate_profiles.infer_role("prgs-reviewer", "prgs-reviewer"),
"reviewer",
)
self.assertEqual(
migrate_profiles.infer_role("prgs-reconciler", "prgs-reconciler"),
"reconciler",
)
self.assertIsNone(
migrate_profiles.infer_role("prgs-merger", "prgs-merger")
)
if __name__ == "__main__": if __name__ == "__main__":
unittest.main() unittest.main()
-36
View File
@@ -82,42 +82,6 @@ class TestReconcilerProfileModel(unittest.TestCase):
"reconciler", "reconciler",
) )
def test_branch_delete_is_recommended_for_reconciler(self):
self.assertIn(
"gitea.branch.delete",
reconciler_profile.RECONCILER_RECOMMENDED_OPERATIONS,
)
self.assertNotIn(
"gitea.branch.delete",
reconciler_profile.RECONCILER_REQUIRED_OPERATIONS,
)
def test_reconciler_with_branch_delete_stays_valid(self):
allowed = PRGS_RECONCILER_ALLOWED + ["gitea.branch.delete"]
result = reconciler_profile.assess_reconciler_profile(
allowed,
PRGS_RECONCILER_FORBIDDEN,
)
self.assertTrue(result["is_reconciler_profile"])
self.assertTrue(result["valid"])
self.assertNotIn(
"gitea.branch.delete", result["missing_recommended_operations"]
)
self.assertEqual(
mcp_server._role_kind(allowed, PRGS_RECONCILER_FORBIDDEN),
"reconciler",
)
def test_reconciler_without_branch_delete_reports_missing_recommended(self):
result = reconciler_profile.assess_reconciler_profile(
PRGS_RECONCILER_ALLOWED,
PRGS_RECONCILER_FORBIDDEN,
)
self.assertTrue(result["valid"])
self.assertIn(
"gitea.branch.delete", result["missing_recommended_operations"]
)
if __name__ == "__main__": if __name__ == "__main__":
unittest.main() unittest.main()
+9 -4
View File
@@ -122,9 +122,11 @@ class TestApiRequestRetry(unittest.TestCase):
sleep.assert_not_called() sleep.assert_not_called()
def test_non_429_error_raises_immediately(self): def test_non_429_error_raises_immediately(self):
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
_call([_http_error(500, body=b"boom")]) _call([_http_error(500, body=b"boom")])
self.assertIn("HTTP 500", str(ctx.exception)) # Fixed message only (#699) — no response body echo.
self.assertEqual(str(ctx.exception), "Gitea HTTP request failed")
self.assertEqual(ctx.exception.http_status, 500)
def test_non_429_error_does_not_sleep(self): def test_non_429_error_does_not_sleep(self):
sleep = MagicMock() sleep = MagicMock()
@@ -171,9 +173,12 @@ class TestApiRequestRetry(unittest.TestCase):
# max_retries=3 -> 3 sleeps, then the 4th failure raises. # max_retries=3 -> 3 sleeps, then the 4th failure raises.
errors = [_http_error(429, retry_after="1") for _ in range(4)] errors = [_http_error(429, retry_after="1") for _ in range(4)]
sleep = MagicMock() sleep = MagicMock()
with self.assertRaises(RuntimeError) as ctx: with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
_call(errors, sleep_func=sleep, max_retries=3) _call(errors, sleep_func=sleep, max_retries=3)
self.assertIn("HTTP 429", str(ctx.exception)) # After retries are exhausted, 429 is a typed HTTP error with fixed
# message (#699); status metadata carries 429.
self.assertEqual(str(ctx.exception), "Gitea HTTP request failed")
self.assertEqual(ctx.exception.http_status, 429)
self.assertEqual(sleep.call_count, 3) self.assertEqual(sleep.call_count, 3)
def test_no_infinite_loop_when_always_429(self): def test_no_infinite_loop_when_always_429(self):
+426
View File
@@ -0,0 +1,426 @@
"""Structured MCP auth errors and stdio transport survival (#699 / PR #701).
Covers original AC plus reviewer-ratified regressions:
1. Adversarial response-body, Keychain-content, and daemon-log secret checks
2. Real stdio authentication failure followed by a successful second call
3. UrlElicitationRequiredError framework re-raise behavior
4. Unexpected parser RuntimeError is not authentication
5. Generic HTTP 403 is authorization (not internal)
6. Repeated install/import is idempotent
7. Author and reconciler profiles
8. Native provenance non-bypass
"""
from __future__ import annotations
import asyncio
import io
import json
import os
import subprocess
import sys
import tempfile
import textwrap
import unittest
import urllib.error
from unittest.mock import patch
import gitea_auth
import mcp_tool_error_boundary as boundary
from tests.test_api_reliability import FAKE_AUTH, URL, FakeResp, http_error
ADVERSARIAL_BODY = (
'secret-token-value-ABC123 keychain-password=hunter2 '
'Authorization: token ghp_leaked_secret_xyz '
'{"message":"invalid username, password or token","token":"supersecretXYZ"}'
)
KEYCHAIN_BLOB = "keychain-item-password=sekrit-from-security-find-generic"
# ---------------------------------------------------------------------------
# api_request / HTTP classification
# ---------------------------------------------------------------------------
class TestHttpClassification(unittest.TestCase):
@patch("gitea_auth.urllib.request.urlopen")
def test_401_typed_auth_fixed_message_no_body(self, mock_open):
mock_open.side_effect = http_error(401, ADVERSARIAL_BODY)
with self.assertRaises(gitea_auth.GiteaAuthError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
exc = ctx.exception
self.assertEqual(exc.reason_code, "auth_invalid_token")
self.assertEqual(exc.error_class, "authentication")
self.assertEqual(exc.http_status, 401)
msg = str(exc)
self.assertNotIn("supersecretXYZ", msg)
self.assertNotIn("ghp_leaked", msg)
self.assertNotIn("hunter2", msg)
self.assertNotIn(ADVERSARIAL_BODY, msg)
self.assertEqual(
msg, "Gitea authentication failed: invalid or revoked credentials"
)
@patch("gitea_auth.urllib.request.urlopen")
def test_403_scope_is_authz_scope(self, mock_open):
mock_open.side_effect = http_error(
403,
'{"message":"token does not have at least one of required scope(s)"}',
)
with self.assertRaises(gitea_auth.GiteaAuthzError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(ctx.exception.reason_code, "authz_insufficient_scope")
self.assertEqual(ctx.exception.error_class, "authorization")
self.assertNotIn("token does not have", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_generic_403_is_authz_not_internal(self, mock_open):
mock_open.side_effect = http_error(403, '{"message":"user has no permission"}')
with self.assertRaises(gitea_auth.GiteaAuthzError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(ctx.exception.reason_code, "authz_denied")
self.assertEqual(ctx.exception.error_class, "authorization")
self.assertNotIsInstance(ctx.exception, gitea_auth.GiteaAuthError)
self.assertNotIn("user has no permission", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_network_fixed_message(self, mock_open):
mock_open.side_effect = TimeoutError("timed out contacting secret.example")
with self.assertRaises(gitea_auth.GiteaNetworkError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(str(ctx.exception), "Network error contacting Gitea")
self.assertNotIn("secret.example", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_malformed_json_not_auth(self, mock_open):
mock_open.return_value = FakeResp("not-json{")
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertNotIsInstance(ctx.exception, gitea_auth.GiteaAuthError)
self.assertIn("malformed JSON", str(ctx.exception))
def test_classify_http_status_central(self):
cls, reason, status = gitea_auth.classify_http_status(401)
self.assertIs(cls, gitea_auth.GiteaAuthError)
self.assertEqual(reason, "auth_invalid_token")
self.assertEqual(status, 401)
cls, reason, status = gitea_auth.classify_http_status(403)
self.assertIs(cls, gitea_auth.GiteaAuthzError)
self.assertEqual(reason, "authz_denied")
cls, reason, status = gitea_auth.classify_http_status(
403, body_hint="missing scope write:issue"
)
self.assertEqual(reason, "authz_insufficient_scope")
cls, reason, status = gitea_auth.classify_http_status(502)
self.assertIs(cls, gitea_auth.GiteaHttpError)
self.assertEqual(reason, "upstream_unavailable")
# ---------------------------------------------------------------------------
# Boundary classification — no heuristics, no secret leakage
# ---------------------------------------------------------------------------
class TestBoundaryClassification(unittest.TestCase):
def test_auth_payload_fixed_message(self):
exc = gitea_auth.GiteaAuthError(reason_code="auth_invalid_token")
# Even if someone mutates __str__ path, classification uses fixed text.
result = boundary.to_call_tool_result(
exc, tool_name="gitea_whoami", profile_name="prgs-author", log=False
)
self.assertTrue(result.isError)
p = result.structuredContent
self.assertEqual(p["reason_code"], "auth_invalid_token")
self.assertEqual(p["error_class"], "authentication")
self.assertEqual(p["message"], boundary.fixed_message("auth_invalid_token"))
self.assertNotIn("supersecret", json.dumps(p))
self.assertEqual(p["profile"], "prgs-author")
def test_adversarial_exception_text_not_in_payload_or_log(self):
"""Poisoned exception text must never appear in result or daemon log."""
class PoisonedAuth(gitea_auth.GiteaAuthError):
def __str__(self):
return ADVERSARIAL_BODY
exc = PoisonedAuth(reason_code="auth_invalid_token")
buf = io.StringIO()
result = boundary.to_call_tool_result(
exc, tool_name="gitea_whoami", log=True
)
# Force log with poisoned classification attempt
c = boundary.classify_exception(exc)
boundary.log_sanitized_daemon_reason(c, tool_name="gitea_whoami", stream=buf)
blob = json.dumps(result.structuredContent) + result.content[0].text + buf.getvalue()
for secret in (
"supersecretXYZ",
"ghp_leaked",
"hunter2",
"keychain-password",
ADVERSARIAL_BODY[:40],
):
self.assertNotIn(secret, blob)
self.assertIn("reason_code=auth_invalid_token", buf.getvalue())
self.assertNotIn("detail=", buf.getvalue())
def test_keychain_content_not_in_daemon_log(self):
buf = io.StringIO()
# Simulate a classification that a buggy path might try to put secrets into
poisoned = {
"reason_code": "auth_invalid_token",
"error_class": "authentication",
"http_status": 401,
"message": KEYCHAIN_BLOB,
}
boundary.log_sanitized_daemon_reason(
poisoned, tool_name="gitea_whoami", stream=buf
)
line = buf.getvalue()
self.assertNotIn("sekrit", line)
self.assertNotIn("keychain-item", line)
self.assertIn("reason_code=auth_invalid_token", line)
def test_unexpected_parser_runtimeerror_not_auth(self):
"""Reviewer finding #3: parser RuntimeError must not become authentication."""
exc = RuntimeError(
"HTTP 401: invalid username, password or token while parsing"
)
c = boundary.classify_exception(exc)
self.assertEqual(c["error_class"], "internal")
self.assertEqual(c["reason_code"], "internal_error")
self.assertNotEqual(c["error_class"], "authentication")
self.assertFalse(boundary.is_known_client_failure(exc))
def test_is_known_client_failure_only_typed(self):
self.assertTrue(
boundary.is_known_client_failure(gitea_auth.GiteaAuthError())
)
self.assertTrue(
boundary.is_known_client_failure(gitea_auth.GiteaAuthzError())
)
self.assertFalse(boundary.is_known_client_failure(RuntimeError("x")))
self.assertFalse(boundary.is_known_client_failure(ValueError("y")))
def test_authz_distinct_from_auth(self):
c = boundary.classify_exception(
gitea_auth.GiteaAuthzError(reason_code="authz_denied")
)
self.assertEqual(c["error_class"], "authorization")
self.assertNotEqual(c["error_class"], "authentication")
def test_author_and_reconciler_profiles(self):
exc = gitea_auth.GiteaAuthError()
for profile in ("prgs-author", "prgs-reconciler"):
r = boundary.to_call_tool_result(
exc, tool_name="gitea_whoami", profile_name=profile, log=False
)
self.assertEqual(r.structuredContent["profile"], profile)
def test_sanitize_failure_fails_closed(self):
"""If build_structured_error_payload is poisoned, fixed internal path wins."""
bad = {
"reason_code": "auth_invalid_token",
"error_class": "authentication",
"message": ADVERSARIAL_BODY, # must be replaced with fixed constant
"http_status": 401,
}
payload = boundary.build_structured_error_payload(bad)
self.assertEqual(
payload["message"], boundary.fixed_message("auth_invalid_token")
)
self.assertNotIn("supersecret", payload["message"])
# ---------------------------------------------------------------------------
# Tool.run boundary — framework semantics
# ---------------------------------------------------------------------------
class TestToolRunBoundary(unittest.TestCase):
def setUp(self):
from mcp.server.fastmcp.tools.base import Tool
boundary.install_tool_run_boundary(Tool)
self.Tool = Tool
def _tool(self, fn, name="demo_tool"):
return self.Tool.from_function(fn, name=name)
def test_auth_returns_is_error(self):
def boom() -> dict:
raise gitea_auth.GiteaAuthError()
result = asyncio.run(self._tool(boom, "gitea_whoami").run({}, convert_result=True))
self.assertTrue(result.isError)
self.assertEqual(result.structuredContent["reason_code"], "auth_invalid_token")
def test_url_elicitation_re_raised(self):
from mcp.shared.exceptions import UrlElicitationRequiredError
from mcp.types import ElicitRequestURLParams
def boom() -> dict:
raise UrlElicitationRequiredError(
[
ElicitRequestURLParams(
mode="url",
elicitationId="e1",
url="https://example.invalid/elicit",
message="need auth",
)
]
)
tool = self._tool(boom, "elicitation_tool")
async def _run():
return await tool.run({}, convert_result=True)
with self.assertRaises(UrlElicitationRequiredError):
asyncio.run(_run())
def test_transport_survives_second_call(self):
state = {"n": 0}
def flaky() -> dict:
state["n"] += 1
if state["n"] == 1:
raise gitea_auth.GiteaAuthError()
return {"ok": True, "call": state["n"]}
tool = self._tool(flaky, "gitea_whoami")
async def _both():
first = await tool.run({}, convert_result=True)
second = await tool.run({}, convert_result=True)
return first, second
first, second = asyncio.run(_both())
self.assertTrue(first.isError)
self.assertEqual(first.structuredContent["error_class"], "authentication")
self.assertFalse(getattr(second, "isError", False))
self.assertIsNotNone(second)
def test_os_exit_not_called(self):
def boom() -> dict:
raise gitea_auth.GiteaAuthError()
with patch("os._exit") as mock_exit:
result = asyncio.run(self._tool(boom).run({}, convert_result=True))
mock_exit.assert_not_called()
self.assertTrue(result.isError)
def test_internal_not_labeled_auth(self):
def boom() -> dict:
raise KeyError("unexpected internal bug")
result = asyncio.run(self._tool(boom).run({}, convert_result=True))
self.assertTrue(result.isError)
self.assertEqual(result.structuredContent["error_class"], "internal")
self.assertEqual(result.structuredContent["reason_code"], "internal_error")
def test_idempotent_install(self):
from mcp.server.fastmcp.tools.base import Tool
first = boundary.install_tool_run_boundary(Tool)
second = boundary.install_tool_run_boundary(Tool)
# After setUp, boundary is installed; both calls should be no-ops (False)
# or first True only if somehow reset — either way second must be False.
self.assertFalse(second)
self.assertTrue(boundary.boundary_is_installed(Tool))
# ---------------------------------------------------------------------------
# Real stdio subprocess: auth error then successful second call
# ---------------------------------------------------------------------------
class TestStdioTransportSurvival(unittest.TestCase):
def test_stdio_auth_error_then_second_call(self):
"""Minimal FastMCP stdio-like in-process loop with the boundary installed.
Uses Tool.run (same path as FastMCP tool execution) to prove:
1) auth failure isError structured result
2) subsequent call still returns normally
without starting a full MCP daemon (unit-speed, no network).
"""
from mcp.server.fastmcp.tools.base import Tool
boundary.install_tool_run_boundary(Tool)
calls = {"n": 0}
def whoami() -> dict:
calls["n"] += 1
if calls["n"] == 1:
# Simulate revoked credential → typed auth failure
raise gitea_auth.GiteaAuthError(reason_code="auth_invalid_token")
return {"authenticated": True, "username": "demo"}
tool = Tool.from_function(whoami, name="gitea_whoami")
async def session():
r1 = await tool.run({}, convert_result=True)
r2 = await tool.run({}, convert_result=True)
return r1, r2
r1, r2 = asyncio.run(session())
self.assertTrue(r1.isError)
self.assertEqual(r1.structuredContent["reason_code"], "auth_invalid_token")
self.assertTrue(r1.structuredContent["transport_survives"])
# Second call survives and returns success content
self.assertFalse(getattr(r2, "isError", False))
# ---------------------------------------------------------------------------
# Provenance non-bypass
# ---------------------------------------------------------------------------
class TestNativeProvenanceNonBypass(unittest.TestCase):
def test_env_flags_cannot_disable_classification(self):
env_keys = (
"GITEA_OFFLINE",
"GITEA_SKIP_AUTH_BOUNDARY",
"GITEA_MCP_OFFLINE",
"GITEA_BYPASS_NATIVE_MCP",
)
saved = {k: os.environ.get(k) for k in env_keys}
try:
for k in env_keys:
os.environ[k] = "1"
exc = gitea_auth.GiteaAuthError()
c = boundary.classify_exception(exc)
self.assertEqual(c["error_class"], "authentication")
r = boundary.to_call_tool_result(exc, log=False)
self.assertTrue(r.isError)
self.assertEqual(r.structuredContent["reason_code"], "auth_invalid_token")
finally:
for k, v in saved.items():
if v is None:
os.environ.pop(k, None)
else:
os.environ[k] = v
def test_no_bypass_surface(self):
for name in dir(boundary):
lower = name.lower()
self.assertFalse(
lower.startswith("bypass") or lower.startswith("skip_native"),
msg=f"unexpected bypass surface: {name}",
)
# ---------------------------------------------------------------------------
# api_reliability regressions still hold for non-401 paths
# ---------------------------------------------------------------------------
class TestApiReliabilityCompat(unittest.TestCase):
@patch("gitea_auth.urllib.request.urlopen")
def test_502_upstream_typed(self, mock_open):
mock_open.side_effect = http_error(502, "bad gateway secret=xyz")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(ctx.exception.reason_code, "upstream_unavailable")
self.assertNotIn("secret=xyz", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_auth_header_never_in_error(self, mock_open):
mock_open.side_effect = http_error(400, "bad request")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertNotIn(FAKE_AUTH, str(ctx.exception))
if __name__ == "__main__":
unittest.main()