Compare commits

..
Author SHA1 Message Date
sysadmin cc3ad0870a Merge pull request 'fix(validator): align final-report validator with canonical schema (Closes #698)' (#705) from fix/issue-698-report-validator-schema into master 2026-07-13 17:26:11 -05:00
sysadminandClaude Fable 5 ee90a5e7a2 fix(validator): align final-report validator with canonical schema (Closes #698)
Fixes the final-report validator defects from Issue #698 (original lead
plus the independent reproduction recorded during the PR #703 formal
review, comment 11246):

- Legacy fields: the review/merger required-field tables no longer demand
  'Pinned reviewed head', 'Scratch worktree used', 'Worktree path',
  'Worktree dirty', 'Mutations', 'Next', 'Issue/PR', 'Branch/SHA', or
  'Files changed' — names the canonical review-merge-final-report schema
  forbids or replaces. The canonical names ('Reviewed head SHA', 'Review
  worktree path/dirty', 'Safe next action', mutation categories) are
  required instead, with backward-compatible aliases where the schema
  permits them.
- Structured proof: workflow-load helper results are recognized in
  colon, key=value, and JSON renderings; validation pass proof is
  accepted anywhere in the Validation field value (for example
  'focused 50 passed; full 2665 passed').
- Mutation inference: review mutations are inferred only from
  authoritative evidence (performed=true and not gated); read-only
  diagnostics and pre-API rejections no longer count as mutations.
- Lease release vs cleanup: canonical reviewer lease release (release
  tool call or terminal phase=released marker) is lease lifecycle, not
  post-merge cleanup, and no longer triggers the branch/worktree cleanup
  checklist; genuine delete/remove claims still require full proof.
- Blocked reports: a legitimately blocked run that states an explicit
  'Reviewed/Candidate head SHA: none' with no verdict, merge, or started
  validation owes no head proofs; approval-time and merge-time live-head
  proofs are demanded only once the corresponding phase begins, and a
  report that states no head at all still fails closed.
- action_log robustness: malformed (non-dict) entries and non-list logs
  are reported as clear sanitized findings (position and type only, no
  content echo) instead of crashing with AttributeError; a defective
  validator rule now fails closed with a sanitized block finding rather
  than raising a secondary exception.

27 new regression tests, including canonical fixtures modeled on the
PR #703 formal-review handoff and the blocked preflight report from the
prior #698 reproductions. Two legacy-field test fixtures updated to the
canonical schema. Full suite: 2663 passed, 6 skipped, 161 subtests.

Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-13 17:37:13 -04:00
13 changed files with 745 additions and 1172 deletions
+94 -9
View File
@@ -134,16 +134,22 @@ _TARGET_BRANCH_SHA_RE = re.compile(
r"target branch sha\s*:\s*[0-9a-f]{40}",
re.IGNORECASE,
)
# #698: structured proof is rendered in several equivalent shapes —
# `workflow_hash: abc...`, `workflow_hash=abc...`, or JSON
# `"workflow_hash": "abc..."`. Recognize all of them; demanding one exact
# punctuation style rejects legitimate structured workflow-load proof.
_WORKFLOW_LOAD_HELPER_RE = re.compile(
r"workflow[- ]load helper result\s*:",
r"workflow[-_ ]load[-_ ]helper[-_ ]result\s*[:=]",
re.IGNORECASE,
)
_WORKFLOW_LOAD_HASH_RE = re.compile(
r"workflow[- ]load helper result[\s\S]{0,400}?workflow[_ ]hash\s*:\s*[0-9a-f]{12}",
r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?"
r"workflow[_ ]hash\"?\s*[:=]\s*\"?[0-9a-f]{12}",
re.IGNORECASE,
)
_WORKFLOW_LOAD_BOUNDARY_RE = re.compile(
r"workflow[- ]load helper result[\s\S]{0,400}?boundary[_ ]status\s*:\s*(?:clean|violation)",
r"workflow[-_ ]load[-_ ]helper[-_ ]result[\s\S]{0,400}?"
r"boundary[_ ]status\"?\s*[:=]\s*\"?(?:clean|violation)",
re.IGNORECASE,
)
_WORKFLOW_FILE_VIEW_NARRATIVE_RE = re.compile(
@@ -244,6 +250,59 @@ def _normalize_task_kind(task_kind: str | None) -> str:
return _TASK_KIND_ALIASES.get(raw, raw)
def _iter_action_entries(action_log: list | None) -> list[dict]:
"""Yield only structured (dict) action-log entries (#698).
Callers must never crash on malformed entries (strings, numbers, null)
that reach the validator from LLM-composed or partially parsed logs;
:func:`sanitize_action_log` reports them separately.
"""
return [e for e in (action_log or []) if isinstance(e, dict)]
def sanitize_action_log(
action_log: list | None,
) -> tuple[list[dict], list[dict[str, str]]]:
"""Split an action log into structured entries and sanitized findings (#698).
Malformed entries become clear, sanitized ``warning`` findings — the
offending value's content is never echoed back (only its position and
type), so secrets or garbage in a broken log cannot leak into validation
errors, and validation itself proceeds without secondary exceptions.
"""
if action_log is None:
return [], []
if not isinstance(action_log, (list, tuple)):
return [], [
validator_finding(
"shared.action_log_malformed",
"downgrade",
"Action log",
"action_log is not a list of structured entries "
f"(got {type(action_log).__name__}); it was ignored",
"pass action_log as a list of dict entries",
)
]
entries: list[dict] = []
findings: list[dict[str, str]] = []
for index, entry in enumerate(action_log):
if isinstance(entry, dict):
entries.append(entry)
continue
findings.append(
validator_finding(
"shared.action_log_malformed",
"downgrade",
"Action log",
f"action_log entry {index} is not a structured mapping "
f"(got {type(entry).__name__}); the entry was ignored",
"repair the malformed action_log entry or drop it before "
"revalidating",
)
)
return entries, findings
def validator_finding(
rule_id: str,
severity: str,
@@ -421,7 +480,7 @@ def _rule_shared_canonical_comment_post_claim(
rejected_in_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text))
rejected_in_log = False
if action_log:
for entry in action_log:
for entry in _iter_action_entries(action_log):
validation = entry.get("canonical_comment_validation") or {}
if validation.get("allowed") is False:
rejected_in_log = True
@@ -475,9 +534,13 @@ def _rule_reviewer_vague_mutations_none(
action_log: list[dict] | None = None,
mutations_observed: bool = False,
) -> list[dict[str, str]]:
# #698: infer review mutations only from authoritative evidence — an
# entry proves a mutation only when it affirmatively records
# performed=true and was not gated. Read-only diagnostics and pre-API
# rejections (entries without a performed flag) are not mutations.
performed = any(
e.get("performed") is not False and not e.get("gated_rejected")
for e in (action_log or [])
e.get("performed") is True and not e.get("gated_rejected")
for e in _iter_action_entries(action_log)
)
if not (mutations_observed or performed):
return []
@@ -536,7 +599,7 @@ def _rule_reviewer_git_fetch_readonly(
text = report_text or ""
fetch_observed = any(
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
for e in (action_log or [])
for e in _iter_action_entries(action_log)
) or _GIT_FETCH_RE.search(text)
if not fetch_observed:
return []
@@ -977,7 +1040,7 @@ def _rule_reviewer_target_branch_freshness(
fields = _handoff_fields(text)
fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any(
_GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or ""))
for e in (action_log or [])
for e in _iter_action_entries(action_log)
)
target_sha_reported = bool(_TARGET_BRANCH_SHA_RE.search(text)) or any(
"target branch" in key and "sha" in key and _FULL_SHA_RE.search(value)
@@ -1735,6 +1798,12 @@ def assess_final_report_validator(
checks: dict[str, Any] = {}
findings: list[dict[str, str]] = []
# #698: malformed action_log data must never crash validation with a
# secondary exception; malformed entries surface as sanitized findings.
sanitized_action_log, action_log_findings = sanitize_action_log(action_log)
action_log = sanitized_action_log
findings.extend(action_log_findings)
if normalized_kind == "issue_filing" and issue_filing_lock is not None:
checks["issue_filing"] = assess_issue_filing_final_report(
report_text,
@@ -1763,7 +1832,23 @@ def assess_final_report_validator(
}
for rule in _RULES_BY_TASK.get(normalized_kind, ()):
findings.extend(_call_rule(rule, report_text, normalized_kind, rule_kwargs))
try:
findings.extend(
_call_rule(rule, report_text, normalized_kind, rule_kwargs)
)
except Exception as exc: # #698: fail closed with a sanitized error
findings.append(
validator_finding(
"shared.validator_rule_error",
"block",
"Validator",
f"validator rule '{getattr(rule, '__name__', 'unknown')}' "
f"failed with {type(exc).__name__} (details withheld; "
"sanitized)",
"file a validator defect with the rule name; do not "
"bypass final-report validation",
)
)
grade, blocked, downgraded = _aggregate_grade(findings)
reasons = [f"{f['rule_id']}: {f['reason']}" for f in findings]
+19 -207
View File
@@ -243,192 +243,6 @@ def _redact(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):
"""Return *url* with the given query parameters added or overridden.
@@ -501,24 +315,23 @@ def api_request(method, url, auth_header, payload=None, *,
"""Make an authenticated JSON request to the Gitea API.
Returns parsed JSON on success (or ``None`` for an empty body), and raises
a classified client error on failure.
``RuntimeError`` on failure.
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
using capped jittered exponential backoff. Successful responses are
unchanged.
Failures raise typed exceptions with **fixed messages only** (#699). HTTP
response bodies are read solely for local 403 reason refinement and are
never stored on exceptions or returned to callers:
All failures are converted to a ``RuntimeError`` with a clear, secret
-redacted message (no raw stack traces or credential material):
- HTTP 401 → :class:`GiteaAuthError` (``auth_invalid_token``)
- HTTP 403 → :class:`GiteaAuthzError` (scope or denied)
- 502/503/504 → :class:`GiteaHttpError` (``upstream_unavailable``)
- Other non-429 HTTP → :class:`GiteaHttpError` (``http_error``)
- Timeouts / DNS / ``URLError`` → :class:`GiteaNetworkError`
- Malformed success JSON → plain ``RuntimeError`` (programming/protocol;
not reclassified as authentication)
- Non-429 HTTP errors surface the status code and a redacted response body.
502/503/504 upstream errors get an explicit "Gitea upstream unavailable"
message.
- Timeouts and network/DNS failures (``URLError`` / ``TimeoutError``) surface
a generic "network error contacting Gitea" message.
- A malformed (non-JSON) success body surfaces a "malformed JSON response"
message rather than a raw decode error.
The ``*_func`` parameters and ``timeout`` are injection points for
deterministic testing.
@@ -556,23 +369,22 @@ def api_request(method, url, auth_header, payload=None, *,
error_body = e.read().decode("utf-8", errors="replace")
except Exception:
error_body = ""
# Classify from status (+ local body hint). Body is not embedded.
try:
raise_for_http_status(e.code, error_body)
except GiteaClientError:
raise
# Defensive: raise_for_http_status always raises.
raise GiteaHttpError(http_status=e.code) from e # pragma: no cover
detail = _redact(error_body).strip()
if e.code in (502, 503, 504):
msg = f"HTTP {e.code}: Gitea upstream unavailable"
raise RuntimeError(f"{msg}: {detail}" if detail else msg) from e
raise RuntimeError(f"HTTP {e.code}: {detail}") from e
except (urllib.error.URLError, TimeoutError) as e:
# Fixed message only — do not embed URLError reason (may leak paths).
raise GiteaNetworkError(reason_code="network_error") from e
reason = getattr(e, "reason", e)
raise RuntimeError(
f"network error contacting Gitea: {_redact(reason)}"
) from e
if not body:
return None
try:
return json.loads(body)
except ValueError as e:
# Programming/protocol failure — not authentication.
raise RuntimeError("malformed JSON response from Gitea") from e
+5 -16
View File
@@ -1084,9 +1084,7 @@ from gitea_auth import ( # noqa: E402
repo_api_url,
get_profile,
gitea_url,
GiteaConfigError,
)
import mcp_tool_error_boundary # noqa: E402
import gitea_audit # noqa: E402
import gitea_config # noqa: E402
import capability_stop_terminal # noqa: E402
@@ -1466,12 +1464,6 @@ def _with_optional_url(result: dict, url: str | None) -> dict:
result["url"] = url
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=(
"Gitea issue tracker and PR management for dadeschools and prgs instances. "
"Use the gitea_ prefixed tools to create issues, PRs, list issues, etc."
@@ -1809,16 +1801,13 @@ def _enforce_remote_repo_guard(
def _auth(host: str) -> str:
"""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).
"""
"""Get auth header, raise if unavailable."""
header = get_auth_header(host)
if header is None:
raise GiteaConfigError(reason_code="config_error")
raise RuntimeError(
f"No credentials for {host}. "
"Ensure you've logged in via HTTPS at least once."
)
return header
-451
View File
@@ -1,451 +0,0 @@
"""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))
+27 -8
View File
@@ -86,6 +86,22 @@ _WRONG_BRANCH_RE = re.compile(
r"deleted branch (?:does not match|!=|differs from) (?:merged )?pr head",
re.IGNORECASE,
)
# #698: canonical reviewer lease lifecycle operations are NOT post-merge
# cleanup. Releasing a reviewer PR lease (or posting its terminal
# phase=released marker) happens after every review — merged or not — and
# must never trigger the post-merge branch/worktree cleanup checklist.
_LEASE_LIFECYCLE_RE = re.compile(
r"(?:gitea_release_reviewer_pr_lease|gitea_abandon_workflow_lease|"
r"release[d]? (?:the )?(?:reviewer|workflow) (?:pr )?lease|"
r"reviewer (?:pr )?lease release[d]?|"
r"lease (?:marker|comment).{0,40}phase\s*[:=]\s*released|"
r"phase\s*[:=]\s*released)",
re.IGNORECASE,
)
_CLEANUP_MUTATIONS_VALUE_RE = re.compile(
r"cleanup mutations\s*:\s*([^\n]+)",
re.IGNORECASE,
)
def _claims_remote_delete(text: str) -> bool:
@@ -204,16 +220,19 @@ def assess_post_merge_cleanup_proof(
for field in _worktree_cleanup_fields_present(text)
)
if (remote_delete or worktree_remove) and not (remote_delete or worktree_remove):
pass
if not remote_delete and not worktree_remove:
cleanup_mutations = re.search(
r"cleanup mutations\s*:\s*(?!none\b)\S",
text,
re.IGNORECASE,
value_match = _CLEANUP_MUTATIONS_VALUE_RE.search(text)
value = (value_match.group(1).strip() if value_match else "")
value_lower = value.lower()
substantive = bool(value) and value_lower not in {
"none", "n/a", "not applicable",
}
# #698: reviewer lease release / terminal lease markers are lease
# lifecycle, not post-merge cleanup — no checklist owed.
lease_lifecycle_only = substantive and bool(
_LEASE_LIFECYCLE_RE.search(value)
)
if cleanup_mutations:
if substantive and not lease_lifecycle_only:
reasons.append(
"cleanup mutations reported without post-merge cleanup proof checklist"
)
+67 -6
View File
@@ -403,10 +403,37 @@ _REVIEWER_ACTIVE_RE = re.compile(
r"whether any reviewer was active\s*:\s*(yes|no|true|false)",
re.IGNORECASE,
)
# #698 phase detection for phase-specific head proofs.
_NO_REVIEWED_HEAD_RE = re.compile(
r"(?:reviewed head sha|candidate head sha)\s*:\s*none\b",
re.IGNORECASE,
)
_VERDICT_RECORDED_RE = re.compile(
r"review decision\s*:\s*(?:approve[d]?|request[_ ]changes)\b"
r"|review_status\s*:\s*(?:approved|request_changes)\b"
r"|terminal review mutation\s*:\s*(?!none\b)\S",
re.IGNORECASE,
)
_MERGE_ATTEMPTED_RE = re.compile(
r"merge result\s*:\s*(?:merged|success|performed|failed|attempted)\b"
r"|merge mutations\s*:\s*(?!none\b|not applicable\b)\S",
re.IGNORECASE,
)
_VALIDATION_STARTED_RE = re.compile(
r"validation\s*:\s*(?!none\b|not run\b|not applicable\b|not started\b)"
r"[^\n]*(?:pass|fail|ran|executed|\d+\s+passed)",
re.IGNORECASE,
)
def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]:
"""Final-report proof for reviewed vs live head SHAs (#399 AC 6)."""
"""Final-report proof for reviewed vs live head SHAs (#399 AC 6).
#698: head proofs are phase-specific. A legitimately blocked run that
never began validation (no reviewed head, no formal verdict, no merge)
owes none of them; approval-time and merge-time live-head proofs are
owed only once the corresponding phase actually begins.
"""
text = report_text or ""
reasons: list[str] = []
reviewed = _normalize_sha(_REVIEWED_HEAD_RE.search(text).group(1) if _REVIEWED_HEAD_RE.search(text) else None)
@@ -422,15 +449,49 @@ def assess_reviewer_stale_head_final_report(report_text: str) -> dict[str, Any]:
)
push_during = _PUSH_DURING_VALIDATION_RE.search(text)
if not reviewed:
# Phase detection from the report's own claims.
no_head_stated = bool(_NO_REVIEWED_HEAD_RE.search(text))
verdict_recorded = bool(_VERDICT_RECORDED_RE.search(text))
merge_attempted = bool(_MERGE_ATTEMPTED_RE.search(text))
validation_started = bool(reviewed) or bool(_VALIDATION_STARTED_RE.search(text))
blocked_before_validation = (
no_head_stated
and not reviewed
and not verdict_recorded
and not merge_attempted
and not validation_started
)
if blocked_before_validation:
return {
"proven": True,
"block": False,
"reasons": [],
"reviewed_head_sha": None,
"live_head_sha_before_approval": None,
"live_head_sha_before_merge": None,
"push_during_validation": (
push_during.group(1).lower() if push_during else None
),
"phase": "blocked_before_validation",
}
if not reviewed and not no_head_stated:
# The head must always be STATED — either a SHA or an explicit
# 'none'. Silence is not a phase claim and fails closed.
reasons.append(
"reviewed head SHA not stated in final report "
"(state the SHA or an explicit 'none')"
)
elif not reviewed and (validation_started or verdict_recorded or merge_attempted):
reasons.append("reviewed head SHA not stated in final report")
if not live_approval:
if verdict_recorded and not live_approval:
reasons.append("final live head SHA before approval not stated")
if not live_merge:
if merge_attempted and not live_merge:
reasons.append("final live head SHA before merge not stated")
if not push_during:
if validation_started and not push_during:
reasons.append("whether push occurred during validation not stated")
elif reviewed and live_approval and reviewed != live_approval:
if reviewed and live_approval and reviewed != live_approval:
reasons.append("live head before approval differs from reviewed head SHA")
elif reviewed and live_merge and reviewed != live_merge:
reasons.append("live head before merge differs from reviewed head SHA")
+6 -1
View File
@@ -25,8 +25,13 @@ _REVIEWED_HEAD_RE = re.compile(
r"(?:pinned reviewed head|reviewed head sha)\s*:\s*([0-9a-f]{7,40})",
re.IGNORECASE,
)
# #698: validation pass proof appears in several legitimate shapes —
# "Validation: pass", "Validation: focused 50 passed; full 2665 passed",
# or structured "validation_status: pass". Accept pass evidence anywhere in
# the Validation field's value, not only as its first token.
_VALIDATION_PASS_RE = re.compile(
r"validation\s*:\s*(?:pass|passed|strong|ok|green)",
r"validation(?:_status)?\s*:[^\n]{0,300}?"
r"(?:\bpass(?:ed)?\b|\bstrong\b|\bok\b|\bgreen\b|\d+\s+passed)",
re.IGNORECASE,
)
_MERGED_CLAIM_RE = re.compile(
+36 -9
View File
@@ -858,9 +858,16 @@ _WALKTHROUGH_ARTIFACT_RE = re.compile(r"walkthrough\.md", re.I)
def _performed_file_mutations(action_log: list[dict] | None) -> list[dict]:
"""Return performed local file mutations, excluding gated rejections."""
"""Return performed local file mutations, excluding gated rejections.
Non-dict entries (malformed JSON, LLM mistakes) are ignored instead of
raising ``AttributeError`` (#698): a malformed ledger entry can never be
authoritative mutation evidence.
"""
performed: list[dict] = []
for entry in action_log or []:
if not isinstance(entry, dict):
continue
if entry.get("gated_rejected") or entry.get("performed") is False:
continue
action = (entry.get("action") or "").strip().lower()
@@ -2228,24 +2235,31 @@ HANDOFF_REVIEW_MUTATION_FIELDS = (
)
HANDOFF_ROLE_FIELDS = {
# #698: the review/merger required-field sets must stay aligned with the
# canonical schema (skills/llm-project-workflow/schemas/
# review-merge-final-report.md). The schema explicitly FORBIDS the legacy
# fields 'Pinned reviewed head', 'Scratch worktree used', and 'Workspace
# mutations' — a validator must never demand a field the schema bans.
"review": (
("Selected PR", ("selected pr",)),
("Reviewer eligibility", ("reviewer eligibility", "eligibility")),
("Pinned reviewed head", ("pinned reviewed head", "pinned head")),
("Worktree path", ("worktree path", "starting worktree path")),
("Worktree dirty", ("worktree dirty", "whether worktree was dirty")),
("Scratch worktree used", ("scratch worktree used", "scratch clone used",
"scratch worktree")),
("Reviewed head SHA", ("reviewed head sha", "candidate head sha")),
("Review worktree path", ("review worktree path", "worktree path",
"starting worktree path")),
("Review worktree dirty", ("review worktree dirty", "worktree dirty",
"whether worktree was dirty")),
("Unrelated local mutations", ("unrelated local mutations",
"unrelated files modified")),
"unrelated files modified",
"file edits by reviewer")),
("Review decision", ("review decision", "decision")),
("Merge result", ("merge result",)),
("Linked issue status", ("linked issue status", "linked issue")),
("Cleanup status", ("cleanup status", "cleanup")),
("Safe next action", ("safe next action", "next")),
) + HANDOFF_REVIEW_MUTATION_FIELDS,
"merger": (
("Selected PR", ("selected pr",)),
("Pinned reviewed head", ("pinned reviewed head", "pinned head")),
("Reviewed head SHA", ("reviewed head sha", "candidate head sha")),
("Active profile", ("active profile",)),
("Role kind", ("role kind",)),
("Merge capability source", ("merge capability source",)),
@@ -2433,9 +2447,22 @@ def assess_controller_handoff(report_text, role=None, local_edits=False):
# Issue #320: reviewer and merger handoffs use the precise mutation categories
# in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous
# "Workspace mutations" field, which is rejected below.
# Issue #698: the canonical review-merge schema has no 'Mutations',
# 'Next', 'Issue/PR', 'Branch/SHA', or 'Files changed' fields — their
# content lives in the precise mutation categories, 'Safe next
# action', 'Selected PR'/'Linked issue', head-SHA fields, and 'Files
# reviewed'. Requiring the legacy names rejects canonical reports.
_non_canonical_for_review = {
"Workspace mutations",
"Mutations",
"Next",
"Issue/PR",
"Branch/SHA",
"Files changed",
}
required = [
field for field in required
if field[0] != "Workspace mutations"
if field[0] not in _non_canonical_for_review
]
if any(label.startswith("workspace mutations") for label in labels):
return {
+16 -22
View File
@@ -79,46 +79,41 @@ class TestApiRequestFailures(unittest.TestCase):
@patch("gitea_auth.urllib.request.urlopen")
def test_timeout_converted_to_runtimeerror(self, mock_open):
mock_open.side_effect = TimeoutError("timed out")
with self.assertRaises(gitea_auth.GiteaNetworkError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
# Fixed message only (#699) — still a RuntimeError subclass.
self.assertIsInstance(ctx.exception, RuntimeError)
self.assertEqual(str(ctx.exception), "Network error contacting Gitea")
self.assertIn("network error contacting Gitea", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_dns_network_failure_converted(self, mock_open):
mock_open.side_effect = urllib.error.URLError("Name or service not known")
with self.assertRaises(gitea_auth.GiteaNetworkError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(str(ctx.exception), "Network error contacting Gitea")
self.assertIn("network error contacting Gitea", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_502_upstream_message(self, mock_open):
mock_open.side_effect = http_error(502, "bad gateway")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
msg = str(ctx.exception)
self.assertEqual(msg, "Gitea upstream unavailable")
self.assertEqual(ctx.exception.reason_code, "upstream_unavailable")
self.assertEqual(ctx.exception.http_status, 502)
self.assertIn("HTTP 502", msg)
self.assertIn("upstream unavailable", msg)
@patch("gitea_auth.urllib.request.urlopen")
def test_503_upstream_message(self, mock_open):
mock_open.side_effect = http_error(503, "")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(str(ctx.exception), "Gitea upstream unavailable")
self.assertEqual(ctx.exception.http_status, 503)
self.assertIn("HTTP 503", str(ctx.exception))
self.assertIn("upstream unavailable", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_malformed_error_payload_does_not_crash(self, mock_open):
# Non-JSON garbage error body must still yield a clean typed error
# with a fixed message (no body echo — #699).
# Non-JSON garbage error body must still yield a clean RuntimeError.
mock_open.side_effect = http_error(500, "<html>garbage</html>")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertEqual(str(ctx.exception), "Gitea HTTP request failed")
self.assertNotIn("<html>", str(ctx.exception))
self.assertIn("HTTP 500", str(ctx.exception))
@patch("gitea_auth.urllib.request.urlopen")
def test_malformed_success_json_raises_clean_error(self, mock_open):
@@ -131,17 +126,16 @@ class TestApiRequestFailures(unittest.TestCase):
def test_no_secret_leak_in_error_body(self, mock_open):
mock_open.side_effect = http_error(
400, "failed: token supersecret123 rejected")
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
msg = str(ctx.exception)
self.assertNotIn("supersecret123", msg)
# Fixed message — body never appears (stronger than redaction).
self.assertEqual(msg, "Gitea HTTP request failed")
self.assertIn(gitea_audit.REDACTED, msg)
@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:
with self.assertRaises(RuntimeError) as ctx:
gitea_auth.api_request("GET", URL, FAKE_AUTH)
self.assertNotIn(FAKE_AUTH, str(ctx.exception))
@@ -0,0 +1,461 @@
"""Regression tests for #698: final-report validator vs canonical schema.
Covers the original #698 lead plus the independent reproduction recorded
during the PR #703 formal review (issue #698 comment 11246):
1. non-dict ``action_log`` entries must fail structured, never crash;
2. the validator must not demand legacy fields the canonical schema forbids
(``Pinned reviewed head``, ``Scratch worktree used``, ``Worktree path``,
``Worktree dirty``, ``Mutations``, ``Next``);
3. a legitimately blocked report (``Candidate head SHA: none``, no formal
verdict) must not owe approval/merge live-head proofs;
4. canonical reviewer lease release must not be misclassified as post-merge
cleanup;
5. structured workflow-load and validation proof must be recognized;
6. review mutations are inferred only from authoritative evidence.
"""
from __future__ import annotations
import sys
import unittest
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
import final_report_validator as frv # noqa: E402
import post_merge_cleanup_proof as pmcp # noqa: E402
import pr_work_lease as pwl # noqa: E402
import review_proofs as rp # noqa: E402
from review_final_report_schema import ( # noqa: E402
assess_review_final_report_schema,
)
REVIEWED_HEAD = "a" * 40
LIVE_HEAD = "a" * 40
def _pr703_style_report(
*,
decision: str = "request_changes",
cleanup_mutations: str = (
"released reviewer PR lease via gitea_release_reviewer_pr_lease "
"(terminal lease marker phase=released posted)"
),
) -> str:
"""Canonical-schema report modeled on the PR #703 formal review handoff."""
return f"""
Formal review completed with a REQUEST_CHANGES verdict submitted and read
back via the native review API.
## Controller Handoff
- Task: review-merge-pr
- Repo: Scaled-Tech-Consulting/Gitea-Tools
- Role: reviewer
- Identity: sysadmin / prgs-reviewer
- Active profile: prgs-reviewer
- Runtime context: neutral workspace binding
- Selected PR: 703
- Linked issue: #702 open
- Eligibility class: reviewable
- Queue ordering policy: oldest eligible first
- Inventory pagination proof: has_more=false, total_count=8
- Earlier PRs skipped: none
- Candidate head SHA: {REVIEWED_HEAD}
- Reviewed head SHA: {REVIEWED_HEAD}
- Target branch: master
- Target branch SHA: {"2" * 40}
- Already-landed gate: not landed
- Author-safety result: pass (author differs from reviewer)
- Prior request-changes state: none
- Review worktree used: true
- Review worktree path: branches/review-pr-703-independent
- Review worktree inside branches: true
- Review worktree HEAD state: detached at pinned head
- Review worktree dirty before validation: clean
- Review worktree dirty after validation: clean
- Baseline worktree used: false
- Baseline worktree path: none
- Files reviewed: 4
- Validation: focused 50 passed; related 94 passed; full 2665 passed, 6 skipped
- Official validation integrity status: intact
- Terminal review mutation: one REQUEST_CHANGES review submitted and read back
- Review decision: {decision}
- Merge preflight: not run
- Merge result: none
- Linked issue status: open (live fetch proof: gitea_view_issue)
- Main checkout branch: master
- Main checkout dirty state: clean
- Main checkout updated: false
- File edits by reviewer: none
- Worktree/index mutations: none
- Git ref mutations: git fetch prgs (recorded)
- MCP/Gitea mutations: review submission and lease comments only
- Review mutations: one formal REQUEST_CHANGES verdict
- Merge mutations: none
- Cleanup mutations: {cleanup_mutations}
- External-state mutations: none
- Read-only diagnostics: gitea_view_pr, gitea_get_pr_review_feedback
- Blockers: findings F1-F6 recorded on the PR thread
- Current status: review complete; author remediation required
- Safe next action: author addresses findings and pushes a new head
- Safety statement: no merge attempted; no self-review; no root-checkout edits
- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean
- Live head SHA before approval: {LIVE_HEAD}
- Pushes occurred during validation: no
"""
def _blocked_preflight_report() -> str:
"""Blocked-run report modeled on the #702 comment 11164 reproduction."""
return """
Fresh review preflight stopped before any worktree or validation work.
## Controller Handoff
- Task: review-merge-pr
- Repo: Scaled-Tech-Consulting/Gitea-Tools
- Role: reviewer
- Identity: sysadmin / prgs-reviewer
- Active profile: prgs-reviewer
- Runtime context: stale workspace binding detected
- Selected PR: 701
- Linked issue: #699 open
- Eligibility class: blocked-before-validation
- Queue ordering policy: oldest eligible first
- Inventory pagination proof: has_more=false, total_count=8
- Earlier PRs skipped: none
- Candidate head SHA: none
- Reviewed head SHA: none
- Target branch: master
- Target branch SHA: none
- Already-landed gate: not run
- Author-safety result: not run
- Prior request-changes state: none
- Review worktree used: false
- Review worktree path: none
- Review worktree inside branches: not applicable
- Review worktree HEAD state: not applicable
- Review worktree dirty before validation: not applicable
- Review worktree dirty after validation: not applicable
- Baseline worktree used: false
- Baseline worktree path: none
- Files reviewed: 0
- Validation: not run
- Official validation integrity status: not applicable
- Terminal review mutation: none
- Review decision: none
- Merge preflight: not run
- Merge result: none
- Linked issue status: open (live fetch proof: gitea_view_issue)
- Main checkout branch: master
- Main checkout dirty state: clean
- Main checkout updated: false
- File edits by reviewer: none
- Worktree/index mutations: none
- Git ref mutations: none
- MCP/Gitea mutations: none
- Review mutations: none
- Merge mutations: none
- Cleanup mutations: none
- External-state mutations: none
- Read-only diagnostics: gitea_view_pr, gitea_get_runtime_context
- Blockers: runtime bound to a foreign task worktree; mutation prohibited
- Current status: stopped before validation began
- Next actor: operator
- Next action: repair the runtime workspace binding, then rerun the full
review workflow in a fresh reviewer session
- Next prompt: Act as REVIEWER for PR 701 after the operator repairs the
runtime binding; acquire the lease before any validation.
- Safe next action: operator repairs runtime binding, then a fresh reviewer
reruns the full workflow
- Safety statement: no lease acquired; no verdict recorded; no source edits
- Workflow-load helper result: workflow_hash=da045d1e1f1f boundary_status=clean
"""
class TestActionLogRobustness(unittest.TestCase):
"""#698 original lead: non-dict action_log must not crash validation."""
MALFORMED = [
"git fetch prgs",
42,
None,
{"action": "edit", "path": "x.py", "performed": True, "tracked": True},
]
def test_assess_final_report_validator_survives_malformed_entries(self):
result = frv.assess_final_report_validator(
_pr703_style_report(),
"review_pr",
action_log=self.MALFORMED,
)
self.assertIsInstance(result, dict)
rule_ids = {f["rule_id"] for f in result["findings"]}
self.assertIn("shared.action_log_malformed", rule_ids)
def test_malformed_entry_errors_are_sanitized(self):
_entries, findings = frv.sanitize_action_log(["secret-token-abc123"])
self.assertEqual(len(findings), 1)
reason = findings[0]["reason"]
self.assertNotIn("secret-token-abc123", reason)
self.assertIn("str", reason)
self.assertIn("entry 0", reason)
def test_non_list_action_log_is_reported_not_raised(self):
entries, findings = frv.sanitize_action_log("not-a-list")
self.assertEqual(entries, [])
self.assertEqual(len(findings), 1)
self.assertIn("not a list", findings[0]["reason"])
def test_performed_file_mutations_skips_non_dict_entries(self):
performed = rp._performed_file_mutations(
["oops", {"action": "edited", "path": "a.py"}]
)
self.assertEqual(len(performed), 1)
self.assertEqual(performed[0]["path"], "a.py")
def test_schema_entrypoint_survives_string_only_log(self):
result = assess_review_final_report_schema(
_pr703_style_report(),
action_log=["just a string", "another string"],
)
self.assertIsInstance(result, dict)
class TestLegacyFieldRequirementsRemoved(unittest.TestCase):
"""#698: prohibited legacy fields must not be REQUIRED of reports."""
PROHIBITED = (
"Pinned reviewed head",
"Scratch worktree used",
"Worktree path",
"Worktree dirty",
"Workspace mutations",
"Mutations",
"Next",
"Issue/PR",
"Branch/SHA",
"Files changed",
)
def test_review_role_field_table_has_no_prohibited_requirements(self):
names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["review"]]
for prohibited in ("Pinned reviewed head", "Scratch worktree used",
"Worktree path", "Worktree dirty"):
self.assertNotIn(prohibited, names)
def test_merger_role_field_table_has_no_pinned_reviewed_head(self):
names = [name for name, _ in rp.HANDOFF_ROLE_FIELDS["merger"]]
self.assertNotIn("Pinned reviewed head", names)
def test_canonical_report_missing_fields_never_include_prohibited(self):
result = rp.assess_controller_handoff(
_pr703_style_report(), role="review"
)
for prohibited in self.PROHIBITED:
self.assertNotIn(prohibited, result.get("missing_fields") or [])
def test_canonical_pr703_report_satisfies_required_fields(self):
result = rp.assess_controller_handoff(
_pr703_style_report(), role="review"
)
self.assertEqual(result.get("missing_fields") or [], [])
self.assertEqual(result.get("verdict"), "complete")
class TestBlockedReportAccepted(unittest.TestCase):
"""#698: blocked run with no reviewed head / verdict is legitimate."""
def test_stale_head_proof_waived_before_validation(self):
result = pwl.assess_reviewer_stale_head_final_report(
_blocked_preflight_report()
)
self.assertTrue(result["proven"])
self.assertEqual(result.get("phase"), "blocked_before_validation")
def test_blocked_report_passes_schema_validation(self):
result = assess_review_final_report_schema(_blocked_preflight_report())
blocking = [
f for f in result["findings"] if f["severity"] == "block"
]
self.assertEqual(blocking, [], blocking)
def test_verdict_phase_still_demands_approval_head_proof(self):
report = _blocked_preflight_report().replace(
"- Review decision: none",
"- Review decision: approve",
).replace(
"- Candidate head SHA: none",
f"- Candidate head SHA: {REVIEWED_HEAD}",
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
joined = " ".join(result["reasons"])
self.assertIn("before approval", joined)
def test_merge_phase_still_demands_merge_head_proof(self):
report = _pr703_style_report().replace(
"- Merge result: none",
"- Merge result: merged",
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
self.assertIn(
"final live head SHA before merge not stated",
result["reasons"],
)
def test_validation_phase_demands_push_disclosure(self):
report = _pr703_style_report().replace(
"- Pushes occurred during validation: no\n", ""
)
result = pwl.assess_reviewer_stale_head_final_report(report)
self.assertFalse(result["proven"])
self.assertIn(
"whether push occurred during validation not stated",
result["reasons"],
)
class TestLeaseReleaseVsPostMergeCleanup(unittest.TestCase):
"""#698 (PR #703 review reproduction): lease release is not cleanup."""
def test_lease_release_cleanup_mutations_do_not_demand_checklist(self):
result = pmcp.assess_post_merge_cleanup_proof(_pr703_style_report())
self.assertFalse(result["block"], result["reasons"])
def test_release_tool_name_alone_is_recognized(self):
report = _pr703_style_report(
cleanup_mutations="gitea_release_reviewer_pr_lease comment 11244"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertFalse(result["block"], result["reasons"])
def test_substantive_non_lease_cleanup_still_demands_checklist(self):
report = _pr703_style_report(
cleanup_mutations="deleted stale scratch directory manually"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertTrue(result["block"])
def test_remote_branch_delete_claims_still_demand_full_proof(self):
report = _pr703_style_report(
cleanup_mutations="gitea_delete_branch removed the remote branch"
)
result = pmcp.assess_post_merge_cleanup_proof(report)
self.assertTrue(result["block"])
self.assertTrue(
any("remote branch deletion missing" in r for r in result["reasons"])
)
def test_full_schema_run_accepts_lease_release_report(self):
result = assess_review_final_report_schema(_pr703_style_report())
lease_cleanup_blocks = [
f for f in result["findings"]
if f["rule_id"] == "reviewer.post_merge_cleanup_proof"
]
self.assertEqual(lease_cleanup_blocks, [], lease_cleanup_blocks)
class TestStructuredProofRecognition(unittest.TestCase):
"""#698: structured workflow-load and validation proof must be accepted."""
def test_key_value_workflow_proof_recognized(self):
findings = frv._rule_reviewer_workflow_load_boundary(
_pr703_style_report()
)
self.assertEqual(findings, [], findings)
def test_colon_form_workflow_proof_still_recognized(self):
report = _pr703_style_report().replace(
"- Workflow-load helper result: workflow_hash=da045d1e1f1f "
"boundary_status=clean",
"- Workflow-load helper result: workflow_hash: da045d1e1f1f, "
"boundary_status: clean",
)
findings = frv._rule_reviewer_workflow_load_boundary(report)
self.assertEqual(findings, [], findings)
def test_incomplete_structured_proof_still_blocks(self):
report = _pr703_style_report().replace(
"workflow_hash=da045d1e1f1f boundary_status=clean",
"workflow_hash=da045d1e1f1f",
)
findings = frv._rule_reviewer_workflow_load_boundary(report)
self.assertTrue(findings)
self.assertIn("boundary_status", findings[0]["reason"])
def test_validation_counts_accepted_as_pass_proof(self):
# "Validation: focused 50 passed; ..." must satisfy the reviewed-head
# validation-proof rule (PR #703 reproduction).
result = assess_review_final_report_schema(_pr703_style_report())
head_blocks = [
f for f in result["findings"]
if f["rule_id"] == "reviewer.reviewed_head_without_validation"
]
self.assertEqual(head_blocks, [], head_blocks)
class TestAuthoritativeMutationInference(unittest.TestCase):
"""#698: review mutations inferred only from authoritative evidence."""
def test_read_only_entries_do_not_imply_mutations(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[
{"action": "gitea_view_pr"},
{"action": "gitea_get_pr_review_feedback", "performed": False},
],
)
self.assertEqual(findings, [], findings)
def test_performed_mutation_still_blocks_vague_none(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[{"action": "edit", "path": "a.py", "performed": True}],
)
self.assertTrue(findings)
def test_gated_rejection_is_not_a_mutation(self):
report = "## Controller Handoff\n- Mutations: none\n"
findings = frv._rule_reviewer_vague_mutations_none(
report,
action_log=[
{"action": "edit", "path": "a.py", "performed": True,
"gated_rejected": True},
],
)
self.assertEqual(findings, [], findings)
class TestValidatorRuleErrorContainment(unittest.TestCase):
"""#698: a defective rule fails closed with a sanitized error."""
def test_rule_exception_becomes_sanitized_block_finding(self):
def _boom(report_text):
raise ValueError("raw secret detail that must not leak")
original = frv._RULES_BY_TASK["review_pr"]
frv._RULES_BY_TASK["review_pr"] = [_boom]
try:
result = frv.assess_final_report_validator(
"report body", "review_pr"
)
finally:
frv._RULES_BY_TASK["review_pr"] = original
self.assertTrue(result["blocked"])
finding = next(
f for f in result["findings"]
if f["rule_id"] == "shared.validator_rule_error"
)
self.assertNotIn("raw secret detail", finding["reason"])
self.assertIn("ValueError", finding["reason"])
self.assertIn("_boom", finding["reason"])
if __name__ == "__main__":
unittest.main()
+4 -9
View File
@@ -122,11 +122,9 @@ class TestApiRequestRetry(unittest.TestCase):
sleep.assert_not_called()
def test_non_429_error_raises_immediately(self):
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
_call([_http_error(500, body=b"boom")])
# Fixed message only (#699) — no response body echo.
self.assertEqual(str(ctx.exception), "Gitea HTTP request failed")
self.assertEqual(ctx.exception.http_status, 500)
self.assertIn("HTTP 500", str(ctx.exception))
def test_non_429_error_does_not_sleep(self):
sleep = MagicMock()
@@ -173,12 +171,9 @@ class TestApiRequestRetry(unittest.TestCase):
# max_retries=3 -> 3 sleeps, then the 4th failure raises.
errors = [_http_error(429, retry_after="1") for _ in range(4)]
sleep = MagicMock()
with self.assertRaises(gitea_auth.GiteaHttpError) as ctx:
with self.assertRaises(RuntimeError) as ctx:
_call(errors, sleep_func=sleep, max_retries=3)
# 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.assertIn("HTTP 429", str(ctx.exception))
self.assertEqual(sleep.call_count, 3)
def test_no_infinite_loop_when_always_429(self):
+10 -8
View File
@@ -957,17 +957,20 @@ class TestControllerHandoff(unittest.TestCase):
if not line.startswith("- Workspace mutations:"))
result = assess_controller_handoff(review_base, role="review")
self.assertEqual(result["verdict"], "incomplete")
self.assertIn("Pinned reviewed head", result["missing_fields"])
self.assertIn("Worktree path", result["missing_fields"])
# #698: the canonical schema forbids the legacy fields, so the
# validator must demand the canonical names instead.
self.assertIn("Reviewed head SHA", result["missing_fields"])
self.assertIn("Review worktree path", result["missing_fields"])
self.assertIn("Merge result", result["missing_fields"])
for legacy in ("Pinned reviewed head", "Scratch worktree used"):
self.assertNotIn(legacy, result["missing_fields"])
complete = review_base + "\n" + "\n".join([
"- Selected PR: #999",
"- Reviewer eligibility: passed",
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Worktree path: /repo/branches/review-pr-999",
"- Worktree dirty: no",
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Review worktree path: /repo/branches/review-pr-999",
"- Review worktree dirty before validation: no",
"- Unrelated local mutations: none",
"- Review decision: approve",
"- Merge result: merged",
@@ -1125,10 +1128,9 @@ class TestReviewHandoffPreciseMutationCategories(unittest.TestCase):
"- Safety: no self-review; no self-merge; no secrets",
"- Selected PR: #999",
"- Reviewer eligibility: passed",
"- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9",
"- Worktree path: /repo/branches/review-pr-999",
"- Worktree dirty: no",
"- Scratch worktree used: yes (/repo/branches/review-pr-999)",
"- Unrelated local mutations: none",
"- Review decision: approve",
"- Merge result: none",
-426
View File
@@ -1,426 +0,0 @@
"""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()