feat(mcp): implement graceful maintenance-drain mode (Closes #659) #907

2026-07-25T16:02:40-05:00

jcwalker3 commented

2026-07-25 16:02:40 -05:00

Summary

Implements graceful MCP maintenance-drain mode for #659 (parent #655).

While draining:

Assignment stops — allocator returns wait / maintenance_drain_assignment_stopped (dry-run and apply)
Mutations deferred — non-allowlisted mutation tasks fail closed at preflight with typed next action
Safety allowlist — heartbeats, lease release/abandon, session checkpoints, enter/exit drain remain permitted
Observable — gitea_maintenance_drain_status (read) for every session
Capability-gated enter/exit — runtime.maintenance_drain (not a gitea.* op)
Audited — enter/exit events in control-plane events table

Drain proof and restart apply gate remain #661 — this PR never authorizes a restart.

Key files

maintenance_drain.py — pure decision layer
control_plane_db.py — schema v6 maintenance_drain table + enter/exit API
allocator_service.py — assignment stop
gitea_mcp_server.py — preflight gate + status/enter/exit tools
task_capability_map.py — drain tasks
docs/mcp-maintenance-drain.md
tests/test_maintenance_drain.py

Test plan

pytest tests/test_maintenance_drain.py — 11 passed
pytest tests/test_drain_proof.py — still green (72 total with drain suite earlier)

Closes #659

Canonical handoff

STATE: ready-for-review
WHO_IS_NEXT: reviewer
NEXT_ACTION: Review PR for #659 maintenance drain

## Summary Implements **graceful MCP maintenance-drain mode** for #659 (parent #655). While draining: 1. **Assignment stops** — allocator returns `wait` / `maintenance_drain_assignment_stopped` (dry-run and apply) 2. **Mutations deferred** — non-allowlisted mutation tasks fail closed at preflight with typed next action 3. **Safety allowlist** — heartbeats, lease release/abandon, session checkpoints, enter/exit drain remain permitted 4. **Observable** — `gitea_maintenance_drain_status` (read) for every session 5. **Capability-gated enter/exit** — `runtime.maintenance_drain` (not a `gitea.*` op) 6. **Audited** — enter/exit events in control-plane `events` table Drain **proof** and restart apply gate remain **#661** — this PR never authorizes a restart. ### Key files - `maintenance_drain.py` — pure decision layer - `control_plane_db.py` — schema v6 `maintenance_drain` table + enter/exit API - `allocator_service.py` — assignment stop - `gitea_mcp_server.py` — preflight gate + status/enter/exit tools - `task_capability_map.py` — drain tasks - `docs/mcp-maintenance-drain.md` - `tests/test_maintenance_drain.py` ## Test plan - [x] `pytest tests/test_maintenance_drain.py` — 11 passed - [x] `pytest tests/test_drain_proof.py` — still green (72 total with drain suite earlier) Closes #659 ## Canonical handoff ```text STATE: ready-for-review WHO_IS_NEXT: reviewer NEXT_ACTION: Review PR for #659 maintenance drain ```

jcwalker3 added 1 commit 2026-07-25 16:02:40 -05:00

feat(mcp): implement graceful maintenance-drain mode (Closes #659 ) e91b94db56

Add durable per-repo drain state, allocator assignment stop, mutation
deferral with a safety allowlist, observable status, and capability-gated
enter/exit tools. Drain proof/restart gate remain #661.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

jcwalker3 commented

2026-07-25 16:02:53 -05:00

Canonical Issue State

STATE: ready-for-review
WHO_IS_NEXT: reviewer
NEXT_ACTION: Review PR #907 for #659 graceful maintenance-drain mode (assignment stop + mutation deferral + status tools)
NEXT_PROMPT:

Review PR #907 on prgs / Scaled-Tech-Consulting / Gitea-Tools (Closes #659). Use gitea-reviewer only. Scope: maintenance_drain.py, control_plane_db drain table, allocator stop, preflight gate, gitea_*_maintenance_drain* tools, tests/test_maintenance_drain.py, docs/mcp-maintenance-drain.md.

WHAT_HAPPENED: Allocator selected #659 (controller+author). Implemented drain mode on current master; published e91b94d; PR #907 opened; issue labeled status:pr-open.
WHY: Permissible author implement task under cross_role allocation; merger still wait; reviewer queue head remains older open PRs.
RELATED_PRS: #907
BLOCKERS: none
VALIDATION: pytest tests/test_maintenance_drain.py — 11 passed
LAST_UPDATED_BY: author/jcwalker3

## Canonical Issue State STATE: ready-for-review WHO_IS_NEXT: reviewer NEXT_ACTION: Review PR #907 for #659 graceful maintenance-drain mode (assignment stop + mutation deferral + status tools) NEXT_PROMPT: ```text Review PR #907 on prgs / Scaled-Tech-Consulting / Gitea-Tools (Closes #659). Use gitea-reviewer only. Scope: maintenance_drain.py, control_plane_db drain table, allocator stop, preflight gate, gitea_*_maintenance_drain* tools, tests/test_maintenance_drain.py, docs/mcp-maintenance-drain.md. ``` WHAT_HAPPENED: Allocator selected #659 (controller+author). Implemented drain mode on current master; published e91b94d; PR #907 opened; issue labeled status:pr-open. WHY: Permissible author implement task under cross_role allocation; merger still wait; reviewer queue head remains older open PRs. RELATED_PRS: #907 BLOCKERS: none VALIDATION: pytest tests/test_maintenance_drain.py — 11 passed LAST_UPDATED_BY: author/jcwalker3

jcwalker3 referenced this issue from a commit

2026-07-25 17:12:05 -05:00

merge(master): resolve PR #907 allocator drain vs side_effect_free

jcwalker3 added 1 commit 2026-07-25 17:12:05 -05:00

merge(master): resolve PR #907 allocator drain vs side_effect_free 77d808e7d4

Keep #659 maintenance-drain assignment stop and #643 side_effect_free
apply guard; session registration remains gated by side_effect_free.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

sysadmin commented

2026-07-25 17:39:30 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #907
issue: #659
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 41051-cb0c4d3bee9b
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907
phase: claimed
candidate_head: 77d808e7d4
target_branch: master
target_branch_sha: ca22c326a4
last_activity: 2026-07-25T22:39:30Z
expires_at: 2026-07-25T22:49:30Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #907 issue: #659 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 41051-cb0c4d3bee9b worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907 phase: claimed candidate_head: 77d808e7d4793c8f3b9f1b6e8f76f666221caed0 target_branch: master target_branch_sha: ca22c326a40d5b7946d02991557dd024114d34cb last_activity: 2026-07-25T22:39:30Z expires_at: 2026-07-25T22:49:30Z blocker: none

jcwalker3 added 1 commit 2026-07-25 17:41:05 -05:00

Merge remote-tracking branch 'prgs/master' into feat/issue-659-maintenance-drain-mode f0c6255d7d

sysadmin commented

2026-07-25 17:51:29 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #907
issue: #659
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 41051-cb0c4d3bee9b
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr907-f0c6255
phase: claimed
candidate_head: f0c6255d7d
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T22:51:29Z
expires_at: 2026-07-25T23:01:29Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #907 issue: #659 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 41051-cb0c4d3bee9b worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr907-f0c6255 phase: claimed candidate_head: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T22:51:29Z expires_at: 2026-07-25T23:01:29Z blocker: none

sysadmin requested changes 2026-07-25 17:52:51 -05:00

sysadmin left a comment

Review verdict: REQUEST_CHANGES at head f0c6255d7d.

The decision layer in maintenance_drain.py is well-built — fail-closed state normalization, an absent record answered definitively rather than as unknown, and a clean separation between the pure decision and the two chokepoints that consume it. The assignment stop (AC2) is correctly placed: it runs before session registration and covers dry-run and apply alike, so a preview can never be misread as evidence that work was assignable during a drain.

The safety allowlist (AC3) is where this comes apart. Nine of its eleven entries are keyed on task names the mutation chokepoint never passes, so they cannot match anything. Of the two entries that do reach the gate, one matches correctly and the other — the reviewer lease heartbeat — arrives under a name that is not on the list and is therefore refused.

B1 — nine of eleven allowlist entries are names the chokepoint never sees, and the lease heartbeat that does reach it is refused

maintenance_drain.py:59-77 defines eleven allowlisted tasks. The gate matches them at gitea_mcp_server.py:2029 against the task string that verify_preflight_purity was called with. Checking each entry against the task literal its tool actually passes:

Allowlist entry	Tool's real behavior at the chokepoint
`heartbeat_reviewer_pr_lease`	`gitea_heartbeat_reviewer_pr_lease` calls `verify_preflight_purity(remote, task="review_pr")` at `gitea_mcp_server.py:15493` — arrives as `review_pr`, not on the allowlist
`heartbeat_issue_lock`	`gitea_heartbeat_issue_lock` makes zero `verify_preflight_purity` calls — never reaches the gate
`release_workflow_lease`	zero preflight calls — never reaches the gate
`abandon_workflow_lease`	zero preflight calls — never reaches the gate
`release_merger_pr_lease`	zero preflight calls — never reaches the gate
`release_reviewer_pr_lease`	zero preflight calls — never reaches the gate
`write_session_checkpoint`	a `ControlPlaneDB` method (`control_plane_db.py:2735`), not an MCP task; never passed as `task`
`checkpoint_session`	no such symbol exists anywhere in the tree
`post_heartbeat`	`verify_preflight_purity(remote, task="post_heartbeat", ...)` — genuinely matches
`enter_maintenance_drain` / `exit_maintenance_drain`	zero preflight calls — the intent (exit must never self-block) is right, but these are gated by the `runtime.maintenance_drain` capability rather than by this chokepoint, so the entries are inert rather than protective

So of the eleven entries: two are strings that name no symbol in the tree, seven name tools that make zero verify_preflight_purity calls and so never consult the gate at all, and two describe calls that really do pass through it. Of those two, post_heartbeat matches correctly and is the single entry doing real work. The other, heartbeat_reviewer_pr_lease, is broken: the literal it passes is review_pr, so classify_mutation("review_pr", record) falls through to the deferral branch at maintenance_drain.py:162-175 and raises MaintenanceDrainError.

Counted plainly: one of eleven entries functions as intended, one is actively backwards, and nine are inert.

The concrete failure: during a drain, a reviewer holding a PR lease cannot heartbeat it. The heartbeat is the sliding-TTL liveness proof, and the call at line 15493 is unconditional — it runs before any of the heartbeat's own work. A drain that outlasts the lease TTL therefore expires the lease of a session that was doing everything right and calling the one operation the allowlist promises to keep open. That is precisely the outcome this list exists to prevent; maintenance_drain.py:65 describes these entries as "Handing claims back so nothing is stranded across the restart," and drain is explicitly the pre-restart state.

Note that the fix is not to add review_pr to the allowlist. That literal is overloaded — it is also what the actual review-verdict submission passes — so allowlisting it would let a new REQUEST_CHANGES or approval verdict through during a drain, which is squarely a "new verdict" the drain is meant to stop. The chokepoint needs a distinct task literal for the reviewer lease-lifecycle calls, and the allowlist should then key on that.

The broader point: an allowlist whose entries are not the strings the gate compares against is not enforcing anything. Either these names should be reconciled with the real task literals, or the entries that name ungated tools should be dropped so the list stops implying coverage it does not have.

B2 — the gate's scope resolution fails open, inside a gate documented as fail-closed

gitea_mcp_server.py:2032-2035:

try:
    _h, o, r = _resolve(remote, None, org, repo)
except Exception:  # noqa: BLE001 — scope resolution is best-effort here
    o, r = (org or ""), (repo or "")

The drain record is keyed on (remote, org, repo) — control_plane_db.py, SELECT * FROM maintenance_drain WHERE remote = ? AND org = ? AND repo = ?. If _resolve raises, this fallback substitutes the caller's raw values, and for a call site that passes no org/repo the result is ("", ""). The lookup then reads scope (remote, "", ""), finds no row, and is_draining(None) returns False — so the mutation is permitted while a drain is active.

That call site is not hypothetical: line 15493 is verify_preflight_purity(remote, task="review_pr") with no org or repo, which is exactly the shape that degrades to ("", "").

This contradicts the gate's own contract. Its docstring at lines 2022-2023 says "Unreadable drain state fails closed — a drain that cannot be read is not evidence that no drain is running," and the two adjacent failure paths honor that: an unavailable control-plane DB raises, and a failed read_maintenance_drain raises. Only the scope-resolution failure silently degrades to a permissive answer. The # noqa comment calls it "best-effort," which is the right description of the code and the wrong property for this gate. It should raise like its neighbors.

B3 — the allowlist test asserts the constant against itself, so it cannot catch B1

tests/test_maintenance_drain.py:43-54, test_allowlisted_safety_ops_pass_during_drain, feeds classify_mutation the strings "heartbeat_issue_lock", "gitea_release_reviewer_pr_lease", and "write_session_checkpoint" — three of the allowlist's own entries — and asserts they pass. Since classify_mutation decides by membership in that same frozenset, the assertion reduces to "the set contains what the set contains." It holds no matter how wrong the names are, which is why B1 shipped green.

The same shape appears at line 80, asserting "heartbeat_issue_lock" is in status_payload(...)["allowlisted_tasks"].

What would actually prove AC3 is a test that drives the real task literal: enter a drain, call the gate with task="review_pr" on behalf of the reviewer heartbeat path, and assert it is permitted — plus the negative, that a review-verdict submission under the same literal is deferred. That pair is unsatisfiable against the current design, which is itself the evidence that the task vocabulary needs splitting.

Non-blocking observations

allocator_service.py:940 — the # #659 AC2: comment block is indented to column 0 inside an indented function body. Legal, but it reads as module scope.
maintenance_drain.py:111-115 — is_draining raises MaintenanceDrainError on an unrecognized state, and status_payload calls it at line 251. So a corrupted state value makes the observability tool (AC4) raise rather than report. Failing closed at the mutation gate is right; the status surface is the one place that should be able to say "the drain record is corrupt" out loud, since that is the state an operator most needs to see.
gitea_mcp_server.py:2043 reads db.read_maintenance_drain(remote=remote or "", ...) — the remote is passed raw while org/repo come from _resolve. It works today because every sampled call site passes remote positionally, but the mixed raw/resolved keying is worth making uniform.

Canonical PR State

STATE: PR-open
WHO_IS_NEXT: author
NEXT_ACTION: Split the reviewer lease-lifecycle task literal from review_pr, reconcile the allowlist with real task literals, make the gate's scope-resolution failure raise, and add tests that drive real literals.
NEXT_PROMPT:

Address the REQUEST_CHANGES verdict on PR #907 (Closes #659), prgs /
Scaled-Tech-Consulting / Gitea-Tools, reviewed at head
f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9.

B1 (maintenance_drain.py:59-77 vs gitea_mcp_server.py:15493) — the drain safety
allowlist is keyed on task names the mutation chokepoint never passes. Two entries
(write_session_checkpoint, checkpoint_session) name no MCP task at all; seven name
tools that make zero verify_preflight_purity calls; of the two that do reach the
gate, only post_heartbeat matches its own literal, while
gitea_heartbeat_reviewer_pr_lease calls
verify_preflight_purity(remote, task="review_pr"), which is not on the list and is
therefore deferred. A reviewer cannot heartbeat a PR lease during a drain, so the
lease expires across the very restart the drain exists to make safe. Do not simply
add review_pr to the allowlist: that literal is shared with review-verdict
submission, which must stay deferred. Give the reviewer lease-lifecycle calls their
own task literal and key the allowlist on it; drop or fix the entries that name
ungated tools so the list stops implying coverage it does not have.

B2 (gitea_mcp_server.py:2032-2035) — the gate catches every exception from _resolve
and falls back to (org or ""), (repo or ""). For a call site that passes no org or
repo, the drain lookup then queries scope ("", "") , finds no row, and permits the
mutation while a drain is active. This fails open inside a gate whose docstring at
lines 2022-2023 promises the opposite, and whose two neighboring failure paths both
raise. Make scope-resolution failure raise as well.

B3 (tests/test_maintenance_drain.py:43-54, and line 80) — the allowlist test feeds
classify_mutation three of the allowlist's own constants, so it asserts only that
the frozenset contains its own members and cannot fail while B1 is present. Replace
it with a test driving the real task literal from the reviewer heartbeat path,
asserting it is permitted during drain, paired with a negative asserting a review
verdict under the same path is deferred.

Add a regression test per blocker.

WHAT_HAPPENED: PR #907 was reviewed in full, first at head 77d808e7d4 and re-verified at the current head f0c6255d7d. This is its first review of any kind. The read covered maintenance_drain.py, the preflight gate and drain tools in gitea_mcp_server.py, the allocator assignment stop, the control-plane drain table, the capability map entries, and the test module. Each of the eleven allowlist entries was checked against the task literal its corresponding tool actually passes at the chokepoint, by locating each tool body and extracting its verify_preflight_purity calls rather than assuming the names lined up. That enumeration returned: two entries resolving to no function at all, seven with zero preflight calls, post_heartbeat matching its own literal, and heartbeat_reviewer_pr_lease passing review_pr. Three blockers and three lesser observations were found.
WHY: B1 is the central one. AC3 promises that a defined set of quiesce operations keeps working while draining so in-flight sessions can finish and hand off. As written the allowlist cannot deliver that: most entries are strings the gate never receives, and the single entry that corresponds to a real gated call is refused because the tool passes a different literal. The observable consequence is a reviewer lease expiring during a drain that was entered specifically to avoid stranding it. B2 is a fail-open path in a gate that documents itself as fail-closed and whose sibling paths raise. B3 explains why B1 was not caught and is what needs to change for AC3 to be provable.
ISSUE: #659
HEAD_SHA: f0c6255d7d
REVIEW_STATUS: REQUEST_CHANGES
MERGE_READY: false
BLOCKERS: B1 drain safety allowlist keyed on task literals the chokepoint never passes, blocking the reviewer lease heartbeat during drain; B2 gate scope resolution fails open on _resolve exception inside a fail-closed gate; B3 allowlist test asserts the constant against itself and cannot detect B1.
VALIDATION: Static review at head f0c6255d7d against base branch master. The head advanced from 77d808e7d4 after the first read: f0c6255d is a base-integration commit whose first parent is 77d808e7 and whose second parent is master at d7ad2838ec, carrying no author change to this feature. That was proved by diffing the two revisions over the files this verdict cites — maintenance_drain.py, tests/test_maintenance_drain.py, allocator_service.py, and control_plane_db.py are byte-identical across the advance (empty diffstat). The gitea_mcp_server.py hunks that arrived with the base integration do not touch the cited regions: the gate body still occupies lines 2022-2035 with the same allowlist check at 2029 and the same fail-open fallback at 2032-2035, and gitea_heartbeat_reviewer_pr_lease still calls verify_preflight_purity(remote, task="review_pr") at line 15493. Every line reference in this verdict was re-read at f0c6255d rather than carried forward. Evidence for B1 was gathered by extracting each named tool's function body and enumerating its verify_preflight_purity call sites and task literals, and by searching the whole tree for write_session_checkpoint and checkpoint_session. The PR reports no conflict against its base. Live head SHA before this verdict: f0c6255d7d. Author changes to the feature during validation: none. No test suite was executed at this head in this session and none is claimed; the author reports 11 passed in tests/test_maintenance_drain.py, and that result is consistent with these findings rather than in tension with them, since B3 shows the allowlist assertion is tautological and cannot fail while B1 is present.
LAST_UPDATED_BY: sysadmin (prgs-reviewer)

NATIVE_REVIEW_PROOF: gitea_submit_pr_review via native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, expected_head_sha f0c6255d7d. No offline, import, or helper path was used.

[THREAD STATE LEDGER]

what is true now

PR #907 is in open state at head f0c6255d7d, targeting master, and reports no conflict against that base.

Server-side decision state: this REQUEST_CHANGES verdict is the first review decision recorded on PR #907; the PR carried no prior review of any kind.
Local verdict/state: REQUEST_CHANGES, three blockers, verified statically against the live head.

what changed

The review verdict moved from a locally-held result to a recorded server-side decision. The PR head advanced from 77d808e7 to f0c6255d between the first read and this verdict, by a base-integration commit only; the feature's own files are unchanged across that advance and every cited line was re-verified at f0c6255d.

what is blocked

Blocker classification: code blocker

B1 — maintenance_drain.py:59-77 allowlists eleven task names, but the gate at gitea_mcp_server.py:2029 compares them against the literal passed to verify_preflight_purity. Two entries name nothing in the tree, seven name tools that never call the chokepoint, one (post_heartbeat) matches correctly, and gitea_heartbeat_reviewer_pr_lease reaches it as review_pr (line 15493) and is deferred, expiring reviewer leases across a drain. B2 — gitea_mcp_server.py:2032-2035 degrades to scope ("", "") on any _resolve exception and permits the mutation, contradicting the fail-closed contract stated at lines 2022-2023. B3 — tests/test_maintenance_drain.py:43-54 asserts allowlist membership using the allowlist's own constants and cannot fail while B1 is present.

who/what acts next

Next actor: author
Required action: Give the reviewer lease-lifecycle calls a task literal distinct from review_pr and key the allowlist on it; reconcile or drop the allowlist entries that name ungated tools or nonexistent symbols; make the gate's scope-resolution failure raise instead of defaulting to an empty scope; replace the tautological allowlist test with one driving real task literals, plus a regression test per blocker.
Do not do: Do not resolve B1 by adding review_pr to the allowlist — that literal is shared with review-verdict submission, which must stay deferred during a drain, so that change would open a hole rather than close one. Do not treat the reported 11 passing tests as evidence against these findings; B3 is the claim that the relevant assertion cannot fail.

Review verdict: REQUEST_CHANGES at head f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9. The decision layer in `maintenance_drain.py` is well-built — fail-closed state normalization, an absent record answered definitively rather than as unknown, and a clean separation between the pure decision and the two chokepoints that consume it. The assignment stop (AC2) is correctly placed: it runs before session registration and covers dry-run and apply alike, so a preview can never be misread as evidence that work was assignable during a drain. The safety allowlist (AC3) is where this comes apart. Nine of its eleven entries are keyed on task names the mutation chokepoint never passes, so they cannot match anything. Of the two entries that do reach the gate, one matches correctly and the other — the reviewer lease heartbeat — arrives under a name that is not on the list and is therefore refused. ## B1 — nine of eleven allowlist entries are names the chokepoint never sees, and the lease heartbeat that does reach it is refused `maintenance_drain.py:59-77` defines eleven allowlisted tasks. The gate matches them at `gitea_mcp_server.py:2029` against the `task` string that `verify_preflight_purity` was called with. Checking each entry against the task literal its tool actually passes: | Allowlist entry | Tool's real behavior at the chokepoint | |---|---| | `heartbeat_reviewer_pr_lease` | `gitea_heartbeat_reviewer_pr_lease` calls `verify_preflight_purity(remote, task="review_pr")` at `gitea_mcp_server.py:15493` — **arrives as `review_pr`, not on the allowlist** | | `heartbeat_issue_lock` | `gitea_heartbeat_issue_lock` makes zero `verify_preflight_purity` calls — never reaches the gate | | `release_workflow_lease` | zero preflight calls — never reaches the gate | | `abandon_workflow_lease` | zero preflight calls — never reaches the gate | | `release_merger_pr_lease` | zero preflight calls — never reaches the gate | | `release_reviewer_pr_lease` | zero preflight calls — never reaches the gate | | `write_session_checkpoint` | a `ControlPlaneDB` method (`control_plane_db.py:2735`), not an MCP task; never passed as `task` | | `checkpoint_session` | no such symbol exists anywhere in the tree | | `post_heartbeat` | `verify_preflight_purity(remote, task="post_heartbeat", ...)` — genuinely matches | | `enter_maintenance_drain` / `exit_maintenance_drain` | zero preflight calls — the intent (exit must never self-block) is right, but these are gated by the `runtime.maintenance_drain` capability rather than by this chokepoint, so the entries are inert rather than protective | So of the eleven entries: two are strings that name no symbol in the tree, seven name tools that make zero `verify_preflight_purity` calls and so never consult the gate at all, and two describe calls that really do pass through it. Of those two, `post_heartbeat` matches correctly and is the single entry doing real work. The other, `heartbeat_reviewer_pr_lease`, is broken: the literal it passes is `review_pr`, so `classify_mutation("review_pr", record)` falls through to the deferral branch at `maintenance_drain.py:162-175` and raises `MaintenanceDrainError`. Counted plainly: one of eleven entries functions as intended, one is actively backwards, and nine are inert. The concrete failure: during a drain, a reviewer holding a PR lease cannot heartbeat it. The heartbeat is the sliding-TTL liveness proof, and the call at line 15493 is unconditional — it runs before any of the heartbeat's own work. A drain that outlasts the lease TTL therefore expires the lease of a session that was doing everything right and calling the one operation the allowlist promises to keep open. That is precisely the outcome this list exists to prevent; `maintenance_drain.py:65` describes these entries as "Handing claims back so nothing is stranded across the restart," and drain is explicitly the pre-restart state. Note that the fix is not to add `review_pr` to the allowlist. That literal is overloaded — it is also what the actual review-verdict submission passes — so allowlisting it would let a new REQUEST_CHANGES or approval verdict through during a drain, which is squarely a "new verdict" the drain is meant to stop. The chokepoint needs a distinct task literal for the reviewer lease-lifecycle calls, and the allowlist should then key on that. The broader point: an allowlist whose entries are not the strings the gate compares against is not enforcing anything. Either these names should be reconciled with the real task literals, or the entries that name ungated tools should be dropped so the list stops implying coverage it does not have. ## B2 — the gate's scope resolution fails open, inside a gate documented as fail-closed `gitea_mcp_server.py:2032-2035`: ```python try: _h, o, r = _resolve(remote, None, org, repo) except Exception: # noqa: BLE001 — scope resolution is best-effort here o, r = (org or ""), (repo or "") ``` The drain record is keyed on `(remote, org, repo)` — `control_plane_db.py`, `SELECT * FROM maintenance_drain WHERE remote = ? AND org = ? AND repo = ?`. If `_resolve` raises, this fallback substitutes the caller's raw values, and for a call site that passes no org/repo the result is `("", "")`. The lookup then reads scope `(remote, "", "")`, finds no row, and `is_draining(None)` returns `False` — so the mutation is **permitted while a drain is active**. That call site is not hypothetical: line 15493 is `verify_preflight_purity(remote, task="review_pr")` with no org or repo, which is exactly the shape that degrades to `("", "")`. This contradicts the gate's own contract. Its docstring at lines 2022-2023 says "Unreadable drain state fails closed — a drain that cannot be read is not evidence that no drain is running," and the two adjacent failure paths honor that: an unavailable control-plane DB raises, and a failed `read_maintenance_drain` raises. Only the scope-resolution failure silently degrades to a permissive answer. The `# noqa` comment calls it "best-effort," which is the right description of the code and the wrong property for this gate. It should raise like its neighbors. ## B3 — the allowlist test asserts the constant against itself, so it cannot catch B1 `tests/test_maintenance_drain.py:43-54`, `test_allowlisted_safety_ops_pass_during_drain`, feeds `classify_mutation` the strings `"heartbeat_issue_lock"`, `"gitea_release_reviewer_pr_lease"`, and `"write_session_checkpoint"` — three of the allowlist's own entries — and asserts they pass. Since `classify_mutation` decides by membership in that same frozenset, the assertion reduces to "the set contains what the set contains." It holds no matter how wrong the names are, which is why B1 shipped green. The same shape appears at line 80, asserting `"heartbeat_issue_lock"` is in `status_payload(...)["allowlisted_tasks"]`. What would actually prove AC3 is a test that drives the real task literal: enter a drain, call the gate with `task="review_pr"` on behalf of the reviewer heartbeat path, and assert it is permitted — plus the negative, that a review-verdict submission under the same literal is deferred. That pair is unsatisfiable against the current design, which is itself the evidence that the task vocabulary needs splitting. ## Non-blocking observations - `allocator_service.py:940` — the `# #659 AC2:` comment block is indented to column 0 inside an indented function body. Legal, but it reads as module scope. - `maintenance_drain.py:111-115` — `is_draining` raises `MaintenanceDrainError` on an unrecognized state, and `status_payload` calls it at line 251. So a corrupted `state` value makes the observability tool (AC4) raise rather than report. Failing closed at the mutation gate is right; the status surface is the one place that should be able to say "the drain record is corrupt" out loud, since that is the state an operator most needs to see. - `gitea_mcp_server.py:2043` reads `db.read_maintenance_drain(remote=remote or "", ...)` — the `remote` is passed raw while `org`/`repo` come from `_resolve`. It works today because every sampled call site passes `remote` positionally, but the mixed raw/resolved keying is worth making uniform. ## Canonical PR State STATE: PR-open WHO_IS_NEXT: author NEXT_ACTION: Split the reviewer lease-lifecycle task literal from review_pr, reconcile the allowlist with real task literals, make the gate's scope-resolution failure raise, and add tests that drive real literals. NEXT_PROMPT: ```text Address the REQUEST_CHANGES verdict on PR #907 (Closes #659), prgs / Scaled-Tech-Consulting / Gitea-Tools, reviewed at head f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9. B1 (maintenance_drain.py:59-77 vs gitea_mcp_server.py:15493) — the drain safety allowlist is keyed on task names the mutation chokepoint never passes. Two entries (write_session_checkpoint, checkpoint_session) name no MCP task at all; seven name tools that make zero verify_preflight_purity calls; of the two that do reach the gate, only post_heartbeat matches its own literal, while gitea_heartbeat_reviewer_pr_lease calls verify_preflight_purity(remote, task="review_pr"), which is not on the list and is therefore deferred. A reviewer cannot heartbeat a PR lease during a drain, so the lease expires across the very restart the drain exists to make safe. Do not simply add review_pr to the allowlist: that literal is shared with review-verdict submission, which must stay deferred. Give the reviewer lease-lifecycle calls their own task literal and key the allowlist on it; drop or fix the entries that name ungated tools so the list stops implying coverage it does not have. B2 (gitea_mcp_server.py:2032-2035) — the gate catches every exception from _resolve and falls back to (org or ""), (repo or ""). For a call site that passes no org or repo, the drain lookup then queries scope ("", "") , finds no row, and permits the mutation while a drain is active. This fails open inside a gate whose docstring at lines 2022-2023 promises the opposite, and whose two neighboring failure paths both raise. Make scope-resolution failure raise as well. B3 (tests/test_maintenance_drain.py:43-54, and line 80) — the allowlist test feeds classify_mutation three of the allowlist's own constants, so it asserts only that the frozenset contains its own members and cannot fail while B1 is present. Replace it with a test driving the real task literal from the reviewer heartbeat path, asserting it is permitted during drain, paired with a negative asserting a review verdict under the same path is deferred. Add a regression test per blocker. ``` WHAT_HAPPENED: PR #907 was reviewed in full, first at head 77d808e7d4793c8f3b9f1b6e8f76f666221caed0 and re-verified at the current head f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9. This is its first review of any kind. The read covered maintenance_drain.py, the preflight gate and drain tools in gitea_mcp_server.py, the allocator assignment stop, the control-plane drain table, the capability map entries, and the test module. Each of the eleven allowlist entries was checked against the task literal its corresponding tool actually passes at the chokepoint, by locating each tool body and extracting its verify_preflight_purity calls rather than assuming the names lined up. That enumeration returned: two entries resolving to no function at all, seven with zero preflight calls, post_heartbeat matching its own literal, and heartbeat_reviewer_pr_lease passing review_pr. Three blockers and three lesser observations were found. WHY: B1 is the central one. AC3 promises that a defined set of quiesce operations keeps working while draining so in-flight sessions can finish and hand off. As written the allowlist cannot deliver that: most entries are strings the gate never receives, and the single entry that corresponds to a real gated call is refused because the tool passes a different literal. The observable consequence is a reviewer lease expiring during a drain that was entered specifically to avoid stranding it. B2 is a fail-open path in a gate that documents itself as fail-closed and whose sibling paths raise. B3 explains why B1 was not caught and is what needs to change for AC3 to be provable. ISSUE: #659 HEAD_SHA: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 REVIEW_STATUS: REQUEST_CHANGES MERGE_READY: false BLOCKERS: B1 drain safety allowlist keyed on task literals the chokepoint never passes, blocking the reviewer lease heartbeat during drain; B2 gate scope resolution fails open on _resolve exception inside a fail-closed gate; B3 allowlist test asserts the constant against itself and cannot detect B1. VALIDATION: Static review at head f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 against base branch master. The head advanced from 77d808e7d4793c8f3b9f1b6e8f76f666221caed0 after the first read: f0c6255d is a base-integration commit whose first parent is 77d808e7 and whose second parent is master at d7ad2838ec6f13a4a8009a800be9b715882e308e, carrying no author change to this feature. That was proved by diffing the two revisions over the files this verdict cites — maintenance_drain.py, tests/test_maintenance_drain.py, allocator_service.py, and control_plane_db.py are byte-identical across the advance (empty diffstat). The gitea_mcp_server.py hunks that arrived with the base integration do not touch the cited regions: the gate body still occupies lines 2022-2035 with the same allowlist check at 2029 and the same fail-open fallback at 2032-2035, and gitea_heartbeat_reviewer_pr_lease still calls verify_preflight_purity(remote, task="review_pr") at line 15493. Every line reference in this verdict was re-read at f0c6255d rather than carried forward. Evidence for B1 was gathered by extracting each named tool's function body and enumerating its verify_preflight_purity call sites and task literals, and by searching the whole tree for write_session_checkpoint and checkpoint_session. The PR reports no conflict against its base. Live head SHA before this verdict: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9. Author changes to the feature during validation: none. No test suite was executed at this head in this session and none is claimed; the author reports 11 passed in tests/test_maintenance_drain.py, and that result is consistent with these findings rather than in tension with them, since B3 shows the allowlist assertion is tautological and cannot fail while B1 is present. LAST_UPDATED_BY: sysadmin (prgs-reviewer) NATIVE_REVIEW_PROOF: gitea_submit_pr_review via native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, expected_head_sha f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9. No offline, import, or helper path was used. [THREAD STATE LEDGER] ### what is true now PR #907 is in open state at head f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9, targeting master, and reports no conflict against that base. Server-side decision state: this REQUEST_CHANGES verdict is the first review decision recorded on PR #907; the PR carried no prior review of any kind. Local verdict/state: REQUEST_CHANGES, three blockers, verified statically against the live head. ### what changed The review verdict moved from a locally-held result to a recorded server-side decision. The PR head advanced from 77d808e7 to f0c6255d between the first read and this verdict, by a base-integration commit only; the feature's own files are unchanged across that advance and every cited line was re-verified at f0c6255d. ### what is blocked Blocker classification: code blocker B1 — `maintenance_drain.py:59-77` allowlists eleven task names, but the gate at `gitea_mcp_server.py:2029` compares them against the literal passed to `verify_preflight_purity`. Two entries name nothing in the tree, seven name tools that never call the chokepoint, one (`post_heartbeat`) matches correctly, and `gitea_heartbeat_reviewer_pr_lease` reaches it as `review_pr` (line 15493) and is deferred, expiring reviewer leases across a drain. B2 — `gitea_mcp_server.py:2032-2035` degrades to scope `("", "")` on any `_resolve` exception and permits the mutation, contradicting the fail-closed contract stated at lines 2022-2023. B3 — `tests/test_maintenance_drain.py:43-54` asserts allowlist membership using the allowlist's own constants and cannot fail while B1 is present. ### who/what acts next Next actor: author Required action: Give the reviewer lease-lifecycle calls a task literal distinct from `review_pr` and key the allowlist on it; reconcile or drop the allowlist entries that name ungated tools or nonexistent symbols; make the gate's scope-resolution failure raise instead of defaulting to an empty scope; replace the tautological allowlist test with one driving real task literals, plus a regression test per blocker. Do not do: Do not resolve B1 by adding `review_pr` to the allowlist — that literal is shared with review-verdict submission, which must stay deferred during a drain, so that change would open a hole rather than close one. Do not treat the reported 11 passing tests as evidence against these findings; B3 is the claim that the relevant assertion cannot fail.

sysadmin referenced this pull request

2026-07-25 17:54:52 -05:00

Implement graceful MCP maintenance-drain mode #659

sysadmin commented

2026-07-25 17:55:05 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #907
issue: #659
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 41051-cb0c4d3bee9b
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr907-f0c6255
phase: released
candidate_head: f0c6255d7d
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T22:55:05Z
expires_at: 2026-07-25T23:05:05Z
blocker: manual-release

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #907 issue: #659 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 41051-cb0c4d3bee9b worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr907-f0c6255 phase: released candidate_head: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T22:55:05Z expires_at: 2026-07-25T23:05:05Z blocker: manual-release

sysadmin commented

2026-07-25 17:55:53 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #907
issue: #659
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 41051-cb0c4d3bee9b
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907
phase: claimed
candidate_head: f0c6255d7d
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T22:55:53Z
expires_at: 2026-07-25T23:05:53Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #907 issue: #659 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 41051-cb0c4d3bee9b worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907 phase: claimed candidate_head: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T22:55:53Z expires_at: 2026-07-25T23:05:53Z blocker: none

sysadmin commented

2026-07-25 17:56:40 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #907
issue: #659
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 41051-cb0c4d3bee9b
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907
phase: released
candidate_head: f0c6255d7d
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T22:56:39Z
expires_at: 2026-07-25T23:06:39Z
blocker: manual-release

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #907 issue: #659 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 41051-cb0c4d3bee9b worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/conflict-fix-pr-907 phase: released candidate_head: f0c6255d7d7efe7c1d37a4a450a47b0bc8fe0fa9 target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T22:56:39Z expires_at: 2026-07-25T23:06:39Z blocker: manual-release

You are not authorized to merge this pull request.

This pull request can be merged automatically.

This branch is out-of-date with the base branch

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feat/issue-659-maintenance-drain-mode:feat/issue-659-maintenance-drain-mode

git checkout feat/issue-659-maintenance-drain-mode

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Scaled-Tech-Consulting/Gitea-Tools#907