pdurlej/patchwarden

Fork 0

safety(controller): bind the approval actuator to a ready controller_approval verdict + drift-guard, before any approval wiring #127

New issue

Closed

opened 2026-06-23 18:53:05 +02:00 by claude · 1 comment

claude commented

2026-06-23 18:53:05 +02:00

Collaborator

Context (architect note from the review loop)

The read-only controller chain is now complete and disciplined: controller_intake → controller_approval (#121, no-self-approval / hard-manual / head-bound / fail-closed verdict, read-only) → executor_preflight (#122) → executor_live_state (#124, GET-only). All write actuators in forgejo_client.py (ensure_pr_approval, create_pr_review, set_status, post_comment, …) currently have zero callers — dormant by design. 👏

The test_d20_architectural_boundary lint already does a lot:

bans merge_pull_request / submit_approval (call, def, and raw /merge URL),
restricts ensure_pr_approval( / create_pr_review( to the designated publication adapter.

The gap this issue records (forward-looking, nothing is broken)

The lint guards where approval may be called (the adapter only). It does not guarantee that the adapter, before calling ensure_pr_approval, actually consulted a ready controller_approval verdict. Today that's moot (no caller). The moment the approval adapter is wired — the single most dangerous upcoming PR — the safety of the whole D24 autonomy grant rests on that call site, and right now the two halves (controller_approval verdict ↔ forgejo_client actuator) are coupled only by convention, not structure.

This repo's idiom is "every invariant gets a build-time guard" (D20 lint, module-inventory drift-guard, cloud_review egress guard). The approval boundary deserves the same.

Definition-of-done for the upcoming approval-adapter PR (inspiration, not a spec)

Structural binding: the adapter consumes a controller_approval ready-verdict as a required precondition input — it must not re-derive eligibility ad-hoc. Unknown/blocked/missing verdict → no approval (fail-closed).
Drift-guard test (sibling to the D20 lint): prove ensure_pr_approval / create_pr_review(event="APPROVED") are unreachable without a ready verdict. The verdict must carry, and the guard must assert:
- no-self-approval — approving identity ≠ PR author (the #121 self_approval_risk / *_identity_unknown blockers must hold at the actuator, not just the preflight);
- hard-manual classes (secrets/workflow/runtime/auth/policy) are never auto-approved;
- head-bound — the verdict is tied to the exact head_sha being approved; stale head → refuse;
- fail-closed default — any uncertainty → no write.
(nit, optional) create_pr_review defaults event="APPROVED". Even adapter-restricted, a fail-unsafe default invites accidents — consider defaulting to COMMENT and requiring APPROVED explicitly.

Why now

#121 already built the preflight half correctly. This issue just writes down the binding + guard requirement so the actuator can't land decoupled from it — catching it as a pre-stated contract is cheaper than catching it in review after the live write surface exists.

Refs #121, #122, #124; D20, D24. — claude (architect loop)

## Context (architect note from the review loop) The read-only controller chain is now complete and disciplined: `controller_intake` → `controller_approval` (#121, no-self-approval / hard-manual / head-bound / fail-closed **verdict**, read-only) → `executor_preflight` (#122) → `executor_live_state` (#124, GET-only). All write actuators in `forgejo_client.py` (`ensure_pr_approval`, `create_pr_review`, `set_status`, `post_comment`, …) currently have **zero callers** — dormant by design. 👏 The `test_d20_architectural_boundary` lint already does a lot: - bans `merge_pull_request` / `submit_approval` (call, def, and raw `/merge` URL), - restricts `ensure_pr_approval(` / `create_pr_review(` to the designated publication adapter. ## The gap this issue records (forward-looking, nothing is broken) The lint guards **where** approval may be called (the adapter only). It does **not** guarantee that the adapter, before calling `ensure_pr_approval`, actually consulted a **ready** `controller_approval` verdict. Today that's moot (no caller). The moment the approval adapter is wired — the single most dangerous upcoming PR — the safety of the whole D24 autonomy grant rests on that call site, and right now the two halves (`controller_approval` verdict ↔ `forgejo_client` actuator) are coupled only by *convention*, not *structure*. This repo's idiom is "every invariant gets a build-time guard" (D20 lint, module-inventory drift-guard, cloud_review egress guard). The approval boundary deserves the same. ## Definition-of-done for the upcoming approval-adapter PR (inspiration, not a spec) 1. **Structural binding**: the adapter consumes a `controller_approval` ready-verdict as a *required precondition input* — it must not re-derive eligibility ad-hoc. Unknown/blocked/missing verdict → no approval (fail-closed). 2. **Drift-guard test** (sibling to the D20 lint): prove `ensure_pr_approval` / `create_pr_review(event="APPROVED")` are **unreachable** without a ready verdict. The verdict must carry, and the guard must assert: - **no-self-approval** — approving identity ≠ PR author (the #121 `self_approval_risk` / `*_identity_unknown` blockers must hold at the actuator, not just the preflight); - **hard-manual classes** (secrets/workflow/runtime/auth/policy) are never auto-approved; - **head-bound** — the verdict is tied to the exact `head_sha` being approved; stale head → refuse; - **fail-closed default** — any uncertainty → no write. 3. (nit, optional) `create_pr_review` defaults `event="APPROVED"`. Even adapter-restricted, a fail-unsafe default invites accidents — consider defaulting to `COMMENT` and requiring `APPROVED` explicitly. ## Why now #121 already built the preflight half correctly. This issue just writes down the *binding + guard* requirement so the actuator can't land decoupled from it — catching it as a pre-stated contract is cheaper than catching it in review after the live write surface exists. Refs #121, #122, #124; D20, D24. — claude (architect loop)

claude added the

area:v0-core

judge/codex-candidate

labels

2026-06-23 18:53:05 +02:00

codex referenced this issue from a pull request that will close it,

2026-06-23 19:22:49 +02:00

feat(controller): bind approvals to controller verdict #129

pdurlej closed this issue

2026-06-23 19:24:21 +02:00

pdurlej referenced this issue from a commit

2026-06-23 19:24:22 +02:00

Merge pull request 'feat(controller): bind approvals to controller verdict' (#129)

codex referenced this issue

2026-06-23 19:33:03 +02:00

docs(status): sync approval guard state #130

claude commented

2026-06-23 19:37:02 +02:00

Author

Collaborator

✅ Resolved by #129 — verified fail-closed, closing

#129 (feat(controller): bind approvals to controller verdict) implements every point of this issue, and then some. Verified against origin/main @ 4d4ba6d:

1. Structural binding ✅ — contract_publish requires a controller_approval ready-verdict artifact (controller_approval_file); missing → blocker controller_approval_file_missing, invalid → controller_approval_file_invalid (fail-closed). approve_on_pass defaults False. The actual ensure_pr_approval write is reached only when _approval_preflight_blockers(...) returns empty.

2. Drift-guard ✅ — test_d20_architectural_boundary.test_contract_publish_approval_surface_requires_controller_guard asserts the adapter calls _approval_preflight_blockers + controller_approval_actuation_blockers, requires controller_approval_file, and — crucially — that the preflight call precedes ensure_pr_approval in source order. The approval surface can't drift out of the gate without a test failing.

3. Fail-safe default ✅ — create_pr_review default flipped APPROVED → COMMENT, with test_create_pr_review_default_is_not_approved guarding it.

Invariants enforced by controller_approval_actuation_blockers:

no-self-approval + hard-manual (transitive): requires controller_approval.blockers to be an empty array — so any #121 verdict blocker (self_approval_risk, *_identity_unknown, hard-manual class) transitively refuses actuation.
head-bound ×2: controller_approval.target_sha and contract.head_sha must both equal the expected head.
live-head anchor (the part I was probing): _publication_guard fetches the live PR head and refuses on head_sha_changed or live_pr_fetch_failed — gating status/comment/approval alike. Tested by test_publish_contract_execute_refuses_stale_live_head and ..._refuses_when_live_preflight_fails.

So approval now requires: explicit approve_on_pass + a present/valid/zero-blocker controller_approval verdict + artifact-consistent head + live head match + contract verdict pass — every uncertainty fails closed. 613 tests green.

This is the read-only → guarded-actuation crossing of the D24 grant, done exactly to spec. Textbook. Closing as resolved. — claude (architect loop)

## ✅ Resolved by #129 — verified fail-closed, closing #129 (`feat(controller): bind approvals to controller verdict`) implements every point of this issue, and then some. Verified against `origin/main` @ `4d4ba6d`: **1. Structural binding** ✅ — `contract_publish` requires a `controller_approval` ready-verdict artifact (`controller_approval_file`); missing → blocker `controller_approval_file_missing`, invalid → `controller_approval_file_invalid` (fail-closed). `approve_on_pass` defaults `False`. The actual `ensure_pr_approval` write is reached **only** when `_approval_preflight_blockers(...)` returns empty. **2. Drift-guard** ✅ — `test_d20_architectural_boundary.test_contract_publish_approval_surface_requires_controller_guard` asserts the adapter calls `_approval_preflight_blockers` + `controller_approval_actuation_blockers`, requires `controller_approval_file`, and — crucially — that the preflight call **precedes** `ensure_pr_approval` in source order. The approval surface can't drift out of the gate without a test failing. **3. Fail-safe default** ✅ — `create_pr_review` default flipped `APPROVED` → `COMMENT`, with `test_create_pr_review_default_is_not_approved` guarding it. **Invariants enforced by `controller_approval_actuation_blockers`:** - **no-self-approval + hard-manual (transitive)**: requires `controller_approval.blockers` to be an **empty array** — so any #121 verdict blocker (`self_approval_risk`, `*_identity_unknown`, hard-manual class) transitively refuses actuation. - **head-bound ×2**: `controller_approval.target_sha` **and** `contract.head_sha` must both equal the expected head. - **live-head anchor** (the part I was probing): `_publication_guard` fetches the **live** PR head and refuses on `head_sha_changed` or `live_pr_fetch_failed` — gating status/comment/approval alike. Tested by `test_publish_contract_execute_refuses_stale_live_head` and `..._refuses_when_live_preflight_fails`. So approval now requires: explicit `approve_on_pass` + a present/valid/zero-blocker controller_approval verdict + artifact-consistent head + **live** head match + contract verdict `pass` — every uncertainty fails closed. 613 tests green. This is the read-only → guarded-actuation crossing of the D24 grant, done exactly to spec. Textbook. Closing as resolved. — claude (architect loop)

claude referenced this issue

2026-06-23 19:50:24 +02:00

docs(decisions): D24 says "Patchwarden never approves" but #129 shipped a guarded contract_publish APPROVED adapter — reconcile the authority record + clarify approver identity #132

claude referenced this issue

2026-06-23 20:49:40 +02:00

policy(design): "all tests/ → policy_governance" conflates guard-tests with feature-tests — refine before auto-approval is enabled on product repos #138

claude referenced this issue

2026-06-24 01:49:13 +02:00

safety(merge-frontier): bind the real merge adapter to a ready merge_actuation verdict + D20-lint carve + D-decision, before any merge call lands #171