Implement pre-restart drain proof and hard gate #661

Closed
opened 2026-07-10 15:31:50 -05:00 by jcwalker3 · 1 comment
Owner

Problem statement

Even with drain mode, restart can proceed without a machine-verifiable proof that unsafe work is clear and checkpoints complete.

Operational impact

False “ready to restart” claims; data loss; dogfooding fails.

Scope

  • DrainProof artifact: signed/hashed checklist results (no in-flight mutations, assignments stopped, checkpoints complete, handoffs ok, leases handled, acks or explicit timeout policy).
  • Hard gate: coordinator apply_restart requires valid unexpired drain proof (except break-glass path).
  • Failed drain → durable incident issue + no restart.

Non-goals

  • Break-glass implementation (separate).
  • UI approval (console child).

Required implementation investigation

#655 #658 #659 #660; final_report style validators; crypto/hash for proof id.

Proposed implementation direction

DrainProof schema + verify_drain_proof() fail closed; wire into restart apply.

Security / workflow-safety

Proof non-forgeable within process; short TTL; audit.

Failure behavior

Missing/stale/failed proof → deny restart; create incident work item.

Acceptance criteria

  1. Restart apply without proof fails closed.
  2. Successful drain produces verifiable proof.
  3. Unsafe mutation open → proof fails.
  4. Tests for pass/fail/expired.
  5. Links #652 #653 #655.

Required tests

Gate unit tests; multi-session unsafe mutation case.

Observability and audit

Proof id in restart audit events.

Dependencies

Rollout

Enforce in dogfood after dry-run period.

Canonical issue state

STATE: ready-for-author
WHO_IS_NEXT: author
NEXT_ACTION: Implement drain proof + hard gate
NEXT_PROMPT: Author drain proof under #655; PR; stop

Required proof

Code + tests + sample proof JSON.

Required final response and handoff

Brief PR → reviewer.

## Problem statement Even with drain mode, restart can proceed without a machine-verifiable proof that unsafe work is clear and checkpoints complete. ## Operational impact False “ready to restart” claims; data loss; dogfooding fails. ## Scope * DrainProof artifact: signed/hashed checklist results (no in-flight mutations, assignments stopped, checkpoints complete, handoffs ok, leases handled, acks or explicit timeout policy). * Hard gate: coordinator apply_restart requires valid unexpired drain proof (except break-glass path). * Failed drain → durable incident issue + no restart. ## Non-goals * Break-glass implementation (separate). * UI approval (console child). ## Required implementation investigation #655 #658 #659 #660; final_report style validators; crypto/hash for proof id. ## Proposed implementation direction `DrainProof` schema + `verify_drain_proof()` fail closed; wire into restart apply. ## Security / workflow-safety Proof non-forgeable within process; short TTL; audit. ## Failure behavior Missing/stale/failed proof → deny restart; create incident work item. ## Acceptance criteria 1. Restart apply without proof fails closed. 2. Successful drain produces verifiable proof. 3. Unsafe mutation open → proof fails. 4. Tests for pass/fail/expired. 5. Links #652 #653 #655. ## Required tests Gate unit tests; multi-session unsafe mutation case. ## Observability and audit Proof id in restart audit events. ## Dependencies * Parent **#655** · Depends **#659** **#660** **#658** · Vision **#652** · Roadmap **#653** ## Rollout Enforce in dogfood after dry-run period. ## Canonical issue state ```text STATE: ready-for-author WHO_IS_NEXT: author NEXT_ACTION: Implement drain proof + hard gate NEXT_PROMPT: Author drain proof under #655; PR; stop ``` ## Required proof Code + tests + sample proof JSON. ## Required final response and handoff Brief PR → reviewer.
jcwalker3 added status:pr-open and removed status:ready labels 2026-07-24 16:19:15 -05:00
Owner

[THREAD STATE LEDGER]

What is true now

  • Server-side decision state: no reviewer verdict was recorded on PR #882 this run. The pre-existing review 576 (APPROVE, head 1cbbde00895dce1545290a660bc31c4ac9210b3e, not dismissed) is still the live server-side verdict, and approval_at_current_head is still true for that head.
  • Local verdict/state: reviewer prepared a REQUEST_CHANGES verdict for PR #882 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e and could not record it; it exists only in this comment.
  • PR #882 head reviewed: 1cbbde00895dce1545290a660bc31c4ac9210b3e (live head re-pinned before review; unchanged).
  • Base master: ccce corrected: ccde9e8f1183bc9a6672393a9923f304086fb874.
  • Blocker classification: process/rule blocker

What changed

  • Reviewer re-read PR #882 at the exact live head and executed a reproduction against an unmodified worktree pinned to that head (git status --porcelain empty and HEAD confirmed before and after).
  • A fail-open was reproduced in the acknowledgement checklist item (detail below).
  • Two attempts to record the terminal verdict were refused by the review-decision lock. No PR review mutation was performed. No local file in any worktree was modified.

What is blocked

The terminal REQUEST_CHANGES cannot be recorded on PR #882.

gitea_mark_final_review_decision(pr_number=882, action=request_changes, expected_head_sha=1cbbde00895dce1545290a660bc31c4ac9210b3e) returned marked_ready: false on both attempts with classification: foreign_pr_terminal, citing a terminal approve on PR #885 (review 580, head dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885).

gitea_diagnose_review_decision_lock(target_pr_number=882) simultaneously returned mark_final_allowed: true, recovery_allowed: true, recovery_mode: cross_pr_isolation, and the reason "last terminal is approve on foreign open/closed PR #885; target PR #882 is isolated (#693)".

Both tools prescribe retrying the identical call, so the prescribed recovery is a loop. It was attempted exactly twice and then stopped. final_review_decision_ready was not forced and gitea_authorize_review_correction was not misused as a cross-PR unlock. Every PR review path is gated behind this same flag — the non-terminal COMMENT path also refused, with "final_review_decision_ready must be true; validation-phase live review mutations are forbidden" — so this issue thread is the only channel available to the reviewer role.

Safety impact: because no terminal verdict could be recorded, PR #882 remains merge-eligible under approval 576 despite a reproduced fail-open. Treat this comment as a hard block on merging PR #882.

Who/what acts next

  • Next actor: author (jcwalker3), for PR #882 / issue #661.
  • Required action: fix the acknowledgement fail-open in drain_proof.py so absent, malformed, stale, or unproven acknowledgement data fails closed; add the absent-key regression test; push a new head on feat/issue-661-drain-proof-hard-gate. The new head will invalidate approval 576 and force a fresh review, which also clears this deadlock.
  • Do not do: do not merge PR #882 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e; do not treat review 576 as a current reviewer verdict; do not delete session-state files; do not call gitea_authorize_review_correction as a cross-PR unlock; do not rebase or force-push the branch.

Blocker A1 — acknowledgement evaluation fails open when acknowledgement data is absent

drain_proof.py, _evaluate_checks, check 6 (acks_or_timeout):

acks = drain_state.get("acks") or {}
ack_values = list(acks.values()) if isinstance(acks, Mapping) else []
all_acked = bool(ack_values) and all(...)
no_sessions_to_ack = isinstance(acks, Mapping) and len(ack_values) == 0
timeout_policy = _bool_input(drain_state.get("ack_timeout_policy_applied"))
acks_ok = all_acked or no_sessions_to_ack or timeout_policy

When the acknowledgement key is absent, drain_state.get("acks") or {} collapses to {}. The empty mapping satisfies no_sessions_to_ack, so acks_ok becomes True and the check reports "no other live sessions required to acknowledge".

That conclusion is reached without consulting the impact report. The report's counts.sessions_live_other is never read by this check, so "no acknowledgement data exists" and "it was proven that nobody needed to acknowledge" collapse into a single state. The first is absence of evidence; the second is evidence of absence.

This is the only checklist item in the file that does not fail closed. Check 1 refuses on inventory_complete=False; checks 2, 3 and 4 refuse via _bool_input on unknown values; check 5 refuses when lease disposition is unasserted. Check 6 is the exception, and it is the item gating whether live sessions are interrupted by a restart.

Reproduction at the exact head

Impact report asserts three other live sessions. Drain state asserts every other outcome true and omits both acks and ack_timeout_policy_applied.

sessions_live_other          = 3
acknowledgement key present  = False
acks_or_timeout.passed       = True
acks_or_timeout.detail       = no other live sessions required to acknowledge
proof.clean                  = True
proof.failed_checks          = []
gate verdict                 = allow
gate allow                   = True
gate reasons                 = ['valid unexpired clean drain proof present; restart authorized']

A restart that would interrupt three live sessions is authorized by a proof carrying zero acknowledgement evidence. Because the proof mints clean, it also passes verify_drain_proof and gate_apply_restart downstream with a valid signature: the gate does not merely fail to notice, it mints a positively-signed clean artifact attesting to a drain that was never demonstrated.

impact_fingerprint() does cover counts.sessions_live_other, but binding only detects a changed report. Nothing compares the live-session count against the acknowledgement evidence, so the contradiction between "3 live sessions" and "no acknowledgements" is never evaluated.

Required invariant

Missing, malformed, stale, or unproven acknowledgement data must fail closed; only explicitly verified acknowledgement data may permit the operation.

acks_or_timeout must pass only when one of the following is affirmatively demonstrated:

  1. the acknowledgement mapping is present and every affected live session acknowledged; or
  2. the impact report proves there were no other live sessions to acknowledge (counts.sessions_live_other == 0), rather than inferring it from an empty or absent mapping; or
  3. an explicit acknowledgement timeout policy was applied and recorded.

An absent key, a non-mapping value, a mapping whose keys do not cover the live sessions named by the report, or any unparseable acknowledgement value must all produce passed=False.

Blocker A2 — no test covers the absent-key path

tests/test_drain_proof.py has 25 cases and exercises each failing checklist item, but every acknowledgement case supplies an acks mapping or sets the timeout policy. No case omits the key while the report shows live sessions, which is why A1 passed a green suite and two prior reviews. A regression test asserting passed=False and proof.clean=False for sessions_live_other > 0 with the acknowledgement key absent is required alongside the fix.

Not in dispute

The remainder of the change is sound and needs no rework: the keyed HMAC-SHA256 proof id over a canonical body, the per-process secret defeating cross-restart replay, the 120s TTL, recomputing cleanliness rather than trusting a presented clean flag, deriving checks 1 and 5 from the impact report instead of caller self-report, and the non-silent break-glass bypass are all correctly built. A1 is a single inverted default inside an otherwise fail-closed design.

Canonical Issue State

STATE:
changes-requested

WHO_IS_NEXT:
author

NEXT_ACTION:
Fix the acks_or_timeout fail-open in drain_proof.py so absent, malformed, stale, or unproven acknowledgement data fails closed, add the absent-key regression test, and push a new head on feat/issue-661-drain-proof-hard-gate.

NEXT_PROMPT:

Author task: PR #882 / issue #661 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e. Fix drain_proof.py check 6 (acks_or_timeout): an absent or non-mapping "acks" key currently collapses to {} and passes as "no other live sessions required to acknowledge" without consulting the impact report, so a drain proof mints clean and gate_apply_restart returns verdict allow while counts.sessions_live_other = 3. Require affirmative evidence: pass only when (a) the acks mapping is present and covers every affected live session with an acknowledged value, or (b) the impact report itself proves counts.sessions_live_other == 0, or (c) an explicit ack timeout policy was applied. Absent key, non-mapping value, partial coverage, or unparseable values must set passed=False. Add a regression test in tests/test_drain_proof.py asserting passed=False and proof.clean=False when the acknowledgement key is absent and sessions_live_other > 0. remote=prgs org=Scaled-Tech-Consulting repo=Gitea-Tools.

WHAT_HAPPENED:
Reviewer re-reviewed PR #882 at head 1cbbde0089 and reproduced an acknowledgement fail-open: with counts.sessions_live_other = 3 and the acknowledgement key absent, acks_or_timeout passed, the proof minted clean, and gate_apply_restart returned verdict allow. The terminal REQUEST_CHANGES could not be recorded because the review-decision lock refused twice with classification foreign_pr_terminal citing PR #885.

WHY:
drain_proof.py check 6 treats an absent acknowledgement mapping as proof that no acknowledgement was required, never consulting counts.sessions_live_other, making it the only checklist item in the file that does not fail closed.

RELATED_PRS:
#882 (this review, head 1cbbde0089); #885 (holder of the foreign terminal decision that refused the lock, review 580 at head dac40ab9b3)

BLOCKERS:
A1 acknowledgement fail-open in drain_proof.py acks_or_timeout, reproduced at the exact head. A2 no regression test covers the absent-key path. Blocker classification for this thread: process/rule blocker — the terminal reviewer verdict could not be recorded because of the cross-PR review-decision lock described above.

VALIDATION:
Executable reproduction against an unmodified worktree pinned to 1cbbde0089, HEAD and clean status confirmed before and after: sessions_live_other=3, acknowledgement key absent, acks_or_timeout.passed=True, detail "no other live sessions required to acknowledge", proof.clean=True, failed_checks=[], gate verdict allow, allow=True. No test suite was re-run for this finding; the defect is demonstrated directly against the shipped module at the pinned head.

LAST_UPDATED_BY:
sysadmin / prgs-reviewer / 2026-07-25

[THREAD STATE LEDGER] ## What is true now - Server-side decision state: no reviewer verdict was recorded on PR #882 this run. The pre-existing review `576` (APPROVE, head `1cbbde00895dce1545290a660bc31c4ac9210b3e`, not dismissed) is still the live server-side verdict, and `approval_at_current_head` is still true for that head. - Local verdict/state: reviewer prepared a REQUEST_CHANGES verdict for PR #882 at head `1cbbde00895dce1545290a660bc31c4ac9210b3e` and could not record it; it exists only in this comment. - PR #882 head reviewed: `1cbbde00895dce1545290a660bc31c4ac9210b3e` (live head re-pinned before review; unchanged). - Base master: `ccce` corrected: `ccde9e8f1183bc9a6672393a9923f304086fb874`. - Blocker classification: process/rule blocker ## What changed - Reviewer re-read PR #882 at the exact live head and executed a reproduction against an unmodified worktree pinned to that head (`git status --porcelain` empty and HEAD confirmed before and after). - A fail-open was reproduced in the acknowledgement checklist item (detail below). - Two attempts to record the terminal verdict were refused by the review-decision lock. No PR review mutation was performed. No local file in any worktree was modified. ## What is blocked The terminal REQUEST_CHANGES cannot be recorded on PR #882. `gitea_mark_final_review_decision(pr_number=882, action=request_changes, expected_head_sha=1cbbde00895dce1545290a660bc31c4ac9210b3e)` returned `marked_ready: false` on both attempts with `classification: foreign_pr_terminal`, citing a terminal approve on **PR #885** (review `580`, head `dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885`). `gitea_diagnose_review_decision_lock(target_pr_number=882)` simultaneously returned `mark_final_allowed: true`, `recovery_allowed: true`, `recovery_mode: cross_pr_isolation`, and the reason `"last terminal is approve on foreign open/closed PR #885; target PR #882 is isolated (#693)"`. Both tools prescribe retrying the identical call, so the prescribed recovery is a loop. It was attempted exactly twice and then stopped. `final_review_decision_ready` was not forced and `gitea_authorize_review_correction` was not misused as a cross-PR unlock. Every PR review path is gated behind this same flag — the non-terminal COMMENT path also refused, with `"final_review_decision_ready must be true; validation-phase live review mutations are forbidden"` — so this issue thread is the only channel available to the reviewer role. **Safety impact:** because no terminal verdict could be recorded, PR #882 remains merge-eligible under approval `576` despite a reproduced fail-open. Treat this comment as a hard block on merging PR #882. ## Who/what acts next - Next actor: author (jcwalker3), for PR #882 / issue #661. - Required action: fix the acknowledgement fail-open in `drain_proof.py` so absent, malformed, stale, or unproven acknowledgement data fails closed; add the absent-key regression test; push a new head on `feat/issue-661-drain-proof-hard-gate`. The new head will invalidate approval `576` and force a fresh review, which also clears this deadlock. - Do not do: do not merge PR #882 at head `1cbbde00895dce1545290a660bc31c4ac9210b3e`; do not treat review `576` as a current reviewer verdict; do not delete session-state files; do not call `gitea_authorize_review_correction` as a cross-PR unlock; do not rebase or force-push the branch. --- ## Blocker A1 — acknowledgement evaluation fails open when acknowledgement data is absent `drain_proof.py`, `_evaluate_checks`, check 6 (`acks_or_timeout`): ```python acks = drain_state.get("acks") or {} ack_values = list(acks.values()) if isinstance(acks, Mapping) else [] all_acked = bool(ack_values) and all(...) no_sessions_to_ack = isinstance(acks, Mapping) and len(ack_values) == 0 timeout_policy = _bool_input(drain_state.get("ack_timeout_policy_applied")) acks_ok = all_acked or no_sessions_to_ack or timeout_policy ``` When the acknowledgement key is absent, `drain_state.get("acks") or {}` collapses to `{}`. The empty mapping satisfies `no_sessions_to_ack`, so `acks_ok` becomes `True` and the check reports `"no other live sessions required to acknowledge"`. That conclusion is reached without consulting the impact report. The report's `counts.sessions_live_other` is never read by this check, so "no acknowledgement data exists" and "it was proven that nobody needed to acknowledge" collapse into a single state. The first is absence of evidence; the second is evidence of absence. This is the only checklist item in the file that does not fail closed. Check 1 refuses on `inventory_complete=False`; checks 2, 3 and 4 refuse via `_bool_input` on unknown values; check 5 refuses when lease disposition is unasserted. Check 6 is the exception, and it is the item gating whether live sessions are interrupted by a restart. ### Reproduction at the exact head Impact report asserts three other live sessions. Drain state asserts every other outcome true and omits both `acks` and `ack_timeout_policy_applied`. ``` sessions_live_other = 3 acknowledgement key present = False acks_or_timeout.passed = True acks_or_timeout.detail = no other live sessions required to acknowledge proof.clean = True proof.failed_checks = [] gate verdict = allow gate allow = True gate reasons = ['valid unexpired clean drain proof present; restart authorized'] ``` A restart that would interrupt three live sessions is authorized by a proof carrying zero acknowledgement evidence. Because the proof mints `clean`, it also passes `verify_drain_proof` and `gate_apply_restart` downstream with a valid signature: the gate does not merely fail to notice, it mints a positively-signed clean artifact attesting to a drain that was never demonstrated. `impact_fingerprint()` does cover `counts.sessions_live_other`, but binding only detects a *changed* report. Nothing compares the live-session count against the acknowledgement evidence, so the contradiction between "3 live sessions" and "no acknowledgements" is never evaluated. ### Required invariant Missing, malformed, stale, or unproven acknowledgement data must fail closed; only explicitly verified acknowledgement data may permit the operation. `acks_or_timeout` must pass only when one of the following is affirmatively demonstrated: 1. the acknowledgement mapping is present and every affected live session acknowledged; or 2. the impact report proves there were no other live sessions to acknowledge (`counts.sessions_live_other == 0`), rather than inferring it from an empty or absent mapping; or 3. an explicit acknowledgement timeout policy was applied and recorded. An absent key, a non-mapping value, a mapping whose keys do not cover the live sessions named by the report, or any unparseable acknowledgement value must all produce `passed=False`. ## Blocker A2 — no test covers the absent-key path `tests/test_drain_proof.py` has 25 cases and exercises each failing checklist item, but every acknowledgement case supplies an `acks` mapping or sets the timeout policy. No case omits the key while the report shows live sessions, which is why A1 passed a green suite and two prior reviews. A regression test asserting `passed=False` and `proof.clean=False` for `sessions_live_other > 0` with the acknowledgement key absent is required alongside the fix. ## Not in dispute The remainder of the change is sound and needs no rework: the keyed HMAC-SHA256 proof id over a canonical body, the per-process secret defeating cross-restart replay, the 120s TTL, recomputing cleanliness rather than trusting a presented `clean` flag, deriving checks 1 and 5 from the impact report instead of caller self-report, and the non-silent break-glass bypass are all correctly built. A1 is a single inverted default inside an otherwise fail-closed design. ## Canonical Issue State STATE: changes-requested WHO_IS_NEXT: author NEXT_ACTION: Fix the acks_or_timeout fail-open in drain_proof.py so absent, malformed, stale, or unproven acknowledgement data fails closed, add the absent-key regression test, and push a new head on feat/issue-661-drain-proof-hard-gate. NEXT_PROMPT: ```text Author task: PR #882 / issue #661 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e. Fix drain_proof.py check 6 (acks_or_timeout): an absent or non-mapping "acks" key currently collapses to {} and passes as "no other live sessions required to acknowledge" without consulting the impact report, so a drain proof mints clean and gate_apply_restart returns verdict allow while counts.sessions_live_other = 3. Require affirmative evidence: pass only when (a) the acks mapping is present and covers every affected live session with an acknowledged value, or (b) the impact report itself proves counts.sessions_live_other == 0, or (c) an explicit ack timeout policy was applied. Absent key, non-mapping value, partial coverage, or unparseable values must set passed=False. Add a regression test in tests/test_drain_proof.py asserting passed=False and proof.clean=False when the acknowledgement key is absent and sessions_live_other > 0. remote=prgs org=Scaled-Tech-Consulting repo=Gitea-Tools. ``` WHAT_HAPPENED: Reviewer re-reviewed PR #882 at head 1cbbde00895dce1545290a660bc31c4ac9210b3e and reproduced an acknowledgement fail-open: with counts.sessions_live_other = 3 and the acknowledgement key absent, acks_or_timeout passed, the proof minted clean, and gate_apply_restart returned verdict allow. The terminal REQUEST_CHANGES could not be recorded because the review-decision lock refused twice with classification foreign_pr_terminal citing PR #885. WHY: drain_proof.py check 6 treats an absent acknowledgement mapping as proof that no acknowledgement was required, never consulting counts.sessions_live_other, making it the only checklist item in the file that does not fail closed. RELATED_PRS: #882 (this review, head 1cbbde00895dce1545290a660bc31c4ac9210b3e); #885 (holder of the foreign terminal decision that refused the lock, review 580 at head dac40ab9b3ef13255c4fe0f6353e6d93e0d0d885) BLOCKERS: A1 acknowledgement fail-open in drain_proof.py acks_or_timeout, reproduced at the exact head. A2 no regression test covers the absent-key path. Blocker classification for this thread: process/rule blocker — the terminal reviewer verdict could not be recorded because of the cross-PR review-decision lock described above. VALIDATION: Executable reproduction against an unmodified worktree pinned to 1cbbde00895dce1545290a660bc31c4ac9210b3e, HEAD and clean status confirmed before and after: sessions_live_other=3, acknowledgement key absent, acks_or_timeout.passed=True, detail "no other live sessions required to acknowledge", proof.clean=True, failed_checks=[], gate verdict allow, allow=True. No test suite was re-run for this finding; the defect is demonstrated directly against the shipped module at the pinned head. LAST_UPDATED_BY: sysadmin / prgs-reviewer / 2026-07-25
sysadmin removed the status:pr-open label 2026-07-24 23:40:04 -05:00
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#661