Feat: Unify policy-conflict detection on /apply, retire /policy/check (#2500-#2504)

aiac/CONTEXT.md

@@ -0,0 +1,66 @@
+# AIAC
+
+The AIAC agent turns natural-language authorization policy into applied
+PolicyRules. It surveys IdP roles and scopes, drives an LLM-backed Policy Rules
+Builder over focal entities, and computes/applies the resulting rules through a
+two-layer policy stack. This glossary fixes the vocabulary for how the builder
+grants, prohibits, and reports collisions.
+
+## Language
+
+**Focal entity**:
+The single role or scope a Policy Rules Builder pass is centred on for one
+`build()` run. Every pass fans candidates against exactly one focal per run.
+_Avoid_: subject, principal, target.
+
+**Scope-focal pass**:
+The pass centred on a scope, fanning candidate roles over it. It is the sole
+**grant authority** for that scope.
+_Avoid_: scope pass, forward pass.
+
+**User-role-focal pass** (a.k.a. **Door B**):
+A pass centred on a `kind=User` role, fanning it over the focus service's own
+scopes to emit the **deny** rules that a user's exclusivity ("Testers may access
+**only** issues") implies — prohibitions the scope-focal pass structurally
+cannot express. Contributes denies only; never broadens access.
+_Avoid_: role pass (ambiguous with the agent-role-focal pass), Door B pass.
+
+**Grant authority**:
+The property that grants on a given scope come from exactly one place — the
+scope-focal pass. Door B adds only prohibitions and never grants.
+_Avoid_: owner, source of truth.
+
+**Contradiction**:
+An *intra-pass* grant∩deny: one focal's own proposed rule set both grants and
+prohibits the same candidate. Detected by the LLM auditor within a single pass,
+which fails that pass closed. Modelled by `Contradiction` / raised as
+`PolicyContradictionError`.
+_Avoid_: using "conflict" for this — the two are distinct.
+
+**Conflict**:
+A *cross-pass* grant∩deny: an `Allow` from one pass and a `Deny` from another on
+the **same `(role, scope)`** pair. Structural (a pure id-level allow∩deny
+set-intersection over the assembled rules), not LLM-audited. Modelled by
+`Conflict` / `ConflictReport`.
+_Avoid_: using "contradiction" for this.
+
+**Within-batch conflict**:
+A **conflict** whose two rules are produced in one `build()` call — i.e. one
+`/apply` request. This is the Door B case: at the focus service's own-scope
+onboarding, both the scope-focal grant and the Door B deny (and any collision
+between them) are in hand in the same build. In scope.
+_Avoid_: intra-request conflict.
+
+**Cross-run conflict** (a.k.a. **cross-service conflict**):
+A **conflict** whose two rules are produced in separate onboarding requests and
+collide only in the persisted SPM store. Surfaced at `/apply` by the
+cross-service check, which reads the already-applied rules of the services that
+own the touched scopes and folds them into detection; the pure within-build
+structural pass alone does not see it.
+_Avoid_: cross-request conflict, store conflict.
+
+**Identify-never-reconcile**:
+The governing principle: a `(role, scope)` carrying both an `Allow` and a `Deny`
+**is** a conflict — surface it, never resolve it. No precedence, no
+"deny wins," no merge. See `docs/adr/0001-identify-never-reconcile.md`.
+_Avoid_: deny-overrides, conflict resolution.

aiac/docs/adr/0001-identify-never-reconcile.md

@@ -0,0 +1,83 @@
+# Identify policy conflicts; never reconcile them
+
+When the assembled PolicyRules for a service carry both an `Allow` and a `Deny`
+on the same `(role, scope)` pair (a **conflict**), the Policy Rules Builder
+**surfaces** it and refuses to apply — it never picks a winner. We deliberately
+reject deny-overrides, allow-overrides, precedence ordering, and silent merging:
+a conflict means the policy prose is genuinely ambiguous, and resolving it in
+code would bury that ambiguity behind a rule the author never stated.
+
+## Status
+
+accepted
+
+## Consequences
+
+- There is a **single entry point, `/apply`**: no conflict → rules are built and
+  applied; conflict → an exception is raised and nothing is applied. A separate
+  read-only `/policy/check` is **not** part of this model.
+- Detection is a pure `(role.id, scope.id)` allow∩deny set-intersection over the
+  assembled `list[PolicyRule]`, run **inside the build** before any compute/apply
+  — so a conflict leaves persisted state untouched (atomic-by-construction).
+- A found conflict is raised as a single `ConflictReport`-carrying exception and
+  mapped to HTTP 422 with the structured report as the body.
+- Scope is **within one service's build** (Q13). Cross-service conflicts — rules
+  written by different `build()` calls colliding only in the persisted store —
+  were originally left as a follow-up gap here; they are now detected (still
+  identify-never-reconcile) — see the `#2504` addendum below.
+- The intra-pass `PolicyContradictionError` (the LLM auditor's grant∩deny within
+  one pass) is a separate, disjoint mechanism and keeps failing that pass closed;
+  it is not merged into the cross-pass detector, only re-shaped to the same 422
+  report body at the boundary (Q15).
+
+## Addendum (#2503): verbatim-quoted reports on /apply — reversing handoff-07 Q15/Q16
+
+Handoff 07 settled the on-`/apply` conflict report as **quote-less / no-LLM**:
+
+- **Q15** ("Boundary unification") decided to *"unify the report shape, not the
+  payload"* — one new structural exception carrying a `ConflictReport`, and
+  mapping `PolicyContradictionError` to that shape *"(shallow, **no LLM**)"* at
+  the 422 handler.
+- **Q16** ("`Conflict.focal` for a structural conflict") anchored the structural
+  conflict on the **SCOPE** side (`FocalType.SCOPE`) and, together with #2502's
+  structural detector, produced each `Conflict` with **empty
+  `granting_quotes`/`prohibiting_quotes` and `quotes_verified=False`** — a
+  deterministic, LLM-free report.
+
+**#2503 reverses the quote-less / no-LLM decision for the structural path.** The
+`/apply` conflict report is now the **rich, verbatim-quoted** `ConflictReport`:
+when — and **only when** — the deterministic `detect_conflicts` finds a structural
+conflict, an LLM explain/quote pass (`conflict_enrichment.enrich_report`, reusing
+the re-homed diagnostic `explain` machinery) runs over exactly the pairs the
+detector surfaced, classifying each `kind` (`direct`/`coarse_scope`) and
+extracting **substring-validated** quotes from the candidate policy text. A clean
+apply stays fast and **LLM-free** (the explain seam never fires), so the gating —
+not the report's fidelity — is what preserves handoff 07's performance intent.
+
+Unchanged from handoff 07: the SCOPE-side focal anchoring (Q16), the identify-
+never-reconcile principle above, and Q15's *shape* unification — both
+`PolicyConflictError` (now enriched) and `PolicyContradictionError` (mapped
+shallow, still **no LLM** at the boundary, `quotes_verified=false`) yield one 422
+`ConflictReport` body. On any quote-validation failure the conflict is **kept**
+with `quotes_verified=false` and a description fallback — never dropped.
+
+## Addendum (#2504): cross-service conflicts are detected (closing the Q13 gap)
+
+The Q13 consequence above scoped detection to **one service's build** and left
+cross-service conflicts — an `Allow` in the current build colliding with a `Deny`
+another service already persisted (or vice versa) on the same `(role.id,
+scope.id)` — as a follow-up gap. `#2504` closes that gap **without** changing the
+principle: still identify-never-reconcile, still no precedence/merge.
+
+`ServicePolicyBuilder.build` now widens the detector's input to the **combined**
+state — this build's assembled rules **plus** the already-applied inbound rules of
+the other services that own the touched scopes, read from the Policy Store
+(`applied_rules_for_scopes`). The **same** `#2502` `(role.id, scope.id)`
+allow∩deny intersection then surfaces an overlap that a single build's own rules
+could never reveal. The store read is **read-only** and the raise still happens
+**before** `compute_and_apply`, so the atomic-by-construction guarantee holds: a
+cross-service conflict leaves persisted state untouched. Detection stays
+order-independent (keyed on ids), so tool-first vs agent-first onboarding yields
+the identical outcome, and the result is emitted in the same 422 `ConflictReport`
+shape. (The single-writer basis of "atomic-by-construction" is unchanged;
+transactional safety across *concurrent* applies remains a separate follow-up.)

aiac/docs/specs/components/aiac-agent/policy-conflict-check.md

@@ -1,5 +1,17 @@
 # Sub-PRD: AIAC Agent — Policy Conflict Check (pre-commit diagnostic)
 
+> **⚠️ RETIRED / SUPERSEDED (#2500, #2503).** The standalone read-only `POST /policy/check`
+> route described below **no longer exists**. `/apply` is now the **sole** policy entry point:
+> the rich conflict diagnostic (verbatim-quoted `ConflictReport`) is folded directly into the
+> apply path and returned as an HTTP 422 body on a genuine grant/deny conflict — whether a
+> cross-pass structural conflict (`PolicyConflictError`, quote-enriched) or the intra-pass LLM
+> auditor's `PolicyContradictionError` (re-shaped into the **same** `ConflictReport`). The
+> `PolicyCheckRequest` model and the `check_policy_conflicts(...)` function are
+> removed; the survey orchestrator was re-homed into `policy_rules_builder/diagnostic_survey.py`
+> and reused by the apply path. See [`../../../adr/0001-identify-never-reconcile.md`](../../../adr/0001-identify-never-reconcile.md)
+> (the `#2503` addendum). This document is kept for historical context only — treat the
+> interface section below as describing a retired route, not a shipped contract.
+
 > **Depends on:** [`../aiac-agent.md`](../aiac-agent.md) — Controller, Shared Module, Configuration, Error Handling, Runtime.
 
 > **Sits next to the live PRB contradiction path.** This diagnostic reuses the Policy Rules Builder machinery specified in [`policy-rules-builder.md`](policy-rules-builder.md), but is a **separate, read-only** path. The live `/apply` → `PolicyContradictionError` → HTTP 422 contract documented there is **UNCHANGED** by this feature.
@@ -23,8 +35,8 @@ report. It does **not** replace or modify the live `/apply` → 422 path.
 
 ## Interface
 
-- **New Controller route** — `POST /policy/check` (**recommended**; the exact path is a #154 open item,
-  `/policy/check` vs `/policy/conflicts`). The route is a **thin serialization shell** over a testable
+- **New Controller route** — `POST /policy/check` (**final** — shipped path; the earlier
+  `/policy/check` vs `/policy/conflicts` open item is settled). The route is a **thin serialization shell** over a testable
   plain function:
 
   ```python
@@ -255,7 +267,6 @@ seam; the opt-in `-m llm` suite runs the real model — see [`policy-rules-build
   per-service check** — a **separate effort**, not this work.
 - **Survey concurrency:** assume **sequential** (matches `builder.py`); parallelize only if latency
   demands.
-- **Route path name:** decide at implementation (`/policy/check` vs `/policy/conflicts`).
 - **Subtle-prose robustness / PRB precedence tuning** (explicit prohibition vs description-derived
   grant): a separate PRB-quality concern, not a correctness gate for this feature.
 - **ALLOW-vs-DENY precedence / tie-break at enforcement time:** a distinct PCE/Rego concern (tracked in

aiac/src/aiac/agent/controller/routes.py

@@ -16,17 +16,18 @@
 import uvicorn
 from fastapi import FastAPI, Request
 from fastapi.responses import JSONResponse, Response
-from pydantic import BaseModel
 
 from aiac.agent.eventbus.consumer import lifespan
-from aiac.agent.policy_rules_builder.diagnostic_models import ConflictReport
+from aiac.agent.policy_rules_builder.conflict_detection import (
+    PolicyConflictError,
+    report_from_contradictions,
+)
 from aiac.agent.policy_rules_builder.graph import (
     PolicyContradictionError,
     PolicyRulesBuilderError,
 )
 from aiac.agent.uc.offboarding.offboard import offboard_service
 from aiac.agent.uc.onboarding.orchestrator import onboard_service
-from aiac.agent.uc.policy_check.check import check_policy_conflicts
 from aiac.agent.uc.policy_update.build import build_policy
 from aiac.agent.uc.policy_update.rebuild import rebuild_policy
 from aiac.agent.uc.role_update.role import update_role
@@ -37,16 +38,35 @@
 
 
 # The Policy Rules Builder raises on a policy-input problem, not a server fault: the auditor
-# rejects the proposed rules after exhausting its retry budget (``PolicyRulesBuilderError``) or
-# finds a genuine grant/deny contradiction (``PolicyContradictionError``). Both are the caller's
-# policy prose failing to lift, so they surface as HTTP 422 (mirroring the contract documented in
-# the PRB spec + pdp-policy-writer-opa.md) rather than escaping as an uncaught 500. The PCE is
-# never reached — these fire during rule construction inside the use-case handlers.
+# rejects the proposed rules after exhausting its retry budget (``PolicyRulesBuilderError``). That
+# is a builder failure with no structured finding, so it surfaces as HTTP 422 with a bare
+# ``{"detail": ...}`` body rather than escaping as an uncaught 500. The PCE is never reached — it
+# fires during rule construction inside the use-case handlers.
 @app.exception_handler(PolicyRulesBuilderError)
-@app.exception_handler(PolicyContradictionError)
 def _policy_input_error(_request: Request, exc: Exception) -> JSONResponse:
     return JSONResponse(status_code=422, content={"detail": str(exc)})
 
+
+# The two grant/deny conflict mechanisms both surface as a 422 whose body IS a structured
+# ``ConflictReport`` (settled design Q15 — one report shape at the boundary):
+#   * ``PolicyConflictError`` (cross-pass structural detector, already enriched with verbatim quotes
+#     + classified kind before the raise) carries a rich ``ConflictReport`` directly.
+#   * ``PolicyContradictionError`` (intra-pass LLM auditor) carries only name-strings, so it is
+#     re-shaped into the SAME ``ConflictReport`` (lower-fidelity: no ids/quotes, ``kind=DIRECT``,
+#     ``quotes_verified=False``) with ``report_from_contradictions`` — no LLM at the boundary.
+# Both are policy findings, not server faults, and both fire before the PCE is reached
+# (atomic-by-construction: nothing is persisted).
+@app.exception_handler(PolicyConflictError)
+def _policy_conflict_error(_request: Request, exc: PolicyConflictError) -> JSONResponse:
+    return JSONResponse(status_code=422, content=exc.report.model_dump(mode="json"))
+
+
+@app.exception_handler(PolicyContradictionError)
+def _policy_contradiction_error(_request: Request, exc: PolicyContradictionError) -> JSONResponse:
+    report = report_from_contradictions(exc.focal, exc.contradictions)
+    return JSONResponse(status_code=422, content=report.model_dump(mode="json"))
+
+
 # Live on-ramp for the per-onboarding default_effect. The PCE-threading side (#146) exposes
 # default_effect as an onboard_service parameter; the integration harness (#149) requests a
 # non-default value by patching AIAC_DEFAULT_EFFECT ("Allow"/"Deny") onto the Controller
@@ -110,27 +130,6 @@ def apply_offboard(service_id: str) -> Response:
     return Response(status_code=200)
 
 
-class PolicyCheckRequest(BaseModel):
-    """Body for ``POST /policy/check``: candidate ``policy_text`` to survey against the focal
-    entities of ``service_id`` (the Keycloak internal client UUID, matching
-    ``/apply/service/{service_id}``). ``policy_text`` is required — its absence is a FastAPI
-    validation 422 with no report body."""
-
-    policy_text: str
-    service_id: str
-
-
-# The read-only conflict-check survey (feature #154). UNLIKE the live /apply routes, this is a
-# diagnostic: ANY completed survey is a success and returns 200 with the ConflictReport body —
-# a found conflict is a recorded finding, NOT a 422. Only the resolver's pre-survey boundary
-# (HTTPException 502 IdP-unreachable / 404 unknown-service) propagates, unchanged, as the bare
-# non-2xx error with no report body (we deliberately do NOT catch it). FastAPI serializes the
-# returned ConflictReport pydantic model to the JSON response body.
-@app.post("/policy/check")
-def policy_check(body: PolicyCheckRequest) -> ConflictReport:
-    return check_policy_conflicts(body.policy_text, body.service_id)
-
-
 def main() -> None:
     uvicorn.run(app, host="0.0.0.0", port=7070)