Compare commits
3
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 jcwalker3
|
c9aa09f341 | ||
|
jcwalker3
|
499b87c482 | ||
  sysadminandClaude Opus 4.8
|
24c52abf6b |
@@ -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,
|
||||
|
||||
@@ -1,223 +0,0 @@
|
||||
# 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.
|
||||
@@ -46,17 +46,3 @@ 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.
|
||||
|
||||
@@ -55,15 +55,6 @@ 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
|
||||
|
||||
+213
-11
@@ -11116,6 +11116,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}
|
||||
@@ -11264,15 +11267,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
|
||||
@@ -11343,6 +11359,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}
|
||||
|
||||
|
||||
@@ -11405,6 +11459,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,
|
||||
@@ -11415,7 +11470,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.
|
||||
@@ -11450,11 +11509,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] = {}
|
||||
@@ -11481,6 +11649,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:
|
||||
@@ -11515,6 +11690,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
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
@@ -1,107 +0,0 @@
|
||||
"""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}"
|
||||
Reference in New Issue
Block a user