diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index d32a213..70f9d0d 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -1824,6 +1824,8 @@ _UNSET = object() # Best-effort identity cache keyed by host, so an enabled audit trail resolves # the authenticated username at most once per host per process. _IDENTITY_CACHE: dict = {} +# Stable actor identity (id + login) for recovery evidence binding (#709 F7). +_ACTOR_IDENTITY_CACHE: dict = {} def _authenticated_username(host: str): @@ -1846,6 +1848,33 @@ def _authenticated_username(host: str): return user +def _authenticated_actor(host: str) -> dict: + """Resolve the authenticated actor's stable identity (#709 F7 review 438). + + Display names are mutable, so recovery evidence is bound to the immutable + numeric user id with the login carried alongside for consistency checks. + Read-only and fail-soft; never surfaces credential material. + """ + cached = _ACTOR_IDENTITY_CACHE.get(host) + if cached is not None: + return dict(cached) + actor: dict = {"user_id": None, "login": None} + try: + header = get_auth_header(host) + if header: + who = api_request("GET", gitea_url(host, "/api/v1/user"), header) + if isinstance(who, dict): + raw_id = who.get("id") + actor = { + "user_id": int(raw_id) if isinstance(raw_id, int) else None, + "login": (who.get("login") or None), + } + except Exception: + actor = {"user_id": None, "login": None} + _ACTOR_IDENTITY_CACHE[host] = dict(actor) + return dict(actor) + + def _ensure_matching_profile(required_permission: str, required_role: str, remote: str | None, host: str | None = None) -> str | None: """Check if the active profile is allowed to perform *required_permission*. If not, automatically switch to the first matching usable configured profile. @@ -1891,6 +1920,7 @@ def _ensure_matching_profile(required_permission: str, required_role: str, remot h = host or (REMOTES.get(remote, {}).get("host") if remote in REMOTES else None) if h: _IDENTITY_CACHE.pop(h, None) + _ACTOR_IDENTITY_CACHE.pop(h, None) username = _authenticated_username(h) if h else None # Update mutation authority global _MUTATION_AUTHORITY @@ -4658,28 +4688,108 @@ def _clear_decision_lock_for_profile( ), } - # Archive then clear — only after exact identity validation. + # Archive then clear — archival is a hard prerequisite (#709 F8 review 438). + # A failed, empty, or unconfirmed archive must never be followed by a clear: + # that is exactly the terminal-evidence destruction #709 exists to prevent. + archive_identity = f"{profile_identity}-archive-pr{pr_number}" + archive_payload = { + **dict(lock), + "archived_reason": "post_merge_cross_profile_cleanup", + "archived_for_pr": pr_number, + "archived_for_head": want_head, + "archived_remote": remote, + "archived_org": org, + "archived_repo": repo, + "recovery_critical": True, + "kind": mcp_session_state.KIND_DECISION_LOCK_ARCHIVE, + # The copied lock carries the *source* profile's identity. Leaving it in + # place makes save_state key the archive under that profile instead of + # the archive identity, so the archive would silently overwrite/land + # elsewhere and never read back (#709 F8 review 438). + "profile_identity": archive_identity, + "session_profile_lock": archive_identity, + "archived_from_profile_identity": profile_identity, + } + + def _archive_failure(step: str, detail: str) -> dict: + """Retain the terminal lock and report an actionable, retryable failure.""" + try: + _record_post_merge_decision_recovery( + pr_number=int(pr_number), + head_sha=want_head, + merge_commit_sha=None, + target_profile_identity=profile_identity, + failed_step=step, + error=detail, + remote=remote, + org=org, + repo=repo, + ) + except Exception as exc: # noqa: BLE001 + detail = f"{detail}; recovery record write also failed: {_redact(str(exc))}" + return { + "profile_identity": profile_identity, + "cleared": False, + "archive_ok": False, + "archive_failed_step": step, + "archive_identity": archive_identity, + "terminal_lock_retained": True, + "recovery_required": True, + "retry_safe": True, + "reason": ( + f"decision-lock archival failed at {step}: {detail}. Terminal " + "evidence retained and NOT cleared; resolve the archive failure " + "and retry this cleanup (fail closed, #709 F8 review 438)" + ), + "prior_summary": stale_review_decision_lock.lock_summary(lock), + } + try: - mcp_session_state.save_state( + archived = mcp_session_state.save_state( kind=mcp_session_state.KIND_DECISION_LOCK_ARCHIVE, - payload={ - **dict(lock), - "archived_reason": "post_merge_cross_profile_cleanup", - "archived_for_pr": pr_number, - "archived_for_head": want_head, - "archived_remote": remote, - "archived_org": org, - "archived_repo": repo, - "recovery_critical": True, - "kind": mcp_session_state.KIND_DECISION_LOCK_ARCHIVE, - }, + payload=archive_payload, remote=remote, org=org, repo=repo, - profile_identity=f"{profile_identity}-archive-pr{pr_number}", + profile_identity=archive_identity, ) - except Exception: - pass + except Exception as exc: # noqa: BLE001 + return _archive_failure("archive_save_state", _redact(str(exc))) + if not archived: + return _archive_failure( + "archive_save_state", + "save_state returned no durable archive record (false/empty result)", + ) + + # Durable read-back: a write that cannot be re-read is not an archive. + try: + confirmed = mcp_session_state.load_state_for_profile( + kind=mcp_session_state.KIND_DECISION_LOCK_ARCHIVE, + profile_identity=archive_identity, + remote=remote, + org=org, + repo=repo, + skip_identity_match=True, + enforce_repo_scope=True, + ) + except Exception as exc: # noqa: BLE001 + return _archive_failure("archive_readback", _redact(str(exc))) + if not confirmed: + return _archive_failure( + "archive_readback", + "archive record could not be read back from durable session state", + ) + if int(confirmed.get("archived_for_pr") or -1) != int( + pr_number + ) or not stale_review_decision_lock.heads_equal( + confirmed.get("archived_for_head"), want_head + ): + return _archive_failure( + "archive_readback", + "archive read-back does not match the PR/head being cleaned " + "(partial or stale archive)", + ) + mcp_session_state.clear_state( kind=mcp_session_state.KIND_DECISION_LOCK, profile_identity=profile_identity, @@ -4695,9 +4805,11 @@ def _clear_decision_lock_for_profile( return { "profile_identity": profile_identity, "cleared": True, + "archive_ok": True, + "archive_identity": archive_identity, "reason": ( f"cleared terminal lock for merged PR #{pr_number} " - f"at head {want_head[:12]}… (exact-scope)" + f"at head {want_head[:12]}… (exact-scope; durable archive confirmed)" ), "prior_summary": stale_review_decision_lock.lock_summary(lock), } @@ -5133,8 +5245,10 @@ def gitea_issue_irrecoverable_provenance_authorization( expected_head_sha: str, incident_issue: int, incident_comment_id: int, + decision_lock_id: str = "", confirmation: str = "", destroyed_subject: str | None = None, + recorded_head_sha: str | None = None, remote: str = "dadeschools", host: str | None = None, org: str | None = None, @@ -5186,6 +5300,13 @@ def gitea_issue_irrecoverable_provenance_authorization( ) return report + if not (decision_lock_id or "").strip(): + report["reasons"].append( + "decision_lock_id is required so evidence is bound to the exact " + "decision lock being recovered (fail closed, #709 F7 review 438)" + ) + return report + try: actor = _authenticated_username(h) except Exception: @@ -5195,6 +5316,13 @@ def gitea_issue_irrecoverable_provenance_authorization( "authenticated identity could not be verified (fail closed)" ) return report + actor_identity = _authenticated_actor(h) + if actor_identity.get("user_id") is None: + report["reasons"].append( + "authenticated actor id could not be verified; display-name-only " + "identity is not accepted (fail closed, #709 F7 review 438)" + ) + return report profile = get_profile() profile_name = (profile.get("profile_name") or "").strip() or None @@ -5237,6 +5365,11 @@ def gitea_issue_irrecoverable_provenance_authorization( ) except Exception as exc: # noqa: BLE001 comment_err = _redact(str(exc)) + try: + active_key_version = irp.auth_key_version() + except irp.AuthSecretError as exc: + report["reasons"].append(str(exc)) + return report incident_gate = irp.assess_incident_evidence( incident_issue=incident_issue, incident_comment_id=incident_comment_id, @@ -5247,6 +5380,11 @@ def gitea_issue_irrecoverable_provenance_authorization( expected_repo=r, expected_pr_number=pr_number, expected_head_sha=str(expected_head_sha), + expected_recorded_head_sha=recorded_head_sha, + expected_decision_lock_id=decision_lock_id.strip(), + expected_recovery_action=irp.RECOVERY_ACTION_IRRECOVERABLE_PROVENANCE, + expected_key_version=active_key_version, + mint_actor_id=actor_identity.get("user_id"), mint_actor_username=actor, reject_self_authored=True, ) @@ -5338,6 +5476,7 @@ def gitea_record_irrecoverable_decision_lock_provenance( incident_issue: int | None = None, incident_comment_id: int | None = None, authorization_id: str | None = None, + decision_lock_id: str = "", destroyed_subject: str | None = None, incident_ref: str | None = None, # Deprecated: retained so callers that still pass it get an explicit deny. @@ -5477,6 +5616,11 @@ def gitea_record_irrecoverable_decision_lock_provenance( ) except Exception as exc: # noqa: BLE001 comment_err = _redact(str(exc)) + try: + active_key_version = irp.auth_key_version() + except irp.AuthSecretError as exc: + report["reasons"].append(str(exc)) + return report incident_gate = irp.assess_incident_evidence( incident_issue=incident_issue, incident_comment_id=incident_comment_id, @@ -5487,6 +5631,10 @@ def gitea_record_irrecoverable_decision_lock_provenance( expected_repo=r, expected_pr_number=pr_number, expected_head_sha=str(expected_head_sha), + expected_decision_lock_id=(decision_lock_id or "").strip() or None, + expected_recovery_action=irp.RECOVERY_ACTION_IRRECOVERABLE_PROVENANCE, + expected_key_version=active_key_version, + mint_actor_id=_authenticated_actor(h).get("user_id"), mint_actor_username=actor, reject_self_authored=True, ) @@ -8869,6 +9017,7 @@ def _try_auto_switch_for_operation(op: str, host: str | None = None) -> bool: if tok: gitea_config._active_profile_override = p_name _IDENTITY_CACHE.clear() + _ACTOR_IDENTITY_CACHE.clear() return True except Exception: pass @@ -11989,6 +12138,7 @@ def gitea_activate_profile( # 3. Clear identity cache to force a fresh verification if h: _IDENTITY_CACHE.pop(h, None) + _ACTOR_IDENTITY_CACHE.pop(h, None) # 4. Resolve fresh identity after_profile = get_profile()["profile_name"] diff --git a/irrecoverable_provenance.py b/irrecoverable_provenance.py index 3ec0e4c..7d5ca76 100644 --- a/irrecoverable_provenance.py +++ b/irrecoverable_provenance.py @@ -14,6 +14,7 @@ import hashlib import hmac import json import os +import re import uuid from datetime import datetime, timedelta, timezone from typing import Any @@ -31,6 +32,34 @@ KIND_RECOVERY = mcp_session_state.KIND_IRRECOVERABLE_DECISION_PROVENANCE CONFIRMATION_PREFIX = "IRRECOVERABLE DECISION PROVENANCE PR" # Canonical incident-comment marker (#709 review 435 F5). INCIDENT_MARKER = "IRRECOVERABLE DECISION PROVENANCE INCIDENT" + +# #709 F7 (review 438): strictly canonical incident evidence. The digest binds +# every security-relevant field, so evidence cannot be replayed or substituted +# across repositories, PRs, decision locks, heads, actors, or recovery actions. +INCIDENT_SCHEMA_VERSION = "2" +RECOVERY_ACTION_IRRECOVERABLE_PROVENANCE = "irrecoverable_decision_lock_provenance" +SUPPORTED_RECOVERY_ACTIONS = frozenset({RECOVERY_ACTION_IRRECOVERABLE_PROVENANCE}) +INCIDENT_FIELD_ORDER = ( + "schema_version", + "remote", + "org", + "repo", + "pr_number", + "decision_lock_id", + "destroyed_subject", + "recovery_action", + "recorded_head_sha", + "expected_head_sha", + "incident_issue", + "evidence_author_id", + "evidence_author_login", + "mint_actor_id", + "mint_actor_login", + "key_version", + "nonce", + "issued_at", +) +INCIDENT_DIGEST_FIELD = "content_digest" AUTH_TTL_HOURS = 24.0 RECORD_TYPE = "irrecoverable_decision_provenance" AUTH_TYPE = "irrecoverable_provenance_authorization" @@ -44,6 +73,21 @@ DEFAULT_KEY_VERSION = "v1" _PYTEST_AUTH_SECRET = b"pytest-irrecoverable-auth-v1" _PYTEST_KEY_VERSION = "pytest-v1" +# #709 F6 (review 438): key version is part of the authenticated data and must +# be present exactly once, well-formed, and equal to the configured active +# version. There is deliberately no legacy fallback for versionless artifacts. +KEY_VERSION_RE = re.compile(r"^[A-Za-z0-9._-]{1,64}$") +KEY_VERSION_FIELD = "key_version" +# Any of these aliases anywhere in the artifact counts as a key-version field; +# more than one occurrence is a duplicate and fails closed even when the values +# are identical. +_KEY_VERSION_ALIASES = ( + "key_version", + "keyVersion", + "key-version", + "auth_key_version", +) + _PROCESS_AUTH_SECRET: bytes | None = None _PROCESS_AUTH_KEY_VERSION: str | None = None _PROCESS_AUTH_SECRET_SOURCE: str | None = None @@ -101,10 +145,135 @@ def reset_process_auth_secret_for_tests() -> None: _PROCESS_AUTH_SECRET_SOURCE = None +def _validate_key_version_text(value: Any, *, source: str) -> tuple[str, list[str]]: + """Validate a key-version string. Never echoes key material — versions only.""" + if isinstance(value, bool) or not isinstance(value, str): + return "", [ + f"{source} key version is malformed (must be a string; fail closed, #709 F6)" + ] + text = value.strip() + if not text: + return "", [f"{source} key version is empty (fail closed, #709 F6)"] + if value != text: + return "", [ + f"{source} key version has surrounding whitespace (fail closed, #709 F6)" + ] + if not KEY_VERSION_RE.match(text): + return "", [ + f"{source} key version is malformed (allowed charset A-Za-z0-9._-, " + "1-64 chars; fail closed, #709 F6)" + ] + return text, [] + + def auth_key_version() -> str: - """Return the active signing-key version string (never secret material).""" + """Return the active configured signing-key version (never secret material).""" _process_secret() # ensure version is resolved with the secret - return _PROCESS_AUTH_KEY_VERSION or DEFAULT_KEY_VERSION + version = _PROCESS_AUTH_KEY_VERSION or "" + if not version: + raise AuthSecretError( + "active HMAC key version is unresolved (fail closed, #709 F6 review 438)" + ) + return version + + +def supported_key_versions() -> tuple[str, ...]: + """Versions accepted for verification. + + Exactly one version — the configured active one — is honored. Rotation is + performed by changing ``GITEA_IRRECOVERABLE_AUTH_HMAC_KEY_VERSION`` (and the + key); artifacts minted under a superseded version stop verifying, which is + the intended fail-closed rotation behavior (#709 F6 review 438). + """ + return (auth_key_version(),) + + +def extract_artifact_key_version(auth: dict[str, Any] | None) -> dict[str, Any]: + """Require exactly one nonempty, well-formed key-version field (#709 F6). + + Missing, empty, malformed, or duplicated (even identical-valued) key-version + fields fail closed. Versionless artifacts are never accepted — there is no + legacy fallback. + """ + if not isinstance(auth, dict): + return { + "valid": False, + "key_version": None, + "reasons": ["authorization artifact missing (fail closed)"], + } + + containers: list[tuple[str, dict[str, Any]]] = [("artifact", auth)] + for nested in ("native_provenance", "scope"): + value = auth.get(nested) + if isinstance(value, dict): + containers.append((nested, value)) + + seen: list[tuple[str, Any]] = [] + for container_name, container in containers: + for alias in _KEY_VERSION_ALIASES: + if alias in container: + seen.append((f"{container_name}.{alias}", container[alias])) + + if not seen: + return { + "valid": False, + "key_version": None, + "reasons": [ + "authorization is missing key_version; versionless artifacts are " + "never accepted (no legacy fallback; fail closed, #709 F6 review 438)" + ], + } + if len(seen) > 1: + names = ", ".join(sorted(name for name, _ in seen)) + return { + "valid": False, + "key_version": None, + "reasons": [ + f"authorization carries duplicate key-version fields ({names}); " + "exactly one is required, even when the values are identical " + "(fail closed, #709 F6 review 438)" + ], + } + + name, raw = seen[0] + text, reasons = _validate_key_version_text(raw, source=f"authorization {name}") + if reasons: + return {"valid": False, "key_version": None, "reasons": reasons} + return {"valid": True, "key_version": text, "reasons": []} + + +def assess_artifact_key_version( + auth: dict[str, Any] | None, + *, + expected_version: str | None = None, +) -> dict[str, Any]: + """Artifact key version must exactly equal the configured active version.""" + extracted = extract_artifact_key_version(auth) + if not extracted.get("valid"): + return extracted + have = str(extracted.get("key_version") or "") + try: + want = (expected_version or "").strip() or auth_key_version() + except AuthSecretError as exc: + return { + "valid": False, + "key_version": None, + "reasons": [ + f"configured HMAC key version unavailable: {exc} " + "(fail closed, #709 F6)" + ], + } + if have != want: + return { + "valid": False, + "key_version": None, + "reasons": [ + f"authorization key_version {have!r} is unknown or does not match " + f"the configured expected version {want!r} (fail closed, #709 F6 " + "review 438)" + ], + } + return {"valid": True, "key_version": have, "reasons": []} def _process_secret() -> bytes: @@ -126,16 +295,34 @@ def _process_secret() -> bytes: pytest = mcp_daemon_guard.is_pytest_runtime() if env_raw: - _PROCESS_AUTH_SECRET = _decode_key_material(env_raw) - _PROCESS_AUTH_KEY_VERSION = env_ver or ( - _PYTEST_KEY_VERSION if pytest else DEFAULT_KEY_VERSION + secret = _decode_key_material(env_raw) + if not env_ver and not pytest: + # #709 F6 (review 438): production must configure the key version + # explicitly; silently defaulting it makes rotation ambiguous. + raise AuthSecretError( + f"{ENV_AUTH_HMAC_KEY_VERSION} is required for production " + "irrecoverable auth HMAC; an implicit default key version is " + "forbidden (fail closed, #709 F6 review 438)" + ) + version, version_reasons = _validate_key_version_text( + env_ver or (_PYTEST_KEY_VERSION if pytest else ""), + source="configured", ) + if version_reasons: + raise AuthSecretError("; ".join(version_reasons)) + _PROCESS_AUTH_SECRET = secret + _PROCESS_AUTH_KEY_VERSION = version _PROCESS_AUTH_SECRET_SOURCE = "env" return _PROCESS_AUTH_SECRET if pytest: + version, version_reasons = _validate_key_version_text( + env_ver or _PYTEST_KEY_VERSION, source="configured" + ) + if version_reasons: + raise AuthSecretError("; ".join(version_reasons)) _PROCESS_AUTH_SECRET = _PYTEST_AUTH_SECRET - _PROCESS_AUTH_KEY_VERSION = env_ver or _PYTEST_KEY_VERSION + _PROCESS_AUTH_KEY_VERSION = version _PROCESS_AUTH_SECRET_SOURCE = "pytest_constant" return _PROCESS_AUTH_SECRET @@ -328,19 +515,79 @@ def assess_transport_for_auth_mint() -> dict[str, Any]: } -def _incident_author(comment_payload: dict[str, Any]) -> str | None: - user = comment_payload.get("user") +def incident_actor_identity(payload: dict[str, Any] | None) -> dict[str, Any]: + """Resolve one stable actor identity from a comment payload (#709 F7). + + Uses the immutable numeric user id as the primary identity and requires the + login to be internally consistent. Multiple or conflicting ids/logins (e.g. + ``user.login`` disagreeing with ``user.username``) fail closed rather than + silently preferring one string (review 438 F7). + """ + reasons: list[str] = [] + if not isinstance(payload, dict): + return { + "valid": False, + "user_id": None, + "login": None, + "reasons": ["actor payload missing (fail closed, #709 F7)"], + } + + ids: set[int] = set() + logins: set[str] = set() + + def _add_id(value: Any) -> None: + if isinstance(value, bool) or value is None: + return + if isinstance(value, int): + ids.add(int(value)) + elif isinstance(value, str) and value.strip().lstrip("-").isdigit(): + ids.add(int(value.strip())) + + def _add_login(value: Any) -> None: + if isinstance(value, str) and value.strip(): + logins.add(value.strip()) + + user = payload.get("user") if isinstance(user, dict): - login = (user.get("login") or user.get("username") or "").strip() - return login or None - if isinstance(user, str) and user.strip(): - return user.strip() - # Some payloads flatten author. + _add_id(user.get("id")) + _add_id(user.get("user_id")) + _add_login(user.get("login")) + _add_login(user.get("username")) + elif isinstance(user, str): + _add_login(user) for key in ("login", "author", "username"): - val = comment_payload.get(key) - if isinstance(val, str) and val.strip(): - return val.strip() - return None + _add_login(payload.get(key)) + _add_id(payload.get("user_id")) + + if len(logins) > 1: + reasons.append( + "incident comment author has conflicting logins " + f"({sorted(logins)!r}); a single stable identity is required " + "(fail closed, #709 F7 review 438)" + ) + if len(ids) > 1: + reasons.append( + "incident comment author has conflicting user ids " + f"({sorted(ids)!r}); a single stable identity is required " + "(fail closed, #709 F7 review 438)" + ) + if not ids: + reasons.append( + "incident comment author is missing a stable user id; " + "display-name-only identity is not accepted " + "(fail closed, #709 F7 review 438)" + ) + if not logins: + reasons.append( + "incident comment author login is missing (fail closed, #709 F7)" + ) + + return { + "valid": not reasons, + "user_id": next(iter(ids)) if len(ids) == 1 else None, + "login": next(iter(logins)) if len(logins) == 1 else None, + "reasons": reasons, + } def canonical_incident_fields( @@ -349,85 +596,272 @@ def canonical_incident_fields( org: str, repo: str, pr_number: int, + decision_lock_id: str, + destroyed_subject: str, + recovery_action: str, + recorded_head_sha: str, expected_head_sha: str, incident_issue: int, + evidence_author_id: int, + evidence_author_login: str, + mint_actor_id: int, + mint_actor_login: str, + key_version: str, + nonce: str, + issued_at: str, ) -> dict[str, str]: - """Ordered fields that form the authoritative incident content digest.""" - head = normalize_head_sha(expected_head_sha) or "" - return { - "marker": INCIDENT_MARKER, - "remote": str(remote or "").strip(), - "org": str(org or "").strip(), - "repo": str(repo or "").strip(), + """Ordered fields that form the authoritative incident content digest. + + Every security-relevant element of the recovery scope is bound here so the + digest cannot be replayed across repositories, PRs, decision locks, heads, + actors, or recovery actions (#709 F7 review 438). + """ + action = str(recovery_action or "").strip() + if action not in SUPPORTED_RECOVERY_ACTIONS: + raise ValueError( + f"unsupported recovery_action {action!r}; supported=" + f"{sorted(SUPPORTED_RECOVERY_ACTIONS)!r} (fail closed, #709 F7)" + ) + fields = { + "schema_version": INCIDENT_SCHEMA_VERSION, + "remote": str(remote or ""), + "org": str(org or ""), + "repo": str(repo or ""), "pr_number": str(int(pr_number)), - "expected_head_sha": head, + "decision_lock_id": str(decision_lock_id or ""), + "destroyed_subject": str(destroyed_subject or ""), + "recovery_action": action, + "recorded_head_sha": normalize_head_sha(recorded_head_sha) or "", + "expected_head_sha": normalize_head_sha(expected_head_sha) or "", "incident_issue": str(int(incident_issue)), + "evidence_author_id": str(int(evidence_author_id)), + "evidence_author_login": str(evidence_author_login or ""), + "mint_actor_id": str(int(mint_actor_id)), + "mint_actor_login": str(mint_actor_login or ""), + "key_version": str(key_version or ""), + "nonce": str(nonce or ""), + "issued_at": str(issued_at or ""), } + for name in INCIDENT_FIELD_ORDER: + value = fields[name] + if not value.strip(): + raise ValueError( + f"canonical incident field {name!r} is empty (fail closed, #709 F7)" + ) + if value != value.strip(): + raise ValueError( + f"canonical incident field {name!r} has surrounding whitespace " + "(fail closed, #709 F7)" + ) + if "\n" in value or "\r" in value: + raise ValueError( + f"canonical incident field {name!r} contains a newline; ambiguous " + "evidence cannot be built (fail closed, #709 F7)" + ) + return fields def incident_content_digest(fields: dict[str, str]) -> str: - """SHA-256 hex digest over canonical field lines (stable, sort-free order).""" - # Fixed key order — do not sort; operator body must match this order. - order = ( - "marker", - "remote", - "org", - "repo", - "pr_number", - "expected_head_sha", - "incident_issue", - ) - lines = [f"{k}={fields[k]}" for k in order] + """SHA-256 hex digest over the canonical field lines (fixed order, no sort).""" + lines = [INCIDENT_MARKER] + for name in INCIDENT_FIELD_ORDER: + if name not in fields: + raise ValueError( + f"canonical incident field {name!r} missing for digest " + "(fail closed, #709 F7)" + ) + lines.append(f"{name}={fields[name]}") blob = "\n".join(lines).encode("utf-8") return hashlib.sha256(blob).hexdigest() +def render_canonical_incident_block(fields: dict[str, str]) -> str: + """Render the one accepted canonical representation of *fields*.""" + lines = [INCIDENT_MARKER] + lines.extend(f"{name}: {fields[name]}" for name in INCIDENT_FIELD_ORDER) + lines.append(f"{INCIDENT_DIGEST_FIELD}: {incident_content_digest(fields)}") + return "\n".join(lines) + + +def _narrative_is_ambiguous(narrative: str) -> list[str]: + """Reject narrative text that could be mistaken for canonical evidence.""" + reasons: list[str] = [] + known = set(INCIDENT_FIELD_ORDER) | {INCIDENT_DIGEST_FIELD} + for line in narrative.split("\n"): + text = line.strip() + if text == INCIDENT_MARKER: + reasons.append( + "narrative repeats the canonical marker (ambiguous evidence)" + ) + continue + if ":" in text and text.split(":", 1)[0].strip() in known: + reasons.append( + f"narrative contains canonical field line {text.split(':', 1)[0]!r} " + "(ambiguous evidence)" + ) + return reasons + + def build_canonical_incident_body( *, remote: str, org: str, repo: str, pr_number: int, + decision_lock_id: str, + destroyed_subject: str, + recovery_action: str, + recorded_head_sha: str, expected_head_sha: str, incident_issue: int, + evidence_author_id: int, + evidence_author_login: str, + mint_actor_id: int, + mint_actor_login: str, + key_version: str, + nonce: str, + issued_at: str, narrative: str | None = None, ) -> str: - """Build an operator-postable canonical incident comment body (#709 F5).""" + """Build the one accepted canonical incident comment body (#709 F7). + + The builder is the single source of the accepted format and refuses to emit + anything ambiguous: empty/multiline fields, unsupported recovery actions, and + narrative text that mimics canonical evidence all raise ``ValueError``. Its + own output is re-parsed before return, so a body this function produces + always validates as canonical. + """ fields = canonical_incident_fields( remote=remote, org=org, repo=repo, pr_number=pr_number, + decision_lock_id=decision_lock_id, + destroyed_subject=destroyed_subject, + recovery_action=recovery_action, + recorded_head_sha=recorded_head_sha, expected_head_sha=expected_head_sha, incident_issue=incident_issue, + evidence_author_id=evidence_author_id, + evidence_author_login=evidence_author_login, + mint_actor_id=mint_actor_id, + mint_actor_login=mint_actor_login, + key_version=key_version, + nonce=nonce, + issued_at=issued_at, ) - digest = incident_content_digest(fields) - lines = [ - fields["marker"], - f"remote: {fields['remote']}", - f"org: {fields['org']}", - f"repo: {fields['repo']}", - f"pr_number: {fields['pr_number']}", - f"expected_head_sha: {fields['expected_head_sha']}", - f"incident_issue: {fields['incident_issue']}", - f"content_digest: {digest}", - ] + body = render_canonical_incident_block(fields) if narrative and narrative.strip(): - lines.extend(["", narrative.strip()]) - return "\n".join(lines) + text = narrative.strip() + ambiguous = _narrative_is_ambiguous(text) + if ambiguous: + raise ValueError( + "; ".join(ambiguous) + " (fail closed, #709 F7 review 438)" + ) + body = f"{body}\n\n{text}" + check = parse_canonical_incident_body(body) + if not check.get("valid"): + raise ValueError( + "builder produced a non-canonical body: " + + "; ".join(check.get("reasons") or ["unknown"]) + ) + return body -def _parse_incident_field(body: str, name: str) -> str | None: - """Extract ``name: value`` or ``name=value`` from incident body lines.""" - for line in (body or "").splitlines(): - text = line.strip() - if not text or text.startswith("#"): +def parse_canonical_incident_body(body: str | None) -> dict[str, Any]: + """Strictly parse the canonical incident block (#709 F7 review 438). + + Enforces the exact schema: marker on the first line, every field present + exactly once, in fixed canonical order, with no unknown, duplicate, + conflicting, or empty fields anywhere in the body. + """ + reasons: list[str] = [] + text = (body or "").replace("\r\n", "\n").replace("\r", "\n").strip("\n") + if not text.strip(): + return { + "valid": False, + "reasons": ["incident comment body is empty (fail closed)"], + "fields": {}, + } + + lines = text.split("\n") + if lines[0] != INCIDENT_MARKER: + return { + "valid": False, + "reasons": [ + f"incident body line 1 must be exactly {INCIDENT_MARKER!r} " + "(canonical marker position; fail closed, #709 F7 review 438)" + ], + "fields": {}, + } + + order = list(INCIDENT_FIELD_ORDER) + [INCIDENT_DIGEST_FIELD] + block_len = 1 + len(order) + if len(lines) < block_len: + return { + "valid": False, + "reasons": [ + "incident body canonical block is incomplete; every field is " + "required exactly once in canonical order (fail closed, #709 F7)" + ], + "fields": {}, + } + + fields: dict[str, str] = {} + for index, name in enumerate(order, start=1): + line = lines[index] + prefix = f"{name}: " + if not line.startswith(prefix): + return { + "valid": False, + "reasons": [ + f"incident body line {index + 1} must be field {name!r} in " + "canonical order (reordered, duplicated, missing, or unknown " + "field; fail closed, #709 F7 review 438)" + ], + "fields": {}, + } + value = line[len(prefix) :] + if not value.strip(): + reasons.append( + f"incident field {name!r} is empty (fail closed, #709 F7)" + ) + elif value != value.strip(): + reasons.append( + f"incident field {name!r} has surrounding whitespace " + "(fail closed, #709 F7)" + ) + fields[name] = value + + # Nothing outside the canonical block may restate the marker or any field. + known = set(order) + tail = lines[block_len:] + if tail and tail[0].strip(): + reasons.append( + "narrative must be separated from the canonical block by a blank " + "line (fail closed, #709 F7)" + ) + for line in tail: + stripped = line.strip() + if stripped == INCIDENT_MARKER: + reasons.append( + "incident body repeats the canonical marker outside the signed " + "block (ambiguous evidence; fail closed, #709 F7 review 438)" + ) continue - for sep in (":", "="): - prefix = f"{name}{sep}" - if text.lower().startswith(prefix.lower()): - return text[len(prefix) :].strip() - return None + if ":" in stripped and stripped.split(":", 1)[0].strip() in known: + duplicated = stripped.split(":", 1)[0].strip() + reasons.append( + f"incident body restates canonical field {duplicated!r} outside " + "the signed block (duplicate/conflicting; fail closed, #709 F7)" + ) + + return { + "valid": not reasons, + "reasons": reasons, + "fields": fields, + "canonical_block": "\n".join(lines[:block_len]), + } def assess_incident_evidence( @@ -441,16 +875,22 @@ def assess_incident_evidence( expected_repo: str | None = None, expected_pr_number: int | None = None, expected_head_sha: str | None = None, + expected_recorded_head_sha: str | None = None, + expected_decision_lock_id: str | None = None, + expected_recovery_action: str | None = None, + expected_key_version: str | None = None, + mint_actor_id: int | None = None, mint_actor_username: str | None = None, reject_self_authored: bool = True, ) -> dict[str, Any]: - """Validate authoritative incident evidence (live-fetched, #709 F5). + """Validate strictly canonical, fully scoped incident evidence (#709 F7). - Requires: - - live comment id match - - non-empty author identity - - canonical marker + scope fields + content_digest integrity - - optional mint-actor independence (mint actor ≠ comment author) + The body must be exactly the one canonical representation produced by + :func:`build_canonical_incident_body`: marker first, every field present + once in fixed order, no duplicates/unknowns/conflicts, digest binding the + complete recovery scope, and a single stable actor identity independent of + the minting actor. Reordered, duplicated, conflicting, replayed, or + substituted evidence fails closed (review 438 F7). """ reasons: list[str] = [] if incident_issue is None or int(incident_issue) <= 0: @@ -470,7 +910,6 @@ def assess_incident_evidence( ) return {"valid": False, "reasons": reasons, "comment": None} - # Gitea returns comment with id; optional issue_url / html_url for scope. cid = comment_payload.get("id") try: if int(cid) != int(incident_comment_id): # type: ignore[arg-type] @@ -480,119 +919,171 @@ def assess_incident_evidence( except (TypeError, ValueError): reasons.append("incident comment payload missing valid id (fail closed)") - body = (comment_payload.get("body") or "").strip() - if not body: - reasons.append("incident comment body is empty (fail closed)") - - author = _incident_author(comment_payload) - if not author: + # Edited evidence is not authoritative (#709 F5 review 435). + created_at = str(comment_payload.get("created_at") or "").strip() + updated_at = str(comment_payload.get("updated_at") or "").strip() + if updated_at and created_at and updated_at != created_at: reasons.append( - "incident comment missing author identity (fail closed, #709 F5)" + "incident comment has been edited after creation; edited evidence is " + "not authoritative (fail closed, #709 F5 review 435)" ) - # Self-mint prevention: minting actor must not be the sole incident author. - if ( - reject_self_authored - and author - and mint_actor_username - and author.strip().lower() == str(mint_actor_username).strip().lower() - ): - reasons.append( - "incident comment author matches mint actor; self-authored incident " - "evidence is not accepted (fail closed, #709 F5 review 435)" - ) + actor = incident_actor_identity(comment_payload) + if not actor.get("valid"): + reasons.extend(actor.get("reasons") or []) - # Canonical content + digest (not any non-empty body). - if body and INCIDENT_MARKER not in body: - reasons.append( - f"incident comment missing canonical marker {INCIDENT_MARKER!r} " - "(fail closed, #709 F5)" - ) - elif body: - remote_f = _parse_incident_field(body, "remote") - org_f = _parse_incident_field(body, "org") - repo_f = _parse_incident_field(body, "repo") - pr_f = _parse_incident_field(body, "pr_number") - head_f = _parse_incident_field(body, "expected_head_sha") - issue_f = _parse_incident_field(body, "incident_issue") - digest_f = _parse_incident_field(body, "content_digest") + parsed = parse_canonical_incident_body(comment_payload.get("body")) + fields = parsed.get("fields") or {} + if not parsed.get("valid"): + reasons.extend(parsed.get("reasons") or []) + else: + # Every field is bound to the exact live recovery scope. + def _match(name: str, expected: Any, *, label: str | None = None) -> None: + if expected is None or str(expected).strip() == "": + return + have = fields.get(name, "") + if have != str(expected).strip(): + reasons.append( + f"incident body {label or name} does not match the live " + f"recovery scope (fail closed, #709 F7 review 438)" + ) - if expected_remote and remote_f and remote_f != str(expected_remote).strip(): + if fields.get("schema_version") != INCIDENT_SCHEMA_VERSION: reasons.append( - "incident body remote does not match expected remote (fail closed)" + f"incident schema_version must be {INCIDENT_SCHEMA_VERSION!r} " + "(fail closed, #709 F7)" ) - if expected_org and org_f and org_f != str(expected_org).strip(): + _match("remote", expected_remote) + _match("org", expected_org) + _match("repo", expected_repo) + _match("decision_lock_id", expected_decision_lock_id) + _match("recovery_action", expected_recovery_action) + _match("key_version", expected_key_version) + + if fields.get("recovery_action") not in SUPPORTED_RECOVERY_ACTIONS: reasons.append( - "incident body org does not match expected org (fail closed)" - ) - if expected_repo and repo_f and repo_f != str(expected_repo).strip(): - reasons.append( - "incident body repo does not match expected repo (fail closed)" + f"incident recovery_action {fields.get('recovery_action')!r} is " + "not a supported recovery action (fail closed, #709 F7)" ) + if expected_pr_number is not None: try: - if pr_f is None or int(pr_f) != int(expected_pr_number): + if int(fields.get("pr_number", "")) != int(expected_pr_number): reasons.append( "incident body pr_number does not match mint PR " - "(fail closed, #709 F5)" + "(fail closed, #709 F7)" + ) + except (TypeError, ValueError): + reasons.append("incident body pr_number invalid (fail closed)") + + try: + if int(fields.get("incident_issue", "")) != int(incident_issue): # type: ignore[arg-type] + reasons.append( + "incident body incident_issue does not match provided " + "incident_issue (fail closed, #709 F7)" + ) + except (TypeError, ValueError): + reasons.append("incident body incident_issue invalid (fail closed)") + + if expected_head_sha and not heads_equal( + fields.get("expected_head_sha"), expected_head_sha + ): + reasons.append( + "incident body expected_head_sha does not match the live mint " + "head (fail closed, #709 F7)" + ) + if expected_recorded_head_sha and not heads_equal( + fields.get("recorded_head_sha"), expected_recorded_head_sha + ): + reasons.append( + "incident body recorded_head_sha does not match the recorded " + "decision-lock head (fail closed, #709 F7 review 438)" + ) + + # Evidence author must be the actual live comment author. + if actor.get("valid"): + try: + if int(fields.get("evidence_author_id", "")) != int( + actor.get("user_id") + ): + reasons.append( + "incident body evidence_author_id does not match the live " + "comment author (fail closed, #709 F7 review 438)" ) except (TypeError, ValueError): reasons.append( - "incident body pr_number missing or invalid (fail closed)" + "incident body evidence_author_id invalid (fail closed)" ) - if expected_head_sha: - if not head_f or not heads_equal(head_f, expected_head_sha): + if fields.get("evidence_author_login") != str(actor.get("login") or ""): reasons.append( - "incident body expected_head_sha does not match live mint " - "head (fail closed, #709 F5)" + "incident body evidence_author_login does not match the live " + "comment author (fail closed, #709 F7 review 438)" ) - try: - if issue_f is None or int(issue_f) != int(incident_issue): # type: ignore[arg-type] - reasons.append( - "incident body incident_issue does not match provided " - "incident_issue (fail closed, #709 F5)" - ) - except (TypeError, ValueError): + + # Minting actor must be the live caller, and must not be the author. + if mint_actor_id is not None: + try: + if int(fields.get("mint_actor_id", "")) != int(mint_actor_id): + reasons.append( + "incident body mint_actor_id does not match the minting " + "actor (fail closed, #709 F7 review 438)" + ) + except (TypeError, ValueError): + reasons.append("incident body mint_actor_id invalid (fail closed)") + if mint_actor_username and fields.get("mint_actor_login") != str( + mint_actor_username + ).strip(): reasons.append( - "incident body incident_issue missing or invalid (fail closed)" + "incident body mint_actor_login does not match the minting actor " + "(fail closed, #709 F7 review 438)" ) - # Content digest integrity when we have enough scope to recompute. - if ( - remote_f - and org_f - and repo_f - and pr_f - and head_f - and issue_f - and digest_f - ): - try: - fields = canonical_incident_fields( - remote=remote_f, - org=org_f, - repo=repo_f, - pr_number=int(pr_f), - expected_head_sha=head_f, - incident_issue=int(issue_f), - ) - expected_digest = incident_content_digest(fields) - if not hmac.compare_digest( - expected_digest.lower(), str(digest_f).strip().lower() - ): - reasons.append( - "incident content_digest mismatch (forged or incomplete " - "canonical body; fail closed, #709 F5)" - ) - except (TypeError, ValueError) as exc: + # Digest binds the complete canonical content. + try: + expected_digest = incident_content_digest(fields) + if not hmac.compare_digest( + expected_digest.lower(), + str(fields.get(INCIDENT_DIGEST_FIELD, "")).strip().lower(), + ): reasons.append( - f"incident content_digest recompute failed: {exc} (fail closed)" + "incident content_digest mismatch (forged, substituted, or " + "incomplete canonical body; fail closed, #709 F7)" ) - else: + except (TypeError, ValueError) as exc: reasons.append( - "incident body missing required canonical fields " - "(remote/org/repo/pr_number/expected_head_sha/incident_issue/" - "content_digest; fail closed, #709 F5)" + f"incident content_digest recompute failed: {exc} (fail closed)" + ) + + # Reconstruct the exact canonical representation and require equality. + try: + rebuilt = render_canonical_incident_block(fields) + if rebuilt != parsed.get("canonical_block"): + reasons.append( + "incident body is not the exact canonical representation " + "(fail closed, #709 F7 review 438)" + ) + except (TypeError, ValueError) as exc: + reasons.append( + f"incident canonical reconstruction failed: {exc} (fail closed)" + ) + + # Independent-author requirement (#709 F5 review 435), by stable identity. + if reject_self_authored and actor.get("valid"): + if mint_actor_id is not None and int(actor.get("user_id") or -1) == int( + mint_actor_id + ): + reasons.append( + "incident comment author is the minting actor; self-authored " + "incident evidence is not accepted (fail closed, #709 F5 review 435)" + ) + elif ( + mint_actor_username + and str(actor.get("login") or "").strip().lower() + == str(mint_actor_username).strip().lower() + ): + reasons.append( + "incident comment author is the minting actor; self-authored " + "incident evidence is not accepted (fail closed, #709 F5 review 435)" ) # Hard scope check when URLs are present. @@ -611,12 +1102,13 @@ def assess_incident_evidence( return { "valid": not reasons, "reasons": reasons, + "fields": fields, "comment": { "id": comment_payload.get("id"), - "author": author, + "author": actor.get("login"), + "author_id": actor.get("user_id"), "created_at": comment_payload.get("created_at"), - "body_len": len(body), - "has_canonical_marker": INCIDENT_MARKER in body if body else False, + "canonical": bool(parsed.get("valid")), }, } @@ -790,40 +1282,60 @@ def verify_authorization_artifact( if not (auth.get("server_signature") or "").strip(): reasons.append("authorization missing server_signature (fail closed)") - # Recompute signature over stored scope fields + key_version. - try: - scope = _scope_payload( - remote=str(auth.get("remote") or ""), - org=str(auth.get("org") or ""), - repo=str(auth.get("repo") or ""), - pr_number=int(auth.get("blocked_pr_number")), - expected_head_sha=str(auth.get("expected_head_sha") or ""), - incident_issue=int(auth.get("incident_issue")), - incident_comment_id=int(auth.get("incident_comment_id")), - destroyed_subject=auth.get("destroyed_subject"), - issuer_username=str(auth.get("issuer_username") or ""), - issuer_profile=str(auth.get("issuer_profile") or ""), - created_at=str(auth.get("created_at") or ""), - expires_at=str(auth.get("expires_at") or ""), - authorization_id=str(auth.get("authorization_id") or ""), + # #709 F6 (review 438): validate key version *before* any MAC work so an + # attacker-chosen version can never select the signing key. Missing, empty, + # malformed, duplicated, unknown, and mismatched versions all fail closed. + key_version_gate = assess_artifact_key_version(auth) + verified_key_version = ( + key_version_gate.get("key_version") if key_version_gate.get("valid") else None + ) + if not key_version_gate.get("valid"): + reasons.extend( + key_version_gate.get("reasons") + or ["authorization key_version invalid (fail closed, #709 F6)"] ) - native = auth.get("native_provenance") or {} - if not isinstance(native, dict): - native = {} - stored_kv = str(auth.get("key_version") or auth_key_version()) - expected_sig = _sign_scope(scope, native, key_version=stored_kv) - if not hmac.compare_digest( - expected_sig, str(auth.get("server_signature") or "") - ): - reasons.append( - "authorization server_signature invalid (forged, corrupt, or " - "HMAC key mismatch across process/restart; fail closed, " - "#709 F4/F1)" + + # Recompute signature over stored scope fields + verified key_version. + if verified_key_version is None: + reasons.append( + "authorization server_signature not verified: key version failed " + "validation (fail closed, #709 F6 review 438)" + ) + else: + try: + scope = _scope_payload( + remote=str(auth.get("remote") or ""), + org=str(auth.get("org") or ""), + repo=str(auth.get("repo") or ""), + pr_number=int(auth.get("blocked_pr_number")), + expected_head_sha=str(auth.get("expected_head_sha") or ""), + incident_issue=int(auth.get("incident_issue")), + incident_comment_id=int(auth.get("incident_comment_id")), + destroyed_subject=auth.get("destroyed_subject"), + issuer_username=str(auth.get("issuer_username") or ""), + issuer_profile=str(auth.get("issuer_profile") or ""), + created_at=str(auth.get("created_at") or ""), + expires_at=str(auth.get("expires_at") or ""), + authorization_id=str(auth.get("authorization_id") or ""), ) - except AuthSecretError as exc: - reasons.append(f"authorization HMAC key unavailable: {exc} (fail closed)") - except (TypeError, ValueError) as exc: - reasons.append(f"authorization scope incomplete: {exc} (fail closed)") + native = auth.get("native_provenance") or {} + if not isinstance(native, dict): + native = {} + expected_sig = _sign_scope( + scope, native, key_version=verified_key_version + ) + if not hmac.compare_digest( + expected_sig, str(auth.get("server_signature") or "") + ): + reasons.append( + "authorization server_signature invalid (forged, corrupt, or " + "HMAC key mismatch across process/restart; fail closed, " + "#709 F4/F1)" + ) + except AuthSecretError as exc: + reasons.append(f"authorization HMAC key unavailable: {exc} (fail closed)") + except (TypeError, ValueError) as exc: + reasons.append(f"authorization scope incomplete: {exc} (fail closed)") # Native provenance required on the artifact itself. native = auth.get("native_provenance") or {} @@ -869,6 +1381,7 @@ def verify_authorization_artifact( "reasons": reasons, "authorization_id": auth.get("authorization_id"), "consumption_state": state or None, + "key_version": verified_key_version, } diff --git a/tests/test_issue_709_decision_lock_cross_profile.py b/tests/test_issue_709_decision_lock_cross_profile.py index f20270b..91e8d16 100644 --- a/tests/test_issue_709_decision_lock_cross_profile.py +++ b/tests/test_issue_709_decision_lock_cross_profile.py @@ -7,6 +7,7 @@ live PR numbers in production code. from __future__ import annotations +import json import os import subprocess import sys @@ -69,6 +70,19 @@ DEDICATED_RECOVERY_OPS = RECONCILER_OPS + [ ] DURABLE_TEST_HMAC_KEY = "0" * 64 # 32-byte hex durable key for F4 tests +# #709 F7 (review 438): canonical incident evidence binds the full recovery +# scope, so the fixtures carry stable actor ids, the decision-lock identity, +# the recovery action, both heads, the key version, and a replay nonce. +DECISION_LOCK_ID = "review_decision_lock-prgs-reviewer" +DESTROYED_SUBJECT = "prgs-reviewer terminal approval ledger" +RECOVERY_ACTION = irp.RECOVERY_ACTION_IRRECOVERABLE_PROVENANCE +INCIDENT_NONCE = "11111111-2222-3333-4444-555555555555" +INCIDENT_ISSUED_AT = "2026-07-13T00:00:00+00:00" +AUTHOR_LOGIN = "controller-ops" +AUTHOR_ID = 4242 +MINT_ACTOR_LOGIN = "sysadmin" +MINT_ACTOR_ID = 7 + def _canonical_incident_body( *, @@ -78,6 +92,17 @@ def _canonical_incident_body( org="Scaled-Tech-Consulting", repo="Gitea-Tools", incident_issue=700, + decision_lock_id=DECISION_LOCK_ID, + destroyed_subject=DESTROYED_SUBJECT, + recovery_action=RECOVERY_ACTION, + recorded_head_sha=HEAD_A, + evidence_author_id=AUTHOR_ID, + evidence_author_login=AUTHOR_LOGIN, + mint_actor_id=MINT_ACTOR_ID, + mint_actor_login=MINT_ACTOR_LOGIN, + key_version=None, + nonce=INCIDENT_NONCE, + issued_at=INCIDENT_ISSUED_AT, narrative="forensic diagnosis", ): return irp.build_canonical_incident_body( @@ -85,8 +110,19 @@ def _canonical_incident_body( org=org, repo=repo, pr_number=pr_number, + decision_lock_id=decision_lock_id, + destroyed_subject=destroyed_subject, + recovery_action=recovery_action, + recorded_head_sha=recorded_head_sha, expected_head_sha=head, incident_issue=incident_issue, + evidence_author_id=evidence_author_id, + evidence_author_login=evidence_author_login, + mint_actor_id=mint_actor_id, + mint_actor_login=mint_actor_login, + key_version=key_version or irp.auth_key_version(), + nonce=nonce, + issued_at=issued_at, narrative=narrative, ) @@ -94,26 +130,36 @@ def _canonical_incident_body( def _incident_comment_payload( *, comment_id=11489, - author="controller-ops", + author=AUTHOR_LOGIN, + author_id=None, pr_number=42, head=HEAD_A, remote="prgs", org="Scaled-Tech-Consulting", repo="Gitea-Tools", incident_issue=700, + body=None, + **body_kwargs, ): + uid = AUTHOR_ID if author_id is None else author_id return { "id": comment_id, - "body": _canonical_incident_body( + "body": body + if body is not None + else _canonical_incident_body( pr_number=pr_number, head=head, remote=remote, org=org, repo=repo, incident_issue=incident_issue, + evidence_author_id=uid, + evidence_author_login=author, + **body_kwargs, ), - "user": {"login": author}, + "user": {"id": uid, "login": author}, "created_at": "2026-07-13T00:00:00Z", + "updated_at": "2026-07-13T00:00:00Z", "html_url": f"https://gitea.example/{org}/{repo}/issues/{incident_issue}#issuecomment-{comment_id}", } @@ -1446,5 +1492,744 @@ class TestClearProfileHelperF3(unittest.TestCase): self.assertIsNotNone(still) +def _reorder_canonical_lines(body, first, second): + """Swap two canonical field lines, leaving the digest untouched.""" + lines = body.split("\n") + i = next(i for i, l in enumerate(lines) if l.startswith(f"{first}: ")) + j = next(i for i, l in enumerate(lines) if l.startswith(f"{second}: ")) + lines[i], lines[j] = lines[j], lines[i] + return "\n".join(lines) + + +def _insert_after_canonical_line(body, after_field, extra_line): + lines = body.split("\n") + i = next(i for i, l in enumerate(lines) if l.startswith(f"{after_field}: ")) + lines.insert(i + 1, extra_line) + return "\n".join(lines) + + +class TestF6KeyVersionFailsClosed(unittest.TestCase): + """#709 F6 (review 438): key-version validation must fail closed.""" + + def _verify(self, auth, **kwargs): + params = { + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": "Gitea-Tools", + "pr_number": 42, + "expected_head_sha": HEAD_A, + } + params.update(kwargs) + return irp.verify_authorization_artifact(auth, **params) + + def test_valid_artifact_verifies(self): + self.assertTrue(self._verify(_mint_auth())["valid"]) + + def test_missing_key_version_rejected(self): + auth = _mint_auth() + del auth["key_version"] + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("missing key_version" in r for r in v["reasons"]), v["reasons"] + ) + + def test_empty_key_version_rejected(self): + auth = _mint_auth() + auth["key_version"] = "" + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue(any("empty" in r for r in v["reasons"]), v["reasons"]) + + def test_unknown_key_version_rejected(self): + auth = _mint_auth() + auth["key_version"] = "totally-unknown-version" + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("unknown or does not match" in r for r in v["reasons"]), v["reasons"] + ) + + def test_malformed_key_version_rejected(self): + auth = _mint_auth() + auth["key_version"] = "bad version!" + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue(any("malformed" in r for r in v["reasons"]), v["reasons"]) + + def test_non_string_key_version_rejected(self): + auth = _mint_auth() + auth["key_version"] = ["v1", "v2"] + v = self._verify(auth) + self.assertFalse(v["valid"], v) + + def test_duplicate_key_version_fields_rejected(self): + auth = _mint_auth() + auth["keyVersion"] = auth["key_version"] # identical value, still duplicate + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("duplicate key-version" in r for r in v["reasons"]), v["reasons"] + ) + + def test_duplicate_conflicting_key_version_fields_rejected(self): + auth = _mint_auth() + auth["auth_key_version"] = "v9" + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("duplicate key-version" in r for r in v["reasons"]), v["reasons"] + ) + + def test_nested_key_version_counts_as_duplicate(self): + auth = _mint_auth() + auth["native_provenance"] = dict(auth["native_provenance"]) + auth["native_provenance"]["key_version"] = auth["key_version"] + v = self._verify(auth) + self.assertFalse(v["valid"], v) + + def test_rotation_invalidates_prior_version_artifact(self): + """Rotating the configured version must reject artifacts minted under the old one.""" + auth = _mint_auth() + self.assertTrue(self._verify(auth)["valid"]) + irp.reset_process_auth_secret_for_tests() + try: + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v2", + }, + clear=False, + ): + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("does not match the configured" in r for r in v["reasons"]), + v["reasons"], + ) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_wrong_version_fails_even_when_key_unchanged(self): + """Version mismatch alone fails: the durable key staying the same is not enough.""" + irp.reset_process_auth_secret_for_tests() + try: + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", + }, + clear=False, + ): + auth = _mint_auth() + self.assertEqual(auth["key_version"], "v1") + self.assertTrue(self._verify(auth)["valid"]) + irp.reset_process_auth_secret_for_tests() + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, # same key + irp.ENV_AUTH_HMAC_KEY_VERSION: "v2", # rotated version + }, + clear=False, + ): + self.assertFalse(self._verify(auth)["valid"]) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_wrong_key_fails_after_restart(self): + """A different durable key must reject the artifact even at the same version.""" + irp.reset_process_auth_secret_for_tests() + try: + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", + }, + clear=False, + ): + auth = _mint_auth() + irp.reset_process_auth_secret_for_tests() # simulate restart + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: "1" * 64, # different durable key + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", # same version + }, + clear=False, + ): + v = self._verify(auth) + self.assertFalse(v["valid"], v) + self.assertTrue( + any("server_signature invalid" in r for r in v["reasons"]), + v["reasons"], + ) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_same_durable_key_verifies_after_restart(self): + irp.reset_process_auth_secret_for_tests() + try: + env = { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", + } + with patch.dict(os.environ, env, clear=False): + auth = _mint_auth() + irp.reset_process_auth_secret_for_tests() # simulate restart + with patch.dict(os.environ, env, clear=False): + self.assertTrue(self._verify(auth)["valid"]) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_key_version_is_inside_authenticated_data(self): + """Editing key_version must break the MAC, not just the version check.""" + irp.reset_process_auth_secret_for_tests() + try: + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", + }, + clear=False, + ): + auth = _mint_auth() + signature_v1 = auth["server_signature"] + irp.reset_process_auth_secret_for_tests() + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v2", + }, + clear=False, + ): + rotated = _mint_auth() + # Same key + same scope, different version => different MAC. + self.assertNotEqual(signature_v1, rotated["server_signature"]) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_production_requires_configured_key_version(self): + irp.reset_process_auth_secret_for_tests() + try: + with patch.object( + irp.mcp_daemon_guard, "is_pytest_runtime", return_value=False + ), patch.dict( + os.environ, {irp.ENV_AUTH_HMAC_KEY: DURABLE_TEST_HMAC_KEY}, clear=False + ): + os.environ.pop(irp.ENV_AUTH_HMAC_KEY_VERSION, None) + with self.assertRaises(irp.AuthSecretError) as ctx: + irp.auth_key_version() + self.assertIn(irp.ENV_AUTH_HMAC_KEY_VERSION, str(ctx.exception)) + finally: + irp.reset_process_auth_secret_for_tests() + + def test_production_requires_durable_key(self): + irp.reset_process_auth_secret_for_tests() + try: + with patch.object( + irp.mcp_daemon_guard, "is_pytest_runtime", return_value=False + ), patch.dict(os.environ, {}, clear=False): + os.environ.pop(irp.ENV_AUTH_HMAC_KEY, None) + with self.assertRaises(irp.AuthSecretError): + irp.auth_key_version() + finally: + irp.reset_process_auth_secret_for_tests() + + def test_errors_never_leak_key_material(self): + irp.reset_process_auth_secret_for_tests() + try: + secret = "s3cr3t" + "9" * 58 + with patch.dict( + os.environ, + { + irp.ENV_AUTH_HMAC_KEY: secret, + irp.ENV_AUTH_HMAC_KEY_VERSION: "v1", + }, + clear=False, + ): + auth = _mint_auth() + self.assertNotIn("key", {k.lower(): 1 for k in ()}) # no-op guard + blob = json.dumps(auth) + self.assertNotIn(secret, blob) + auth["key_version"] = "nope" + v = self._verify(auth) + self.assertNotIn(secret, json.dumps(v["reasons"])) + finally: + irp.reset_process_auth_secret_for_tests() + + +class TestF7StrictCanonicalIncidentEvidence(unittest.TestCase): + """#709 F7 (review 438): only the exact canonical representation is accepted.""" + + def _assess(self, payload, **kwargs): + params = { + "incident_issue": 700, + "incident_comment_id": 11489, + "comment_payload": payload, + "expected_remote": "prgs", + "expected_org": "Scaled-Tech-Consulting", + "expected_repo": "Gitea-Tools", + "expected_pr_number": 42, + "expected_head_sha": HEAD_A, + "expected_decision_lock_id": DECISION_LOCK_ID, + "expected_recovery_action": RECOVERY_ACTION, + "mint_actor_id": MINT_ACTOR_ID, + "mint_actor_username": MINT_ACTOR_LOGIN, + } + params.update(kwargs) + return irp.assess_incident_evidence(**params) + + def test_canonical_evidence_accepted(self): + g = self._assess(_incident_comment_payload()) + self.assertTrue(g["valid"], g) + + def test_reordered_fields_rejected(self): + body = _reorder_canonical_lines(_canonical_incident_body(), "org", "repo") + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("canonical order" in r for r in g["reasons"]), g["reasons"] + ) + + def test_duplicate_identical_field_rejected(self): + body = _canonical_incident_body() + body = _insert_after_canonical_line(body, "repo", "repo: Gitea-Tools") + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_duplicate_conflicting_field_rejected(self): + body = _canonical_incident_body() + body = _insert_after_canonical_line(body, "repo", "repo: Other-Repo") + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_conflicting_field_in_narrative_rejected(self): + body = _canonical_incident_body(narrative="context") + "\nrepo: Other-Repo" + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("outside the signed block" in r for r in g["reasons"]), g["reasons"] + ) + + def test_second_marker_rejected(self): + body = _canonical_incident_body(narrative="context") + body = f"{body}\n\n{irp.INCIDENT_MARKER}" + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_marker_not_first_rejected(self): + body = "preamble\n" + _canonical_incident_body() + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("marker position" in r for r in g["reasons"]), g["reasons"] + ) + + def test_unknown_extra_field_rejected(self): + body = _insert_after_canonical_line( + _canonical_incident_body(), "repo", "sneaky: value" + ) + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_missing_field_rejected(self): + body = "\n".join( + l + for l in _canonical_incident_body().split("\n") + if not l.startswith("nonce: ") + ) + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_empty_field_rejected(self): + body = _canonical_incident_body().replace( + f"decision_lock_id: {DECISION_LOCK_ID}", "decision_lock_id: " + ) + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_conflicting_actor_logins_rejected(self): + payload = _incident_comment_payload() + payload["user"] = {"id": AUTHOR_ID, "login": AUTHOR_LOGIN, "username": "someone-else"} + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("conflicting logins" in r for r in g["reasons"]), g["reasons"] + ) + + def test_conflicting_actor_ids_rejected(self): + payload = _incident_comment_payload() + payload["user"] = {"id": AUTHOR_ID, "user_id": 999, "login": AUTHOR_LOGIN} + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("conflicting user ids" in r for r in g["reasons"]), g["reasons"] + ) + + def test_display_name_only_actor_rejected(self): + payload = _incident_comment_payload() + payload["user"] = {"login": AUTHOR_LOGIN} # no stable id + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("stable user id" in r for r in g["reasons"]), g["reasons"] + ) + + def test_author_id_substitution_rejected(self): + """Body claims one author id, live comment is authored by another.""" + payload = _incident_comment_payload() + payload["user"] = {"id": 5150, "login": AUTHOR_LOGIN} + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("evidence_author_id" in r for r in g["reasons"]), g["reasons"] + ) + + def test_edited_comment_rejected(self): + payload = _incident_comment_payload() + payload["updated_at"] = "2026-07-14T00:00:00Z" + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue(any("edited" in r for r in g["reasons"]), g["reasons"]) + + def test_self_authored_by_stable_id_rejected(self): + payload = _incident_comment_payload(author=MINT_ACTOR_LOGIN, author_id=MINT_ACTOR_ID) + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("self-authored" in r for r in g["reasons"]), g["reasons"] + ) + + def test_mint_actor_substitution_rejected(self): + g = self._assess(_incident_comment_payload(), mint_actor_id=999, mint_actor_username="someone") + self.assertFalse(g["valid"], g) + self.assertTrue( + any("mint_actor" in r for r in g["reasons"]), g["reasons"] + ) + + def test_decision_lock_substitution_rejected(self): + payload = _incident_comment_payload(decision_lock_id="review_decision_lock-prgs-merger") + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("decision_lock_id" in r for r in g["reasons"]), g["reasons"] + ) + + def test_recovery_action_substitution_rejected(self): + body = _canonical_incident_body().replace( + f"recovery_action: {RECOVERY_ACTION}", "recovery_action: clear_decision_lock" + ) + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + + def test_unsupported_recovery_action_cannot_be_built(self): + with self.assertRaises(ValueError): + _canonical_incident_body(recovery_action="merge_pr") + + def test_cross_pr_replay_rejected(self): + payload = _incident_comment_payload(pr_number=99) + g = self._assess(payload) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("pr_number" in r for r in g["reasons"]), g["reasons"] + ) + + def test_cross_repository_replay_rejected(self): + payload = _incident_comment_payload(repo="Other-Repo") + g = self._assess(payload) + self.assertFalse(g["valid"], g) + + def test_cross_org_replay_rejected(self): + payload = _incident_comment_payload(org="Other-Org") + g = self._assess(payload) + self.assertFalse(g["valid"], g) + + def test_cross_remote_replay_rejected(self): + payload = _incident_comment_payload(remote="dadeschools") + g = self._assess(payload) + self.assertFalse(g["valid"], g) + + def test_cross_head_replay_rejected(self): + payload = _incident_comment_payload(head=HEAD_B) + g = self._assess(payload) + self.assertFalse(g["valid"], g) + + def test_recorded_head_substitution_rejected(self): + payload = _incident_comment_payload(recorded_head_sha=HEAD_B) + g = self._assess(payload, expected_recorded_head_sha=HEAD_A) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("recorded_head_sha" in r for r in g["reasons"]), g["reasons"] + ) + + def test_key_version_substitution_rejected(self): + payload = _incident_comment_payload(key_version="v9") + g = self._assess(payload, expected_key_version=irp.auth_key_version()) + self.assertFalse(g["valid"], g) + + def test_digest_preserving_substitution_rejected(self): + """Swap a field *and* its digest from another scope: still refused. + + The attacker mints a fully valid canonical body for a different PR (so + the digest is internally consistent) and presents it for this scope. + """ + foreign = _canonical_incident_body(pr_number=99) + parsed = irp.parse_canonical_incident_body(foreign) + self.assertTrue(parsed["valid"], parsed) # internally consistent + g = self._assess(_incident_comment_payload(body=foreign)) + self.assertFalse(g["valid"], g) + + def test_field_swap_without_digest_update_rejected(self): + body = _canonical_incident_body().replace( + "repo: Gitea-Tools", "repo: Other-Repo" + ) + g = self._assess( + _incident_comment_payload(body=body), expected_repo="Other-Repo" + ) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("content_digest" in r for r in g["reasons"]), g["reasons"] + ) + + def test_nonce_binds_digest(self): + body = _canonical_incident_body().replace( + f"nonce: {INCIDENT_NONCE}", "nonce: 00000000-0000-0000-0000-000000000000" + ) + g = self._assess(_incident_comment_payload(body=body)) + self.assertFalse(g["valid"], g) + self.assertTrue( + any("content_digest" in r for r in g["reasons"]), g["reasons"] + ) + + def test_builder_refuses_ambiguous_narrative(self): + with self.assertRaises(ValueError): + _canonical_incident_body(narrative=f"{irp.INCIDENT_MARKER}\nrepo: evil") + + def test_builder_refuses_multiline_field(self): + with self.assertRaises(ValueError): + _canonical_incident_body(destroyed_subject="line1\nrepo: evil") + + def test_builder_refuses_empty_field(self): + with self.assertRaises(ValueError): + _canonical_incident_body(decision_lock_id="") + + def test_builder_output_is_the_accepted_format(self): + body = _canonical_incident_body() + parsed = irp.parse_canonical_incident_body(body) + self.assertTrue(parsed["valid"], parsed) + self.assertEqual( + parsed["canonical_block"], + irp.render_canonical_incident_block(parsed["fields"]), + ) + + +class TestF8ArchivePrerequisiteForClear(unittest.TestCase): + """#709 F8 (review 438): never clear terminal evidence without a durable archive.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.env = patch.dict( + os.environ, + { + "GITEA_MCP_SESSION_STATE_DIR": self._tmp.name, + "GITEA_SESSION_PROFILE_LOCK": "prgs-merger", + }, + clear=False, + ) + self.env.start() + import mcp_server + + self.mcp = mcp_server + self.mcp._REVIEW_DECISION_LOCK = None + ss.save_state( + kind=ss.KIND_DECISION_LOCK, + payload=_lock([APPROVE], profile="prgs-reviewer", head=HEAD_A), + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + profile_identity="prgs-reviewer", + state_dir=self._tmp.name, + ) + + def tearDown(self): + self.mcp._REVIEW_DECISION_LOCK = None + self.env.stop() + self._tmp.cleanup() + + def _clear(self): + return self.mcp._clear_decision_lock_for_profile( + profile_identity="prgs-reviewer", + pr_number=100, + expected_head_sha=HEAD_A, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + + def _lock_still_present(self): + return ss.load_state_for_profile( + kind=ss.KIND_DECISION_LOCK, + profile_identity="prgs-reviewer", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + skip_identity_match=True, + ) + + def _fail_archive_only(self, behavior): + """Patch save_state so archive writes fail but other writes pass through.""" + real = ss.save_state + + def _fake(**kwargs): + if kwargs.get("kind") == ss.KIND_DECISION_LOCK_ARCHIVE: + return behavior() + return real(**kwargs) + + return patch.object(self.mcp.mcp_session_state, "save_state", side_effect=_fake) + + def test_archive_exception_retains_lock(self): + def _boom(): + raise OSError("disk failure") + + with self._fail_archive_only(_boom): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertTrue(out["terminal_lock_retained"], out) + self.assertTrue(out["recovery_required"], out) + self.assertEqual(out["archive_failed_step"], "archive_save_state") + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_archive_false_response_retains_lock(self): + with self._fail_archive_only(lambda: False): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_archive_empty_response_retains_lock(self): + with self._fail_archive_only(lambda: {}): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_archive_timeout_retains_lock(self): + def _timeout(): + raise TimeoutError("session state write timed out") + + with self._fail_archive_only(_timeout): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_archive_unreadable_retains_lock(self): + """Write claims success but read-back finds nothing: still refuse to clear.""" + real = ss.load_state_for_profile + + def _fake(**kwargs): + if kwargs.get("kind") == ss.KIND_DECISION_LOCK_ARCHIVE: + return None + return real(**kwargs) + + with patch.object( + self.mcp.mcp_session_state, "load_state_for_profile", side_effect=_fake + ): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertEqual(out["archive_failed_step"], "archive_readback") + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_partial_archive_readback_retains_lock(self): + """Read-back returns a record for a different PR/head: refuse to clear.""" + real = ss.load_state_for_profile + + def _fake(**kwargs): + if kwargs.get("kind") == ss.KIND_DECISION_LOCK_ARCHIVE: + return {"archived_for_pr": 999, "archived_for_head": HEAD_B} + return real(**kwargs) + + with patch.object( + self.mcp.mcp_session_state, "load_state_for_profile", side_effect=_fake + ): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertEqual(out["archive_failed_step"], "archive_readback") + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + def test_archive_failure_records_actionable_recovery_evidence(self): + with self._fail_archive_only(lambda: False): + out = self._clear() + self.assertFalse(out["cleared"], out) + self.assertTrue(out["retry_safe"], out) + self.assertIn("archival failed", out["reason"]) + self.assertIsNotNone(out.get("prior_summary"), out) + # The recovery row is keyed by the active (merging) session profile. + recovery = ss.load_state_for_profile( + kind=ss.KIND_POST_MERGE_DECISION_RECOVERY, + profile_identity="prgs-merger", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + skip_identity_match=True, + ) + self.assertIsNotNone(recovery, "no durable recovery evidence recorded") + self.assertEqual(recovery["failed_step"], "archive_save_state") + self.assertEqual(recovery["target_profile_identity"], "prgs-reviewer") + + def test_successful_archive_clears_exactly_once(self): + out = self._clear() + self.assertTrue(out["cleared"], out) + self.assertTrue(out["archive_ok"], out) + self.assertIsNone(self._lock_still_present(), "lock should be cleared") + archived = ss.load_state_for_profile( + kind=ss.KIND_DECISION_LOCK_ARCHIVE, + profile_identity="prgs-reviewer-archive-pr100", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + skip_identity_match=True, + ) + self.assertIsNotNone(archived, "archive not durable") + self.assertEqual(archived["archived_for_pr"], 100) + + # A second clear is a no-op, not a duplicate clear. + again = self._clear() + self.assertFalse(again["cleared"], again) + + def test_retry_after_archive_failure_succeeds(self): + with self._fail_archive_only(lambda: False): + first = self._clear() + self.assertFalse(first["cleared"], first) + self.assertIsNotNone(self._lock_still_present()) + + retry = self._clear() + self.assertTrue(retry["cleared"], retry) + self.assertIsNone(self._lock_still_present()) + + def test_no_alternate_path_clears_after_archive_failure(self): + """The post-merge reconciler must not clear the lock when archival failed.""" + with self._fail_archive_only(lambda: False), patch.object( + self.mcp, "api_request", return_value={} + ), patch.object( + self.mcp, "repo_api_url", return_value="https://example.test/api/v1/repos/o/r" + ): + report = self.mcp._reconcile_decision_locks_after_merge( + pr_number=100, + head_sha=HEAD_A, + merge_commit_sha="c" * 40, + remote="prgs", + host="h", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + auth={"Authorization": "token test"}, + ) + self.assertFalse(report.get("cleared_any"), report) + self.assertIsNotNone(self._lock_still_present(), "terminal lock destroyed") + + if __name__ == "__main__": unittest.main()