arch(controller): no-self-approval (D24) needs a concrete fail-closed checkpoint before actuation #120

New issue

Closed

opened 2026-06-23 17:34:22 +02:00 by claude · 2 comments

claude commented

2026-06-23 17:34:22 +02:00

Collaborator

Architect follow-up from the loop review of #119 (controller-approval-preflight). Not a blocker on #119 — that PR is correct as a read-only preflight (merge_allowed=false, head-bound, fail-closed). This tracks the next-step completeness item. Inspiration, not a spec — the where is codex's call; the invariant just needs one concrete home.

The gap

Reviewing the controller chain (#107 / PW-G011–G012, G016) against D24, one invariant has no concrete enforcement point yet: "Codex-generated output never approves itself" — the merging/approving controller identity MUST differ from the PR-authoring identity (D24 + the docs/status.html hard boundary).

controller_approval.py (#119) consumes the handoff + contract + live PR state and emits ready/needs_live_state/blocked, but it does not carry or verify the PR-author identity vs the intended approver identity (no author/identity/self logic in the module). That's fine today because the preflight is read-only — but a ready verdict is precisely the signal a future actuator (PW-G011 guarded executor / PW-G012 visible-approval publication) will consume. Without an explicit no-self-approval checkpoint, a controller that is also the PR author could self-approve.

Why it matters

This is the scary failure mode the whole "controller ≠ author" rule exists to prevent — autonomous self-approval into merge. It's cheap to track + wire now (while the chain is still read-only) and expensive to discover after actuation lands.

Inspiration — possible shapes (your call)

Surface the PR-author identity in the approval-preflight verdict + an explicit self_approval_risk / author_differs_from_approver assertion, so the actuator inherits a fail-closed signal rather than re-deriving it; OR
Enforce at the actuator, with the preflight providing the identity inputs; OR
A dedicated tiny check module both consume.
Whatever the shape: default deny — if the approver identity is unknown or equals the author, the verdict is not ready.

Hard boundary (non-negotiable)

Fail-closed: unknown/equal identity → never ready. This is a D24 invariant, not a nice-to-have.

Refs

#119 (preflight — sound, read-only) · #107 (controller loop) · decisions.md D24 · docs/status.html hard boundary "Codex-generated output never approves itself" · src/patchwarden/controller_approval.py

Filed by claude (architect loop review, 2026-06-23). Executor: codex.

> Architect follow-up from the loop review of **#119** (controller-approval-preflight). **Not a blocker on #119** — that PR is correct as a read-only preflight (`merge_allowed=false`, head-bound, fail-closed). This tracks the next-step completeness item. Inspiration, not a spec — the *where* is codex's call; the invariant just needs one concrete home. ## The gap Reviewing the controller chain (#107 / PW-G011–G012, G016) against **D24**, one invariant has **no concrete enforcement point yet**: *"Codex-generated output never approves itself" — the merging/approving controller identity MUST differ from the PR-authoring identity* (D24 + the `docs/status.html` hard boundary). `controller_approval.py` (#119) consumes the handoff + contract + live PR state and emits `ready`/`needs_live_state`/`blocked`, but it does **not** carry or verify the **PR-author identity vs the intended approver identity** (no `author`/`identity`/`self` logic in the module). That's fine *today* because the preflight is read-only — but a `ready` verdict is precisely the signal a future actuator (PW-G011 guarded executor / PW-G012 visible-approval publication) will consume. Without an explicit no-self-approval checkpoint, a controller that is *also* the PR author could self-approve. ## Why it matters This is **the** scary failure mode the whole "controller ≠ author" rule exists to prevent — autonomous self-approval into merge. It's cheap to track + wire now (while the chain is still read-only) and expensive to discover after actuation lands. ## Inspiration — possible shapes (your call) - Surface the PR-author identity in the approval-preflight verdict + an explicit `self_approval_risk` / `author_differs_from_approver` assertion, so the actuator inherits a **fail-closed** signal rather than re-deriving it; OR - Enforce at the actuator, with the preflight providing the identity inputs; OR - A dedicated tiny check module both consume. Whatever the shape: **default deny** — if the approver identity is unknown or equals the author, the verdict is not `ready`. ## Hard boundary (non-negotiable) Fail-closed: unknown/equal identity → never `ready`. This is a D24 invariant, not a nice-to-have. ## Refs - #119 (preflight — sound, read-only) · #107 (controller loop) · `decisions.md` D24 · `docs/status.html` hard boundary "Codex-generated output never approves itself" · `src/patchwarden/controller_approval.py` Filed by claude (architect loop review, 2026-06-23). Executor: codex.

codex referenced this issue

2026-06-23 17:41:44 +02:00

feat: enforce no-self approval preflight #121

pdurlej commented

2026-06-23 17:45:04 +02:00

Owner

Addressed by merged PR #121. controller-approval-preflight now emits a required identity section and fails closed unless the PR author and visible approver are both known and different. Schema, public example, status docs, and regression tests were updated.

Addressed by merged PR #121. `controller-approval-preflight` now emits a required identity section and fails closed unless the PR author and visible approver are both known and different. Schema, public example, status docs, and regression tests were updated.

pdurlej closed this issue

2026-06-23 17:45:04 +02:00

claude commented

2026-06-23 17:48:27 +02:00

Author

Collaborator

Resolved by #121 — verified fail-closed ✅

controller_approval.py now enforces the D24 no-self-approval invariant, and it's default-deny exactly as this issue required:

unknown PR author → blocker pr_author_identity_unknown (not ready)
unknown approver/visible_actor → blocker visible_approver_identity_unknown (not ready)
pr_author == visible_actor (case-insensitive casefold) → blocker self_approval_risk ("visible approver identity must differ from the PR author identity")
only author ≠ approver, both known → no blocker

So "unknown or equal identity → never ready" holds. The D24 / status.html "Codex-generated output never approves itself" boundary now has a concrete, fail-closed checkpoint in the controller chain — before any actuator lands. 574 tests green.

Closing as done. Nice fast turnaround, codex. 🤝

## Resolved by #121 — verified fail-closed ✅ `controller_approval.py` now enforces the D24 no-self-approval invariant, and it's **default-deny** exactly as this issue required: - unknown PR author → blocker `pr_author_identity_unknown` (not `ready`) - unknown approver/`visible_actor` → blocker `visible_approver_identity_unknown` (not `ready`) - `pr_author == visible_actor` (case-insensitive `casefold`) → blocker `self_approval_risk` ("visible approver identity must differ from the PR author identity") - only author **≠** approver, both known → no blocker So "unknown or equal identity → never `ready`" holds. The D24 / `status.html` "Codex-generated output never approves itself" boundary now has a concrete, fail-closed checkpoint in the controller chain — before any actuator lands. 574 tests green. Closing as done. Nice fast turnaround, codex. 🤝