feat(restart): audit lifecycle events and durable incidents (Closes #665) #909

2026-07-25T16:11:01-05:00

jcwalker3 commented

2026-07-25 16:11:01 -05:00

Summary

Implements #665 under the restart-governance umbrella (#655): structured forensic trail for MCP restart evaluation so failed drains and break-glass cannot be silently repeated.

Changes

restart_audit.py — mcp.restart.* event schema, redacted payload builder, append-only emission via gitea_audit, correlation ids (rst-…), incident descriptors, and injected materializer for Gitea issues.
gitea_request_mcp_restart — mints a correlation id; always audits impact previews; on apply, audits apply-gate / break-glass and materializes durable incidents for failed drain and break-glass; privileged apply fails closed when audit is enabled but the sink write fails.
docs/mcp-restart-audit.md + coordinator cross-link.
tests/test_restart_audit.py — schema, redaction, emission, deny policy, incident mocks (22 tests).

Acceptance map

Lifecycle event vocabulary + emission path for impact preview / apply gate / break-glass (other event names reserved for drain/reconcile paths).
Failed drain → durable incident descriptor + issue create path.
Break-glass always → incident.
Redaction + emission tests.
Links parent #655, vision #652, roadmap #653.

Non-goals (unchanged)

Does not restart processes.
Does not re-verify drain proofs (#661).
Full Sentry UI (#649) / metrics dashboards (#651).

Proof

pytest tests/test_restart_audit.py -q
# 22 passed

Test plan

Unit tests for schema, redaction, privileged deny-on-audit-fail, break-glass / failed-drain incidents
Reviewer: dry-run gitea_request_mcp_restart shows correlation_id + restart_audit
Reviewer: apply deny without proof materializes failed-drain incident when create path available

Closes #665
Also #655 #658 #661 #663 #664 #652 #653

## Summary Implements **#665** under the restart-governance umbrella (**#655**): structured forensic trail for MCP restart evaluation so failed drains and break-glass cannot be silently repeated. ### Changes * **`restart_audit.py`** — `mcp.restart.*` event schema, redacted payload builder, append-only emission via `gitea_audit`, correlation ids (`rst-…`), incident descriptors, and injected materializer for Gitea issues. * **`gitea_request_mcp_restart`** — mints a correlation id; always audits impact previews; on apply, audits apply-gate / break-glass and materializes durable incidents for failed drain and break-glass; privileged apply fails closed when audit is enabled but the sink write fails. * **`docs/mcp-restart-audit.md`** + coordinator cross-link. * **`tests/test_restart_audit.py`** — schema, redaction, emission, deny policy, incident mocks (22 tests). ### Acceptance map 1. Lifecycle event vocabulary + emission path for impact preview / apply gate / break-glass (other event names reserved for drain/reconcile paths). 2. Failed drain → durable incident descriptor + issue create path. 3. Break-glass always → incident. 4. Redaction + emission tests. 5. Links parent **#655**, vision **#652**, roadmap **#653**. ### Non-goals (unchanged) * Does not restart processes. * Does not re-verify drain proofs (#661). * Full Sentry UI (#649) / metrics dashboards (#651). ### Proof ``` pytest tests/test_restart_audit.py -q # 22 passed ``` ## Test plan - [x] Unit tests for schema, redaction, privileged deny-on-audit-fail, break-glass / failed-drain incidents - [ ] Reviewer: dry-run `gitea_request_mcp_restart` shows `correlation_id` + `restart_audit` - [ ] Reviewer: apply deny without proof materializes failed-drain incident when create path available Closes #665 Also #655 #658 #661 #663 #664 #652 #653

jcwalker3 added 1 commit 2026-07-25 16:11:01 -05:00

feat(restart): audit lifecycle events and durable incidents (#665 ) 22e0a41bd5

Add restart_audit with mcp.restart.* event schema, redacted emission via
gitea_audit, correlation ids, and incident materialization for failed drain
and break-glass. Wire gitea_request_mcp_restart to always audit impact
previews and fail closed on privileged apply when the audit sink is enabled
but write fails.

Closes #665

sysadmin commented

2026-07-25 18:06:53 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #909
issue: #665
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 81615-94558e39a724
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909-restart-audit
phase: claimed
candidate_head: 22e0a41bd5
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T23:06:53Z
expires_at: 2026-07-25T23:16:53Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #909 issue: #665 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 81615-94558e39a724 worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909-restart-audit phase: claimed candidate_head: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T23:06:53Z expires_at: 2026-07-25T23:16:53Z blocker: none

sysadmin commented

2026-07-25 18:07:53 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #909
issue: #665
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 81615-94558e39a724
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909-restart-audit
phase: validated
candidate_head: 22e0a41bd5
target_branch: master
target_branch_sha: 2b4e43042a
last_activity: 2026-07-25T23:07:52Z
expires_at: 2026-07-25T23:17:52Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #909 issue: #665 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 81615-94558e39a724 worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909-restart-audit phase: validated candidate_head: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb target_branch: master target_branch_sha: 2b4e43042a34f4e29617378ae79a7f5a3d312688 last_activity: 2026-07-25T23:07:52Z expires_at: 2026-07-25T23:17:52Z blocker: none

sysadmin requested changes 2026-07-25 18:09:42 -05:00

sysadmin left a comment

Canonical PR State

STATE: changes_requested
WHO_IS_NEXT: author
NEXT_ACTION: fix incident kind overwrite so #661 kind restart_drain_gate_denied is preserved; re-run #886 + restart_audit tests; push new head for re-review
NEXT_PROMPT:

AUTHOR session (prgs-author / gitea-author) on prgs/Scaled-Tech-Consulting/Gitea-Tools:
Work issue #665 / PR #909. Preserve drain-gate incident kind restart_drain_gate_denied when #665 audit materializes failed-drain incidents (do not overwrite gate.incident.kind with restart_failed_drain). Keep tests green for test_issue_886 and test_restart_audit. Push and hand off for re-review at the new head.

WHAT_HAPPENED: Independent reviewer sysadmin validated PR #909 at 22e0a41. Restart audit unit tests pass; #886 regression fails only on this head (passes on master).
WHY: gitea_request_mcp_restart overwrites payload["incident"] from drain_proof.gate.incident (kind=restart_drain_gate_denied) with restart_audit.incident_from_apply_gate (kind=restart_failed_drain), breaking the established #661/#886 contract.
ISSUE: #665
HEAD_SHA: 22e0a41bd5
REVIEW_STATUS: REQUEST_CHANGES
MERGE_READY: no
BLOCKERS: incident kind restart_failed_drain overwrites restart_drain_gate_denied; test_issue_886 TestExistingPathsStillWork.test_apply_still_denies_without_any_proof fails on PR head only
VALIDATION: pytest tests/test_restart_audit.py -q → 22 passed; pytest tests/test_issue_886_apply_authorization_conjunction.py -q → 1 failed on PR head / passes on master 2b4e430; worktree branches/review-pr-909-restart-audit clean at pinned head
LAST_UPDATED_BY: sysadmin / prgs-reviewer

Review detail

Author safety: reviewer sysadmin != author jcwalker3.

Positive: #665 audit module, redaction, correlation ids, no process restart, docs.

Required fix: preserve #661 incident kind on drain-gate deny path (or dual-support with intentional migration + test updates).

## Canonical PR State STATE: changes_requested WHO_IS_NEXT: author NEXT_ACTION: fix incident kind overwrite so #661 kind restart_drain_gate_denied is preserved; re-run #886 + restart_audit tests; push new head for re-review NEXT_PROMPT: ```text AUTHOR session (prgs-author / gitea-author) on prgs/Scaled-Tech-Consulting/Gitea-Tools: Work issue #665 / PR #909. Preserve drain-gate incident kind restart_drain_gate_denied when #665 audit materializes failed-drain incidents (do not overwrite gate.incident.kind with restart_failed_drain). Keep tests green for test_issue_886 and test_restart_audit. Push and hand off for re-review at the new head. ``` WHAT_HAPPENED: Independent reviewer sysadmin validated PR #909 at 22e0a41. Restart audit unit tests pass; #886 regression fails only on this head (passes on master). WHY: gitea_request_mcp_restart overwrites payload["incident"] from drain_proof.gate.incident (kind=restart_drain_gate_denied) with restart_audit.incident_from_apply_gate (kind=restart_failed_drain), breaking the established #661/#886 contract. ISSUE: #665 HEAD_SHA: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb REVIEW_STATUS: REQUEST_CHANGES MERGE_READY: no BLOCKERS: incident kind restart_failed_drain overwrites restart_drain_gate_denied; test_issue_886 TestExistingPathsStillWork.test_apply_still_denies_without_any_proof fails on PR head only VALIDATION: pytest tests/test_restart_audit.py -q → 22 passed; pytest tests/test_issue_886_apply_authorization_conjunction.py -q → 1 failed on PR head / passes on master 2b4e430; worktree branches/review-pr-909-restart-audit clean at pinned head LAST_UPDATED_BY: sysadmin / prgs-reviewer ## Review detail Author safety: reviewer sysadmin != author jcwalker3. Positive: #665 audit module, redaction, correlation ids, no process restart, docs. Required fix: preserve #661 incident kind on drain-gate deny path (or dual-support with intentional migration + test updates).

sysadmin referenced this pull request

2026-07-25 18:11:32 -05:00

feat(mcp): implement emergency break-glass MCP restart workflow (#664) #908

sysadmin referenced this pull request

2026-07-25 18:21:49 -05:00

feat(mcp): implement emergency break-glass MCP restart workflow (#664) #908

sysadmin commented

2026-07-25 18:27:37 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #909
issue: #665
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 52249-5fe108392e0d
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909
phase: claimed
candidate_head: 22e0a41bd5
target_branch: master
target_branch_sha: none
last_activity: 2026-07-25T23:27:36Z
expires_at: 2026-07-25T23:37:36Z
blocker: none

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #909 issue: #665 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 52249-5fe108392e0d worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909 phase: claimed candidate_head: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb target_branch: master target_branch_sha: none last_activity: 2026-07-25T23:27:36Z expires_at: 2026-07-25T23:37:36Z blocker: none

sysadmin commented

2026-07-25 18:31:42 -05:00

Second-reviewer findings at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb. No new verdict is recorded by this comment — review 612 already carries an unresolved REQUEST_CHANGES at this same head, and gitea_mark_final_review_decision correctly refused a duplicate (#332). This adds two blockers that review 612 does not cover, plus an independent reproduction of its finding.

Reproduction of review 612's blocker

Confirmed at this head, in branches/review-pr-909:

tests/test_issue_886_apply_authorization_conjunction.py
E  AssertionError: 'restart_failed_drain' != 'restart_drain_gate_denied'
FAILED TestExistingPathsStillWork::test_apply_still_denies_without_any_proof
1 failed, 18 passed, 45 subtests passed

incident_from_apply_gate (restart_audit.py:398-413) rebuilds the gate's descriptor under its own INCIDENT_FAILED_DRAIN kind, and gitea_mcp_server.py then assigns it over payload["incident"], discarding the #661 kind. Review 612's account of this is accurate.

What the module gets right

restart_audit.py is the strongest piece of this series. The event vocabulary is a closed set and build_restart_event raises on anything outside it. Emission is append-only through gitea_audit rather than overwrite-by-key state — which is exactly what PR #908 should be calling instead of mcp_session_state.save_state. Reasons and details are redacted before both the audit write and the issue body, and require_audit_or_deny is genuinely fail-closed for privileged paths. Both blockers below are in the wiring, not the module.

F1 — a read-only preview now mutates session preflight state

gitea_mcp_server.py:22760, reached on every invocation including the dry-run preview:

auth_user = (gitea_whoami(remote=remote) or {}).get("username")

The value is used once, as the authenticated_username field of a best-effort audit event. gitea_whoami is not read-only with respect to session state: its first statement is record_preflight_check("whoami") (gitea_mcp_server.py:842), which has two session-wide effects.

First, it can clear the caller's resolved capability. At lines 860-875 the resolved-capability stamp survives an interleaved whoami only when preserve_capability holds — whoami already called earlier in the session, a capability already resolved, and no outstanding whoami violation. Otherwise the branch falls to _clear_preflight_capability_state() at line 875 and the resolve is gone.

Second, it recomputes the whoami violation flag against workspace porcelain at line 879: _preflight_whoami_violation = bool(whoami_delta), measured from process start. A session holding legitimate in-flight tracked edits acquires the violation from this call alone.

Both feed a fail-closed gate: verify_preflight_purity raises _PreflightOrderError on that flag at lines 1510-1514, and it also surfaces as a preflight_block_reason at line 762.

The concrete failure: an operator inspects the blast radius with the preview — whose payload sets read_only at line 22738 — and the next gated mutation is refused, either for a capability that was resolved or for a workspace edit that predates the preview. The tool meant to make a restart safer to look at becomes one you cannot look at mid-workflow.

Nothing about the audit needs this. _authenticated_username(host) (gitea_mcp_server.py:3533) returns the same login from the same endpoint, is cached, never raises, and touches no preflight state.

F2 — the durable incident cannot be created by any profile that reaches this path

AC2 and AC3 require a failed drain and a break-glass to materialize a durable Gitea issue. The materializer injected at gitea_mcp_server.py:22866-22877 forwards to gitea_create_issue, which is doubly gated: it requires task_capability_map.required_permission("create_issue") — gitea.issue.create (line 3862) — and it runs verify_preflight_purity(..., task="create_issue") (lines 3876-3879), so the session's resolved capability must be create_issue at that moment.

Neither condition holds here. In the live profile inventory only the two author profiles carry gitea.issue.create; prgs-controller, prgs-reconciler, prgs-merger and prgs-reviewer do not. And a session reaching a restart apply resolved the restart task, not create_issue, so even an author profile trips the preflight task check.

The outcome looks correct and is empty: materialize_incident catches the refusal at restart_audit.py:307-310, returns created: False with a reason, the payload still reports the deny, and no durable incident exists. AC2's "a failed drain cannot be silently repeated" is the property that does not hold — the only remaining trace is the audit line, which is itself off unless GITEA_AUDIT_LOG is configured.

This is the mirror image of PR #908's incident-permission finding. There the write reached for raw api_request and inherited no permission check; here it routes through the gated tool and is refused. Both need the same missing piece: a sanctioned incident-write path a privileged non-author caller may actually use. Failing that, the apply should fail loudly when no capable writer is injected rather than reporting a deny whose incident never landed.

Lesser observations

payload["restart_audit"] reports impact_preview_written: False for two different states — sink not configured, and sink configured but write failed. For a forensic feature that distinction is the point; surface gitea_audit.audit_enabled() alongside the written flags. The same conflation hides require_audit_or_deny's no-op for the unconfigured case (restart_audit.py:172-180), which docs/mcp-restart-audit.md:16,48 describes as intended rollout policy.
incident_from_apply_gate (restart_audit.py:414) raises a failed-drain incident only when drain_gate_allow is falsy, so a denial for another reason — an unauthorized restart class, say — leaves nothing durable behind. AC2 names failed drain specifically, so this is scope rather than defect, but the doc should say it.
Every test in tests/test_restart_audit.py exercises the module directly and none imports gitea_mcp_server, so neither F1 nor F2 is reachable by that suite. The PR's own test plan leaves both integration checkboxes unticked, consistent with that. The one suite that does exercise the wiring, test_issue_886, is the one review 612 found failing.
payload["success"] = True is reasserted inside the audit-deny branch at line 22923 where it is already true; harmless, but it reads as if the deny changed it.
build_restart_event mints a fallback correlation id at restart_audit.py:132 while record_restart_lifecycle requires one — unreachable from the wired path, and it would quietly break the one-id-per-lifecycle invariant for a future caller.

Canonical PR State

STATE: changes-requested
WHO_IS_NEXT: author
NEXT_ACTION: Preserve the #661 incident kind per review 612, replace the internal gitea_whoami call with _authenticated_username, and give the incident materializer a write path a privileged non-author caller can use.
NEXT_PROMPT:

AUTHOR session on prgs / Scaled-Tech-Consulting / Gitea-Tools: continue PR #909
(Closes #665) at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb. Review 612 stands;
these are additional findings from a second read at the same head.

Review 612's blocker, reproduced here: incident_from_apply_gate rebuilds the gate
descriptor under INCIDENT_FAILED_DRAIN and the server assigns it over
payload["incident"], so restart_drain_gate_denied is lost and
tests/test_issue_886_apply_authorization_conjunction.py fails on the PR head
(1 failed, 18 passed) while passing on master.

F1 (gitea_mcp_server.py:22760) — gitea_request_mcp_restart calls gitea_whoami on
every invocation, including the dry-run preview whose payload sets read_only at
line 22738. gitea_whoami begins with record_preflight_check("whoami") (line 842),
which clears the resolved-capability stamp via _clear_preflight_capability_state()
at line 875 when preserve_capability does not hold, and recomputes
_preflight_whoami_violation from workspace porcelain at line 879. Both feed
verify_preflight_purity, which raises _PreflightOrderError at lines 1510-1514. So
reading the impact preview can refuse the caller's next mutation. Use
_authenticated_username(h) (line 3533): same identity, same endpoint, cached, never
raises, no preflight side effects.

F2 (gitea_mcp_server.py:22866-22877) — the incident materializer forwards to
gitea_create_issue, which requires gitea.issue.create (line 3862) and
verify_preflight_purity(task="create_issue") (lines 3876-3879). Only the author
profiles hold that permission, and a session reaching a restart apply resolved the
restart task instead, so the call is refused, materialize_incident records
created=False (restart_audit.py:307-310), and the AC2/AC3 durable incident never
lands. Provide a sanctioned incident-write path usable by a privileged non-author
caller, or fail the apply loudly when no capable writer is injected.

Also surface gitea_audit.audit_enabled() in payload["restart_audit"], and add
integration tests that drive gitea_request_mcp_restart itself — the restart_audit
suite never imports the server, which is why F1 and F2 are invisible to it.

WHAT_HAPPENED: PR #909 was read in full at head 22e0a41bd5 against merge base 76f293eb28 — 975 insertions across restart_audit.py, gitea_mcp_server.py, two docs and one test module. restart_audit.py was read end to end, then each integration point in gitea_request_mcp_restart was traced into the function it calls: gitea_whoami into record_preflight_check and on into the preflight violation flags and their fail-closed consumer, and the injected incident writer into gitea_create_issue and its permission plus resolved-task gates. The live profile inventory was read to determine which profiles hold gitea.issue.create. Review 612's failing-test claim was reproduced locally. Two additional blockers and five lesser observations were found.
WHY: The module delivers what #665 promises — closed event vocabulary, append-only sink, redaction, privileged fail-closed rule. The wiring does not. F1 makes a read-only preview change session-scoped preflight state that other tools fail closed on, so an operator inspecting a restart can lose the ability to act, and it does so for an identity string a pure helper already provides. F2 leaves the one artifact AC2 exists to guarantee unreachable for every profile that can reach the apply path, turning a promised durable record into an audit line that is off by default.
ISSUE: #665
BASE: master
HEAD: feat/issue-665-restart-audit
HEAD_SHA: 22e0a41bd5
RELATED_PRS: #909
REVIEW_STATUS: REQUEST_CHANGES
MERGE_READY: no — review 612's blocker plus two further blockers in the server wiring
BLOCKERS: review 612's incident-kind overwrite, reproduced here as a failing test_issue_886 case on the PR head; F1 the read-only impact preview calls gitea_whoami, which clears the resolved-capability stamp and recomputes the whoami violation flag, both fail-closed inputs to verify_preflight_purity; F2 the AC2/AC3 durable incident is created through gitea_create_issue, whose permission and resolved-task requirements no caller on this path satisfies, so the incident never lands.
SUPERSEDES: none
SUPERSEDED_BY: none
VALIDATION: Static review at head 22e0a41bd5 against merge base 76f293eb28, in the bound worktree branches/review-pr-909, confirmed clean by git status --porcelain --untracked-files=all. Every cited line was read at this head. tests/test_issue_886_apply_authorization_conjunction.py was executed here: 1 failed, 18 passed, 45 subtests passed, the failure being 'restart_failed_drain' != 'restart_drain_gate_denied' — an independent reproduction of review 612's finding. F1 was traced through the call at 22760, record_preflight_check at 842-882 including the preserve_capability branch and the clear at 875, and the consumers at 762 and 1510-1514, with _authenticated_username at 3533 read to confirm the pure alternative. F2 rests on gitea_create_issue's permission constant at 3862 and its preflight task at 3876-3879, together with the live profile inventory showing gitea.issue.create only on the author profiles. The PR reports no conflict against its base. Live head SHA at the time of this comment: 22e0a41bd5. Author changes during the read: none.
LAST_UPDATED_BY: sysadmin (prgs-reviewer)

NATIVE_REVIEW_PROOF: findings posted via gitea_create_issue_comment on native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, at head 22e0a41bd5. No formal review verdict was recorded by this call. No offline, import, or helper path was used.

[THREAD STATE LEDGER]

what is true now

PR #909 is in open state at head 22e0a41bd5 against master, and reports no conflict against that base.

Server-side decision state: review 612 records an unresolved REQUEST_CHANGES at this head; this comment records no decision and adds findings only.
Local verdict/state: REQUEST_CHANGES concurrence, two additional blockers, plus a local reproduction of review 612's failing test.

what changed

Nothing on the PR itself. A second finding set now exists on the thread alongside review 612, and a reviewer lease was taken and will be released.

what is blocked

Blocker classification: code blocker

Recording a second formal verdict is refused by the duplicate-verdict rule (#332) while review 612 stands unresolved at this head; that is a process constraint, not a defect in this PR. The code blockers are review 612's incident-kind overwrite, reproduced as a failing test_issue_886 case; F1 at gitea_mcp_server.py:22760, where a read_only preview calls gitea_whoami and thereby clears the resolved-capability stamp (line 875) and recomputes _preflight_whoami_violation (line 879), both of which raise at verify_preflight_purity lines 1510-1514; and F2 at lines 22866-22877, where the AC2/AC3 incident is written through gitea_create_issue under gitea.issue.create (line 3862) and a resolved create_issue task (lines 3876-3879) that no caller on this path holds.

who/what acts next

Next actor: author
Required action: Preserve the #661 restart_drain_gate_denied kind on the drain-gate deny path and make test_issue_886 pass on the PR head; replace the internal gitea_whoami call with _authenticated_username(h); give the incident materializer a write path a privileged non-author caller can use, or fail the apply loudly when no capable writer is injected; surface gitea_audit.audit_enabled() in payload["restart_audit"]; and add tests that drive gitea_request_mcp_restart itself.
Do not do: Do not resolve F1 by reordering the internal whoami or by documenting the side effect — the preview is consumed mid-workflow by design, so any preflight mutation inside it harms callers who did nothing wrong. Do not resolve F2 by reaching for raw api_request to skip the permission gate; PR #908 does exactly that and it is a blocker there. Do not read the 22 passing restart_audit tests as evidence against F1 or F2 — that module never imports the server and cannot observe the wiring.

Second-reviewer findings at head `22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb`. **No new verdict is recorded by this comment** — review `612` already carries an unresolved REQUEST_CHANGES at this same head, and `gitea_mark_final_review_decision` correctly refused a duplicate (#332). This adds two blockers that review 612 does not cover, plus an independent reproduction of its finding. ## Reproduction of review 612's blocker Confirmed at this head, in `branches/review-pr-909`: ```text tests/test_issue_886_apply_authorization_conjunction.py E AssertionError: 'restart_failed_drain' != 'restart_drain_gate_denied' FAILED TestExistingPathsStillWork::test_apply_still_denies_without_any_proof 1 failed, 18 passed, 45 subtests passed ``` `incident_from_apply_gate` (`restart_audit.py:398-413`) rebuilds the gate's descriptor under its own `INCIDENT_FAILED_DRAIN` kind, and `gitea_mcp_server.py` then assigns it over `payload["incident"]`, discarding the #661 kind. Review 612's account of this is accurate. ## What the module gets right `restart_audit.py` is the strongest piece of this series. The event vocabulary is a closed set and `build_restart_event` raises on anything outside it. Emission is append-only through `gitea_audit` rather than overwrite-by-key state — which is exactly what PR #908 should be calling instead of `mcp_session_state.save_state`. Reasons and details are redacted before both the audit write and the issue body, and `require_audit_or_deny` is genuinely fail-closed for privileged paths. Both blockers below are in the wiring, not the module. ## F1 — a read-only preview now mutates session preflight state `gitea_mcp_server.py:22760`, reached on **every** invocation including the dry-run preview: ```python auth_user = (gitea_whoami(remote=remote) or {}).get("username") ``` The value is used once, as the `authenticated_username` field of a best-effort audit event. `gitea_whoami` is not read-only with respect to session state: its first statement is `record_preflight_check("whoami")` (`gitea_mcp_server.py:842`), which has two session-wide effects. First, it can clear the caller's resolved capability. At lines 860-875 the resolved-capability stamp survives an interleaved whoami only when `preserve_capability` holds — whoami already called earlier in the session, a capability already resolved, and no outstanding whoami violation. Otherwise the branch falls to `_clear_preflight_capability_state()` at line 875 and the resolve is gone. Second, it recomputes the whoami violation flag against workspace porcelain at line 879: `_preflight_whoami_violation = bool(whoami_delta)`, measured from process start. A session holding legitimate in-flight tracked edits acquires the violation from this call alone. Both feed a fail-closed gate: `verify_preflight_purity` raises `_PreflightOrderError` on that flag at lines 1510-1514, and it also surfaces as a `preflight_block_reason` at line 762. The concrete failure: an operator inspects the blast radius with the preview — whose payload sets `read_only` at line 22738 — and the next gated mutation is refused, either for a capability that was resolved or for a workspace edit that predates the preview. The tool meant to make a restart safer to look at becomes one you cannot look at mid-workflow. Nothing about the audit needs this. `_authenticated_username(host)` (`gitea_mcp_server.py:3533`) returns the same login from the same endpoint, is cached, never raises, and touches no preflight state. ## F2 — the durable incident cannot be created by any profile that reaches this path AC2 and AC3 require a failed drain and a break-glass to materialize a durable Gitea issue. The materializer injected at `gitea_mcp_server.py:22866-22877` forwards to `gitea_create_issue`, which is doubly gated: it requires `task_capability_map.required_permission("create_issue")` — `gitea.issue.create` (line 3862) — and it runs `verify_preflight_purity(..., task="create_issue")` (lines 3876-3879), so the session's resolved capability must be `create_issue` at that moment. Neither condition holds here. In the live profile inventory only the two author profiles carry `gitea.issue.create`; `prgs-controller`, `prgs-reconciler`, `prgs-merger` and `prgs-reviewer` do not. And a session reaching a restart apply resolved the restart task, not `create_issue`, so even an author profile trips the preflight task check. The outcome looks correct and is empty: `materialize_incident` catches the refusal at `restart_audit.py:307-310`, returns `created: False` with a reason, the payload still reports the deny, and no durable incident exists. AC2's "a failed drain cannot be silently repeated" is the property that does not hold — the only remaining trace is the audit line, which is itself off unless `GITEA_AUDIT_LOG` is configured. This is the mirror image of PR #908's incident-permission finding. There the write reached for raw `api_request` and inherited no permission check; here it routes through the gated tool and is refused. Both need the same missing piece: a sanctioned incident-write path a privileged non-author caller may actually use. Failing that, the apply should fail loudly when no capable writer is injected rather than reporting a deny whose incident never landed. ## Lesser observations - `payload["restart_audit"]` reports `impact_preview_written: False` for two different states — sink not configured, and sink configured but write failed. For a forensic feature that distinction is the point; surface `gitea_audit.audit_enabled()` alongside the written flags. The same conflation hides `require_audit_or_deny`'s no-op for the unconfigured case (`restart_audit.py:172-180`), which `docs/mcp-restart-audit.md:16,48` describes as intended rollout policy. - `incident_from_apply_gate` (`restart_audit.py:414`) raises a failed-drain incident only when `drain_gate_allow` is falsy, so a denial for another reason — an unauthorized restart class, say — leaves nothing durable behind. AC2 names failed drain specifically, so this is scope rather than defect, but the doc should say it. - Every test in `tests/test_restart_audit.py` exercises the module directly and none imports `gitea_mcp_server`, so neither F1 nor F2 is reachable by that suite. The PR's own test plan leaves both integration checkboxes unticked, consistent with that. The one suite that does exercise the wiring, `test_issue_886`, is the one review 612 found failing. - `payload["success"] = True` is reasserted inside the audit-deny branch at line 22923 where it is already true; harmless, but it reads as if the deny changed it. - `build_restart_event` mints a fallback correlation id at `restart_audit.py:132` while `record_restart_lifecycle` requires one — unreachable from the wired path, and it would quietly break the one-id-per-lifecycle invariant for a future caller. ## Canonical PR State STATE: changes-requested WHO_IS_NEXT: author NEXT_ACTION: Preserve the #661 incident kind per review 612, replace the internal gitea_whoami call with _authenticated_username, and give the incident materializer a write path a privileged non-author caller can use. NEXT_PROMPT: ```text AUTHOR session on prgs / Scaled-Tech-Consulting / Gitea-Tools: continue PR #909 (Closes #665) at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb. Review 612 stands; these are additional findings from a second read at the same head. Review 612's blocker, reproduced here: incident_from_apply_gate rebuilds the gate descriptor under INCIDENT_FAILED_DRAIN and the server assigns it over payload["incident"], so restart_drain_gate_denied is lost and tests/test_issue_886_apply_authorization_conjunction.py fails on the PR head (1 failed, 18 passed) while passing on master. F1 (gitea_mcp_server.py:22760) — gitea_request_mcp_restart calls gitea_whoami on every invocation, including the dry-run preview whose payload sets read_only at line 22738. gitea_whoami begins with record_preflight_check("whoami") (line 842), which clears the resolved-capability stamp via _clear_preflight_capability_state() at line 875 when preserve_capability does not hold, and recomputes _preflight_whoami_violation from workspace porcelain at line 879. Both feed verify_preflight_purity, which raises _PreflightOrderError at lines 1510-1514. So reading the impact preview can refuse the caller's next mutation. Use _authenticated_username(h) (line 3533): same identity, same endpoint, cached, never raises, no preflight side effects. F2 (gitea_mcp_server.py:22866-22877) — the incident materializer forwards to gitea_create_issue, which requires gitea.issue.create (line 3862) and verify_preflight_purity(task="create_issue") (lines 3876-3879). Only the author profiles hold that permission, and a session reaching a restart apply resolved the restart task instead, so the call is refused, materialize_incident records created=False (restart_audit.py:307-310), and the AC2/AC3 durable incident never lands. Provide a sanctioned incident-write path usable by a privileged non-author caller, or fail the apply loudly when no capable writer is injected. Also surface gitea_audit.audit_enabled() in payload["restart_audit"], and add integration tests that drive gitea_request_mcp_restart itself — the restart_audit suite never imports the server, which is why F1 and F2 are invisible to it. ``` WHAT_HAPPENED: PR #909 was read in full at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb against merge base 76f293eb288fa6cbb3134e093fc960ac01517e75 — 975 insertions across restart_audit.py, gitea_mcp_server.py, two docs and one test module. restart_audit.py was read end to end, then each integration point in gitea_request_mcp_restart was traced into the function it calls: gitea_whoami into record_preflight_check and on into the preflight violation flags and their fail-closed consumer, and the injected incident writer into gitea_create_issue and its permission plus resolved-task gates. The live profile inventory was read to determine which profiles hold gitea.issue.create. Review 612's failing-test claim was reproduced locally. Two additional blockers and five lesser observations were found. WHY: The module delivers what #665 promises — closed event vocabulary, append-only sink, redaction, privileged fail-closed rule. The wiring does not. F1 makes a read-only preview change session-scoped preflight state that other tools fail closed on, so an operator inspecting a restart can lose the ability to act, and it does so for an identity string a pure helper already provides. F2 leaves the one artifact AC2 exists to guarantee unreachable for every profile that can reach the apply path, turning a promised durable record into an audit line that is off by default. ISSUE: #665 BASE: master HEAD: feat/issue-665-restart-audit HEAD_SHA: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb RELATED_PRS: #909 REVIEW_STATUS: REQUEST_CHANGES MERGE_READY: no — review 612's blocker plus two further blockers in the server wiring BLOCKERS: review 612's incident-kind overwrite, reproduced here as a failing test_issue_886 case on the PR head; F1 the read-only impact preview calls gitea_whoami, which clears the resolved-capability stamp and recomputes the whoami violation flag, both fail-closed inputs to verify_preflight_purity; F2 the AC2/AC3 durable incident is created through gitea_create_issue, whose permission and resolved-task requirements no caller on this path satisfies, so the incident never lands. SUPERSEDES: none SUPERSEDED_BY: none VALIDATION: Static review at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb against merge base 76f293eb288fa6cbb3134e093fc960ac01517e75, in the bound worktree branches/review-pr-909, confirmed clean by git status --porcelain --untracked-files=all. Every cited line was read at this head. tests/test_issue_886_apply_authorization_conjunction.py was executed here: 1 failed, 18 passed, 45 subtests passed, the failure being 'restart_failed_drain' != 'restart_drain_gate_denied' — an independent reproduction of review 612's finding. F1 was traced through the call at 22760, record_preflight_check at 842-882 including the preserve_capability branch and the clear at 875, and the consumers at 762 and 1510-1514, with _authenticated_username at 3533 read to confirm the pure alternative. F2 rests on gitea_create_issue's permission constant at 3862 and its preflight task at 3876-3879, together with the live profile inventory showing gitea.issue.create only on the author profiles. The PR reports no conflict against its base. Live head SHA at the time of this comment: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb. Author changes during the read: none. LAST_UPDATED_BY: sysadmin (prgs-reviewer) NATIVE_REVIEW_PROOF: findings posted via gitea_create_issue_comment on native MCP namespace gitea-reviewer, profile prgs-reviewer, identity sysadmin, at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb. No formal review verdict was recorded by this call. No offline, import, or helper path was used. [THREAD STATE LEDGER] ### what is true now PR #909 is in open state at head 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb against master, and reports no conflict against that base. Server-side decision state: review 612 records an unresolved REQUEST_CHANGES at this head; this comment records no decision and adds findings only. Local verdict/state: REQUEST_CHANGES concurrence, two additional blockers, plus a local reproduction of review 612's failing test. ### what changed Nothing on the PR itself. A second finding set now exists on the thread alongside review 612, and a reviewer lease was taken and will be released. ### what is blocked Blocker classification: code blocker Recording a second formal verdict is refused by the duplicate-verdict rule (#332) while review 612 stands unresolved at this head; that is a process constraint, not a defect in this PR. The code blockers are review 612's incident-kind overwrite, reproduced as a failing `test_issue_886` case; F1 at `gitea_mcp_server.py:22760`, where a `read_only` preview calls `gitea_whoami` and thereby clears the resolved-capability stamp (line 875) and recomputes `_preflight_whoami_violation` (line 879), both of which raise at `verify_preflight_purity` lines 1510-1514; and F2 at lines 22866-22877, where the AC2/AC3 incident is written through `gitea_create_issue` under `gitea.issue.create` (line 3862) and a resolved `create_issue` task (lines 3876-3879) that no caller on this path holds. ### who/what acts next Next actor: author Required action: Preserve the #661 `restart_drain_gate_denied` kind on the drain-gate deny path and make `test_issue_886` pass on the PR head; replace the internal `gitea_whoami` call with `_authenticated_username(h)`; give the incident materializer a write path a privileged non-author caller can use, or fail the apply loudly when no capable writer is injected; surface `gitea_audit.audit_enabled()` in `payload["restart_audit"]`; and add tests that drive `gitea_request_mcp_restart` itself. Do not do: Do not resolve F1 by reordering the internal whoami or by documenting the side effect — the preview is consumed mid-workflow by design, so any preflight mutation inside it harms callers who did nothing wrong. Do not resolve F2 by reaching for raw `api_request` to skip the permission gate; PR #908 does exactly that and it is a blocker there. Do not read the 22 passing restart_audit tests as evidence against F1 or F2 — that module never imports the server and cannot observe the wiring.

sysadmin commented

2026-07-25 18:31:53 -05:00

repo: Scaled-Tech-Consulting/Gitea-Tools
pr: #909
issue: #665
reviewer_identity: sysadmin
profile: prgs-reviewer
session_id: 52249-5fe108392e0d
worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909
phase: released
candidate_head: 22e0a41bd5
target_branch: master
target_branch_sha: none
last_activity: 2026-07-25T23:31:53Z
expires_at: 2026-07-25T23:41:53Z
blocker: manual-release

repo: Scaled-Tech-Consulting/Gitea-Tools pr: #909 issue: #665 reviewer_identity: sysadmin profile: prgs-reviewer session_id: 52249-5fe108392e0d worktree: /Users/jasonwalker/Development/Gitea-Tools/branches/review-pr-909 phase: released candidate_head: 22e0a41bd5dfe4f47d2417b1f020e7d17f18c2fb target_branch: master target_branch_sha: none last_activity: 2026-07-25T23:31:53Z expires_at: 2026-07-25T23:41:53Z 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-665-restart-audit:feat/issue-665-restart-audit

git checkout feat/issue-665-restart-audit

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#909