diff --git a/branch_cleanup_guard.py b/branch_cleanup_guard.py index 64db84a..e91ea02 100644 --- a/branch_cleanup_guard.py +++ b/branch_cleanup_guard.py @@ -525,6 +525,90 @@ def assess_ownership_record_activity(record: dict[str, Any]) -> dict[str, Any]: } +# Reviewer-lease reclaim is only reachable from a non-live (expired/stale) lease. +_RECLAIMABLE_REVIEWER_STATUSES = _EXPIRED_STATUSES | _STALE_STATUSES + + +def is_active_ownership_status(status: str | None) -> bool: + """True when *status* denotes live/active ownership of a branch (#855). + + Used to decide whether a *competing* active claimant still uses a branch + when weighing an expired reviewer lease for reclaim. Expired, stale, + released, and terminal statuses are not active. + """ + return _norm_str(status).lower() in _ACTIVE_OWNERSHIP_STATUSES + + +def assess_expired_reviewer_lease_reclaim( + *, + role: str, + status: str, + pr_merged: bool | None, + owner_pid_alive: bool | None, + competing_active_claimant: bool | None, +) -> dict[str, Any]: + """Decide, explicitly and fail-closed, whether an expired reviewer lease + may stop protecting an already-merged branch (#855 AC4). + + An expired reviewer lease should not protect a merged branch forever once + its work is done and no live claimant remains. Reclaim is permitted only + when **every** condition below is provably satisfied; any unknown + (``None``) or contrary value keeps the lease protective: + + - the lease is a ``reviewer`` lease (author/merger/controller/reconciler + leases are out of scope and always keep protecting); + - its status is expired or stale (never an active/live lease); + - the PR is proven merged (``pr_merged is True``); + - the lease owner process is proven dead (``owner_pid_alive is False``); + - no competing active claimant uses the branch + (``competing_active_claimant is False``). + + Returns a decision dict with ``reclaim_allowed`` and, when refused, the + fail-closed ``reasons``. The reasons never contain secrets — only the + role, the status, and which condition was unproven. + """ + reasons: list[str] = [] + normalized_role = _norm_str(role).lower() + normalized_status = _norm_str(status).lower() + + if normalized_role != "reviewer": + reasons.append( + f"lease role '{normalized_role or 'unknown'}' is not a reviewer " + "lease; expired-reviewer reclaim does not apply" + ) + if normalized_status not in _RECLAIMABLE_REVIEWER_STATUSES: + reasons.append( + f"lease status '{normalized_status or 'unknown'}' is not expired " + "or stale; only a non-live reviewer lease may be reclaimed" + ) + if pr_merged is not True: + reasons.append( + "PR merged state is not proven true; reclaim requires an " + "already-merged PR (fail closed)" + ) + if owner_pid_alive is not False: + reasons.append( + "lease owner process liveness is not proven dead; a live owner " + "still protects the branch (fail closed)" + ) + if competing_active_claimant is not False: + reasons.append( + "a competing active claimant may still use the branch; reclaim " + "requires no other active ownership (fail closed)" + ) + + allowed = not reasons + return { + "reclaim_allowed": allowed, + "role": normalized_role, + "status": normalized_status, + "decision": ( + "reclaim_expired_reviewer_lease" if allowed else "keep_protecting" + ), + "reasons": [] if allowed else reasons, + } + + def assess_active_branch_ownership( *, remote: str, diff --git a/docs/architecture/mcp-restart-governance.md b/docs/architecture/mcp-restart-governance.md new file mode 100644 index 0000000..2931d10 --- /dev/null +++ b/docs/architecture/mcp-restart-governance.md @@ -0,0 +1,223 @@ +# ADR: MCP restart governance and authorization policy + +- **Status:** Accepted (policy effective immediately for LLM and operator sessions; enforcement tooling may lag) +- **Date:** 2026-07-23 +- **Tracking issue:** [#656](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/656) +- **Policy version:** `restart-governance/v1` +- **Related:** + - Umbrella: [#655](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/655) — governed MCP restart coordination and zero-disruption recovery + - Vision: [#652](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/652) — MCP Control Plane Web Console product vision (§A system health and process control) + - Roadmap: [#653](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/653) — Control Plane Web Console phased delivery (Phase 2 restart controls) + - Contamination guard: [#630](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/630) — blocks manual process-kill recovery + - Console restart UX: [#642](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/642) — sanctioned restart and graceful reload + - Existing restart / reconnect paths to inventory: [#591](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/591) — auto-restart on master advance (closed); [#584](https://gitea.prgs.cc/Scaled-Tech-Consulting/Gitea-Tools/issues/584) — host auto-reconnect on transport flap + - Stable-control runtime split: `docs/architecture/mcp-stable-control-runtime-policy-adr.md` (#615) + - Client-namespace health: `docs/mcp-namespace-health.md` (#543) + - Reconnect-only EOF recovery: `docs/mcp-namespace-eof-recovery.md` + +## 1. Context + +The Gitea MCP server is the **control plane** for real issue and PR mutations +(create, comment, lock, review, merge, reconcile). The same process serves every +role namespace (`gitea-author`, `gitea-reviewer`, `gitea-merger`, +`gitea-reconciler`, `gitea-controller`) and holds the in-memory capability-gate +code loaded at startup. + +Restarting that process is destructive to concurrent work: + +- It resets every session's identity, preflight, and capability-lease binding. +- It can interrupt a mutation mid-critical-section (a lock acquire, a review + submit, a merge), leaving durable state half-written. +- Relaunching from the wrong checkout or worktree silently changes which code + the control plane runs, defeating master-parity gates (#420 / #615). + +Today there is **no durable written policy** stating who may restart MCP, under +what conditions, that restart is a last resort, and how controller approval, +automated safety gates, and break-glass interact. Operators and LLM sessions +therefore invent restart behavior ad hoc, which makes concurrent multi-role work +unsafe. #630 and #642 need this policy as their backbone. + +This ADR defines that policy. It does **not** implement coordinator code or HA +multi-instance restart (those are later children of #655). + +## 2. Decision + +### 2.1 v1 decision (recorded) + +**Restart authority in v1 is `controller approval + automated safety gates`.** + +A restart of the stable control runtime is authorized only when **both** hold: + +1. A **controller** role explicitly approves the restart, recording an audit + entry (who, why, scope, affected sessions), **and** +2. The **automated safety gates** pass: a completed drain acknowledgement (no + affected session is mid-critical-section) or a declared break-glass incident + (§2.5). + +Quorum among multiple controllers is **not** required day-one. It is deferred +unless a later investigation (tracked under #653) proves single-controller +approval is insufficient. This ADR records the v1 decision so enforcement code +(#630) has a fixed target; changing it requires a superseding ADR. + +### 2.2 Restart is a last resort — the recovery ladder + +Restart is the **last** rung. Before any restart, exhaust the narrower +recoveries, in order: + +1. **Reconnect** the IDE/client MCP namespace (transport EOF, `client is + closing: EOF`, transient `#584` flap). No process change. See + `docs/mcp-namespace-eof-recovery.md`. +2. **Refresh / rebind** the session workspace: re-run `gitea_whoami`, + `gitea_resolve_task_capability`, and pass an explicit validated + `worktree_path`. Fixes stale session context without touching the process. +3. **Scoped restart** of a single misbehaving namespace/service (where the + deployment supports per-service restart) rather than the whole control plane. +4. **Full restart** of the stable control runtime process — operator-owned, + controller-approved, drained. +5. **Host / infrastructure restart** — the broadest action; same authorization + as a full restart plus infrastructure ownership. + +A session **must** try rungs 1–2 and record why they were insufficient before +requesting a restart at rung 3 or above. Skipping straight to restart is a +policy violation. + +### 2.3 Authorization matrix + +| Role | Reconnect (1) | Refresh/rebind (2) | Scoped restart (3) | Full restart (4) | Host restart (5) | +|---|---|---|---|---|---| +| **author** | self | self | request only | **forbidden** | forbidden | +| **reviewer** | self | self | request only | **forbidden** | forbidden | +| **merger** | self | self | request only | **forbidden** | forbidden | +| **reconciler** | self | self | request only | **forbidden** | forbidden | +| **controller** | self | self | **approve** (+gates) | **approve** (+gates) | request to operator | +| **operator** | self | self | execute (controller-approved) | execute (controller-approved) | execute (controller-approved) | +| **admin** | self | self | execute | execute | execute (break-glass) | + +Legend: *self* = may perform for its own client session; *request only* = may +raise a restart request but not authorize or execute it; *approve* = may +authorize under §2.1 gates; *execute* = may perform the process action after the +authorization is recorded. + +Key invariants: + +- **No LLM worker role (author/reviewer/merger/reconciler) may perform or + authorize a full or host restart.** They may only reconnect/rebind their own + client and file a restart request. +- **Controller approval authorizes; operator/admin executes.** The approving + controller and the executing operator may be the same human, but both the + approval and the execution are audited. +- Privileged process actions (full restart, host restart) are reserved to + **operator/admin**, never to an automated worker. + +### 2.4 Approved conditions + +A restart at rung 3+ is approved only under one of these recorded conditions: + +- **No affected sessions:** the control plane has no live session that would be + interrupted (verified, not assumed). +- **Full drain acknowledged:** every affected session has drained + (no open critical section — no held mutation lease mid-write) and the drain is + acknowledged in the audit record. +- **Controller + gates:** controller approval plus passing automated safety + gates (§2.1), the standard v1 path. +- **Quorum:** not required in v1; reserved for a future superseding ADR. +- **Break-glass:** an incident-backed emergency exception (§2.5). + +Restart **never** bypasses mutation gates mid-critical-section. Drain before +restart is mandatory except under break-glass with a declared incident. + +### 2.5 Break-glass + +Break-glass is a **separate, narrower** authorization path for emergencies where +the normal drain-and-approve path cannot complete (e.g. the control plane is +wedged and cannot drain). + +Break-glass conditions: + +- A declared incident record exists (id, timestamp, declarer) **before** the + action. +- The action is taken by **operator or admin** authority only — never by an LLM + worker role, and never unilaterally by an operator with active peers when a + controller is reachable. +- The scope is the minimum necessary rung of the ladder. +- A **mandatory post-hoc audit** entry is filed: what was restarted, why the + normal path was impossible, which sessions were affected, and the incident id. + +Break-glass suspends the drain requirement, not the audit requirement. + +### 2.6 Explicit prohibitions + +- **A unilateral LLM or operator full restart while active peer sessions + exist is forbidden.** An LLM worker role must not kill, restart, or relaunch + the MCP process; a lone operator must not full-restart over live peer work + without controller approval or a break-glass incident. +- Process-kill recovery is forbidden as a routine tool (#630). This ADR does not + introduce a kill path. +- Ambiguous policy state **denies** restart (§4). + +## 3. Security requirements + +- Full restart and host restart are **privileged**; only operator/admin execute + them, only after a controller approval or break-glass incident is recorded. +- Break-glass is a distinct authorization path with its own audit mandate; it is + never the default and never silent. +- **Every approval and every restart action is audited** (who approved, who + executed, scope, affected sessions, condition, policy version). No restart is + authorized without a durable audit entry. + +## 4. Failure behavior + +**Ambiguous policy → deny restart.** If it cannot be established that a +restart is authorized under §2 — unknown affected-session state, missing +controller approval, absent break-glass incident, or an unclassifiable request — +the safe action is to **refuse** the restart and stop with a recovery report, +never to restart on assumption. + +## 5. Policy IDs (for enforcement code) + +Enforcement code — the restart coordinator (a later child of #655), the #630 +contamination guard, and the #642 console restart UX — binds to these stable +policy identifiers rather than to prose: + +| Policy ID | Statement | +|---|---| +| `RG-01` | Restart is last resort; rungs 1–2 must be tried and recorded first (§2.2). | +| `RG-02` | v1 authority = controller approval + automated safety gates (§2.1). | +| `RG-03` | No LLM worker role performs or authorizes full/host restart (§2.3). | +| `RG-04` | Full/host restart executed by operator/admin only, post approval (§2.3). | +| `RG-05` | Drain before restart is mandatory except break-glass with incident (§2.4). | +| `RG-06` | Break-glass requires a pre-declared incident and post-hoc audit (§2.5). | +| `RG-07` | Unilateral LLM/operator full restart with active peers is forbidden (§2.6). | +| `RG-08` | Ambiguous policy state denies restart (§4). | + +The `restart-governance/v1` **policy version** field is emitted on future +restart audit events so approvals can be reconciled against the policy revision +in force. + +## 6. Dogfooding + +Gitea-Tools governs its own MCP control plane by this policy. Author, reviewer, +merger, and reconciler sessions operating on this repository use the recovery +ladder (§2.2) — reconnect and rebind, never self-restart — and any real restart +of the Gitea-Tools stable control runtime follows the controller-approval + +drain path defined here. + +## 7. Acceptance and cross-links + +This ADR is the authoritative restart-governance policy. It **must** stay +cross-linked from the safety model and the web-console deployment boundary: + +- `docs/safety-model.md` § Process restart governance references this ADR. +- `docs/webui-deployment.md` references this ADR for restart/reload disposition. + +It is linked to its issue lineage — umbrella **#655**, vision **#652**, roadmap +**#653**, contamination guard **#630**, and console restart UX **#642** — in +§ Related above. + +## 8. Non-goals + +- Implementing the restart coordinator or approval state machine (#630, later + children of #655). +- Implementing HA multi-instance restart or quorum machinery. +- Introducing any process-kill or auto-restart tool; existing auto-restart + behavior must be inventoried before any new restart tool is enabled. diff --git a/docs/mcp-restart-path-inventory.md b/docs/mcp-restart-path-inventory.md new file mode 100644 index 0000000..c880269 --- /dev/null +++ b/docs/mcp-restart-path-inventory.md @@ -0,0 +1,90 @@ +# MCP restart / reload / kill path inventory (#657) + +Complete inventory of every code, script, and host path that can **restart, +reload, reconnect, kill, or force-recreate** an MCP process in this project, +with each path classified and linked to the guard that constrains it. + +This document is the human-readable companion to the machine-readable registry +in [`mcp_restart_paths.py`](../mcp_restart_paths.py). The two are kept in +lock-step by [`tests/test_mcp_restart_paths.py`](../tests/test_mcp_restart_paths.py): +every `path_id` below must appear in this file, and the source guards are run +against the live tree. + +Roadmap linkage: this inventory is the enumeration step of the restart +governance work — parent **#655**, restart-governance ADR **#656**, vision +**#652**, roadmap **#653**. Related detection/guard work: master-advance +staleness **#591**/**#420**, side-effect-free resolver **#685**, transport flap +**#584**, manual-kill contamination **#630**. + +## Classifications + +| Classification | Meaning | +|---|---| +| `sanctioned_narrow_recovery` | One-shot, safe-by-construction recovery that never targets the running daemon. | +| `guarded_fail_closed` | Detects a restart-requiring condition, then fails mutations closed and emits reconnect guidance. Never self-restarts. | +| `forbidden` | A workflow-safety violation; where an LLM tool could invoke it, it is marked contamination. | +| `removed` | A previously-existing unguarded restart primitive that has been deleted; a regression guard keeps it absent. | +| `host_residual` | Behavior owned by the host/IDE, outside this process's control. Documented, not code-guarded here. | + +## The rule + +**No component may perform an unguarded full restart of the MCP daemon.** The +in-process daemon (`gitea_mcp_server.py`, `mcp_server.py`, +`role_session_router.py`) must never replace or terminate its own process: +replacing the process after the host has wired up the stdio pipes desyncs the +JSON-RPC transport (observed with Antigravity/Cascade hosts). Recovery is owned +by the host/operator via a client reconnect — the daemon only ever *detects* +and *fails closed*. + +## Inventory + +| path_id | Classification | Mechanism | Guard | Refs | +|---|---|---|---|---| +| `cli_venv_bootstrap_execv` | sanctioned_narrow_recovery | CLI wrapper scripts re-exec into `venv/bin/python3` via `os.execv`, guarded by `sys.executable != venv_python`. | One-shot pre-import bootstrap; runs before any MCP transport exists and only when not already on the venv interpreter; idempotent guard prevents a re-exec loop. | #657 | +| `daemon_self_replacement` | forbidden | The daemon replacing/terminating its own process (`os.execv`/`os.kill`/`os._exit`) to reload code. | Forbidden by design; enforced against the source tree by `assert_no_daemon_self_replacement()`. | #657, #584 | +| `legacy_auto_restart_helper` | removed | A helper (`_trigger_mcp_auto_restart`) that actively restarted the server from the read-only resolver path. | Removed in #685; kept absent by `assert_auto_restart_helper_absent()`. | #685, #657 | +| `config_touch_reload` | removed | Touching (utime) the MCP client config to make the host reload the server. | Removed from the resolver in #685: stale detection is report-only, never mutating config, spawning threads, or calling `os._exit`. | #685, #657 | +| `master_advance_auto_restart` | guarded_fail_closed | On-disk master advancing past the running code. | `master_parity_gate` captures startup parity and blocks mutations while stale, emitting restart guidance; the process never self-restarts. | #420, #591, #657 | +| `stale_runtime_resolver_reconnect` | guarded_fail_closed | The capability resolver detecting a stale serving process. | Report-only (#685): returns `restart_required`/`stop_required` and an exact reconnect action; no restart, thread, config touch, or `os._exit`. | #685, #657 | +| `manual_daemon_kill` | forbidden | Shell kills of the daemon: `pkill -f mcp_server.py`, `killall`, broad `pkill -f python` sweeps, or `kill ` of a daemon pid. | Forbidden (#630): `runtime_recovery_guard` classifies these as contamination and `gitea_record_daemon_process_kill_attempt` writes a durable marker that fails later mutations closed. Operator maintenance authorization is read only from the environment. | #630, #657 | +| `conflict_marker_infra_stop` | guarded_fail_closed | The daemon entrypoint scans for unresolved merge-conflict markers at startup and stops (`sys.exit(1)`). | Fail-closed startup stop, not a restart: the process exits and waits for the operator to resolve conflicts and relaunch; never loops. | #657 | +| `ide_client_reconnect` | host_residual | A manual `/mcp reconnect` (or equivalent host action) that recreates the MCP client connection. | Outside this process's control; the sanctioned recovery the gates point operators toward. No in-process code initiates it. | #584, #656, #657 | +| `profile_switch_runtime` | sanctioned_narrow_recovery | Switching the active execution profile at runtime (dynamic-profile mode). | In-process and restart-free: `runtime_switching_supported` is true, so a switch rebinds capability without recreating the process. | #656, #657 | + +## Guards enforced in CI + +`tests/test_mcp_restart_paths.py` asserts, against the live source tree: + +1. **Registry well-formedness** — every path has a valid classification, a + non-empty guard description, references, and locations; ids are unique; all + five classifications are represented. +2. **Unknown restart attempts fail closed** — + `assert_restart_attempt_registered()` raises `UnknownRestartPathError` for + any path id not in this inventory, so a novel/unnamed restart primitive + cannot slip through silently. +3. **Daemon never self-replaces** — `assert_no_daemon_self_replacement()` scans + the daemon modules for `os.execv`/`os.kill`/`os._exit`/`os.abort` calls + (comment/docstring mentions are ignored) and finds none. +4. **Legacy helper stays removed** — `assert_auto_restart_helper_absent()` + confirms `_trigger_mcp_auto_restart` has not returned. +5. **pkill stays forbidden** — a daemon `pkill` command still classifies as + contamination via `runtime_recovery_guard`. + +## Residual host behaviors (outside process control) + +* `/mcp reconnect` in the IDE/host — the sanctioned recovery for stale-runtime, + transport-flap (#584), and worktree-binding conditions. The daemon can only + emit guidance toward it. +* Host-level process management (the operator relaunching the daemon after a + fail-closed stop, or after resolving merge conflicts). + +These are documented rather than code-guarded because the process cannot +observe or gate them from inside itself. + +## Rollout + +Per #657, guards are introduced flag-free as **regression assertions** (they +codify invariants that already hold) before any hard runtime block is layered +on. When the restart coordinator (#655/#656) lands, registered paths gain a +coordinator token/capability check; unregistered attempts already fail closed +today via `assert_restart_attempt_registered()`. diff --git a/docs/safety-model.md b/docs/safety-model.md index 31c740a..bbab241 100644 --- a/docs/safety-model.md +++ b/docs/safety-model.md @@ -46,3 +46,17 @@ If shell helpers are unavailable and MCP commit cannot run, stop with a recovery report (restart session, clear hung terminals, use MCP-native commit). See [`llm-workflow-runbooks.md`](llm-workflow-runbooks.md) § MCP-native commit path (#260) and agent temp artifact cleanup (#261). + +## 7. Process restart governance + +Restarting the MCP control-plane process is destructive to concurrent multi-role +work and is governed by a dedicated policy. Restart is a **last resort** behind +narrower recoveries (reconnect, rebind), full/host restart is reserved to +operator/admin under **controller approval + automated safety gates**, a +unilateral LLM or operator full restart with active peers is **forbidden**, and +ambiguous policy state **denies** restart. Break-glass is a separate, +incident-backed path with a mandatory audit. + +See [`architecture/mcp-restart-governance.md`](architecture/mcp-restart-governance.md) +(#656) for the authorization matrix, the recovery ladder, break-glass +conditions, and the `RG-01`–`RG-08` policy IDs. diff --git a/docs/webui-deployment.md b/docs/webui-deployment.md index fa754ad..2ad46ab 100644 --- a/docs/webui-deployment.md +++ b/docs/webui-deployment.md @@ -55,6 +55,15 @@ shipped to the browser. assumption paths, and the client-secret policy. Use it to verify an instance is configured for internal-only operation. +## Process restart / reload disposition + +The console never exposes a restart or reload control; process restart of the +MCP control-plane runtime is governed separately. Restart is a last resort behind +reconnect/rebind, full restart is operator/admin-only under controller approval +plus safety gates, and break-glass is an incident-backed path. See +[`architecture/mcp-restart-governance.md`](architecture/mcp-restart-governance.md) +(#656). + ## Non-goals (MVP) - Full SSO or session login in the UI diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed5fc11..e5272e0 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -2446,6 +2446,20 @@ def _evaluate_issue_lock_recovery( descendant_sha=local_head, ) + # #871: the inverse of the #768 descendant relation — the *remote* head may + # have advanced past the local/recorded head via a sanctioned merge-based + # branch sync (``gitea_update_pr_branch_by_merge``) while the local worktree + # stayed put. Observe that provenance server-side so the assessor can prove + # it and nothing else. Probed only when the heads differ; never from any + # caller-supplied value. + sync_provenance: dict | None = None + if remote_head and local_head and remote_head != local_head: + sync_provenance = issue_lock_worktree.read_merge_sync_provenance( + worktree_path, + prior_head_sha=local_head, + synced_head_sha=remote_head, + ) + # #772: with no remote branch there is no head to measure against, so the # base the branch was cut from is observed instead. Probed only in that # case, so the published path's evidence is untouched (#772 AC8). @@ -2488,6 +2502,7 @@ def _evaluate_issue_lock_recovery( remote_branch_exists=remote_branch_exists, recorded_base_sha=recorded_base, base_ancestry=base_ancestry, + sync_provenance=sync_provenance, ) @@ -11117,6 +11132,9 @@ def _collect_branch_ownership_records( """ records: list[dict] = [] inventory_error = False + # #855 AC4: expired/stale reviewer-lease records eligible for an explicit + # reclaim decision, evaluated after the full ownership inventory is built. + reviewer_reclaim_candidates: list[tuple[dict, bool | None]] = [] target_branch = (branch or "").strip() if not target_branch: return {"records": records, "inventory_error": False} @@ -11265,15 +11283,28 @@ def _collect_branch_ownership_records( else: status = freshness_status reclaim_allowed = False - records.append( - _base_rec( - category=category, - status=status, - reclaim_allowed=reclaim_allowed, - role=role, - host=lease_host or host_n or host, - ) + rec = _base_rec( + category=category, + status=status, + reclaim_allowed=reclaim_allowed, + role=role, + host=lease_host or host_n or host, ) + records.append(rec) + # #855 AC4: a reviewer lease that is expired/stale (its owner + # gone) becomes a candidate for an explicit, fail-closed + # reclaim decision made once the full inventory is known. + if ( + role == "reviewer" + and status + in branch_cleanup_guard._RECLAIMABLE_REVIEWER_STATUSES + ): + owner_alive = ( + fr.get("owner_pid_alive") if isinstance(fr, dict) else None + ) + reviewer_reclaim_candidates.append( + (rec, owner_alive if isinstance(owner_alive, bool) else None) + ) except Exception: # O1: fail closed on control-plane inventory errors. inventory_error = True @@ -11344,6 +11375,44 @@ def _collect_branch_ownership_records( ) ) + # #855 AC4: decide, explicitly and fail-closed, whether any expired/stale + # reviewer lease may stop protecting an already-merged branch. This runs + # only after the full ownership inventory is built, so a competing active + # claimant (an active lease, author session, worktree binding, or active + # reviewer comment lease) is visible. An inventory failure keeps every + # reclaim candidate protective (reclaim_allowed stays False). + if reviewer_reclaim_candidates and not inventory_error: + pr_merged_state: bool | None = None + if pr_number is not None and auth and base_api: + try: + pr_live = api_request( + "GET", f"{base_api}/pulls/{int(pr_number)}", auth + ) + if isinstance(pr_live, dict) and pr_live: + pr_merged_state = bool( + pr_live.get("merged") or pr_live.get("merged_at") + ) + except Exception: + # Unknown merged state fails closed (candidate stays protective). + pr_merged_state = None + for cand_rec, owner_alive in reviewer_reclaim_candidates: + competing = any( + other is not cand_rec + and branch_cleanup_guard.is_active_ownership_status( + other.get("status") + ) + for other in records + ) + decision = branch_cleanup_guard.assess_expired_reviewer_lease_reclaim( + role=str(cand_rec.get("role")), + status=str(cand_rec.get("status")), + pr_merged=pr_merged_state, + owner_pid_alive=owner_alive, + competing_active_claimant=competing, + ) + cand_rec["reclaim_allowed"] = decision["reclaim_allowed"] + cand_rec["reclaim_decision"] = decision["decision"] + return {"records": records, "inventory_error": inventory_error} @@ -11406,6 +11475,7 @@ def gitea_reconcile_merged_cleanups( dry_run: bool = True, execute_confirmed: bool = False, limit: int = 50, + pr_number: int | None = None, remote: str = "dadeschools", host: str | None = None, org: str | None = None, @@ -11416,7 +11486,11 @@ def gitea_reconcile_merged_cleanups( Args: dry_run: Defaults to True. When True, only builds the reconciliation report. execute_confirmed: Must be True when dry_run=False. - limit: Max number of closed PRs to inspect. + limit: Max number of closed PRs to inspect (batch mode only; ignored when + ``pr_number`` is set). + pr_number: Optional exact merged PR selector (#855). When set, only that + PR is assessed/acted on (fail closed if missing, unmerged, or + ambiguous). When omitted, existing batch behaviour is preserved. remote: Known Gitea instance ('dadeschools' or 'prgs'). host: Override the Gitea host. org: Override the owner/organization. @@ -11451,11 +11525,120 @@ def gitea_reconcile_merged_cleanups( "audit_phase": audit_reconciliation_mode.current_phase(), } + # #855: optional exact PR pin. Fail closed before any inventory mutation. + exact_pr: int | None = None + if pr_number is not None: + try: + exact_pr = int(pr_number) + except (TypeError, ValueError): + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": "exact_pr", + "selected_pr_number": pr_number, + "reasons": [ + f"pr_number={pr_number!r} is not a valid integer " + "(fail closed; no mutation)" + ], + "blocker_kind": "invalid_pr_number", + } + if exact_pr <= 0: + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": "exact_pr", + "selected_pr_number": exact_pr, + "reasons": [ + f"pr_number={exact_pr} must be a positive integer " + "(fail closed; no mutation)" + ], + "blocker_kind": "invalid_pr_number", + } + h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) base = repo_api_url(h, o, r) - closed_prs = api_get_all(f"{base}/pulls?state=closed", auth, limit=limit) - open_prs = api_get_all(f"{base}/pulls?state=open", auth) + + selection_mode = "batch" + closed_prs: list[dict] = [] + open_prs: list[dict] = [] + if exact_pr is not None: + selection_mode = "exact_pr" + try: + pr_live = api_request("GET", f"{base}/pulls/{exact_pr}", auth) + except Exception as exc: + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": selection_mode, + "selected_pr_number": exact_pr, + "reasons": [ + f"PR #{exact_pr} could not be uniquely resolved " + f"(fail closed; no mutation): {_redact(str(exc))}" + ], + "blocker_kind": "pr_unresolvable", + } + if not isinstance(pr_live, dict) or not pr_live: + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": selection_mode, + "selected_pr_number": exact_pr, + "reasons": [ + f"PR #{exact_pr} could not be uniquely resolved " + "(empty response; fail closed; no mutation)" + ], + "blocker_kind": "pr_unresolvable", + } + live_number = pr_live.get("number") + try: + live_number_int = int(live_number) if live_number is not None else None + except (TypeError, ValueError): + live_number_int = None + if live_number_int != exact_pr: + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": selection_mode, + "selected_pr_number": exact_pr, + "reasons": [ + f"PR #{exact_pr} resolution is ambiguous or mismatched " + f"(live number={live_number!r}; fail closed; no mutation)" + ], + "blocker_kind": "pr_ambiguous", + } + if not (pr_live.get("merged") or pr_live.get("merged_at")): + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": selection_mode, + "selected_pr_number": exact_pr, + "reasons": [ + f"PR #{exact_pr} is not merged " + "(exact-target cleanup requires a merged PR; " + "fail closed; no mutation)" + ], + "blocker_kind": "pr_not_merged", + } + closed_prs = [pr_live] + # Exact mode still needs open heads for remote-delete safety gates. + open_prs = api_get_all(f"{base}/pulls?state=open", auth) + else: + # Preserve historical call order (closed then open) for batch callers/tests. + closed_prs = api_get_all(f"{base}/pulls?state=closed", auth, limit=limit) + open_prs = api_get_all(f"{base}/pulls?state=open", auth) merged_closed: list[dict] = [] remote_branch_exists: dict[str, bool] = {} @@ -11482,6 +11665,13 @@ def gitea_reconcile_merged_cleanups( scratch_candidates = merged_cleanup_reconcile.discover_reviewer_scratch_worktrees( _canonical_local_git_root() ) + # #855: exact-target never inventories or mutates foreign PR scratch trees. + if exact_pr is not None: + scratch_candidates = [ + s + for s in scratch_candidates + if int(s.get("pr_number") or 0) == int(exact_pr) + ] active_reviewer_leases: dict[int, bool] = {} pr_states: dict[int, dict] = {} for scratch in scratch_candidates: @@ -11516,6 +11706,33 @@ def gitea_reconcile_merged_cleanups( active_reviewer_leases=active_reviewer_leases, pr_states=pr_states, ) + report["selection_mode"] = selection_mode + if exact_pr is not None: + report["selected_pr_number"] = exact_pr + # Fail closed if exact pin somehow produced other or zero entries. + entries = list(report.get("entries") or []) + entry_numbers = [] + for entry in entries: + try: + entry_numbers.append(int(entry.get("pr_number"))) + except (TypeError, ValueError): + entry_numbers.append(entry.get("pr_number")) + if entry_numbers != [exact_pr]: + return { + "success": False, + "performed": False, + "executed": False, + "dry_run": bool(dry_run), + "selection_mode": selection_mode, + "selected_pr_number": exact_pr, + "reasons": [ + f"exact PR #{exact_pr} selection produced unexpected " + f"candidate set {entry_numbers!r} " + "(fail closed; no mutation)" + ], + "blocker_kind": "exact_selection_mismatch", + "entries": entries, + } if dry_run: report["dry_run"] = True @@ -18890,10 +19107,59 @@ def gitea_update_pr_branch_by_merge( prepared_verdict_head_sha=live_pr_head, ) - return { - "success": True, + # #871: the remote head is now advanced; the durable linked-issue lock must + # be advanced with it, or a later dead-session recovery can never prove + # ownership at the new head. This runs AFTER the successful remote update, so + # a failure here is a *partial* lifecycle failure — the remote moved but the + # durable state did not — and must never be reported as a full success. + claimant = _work_lease_claimant(h) + matched_issue = ownership.get("matched_issue") + synced_at = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + lock_refresh: dict = { + "refreshed": False, + "reasons": ["durable lock head refresh was not attempted"], + } + if new_head and matched_issue and source_branch and (wt or None): + try: + lock_refresh = issue_lock_store.apply_durable_lock_head_refresh( + remote=remote, + org=o, + repo=r, + issue_number=int(matched_issue), + branch_name=source_branch, + worktree_path=wt, + pr_number=pr_number, + identity=claimant.get("username"), + profile=claimant.get("profile"), + current_pid=os.getpid(), + expected_old_head=live_pr_head, + new_head=new_head, + synced_at=synced_at, + base_head=live_base_head, + ) + except Exception as exc: + lock_refresh = { + "refreshed": False, + "reasons": [ + f"durable lock head refresh raised (fail closed): {_redact(str(exc))}" + ], + } + else: + lock_refresh = { + "refreshed": False, + "reasons": [ + "durable lock head refresh could not run: missing new head, " + "linked issue, source branch, or worktree binding" + ], + } + + durable_refreshed = bool(lock_refresh.get("refreshed")) + base_result = { "performed": True, "mutation_allowed": True, + "durable_lock_refreshed": durable_refreshed, + "durable_lock_refresh": lock_refresh, + "fully_synchronized": durable_refreshed, "style": "merge", "force_push": False, "rebase": False, @@ -18915,19 +19181,37 @@ def gitea_update_pr_branch_by_merge( "prepared_verdict_invalidated": transition.get( "prepared_verdict_invalidated" ), - "recommended_next_action": transition.get( - "recommended_next_action", - pr_sync_status.ACTION_FRESH_REVIEW_REQUIRED, - ), "transition": transition, "role_kind": role, "profile_name": profile.get("profile_name"), "worktree_path": wt or None, - "reasons": list(transition.get("reasons") or []) + [ - "update-by-merge completed via native Gitea API (style=merge only)" - ], } + if durable_refreshed: + base_result["success"] = True + base_result["recommended_next_action"] = transition.get( + "recommended_next_action", + pr_sync_status.ACTION_FRESH_REVIEW_REQUIRED, + ) + base_result["reasons"] = list(transition.get("reasons") or []) + [ + "update-by-merge completed via native Gitea API (style=merge only)", + f"durable linked-issue lock #{matched_issue} head refreshed to " + f"{new_head} (verified by read-after-write)", + ] + return base_result + + # Partial lifecycle failure: the remote advanced but the durable lock did + # not. Do NOT report a fully successful synchronization (#871). + base_result["success"] = False + base_result["partial_lifecycle_failure"] = True + base_result["recommended_next_action"] = pr_sync_status.ACTION_BLOCKED + base_result["reasons"] = list(lock_refresh.get("reasons") or []) + [ + f"PARTIAL LIFECYCLE FAILURE: PR #{pr_number} remote head advanced to " + f"{new_head} but the durable linked-issue lock head was not refreshed; " + "the synchronization is NOT complete", + ] + return base_result + @mcp.tool() def gitea_assess_conflict_fix_push( diff --git a/issue_lock_recovery.py b/issue_lock_recovery.py index ecc99c6..ae0cc35 100644 --- a/issue_lock_recovery.py +++ b/issue_lock_recovery.py @@ -85,6 +85,12 @@ HEAD_RELATION_STRICT_DESCENDANT = "strict_descendant" # #772: an unpublished claim has no recorded head to compare against at all, so # its head is measured against the base the branch was cut from instead. HEAD_RELATION_DESCENDS_FROM_BASE = "descends_from_recorded_base" +# #871: the remote/PR head advanced *past* the recorded head via a sanctioned +# merge-based branch synchronization (``gitea_update_pr_branch_by_merge``) while +# the local worktree stayed at the recorded head. This is the inverse of the +# #768 descendant relation — here the *remote* strictly descends the local head, +# and only because a base was merged into the branch, proven server-side. +HEAD_RELATION_REMOTE_MERGE_SYNCED = "remote_merge_synced" # Which body of evidence a recovery was decided on (#772 AC10). These are not # interchangeable: a published claim proves ownership against a remote/PR head, @@ -266,6 +272,70 @@ def _assess_base_descendancy( ] +def _assess_remote_merge_synced( + sync_provenance: Mapping[str, Any] | None, + *, + recorded_head: str, + remote_head: str, +) -> tuple[bool, list[str]]: + """Did ``remote_head`` advance past ``recorded_head`` via a sanctioned + merge-based branch sync (#871)? + + ``sync_provenance`` is the server-side git observation from + ``issue_lock_worktree.read_merge_sync_provenance``. Its own + ``prior_head_sha`` / ``synced_head_sha`` are re-checked against the heads + this assessment is actually reasoning about, so an observation taken for some + other pair of commits — stale, mismatched, or hand-built — can never + authorize recovery. This is the inverse of ``_assess_strict_descendant``: the + recorded head is the ancestor and the *remote* head is the descendant, and it + is accepted only because the remote head is a base-into-branch merge that + preserved the branch mainline back to the recorded head. + + Returns ``(proven, notes)``. Notes name the exact missing element so a + refused caller sees why, never a bare "unproven". + """ + if not isinstance(sync_provenance, Mapping): + return False, [ + "no server-derived merge-sync provenance observation was available; a " + "remote head ahead of the recorded head cannot be accepted" + ] + + probe_prior = _text(sync_provenance.get("prior_head_sha")) + probe_synced = _text(sync_provenance.get("synced_head_sha")) + if probe_prior != recorded_head or probe_synced != remote_head: + return False, [ + f"merge-sync observation covers {probe_prior or 'unknown'} -> " + f"{probe_synced or 'unknown'}, not the heads under assessment " + f"({recorded_head} -> {remote_head})" + ] + if not sync_provenance.get("probe_ok"): + return False, ( + list(sync_provenance.get("reasons") or []) + or ["merge-sync provenance probe did not complete; provenance unproven"] + ) + if not sync_provenance.get("prior_is_ancestor"): + return False, [ + f"recorded head {recorded_head} is not an ancestor of remote head " + f"{remote_head}; a rewritten or force-moved head cannot be recovered" + ] + if not sync_provenance.get("is_merge_sync"): + return False, ( + list(sync_provenance.get("reasons") or []) + or [ + f"remote head {remote_head} is not a sanctioned merge-based sync " + f"of the base into the branch above {recorded_head}" + ] + ) + + proof = _text(sync_provenance.get("proof")) or ( + f"{remote_head} merged the base into the branch above {recorded_head}" + ) + return True, [ + f"remote head {remote_head} advanced past recorded head {recorded_head} " + f"via a sanctioned merge-based branch sync ({proof})" + ] + + def assess_dead_session_lock_recovery( existing_lock: Mapping[str, Any] | None, *, @@ -290,6 +360,7 @@ def assess_dead_session_lock_recovery( remote_branch_exists: bool | None = None, recorded_base_sha: str | None = None, base_ancestry: Mapping[str, Any] | None = None, + sync_provenance: Mapping[str, Any] | None = None, ) -> dict[str, Any]: """Decide whether a dead-session author lock may be natively recovered. @@ -469,19 +540,39 @@ def assess_dead_session_lock_recovery( head_relation = HEAD_RELATION_STRICT_DESCENDANT ancestry_proof = notes[0] if notes else None else: - reasons.append( - f"local head {local_head} does not match remote branch head " - f"{remote_head}" + # #871: the reverse relation — the remote head advanced past + # the recorded/local head via a sanctioned merge-based branch + # sync while the local worktree stayed put. Accepted only on + # server-proven merge-sync provenance, never a caller claim. + synced, sync_notes = _assess_remote_merge_synced( + sync_provenance, + recorded_head=local_head, + remote_head=remote_head, ) - reasons.extend(notes) + if synced: + head_relation = HEAD_RELATION_REMOTE_MERGE_SYNCED + ancestry_proof = sync_notes[0] if sync_notes else None + else: + reasons.append( + f"local head {local_head} does not match remote branch " + f"head {remote_head}" + ) + reasons.extend(notes) + reasons.extend(sync_notes) evidence["recorded_base"] = recorded_base or None evidence["local_head"] = local_head or None evidence["remote_head"] = remote_head or None # ``recorded_head`` is the head recovery is being measured against; # ``accepted_head`` is the head this recovery actually adopts. They differ # only in the descendant case, and downstream gates need both (#768 AC2/AC7). + # #871: in the merge-sync case the branch/PR already carries the synced + # remote head, so that is the head recovery adopts; the local worktree stays + # at the ancestor recorded head. evidence["recorded_head"] = remote_head or None - evidence["accepted_head"] = local_head or None + if head_relation == HEAD_RELATION_REMOTE_MERGE_SYNCED: + evidence["accepted_head"] = remote_head or None + else: + evidence["accepted_head"] = local_head or None evidence["head_relation"] = head_relation evidence["ancestry_proof"] = ancestry_proof @@ -493,12 +584,17 @@ def assess_dead_session_lock_recovery( # contradictory; re-stating it as a head mismatch would only obscure why. if not unpublished and local_head and pr_head != local_head: # A descendant recovery has not been published yet, so the open PR - # legitimately still points at the recorded head. Any other - # disagreement is a real mismatch. + # legitimately still points at the recorded head. A merge-sync + # recovery's PR legitimately sits at the advanced remote head. Any + # other disagreement is a real mismatch. if not ( head_relation == HEAD_RELATION_STRICT_DESCENDANT and remote_head and pr_head == remote_head + ) and not ( + head_relation == HEAD_RELATION_REMOTE_MERGE_SYNCED + and remote_head + and pr_head == remote_head ): reasons.append( f"open PR #{pr_number} head {pr_head} does not match local head " @@ -619,7 +715,11 @@ def assess_dead_session_lock_recovery( ) if ( head_relation - in (HEAD_RELATION_STRICT_DESCENDANT, HEAD_RELATION_DESCENDS_FROM_BASE) + in ( + HEAD_RELATION_STRICT_DESCENDANT, + HEAD_RELATION_DESCENDS_FROM_BASE, + HEAD_RELATION_REMOTE_MERGE_SYNCED, + ) and ancestry_proof ): proof.append(ancestry_proof) @@ -697,6 +797,16 @@ def owning_pr_recovery_evidence( return None if accepted_head != local_head: return None + elif relation == HEAD_RELATION_REMOTE_MERGE_SYNCED: + # #871: the PR already sits at the advanced remote head; the local + # worktree is the ancestor the merge preserved. The head the open PR + # shows and the head recovery adopts are both the synced remote head. + if not remote_head or pr_head != remote_head: + return None + if accepted_head != remote_head: + return None + if not local_head or local_head == remote_head: + return None else: return None try: @@ -765,6 +875,18 @@ def recovered_owning_pr_from_lock( return None if not accepted_head or accepted_head == recorded_head: return None + elif relation == HEAD_RELATION_REMOTE_MERGE_SYNCED: + # #871: PR sits at the advanced remote head, which is both the recorded + # measured-against head and the adopted head; the local worktree is the + # ancestor the merge preserved. + remote_head = _text(record.get("remote_head")) + local_head = _text(record.get("local_head")) + if not remote_head or pr_head != remote_head: + return None + if accepted_head and accepted_head != remote_head: + return None + if not local_head or local_head == remote_head: + return None else: return None try: diff --git a/issue_lock_store.py b/issue_lock_store.py index 713fa2a..a963174 100644 --- a/issue_lock_store.py +++ b/issue_lock_store.py @@ -737,4 +737,308 @@ def format_lock_proof( parts.append("lock released") elif released is False: parts.append("lock retained") - return "; ".join(parts) \ No newline at end of file + return "; ".join(parts) + + +# ── #871: durable linked-issue lock head refresh after branch synchronization ── +_FULL_SHA_RE = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE) + +# Provenance recorded on the lock when the head is refreshed by a sanctioned +# merge-based branch synchronization (``gitea_update_pr_branch_by_merge``). +LOCK_HEAD_REFRESH_PROVENANCE_MERGE_SYNC = "gitea_update_pr_branch_by_merge" + + +def _norm_sha(value: Any) -> str | None: + text = str(value or "").strip().lower() + return text if _FULL_SHA_RE.match(text) else None + + +def _lock_claimant_view(lock: dict[str, Any] | None) -> dict[str, Any]: + if not isinstance(lock, dict): + return {} + claimant = lock.get("claimant") + if not isinstance(claimant, dict): + lease = lock.get("work_lease") + claimant = lease.get("claimant") if isinstance(lease, dict) else None + return dict(claimant) if isinstance(claimant, dict) else {} + + +def assess_durable_lock_head_refresh( + existing_lock: dict[str, Any] | None, + *, + remote: str, + org: str, + repo: str, + issue_number: int, + branch_name: str, + worktree_path: str, + pr_number: int | None, + identity: str | None, + profile: str | None, + current_pid: int | None, + expected_old_head: str | None, + new_head: str | None, + base_head: str | None = None, +) -> dict[str, Any]: + """Fail-closed assessment for refreshing a durable lock's recorded head (#871). + + A successful ``gitea_update_pr_branch_by_merge`` advances the *remote* PR head + but must also advance the durable linked-issue lock so a later dead-session + recovery can prove ownership. This decides whether that refresh is permitted; + it mutates nothing. + + Every element of durable ownership is re-verified against the persisted lock — + repository, issue, branch, worktree, claimant identity/profile, and the live + owning session — and the recorded head is compare-and-swapped: the lock's + currently recorded synced head (if any) must equal ``expected_old_head``, so a + lock whose head or provenance changed concurrently is never overwritten. + """ + reasons: list[str] = [] + old = _norm_sha(expected_old_head) + new = _norm_sha(new_head) + evidence: dict[str, Any] = { + "issue_number": issue_number, + "branch_name": branch_name, + "worktree_path": worktree_path, + "pr_number": pr_number, + "expected_old_head": old, + "new_head": new, + "base_head": _norm_sha(base_head), + } + + if not isinstance(existing_lock, dict) or not existing_lock: + reasons.append("no durable lock exists for this issue; nothing to refresh") + return {"allowed": False, "reasons": reasons, "evidence": evidence, + "expected_generation": 0} + + lock = dict(existing_lock) + evidence["current_generation"] = lock_generation(lock) + + if lock.get("issue_number") != issue_number: + reasons.append( + f"durable lock targets issue #{lock.get('issue_number')}, not " + f"#{issue_number}; refusing head refresh" + ) + + for field, expected in (("remote", remote), ("org", org), ("repo", repo)): + actual = str(lock.get(field) or "").strip() + if actual != str(expected or "").strip(): + reasons.append( + f"lock {field} '{actual}' does not match requested " + f"'{str(expected or '').strip()}'" + ) + + locked_branch = str(lock.get("branch_name") or "").strip() + if locked_branch != str(branch_name or "").strip(): + reasons.append( + f"lock branch '{locked_branch}' does not match requested " + f"'{str(branch_name or '').strip()}'" + ) + + locked_worktree = str(lock.get("worktree_path") or "").strip() + try: + same_wt = bool(locked_worktree) and bool(worktree_path) and ( + os.path.realpath(locked_worktree) == os.path.realpath(worktree_path) + ) + except OSError: + same_wt = locked_worktree == (worktree_path or "") + if not same_wt: + reasons.append( + f"lock worktree '{locked_worktree}' does not match declared " + f"'{str(worktree_path or '').strip()}'" + ) + + claimant = _lock_claimant_view(lock) + locked_identity = str(claimant.get("username") or "").strip() + locked_profile = str(claimant.get("profile") or "").strip() + if not locked_identity or not locked_profile: + reasons.append( + "durable lock does not record a claimant identity/profile; " + "ownership could not be proven for head refresh" + ) + if not str(identity or "").strip() or not str(profile or "").strip(): + reasons.append( + "active session identity/profile is unknown; ownership could not be " + "proven for head refresh" + ) + if locked_identity and str(identity or "").strip() and locked_identity != str(identity).strip(): + reasons.append( + f"lock claimant '{locked_identity}' does not match active identity " + f"'{str(identity).strip()}'" + ) + if locked_profile and str(profile or "").strip() and locked_profile != str(profile).strip(): + reasons.append( + f"lock profile '{locked_profile}' does not match active profile " + f"'{str(profile).strip()}'" + ) + + # The refresh is written by the LIVE owning author session. A refresh is not + # a recovery: the current process must be the recorded owner. + recorded_pid = lock.get("session_pid") + if recorded_pid is None: + recorded_pid = lock.get("pid") + evidence["recorded_pid"] = recorded_pid + evidence["current_pid"] = current_pid + if current_pid is None: + reasons.append("current session pid is unknown; cannot prove live ownership") + else: + try: + if recorded_pid is None or int(recorded_pid) != int(current_pid): + reasons.append( + f"durable lock is owned by pid {recorded_pid}, not the current " + f"session pid {current_pid}; head refresh requires the live owner" + ) + except (TypeError, ValueError): + reasons.append( + "durable lock owner pid is malformed; cannot prove live ownership" + ) + + if not old: + reasons.append("expected_old_head is not a full 40-char hex SHA (fail closed)") + if not new: + reasons.append("new_head is not a full 40-char hex SHA (fail closed)") + if old and new and old == new: + reasons.append( + "new head equals the expected old head; a sync must advance the head" + ) + + # Compare-and-swap on the recorded head: if the lock already records a synced + # head it must be exactly the expected old head, else another sync moved it. + recorded_synced = _norm_sha(lock.get("synced_pr_head")) + evidence["recorded_synced_pr_head"] = recorded_synced + if recorded_synced is not None and old is not None and recorded_synced != old: + reasons.append( + f"durable lock already records synced head {recorded_synced}, not the " + f"expected old head {old}; a concurrent sync changed it (CAS fail closed)" + ) + + if reasons: + return {"allowed": False, "reasons": reasons, "evidence": evidence, + "expected_generation": lock_generation(lock)} + + return { + "allowed": True, + "reasons": [ + f"durable lock for issue #{issue_number} branch '{locked_branch}' is " + f"owned by the live session; refresh recorded head {old} -> {new}" + ], + "evidence": evidence, + "expected_generation": lock_generation(lock), + } + + +def apply_durable_lock_head_refresh( + *, + remote: str, + org: str, + repo: str, + issue_number: int, + branch_name: str, + worktree_path: str, + pr_number: int | None, + identity: str | None, + profile: str | None, + current_pid: int | None, + expected_old_head: str | None, + new_head: str | None, + synced_at: str, + base_head: str | None = None, + provenance: str = LOCK_HEAD_REFRESH_PROVENANCE_MERGE_SYNC, + lock_dir: str | None = None, +) -> dict[str, Any]: + """CAS-refresh the durable lock's recorded head after a branch sync (#871). + + Reads the durable lock from disk, re-asserts ownership via + ``assess_durable_lock_head_refresh``, and — only when permitted — writes the + new synced head through ``bind_session_lock`` with a generation compare-and- + swap. Then re-reads the lock and proves it records the complete new head + (read-after-write). Any failure at any step returns ``refreshed=False`` with + reasons; the caller must treat that as a partial lifecycle failure and never + report a fully successful synchronization. + """ + existing = load_issue_lock( + remote=remote, org=org, repo=repo, issue_number=issue_number, lock_dir=lock_dir + ) + assessment = assess_durable_lock_head_refresh( + existing, + remote=remote, + org=org, + repo=repo, + issue_number=issue_number, + branch_name=branch_name, + worktree_path=worktree_path, + pr_number=pr_number, + identity=identity, + profile=profile, + current_pid=current_pid, + expected_old_head=expected_old_head, + new_head=new_head, + base_head=base_head, + ) + result: dict[str, Any] = { + "refreshed": False, + "read_after_write_ok": False, + "prior_head": _norm_sha(expected_old_head), + "new_head": _norm_sha(new_head), + "reasons": list(assessment.get("reasons") or []), + "evidence": assessment.get("evidence"), + } + if not assessment.get("allowed"): + return result + + new = _norm_sha(new_head) + old = _norm_sha(expected_old_head) + record = dict(existing or {}) + sync_block = { + "last_synced_pr_head": new, + "prior_pr_head": old, + "base_head": _norm_sha(base_head), + "pr_number": pr_number, + "provenance": provenance, + "synced_at": synced_at, + "synced_by_pid": current_pid, + "synced_by": { + "username": str(identity or "").strip() or None, + "profile": str(profile or "").strip() or None, + }, + } + record["synced_pr_head"] = new + record["branch_sync"] = sync_block + history = record.get("branch_sync_history") + if not isinstance(history, list): + history = [] + history = list(history) + history.append(sync_block) + record["branch_sync_history"] = history + + try: + bind_session_lock( + record, + lock_dir=lock_dir, + expected_generation=assessment.get("expected_generation"), + renewal_sanctioned=True, + ) + except Exception as exc: # CAS miss or write failure — partial lifecycle failure + result["reasons"].append( + f"durable lock head refresh write failed (fail closed): {exc}" + ) + return result + + after = load_issue_lock( + remote=remote, org=org, repo=repo, issue_number=issue_number, lock_dir=lock_dir + ) + after_head = _norm_sha((after or {}).get("synced_pr_head")) + result["lock_generation_after"] = lock_generation(after) + if after_head == new and new is not None: + result["refreshed"] = True + result["read_after_write_ok"] = True + result["reasons"].append( + f"durable lock recorded head refreshed to {new} and verified by " + "read-after-write" + ) + else: + result["reasons"].append( + "read-after-write verification failed: durable lock does not record " + f"the new head {new} (found {after_head}); partial lifecycle failure" + ) + return result \ No newline at end of file diff --git a/issue_lock_worktree.py b/issue_lock_worktree.py index de52ff7..8188536 100644 --- a/issue_lock_worktree.py +++ b/issue_lock_worktree.py @@ -145,6 +145,175 @@ def read_head_ancestry( return result +def read_merge_sync_provenance( + worktree_path: str, + *, + prior_head_sha: str | None, + synced_head_sha: str | None, +) -> dict: + """Observe whether ``synced_head_sha`` is a sanctioned merge-based branch sync + that advanced the PR branch past ``prior_head_sha`` (#871). + + ``gitea_update_pr_branch_by_merge`` advances a PR branch by merging the base + branch *into* the branch (``POST /pulls/{n}/update?style=merge``). The result + is a merge commit ``M`` on the branch whose **first** parent is the prior + branch head and whose second parent is the base tip. When the owning session + then dies without the durable lock's recorded head being refreshed, the local + worktree still sits at ``prior_head_sha`` while the live PR head is ``M``. + + Recovering that drift safely requires proving the remote head is *exactly* + such a merge-sync — not a rewrite, rebase, force-push, or an unrelated + commit. This is that server-side observation. It reports facts only; the + disposition lives in ``issue_lock_recovery``. Every field is read from git in + the declared worktree — nothing is supplied by, or reachable from, an MCP + caller (#871). + + Provenance is proven only when ALL hold: + + * both commits are present (a rewritten/force-moved prior head leaves the + object graph and fails closed); + * ``prior_head_sha`` is a strict ancestor of ``synced_head_sha`` (the branch + history is preserved, never replaced); + * ``synced_head_sha`` is a merge commit (two or more parents), i.e. a base + merged in — a plain fast-forward of new direct commits is not a sync; + * ``prior_head_sha`` is an ancestor of the merge's **first** parent, so the + branch mainline (first-parent lineage) still reaches the prior head — a + rebase/force-push that re-authored the branch side fails this. + """ + path = (worktree_path or "").strip() + prior = (prior_head_sha or "").strip() + synced = (synced_head_sha or "").strip() + result: dict = { + "prior_head_sha": prior or None, + "synced_head_sha": synced or None, + "probe_ok": False, + "prior_present": False, + "synced_present": False, + "prior_is_ancestor": False, + "synced_is_merge": False, + "first_parent_reaches_prior": False, + "is_merge_sync": False, + "first_parent_sha": None, + "parent_count": None, + "proof": None, + "reasons": [], + } + if not path or not prior or not synced: + result["reasons"].append( + "merge-sync provenance probe requires a worktree path and both " + "commit SHAs" + ) + return result + if prior == synced: + result["reasons"].append( + "prior and synced heads are identical; no branch sync occurred" + ) + return result + + def _present(sha: str) -> bool: + res = subprocess.run( + ["git", "-C", path, "rev-parse", "--verify", "--quiet", f"{sha}^{{commit}}"], + capture_output=True, + text=True, + check=False, + ) + return res.returncode == 0 + + def _is_ancestor(ancestor: str, descendant: str) -> bool | None: + res = subprocess.run( + ["git", "-C", path, "merge-base", "--is-ancestor", ancestor, descendant], + capture_output=True, + text=True, + check=False, + ) + if res.returncode == 0: + return True + if res.returncode == 1: + return False + return None # failed probe — never a silent "no" + + try: + result["prior_present"] = _present(prior) + result["synced_present"] = _present(synced) + except OSError as exc: # git unavailable — fail closed, never assume + result["reasons"].append(f"merge-sync provenance probe could not run: {exc}") + return result + + if not result["prior_present"]: + result["reasons"].append( + f"prior head {prior} is not reachable in '{path}'; history may have " + "been rewritten or force-moved" + ) + if not result["synced_present"]: + result["reasons"].append( + f"synced head {synced} is not reachable in '{path}'" + ) + if not (result["prior_present"] and result["synced_present"]): + return result + + ancestor = _is_ancestor(prior, synced) + if ancestor is None: + result["reasons"].append( + "ancestry probe failed; merge-sync provenance unproven" + ) + return result + result["prior_is_ancestor"] = bool(ancestor) + if not ancestor: + result["reasons"].append( + f"prior head {prior} is not an ancestor of synced head {synced}; " + "the branch history was not preserved (not a merge-based sync)" + ) + return result + + parents_res = subprocess.run( + ["git", "-C", path, "rev-list", "--parents", "-n", "1", synced], + capture_output=True, + text=True, + check=False, + ) + if parents_res.returncode != 0: + result["reasons"].append( + f"could not read parents of {synced}; merge-sync provenance unproven" + ) + return result + tokens = (parents_res.stdout or "").split() + # tokens[0] is the commit itself; the rest are its parents. + parents = tokens[1:] + result["parent_count"] = len(parents) + result["synced_is_merge"] = len(parents) >= 2 + if not result["synced_is_merge"]: + result["probe_ok"] = True + result["reasons"].append( + f"synced head {synced} has {len(parents)} parent(s); a merge-based " + "branch sync produces a merge commit (two or more parents)" + ) + return result + first_parent = parents[0] + result["first_parent_sha"] = first_parent + + fp_reaches = _is_ancestor(prior, first_parent) if prior != first_parent else True + if fp_reaches is None: + result["reasons"].append( + "first-parent ancestry probe failed; merge-sync provenance unproven" + ) + return result + result["first_parent_reaches_prior"] = bool(fp_reaches) + result["probe_ok"] = True + if not fp_reaches: + result["reasons"].append( + f"merge first parent {first_parent} does not reach prior head " + f"{prior}; the branch mainline was re-authored (not a sanctioned sync)" + ) + return result + + result["is_merge_sync"] = True + result["proof"] = ( + f"synced head {synced} is a merge commit (parents={len(parents)}) whose " + f"first-parent lineage reaches prior head {prior}; base merged into branch" + ) + return result + + def read_recorded_base( worktree_path: str, *, diff --git a/mcp_restart_paths.py b/mcp_restart_paths.py new file mode 100644 index 0000000..087fe72 --- /dev/null +++ b/mcp_restart_paths.py @@ -0,0 +1,475 @@ +"""Inventory and fail-closed guards for MCP restart/reload/kill paths (#657). + +Single source of truth enumerating every code/script/doc path that can +restart, reload, reconnect, kill, or force-recreate an MCP process. Each path +is classified and linked to the guard that constrains it. The companion +human-readable inventory lives in ``docs/mcp-restart-path-inventory.md`` and is +kept in lock-step with this module by ``tests/test_mcp_restart_paths.py``. + +Design intent (aligns with #655 restart-coordinator roadmap): + +* **No unguarded full restart.** The in-process MCP daemon + (``gitea_mcp_server.py`` / ``mcp_server.py`` / ``role_session_router.py``) + must never replace or kill its own process — replacing the process after the + host wired up the stdio pipes desyncs the JSON-RPC transport (observed with + Antigravity/Cascade hosts). ``assert_no_daemon_self_replacement`` enforces + this against the live source tree. +* **No legacy auto-restart helper.** ``_trigger_mcp_auto_restart`` was removed + when the stale-runtime resolver became side-effect free (#685); + ``assert_auto_restart_helper_absent`` keeps it removed. +* **Unknown restart attempts fail closed.** LLM tools must route any restart + intent through a *registered* path. ``assert_restart_attempt_registered`` + raises ``UnknownRestartPathError`` for anything not in this inventory. +* **pkill stays forbidden (#630).** Manual daemon kills are classified as + contamination by :mod:`runtime_recovery_guard`; this module records that path + and the test asserts the classification still holds. + +This module performs no restarts, spawns no threads, and touches no config or +process state. It is pure inventory + read-only source assertions. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from pathlib import Path +from typing import Iterable + +# --- Classifications ------------------------------------------------------- + +#: A narrow, one-shot recovery that is safe by construction (e.g. a CLI wrapper +#: re-execing into the venv interpreter before importing anything, or an +#: in-process profile switch). Never targets the running MCP daemon process. +CLASS_SANCTIONED_NARROW = "sanctioned_narrow_recovery" + +#: The path detects a condition that would require a restart, then *fails +#: closed* on mutations and emits restart/reconnect guidance. It never restarts +#: the process itself (recovery is owned by the host/operator). +CLASS_GUARDED_FAIL_CLOSED = "guarded_fail_closed" + +#: The path is forbidden. Attempting it is a workflow-safety violation and, +#: where an LLM tool could invoke it, is marked as contamination. +CLASS_FORBIDDEN = "forbidden" + +#: A previously-existing unguarded restart primitive that has been deleted. A +#: regression guard keeps it absent. +CLASS_REMOVED = "removed" + +#: Behavior that lives in the host/IDE and is outside this process's control +#: (e.g. a manual ``/mcp reconnect``). Documented, not code-guarded here. +CLASS_HOST_RESIDUAL = "host_residual" + +VALID_CLASSIFICATIONS = frozenset( + { + CLASS_SANCTIONED_NARROW, + CLASS_GUARDED_FAIL_CLOSED, + CLASS_FORBIDDEN, + CLASS_REMOVED, + CLASS_HOST_RESIDUAL, + } +) + +#: The in-process MCP daemon modules. These must never self-replace/self-kill. +DAEMON_MODULES = ( + "gitea_mcp_server.py", + "mcp_server.py", + "role_session_router.py", +) + +#: The legacy auto-restart helper removed in #685. Must stay removed. +LEGACY_AUTO_RESTART_HELPER = "_trigger_mcp_auto_restart" + +#: Call patterns that would let the daemon replace or terminate its own +#: process. Matched as calls (trailing ``(``) so prose/docstring mentions such +#: as "we do NOT os.execv() here" or "never calls ``os._exit``" do not trip the +#: scanner (comment lines are stripped first regardless). +DAEMON_SELF_REPLACEMENT_PRIMITIVES = ( + "os.execv(", + "os.execve(", + "os.execvp(", + "os.execvpe(", + "os.kill(", + "os.killpg(", + "os._exit(", + "os.abort(", +) + + +@dataclass(frozen=True) +class RestartPath: + """One classified restart/reload/kill path in the inventory.""" + + path_id: str + title: str + mechanism: str + classification: str + guard: str + locations: tuple[str, ...] + references: tuple[str, ...] + residual_host: bool = False + notes: str = "" + + +class UnknownRestartPathError(RuntimeError): + """Raised when a restart attempt is not a registered, classified path.""" + + +# --- The inventory --------------------------------------------------------- + +_RESTART_PATHS: tuple[RestartPath, ...] = ( + RestartPath( + path_id="cli_venv_bootstrap_execv", + title="CLI wrapper venv re-exec", + mechanism=( + "Standalone CLI scripts re-exec into venv/bin/python3 via os.execv " + "at import top, guarded by `sys.executable != venv_python`." + ), + classification=CLASS_SANCTIONED_NARROW, + guard=( + "One-shot, pre-import bootstrap; runs before any MCP transport " + "exists and only when not already on the venv interpreter, so it " + "cannot desync a live daemon. Idempotent guard condition prevents " + "a re-exec loop." + ), + locations=( + "create_pr.py", + "create_issue.py", + "close_issue.py", + "merge_pr.py", + "review_pr.py", + "edit_pr.py", + "delete_branch.py", + "mark_issue.py", + "manage_labels.py", + "list_issues.py", + "list_prs.py", + ), + references=("#657",), + ), + RestartPath( + path_id="daemon_self_replacement", + title="MCP daemon self-replacement", + mechanism=( + "The in-process MCP daemon replacing/terminating its own process " + "(os.execv/os.kill/os._exit) to reload code." + ), + classification=CLASS_FORBIDDEN, + guard=( + "Forbidden by design: replacing the process after the host wired " + "up stdio desyncs JSON-RPC (Antigravity/Cascade). Enforced against " + "the source tree by assert_no_daemon_self_replacement()." + ), + locations=("gitea_mcp_server.py:~155 (decision comment)",) + DAEMON_MODULES, + references=("#657", "#584"), + ), + RestartPath( + path_id="legacy_auto_restart_helper", + title="Legacy _trigger_mcp_auto_restart helper", + mechanism=( + "A helper that actively restarted the MCP server from the " + "read-only resolver path." + ), + classification=CLASS_REMOVED, + guard=( + "Removed in #685 when the resolver became side-effect free. Kept " + "absent by assert_auto_restart_helper_absent()." + ), + locations=("gitea_mcp_server.py", "mcp_server.py"), + references=("#685", "#657"), + ), + RestartPath( + path_id="config_touch_reload", + title="MCP client config-touch reload", + mechanism=( + "Touching (utime) the MCP client config file to make the host " + "reload/recreate the server process." + ), + classification=CLASS_REMOVED, + guard=( + "Removed from the resolver in #685: stale-runtime detection is " + "report-only and never mutates client config, spawns threads, or " + "calls os._exit." + ), + locations=("gitea_mcp_server.py (resolve_task_capability)",), + references=("#685", "#657"), + ), + RestartPath( + path_id="master_advance_auto_restart", + title="Master-advance staleness gate", + mechanism=( + "On-disk master advancing past the running code. The master-parity " + "gate detects it and fails mutations closed with restart guidance." + ), + classification=CLASS_GUARDED_FAIL_CLOSED, + guard=( + "Detect + fail closed only; the process never self-restarts. " + "master_parity_gate captures startup parity and blocks mutations " + "while stale, emitting restart/reconnect guidance." + ), + locations=( + "master_parity_gate.py", + "gitea_mcp_server.py (gitea_assess_master_parity)", + ), + references=("#420", "#591", "#657"), + ), + RestartPath( + path_id="stale_runtime_resolver_reconnect", + title="Stale-runtime resolver reconnect guidance", + mechanism=( + "The capability resolver detecting a stale serving process and " + "reporting restart_required/stop_required for a client reconnect." + ), + classification=CLASS_GUARDED_FAIL_CLOSED, + guard=( + "Report-only (#685): returns restart_required/stop_required and an " + "exact_safe_next_action pointing at IDE/client reconnect; performs " + "no restart, thread spawn, config touch, or os._exit." + ), + locations=("gitea_mcp_server.py (gitea_resolve_task_capability)",), + references=("#685", "#657"), + ), + RestartPath( + path_id="manual_daemon_kill", + title="Manual daemon kill (pkill/killall/kill)", + mechanism=( + "Shell kills of the MCP daemon: `pkill -f mcp_server.py`, " + "`killall`, broad `pkill -f python` sweeps, or `kill ` of a " + "daemon pid." + ), + classification=CLASS_FORBIDDEN, + guard=( + "Forbidden (#630): runtime_recovery_guard classifies these as " + "contamination and gitea_record_daemon_process_kill_attempt writes " + "a durable marker that fails subsequent mutations closed. Operator " + "maintenance authorization is read only from the environment, not " + "from a tool argument." + ), + locations=( + "runtime_recovery_guard.py", + "gitea_mcp_server.py (gitea_record_daemon_process_kill_attempt)", + ), + references=("#630", "#657"), + ), + RestartPath( + path_id="conflict_marker_infra_stop", + title="Startup conflict-marker infra stop", + mechanism=( + "The daemon entrypoint scans for unresolved merge-conflict markers " + "at startup and stops (sys.exit(1)) if found." + ), + classification=CLASS_GUARDED_FAIL_CLOSED, + guard=( + "Fail-closed startup stop, not a restart: the process exits and " + "waits for the operator to resolve conflicts and relaunch. Never " + "self-restarts or loops." + ), + locations=("mcp_server.py (check_conflict_markers)",), + references=("#657",), + ), + RestartPath( + path_id="ide_client_reconnect", + title="Host/IDE MCP reconnect", + mechanism=( + "A manual `/mcp reconnect` (or equivalent host action) that the " + "IDE performs to recreate the MCP client connection." + ), + classification=CLASS_HOST_RESIDUAL, + guard=( + "Outside this process's control. It is the sanctioned recovery the " + "gates point operators toward; documented as residual host " + "behavior. No in-process code initiates it." + ), + locations=("host/IDE",), + references=("#584", "#656", "#657"), + residual_host=True, + ), + RestartPath( + path_id="profile_switch_runtime", + title="Runtime profile switch", + mechanism=( + "Switching the active execution profile at runtime " + "(dynamic-profile mode)." + ), + classification=CLASS_SANCTIONED_NARROW, + guard=( + "In-process and restart-free: runtime_switching_supported is true, " + "so a profile switch rebinds capability without recreating the " + "process. No restart primitive is invoked." + ), + locations=("gitea_mcp_server.py (gitea_activate_profile)",), + references=("#656", "#657"), + ), +) + +_BY_ID: dict[str, RestartPath] = {p.path_id: p for p in _RESTART_PATHS} + + +# --- Read-only accessors --------------------------------------------------- + + +def iter_restart_paths() -> tuple[RestartPath, ...]: + """Return the full inventory as an immutable tuple.""" + + return _RESTART_PATHS + + +def restart_path_ids() -> frozenset[str]: + """Return the set of registered path ids.""" + + return frozenset(_BY_ID) + + +def get_restart_path(path_id: str) -> RestartPath: + """Return the registered path, or raise :class:`UnknownRestartPathError`.""" + + try: + return _BY_ID[path_id] + except KeyError as exc: + raise UnknownRestartPathError( + f"unknown restart path id {path_id!r}; not in the #657 inventory" + ) from exc + + +def paths_by_classification(classification: str) -> tuple[RestartPath, ...]: + """Return all registered paths with the given classification.""" + + if classification not in VALID_CLASSIFICATIONS: + raise ValueError(f"unknown classification {classification!r}") + return tuple(p for p in _RESTART_PATHS if p.classification == classification) + + +def assert_restart_attempt_registered(path_id: str) -> RestartPath: + """Fail closed unless ``path_id`` is a registered, classified restart path. + + LLM tools that intend to trigger any restart/reload/reconnect must name a + registered path so an unknown/novel restart primitive cannot slip through + silently. Forbidden and removed paths are registered too — this only + asserts the attempt is *known*, not that it is *permitted*; callers must + still honor the classification. + """ + + return get_restart_path(path_id) + + +def assert_registry_wellformed() -> None: + """Validate the inventory's own invariants (fail closed on drift).""" + + seen: set[str] = set() + for path in _RESTART_PATHS: + if path.path_id in seen: + raise ValueError(f"duplicate restart path id {path.path_id!r}") + seen.add(path.path_id) + if path.classification not in VALID_CLASSIFICATIONS: + raise ValueError( + f"{path.path_id!r} has invalid classification " + f"{path.classification!r}" + ) + if not path.guard.strip(): + raise ValueError(f"{path.path_id!r} is missing a guard description") + if not path.references: + raise ValueError(f"{path.path_id!r} is missing references") + if not path.locations: + raise ValueError(f"{path.path_id!r} is missing locations") + if path.classification == CLASS_HOST_RESIDUAL and not path.residual_host: + raise ValueError( + f"{path.path_id!r} is host_residual but residual_host is False" + ) + + +# --- Source-tree guards ---------------------------------------------------- + + +def _repo_root(root: str | os.PathLike[str] | None = None) -> Path: + if root is not None: + return Path(root) + return Path(__file__).resolve().parent + + +def _iter_code_lines(text: str) -> Iterable[tuple[int, str]]: + """Yield (1-based lineno, line) for lines that are not full-line comments.""" + + for lineno, line in enumerate(text.splitlines(), start=1): + if line.lstrip().startswith("#"): + continue + yield lineno, line + + +def scan_daemon_self_replacement( + root: str | os.PathLike[str] | None = None, +) -> list[dict[str, object]]: + """Return violations where a daemon module could self-replace/self-kill. + + Scans :data:`DAEMON_MODULES` for calls in + :data:`DAEMON_SELF_REPLACEMENT_PRIMITIVES`. Full-line comments are ignored, + and only call forms (with a trailing ``(``) match, so decision comments and + docstrings that merely mention the primitives do not produce false hits. + """ + + repo = _repo_root(root) + violations: list[dict[str, object]] = [] + for module in DAEMON_MODULES: + path = repo / module + if not path.exists(): + continue + text = path.read_text(encoding="utf-8", errors="replace") + for lineno, line in _iter_code_lines(text): + for primitive in DAEMON_SELF_REPLACEMENT_PRIMITIVES: + if primitive in line: + violations.append( + { + "module": module, + "line": lineno, + "primitive": primitive, + "text": line.strip(), + } + ) + return violations + + +def assert_no_daemon_self_replacement( + root: str | os.PathLike[str] | None = None, +) -> None: + """Fail closed if any daemon module can restart/kill its own process.""" + + violations = scan_daemon_self_replacement(root) + if violations: + rendered = "; ".join( + f"{v['module']}:{v['line']} {v['primitive']}" for v in violations + ) + raise AssertionError( + "MCP daemon must never self-replace/self-kill (#657); found: " + f"{rendered}" + ) + + +def scan_auto_restart_helper( + root: str | os.PathLike[str] | None = None, +) -> list[dict[str, object]]: + """Return occurrences of a *definition* of the legacy auto-restart helper.""" + + repo = _repo_root(root) + needle = f"def {LEGACY_AUTO_RESTART_HELPER}" + hits: list[dict[str, object]] = [] + for module in DAEMON_MODULES: + path = repo / module + if not path.exists(): + continue + text = path.read_text(encoding="utf-8", errors="replace") + for lineno, line in _iter_code_lines(text): + if needle in line: + hits.append({"module": module, "line": lineno}) + return hits + + +def assert_auto_restart_helper_absent( + root: str | os.PathLike[str] | None = None, +) -> None: + """Fail closed if the removed ``_trigger_mcp_auto_restart`` reappears.""" + + hits = scan_auto_restart_helper(root) + if hits: + rendered = "; ".join(f"{h['module']}:{h['line']}" for h in hits) + raise AssertionError( + f"{LEGACY_AUTO_RESTART_HELPER} was removed in #685 and must not " + f"return (#657); found definition at: {rendered}" + ) diff --git a/tests/test_branch_cleanup_guard.py b/tests/test_branch_cleanup_guard.py index 0bd1fda..fddb534 100644 --- a/tests/test_branch_cleanup_guard.py +++ b/tests/test_branch_cleanup_guard.py @@ -1639,6 +1639,358 @@ class TestSecondRemediationIntegration(unittest.TestCase): self.assertTrue(ownership_calls) +class TestIssue855ExactPrSelector(unittest.TestCase): + """#855: exact pr_number pin for reconcile_merged_cleanups (#851 lifecycle).""" + + def setUp(self): + self._remotes = patch.dict( + mcp_server.REMOTES, + { + "prgs": { + "host": "gitea.example.com", + "org": "Scaled-Tech-Consulting", + "repo": "Gitea-Tools", + } + }, + ) + self._remotes.start() + patch("gitea_audit.audit_enabled", return_value=False).start() + self.mock_api = patch("mcp_server.api_request").start() + self.mock_all = patch("mcp_server.api_get_all", return_value=[]).start() + patch("mcp_server.get_auth_header", return_value=FAKE_AUTH).start() + patch( + "mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", + return_value=True, + ).start() + patch( + "mcp_server.get_profile", + return_value=dict(RECONCILER_WITH_DELETE), + ).start() + patch( + "mcp_server._profile_operation_gate", + return_value=[], + ).start() + patch( + "mcp_server._collect_branch_ownership_records", + return_value={"records": [], "inventory_error": False}, + ).start() + patch( + "mcp_server.merged_cleanup_reconcile.discover_reviewer_scratch_worktrees", + return_value=[], + ).start() + patch("mcp_server.verify_preflight_purity", return_value=None).start() + patch( + "mcp_server.audit_reconciliation_mode.check_cleanup_execution_allowed", + return_value=(True, []), + ).start() + + def tearDown(self): + patch.stopall() + + def _merged_pr(self, number, branch, sha="c" * 40): + return { + "number": number, + "title": f"PR {number}", + "body": f"Closes #{number - 4}", + "merged": True, + "merged_at": "2026-07-23T12:00:00Z", + "merge_commit_sha": "f" * 40, + "state": "closed", + "head": {"ref": branch, "sha": sha}, + "base": {"ref": "master"}, + } + + def test_exact_pr_848_ignores_newer_852_in_batch_queue(self): + """pr_number=848 selects only #848 even when #852 is newer/first.""" + from mcp_server import gitea_reconcile_merged_cleanups + + pr_848 = self._merged_pr( + 848, "fix/issue-844-exclude-epic-containers", sha="c3f282ba" + "0" * 32 + ) + # Closed list would rank #852 first in batch mode; exact pin must ignore it. + closed_batch = [ + self._merged_pr(852, "fix/issue-851-cleanup-worktree-before-remote-delete"), + pr_848, + self._merged_pr(849, "fix/issue-849-other"), + self._merged_pr(846, "fix/issue-846-other"), + self._merged_pr(845, "fix/issue-845-other"), + ] + batch_fetch_calls = [] + + def fake_api(method, url, *args, **kwargs): + if method == "GET" and url.rstrip("/").endswith("/pulls/848"): + return dict(pr_848) + if method == "GET" and "/pulls/" in url: + raise AssertionError(f"unexpected PR fetch: {url}") + if method == "GET" and "/branches/" in url: + return {"name": "present"} + return {} + + def fake_all(url, auth, limit=None): + batch_fetch_calls.append((url, limit)) + if "state=open" in url: + return [] + if "state=closed" in url: + # Exact mode must not use the closed batch list. + raise AssertionError( + "exact pr_number mode must not page closed PRs: " + url + ) + return [] + + self.mock_api.side_effect = fake_api + self.mock_all.side_effect = fake_all + patch( + "mcp_server._remote_branch_exists", + return_value=True, + ).start() + patch( + "mcp_server.merged_cleanup_reconcile.build_reconciliation_report", + side_effect=lambda **kwargs: { + "entries": [ + { + "pr_number": int(pr["number"]), + "head_branch": (pr.get("head") or {}).get("ref"), + "issue_number": 844, + "remote_branch": { + "safe_to_delete_remote": True, + "head_branch": (pr.get("head") or {}).get("ref"), + }, + "local_worktree": { + "safe_to_remove_worktree": True, + "worktree_path": ( + "/tmp/branches/fix-issue-844-exclude-epic-containers" + ), + }, + "planned_execution_order": ( + mcp_server.merged_cleanup_reconcile.plan_cleanup_execution_order( + remote_assessment={"safe_to_delete_remote": True}, + local_assessment={"safe_to_remove_worktree": True}, + ) + ), + } + for pr in kwargs.get("closed_prs") or [] + if pr.get("merged_at") or pr.get("merged") + ], + "reviewer_scratch_entries": [], + "merged_pr_count": len(kwargs.get("closed_prs") or []), + }, + ).start() + + res = gitea_reconcile_merged_cleanups( + dry_run=True, + pr_number=848, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(res.get("success")) + self.assertFalse(res.get("performed")) + self.assertEqual(res.get("selection_mode"), "exact_pr") + self.assertEqual(res.get("selected_pr_number"), 848) + entries = res.get("entries") or [] + self.assertEqual(len(entries), 1, entries) + self.assertEqual(entries[0].get("pr_number"), 848) + self.assertEqual( + entries[0].get("head_branch"), + "fix/issue-844-exclude-epic-containers", + ) + # No other PR appears in plan. + self.assertEqual(list((res.get("planned_execution_orders") or {}).keys()), ["848"]) + plan = (res.get("planned_execution_orders") or {}).get("848") or [] + actions = [s.get("action") for s in plan] + self.assertEqual( + actions, + [ + "remove_local_worktree", + "reassess_branch_ownership", + "delete_remote_branch", + ], + ) + # Prove we never scanned the multi-PR closed batch. + self.assertFalse(any("state=closed" in (u or "") for u, _ in batch_fetch_calls)) + # closed_batch fixture must remain unused (sanity). + self.assertEqual(closed_batch[0]["number"], 852) + + def test_exact_pr_execute_only_mutates_selected_pr(self): + """Execute with pr_number must never touch #845/#846/#849/#852.""" + from mcp_server import gitea_reconcile_merged_cleanups + + pr_848 = self._merged_pr(848, "fix/issue-844-exclude-epic-containers") + worktree_path = "/tmp/branches/fix-issue-844-exclude-epic-containers" + remove_calls = [] + delete_api_calls = [] + ownership_branches = [] + + def fake_api(method, url, *args, **kwargs): + if method == "GET" and url.rstrip("/").endswith("/pulls/848"): + return dict(pr_848) + if method == "DELETE": + delete_api_calls.append(url) + # Forbid foreign PR branch deletion by URL content. + for forbidden in ("845", "846", "849", "852"): + self.assertNotIn(forbidden, url) + return {} + + def fake_remove(project_root, branch, worktree_path=None): + remove_calls.append({"branch": branch, "worktree_path": worktree_path}) + return { + "success": True, + "performed": True, + "message": f"removed {worktree_path}", + "worktree_path": worktree_path, + } + + def fake_collect(**kwargs): + ownership_branches.append(kwargs.get("branch")) + return {"records": [], "inventory_error": False} + + def fake_probe(h, o, r, auth, br): + return guard.classify_branch_readback_http_status( + 404, not_found_scope=guard.NOT_FOUND_SCOPE_BRANCH + ) + + self.mock_api.side_effect = fake_api + self.mock_all.side_effect = lambda url, auth, limit=None: [] + patch("mcp_server._remote_branch_exists", return_value=True).start() + patch( + "mcp_server.merged_cleanup_reconcile.build_reconciliation_report", + return_value={ + "entries": [ + { + "pr_number": 848, + "head_branch": "fix/issue-844-exclude-epic-containers", + "remote_branch": {"safe_to_delete_remote": True}, + "local_worktree": { + "safe_to_remove_worktree": True, + "worktree_path": worktree_path, + }, + "planned_execution_order": [ + {"action": "remove_local_worktree", "phase": 1}, + {"action": "reassess_branch_ownership", "phase": 2}, + {"action": "delete_remote_branch", "phase": 3}, + ], + } + ], + "reviewer_scratch_entries": [ + # Foreign scratch must be filtered before report execute loop; + # if present here it would still be a test failure if acted on. + ], + "merged_pr_count": 1, + }, + ).start() + patch( + "mcp_server.merged_cleanup_reconcile.remove_local_worktree", + side_effect=fake_remove, + ).start() + patch( + "mcp_server._collect_branch_ownership_records", + side_effect=fake_collect, + ).start() + patch("mcp_server._probe_remote_branch", side_effect=fake_probe).start() + + res = gitea_reconcile_merged_cleanups( + dry_run=False, + execute_confirmed=True, + pr_number=848, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(res.get("performed") or res.get("executed")) + self.assertEqual(res.get("selection_mode"), "exact_pr") + self.assertEqual(res.get("selected_pr_number"), 848) + actions = res.get("actions") or [] + pr_numbers_touched = { + a.get("pr_number") for a in actions if a.get("pr_number") is not None + } + self.assertTrue(pr_numbers_touched.issubset({None, 848}) or not pr_numbers_touched) + removes = [a for a in actions if a.get("action") == "remove_local_worktree"] + deletes = [a for a in actions if a.get("action") == "delete_remote_branch"] + self.assertEqual(len(removes), 1) + self.assertEqual(remove_calls[0]["branch"], "fix/issue-844-exclude-epic-containers") + self.assertEqual(len(deletes), 1) + self.assertTrue(deletes[0].get("success")) + self.assertTrue(deletes[0].get("after_worktree_removal")) + self.assertEqual(len(delete_api_calls), 1) + self.assertEqual( + ownership_branches, ["fix/issue-844-exclude-epic-containers"] + ) + + def test_exact_pr_unknown_fails_closed_without_mutation(self): + from mcp_server import gitea_reconcile_merged_cleanups + + def fake_api(method, url, *args, **kwargs): + if method == "GET" and "/pulls/99999" in url: + raise RuntimeError("HTTP 404 Not Found") + raise AssertionError(f"unexpected API call {method} {url}") + + self.mock_api.side_effect = fake_api + res = gitea_reconcile_merged_cleanups( + dry_run=True, + pr_number=99999, + remote="prgs", + ) + self.assertFalse(res.get("success")) + self.assertFalse(res.get("performed")) + self.assertEqual(res.get("blocker_kind"), "pr_unresolvable") + self.assertIn("99999", " ".join(res.get("reasons") or [])) + + def test_exact_pr_not_merged_fails_closed(self): + from mcp_server import gitea_reconcile_merged_cleanups + + def fake_api(method, url, *args, **kwargs): + if method == "GET" and url.rstrip("/").endswith("/pulls/900"): + return { + "number": 900, + "merged": False, + "merged_at": None, + "state": "open", + "head": {"ref": "feat/x", "sha": "a" * 40}, + } + raise AssertionError(f"unexpected {method} {url}") + + self.mock_api.side_effect = fake_api + res = gitea_reconcile_merged_cleanups( + dry_run=False, + execute_confirmed=True, + pr_number=900, + remote="prgs", + ) + self.assertFalse(res.get("success")) + self.assertFalse(res.get("performed")) + self.assertEqual(res.get("blocker_kind"), "pr_not_merged") + + def test_exact_pr_invalid_number_fails_closed(self): + from mcp_server import gitea_reconcile_merged_cleanups + + res = gitea_reconcile_merged_cleanups( + dry_run=True, + pr_number=0, + remote="prgs", + ) + self.assertFalse(res.get("success")) + self.assertEqual(res.get("blocker_kind"), "invalid_pr_number") + self.mock_api.assert_not_called() + + def test_batch_mode_still_works_without_pr_number(self): + """Unfiltered batch path remains backward compatible.""" + from mcp_server import gitea_reconcile_merged_cleanups + + self.mock_all.side_effect = lambda url, auth, limit=None: [] + self.mock_api.side_effect = lambda *a, **k: {} + patch( + "mcp_server.merged_cleanup_reconcile.build_reconciliation_report", + return_value={ + "entries": [], + "reviewer_scratch_entries": [], + "merged_pr_count": 0, + }, + ).start() + res = gitea_reconcile_merged_cleanups(dry_run=True, remote="prgs", limit=10) + self.assertTrue(res.get("success")) + self.assertEqual(res.get("selection_mode"), "batch") + self.assertIsNone(res.get("selected_pr_number")) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_issue_855_expired_reviewer_reclaim.py b/tests/test_issue_855_expired_reviewer_reclaim.py new file mode 100644 index 0000000..288b7f7 --- /dev/null +++ b/tests/test_issue_855_expired_reviewer_reclaim.py @@ -0,0 +1,244 @@ +"""#855 AC4: an expired reviewer lease must not indefinitely protect an +already-merged branch when no live claimant exists. + +Two layers are covered: + +* ``branch_cleanup_guard.assess_expired_reviewer_lease_reclaim`` — the pure, + fail-closed reclaim decision. Every condition must be provably satisfied or + the lease keeps protecting the branch. +* ``gitea_mcp_server._collect_branch_ownership_records`` — the wiring that + supplies authoritative evidence (PR merged state, owner-process liveness, + competing ownership) to that decision, and flips an expired reviewer lease + to reclaimable only under the full policy. + +All inputs are fabricated; no real repository, lease, or credential is used. +""" + +import importlib +import unittest +from unittest.mock import patch + +import branch_cleanup_guard + +mcp_server = importlib.import_module("gitea_mcp_server") + +FAKE_AUTH = "token fake" +REMOTE = "prgs" +ORG = "Scaled-Tech-Consulting" +REPO = "Gitea-Tools" +HOST = "gitea.prgs.cc" +BRANCH = "feat/issue-638-webui-app-shell-phase1" +PR_NUMBER = 818 + + +class TestAssessExpiredReviewerLeaseReclaim(unittest.TestCase): + """Pure fail-closed reclaim decision (#855 AC4).""" + + def _call(self, **overrides): + base = dict( + role="reviewer", + status="expired", + pr_merged=True, + owner_pid_alive=False, + competing_active_claimant=False, + ) + base.update(overrides) + return branch_cleanup_guard.assess_expired_reviewer_lease_reclaim(**base) + + def test_full_policy_satisfied_allows_reclaim(self): + out = self._call() + self.assertTrue(out["reclaim_allowed"]) + self.assertEqual(out["reasons"], []) + self.assertEqual(out["decision"], "reclaim_expired_reviewer_lease") + + def test_stale_dead_process_reviewer_also_reclaimable(self): + out = self._call(status="stale_dead_process") + self.assertTrue(out["reclaim_allowed"]) + + def test_non_reviewer_role_never_reclaims(self): + for role in ("author", "merger", "controller", "reconciler", "unknown"): + with self.subTest(role=role): + out = self._call(role=role) + self.assertFalse(out["reclaim_allowed"]) + self.assertTrue(out["reasons"]) + self.assertEqual(out["decision"], "keep_protecting") + + def test_active_status_never_reclaims(self): + out = self._call(status="active") + self.assertFalse(out["reclaim_allowed"]) + + def test_pr_not_merged_blocks_reclaim(self): + out = self._call(pr_merged=False) + self.assertFalse(out["reclaim_allowed"]) + + def test_pr_merged_unknown_fails_closed(self): + out = self._call(pr_merged=None) + self.assertFalse(out["reclaim_allowed"]) + + def test_owner_process_alive_blocks_reclaim(self): + out = self._call(owner_pid_alive=True) + self.assertFalse(out["reclaim_allowed"]) + + def test_owner_liveness_unknown_fails_closed(self): + out = self._call(owner_pid_alive=None) + self.assertFalse(out["reclaim_allowed"]) + + def test_competing_active_claimant_blocks_reclaim(self): + out = self._call(competing_active_claimant=True) + self.assertFalse(out["reclaim_allowed"]) + + def test_competing_claimant_unknown_fails_closed(self): + out = self._call(competing_active_claimant=None) + self.assertFalse(out["reclaim_allowed"]) + + def test_reasons_never_leak_secrets(self): + out = self._call(role="author") + blob = " ".join(out["reasons"]).lower() + self.assertNotIn("token", blob) + self.assertNotIn("password", blob) + + +class _FakeLease(dict): + pass + + +class TestCollectorExpiredReviewerReclaimWiring(unittest.TestCase): + """`_collect_branch_ownership_records` supplies authoritative evidence and + flips an expired reviewer lease to reclaimable only under the full policy.""" + + def _run( + self, + *, + lease_role="reviewer", + lease_freshness="stale_dead_process", + owner_pid_alive=False, + pr_merged=True, + extra_leases=None, + worktree_on_branch=False, + ): + lease = _FakeLease( + role=lease_role, + work_kind="pr", + work_number=PR_NUMBER, + branch=BRANCH, + status="active", + owner_pid=999999, + remote=REMOTE, + org=ORG, + repo=REPO, + host=HOST, + freshness={ + "freshness": lease_freshness, + "owner_pid": 999999, + "owner_pid_alive": owner_pid_alive, + "expired_by_time": lease_freshness == "expired", + }, + ) + leases = [lease] + list(extra_leases or []) + + pr_payload = { + "number": PR_NUMBER, + "merged": pr_merged, + "merged_at": "2026-07-23T00:00:00Z" if pr_merged else None, + "head": {"ref": BRANCH}, + } + + def fake_api_request(method, url, *a, **k): + if method == "GET" and f"/pulls/{PR_NUMBER}" in url: + return pr_payload + raise AssertionError(f"unexpected api_request {method} {url}") + + wt_entries = [] + if worktree_on_branch: + wt_entries = [{"branch": BRANCH, "path": f"/x/branches/{BRANCH}"}] + + with patch.object( + mcp_server.lease_lifecycle, + "list_active_leases", + return_value={"leases": leases}, + ), patch.object( + mcp_server.control_plane_db, "get_db", return_value=object(), create=True + ), patch.object( + mcp_server.issue_lock_store, "iter_lock_files", return_value=[] + ), patch.object( + mcp_server.worktree_cleanup_audit, + "list_worktrees", + return_value=wt_entries, + ), patch.object( + mcp_server, "api_get_all", return_value=[] + ), patch.object( + mcp_server, "api_request", side_effect=fake_api_request + ): + return mcp_server._collect_branch_ownership_records( + remote=REMOTE, + host=HOST, + org=ORG, + repo=REPO, + branch=BRANCH, + pr_number=PR_NUMBER, + project_root="/x", + auth=FAKE_AUTH, + base_api="https://gitea.prgs.cc/api/v1/repos/x/y", + ) + + def _reviewer_records(self, bundle): + return [ + rec + for rec in bundle["records"] + if rec.get("category") + == branch_cleanup_guard.OWNERSHIP_CATEGORY_REVIEWER_LEASE + ] + + def test_merged_dead_uncontested_reviewer_lease_is_reclaimable(self): + bundle = self._run() + self.assertFalse(bundle["inventory_error"]) + recs = self._reviewer_records(bundle) + self.assertEqual(len(recs), 1) + self.assertTrue(recs[0]["reclaim_allowed"]) + # And the guard consequently does not block deletion on it. + ownership = branch_cleanup_guard.assess_active_branch_ownership( + remote=REMOTE, org=ORG, repo=REPO, branch=BRANCH, host=HOST, + records=bundle["records"], + ) + self.assertFalse(ownership["block"]) + + def test_unmerged_pr_keeps_reviewer_lease_protective(self): + bundle = self._run(pr_merged=False) + recs = self._reviewer_records(bundle) + self.assertEqual(len(recs), 1) + self.assertFalse(recs[0]["reclaim_allowed"]) + ownership = branch_cleanup_guard.assess_active_branch_ownership( + remote=REMOTE, org=ORG, repo=REPO, branch=BRANCH, host=HOST, + records=bundle["records"], + ) + self.assertTrue(ownership["block"]) + + def test_owner_process_alive_keeps_reviewer_lease_protective(self): + bundle = self._run(owner_pid_alive=True, lease_freshness="expired") + recs = self._reviewer_records(bundle) + self.assertFalse(recs[0]["reclaim_allowed"]) + + def test_competing_worktree_binding_keeps_reviewer_lease_protective(self): + bundle = self._run(worktree_on_branch=True) + recs = self._reviewer_records(bundle) + self.assertFalse(recs[0]["reclaim_allowed"]) + ownership = branch_cleanup_guard.assess_active_branch_ownership( + remote=REMOTE, org=ORG, repo=REPO, branch=BRANCH, host=HOST, + records=bundle["records"], + ) + self.assertTrue(ownership["block"]) + + def test_expired_author_lease_never_reclaimed_by_reviewer_policy(self): + bundle = self._run(lease_role="author") + author_recs = [ + rec + for rec in bundle["records"] + if rec.get("category") + == branch_cleanup_guard.OWNERSHIP_CATEGORY_AUTHOR_LEASE + ] + self.assertEqual(len(author_recs), 1) + self.assertFalse(author_recs[0]["reclaim_allowed"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_issue_871_durable_lock_head_refresh.py b/tests/test_issue_871_durable_lock_head_refresh.py new file mode 100644 index 0000000..37851b1 --- /dev/null +++ b/tests/test_issue_871_durable_lock_head_refresh.py @@ -0,0 +1,630 @@ +"""Durable linked-issue lock head refresh + merge-sync dead-session recovery (#871). + +``gitea_update_pr_branch_by_merge`` advances a PR's *remote* head but historically +never advanced the linked durable issue lock's recorded head. After the owning +session died the drifted lock became unrecoverable and no further synchronization +was possible (PR #866 / issue #855). + +Two halves are covered: + +* the write-side refresh (``issue_lock_store.assess/apply_durable_lock_head_refresh``) + that records the new synced head under compare-and-swap with read-after-write; and +* the read-side recovery relation (``issue_lock_recovery`` + + ``issue_lock_worktree.read_merge_sync_provenance``) that lets a dead-session lock + whose recorded head is a merge-sync *ancestor* of the live PR head be recovered — + and nothing else. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from datetime import datetime, timedelta, timezone +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import issue_lock_recovery # noqa: E402 +import issue_lock_store # noqa: E402 +import issue_lock_worktree # noqa: E402 + +ISSUE = 8710 +PR_NUMBER = 8711 +BRANCH = f"fix/issue-{ISSUE}-durable-lock-head-refresh" +IDENTITY = "example-user" +PROFILE = "example-author" +OLD = "a" * 40 +NEW1 = "b" * 40 +NEW2 = "c" * 40 +BASE = "d" * 40 +REMOTE = "prgs" +ORG = "ExampleOrg" +REPO = "ExampleRepo" + + +def dead_pid() -> int: + proc = subprocess.Popen([sys.executable, "-c", "pass"]) + proc.wait() + return proc.pid + + +def future_ts(hours: int = 4) -> str: + return ( + (datetime.now(timezone.utc) + timedelta(hours=hours)) + .isoformat() + .replace("+00:00", "Z") + ) + + +def _git(cwd, *args): + return subprocess.run( + ["git", "-C", cwd, *args], + capture_output=True, + text=True, + check=True, + ) + + +def _rev(cwd, ref="HEAD") -> str: + return _git(cwd, "rev-parse", ref).stdout.strip() + + +def build_merge_sync_repo(tmp: str) -> dict: + """Build a repo where a feature branch was synced by merging master in. + + Returns a dict with the prior (branch) head, the synced merge-commit head, + the master tip, plus a rebase-style linear descendant and an unrelated head. + """ + _git(tmp, "init", "-q", "-b", "master") + _git(tmp, "config", "user.email", "t@example.com") + _git(tmp, "config", "user.name", "T") + Path(tmp, "base.txt").write_text("base\n") + _git(tmp, "add", "-A") + _git(tmp, "commit", "-q", "-m", "root") + + # Feature branch cut from root, one commit — this is the PRIOR/recorded head. + _git(tmp, "checkout", "-q", "-b", BRANCH) + Path(tmp, "feature.txt").write_text("feature\n") + _git(tmp, "add", "-A") + _git(tmp, "commit", "-q", "-m", "feature work") + prior = _rev(tmp) + + # Master advances (the base the sync will merge in). + _git(tmp, "checkout", "-q", "master") + Path(tmp, "base.txt").write_text("base\nmore\n") + _git(tmp, "add", "-A") + _git(tmp, "commit", "-q", "-m", "master advance") + master_tip = _rev(tmp) + + # Sync: merge master INTO the feature branch → merge commit, first parent = prior. + _git(tmp, "checkout", "-q", BRANCH) + _git(tmp, "merge", "-q", "--no-ff", "-m", "Merge master into feature", "master") + synced = _rev(tmp) + + # A plain linear descendant of prior (NOT a merge) — a rebase/extra-commit shape. + _git(tmp, "checkout", "-q", "-b", "linear-branch", prior) + Path(tmp, "extra.txt").write_text("extra\n") + _git(tmp, "add", "-A") + _git(tmp, "commit", "-q", "-m", "extra linear commit") + linear = _rev(tmp) + + # An unrelated root (force-push / rewritten history shape). + unrelated_dir = tempfile.mkdtemp() + _git(unrelated_dir, "init", "-q", "-b", "x") + _git(unrelated_dir, "config", "user.email", "t@example.com") + _git(unrelated_dir, "config", "user.name", "T") + Path(unrelated_dir, "z.txt").write_text("z\n") + _git(unrelated_dir, "add", "-A") + _git(unrelated_dir, "commit", "-q", "-m", "unrelated") + unrelated = _rev(unrelated_dir) + + # Leave the worktree checked out on the feature branch at the PRIOR head, as + # a dead author session that never advanced would have left it. + _git(tmp, "checkout", "-q", BRANCH) + _git(tmp, "reset", "-q", "--hard", prior) + + return { + "prior": prior, + "master_tip": master_tip, + "synced": synced, + "linear": linear, + "unrelated": unrelated, + } + + +# ─────────────────────────── write-side refresh ─────────────────────────── + + +class TestDurableLockHeadRefresh(unittest.TestCase): + def setUp(self): + self.lock_dir = tempfile.mkdtemp() + self.wt = tempfile.mkdtemp() + lock_data = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "worktree_path": self.wt, + "remote": REMOTE, + "org": ORG, + "repo": REPO, + "claimant": {"username": IDENTITY, "profile": PROFILE}, + "work_lease": { + "operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE, + "issue_number": ISSUE, + "branch": BRANCH, + "worktree_path": self.wt, + "claimant": {"username": IDENTITY, "profile": PROFILE}, + "expires_at": future_ts(), + }, + } + issue_lock_store.bind_session_lock(lock_data, lock_dir=self.lock_dir) + + def _apply(self, **over): + kw = dict( + remote=REMOTE, org=ORG, repo=REPO, issue_number=ISSUE, + branch_name=BRANCH, worktree_path=self.wt, pr_number=PR_NUMBER, + identity=IDENTITY, profile=PROFILE, current_pid=os.getpid(), + expected_old_head=OLD, new_head=NEW1, synced_at=future_ts(0), + base_head=BASE, lock_dir=self.lock_dir, + ) + kw.update(over) + return issue_lock_store.apply_durable_lock_head_refresh(**kw) + + def _load(self): + return issue_lock_store.load_issue_lock( + remote=REMOTE, org=ORG, repo=REPO, issue_number=ISSUE, + lock_dir=self.lock_dir, + ) + + def test_first_sync_updates_recorded_head(self): + """AC1: first base sync writes the resulting head to the durable lock.""" + res = self._apply() + self.assertTrue(res["refreshed"], res["reasons"]) + self.assertTrue(res["read_after_write_ok"]) + self.assertEqual(self._load().get("synced_pr_head"), NEW1) + + def test_second_sync_after_master_advance(self): + """AC2: a later master advance permits a second sanctioned sync.""" + self.assertTrue(self._apply()["refreshed"]) + res2 = self._apply(expected_old_head=NEW1, new_head=NEW2) + self.assertTrue(res2["refreshed"], res2["reasons"]) + self.assertEqual(self._load().get("synced_pr_head"), NEW2) + history = self._load().get("branch_sync_history") + self.assertEqual(len(history), 2) + self.assertEqual(history[0]["last_synced_pr_head"], NEW1) + self.assertEqual(history[1]["prior_pr_head"], NEW1) + + def test_cas_detects_concurrent_head_change(self): + """AC6: CAS refuses when the recorded synced head is not the old head.""" + self.assertTrue(self._apply()["refreshed"]) # recorded head now NEW1 + # A second sync claiming the old head is still OLD must fail closed. + res = self._apply(expected_old_head=OLD, new_head=NEW2) + self.assertFalse(res["refreshed"]) + self.assertTrue(any("CAS" in r or "concurrent" in r for r in res["reasons"])) + self.assertEqual(self._load().get("synced_pr_head"), NEW1) + + def test_wrong_issue_fails_closed(self): + res = self._apply(issue_number=999999) + self.assertFalse(res["refreshed"]) + + def test_wrong_branch_fails_closed(self): + res = self._apply(branch_name="fix/issue-8710-wrong") + self.assertFalse(res["refreshed"]) + + def test_wrong_repo_fails_closed(self): + res = self._apply(repo="OtherRepo") + self.assertFalse(res["refreshed"]) + + def test_wrong_identity_fails_closed(self): + res = self._apply(identity="intruder") + self.assertFalse(res["refreshed"]) + + def test_wrong_profile_fails_closed(self): + res = self._apply(profile="prgs-reviewer") + self.assertFalse(res["refreshed"]) + + def test_foreign_session_fails_closed(self): + """A refresh is not a recovery: the current process must own the lock.""" + path = issue_lock_store.lock_file_path( + remote=REMOTE, org=ORG, repo=REPO, issue_number=ISSUE, + lock_dir=self.lock_dir, + ) + rec = issue_lock_store.read_lock_file(path) + rec["session_pid"] = dead_pid() + rec["pid"] = rec["session_pid"] + issue_lock_store.save_lock_file(path, rec) + res = self._apply() + self.assertFalse(res["refreshed"]) + self.assertTrue(any("current session" in r or "live owner" in r for r in res["reasons"])) + + def test_new_equals_old_fails_closed(self): + res = self._apply(expected_old_head=OLD, new_head=OLD) + self.assertFalse(res["refreshed"]) + + def test_non_full_sha_fails_closed(self): + self.assertFalse(self._apply(new_head="deadbeef")["refreshed"]) + self.assertFalse(self._apply(expected_old_head="xyz")["refreshed"]) + + def test_no_lock_fails_closed(self): + assessment = issue_lock_store.assess_durable_lock_head_refresh( + None, remote=REMOTE, org=ORG, repo=REPO, issue_number=ISSUE, + branch_name=BRANCH, worktree_path=self.wt, pr_number=PR_NUMBER, + identity=IDENTITY, profile=PROFILE, current_pid=os.getpid(), + expected_old_head=OLD, new_head=NEW1, + ) + self.assertFalse(assessment["allowed"]) + + +# ─────────────────────── merge-sync provenance (real git) ─────────────────── + + +class TestMergeSyncProvenanceObservation(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.shas = build_merge_sync_repo(self.tmp) + + def test_merge_sync_is_recognized(self): + obs = issue_lock_worktree.read_merge_sync_provenance( + self.tmp, prior_head_sha=self.shas["prior"], + synced_head_sha=self.shas["synced"], + ) + self.assertTrue(obs["is_merge_sync"], obs["reasons"]) + self.assertTrue(obs["prior_is_ancestor"]) + self.assertTrue(obs["synced_is_merge"]) + self.assertTrue(obs["first_parent_reaches_prior"]) + + def test_linear_descendant_is_not_a_merge_sync(self): + """A plain non-merge descendant (rebase/extra commit) is not a sync.""" + obs = issue_lock_worktree.read_merge_sync_provenance( + self.tmp, prior_head_sha=self.shas["prior"], + synced_head_sha=self.shas["linear"], + ) + self.assertTrue(obs["probe_ok"]) + self.assertFalse(obs["is_merge_sync"]) + self.assertFalse(obs["synced_is_merge"]) + + def test_unrelated_history_fails_closed(self): + """A rewritten/force-pushed head where prior is unreachable fails closed.""" + obs = issue_lock_worktree.read_merge_sync_provenance( + self.tmp, prior_head_sha=self.shas["prior"], + synced_head_sha=self.shas["unrelated"], + ) + self.assertFalse(obs["is_merge_sync"]) + + def test_missing_args_fail_closed(self): + obs = issue_lock_worktree.read_merge_sync_provenance( + self.tmp, prior_head_sha=None, synced_head_sha=self.shas["synced"], + ) + self.assertFalse(obs["is_merge_sync"]) + + +# ──────────────────── merge-sync dead-session recovery ────────────────────── + + +def make_dead_lock(worktree, **over): + pid = dead_pid() + lock = { + "issue_number": ISSUE, + "branch_name": BRANCH, + "worktree_path": worktree, + "remote": REMOTE, + "org": ORG, + "repo": REPO, + "session_pid": pid, + "pid": pid, + "claimant": {"username": IDENTITY, "profile": PROFILE}, + "work_lease": { + "operation_type": issue_lock_store.AUTHOR_ISSUE_WORK_LEASE, + "issue_number": ISSUE, + "branch": BRANCH, + "worktree_path": worktree, + "claimant": {"username": IDENTITY, "profile": PROFILE}, + "expires_at": future_ts(), + }, + } + lock.update(over) + return lock + + +def sync_prov(prior, synced, **over): + d = { + "prior_head_sha": prior, + "synced_head_sha": synced, + "probe_ok": True, + "prior_present": True, + "synced_present": True, + "prior_is_ancestor": True, + "synced_is_merge": True, + "first_parent_reaches_prior": True, + "is_merge_sync": True, + "first_parent_sha": prior, + "parent_count": 2, + "proof": f"{synced} merged base into branch above {prior}", + "reasons": [], + } + d.update(over) + return d + + +class TestMergeSyncRecovery(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.shas = build_merge_sync_repo(self.tmp) + self.prior = self.shas["prior"] + self.synced = self.shas["synced"] + + def _assess(self, **over): + lock = over.pop("_lock", None) or make_dead_lock(self.tmp) + kw = dict( + issue_number=ISSUE, branch_name=BRANCH, worktree_path=self.tmp, + remote=REMOTE, org=ORG, repo=REPO, identity=IDENTITY, profile=PROFILE, + current_branch=BRANCH, porcelain_status="", + head_sha=self.prior, remote_head_sha=self.synced, + pr_head_sha=self.synced, pr_number=PR_NUMBER, + competing_live_locks=[], candidate_branches=[BRANCH], + current_pid=os.getpid(), + remote_branch_exists=True, + sync_provenance=sync_prov(self.prior, self.synced), + ) + kw.update(over) + return issue_lock_recovery.assess_dead_session_lock_recovery(lock, **kw) + + def test_merge_sync_drift_is_recoverable(self): + """AC3/AC4: dead session, recorded head is a merge-sync ancestor of PR head.""" + res = self._assess() + self.assertEqual(res["outcome"], issue_lock_recovery.RECOVERY_SANCTIONED, res["reasons"]) + self.assertEqual( + res["evidence"]["head_relation"], + issue_lock_recovery.HEAD_RELATION_REMOTE_MERGE_SYNCED, + ) + self.assertEqual(res["evidence"]["accepted_head"], self.synced) + + def test_missing_provenance_fails_closed(self): + """No server-derived provenance → cannot accept a remote ahead of local.""" + res = self._assess(sync_provenance=None) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_non_ancestor_recorded_head_fails_closed(self): + """AC7: provenance that does not prove ancestry is rejected.""" + res = self._assess( + sync_provenance=sync_prov( + self.prior, self.synced, prior_is_ancestor=False, is_merge_sync=False, + reasons=["prior head is not an ancestor"], + ) + ) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_force_pushed_history_fails_closed(self): + """AC8: a rewritten head (not a merge sync) stays protected.""" + res = self._assess( + sync_provenance=sync_prov( + self.prior, self.synced, is_merge_sync=False, synced_is_merge=False, + reasons=["not a merge-based sync"], + ) + ) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_provenance_for_other_commits_fails_closed(self): + """Provenance whose endpoints differ from the heads under assessment is rejected.""" + res = self._assess( + sync_provenance=sync_prov("f" * 40, self.synced), + ) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_dirty_worktree_fails_closed(self): + """AC11: dirty worktrees remain protected.""" + res = self._assess(porcelain_status=" M feature.txt\n") + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_live_owner_fails_closed(self): + """AC10: a live recorded owner is not a dead-session recovery.""" + lock = make_dead_lock(self.tmp, session_pid=os.getpid(), pid=os.getpid()) + res = self._assess(_lock=lock) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_competing_claimant_fails_closed(self): + """AC13: a competing live lock blocks recovery.""" + res = self._assess( + competing_live_locks=[{ + "issue_number": ISSUE, "branch_name": BRANCH, + "worktree_path": "/some/other/wt", "pid": os.getpid(), + }] + ) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_wrong_branch_fails_closed(self): + """AC9: worktree on a different branch fails closed.""" + res = self._assess(current_branch="fix/issue-8710-other") + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_wrong_identity_fails_closed(self): + res = self._assess(identity="intruder") + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_pr_head_mismatch_fails_closed(self): + """The open PR must sit at the synced remote head.""" + res = self._assess(pr_head_sha="e" * 40) + self.assertEqual(res["outcome"], issue_lock_recovery.REFUSED) + + def test_owning_pr_evidence_for_merge_sync(self): + res = self._assess() + ev = issue_lock_recovery.owning_pr_recovery_evidence(res) + self.assertIsNotNone(ev) + self.assertEqual(ev["pr_number"], PR_NUMBER) + self.assertEqual(ev["head_sha"], self.synced) + self.assertEqual( + ev["head_relation"], + issue_lock_recovery.HEAD_RELATION_REMOTE_MERGE_SYNCED, + ) + + def test_recovered_owning_pr_from_persisted_record(self): + res = self._assess() + record = issue_lock_recovery.build_recovery_record(res, recovered_at=future_ts(0)) + lock = {"issue_number": ISSUE, "branch_name": BRANCH, + "dead_session_recovery": record} + rebuilt = issue_lock_recovery.recovered_owning_pr_from_lock(lock) + self.assertIsNotNone(rebuilt) + self.assertEqual(rebuilt["head_sha"], self.synced) + self.assertEqual( + rebuilt["head_relation"], + issue_lock_recovery.HEAD_RELATION_REMOTE_MERGE_SYNCED, + ) + + +class TestExistingRelationsUnchanged(unittest.TestCase): + """AC14/AC15: equal-head recovery still works; merge-sync did not weaken it.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp() + self.shas = build_merge_sync_repo(self.tmp) + + def test_equal_head_recovery_still_sanctioned(self): + # Worktree at prior head; remote also at prior head → the #753 equal case. + prior = self.shas["prior"] + lock = make_dead_lock(self.tmp) + res = issue_lock_recovery.assess_dead_session_lock_recovery( + lock, issue_number=ISSUE, branch_name=BRANCH, worktree_path=self.tmp, + remote=REMOTE, org=ORG, repo=REPO, identity=IDENTITY, profile=PROFILE, + current_branch=BRANCH, porcelain_status="", + head_sha=prior, remote_head_sha=prior, + pr_head_sha=prior, pr_number=PR_NUMBER, + competing_live_locks=[], candidate_branches=[BRANCH], + current_pid=os.getpid(), remote_branch_exists=True, + ) + self.assertEqual(res["outcome"], issue_lock_recovery.RECOVERY_SANCTIONED, res["reasons"]) + self.assertEqual( + res["evidence"]["head_relation"], issue_lock_recovery.HEAD_RELATION_EQUAL, + ) + + +class TestUpdatePrWrapperPartialFailure(unittest.TestCase): + """AC5/AC16: the tool advances the remote head then refreshes the durable lock. + + When the durable refresh fails after the remote advance, the tool must report a + partial lifecycle failure and NOT a fully successful synchronization. Exact PR- + head / base-head pinning is preserved (delegated to the real preflight, stubbed + here only to isolate the post-update lifecycle branch). + """ + + def setUp(self): + import gitea_mcp_server as gms # noqa: E402 + self.gms = gms + self._orig = {} + + def _patch(name, value): + self._orig[name] = getattr(gms, name) + setattr(gms, name, value) + + _patch("get_profile", lambda *a, **k: { + "allowed_operations": ["gitea.branch.push"], + "forbidden_operations": [], + "profile_name": "prgs-author", + }) + _patch("_role_kind", lambda *a, **k: "author") + _patch("_profile_operation_gate", lambda *a, **k: None) + _patch("_permission_block_report", lambda *a, **k: {}) + _patch("_resolve", lambda *a, **k: ("gitea.prgs.cc", ORG, REPO)) + _patch("_verify_role_mutation_workspace", lambda *a, **k: None) + _patch("_get_workspace_porcelain", lambda *a, **k: "") + _patch("_canonical_local_git_root", lambda *a, **k: "/x") + _patch("_master_parity_block", lambda *a, **k: None) + _patch("_auth", lambda *a, **k: {"token": "x"}) + _patch("repo_api_url", lambda *a, **k: "http://api") + _patch("_redact", lambda s: s) + _patch("_work_lease_claimant", lambda *a, **k: { + "username": IDENTITY, "profile": PROFILE, + }) + _patch("_prove_author_ownership_for_pr", lambda *a, **k: { + "has_author_lock": True, "matched_issue": ISSUE, + "matched_via": "branch", "linked_issues": [ISSUE], + "recovered_owning_pr": None, "reasons": [], + }) + + # Real preflight is unit-tested elsewhere; stub it to isolate the + # post-update durable-lock lifecycle branch under test. + orig_pf = gms.pr_sync_status.assess_update_pr_branch_preflight + self._orig_pf = orig_pf + gms.pr_sync_status.assess_update_pr_branch_preflight = ( + lambda *a, **k: {"mutation_allowed": True, "reasons": [], "performed": False} + ) + + # Sequence the two GET /pulls calls: OLD before update, NEW after. + self._pull_calls = {"n": 0} + + def fake_api_request(method, url, auth, *a, **k): + m = method.upper() + if m == "GET" and url.endswith(f"/pulls/{PR_NUMBER}"): + self._pull_calls["n"] += 1 + head = OLD if self._pull_calls["n"] == 1 else NEW1 + return { + "state": "open", + "head": {"sha": head, "ref": BRANCH}, + "base": {"sha": BASE, "ref": "master"}, + "mergeable": True, "title": "t", "body": "b", + } + if m == "GET" and "/branches/" in url: + return {"commit": {"id": BASE}} + if m == "POST" and "/update" in url: + return {} + return {} + + _patch("api_request", fake_api_request) + + def tearDown(self): + for name, value in self._orig.items(): + setattr(self.gms, name, value) + self.gms.pr_sync_status.assess_update_pr_branch_preflight = self._orig_pf + + def _run(self): + return self.gms.gitea_update_pr_branch_by_merge( + pr_number=PR_NUMBER, + expected_pr_head_sha=OLD, + expected_base_head_sha=BASE, + remote=REMOTE, + worktree_path="/tmp/branches/wt-871", + ) + + def test_partial_failure_when_refresh_fails(self): + self._orig["apply_durable_lock_head_refresh"] = ( + self.gms.issue_lock_store.apply_durable_lock_head_refresh + ) + self.gms.issue_lock_store.apply_durable_lock_head_refresh = ( + lambda **k: {"refreshed": False, "reasons": ["forced refresh failure"]} + ) + try: + res = self._run() + finally: + self.gms.issue_lock_store.apply_durable_lock_head_refresh = ( + self._orig["apply_durable_lock_head_refresh"] + ) + self.assertTrue(res["performed"]) + self.assertEqual(res["new_pr_head_sha"], NEW1) + self.assertFalse(res["success"]) + self.assertTrue(res["partial_lifecycle_failure"]) + self.assertFalse(res["durable_lock_refreshed"]) + + def test_full_success_when_refresh_succeeds(self): + self._orig["apply_durable_lock_head_refresh"] = ( + self.gms.issue_lock_store.apply_durable_lock_head_refresh + ) + self.gms.issue_lock_store.apply_durable_lock_head_refresh = ( + lambda **k: {"refreshed": True, "read_after_write_ok": True, + "new_head": NEW1, "reasons": ["ok"]} + ) + try: + res = self._run() + finally: + self.gms.issue_lock_store.apply_durable_lock_head_refresh = ( + self._orig["apply_durable_lock_head_refresh"] + ) + self.assertTrue(res["success"]) + self.assertTrue(res["performed"]) + self.assertTrue(res["durable_lock_refreshed"]) + self.assertTrue(res["fully_synchronized"]) + self.assertEqual(res["new_pr_head_sha"], NEW1) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_restart_governance_docs.py b/tests/test_mcp_restart_governance_docs.py new file mode 100644 index 0000000..5b12e50 --- /dev/null +++ b/tests/test_mcp_restart_governance_docs.py @@ -0,0 +1,107 @@ +"""Documentation acceptance for the MCP restart governance ADR (#656). + +Enforces issue #656 acceptance criteria: + +* AC1 — policy document exists with an authorization matrix and the recorded + v1 decision (controller approval + automated safety gates). +* AC2 — restart is stated as a last resort with enumerated narrower recoveries. +* AC3 — a unilateral LLM full restart with affected sessions is forbidden. +* AC4 — break-glass conditions are listed. +* AC5 — the ADR is linked to #655, #652, #653, #630, #642, and is cross-linked + from the safety model and the web-console deployment boundary docs. +""" +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +ADR = REPO_ROOT / "docs" / "architecture" / "mcp-restart-governance.md" +ADR_BASENAME = "mcp-restart-governance.md" + +CROSS_LINK_DOCS = ( + REPO_ROOT / "docs" / "safety-model.md", + REPO_ROOT / "docs" / "webui-deployment.md", +) + +LINKED_ISSUES = ("#655", "#652", "#653", "#630", "#642") +POLICY_IDS = ("RG-01", "RG-02", "RG-03", "RG-04", "RG-05", "RG-06", "RG-07", "RG-08") + + +def _read(path: Path) -> str: + assert path.is_file(), f"missing {path.relative_to(REPO_ROOT)}" + return path.read_text(encoding="utf-8") + + +def test_ac1_adr_exists_with_matrix_and_v1_decision(): + text = _read(ADR) + lower = text.lower() + assert text.lstrip().startswith("#"), "ADR lacks a title" + assert "#656" in text + assert "authorization matrix" in lower + # The matrix is a real table with the worker and privileged roles. + for role in ("author", "reviewer", "merger", "reconciler", "controller", + "operator", "admin"): + assert role in lower, f"authorization matrix missing role {role!r}" + # Recorded v1 decision. + assert "restart-governance/v1" in text + assert "controller approval" in lower and "automated safety gates" in lower + + +def test_ac2_restart_is_last_resort_with_narrower_recoveries(): + text = _read(ADR) + lower = text.lower() + assert "last resort" in lower + # Enumerated narrower recoveries precede full restart on the ladder. + for rung in ("reconnect", "rebind", "scoped restart", "full restart", + "host"): + assert rung in lower, f"recovery ladder missing rung {rung!r}" + + +def test_ac3_forbids_unilateral_llm_full_restart_with_affected_sessions(): + text = _read(ADR) + lower = text.lower() + assert "forbidden" in lower + assert "llm" in lower and "restart" in lower + assert "unilateral" in lower + # A worker role must not perform or authorize full/host restart. + assert "must not" in lower + + +def test_ac4_break_glass_conditions_listed(): + text = _read(ADR) + lower = text.lower() + assert "break-glass" in lower + assert "incident" in lower + assert "audit" in lower + + +def test_ac5_adr_links_issue_lineage(): + text = _read(ADR) + for issue in LINKED_ISSUES: + assert issue in text, f"ADR must link issue {issue}" + + +def test_ac5_safety_model_and_deployment_cross_link_adr(): + for path in CROSS_LINK_DOCS: + text = _read(path) + assert ADR_BASENAME in text, ( + f"{path.relative_to(REPO_ROOT)} must cross-link {ADR_BASENAME} " + f"(issue #656 acceptance criterion 5)" + ) + + +def test_policy_ids_present_for_enforcement_code(): + text = _read(ADR) + for pid in POLICY_IDS: + assert pid in text, f"policy id {pid} missing from ADR" + + +def test_failure_behavior_denies_on_ambiguity(): + text = _read(ADR) + lower = text.lower() + assert "ambiguous" in lower and "deny" in lower + + +def test_cross_links_do_not_embed_secrets(): + for path in (ADR,) + CROSS_LINK_DOCS: + text = _read(path) + for marker in ("ghp_", "BEGIN PRIVATE KEY", "Authorization: Bearer"): + assert marker not in text, f"{path} contains {marker!r}" diff --git a/tests/test_mcp_restart_paths.py b/tests/test_mcp_restart_paths.py new file mode 100644 index 0000000..cf0c5eb --- /dev/null +++ b/tests/test_mcp_restart_paths.py @@ -0,0 +1,146 @@ +"""Tests for the MCP restart-path inventory and guards (#657). + +Covers: +* the registry is well-formed and every path is classified; +* unknown restart attempts fail closed (AC "fail closed on unknown restart"); +* the previously-unguarded full-restart primitives stay guarded/absent + against the real source tree (AC "tests for at least one previously + unguarded path"); +* pkill of the daemon is still classified as contamination (#630, AC3); +* the inventory doc and module stay in lock-step. +""" + +import os +import tempfile +import unittest +from pathlib import Path + +import mcp_restart_paths as rp +import runtime_recovery_guard + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +DOC_PATH = os.path.join(REPO_ROOT, "docs", "mcp-restart-path-inventory.md") + + +class TestRegistryWellformed(unittest.TestCase): + def test_registry_is_wellformed(self): + # Must not raise. + rp.assert_registry_wellformed() + + def test_every_path_has_valid_classification(self): + for path in rp.iter_restart_paths(): + self.assertIn(path.classification, rp.VALID_CLASSIFICATIONS) + self.assertTrue(path.guard.strip(), path.path_id) + self.assertTrue(path.references, path.path_id) + self.assertTrue(path.locations, path.path_id) + + def test_ids_are_unique(self): + ids = [p.path_id for p in rp.iter_restart_paths()] + self.assertEqual(len(ids), len(set(ids))) + + def test_covers_every_classification(self): + present = {p.classification for p in rp.iter_restart_paths()} + self.assertEqual(present, set(rp.VALID_CLASSIFICATIONS)) + + +class TestUnknownAttemptFailsClosed(unittest.TestCase): + def test_unknown_path_raises(self): + with self.assertRaises(rp.UnknownRestartPathError): + rp.assert_restart_attempt_registered("totally_novel_restart_hack") + + def test_get_unknown_raises(self): + with self.assertRaises(rp.UnknownRestartPathError): + rp.get_restart_path("nope") + + def test_registered_attempt_returns_path(self): + path = rp.assert_restart_attempt_registered("manual_daemon_kill") + self.assertEqual(path.classification, rp.CLASS_FORBIDDEN) + + +class TestDaemonNeverSelfReplaces(unittest.TestCase): + """Previously-unguarded full-restart primitive: daemon self-replacement.""" + + def test_no_self_replacement_in_source(self): + # The live daemon modules must contain no os.execv/os.kill/os._exit + # self-restart call. Must not raise. + rp.assert_no_daemon_self_replacement(REPO_ROOT) + + def test_scanner_flags_injected_violation(self): + # Guard the guard: prove the scanner catches a real self-replace call. + with tempfile.TemporaryDirectory() as tmp: + bad = Path(tmp) / "gitea_mcp_server.py" + bad.write_text( + "import os\n" + "def restart():\n" + " os.execv('/usr/bin/python', ['python'])\n", + encoding="utf-8", + ) + found = rp.scan_daemon_self_replacement(tmp) + self.assertTrue(found) + with self.assertRaises(AssertionError): + rp.assert_no_daemon_self_replacement(tmp) + + def test_scanner_ignores_comment_and_docstring_mentions(self): + with tempfile.TemporaryDirectory() as tmp: + ok = Path(tmp) / "gitea_mcp_server.py" + ok.write_text( + "import os\n" + "# NOT os.execv() to re-point the interpreter here.\n" + '"""Never calls os._exit to restart."""\n' + "value = 1\n", + encoding="utf-8", + ) + self.assertEqual(rp.scan_daemon_self_replacement(tmp), []) + + +class TestLegacyAutoRestartHelperRemoved(unittest.TestCase): + """Previously-unguarded full-restart path: _trigger_mcp_auto_restart.""" + + def test_helper_absent_in_source(self): + # Must not raise: helper was removed in #685. + rp.assert_auto_restart_helper_absent(REPO_ROOT) + + def test_scanner_flags_reintroduced_helper(self): + with tempfile.TemporaryDirectory() as tmp: + bad = Path(tmp) / "mcp_server.py" + bad.write_text( + "def _trigger_mcp_auto_restart():\n return True\n", + encoding="utf-8", + ) + with self.assertRaises(AssertionError): + rp.assert_auto_restart_helper_absent(tmp) + + +class TestPkillStaysForbidden(unittest.TestCase): + """AC3: pkill of the daemon remains forbidden/contaminating (#630).""" + + def test_manual_daemon_kill_registered_as_forbidden(self): + path = rp.get_restart_path("manual_daemon_kill") + self.assertEqual(path.classification, rp.CLASS_FORBIDDEN) + + def test_pkill_classified_as_contamination(self): + assessment = runtime_recovery_guard.assess_recovery_command( + "pkill -f mcp_server.py" + ) + self.assertTrue(assessment["contaminated"]) + + def test_read_only_probe_not_contamination(self): + assessment = runtime_recovery_guard.assess_recovery_command( + "ps aux | grep mcp_server" + ) + self.assertFalse(assessment["contaminated"]) + + +class TestInventoryDocInSync(unittest.TestCase): + def test_doc_exists(self): + self.assertTrue(os.path.exists(DOC_PATH), DOC_PATH) + + def test_doc_mentions_every_path_id(self): + with open(DOC_PATH, encoding="utf-8") as handle: + doc = handle.read() + for path in rp.iter_restart_paths(): + self.assertIn(path.path_id, doc, f"doc missing {path.path_id}") + + +if __name__ == "__main__": + unittest.main()