fix(restart): require both authorizations for apply, correct coordinator doc
Addresses the two blockers raised in the PR #886 review (comment 16559) for
issue #663.
B1 — apply_authorized ignored restart-class authorization.
The #663 restart-class matrix and the #661 drain-proof hard gate are
independent authorizations that first coexisted when PR #882 landed on
master and this branch merged it. The union preserved both, but the apply
decision consulted only the drain gate:
payload["apply_authorized"] = gate.allow
so a clean drain proof — or an authorized break-glass, which needs no proof
at all — reported apply_authorized: True for a class the least-privilege
matrix had just denied, in the same payload carrying allow_restart: False
and "role 'author' may not request full_mcp_restart". One environment
variable therefore collapsed the whole nine-class matrix for the apply
decision, including host_restart.
The apply decision is now the conjunction of both authorizations, and
apply_gate carries drain_gate_allow and restart_class_authorized so a denial
is attributable to the authorization that produced it. Break-glass keeps its
purpose — bypassing the drain proof — and never bypasses the class matrix.
No existing fail-closed behaviour is weakened: allow_restart, drain-proof
verification, fingerprint binding, and requester authorization are untouched.
B2 — docs/mcp-restart-coordinator.md described pre-#661 behaviour.
The document still called the drain proof "a separate child" and omitted
drain_proof_json and request_break_glass from the published signature, so a
safety document asserted there was no gate where a gate now exists. It now
documents both parameters, states that the gate executes inside this tool,
and records dry-run versus apply behaviour, authorization ordering, the
break-glass scope, and fail-closed conditions as implemented.
Regression coverage.
tests/test_issue_886_apply_authorization_conjunction.py exercises the MCP
tool itself, which previously had no test at all — that absence is why the
defect shipped. It pins both conjunction directions, proves a clean proof
cannot override a role, approval, unknown-class, or missing-target denial,
proves break-glass does not collapse the matrix for any worker role or
restricted class, and proves the existing scoped and unscoped paths and the
#661 denials still hold. Against the pre-fix tree 24 of these fail; against
this commit all 19 pass with 45 subtests.
tests/test_mcp_restart_governance_docs.py now binds the published signature
to inspect.signature() of the real tool and forbids the stale pre-#661
phrasing, so the drift that produced B2 cannot return unnoticed.
Verification: targeted restart/drain/governance/webui suites 194 passed,
113 subtests. Full suite 23 failed, 5230 passed, 6 skipped, 912 subtests —
the failure set is identical to the reviewed baseline at 9bc021e
(23 failed, 5201 passed), with +29 passing from the added tests and no new
or changed failure. Zero conflict markers; py_compile passes; the #882
union remains intact in both directions.
Refs #663, PR #886
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01V6xFqovhbArPv61j9KCGkL
This commit is contained in:
@@ -6,11 +6,17 @@ console (#642 / #652) can see the blast radius *before* concurrent LLM work is
|
|||||||
disrupted. Uncoordinated restarts destroy in-flight author/reviewer/merger work
|
disrupted. Uncoordinated restarts destroy in-flight author/reviewer/merger work
|
||||||
and give operators no way to see what they are about to break.
|
and give operators no way to see what they are about to break.
|
||||||
|
|
||||||
This lands the coordinator + impact DTO + a dry-run MCP tool. It is the single
|
This lands the coordinator + impact DTO + the MCP tool. It is the single
|
||||||
sanctioned entry point for restart evaluation post-#657 (which inventoried the
|
sanctioned entry point for restart evaluation post-#657 (which inventoried the
|
||||||
restart/reload/kill paths). The **mutative apply** path — actually performing a
|
restart/reload/kill paths).
|
||||||
restart — is a later child gated by a drain proof and is explicitly out of
|
|
||||||
scope here.
|
The **drain-proof hard gate now executes inside this tool** (#661, via PR #882):
|
||||||
|
an apply request (`dry_run=False`) is evaluated against a drain proof here and
|
||||||
|
denied when that proof is missing, expired, unclean, tampered with, or stale.
|
||||||
|
It is no longer a separate child operation. What remains a later child is only
|
||||||
|
the **execution** step — actually stopping and restoring a process. This tool
|
||||||
|
still never restarts anything: `apply_supported` is always `false` and
|
||||||
|
`restart_performed` is always `false`.
|
||||||
|
|
||||||
The coordinator now routes every request through the restart-class policy
|
The coordinator now routes every request through the restart-class policy
|
||||||
matrix defined for #663. See
|
matrix defined for #663. See
|
||||||
@@ -25,7 +31,8 @@ recovery behavior for all nine classes.
|
|||||||
| `restart_coordinator.evaluate_restart_impact` | `restart_coordinator.py` | Pure classification: inventory → impact report DTO. No I/O, no restart. |
|
| `restart_coordinator.evaluate_restart_impact` | `restart_coordinator.py` | Pure classification: inventory → impact report DTO. No I/O, no restart. |
|
||||||
| `RestartImpactReport` / `SessionImpact` / `LeaseImpact` | `restart_coordinator.py` | Console-facing DTO (`.as_dict()` is JSON-serializable). |
|
| `RestartImpactReport` / `SessionImpact` / `LeaseImpact` | `restart_coordinator.py` | Console-facing DTO (`.as_dict()` is JSON-serializable). |
|
||||||
| `ControlPlaneDB.list_sessions` | `control_plane_db.py` | Read-only session inventory (the process-level unit a restart kills). |
|
| `ControlPlaneDB.list_sessions` | `control_plane_db.py` | Read-only session inventory (the process-level unit a restart kills). |
|
||||||
| `gitea_request_mcp_restart` | `gitea_mcp_server.py` | MCP tool: gathers inventory from the #613 DB, calls the coordinator, returns the report. Dry-run only. |
|
| `gitea_request_mcp_restart` | `gitea_mcp_server.py` | MCP tool: gathers inventory from the #613 DB, calls the coordinator, returns the report, and on `dry_run=False` runs the #661 drain-proof hard gate. Never restarts a process. |
|
||||||
|
| `drain_proof.gate_apply_restart` | `drain_proof.py` | The #661 hard gate: verifies a drain proof against the current impact fingerprint, or records an authorized break-glass bypass. |
|
||||||
|
|
||||||
## Dimensions evaluated
|
## Dimensions evaluated
|
||||||
|
|
||||||
@@ -86,12 +93,52 @@ gitea_request_mcp_restart(remote, host, org, repo,
|
|||||||
session_id=None, limit=200,
|
session_id=None, limit=200,
|
||||||
restart_class="full_mcp_restart",
|
restart_class="full_mcp_restart",
|
||||||
target_session_id=None, target_role=None,
|
target_session_id=None, target_role=None,
|
||||||
target_connector=None)
|
target_connector=None,
|
||||||
|
drain_proof_json=None,
|
||||||
|
request_break_glass=False)
|
||||||
```
|
```
|
||||||
|
|
||||||
Read-only, dry-run, and it **never restarts anything**. `apply_supported` is
|
It **never restarts anything**: `apply_supported` is always `false` and
|
||||||
always `false`; passing `dry_run=False` performs no restart and reports that
|
`restart_performed` is always `false`.
|
||||||
apply is gated by a drain proof (a separate child).
|
|
||||||
|
### Dry-run versus apply
|
||||||
|
|
||||||
|
| Call | Behavior |
|
||||||
|
|------|----------|
|
||||||
|
| `dry_run=True` (default) | Read-only impact preview. No drain proof is required or consulted. |
|
||||||
|
| `dry_run=False` | The #661 drain-proof hard gate runs **in this tool**. The outcome is reported under `apply_gate` / `apply_authorized`; a denial also returns a durable `incident` descriptor. Still no restart. |
|
||||||
|
|
||||||
|
### Authorization ordering
|
||||||
|
|
||||||
|
An apply requires **both** authorizations, and they are independent:
|
||||||
|
|
||||||
|
1. **Restart-class authorization** (#663) — the requester's role and permissions
|
||||||
|
must allow the requested class, the class's approval requirement must be
|
||||||
|
satisfied, and any target-scoped class must name its target. Failing any of
|
||||||
|
these makes `allow_restart` `false`.
|
||||||
|
2. **Drain-proof gate** (#661) — a valid, unexpired, clean proof bound to the
|
||||||
|
current impact fingerprint, or an authorized break-glass.
|
||||||
|
|
||||||
|
`apply_authorized` is the conjunction: `gate.allow and allow_restart`. A clean
|
||||||
|
drain proof therefore cannot override a class or requester-role denial, and a
|
||||||
|
denied class never reports an authorized apply. `apply_gate` carries
|
||||||
|
`drain_gate_allow` and `restart_class_authorized` so a denial is attributable to
|
||||||
|
the authorization that produced it.
|
||||||
|
|
||||||
|
### Break-glass
|
||||||
|
|
||||||
|
Break-glass bypasses the **drain proof only** — never the restart-class matrix.
|
||||||
|
It is honoured solely when `request_break_glass` is set *and* the environment
|
||||||
|
carries `GITEA_BREAKGLASS_RESTART_AUTHORIZATION`; like operator override, the
|
||||||
|
tool argument expresses caller intent and cannot be self-asserted by a worker
|
||||||
|
session. `break_glass_requested` and `break_glass_authorized` are both reported,
|
||||||
|
so a bypass is never silent.
|
||||||
|
|
||||||
|
### Fail closed on apply
|
||||||
|
|
||||||
|
A missing, malformed, expired, unclean, tampered, or fingerprint-stale drain
|
||||||
|
proof denies the apply and returns an `incident` descriptor. An unknown restart
|
||||||
|
class denies before any of this. Ambiguity always denies.
|
||||||
|
|
||||||
## Audit
|
## Audit
|
||||||
|
|
||||||
|
|||||||
+28
-2
@@ -22367,7 +22367,16 @@ def gitea_request_mcp_restart(
|
|||||||
only when ``request_break_glass`` is set *and* the environment carries
|
only when ``request_break_glass`` is set *and* the environment carries
|
||||||
``GITEA_BREAKGLASS_RESTART_AUTHORIZATION``. Even an authorized gate performs
|
``GITEA_BREAKGLASS_RESTART_AUTHORIZATION``. Even an authorized gate performs
|
||||||
no restart here; actual execution is a further child. The gate outcome is
|
no restart here; actual execution is a further child. The gate outcome is
|
||||||
reported under ``apply_gate`` / ``apply_authorized``.
|
reported under ``apply_gate``.
|
||||||
|
|
||||||
|
``apply_authorized`` requires **both** authorizations to pass: the #661 drain
|
||||||
|
gate *and* the #663 restart-class matrix (``allow_restart``). They are
|
||||||
|
independent — the drain gate proves the blast radius was drained and knows
|
||||||
|
nothing about whether this requester may request this class — so a class the
|
||||||
|
matrix denied never reports an authorized apply. Break-glass bypasses the
|
||||||
|
drain proof only; it never bypasses the class matrix. ``apply_gate`` carries
|
||||||
|
``drain_gate_allow`` and ``restart_class_authorized`` so a denial is
|
||||||
|
attributable to the authorization that produced it.
|
||||||
|
|
||||||
Operator override authority is read from the process environment
|
Operator override authority is read from the process environment
|
||||||
(``GITEA_OPERATOR_RESTART_OVERRIDE_AUTHORIZATION``), never self-asserted by
|
(``GITEA_OPERATOR_RESTART_OVERRIDE_AUTHORIZATION``), never self-asserted by
|
||||||
@@ -22546,8 +22555,25 @@ def gitea_request_mcp_restart(
|
|||||||
gate_payload["reasons"] = [proof_parse_error] + list(
|
gate_payload["reasons"] = [proof_parse_error] + list(
|
||||||
gate_payload.get("reasons") or []
|
gate_payload.get("reasons") or []
|
||||||
)
|
)
|
||||||
|
# The #663 restart-class matrix and the #661 drain gate are two
|
||||||
|
# independent authorizations, and an apply requires BOTH. ``gate.allow``
|
||||||
|
# proves only that the blast radius was drained — or that break-glass
|
||||||
|
# was authorized — and knows nothing about whether this requester may
|
||||||
|
# request this class at all. Conjoining them keeps a class the matrix
|
||||||
|
# denied from ever reporting an authorized apply, and keeps break-glass
|
||||||
|
# scoped to what it is for: bypassing the drain proof, never the
|
||||||
|
# least-privilege class matrix.
|
||||||
|
restart_class_authorized = bool(report.allow_restart)
|
||||||
|
gate_payload["drain_gate_allow"] = bool(gate.allow)
|
||||||
|
gate_payload["restart_class_authorized"] = restart_class_authorized
|
||||||
|
if not restart_class_authorized:
|
||||||
|
gate_payload["reasons"] = list(gate_payload.get("reasons") or []) + [
|
||||||
|
"restart class authorization denied; apply denied regardless of "
|
||||||
|
"drain proof or break-glass (fail closed, #663)",
|
||||||
|
*(report.authorization_reasons or []),
|
||||||
|
]
|
||||||
payload["apply_gate"] = gate_payload
|
payload["apply_gate"] = gate_payload
|
||||||
payload["apply_authorized"] = gate.allow
|
payload["apply_authorized"] = bool(gate.allow and restart_class_authorized)
|
||||||
payload["break_glass_requested"] = bool(request_break_glass)
|
payload["break_glass_requested"] = bool(request_break_glass)
|
||||||
payload["break_glass_authorized"] = break_glass_authorized
|
payload["break_glass_authorized"] = break_glass_authorized
|
||||||
# Even an authorized gate performs no restart here: execution is a later
|
# Even an authorized gate performs no restart here: execution is a later
|
||||||
|
|||||||
@@ -0,0 +1,381 @@
|
|||||||
|
"""``apply_authorized`` requires BOTH authorizations (#886 review blocker B1).
|
||||||
|
|
||||||
|
The #663 restart-class matrix and the #661 drain-proof hard gate are independent
|
||||||
|
authorizations that first coexisted when PR #882 landed on master and PR #886
|
||||||
|
merged it into the restart-class branch. The union preserved both, but the apply
|
||||||
|
decision consulted only the drain gate::
|
||||||
|
|
||||||
|
payload["apply_authorized"] = gate.allow # pre-fix
|
||||||
|
|
||||||
|
so a clean drain proof — or an authorized break-glass, which needs no proof at
|
||||||
|
all — reported ``apply_authorized: True`` for a restart class the least-privilege
|
||||||
|
matrix had just denied, in the same payload that carried
|
||||||
|
``allow_restart: False`` and "role 'author' may not request full_mcp_restart".
|
||||||
|
|
||||||
|
These tests pin the conjunction and the properties that must survive it. They
|
||||||
|
exercise the real MCP tool, which previously had no test coverage at all — that
|
||||||
|
absence is why the defect shipped.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import json
|
||||||
|
import os
|
||||||
|
import unittest
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
import drain_proof
|
||||||
|
import gitea_mcp_server as srv
|
||||||
|
|
||||||
|
CONTROLLER_APPROVAL_ENV = "GITEA_CONTROLLER_RESTART_APPROVAL_AUTHORIZATION"
|
||||||
|
BREAK_GLASS_ENV = "GITEA_BREAKGLASS_RESTART_AUTHORIZATION"
|
||||||
|
|
||||||
|
# A quiet control plane: nothing live, so the blast radius never masks the
|
||||||
|
# authorization outcome under test.
|
||||||
|
QUIET_SESSIONS: list[dict] = []
|
||||||
|
QUIET_LEASES: list[dict] = []
|
||||||
|
|
||||||
|
|
||||||
|
class _FakeDB:
|
||||||
|
"""Minimal control-plane DB stand-in for the restart inventory."""
|
||||||
|
|
||||||
|
def __init__(self, sessions=QUIET_SESSIONS, terminal=None):
|
||||||
|
self._sessions = list(sessions)
|
||||||
|
self._terminal = terminal
|
||||||
|
|
||||||
|
def list_sessions(self, statuses=None, limit=None):
|
||||||
|
return list(self._sessions)
|
||||||
|
|
||||||
|
def get_active_terminal_lock(self, remote=None, org=None, repo=None):
|
||||||
|
return self._terminal
|
||||||
|
|
||||||
|
|
||||||
|
def _profile(role: str) -> dict:
|
||||||
|
return {"profile_name": f"prgs-{role}", "role_kind": role, "role": role}
|
||||||
|
|
||||||
|
|
||||||
|
class _RestartToolHarness(unittest.TestCase):
|
||||||
|
"""Drives the real ``gitea_request_mcp_restart`` with a stubbed inventory."""
|
||||||
|
|
||||||
|
def _call(self, *, role: str, env: dict | None = None, **kwargs) -> dict:
|
||||||
|
environ = {k: v for k, v in os.environ.items()
|
||||||
|
if k not in (CONTROLLER_APPROVAL_ENV, BREAK_GLASS_ENV)}
|
||||||
|
environ.update(env or {})
|
||||||
|
with patch.object(srv, "_profile_operation_gate", return_value=None), \
|
||||||
|
patch.object(srv, "_resolve",
|
||||||
|
return_value=("gitea.prgs.cc",
|
||||||
|
"Scaled-Tech-Consulting",
|
||||||
|
"Gitea-Tools")), \
|
||||||
|
patch.object(srv, "get_profile", return_value=_profile(role)), \
|
||||||
|
patch.object(srv, "_control_plane_db_or_error",
|
||||||
|
return_value=(_FakeDB(), [])), \
|
||||||
|
patch.object(srv.lease_lifecycle, "list_active_leases",
|
||||||
|
return_value={"leases": list(QUIET_LEASES)}), \
|
||||||
|
patch.dict(os.environ, environ, clear=True):
|
||||||
|
return srv.gitea_request_mcp_restart(
|
||||||
|
remote="prgs",
|
||||||
|
org="Scaled-Tech-Consulting",
|
||||||
|
repo="Gitea-Tools",
|
||||||
|
session_id="probe-session",
|
||||||
|
**kwargs,
|
||||||
|
)
|
||||||
|
|
||||||
|
def _clean_proof_for(self, preview: dict) -> str:
|
||||||
|
"""Mint a genuinely clean, signature-valid proof bound to *preview*.
|
||||||
|
|
||||||
|
Built from the tool's own dry-run report, so the fingerprint matches and
|
||||||
|
the proof is rejected for authorization reasons only — never because it
|
||||||
|
was stale or forged.
|
||||||
|
"""
|
||||||
|
proof = drain_proof.build_drain_proof(
|
||||||
|
impact_report=preview,
|
||||||
|
drain_state={
|
||||||
|
"assignments_stopped": True,
|
||||||
|
"checkpoints_complete": True,
|
||||||
|
"handoffs_verified": True,
|
||||||
|
"leases_handled": True,
|
||||||
|
"acks": {},
|
||||||
|
},
|
||||||
|
requesting_session_id="probe-session",
|
||||||
|
)
|
||||||
|
self.assertTrue(proof.clean, "harness must mint a clean proof")
|
||||||
|
return json.dumps(proof.as_dict())
|
||||||
|
|
||||||
|
|
||||||
|
class TestConjunction(_RestartToolHarness):
|
||||||
|
"""AC1/AC2 — the two authorizations are ANDed, in both directions."""
|
||||||
|
|
||||||
|
def test_gate_allow_with_class_denied_yields_apply_authorized_false(self):
|
||||||
|
# An author may not request full_mcp_restart (CONTROL_ROLES only).
|
||||||
|
preview = self._call(role="author", restart_class="full_mcp_restart")
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
|
||||||
|
result = self._call(
|
||||||
|
role="author",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertTrue(result["apply_gate"]["drain_gate_allow"],
|
||||||
|
"drain gate itself should have allowed this proof")
|
||||||
|
self.assertFalse(result["apply_gate"]["restart_class_authorized"])
|
||||||
|
self.assertFalse(result["apply_authorized"],
|
||||||
|
"a clean proof must not authorize a denied class")
|
||||||
|
self.assertFalse(result["allow_restart"])
|
||||||
|
|
||||||
|
def test_gate_allow_with_class_allowed_can_yield_apply_authorized_true(self):
|
||||||
|
preview = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
self.assertTrue(preview["allow_restart"],
|
||||||
|
"operator + controller approval must authorize the class")
|
||||||
|
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertTrue(result["apply_gate"]["drain_gate_allow"])
|
||||||
|
self.assertTrue(result["apply_gate"]["restart_class_authorized"])
|
||||||
|
self.assertTrue(result["apply_authorized"],
|
||||||
|
"both authorizations pass; apply must be authorized")
|
||||||
|
|
||||||
|
def test_denial_is_attributable_to_the_authorization_that_caused_it(self):
|
||||||
|
preview = self._call(role="author", restart_class="full_mcp_restart")
|
||||||
|
result = self._call(
|
||||||
|
role="author",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
)
|
||||||
|
blob = " ".join(result["apply_gate"]["reasons"]).lower()
|
||||||
|
self.assertIn("restart class authorization denied", blob)
|
||||||
|
self.assertIn("full_mcp_restart", blob)
|
||||||
|
|
||||||
|
|
||||||
|
class TestProofCannotOverrideAuthorization(_RestartToolHarness):
|
||||||
|
"""AC3 — a clean proof never overrides a class or requester-role denial."""
|
||||||
|
|
||||||
|
def test_clean_proof_cannot_override_role_denial(self):
|
||||||
|
for role in ("author", "reviewer", "merger", "reconciler"):
|
||||||
|
with self.subTest(role=role):
|
||||||
|
preview = self._call(role=role, restart_class="full_mcp_restart")
|
||||||
|
result = self._call(
|
||||||
|
role=role,
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
|
||||||
|
def test_clean_proof_cannot_override_missing_controller_approval(self):
|
||||||
|
# Correct role, but the class demands controller approval and the
|
||||||
|
# environment carries none.
|
||||||
|
preview = self._call(role="operator", restart_class="full_mcp_restart")
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
|
||||||
|
def test_clean_proof_cannot_override_unknown_class(self):
|
||||||
|
preview = self._call(role="operator", restart_class="not_a_real_class",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"})
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="not_a_real_class",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"},
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
|
||||||
|
def test_clean_proof_cannot_override_missing_scope_target(self):
|
||||||
|
# worker_restart without target_session_id fails closed on scoping.
|
||||||
|
preview = self._call(role="operator", restart_class="worker_restart",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"})
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="worker_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"},
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestBreakGlassDoesNotCollapseTheMatrix(_RestartToolHarness):
|
||||||
|
"""AC4 — break-glass bypasses the drain proof only, never the class matrix."""
|
||||||
|
|
||||||
|
def test_break_glass_does_not_authorize_a_denied_class(self):
|
||||||
|
result = self._call(
|
||||||
|
role="author",
|
||||||
|
restart_class="host_restart",
|
||||||
|
dry_run=False,
|
||||||
|
request_break_glass=True,
|
||||||
|
env={BREAK_GLASS_ENV: "operator-issued"},
|
||||||
|
)
|
||||||
|
self.assertTrue(result["break_glass_authorized"])
|
||||||
|
self.assertTrue(result["apply_gate"]["drain_gate_allow"],
|
||||||
|
"break-glass does satisfy the drain gate")
|
||||||
|
self.assertFalse(result["apply_gate"]["restart_class_authorized"])
|
||||||
|
self.assertFalse(result["apply_authorized"],
|
||||||
|
"break-glass must not collapse the class matrix")
|
||||||
|
|
||||||
|
def test_break_glass_across_every_worker_role_and_restricted_class(self):
|
||||||
|
for role in ("author", "reviewer", "merger", "reconciler"):
|
||||||
|
for klass in ("rolling_mcp_restart", "full_mcp_restart",
|
||||||
|
"host_restart"):
|
||||||
|
with self.subTest(role=role, restart_class=klass):
|
||||||
|
result = self._call(
|
||||||
|
role=role,
|
||||||
|
restart_class=klass,
|
||||||
|
dry_run=False,
|
||||||
|
request_break_glass=True,
|
||||||
|
env={BREAK_GLASS_ENV: "operator-issued"},
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
|
||||||
|
def test_break_glass_still_works_when_the_class_is_authorized(self):
|
||||||
|
# Break-glass keeps its purpose: skipping the drain proof for a caller
|
||||||
|
# the matrix does allow.
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
request_break_glass=True,
|
||||||
|
env={BREAK_GLASS_ENV: "operator-issued",
|
||||||
|
CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
self.assertTrue(result["apply_authorized"])
|
||||||
|
self.assertEqual(result["apply_gate"]["verdict"], "break_glass")
|
||||||
|
|
||||||
|
def test_break_glass_is_not_self_assertable(self):
|
||||||
|
# Requested but no environment authorization -> no bypass, and the
|
||||||
|
# unproven apply is denied.
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
request_break_glass=True,
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
self.assertTrue(result["break_glass_requested"])
|
||||||
|
self.assertFalse(result["break_glass_authorized"])
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
self.assertIn("incident", result)
|
||||||
|
|
||||||
|
|
||||||
|
class TestRestrictedClassesStayDenied(_RestartToolHarness):
|
||||||
|
"""AC5 — restricted classes remain denied to unauthorized requesters."""
|
||||||
|
|
||||||
|
def test_restricted_classes_denied_for_worker_roles(self):
|
||||||
|
for role in ("author", "reviewer", "merger", "reconciler"):
|
||||||
|
for klass in ("rolling_mcp_restart", "full_mcp_restart",
|
||||||
|
"host_restart"):
|
||||||
|
with self.subTest(role=role, restart_class=klass):
|
||||||
|
preview = self._call(
|
||||||
|
role=role,
|
||||||
|
restart_class=klass,
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"},
|
||||||
|
)
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
self.assertFalse(preview["permission_authorized"])
|
||||||
|
self.assertFalse(preview["role_authorized"])
|
||||||
|
|
||||||
|
def test_host_restart_needs_controller_and_infrastructure_operator(self):
|
||||||
|
# controller approval alone is not enough for host_restart.
|
||||||
|
preview = self._call(role="controller", restart_class="host_restart",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"})
|
||||||
|
self.assertFalse(preview["approval_satisfied"])
|
||||||
|
self.assertFalse(preview["allow_restart"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestExistingPathsStillWork(_RestartToolHarness):
|
||||||
|
"""AC6 — valid scoped and unscoped restart paths are unaffected."""
|
||||||
|
|
||||||
|
def test_dry_run_never_reports_apply_authorization(self):
|
||||||
|
result = self._call(role="operator", restart_class="full_mcp_restart",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "yes"})
|
||||||
|
self.assertNotIn("apply_authorized", result)
|
||||||
|
self.assertNotIn("apply_gate", result)
|
||||||
|
self.assertFalse(result["apply_supported"])
|
||||||
|
self.assertFalse(result["restart_performed"])
|
||||||
|
|
||||||
|
def test_self_service_unscoped_classes_authorize_for_every_role(self):
|
||||||
|
for role in ("author", "reviewer", "merger", "reconciler",
|
||||||
|
"controller", "operator", "admin"):
|
||||||
|
for klass in ("client_reconnect", "session_reconnect"):
|
||||||
|
with self.subTest(role=role, restart_class=klass):
|
||||||
|
preview = self._call(role=role, restart_class=klass)
|
||||||
|
self.assertTrue(preview["allow_restart"])
|
||||||
|
|
||||||
|
def test_scoped_class_with_target_authorizes_and_applies(self):
|
||||||
|
env = {CONTROLLER_APPROVAL_ENV: "operator-approved"}
|
||||||
|
preview = self._call(role="operator", restart_class="worker_restart",
|
||||||
|
target_session_id="worker-1", env=env)
|
||||||
|
self.assertTrue(preview["allow_restart"])
|
||||||
|
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="worker_restart",
|
||||||
|
target_session_id="worker-1",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json=self._clean_proof_for(preview),
|
||||||
|
env=env,
|
||||||
|
)
|
||||||
|
self.assertTrue(result["apply_authorized"])
|
||||||
|
|
||||||
|
def test_apply_still_denies_without_any_proof(self):
|
||||||
|
# The #661 hard gate is untouched by the conjunction.
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_gate"]["drain_gate_allow"])
|
||||||
|
self.assertTrue(result["apply_gate"]["restart_class_authorized"])
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
self.assertEqual(result["incident"]["kind"], "restart_drain_gate_denied")
|
||||||
|
|
||||||
|
def test_apply_denies_on_malformed_proof(self):
|
||||||
|
result = self._call(
|
||||||
|
role="operator",
|
||||||
|
restart_class="full_mcp_restart",
|
||||||
|
dry_run=False,
|
||||||
|
drain_proof_json="{not valid json",
|
||||||
|
env={CONTROLLER_APPROVAL_ENV: "operator-approved"},
|
||||||
|
)
|
||||||
|
self.assertFalse(result["apply_authorized"])
|
||||||
|
self.assertTrue(any("invalid drain_proof_json" in reason
|
||||||
|
for reason in result["apply_gate"]["reasons"]))
|
||||||
|
|
||||||
|
def test_tool_never_restarts_on_any_path(self):
|
||||||
|
for kwargs in (
|
||||||
|
{"restart_class": "client_reconnect"},
|
||||||
|
{"restart_class": "full_mcp_restart", "dry_run": False},
|
||||||
|
{"restart_class": "host_restart", "dry_run": False,
|
||||||
|
"request_break_glass": True},
|
||||||
|
):
|
||||||
|
with self.subTest(**kwargs):
|
||||||
|
result = self._call(role="operator", env={
|
||||||
|
CONTROLLER_APPROVAL_ENV: "yes", BREAK_GLASS_ENV: "yes"},
|
||||||
|
**kwargs)
|
||||||
|
self.assertFalse(result["restart_performed"])
|
||||||
|
self.assertFalse(result["apply_supported"])
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
@@ -105,3 +105,120 @@ def test_cross_links_do_not_embed_secrets():
|
|||||||
text = _read(path)
|
text = _read(path)
|
||||||
for marker in ("ghp_", "BEGIN PRIVATE KEY", "Authorization: Bearer"):
|
for marker in ("ghp_", "BEGIN PRIVATE KEY", "Authorization: Bearer"):
|
||||||
assert marker not in text, f"{path} contains {marker!r}"
|
assert marker not in text, f"{path} contains {marker!r}"
|
||||||
|
|
||||||
|
|
||||||
|
# --- Coordinator doc stays in lock-step with the tool (#886 review blocker B2) --
|
||||||
|
#
|
||||||
|
# PR #882 moved the #661 drain-proof hard gate *into* gitea_request_mcp_restart,
|
||||||
|
# but the coordinator document still described the proof as "a separate child"
|
||||||
|
# and omitted both new parameters. Nothing referenced that document, so nothing
|
||||||
|
# caught the drift. These tests bind the prose to the real signature.
|
||||||
|
|
||||||
|
COORDINATOR_DOC = REPO_ROOT / "docs" / "mcp-restart-coordinator.md"
|
||||||
|
|
||||||
|
# Affirmative claims that were accurate before #661 landed and are now false.
|
||||||
|
# Matched against whitespace-normalized text so re-wrapping cannot hide them.
|
||||||
|
# Deliberately not the bare phrase "a separate child": the corrected prose uses
|
||||||
|
# it in a negation ("no longer a separate child operation"), and a guard that
|
||||||
|
# forbids naming the old behaviour would block explaining that it changed.
|
||||||
|
STALE_PRE_661_PHRASES = (
|
||||||
|
"gated by a drain proof (a separate child)",
|
||||||
|
"is a later child gated by a drain proof",
|
||||||
|
"mutative apply path is explicitly out of scope",
|
||||||
|
"apply is gated by a drain proof (a separate child)",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _documented_signature_block() -> str:
|
||||||
|
"""The fenced signature block for the tool, as published in the doc."""
|
||||||
|
text = _read(COORDINATOR_DOC)
|
||||||
|
marker = "gitea_request_mcp_restart("
|
||||||
|
start = text.index(marker)
|
||||||
|
end = text.index("```", start)
|
||||||
|
return text[start:end]
|
||||||
|
|
||||||
|
|
||||||
|
def test_documented_signature_matches_the_real_tool_signature():
|
||||||
|
import inspect
|
||||||
|
|
||||||
|
import gitea_mcp_server
|
||||||
|
|
||||||
|
block = _documented_signature_block()
|
||||||
|
real = inspect.signature(gitea_mcp_server.gitea_request_mcp_restart)
|
||||||
|
for name in real.parameters:
|
||||||
|
assert name in block, (
|
||||||
|
f"docs/mcp-restart-coordinator.md documents no {name!r} parameter; "
|
||||||
|
"the published signature has drifted from the tool"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_drain_proof_and_break_glass_parameters_are_documented():
|
||||||
|
block = _documented_signature_block()
|
||||||
|
for name in ("drain_proof_json", "request_break_glass"):
|
||||||
|
assert name in block, f"signature block missing {name}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_restart_class_and_target_scoping_parameters_survive():
|
||||||
|
block = _documented_signature_block()
|
||||||
|
for name in ("restart_class", "target_session_id", "target_role",
|
||||||
|
"target_connector"):
|
||||||
|
assert name in block, f"signature block lost #663 parameter {name}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_gate_is_documented_as_executing_inside_this_tool():
|
||||||
|
lower = _read(COORDINATOR_DOC).lower()
|
||||||
|
assert "inside this tool" in lower, (
|
||||||
|
"the coordinator doc must state that the drain-proof gate executes in "
|
||||||
|
"gitea_request_mcp_restart, not in a later child"
|
||||||
|
)
|
||||||
|
assert "no longer a separate child operation" in lower
|
||||||
|
|
||||||
|
|
||||||
|
def test_stale_pre_661_wording_cannot_return():
|
||||||
|
normalized = " ".join(_read(COORDINATOR_DOC).split()).lower()
|
||||||
|
for phrase in STALE_PRE_661_PHRASES:
|
||||||
|
assert phrase not in normalized, (
|
||||||
|
f"stale pre-#661 wording returned to the coordinator doc: {phrase!r}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_dry_run_versus_apply_behavior_is_documented():
|
||||||
|
lower = _read(COORDINATOR_DOC).lower()
|
||||||
|
assert "dry_run=true" in lower and "dry_run=false" in lower
|
||||||
|
assert "apply_supported" in lower and "restart_performed" in lower
|
||||||
|
assert "never restarts anything" in lower
|
||||||
|
|
||||||
|
|
||||||
|
def test_authorization_ordering_and_conjunction_are_documented():
|
||||||
|
text = _read(COORDINATOR_DOC)
|
||||||
|
lower = text.lower()
|
||||||
|
assert "authorization ordering" in lower
|
||||||
|
assert "allow_restart" in text
|
||||||
|
assert "apply_authorized" in text
|
||||||
|
# The conjunction itself, and the attribution fields behind it.
|
||||||
|
assert "gate.allow and allow_restart" in text
|
||||||
|
for field in ("drain_gate_allow", "restart_class_authorized"):
|
||||||
|
assert field in text, f"doc omits apply_gate.{field}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_break_glass_scope_is_documented_as_drain_proof_only():
|
||||||
|
text = _read(COORDINATOR_DOC)
|
||||||
|
lower = text.lower()
|
||||||
|
assert "break-glass" in lower
|
||||||
|
assert "drain proof only" in lower, (
|
||||||
|
"doc must state break-glass never bypasses the restart-class matrix"
|
||||||
|
)
|
||||||
|
assert "GITEA_BREAKGLASS_RESTART_AUTHORIZATION" in text
|
||||||
|
|
||||||
|
|
||||||
|
def test_fail_closed_on_apply_is_documented():
|
||||||
|
lower = _read(COORDINATOR_DOC).lower()
|
||||||
|
assert "fail closed" in lower
|
||||||
|
for condition in ("expired", "unclean", "tampered", "stale"):
|
||||||
|
assert condition in lower, f"fail-closed list omits {condition!r}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_coordinator_doc_embeds_no_secrets():
|
||||||
|
text = _read(COORDINATOR_DOC)
|
||||||
|
for marker in ("ghp_", "BEGIN PRIVATE KEY", "Authorization: Bearer"):
|
||||||
|
assert marker not in text, f"{COORDINATOR_DOC} contains {marker!r}"
|
||||||
|
|||||||
Reference in New Issue
Block a user