From f4203be5a09b48d7606f23072605dd3c3f274b27 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Wed, 26 Aug 2026 16:10:55 +0000 Subject: [PATCH 01/13] Fix: Surface unjoinable auditor contradictions instead of dropping them Address review feedback on #154 (PR #809). Correctness (was: silent drop -> clean masquerade): _explain joined the auditor's free-form Contradiction.candidate_name against this run's typed candidate set, and on a miss it 'continue'd silently. precheck filters the PROPOSER's name lists, not the auditor's candidate_name, so a confirmed contradiction against an unjoinable name was dropped with no trace -- and if it was the only one for the entity, the survey classified the entity clean and the report could reach no_conflict while a contradiction was confirmed. Now mark the focal entity Unevaluated(reason=unjoinable_candidate) so its presence forces status to incomplete (never no_conflict). New UnevaluatedReason enum member + regression test; existing enum-values test updated. Docs: declare POST /policy/check final in the sub-PRD (remove the settled /policy/check vs /policy/conflicts open item) -- the route shipped as /policy/check and PRD/component specs already say so. Docs (route): document that an empty policy_text is a well-formed request that surveys to no_conflict/incomplete (never conflicts_found), not a 422 -- AC#4 only requires a *missing* field to be non-2xx. Not changed (by design): quotes_verified=True with one empty quote side is intended per US#18 (description-derived grants carry no citation); a quoting FAILURE is what sets quotes_verified=False (US#19). The double get_services() fetch is tracked separately in rossoctl/rossoctl#2476. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../aiac-agent/policy-conflict-check.md | 5 +-- aiac/src/aiac/agent/controller/routes.py | 7 +++- .../agent/policy_rules_builder/diagnostic.py | 20 ++++++++-- .../policy_rules_builder/diagnostic_models.py | 10 +++-- .../policy_rules_builder/test_diagnostic.py | 39 +++++++++++++++++++ .../test_diagnostic_models.py | 2 +- 6 files changed, 72 insertions(+), 11 deletions(-) diff --git a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md index 6a81dcaa0..bc6d0ecdb 100644 --- a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md +++ b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md @@ -23,8 +23,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 +255,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 diff --git a/aiac/src/aiac/agent/controller/routes.py b/aiac/src/aiac/agent/controller/routes.py index a06fffbab..65fbff438 100644 --- a/aiac/src/aiac/agent/controller/routes.py +++ b/aiac/src/aiac/agent/controller/routes.py @@ -114,7 +114,12 @@ 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.""" + validation 422 with no report body. + + An *empty* ``policy_text`` (``""``) is a well-formed request, not a 422: it is surveyed like + any other prose. With no grants or prohibitions to collide, the report lands on + ``no_conflict`` (or ``incomplete`` when zero focal entities could be evaluated), never + ``conflicts_found`` — an honest "nothing to conflict" result rather than a boundary error.""" policy_text: str service_id: str diff --git a/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py b/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py index f8434a3e4..a4ebd2814 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py +++ b/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py @@ -229,11 +229,25 @@ def _explain( policy_text = state["policy_text"] focal_ref = FocalRef(name=focal_obj.name, id=focal_obj.id, type=focal_type) out: list[Conflict] = [] + dropped: list[Unevaluated] = [] for contradiction in state["recorded_contradictions"]: candidate = candidate_by_name.get(contradiction.candidate_name) if candidate is None: - # Defensive: precheck already filtered names to the candidate set, so an unjoinable - # name should not occur. Skip rather than emit a conflict with a fabricated id. + # precheck filters the PROPOSER's name lists, not the AUDITOR's free-form + # candidate_name, so an unjoinable name can still reach here. Never emit a conflict + # with a fabricated id — but never lose the signal either: mark the focal entity + # unevaluated so a confirmed-but-unjoinable contradiction cannot let the survey + # report no_conflict (its presence forces status to incomplete). + dropped.append( + Unevaluated( + focal=focal_ref, + reason=UnevaluatedReason.UNJOINABLE_CANDIDATE, + detail=( + f"auditor confirmed a contradiction for candidate " + f"{contradiction.candidate_name!r} not in this run's candidate set" + ), + ) + ) continue if focal_type is FocalType.ROLE: role_obj, scope_obj = focal_obj, candidate @@ -268,7 +282,7 @@ def _explain( quotes_verified=verified, ) ) - return {"conflicts": out} + return {"conflicts": out, "unevaluated": dropped} def _route_diagnostic(state: DiagnosticState) -> str: diff --git a/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py index 0d4e523ff..8bad9d531 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py +++ b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py @@ -48,11 +48,15 @@ class ConflictStatus(str, Enum): class UnevaluatedReason(str, Enum): - """Why a focal entity could not be evaluated. Currently a single value: ``nonconvergence`` - (the entity exhausted the audit retry budget without a verdict). Kept as an enum so callers - switch on a stable token, with the free-text ``detail`` carrying specifics.""" + """Why a focal entity could not be evaluated. ``nonconvergence`` — the entity exhausted the + audit retry budget without a verdict. ``unjoinable_candidate`` — the auditor confirmed a + contradiction against a ``candidate_name`` that does not join to this run's typed candidate + set, so the conflict cannot be emitted with a real id yet must not be dropped (else a + confirmed contradiction could let the report reach ``no_conflict``). Kept as an enum so + callers switch on a stable token, with the free-text ``detail`` carrying specifics.""" NONCONVERGENCE = "nonconvergence" + UNJOINABLE_CANDIDATE = "unjoinable_candidate" class EntityRef(BaseModel): diff --git a/aiac/test/agent/policy_rules_builder/test_diagnostic.py b/aiac/test/agent/policy_rules_builder/test_diagnostic.py index f2df1739b..46e430464 100644 --- a/aiac/test/agent/policy_rules_builder/test_diagnostic.py +++ b/aiac/test/agent/policy_rules_builder/test_diagnostic.py @@ -180,6 +180,45 @@ def se(schema, messages): assert u.detail == "still not right" +# --------------------------------------------------------------------------- # +# 4b — the auditor CONFIRMS a contradiction against an UNJOINABLE candidate # +# name (one not in this run's typed candidate set). The conflict cannot be # +# emitted with a real id, but it must NOT be dropped silently: the focal # +# entity is marked unevaluated (unjoinable_candidate) so a confirmed # +# contradiction can never let the survey report no_conflict. # +# --------------------------------------------------------------------------- # +def test_unjoinable_auditor_candidate_is_marked_unevaluated_not_clean(): + role = _role("r-dev", "developer") + issues = _scope("s-iss", "issues") + + def se(schema, messages): + if schema is AuditVerdict: + # candidate_name the precheck never saw (precheck filters the PROPOSER's names, not + # the auditor's) and which does not join to the candidate scope set {"issues"}. + return AuditVerdict( + approved=False, + contradictions=[Contradiction(candidate_name="ghost", description="phantom collision")], + ) + return RoleSelection( + granted_scope_names=["issues"], denied_scope_names=["issues"], reasoning="r" + ) + + with ExitStack() as stack: + _patch_calls(stack, se) + result = run_role_diagnostic("Developers policy about issues.", role, [issues]) + + # No conflict with a fabricated id is emitted ... + assert result.conflicts == [] + # ... but the confirmed-yet-unjoinable contradiction is surfaced as unevaluated, so the + # survey cannot classify this entity clean (status is forced away from no_conflict). + assert len(result.unevaluated) == 1 + u = result.unevaluated[0] + assert u.reason.value == "unjoinable_candidate" + assert u.focal.type is FocalType.ROLE + assert (u.focal.name, u.focal.id) == ("developer", "r-dev") + assert "ghost" in u.detail + + # --------------------------------------------------------------------------- # # 5 — substring-validation FAILURE: the explain call returns a non-substring # # granting quote, so the conflict is KEPT with quotes_verified=False and # diff --git a/aiac/test/agent/policy_rules_builder/test_diagnostic_models.py b/aiac/test/agent/policy_rules_builder/test_diagnostic_models.py index 91d4620af..7f59fd259 100644 --- a/aiac/test/agent/policy_rules_builder/test_diagnostic_models.py +++ b/aiac/test/agent/policy_rules_builder/test_diagnostic_models.py @@ -34,7 +34,7 @@ def test_status_values_exactly(): def test_unevaluated_reason_values(): - assert [m.value for m in UnevaluatedReason] == ["nonconvergence"] + assert [m.value for m in UnevaluatedReason] == ["nonconvergence", "unjoinable_candidate"] # --- ref models ------------------------------------------------------------------------------- From be30ded96e5ac916482af2b919b1a941215156e5 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 09:50:53 +0000 Subject: [PATCH 02/13] Refactor: Retire /policy/check route, re-home diagnostic as library (#2500) Unify the two policy entry points into one: /apply is now the sole policy route. The read-only /policy/check route and its PolicyCheckRequest model are removed from the controller. The rich conflict-diagnostic engine is preserved and re-homed as an internal library (not deleted, reversing handoff 07 step 4 per issue #2500): - Move the survey orchestrator check_policy_conflicts from the route-tied uc/policy_check/check.py into policy_rules_builder/diagnostic_survey.py, next to the per-entity engine; dissolve the orphaned uc/policy_check/ pkg. - diagnostic.py, diagnostic_models.py, prompts.py already lived in the library; drop the last textual tie to the removed route from the diagnostic_models docstring. Tests mirror src: delete the route-level test, move + retarget the survey and live-LLM tests to policy_rules_builder/, and move the /apply contradiction regression guard (no symbol change). The live /apply proposer/auditor graph is byte-for-byte unchanged. Verification: unit suite 652 passed (no --ignore); llm suite 8 passed; integration errors are pre-existing LLM auditor non-convergence in the live /apply onboarding path, unrelated to this route retirement. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- aiac/src/aiac/agent/controller/routes.py | 29 --- .../policy_rules_builder/diagnostic_models.py | 4 +- .../diagnostic_survey.py} | 7 +- .../aiac/agent/uc/policy_check/__init__.py | 1 - .../controller/test_policy_check_route.py | 188 ------------------ aiac/test/agent/policy_check/__init__.py | 0 .../test_apply_conflict_regression.py | 3 +- .../test_conflict_check_live_llm.py | 2 +- .../test_diagnostic_survey.py} | 2 +- aiac/test/agent/uc/policy_check/__init__.py | 0 10 files changed, 12 insertions(+), 224 deletions(-) rename aiac/src/aiac/agent/{uc/policy_check/check.py => policy_rules_builder/diagnostic_survey.py} (92%) delete mode 100644 aiac/src/aiac/agent/uc/policy_check/__init__.py delete mode 100644 aiac/test/agent/controller/test_policy_check_route.py delete mode 100644 aiac/test/agent/policy_check/__init__.py rename aiac/test/agent/{policy_check => policy_rules_builder}/test_apply_conflict_regression.py (95%) rename aiac/test/agent/{policy_check => policy_rules_builder}/test_conflict_check_live_llm.py (99%) rename aiac/test/agent/{uc/policy_check/test_check.py => policy_rules_builder/test_diagnostic_survey.py} (99%) delete mode 100644 aiac/test/agent/uc/policy_check/__init__.py diff --git a/aiac/src/aiac/agent/controller/routes.py b/aiac/src/aiac/agent/controller/routes.py index 65fbff438..85dc4e870 100644 --- a/aiac/src/aiac/agent/controller/routes.py +++ b/aiac/src/aiac/agent/controller/routes.py @@ -16,17 +16,14 @@ 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.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 @@ -110,32 +107,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. - - An *empty* ``policy_text`` (``""``) is a well-formed request, not a 422: it is surveyed like - any other prose. With no grants or prohibitions to collide, the report lands on - ``no_conflict`` (or ``incomplete`` when zero focal entities could be evaluated), never - ``conflicts_found`` — an honest "nothing to conflict" result rather than a boundary error.""" - - 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) diff --git a/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py index 8bad9d531..ed1ea2a96 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py +++ b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_models.py @@ -1,7 +1,7 @@ """Structured models for the read-only policy **conflict-check** diagnostic (feature #154). -This module defines ONLY the stable serialization shape shared by the diagnostic engine, the -survey use-case, and the ``POST /policy/check`` route — no pipeline logic. The live ``/apply`` +This module defines ONLY the stable serialization shape shared by the diagnostic engine and the +survey orchestrator — no pipeline logic. The live ``/apply`` path and its models (``PolicyRule``, ``Contradiction``, ``AuditVerdict`` in ``graph.py`` / ``policy.model.models``) are deliberately untouched (decision D11): the typed conflict ``kind`` lives here, produced by the diagnostic's ``explain`` node, rather than being bolted onto the diff --git a/aiac/src/aiac/agent/uc/policy_check/check.py b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_survey.py similarity index 92% rename from aiac/src/aiac/agent/uc/policy_check/check.py rename to aiac/src/aiac/agent/policy_rules_builder/diagnostic_survey.py index 694cf0ea5..44a8c89f0 100644 --- a/aiac/src/aiac/agent/uc/policy_check/check.py +++ b/aiac/src/aiac/agent/policy_rules_builder/diagnostic_survey.py @@ -1,4 +1,9 @@ -"""Policy Conflict Check survey use-case (feature #154, task #158). +"""Policy Conflict Check survey orchestrator (feature #154, task #158). + +Re-homed (#2500) from the retired ``uc/policy_check`` use-case into the diagnostic library, next +to the per-entity engine it drives (``diagnostic.py``). It is now a plain internal library +function with no dependency on any route — the standalone ``/policy/check`` route was retired so +``/apply`` is the sole policy entry point; a later ticket folds this orchestrator into ``/apply``. A **sequential, read-only** survey that runs EVERY focal entity of a target service through the Conflict-Check diagnostic graph (#157) to completion, accumulates every run's ``conflicts`` + diff --git a/aiac/src/aiac/agent/uc/policy_check/__init__.py b/aiac/src/aiac/agent/uc/policy_check/__init__.py deleted file mode 100644 index c3cad07c1..000000000 --- a/aiac/src/aiac/agent/uc/policy_check/__init__.py +++ /dev/null @@ -1 +0,0 @@ -"""Read-only Policy Conflict Check survey use-case (feature #154, task #158).""" diff --git a/aiac/test/agent/controller/test_policy_check_route.py b/aiac/test/agent/controller/test_policy_check_route.py deleted file mode 100644 index dbad013c6..000000000 --- a/aiac/test/agent/controller/test_policy_check_route.py +++ /dev/null @@ -1,188 +0,0 @@ -"""Unit tests for the ``POST /policy/check`` conflict-check route (feature #154, task #159). - -This is the diagnostic serialization shell: it calls the read-only survey use-case -(``check_policy_conflicts``) and serializes the returned :class:`ConflictReport` as a JSON -response body. The use-case is patched at the routes-module boundary — no live IdP, no LLM, no -real diagnostic graph. UNLIKE the live ``/apply`` path, a found conflict is a successful diagnosis -and returns 200 (never 422); only the survey's pre-survey ``HTTPException(502/404)`` propagates. -""" - -from unittest.mock import patch - -from fastapi import HTTPException -from fastapi.testclient import TestClient - -from aiac.agent.controller.routes import app -from aiac.agent.policy_rules_builder.diagnostic_models import ( - Conflict, - ConflictKind, - ConflictReport, - ConflictStatus, - EntityRef, - FocalRef, - FocalType, - Unevaluated, - UnevaluatedReason, -) - -client = TestClient(app) - - -def _focal(name: str = "editor", id: str = "r-1") -> FocalRef: - return FocalRef(name=name, id=id, type=FocalType.ROLE) - - -def _conflict() -> Conflict: - return Conflict( - focal=_focal(), - role=EntityRef(name="editor", id="r-1"), - scope=EntityRef(name="write", id="s-1"), - kind=ConflictKind.DIRECT, - granting_quotes=["editors may write"], - prohibiting_quotes=["editors must not write"], - explanation="write is both granted and prohibited for editor", - quotes_verified=True, - ) - - -def _unevaluated() -> Unevaluated: - return Unevaluated( - focal=_focal("viewer", "r-2"), - reason=UnevaluatedReason.NONCONVERGENCE, - detail="retry budget exhausted", - ) - - -def test_clean_report_returns_200_no_conflict(): - # A survey that evaluated ≥1 entity with nothing outstanding is a positive clean result. - report = ConflictReport.from_survey([], [], evaluated_count=2) - assert report.status is ConflictStatus.NO_CONFLICT - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", return_value=report - ): - resp = client.post( - "/policy/check", json={"policy_text": "editors may read", "service_id": "svc-1"} - ) - - assert resp.status_code == 200 - body = resp.json() - assert body["status"] == "no_conflict" - assert body["conflicts"] == [] - assert body["unevaluated"] == [] - - -def test_conflicts_found_returns_200_not_422(): - # A found conflict is a recorded diagnosis, NOT a policy-input error — it must be 200, unlike - # the live /apply path which maps a contradiction to 422. - report = ConflictReport.from_survey([_conflict()], [], evaluated_count=1) - assert report.status is ConflictStatus.CONFLICTS_FOUND - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", return_value=report - ): - resp = client.post( - "/policy/check", json={"policy_text": "contradictory", "service_id": "svc-1"} - ) - - assert resp.status_code == 200 - body = resp.json() - assert body["status"] == "conflicts_found" - assert len(body["conflicts"]) == 1 - c = body["conflicts"][0] - assert c["kind"] == "direct" - assert c["role"]["name"] == "editor" - assert c["scope"]["name"] == "write" - assert c["granting_quotes"] == ["editors may write"] - assert c["prohibiting_quotes"] == ["editors must not write"] - - -def test_unevaluated_present_returns_200_and_not_no_conflict(): - # A partial run (some entity did not converge) must never look clean — status is forced away - # from no_conflict, and it is still a completed survey ⇒ 200. - report = ConflictReport.from_survey([], [_unevaluated()], evaluated_count=1) - assert report.status is not ConflictStatus.NO_CONFLICT - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", return_value=report - ): - resp = client.post( - "/policy/check", json={"policy_text": "some policy", "service_id": "svc-1"} - ) - - assert resp.status_code == 200 - body = resp.json() - assert body["status"] != "no_conflict" - assert len(body["unevaluated"]) == 1 - assert body["unevaluated"][0]["reason"] == "nonconvergence" - - -def test_incomplete_zero_evaluated_returns_200(): - # Zero focal entities evaluated (empty-input / no-focal case) ⇒ incomplete, still 200. - report = ConflictReport.from_survey([], [], evaluated_count=0) - assert report.status is ConflictStatus.INCOMPLETE - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", return_value=report - ): - resp = client.post( - "/policy/check", json={"policy_text": "some policy", "service_id": "svc-1"} - ) - - assert resp.status_code == 200 - assert resp.json()["status"] == "incomplete" - - -def test_pre_survey_http_502_propagates_with_no_report_body(): - # The resolver's IdP-unreachable boundary must escape the diagnostic unchanged: bare 502, - # no report (FastAPI renders the HTTPException as its default error body). - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", - side_effect=HTTPException(502, "IdP Configuration Service unavailable"), - ): - resp = client.post( - "/policy/check", json={"policy_text": "p", "service_id": "svc-down"} - ) - - assert resp.status_code == 502 - body = resp.json() - assert "status" not in body - assert "conflicts" not in body - - -def test_pre_survey_http_404_propagates_with_no_report_body(): - # Unknown-service boundary likewise propagates as a bare 404 with no report body. - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", - side_effect=HTTPException(404, "service not found in IdP catalog"), - ): - resp = client.post( - "/policy/check", json={"policy_text": "p", "service_id": "svc-missing"} - ) - - assert resp.status_code == 404 - body = resp.json() - assert "status" not in body - assert "conflicts" not in body - - -def test_missing_policy_text_is_422_validation_and_never_calls_survey(): - # policy_text is a required field on the request model — its absence is a FastAPI validation - # 422 (no report body), and the survey is never invoked. - with patch("aiac.agent.controller.routes.check_policy_conflicts") as survey: - resp = client.post("/policy/check", json={"service_id": "svc-1"}) - - assert resp.status_code == 422 - assert "status" not in resp.json() - survey.assert_not_called() - - -def test_route_calls_survey_with_posted_policy_text_and_service_id(): - # The thin shell forwards exactly what was posted to the use-case. - report = ConflictReport.from_survey([], [], evaluated_count=1) - with patch( - "aiac.agent.controller.routes.check_policy_conflicts", return_value=report - ) as survey: - resp = client.post( - "/policy/check", - json={"policy_text": "editors may read", "service_id": "svc-abc"}, - ) - - assert resp.status_code == 200 - survey.assert_called_once_with("editors may read", "svc-abc") diff --git a/aiac/test/agent/policy_check/__init__.py b/aiac/test/agent/policy_check/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/aiac/test/agent/policy_check/test_apply_conflict_regression.py b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py similarity index 95% rename from aiac/test/agent/policy_check/test_apply_conflict_regression.py rename to aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py index 82adb8f95..7af61eb89 100644 --- a/aiac/test/agent/policy_check/test_apply_conflict_regression.py +++ b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py @@ -5,7 +5,8 @@ diagnostic is a *separate* assembly and the safety-critical live ``/apply`` graph stays byte-for-byte unchanged, still **raising** ``PolicyContradictionError`` -> HTTP 422 on a genuine grant/deny contradiction (the diagnostic *records* instead). This guard pins both ends of that live -contract, independent of the #159 ``/policy/check`` route: +contract; it stays valid after ``/policy/check`` was retired (#2500) and the diagnostic engine was +re-homed as an internal library — the live ``/apply`` raise path is unaffected by that move: 1. **Builder level** -- with ``graph._structured_call`` patched so the auditor returns a genuine ``Contradiction``, ``build_role_rules`` RAISES ``PolicyContradictionError`` and returns no rule diff --git a/aiac/test/agent/policy_check/test_conflict_check_live_llm.py b/aiac/test/agent/policy_rules_builder/test_conflict_check_live_llm.py similarity index 99% rename from aiac/test/agent/policy_check/test_conflict_check_live_llm.py rename to aiac/test/agent/policy_rules_builder/test_conflict_check_live_llm.py index 4dfff10f8..77862a54b 100644 --- a/aiac/test/agent/policy_check/test_conflict_check_live_llm.py +++ b/aiac/test/agent/policy_rules_builder/test_conflict_check_live_llm.py @@ -35,7 +35,7 @@ from aiac.agent.policy_rules_builder.diagnostic import _verify_quote from aiac.agent.policy_rules_builder.diagnostic_models import ConflictStatus from aiac.idp.configuration.models import Role, Scope, Service, ServiceType, Subject -from aiac.agent.uc.policy_check.check import check_policy_conflicts +from aiac.agent.policy_rules_builder.diagnostic_survey import check_policy_conflicts from test.integration.launcher import require_env_or_skip pytestmark = [pytest.mark.integration, pytest.mark.llm] diff --git a/aiac/test/agent/uc/policy_check/test_check.py b/aiac/test/agent/policy_rules_builder/test_diagnostic_survey.py similarity index 99% rename from aiac/test/agent/uc/policy_check/test_check.py rename to aiac/test/agent/policy_rules_builder/test_diagnostic_survey.py index 34c393540..ba99f1783 100644 --- a/aiac/test/agent/uc/policy_check/test_check.py +++ b/aiac/test/agent/policy_rules_builder/test_diagnostic_survey.py @@ -31,7 +31,7 @@ ScopeSelection, ) from aiac.agent.shared import focal_entities -from aiac.agent.uc.policy_check.check import check_policy_conflicts +from aiac.agent.policy_rules_builder.diagnostic_survey import check_policy_conflicts from aiac.idp.configuration.models import RoleKind, Scope, Service, ServiceType, Subject from aiac.idp.configuration.models import Role as RoleModel diff --git a/aiac/test/agent/uc/policy_check/__init__.py b/aiac/test/agent/uc/policy_check/__init__.py deleted file mode 100644 index e69de29bb..000000000 From e3067752978c5198efc9219dadf83cc5db823529 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 10:13:23 +0000 Subject: [PATCH 03/13] Feat: Add Door B user-role-focal deny-only pass (#2501) Add a user-role-focal DENY-only pass to the service policy build. It fans the kind=User subset of candidate roles over the focus service's own scopes and emits DENY rules only, expressing a user role's exclusivity ("Testers may access only issues") as the exclusivity-complement prohibition the scope-focal pass structurally cannot produce. The scope-focal pass remains the single grant authority; Door B never emits an ALLOW. - graph.py: build_role_graph gains a deny_only variant whose build node keeps only DENY effects; new build_role_denies() entry point runs it. A permissive policy (no exclusivity, no explicit prohibition) yields [] -- a structural no-op, reusing the existing role-focal proposer/auditor prompt unchanged (no new LLM reasoning, no model upsize). - builder.py: run the pass at the focus's own-scope onboarding alongside the scope-focal pass, fanning each kind=User candidate role over own scopes. Order-independent (own scopes always exist at the service's own onboarding). - Tests: deny-only graph slices (exclusivity complement, permissive no-op, explicit prohibition); builder fan-out (per user role over own scopes, agent roles skipped, consistent-denyworld agreement with no conflict, order independence); one live-LLM exclusivity-complement case (-m llm). Conflict detection over the assembled rules is out of scope here (#2502/#2503): this pass only produces denies. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../aiac/agent/policy_rules_builder/graph.py | 52 ++++- .../uc/onboarding/policy_builder/builder.py | 15 +- .../agent/policy_rules_builder/test_graph.py | 98 ++++++++- .../test_graph_live_llm.py | 36 +++- .../onboarding/policy_builder/test_builder.py | 192 ++++++++++++++++-- 5 files changed, 365 insertions(+), 28 deletions(-) diff --git a/aiac/src/aiac/agent/policy_rules_builder/graph.py b/aiac/src/aiac/agent/policy_rules_builder/graph.py index 6f1474e5b..9b231b3d4 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/graph.py +++ b/aiac/src/aiac/agent/policy_rules_builder/graph.py @@ -302,7 +302,15 @@ def _assemble(state_type: type, propose, precheck, audit, build): return g.compile() -def build_role_graph(): +def build_role_graph(*, deny_only: bool = False): + """Role-focal PRB graph. With ``deny_only=True`` this is the **Door B** variant + (the user-role-focal deny pass): its build node emits **DENY rules only** — the + exclusivity complement plus any explicit prohibitions — and never an ALLOW, so the + scope-focal pass remains the single grant authority. The proposer/precheck/audit + nodes are byte-identical to the allow+deny variant (the LLM still extracts the + "X may access only Y" grant so the complement can be derived); only the build node + differs in which effects it keeps.""" + def propose(s: RoleRulesState) -> dict[str, Any]: return _propose( s, @@ -325,18 +333,23 @@ def audit(s: RoleRulesState) -> dict[str, Any]: ) def build(s: RoleRulesState) -> dict[str, Any]: - # ALLOW from granted names, DENY from explicit prohibitions -- every rule rebuilt from the - # typed scopes (never LLM string fields). Allows first, then denies, each in candidate order. + # DENY from the exclusivity complement + explicit prohibitions -- every rule rebuilt from + # the typed scopes (never LLM string fields), in candidate order. denied = _denied_names( s["denied_names"], s["exclusive"], [sc.name for sc in s["scopes"]], set(s["selected_names"]) ) + denies = [ + PolicyRule(role=s["role"], scope=sc, effect=RuleEffect.DENY) for sc in s["scopes"] if sc.name in denied + ] + if deny_only: + # Door B contributes only prohibitions; a purely permissive policy (no exclusivity, + # no explicit deny) yields [] -- a structural no-op that never broadens access. + return {"rules": denies} + # ALLOW from granted names first, then the denies -- each in candidate order. granted = set(s["selected_names"]) allows = [ PolicyRule(role=s["role"], scope=sc, effect=RuleEffect.ALLOW) for sc in s["scopes"] if sc.name in granted ] - denies = [ - PolicyRule(role=s["role"], scope=sc, effect=RuleEffect.DENY) for sc in s["scopes"] if sc.name in denied - ] return {"rules": allows + denies} return _assemble(RoleRulesState, propose, precheck, audit, build) @@ -381,6 +394,7 @@ def build(s: ScopeRulesState) -> dict[str, Any]: ROLE_GRAPH = build_role_graph() # module-level compile is safe (never builds the LLM) +ROLE_DENY_GRAPH = build_role_graph(deny_only=True) # Door B: user-role-focal deny-only variant SCOPE_GRAPH = build_scope_graph() @@ -402,6 +416,32 @@ def build_role_rules(role: Role, scopes: list[Scope]) -> list[PolicyRule]: return ROLE_GRAPH.invoke(state)["rules"] +def build_role_denies(role: Role, scopes: list[Scope]) -> list[PolicyRule]: + """Door B -- run the user-role-focal DENY-only pass for ``role`` over ``scopes``. + + Same role-focal graph as :func:`build_role_rules` (propose/precheck/audit), but the + build node emits **only DENY rules**: the derived exclusivity complement over ``scopes`` + plus any explicit prohibitions. It NEVER emits an ALLOW -- the scope-focal pass is the + single grant authority. A permissive policy (no exclusivity, no explicit prohibition) + returns ``[]``, so Door B is a structural no-op unless a user role's access is exclusive + or explicitly restricted.""" + state: RoleRulesState = { + "role": role, + "scopes": scopes, + "policy_text": "", + "selected_names": [], + "denied_names": [], + "conflict_names": [], + "exclusive": False, + "reasoning": "", + "approved": False, + "audit_feedback": None, + "retry_count": 0, + "rules": [], + } + return ROLE_DENY_GRAPH.invoke(state)["rules"] + + def build_scope_rules(roles: list[Role], scope: Scope) -> list[PolicyRule]: state: ScopeRulesState = { "roles": roles, diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index daed94df1..43e91fdc1 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -29,11 +29,11 @@ before. """ -from aiac.agent.policy_rules_builder.graph import build_role_rules, build_scope_rules +from aiac.agent.policy_rules_builder.graph import build_role_denies, build_role_rules, build_scope_rules from aiac.agent.shared.focal_entities import resolve_focal_entities from aiac.agent.shared.roles import flatten_role from aiac.idp.configuration.api import Configuration -from aiac.idp.configuration.models import ServiceType +from aiac.idp.configuration.models import RoleKind, ServiceType from aiac.policy.model.models import PolicyRule @@ -51,6 +51,17 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: rules: list[PolicyRule] = [] for scope in focal.own_scopes: rules.extend(build_scope_rules(focal.candidate_roles, scope)) + # Door B -- user-role-focal DENY-only pass at the focus's OWN-scope onboarding, alongside + # the scope-focal pass above. Fan the kind=User subset of the (already flattened+deduped) + # candidate roles over the focus's own scopes to surface each user role's exclusivity + # ("Testers may access only issues") as the DENY rules the scope-focal pass structurally + # cannot express. Deny-only: the scope-focal pass stays the single grant authority. Placing + # it here -- own scopes always exist at the service's own onboarding -- keeps it + # order-independent (tool-first vs agent-first yields identical denies), and produces both + # the scope-focal (role, own-scope) grant and the Door B (role, own-scope) prohibition in + # the same build. + for user_role in (r for r in focal.candidate_roles if r.kind is RoleKind.USER): + rules.extend(build_role_denies(user_role, focal.own_scopes)) if service_type is ServiceType.AGENT: for own_role in focal.own_roles: for role in flatten_role(own_role): diff --git a/aiac/test/agent/policy_rules_builder/test_graph.py b/aiac/test/agent/policy_rules_builder/test_graph.py index 6d521069a..d4b8fdefb 100644 --- a/aiac/test/agent/policy_rules_builder/test_graph.py +++ b/aiac/test/agent/policy_rules_builder/test_graph.py @@ -20,7 +20,7 @@ RoleSelection, ScopeSelection, _build_llm, - _build_llm, + build_role_denies, build_role_rules, build_scope_rules, ) @@ -747,3 +747,99 @@ def test_policy_block_labels_baseline_grants_only_and_scenario(): assert "SCENARIO POLICY" in human # The scenario text sits under the SCENARIO label, after the baseline. assert human.index("BASELINE POLICY") < human.index("SCENARIO POLICY") < human.index("SCEN-TEXT") + + +# --------------------------------------------------------------------------- # +# Door B — user-role-focal DENY-only pass (build_role_denies). Same graph as # +# build_role_rules (propose/precheck/audit), but the build node keeps ONLY the # +# DENY effects: the scope-focal pass stays the single grant authority, so Door # +# B never emits an ALLOW. These slices mirror the exclusivity/prohibition/ # +# permissive fixtures above but assert the deny-only projection. # +# --------------------------------------------------------------------------- # +def test_door_b_exclusivity_yields_complement_denies_only(): + # "Testers may access only issues" -> grant issues (exclusive) over the focus's own + # scopes {issues, source-read, source-write}; Door B emits the DERIVED complement DENY on + # every other own scope and DROPS the ALLOW(issues) that the scope-focal pass owns. + tester = _role("r-tst", "tester") + issues = _scope("s-iss", "issues") + source_read = _scope("s-sr", "source-read") + source_write = _scope("s-sw", "source-write") + + with ExitStack() as stack: + stack.enter_context(patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source())) + stack.enter_context( + patch( + "aiac.agent.policy_rules_builder.graph._structured_call", + side_effect=[ + RoleSelection( + granted_scope_names=["issues"], + denied_scope_names=[], + grant_is_exclusive=True, + reasoning="testers may access only issues", + ), + AuditVerdict(approved=True), + ], + ) + ) + rules = build_role_denies(tester, [issues, source_read, source_write]) + + # DENY-only: no ALLOW(issues); the complement (source-read, source-write) in candidate order. + assert rules == [ + PolicyRule(role=tester, scope=source_read, effect=RuleEffect.DENY), + PolicyRule(role=tester, scope=source_write, effect=RuleEffect.DENY), + ] + + +def test_door_b_permissive_policy_is_noop(): + # A non-exclusive grant with no explicit prohibition imposes nothing -> Door B returns []. + tester = _role("r-tst", "tester") + issues = _scope("s-iss", "issues") + source = _scope("s-src", "source") + + with ExitStack() as stack: + stack.enter_context(patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source())) + stack.enter_context( + patch( + "aiac.agent.policy_rules_builder.graph._structured_call", + side_effect=[ + RoleSelection( + granted_scope_names=["issues"], + denied_scope_names=[], + grant_is_exclusive=False, + reasoning="testers may access issues", + ), + AuditVerdict(approved=True), + ], + ) + ) + rules = build_role_denies(tester, [issues, source]) + + assert rules == [] + + +def test_door_b_explicit_prohibition_deny_only(): + # An explicit prohibition ("DevOps may not access source") emits the DENY and, being + # deny-only, contributes no ALLOW even if the proposer also named a grant. + devops = _role("r-ops", "devops") + source = _scope("s-src", "source") + issues = _scope("s-iss", "issues") + + with ExitStack() as stack: + stack.enter_context(patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source())) + stack.enter_context( + patch( + "aiac.agent.policy_rules_builder.graph._structured_call", + side_effect=[ + RoleSelection( + granted_scope_names=["issues"], + denied_scope_names=["source"], + grant_is_exclusive=False, + reasoning="devops may not access source", + ), + AuditVerdict(approved=True), + ], + ) + ) + rules = build_role_denies(devops, [source, issues]) + + assert rules == [PolicyRule(role=devops, scope=source, effect=RuleEffect.DENY)] diff --git a/aiac/test/agent/policy_rules_builder/test_graph_live_llm.py b/aiac/test/agent/policy_rules_builder/test_graph_live_llm.py index 980fae582..6fbc57ef8 100644 --- a/aiac/test/agent/policy_rules_builder/test_graph_live_llm.py +++ b/aiac/test/agent/policy_rules_builder/test_graph_live_llm.py @@ -32,7 +32,7 @@ import pytest -from aiac.agent.policy_rules_builder.graph import build_role_rules, build_scope_rules +from aiac.agent.policy_rules_builder.graph import build_role_denies, build_role_rules, build_scope_rules from aiac.idp.configuration.models import Role, Scope from aiac.policy.model.models import PolicyRule, RuleEffect from test.integration.launcher import require_env_or_skip @@ -78,6 +78,13 @@ def _role_rules(policy: str, role: Role, scopes: list[Scope]) -> list[PolicyRule return build_role_rules(role, scopes) +def _role_denies(policy: str, role: Role, scopes: list[Scope]) -> list[PolicyRule]: + """Run the Door B deny-only pass (build_role_denies) against the real LLM with `policy` as + the scenario text — the user-role-focal DENY-only projection of the role-focal graph.""" + with patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source(policy)): + return build_role_denies(role, scopes) + + def _scope_rules(policy: str, roles: list[Role], scope: Scope) -> list[PolicyRule]: """Run build_scope_rules against the real LLM with `policy` as the scenario text.""" with patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source(policy)): @@ -207,3 +214,30 @@ def test_exclusivity_derives_complement(): ("issues", DENY), ("deploy", DENY), } + + +# --------------------------------------------------------------------------- # +# Slice 6 — Door B (build_role_denies): the user-role-focal DENY-only pass over # +# the focus's OWN scopes. Real "Testers may access only issues" prose closes the # +# tester's set to issues, so the pass derives a DENY on every OTHER own scope # +# (source-read, source-write) and — being deny-only — emits NO ALLOW on issues # +# (the scope-focal pass owns that grant). This is the exclusivity-complement # +# prohibition the scope-focal pass structurally cannot express. # +# --------------------------------------------------------------------------- # +def test_door_b_exclusivity_complement_denies_only(): + tester = _role( + "r-tst", "tester", "A QA tester who works in the issue tracker, not in the source repository." + ) + issues = _scope("s-iss", "issues", "Access the issue tracker.") + source_read = _scope("s-sr", "source-read", "Read source code from the repository.") + source_write = _scope("s-sw", "source-write", "Write and modify source code in the repository.") + + policy = "Testers may access only issues; they may not access source." + + rules = _role_denies(policy, tester, [issues, source_read, source_write]) + + # DENY-only: the exclusivity complement over the focus's own scopes, no ALLOW(issues). + assert _scope_effects(rules) == { + ("source-read", DENY), + ("source-write", DENY), + } diff --git a/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py b/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py index 56918240f..a45b1f951 100644 --- a/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py +++ b/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py @@ -22,7 +22,7 @@ from aiac.agent.uc.onboarding.policy_builder import builder from aiac.idp.configuration.models import RoleKind, Scope, Service, ServiceType, Subject from aiac.idp.configuration.models import Role as RoleModel -from aiac.policy.model.models import PolicyRule +from aiac.policy.model.models import PolicyRule, RuleEffect FOCUS_ID = "svc-focus" OTHER_ID = "svc-other" @@ -82,6 +82,7 @@ def _invoke( service_id=FOCUS_ID, scope_rules=None, role_rules=None, + role_denies=None, get_services_exc=None, get_subjects_exc=None, ): @@ -90,14 +91,16 @@ def _invoke( `services` / `subjects` back `get_services()` / `get_subjects()` respectively. Both candidate roles and scopes are sourced from `get_services()`; `all_scopes` is retained only to describe the global scope catalog in each fixture and is not consulted by the - builder. `scope_rules` / `role_rules` are optional side_effect callables; default to - returning an empty list so calls are counted without inventing rule content. `get_*_exc` - injects an IdP-read failure on the corresponding call. + builder. `scope_rules` / `role_rules` / `role_denies` are optional side_effect callables + (scope-focal pass / agent role-focal pass / Door B user-role-focal deny pass respectively); + default to returning an empty list so calls are counted without inventing rule content. + `get_*_exc` injects an IdP-read failure on the corresponding call. """ with ( patch.object(builder, "_config") as cfg, patch.object(builder, "build_scope_rules") as bsr, patch.object(builder, "build_role_rules") as brr, + patch.object(builder, "build_role_denies") as brd, ): conf = MagicMock() if get_services_exc is not None: @@ -111,8 +114,9 @@ def _invoke( cfg.return_value = conf bsr.side_effect = scope_rules or (lambda roles, scope: []) brr.side_effect = role_rules or (lambda role, scopes: []) + brd.side_effect = role_denies or (lambda role, scopes: []) result = builder.ServicePolicyBuilder.build(service_id, service_type) - return result, bsr, brr, conf + return result, bsr, brr, brd, conf class TestTool: @@ -123,7 +127,7 @@ def test_single_scope_calls_build_scope_rules_once_and_merges(self): other = _service(OTHER_ID, roles=[other_role]) rule = _rule(other_role, own_scope) - result, bsr, brr, _ = _invoke( + result, bsr, brr, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[own_scope], @@ -146,7 +150,7 @@ def test_build_scope_rules_once_per_own_scope_and_results_merged(self): other = _service(OTHER_ID, roles=[other_role]) r1, r2 = _rule(other_role, s1), _rule(other_role, s2) - result, bsr, brr, _ = _invoke( + result, bsr, brr, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[s1, s2], @@ -171,7 +175,7 @@ def test_scope_rules_per_own_scope_and_role_rules_per_own_role(self): scope_rule = _rule(other_role, own_scope) role_rule = _rule(own_role, other_scope) - result, bsr, brr, _ = _invoke( + result, bsr, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus, other], all_scopes=[own_scope, other_scope], @@ -206,7 +210,7 @@ def test_composite_other_role_expanded_to_closure_deduped_by_id(self): focus = _service(FOCUS_ID, scopes=[own_scope]) other = _service(OTHER_ID, roles=[admin, reader]) - _, bsr, _, _ = _invoke( + _, bsr, _, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[own_scope], @@ -225,7 +229,7 @@ def test_composite_own_agent_role_calls_build_role_rules_per_closure_member(self focus = _service(FOCUS_ID, roles=[own_role], service_type=ServiceType.AGENT) other = _service(OTHER_ID, scopes=[other_scope]) - _, _, brr, _ = _invoke( + _, _, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus, other], all_scopes=[other_scope], @@ -249,7 +253,7 @@ def test_own_role_and_scope_excluded_from_other_universe_even_when_name_matches( focus = _service(FOCUS_ID, roles=[own_role], scopes=[own_scope], service_type=ServiceType.AGENT) other = _service(OTHER_ID, roles=[other_role], scopes=[other_scope]) - _, bsr, brr, _ = _invoke( + _, bsr, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus, other], all_scopes=[own_scope, other_scope], @@ -281,7 +285,7 @@ def test_no_own_role_in_any_scope_call_and_no_own_scope_in_any_role_call(self): focus = _service(FOCUS_ID, roles=own_roles, scopes=own_scopes, service_type=ServiceType.AGENT) other = _service(OTHER_ID, roles=other_roles, scopes=other_scopes) - _, bsr, brr, _ = _invoke( + _, bsr, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus, other], all_scopes=own_scopes + other_scopes, @@ -305,7 +309,7 @@ def test_role_owned_by_focus_and_held_by_user_is_excluded_from_candidates(self): focus = _service(FOCUS_ID, roles=[own_role], scopes=[own_scope]) other = _service(OTHER_ID) - _, bsr, _, _ = _invoke( + _, bsr, _, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[own_scope], @@ -324,7 +328,7 @@ def test_other_agent_role_carries_agent_kind_and_user_role_carries_user_kind(sel focus = _service(FOCUS_ID, scopes=[own_scope]) other = _service(OTHER_ID, roles=[other_role]) - result, bsr, _, _ = _invoke( + result, bsr, _, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[own_scope], @@ -344,7 +348,7 @@ def test_non_aiac_managed_own_scope_never_reaches_build_scope_rules(self): focus = _service(FOCUS_ID, scopes=[managed_scope, builtin_scope]) other = _service(OTHER_ID) - _, bsr, _, _ = _invoke( + _, bsr, _, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[managed_scope, builtin_scope], @@ -385,7 +389,7 @@ def test_focus_resolved_by_uuid_when_serviceid_differs(self): focus = _service(self.UUID, ref=self.CLIENT_ID, scopes=[own_scope]) other = _service(OTHER_ID, ref="svc-other-client") - result, bsr, _, _ = _invoke( + result, bsr, _, _, _ = _invoke( ServiceType.TOOL, services=[focus, other], all_scopes=[own_scope], @@ -417,7 +421,7 @@ class TestEmptyUniverse: def test_no_other_services_or_subjects_yields_no_rules_without_error(self): focus = _service(FOCUS_ID) - result, bsr, brr, _ = _invoke( + result, bsr, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus], all_scopes=[], @@ -432,7 +436,7 @@ def test_own_entities_only_invokes_prb_with_empty_lists(self): own_role, own_scope = _role("weather.agent"), _scope("weather.forecast", service_id=FOCUS_ID) focus = _service(FOCUS_ID, roles=[own_role], scopes=[own_scope], service_type=ServiceType.AGENT) - result, bsr, brr, _ = _invoke( + result, bsr, brr, _, _ = _invoke( ServiceType.AGENT, services=[focus], # no other services, no subjects all_scopes=[own_scope], @@ -487,3 +491,155 @@ def _boom(roles, scope): subjects=[], scope_rules=_boom, ) + + +def _deny(role, scope): + return PolicyRule(role=role, scope=scope, effect=RuleEffect.DENY) + + +class TestDoorB: + """Door B — the user-role-focal DENY-only pass fanned over the focus's OWN scopes, + alongside the scope-focal pass. It runs `build_role_denies` once per `kind=User` + candidate role (never for `kind=Agent` candidates), always with the focus's own scopes.""" + + def test_deny_pass_runs_per_user_role_over_own_scopes(self): + # github-tool owns two source scopes; candidates are a user role (tester) and an + # agent role. Door B fans ONLY the user role over the own scopes, deny-only. + source_read = _scope("source-read", service_id=FOCUS_ID) + source_write = _scope("source-write", service_id=FOCUS_ID) + tester = _role("tester", kind=RoleKind.USER, aiac_managed=False) + agent_role = _role("github.agent", kind=RoleKind.AGENT) + subject = _subject("tina", roles=[tester]) + focus = _service(FOCUS_ID, scopes=[source_read, source_write]) + other = _service(OTHER_ID, roles=[agent_role]) + + result, _, _, brd, _ = _invoke( + ServiceType.TOOL, + services=[focus, other], + all_scopes=[source_read, source_write], + subjects=[subject], + role_denies=lambda role, scopes: [_deny(role, sc) for sc in scopes], + ) + + # invoked once, for the user role only, with the focus's own scopes + assert brd.call_count == 1 + passed_role, passed_scopes = brd.call_args.args + assert passed_role.name == "tester" + assert [s.name for s in passed_scopes] == ["source-read", "source-write"] + # output is DENY-only + assert all(r.effect is RuleEffect.DENY for r in result) + assert {(r.role.name, r.scope.name) for r in result} == { + ("tester", "source-read"), + ("tester", "source-write"), + } + + def test_deny_pass_skips_agent_candidate_roles(self): + own_scope = _scope("source-read", service_id=FOCUS_ID) + agent_role = _role("github.agent", kind=RoleKind.AGENT) + focus = _service(FOCUS_ID, scopes=[own_scope]) + other = _service(OTHER_ID, roles=[agent_role]) + + _, _, _, brd, _ = _invoke( + ServiceType.TOOL, + services=[focus, other], + all_scopes=[own_scope], + subjects=[], + ) + + brd.assert_not_called() + + def test_permissive_policy_deny_pass_is_noop(self): + # A user role candidate with a permissive policy: build_role_denies returns [] and the + # assembled result carries no Door B deny. + own_scope = _scope("issues", service_id=FOCUS_ID) + tester = _role("tester", kind=RoleKind.USER, aiac_managed=False) + subject = _subject("tina", roles=[tester]) + focus = _service(FOCUS_ID, scopes=[own_scope]) + other = _service(OTHER_ID) + + result, _, _, brd, _ = _invoke( + ServiceType.TOOL, + services=[focus, other], + all_scopes=[own_scope], + subjects=[subject], + role_denies=lambda role, scopes: [], # exclusivity-free policy -> no denies + ) + + assert brd.call_count == 1 # the pass ran + assert result == [] # but contributed nothing + + def test_consistent_denyworld_both_passes_deny_same_pair_no_conflict(self): + # denyworld: the scope-focal pass (description-driven) and Door B (exclusivity complement) + # BOTH deny (tester, source). The assembled list carries the agreeing DENYs and NO pair + # holds both an ALLOW and a DENY -- consistent, so no conflict arises. + source = _scope("source-read", service_id=FOCUS_ID) + issues = _scope("issues-read", service_id=FOCUS_ID) + tester = _role("tester", kind=RoleKind.USER, aiac_managed=False) + subject = _subject("tina", roles=[tester]) + focus = _service(FOCUS_ID, scopes=[source, issues]) + other = _service(OTHER_ID) + + def _scope_side(roles, scope): + # scope-focal: grant issues, deny source (description-driven) for the tester. + if scope.name == "issues-read": + return [PolicyRule(role=tester, scope=scope, effect=RuleEffect.ALLOW)] + return [_deny(tester, scope)] + + def _deny_side(role, scopes): + # Door B: exclusivity "only issues" -> complement deny on source. + return [_deny(role, sc) for sc in scopes if sc.name == "source-read"] + + result, _, _, _, _ = _invoke( + ServiceType.TOOL, + services=[focus, other], + all_scopes=[source, issues], + subjects=[subject], + scope_rules=_scope_side, + role_denies=_deny_side, + ) + + allow_pairs = {(r.role.name, r.scope.name) for r in result if r.effect is RuleEffect.ALLOW} + deny_pairs = {(r.role.name, r.scope.name) for r in result if r.effect is RuleEffect.DENY} + # both passes agree on the (tester, source) DENY; issues is an ALLOW only + assert ("tester", "source-read") in deny_pairs + assert ("tester", "issues-read") in allow_pairs + # consistent: no pair carries BOTH an allow and a deny (no conflict) + assert allow_pairs.isdisjoint(deny_pairs) + + def test_order_independence_tool_first_vs_agent_first(self): + # build() is a pure function of its focus + the (identical) live IdP state: onboarding the + # tool first or the agent first yields identical denies for each focus. Door B runs at each + # service's OWN-scope onboarding, so the tool's user-role denies are produced by the tool's + # build regardless of whether the agent was onboarded before or after. + tool_scope = _scope("source-read", service_id=FOCUS_ID) + agent_scope = _scope("source_operations", service_id=OTHER_ID) + agent_role = _role("github.agent", role_id="agent-role-id", kind=RoleKind.AGENT) + tester = _role("tester", kind=RoleKind.USER, aiac_managed=False) + subject = _subject("tina", roles=[tester]) + tool = _service(FOCUS_ID, scopes=[tool_scope], service_type=ServiceType.TOOL) + agent = _service( + OTHER_ID, roles=[agent_role], scopes=[agent_scope], service_type=ServiceType.AGENT + ) + + def run(service_id, service_type): + result, _, _, _, _ = _invoke( + service_type, + services=[tool, agent], + all_scopes=[tool_scope, agent_scope], + subjects=[subject], + service_id=service_id, + role_denies=lambda role, scopes: [_deny(role, sc) for sc in scopes], + ) + return {(r.role.name, r.scope.name, r.effect) for r in result} + + # tool-first ordering + tool_a = run(FOCUS_ID, ServiceType.TOOL) + agent_a = run(OTHER_ID, ServiceType.AGENT) + # agent-first ordering + agent_b = run(OTHER_ID, ServiceType.AGENT) + tool_b = run(FOCUS_ID, ServiceType.TOOL) + + assert tool_a == tool_b + assert agent_a == agent_b + # the tool's build owns the (tester, source-read) deny in either order + assert ("tester", "source-read", RuleEffect.DENY) in tool_a From ae34bcf0016843ac5f4a274d48ae8a30689de85f Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 10:24:52 +0000 Subject: [PATCH 04/13] Feat: Add inline structural conflict detection with atomic raise (#2502) Detect allow/deny conflicts inline in the service build and make apply atomic. After all passes (scope-focal grants + Door B denies) are assembled into one rule list, ServicePolicyBuilder.build runs a pure, deterministic detect_conflicts -- the (role.id, scope.id) set-intersection of the ALLOW and DENY rule sets, no LLM. On overlap it raises a ConflictReport-carrying PolicyConflictError BEFORE the Orchestrator/Controller reach compute_and_apply, so a conflict leaves persisted state untouched (atomic-by-construction). Per ADR 0001 the detector surfaces, never reconciles. The raised report is the structural form: real ids, kind=DIRECT, focal on the SCOPE side, synthesized explanation, no quotes (quotes_verified=False). Verbatim-quote enrichment and the 422-body wiring are deferred to #2503; the existing PolicyContradictionError->422 handler (the disjoint LLM-auditor mechanism) is left intact. Tests: pure detect_conflicts unit suite (clean/overlap/order-independence/ id-vs-name/multi/empty) plus the repurposed apply-conflict regression seed, whose atomic-proof drives the real onboarding path and asserts compute_and_apply is never called on conflict (and is reached on a clean build). Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../conflict_detection.py | 89 +++++++++++ .../uc/onboarding/policy_builder/builder.py | 10 ++ .../test_apply_conflict_regression.py | 138 +++++++++++++++--- .../test_conflict_detection.py | 104 +++++++++++++ 4 files changed, 323 insertions(+), 18 deletions(-) create mode 100644 aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py create mode 100644 aiac/test/agent/policy_rules_builder/test_conflict_detection.py diff --git a/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py new file mode 100644 index 000000000..f8332bb1b --- /dev/null +++ b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py @@ -0,0 +1,89 @@ +"""Inline structural conflict detection for the assembled service policy (#2502). + +After a ``build()`` assembles every pass's output (scope-focal grants + the Door B +user-role-focal deny pass) into one ``list[PolicyRule]``, :func:`detect_conflicts` runs a +**pure, deterministic** allow∩deny set-intersection over ``(role.id, scope.id)``: a pair carrying +**both** an ``Allow`` and a ``Deny`` **is** a conflict. There is no LLM anywhere in this module. + +Per ADR 0001 (*identify-never-reconcile*) the detector NEVER merges, drops, or picks a winner — +it surfaces the overlap as a :class:`ConflictReport` so the build can **raise before** +``compute_and_apply`` (atomic-by-construction: a conflict leaves persisted state untouched). This +is the *cross-pass* structural **conflict**, distinct from the LLM auditor's *intra-pass* +``PolicyContradictionError`` (``graph.py``); the two are disjoint by construction. + +At this ticket the report is the **structural** form: real ids, ``kind=DIRECT``, a synthesized +``explanation``, no quotes (``quotes_verified=False``). ``Conflict.focal`` anchors on the SCOPE +side (settled design Q16). Verbatim-quote enrichment and the 422-body wiring land in #2503. +""" + +from aiac.policy.model.models import PolicyRule, RuleEffect + +from .diagnostic_models import ( + Conflict, + ConflictKind, + ConflictReport, + EntityRef, + FocalRef, + FocalType, +) + + +class PolicyConflictError(Exception): + """Raised by the build when the assembled rules both grant and prohibit the same + ``(role, scope)`` pair. Carries the structural :class:`ConflictReport` (real ids, ``DIRECT``, + no quotes) so the caller can surface it. Raised **before** ``compute_and_apply`` so nothing is + persisted (atomic-by-construction). This is a policy finding, not a builder fault — kept + separate from ``PolicyContradictionError`` (the LLM auditor's intra-pass mechanism).""" + + def __init__(self, report: ConflictReport): + self.report = report + pairs = ", ".join(f"({c.role.name}, {c.scope.name})" for c in report.conflicts) + super().__init__( + f"Policy conflict: {len(report.conflicts)} (role, scope) pair(s) both " + f"granted (Allow) and prohibited (Deny): {pairs}" + ) + + +def detect_conflicts(rules: list[PolicyRule]) -> ConflictReport: + """Pure, deterministic allow∩deny detection over the assembled rule list — **no LLM**. + + A conflict is a ``(role.id, scope.id)`` pair present in BOTH the ``Allow`` set and the ``Deny`` + set. Detection is keyed on ids only (names are for display), so it is order-independent: the + same rule list in any order yields the identical set of conflicts. Never reconciles — every + overlap is surfaced as a :class:`Conflict`. + + Returns a :class:`ConflictReport`; the caller raises iff ``report.conflicts`` is non-empty. + """ + allow: dict[tuple[str, str], PolicyRule] = {} + deny: dict[tuple[str, str], PolicyRule] = {} + for rule in rules: + key = (rule.role.id, rule.scope.id) + (deny if rule.effect is RuleEffect.DENY else allow)[key] = rule + + conflicts = [ + _to_conflict(allow[key]) for key in sorted(allow.keys() & deny.keys()) + ] + # evaluated_count = distinct (role, scope) pairs examined; non-zero when any pair exists so a + # clean list derives NO_CONFLICT (not INCOMPLETE). The raise decision only reads ``conflicts``. + return ConflictReport.from_survey( + conflicts, [], evaluated_count=len(allow.keys() | deny.keys()) + ) + + +def _to_conflict(rule: PolicyRule) -> Conflict: + """Build the structural :class:`Conflict` for one colliding ``(role, scope)`` pair. ``focal`` + anchors on the SCOPE side (Q16); quotes are empty and unverified (enrichment is #2503).""" + role, scope = rule.role, rule.scope + return Conflict( + focal=FocalRef(name=scope.name, id=scope.id, type=FocalType.SCOPE), + role=EntityRef(name=role.name, id=role.id), + scope=EntityRef(name=scope.name, id=scope.id), + kind=ConflictKind.DIRECT, + granting_quotes=[], + prohibiting_quotes=[], + explanation=( + f"Role '{role.name}' is both granted (Allow) and prohibited (Deny) on " + f"scope '{scope.name}' — a direct grant/deny conflict on the same (role, scope) pair." + ), + quotes_verified=False, + ) diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index 43e91fdc1..371edc10d 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -29,6 +29,7 @@ before. """ +from aiac.agent.policy_rules_builder.conflict_detection import PolicyConflictError, detect_conflicts from aiac.agent.policy_rules_builder.graph import build_role_denies, build_role_rules, build_scope_rules from aiac.agent.shared.focal_entities import resolve_focal_entities from aiac.agent.shared.roles import flatten_role @@ -66,4 +67,13 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: for own_role in focal.own_roles: for role in flatten_role(own_role): rules.extend(build_role_rules(role, focal.other_scopes)) + # Inline, deterministic (non-LLM) cross-pass conflict detection over the fully assembled + # rule list (scope-focal grants + Door B denies). Per ADR 0001 we surface, never reconcile: + # a (role, scope) carrying both an Allow and a Deny raises HERE -- before the Orchestrator/ + # Controller reach ``compute_and_apply`` -- so a conflict leaves persisted state untouched + # (atomic-by-construction). detection is order-independent (keyed on ids), so tool-first vs + # agent-first onboarding yields the identical outcome. + report = detect_conflicts(rules) + if report.conflicts: + raise PolicyConflictError(report) return rules diff --git a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py index 7af61eb89..690b5fad2 100644 --- a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py +++ b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py @@ -1,28 +1,34 @@ -"""Regression guard: the live ``/apply`` conflict path is UNCHANGED by the conflict-check diagnostic. - -This test is **deterministic** (NOT marked ``integration`` / ``llm``) so it runs in the default -suite and under ``-m "not integration"``. Feature #154's design rests on D1/D8 -- the read-only -diagnostic is a *separate* assembly and the safety-critical live ``/apply`` graph stays -byte-for-byte unchanged, still **raising** ``PolicyContradictionError`` -> HTTP 422 on a genuine -grant/deny contradiction (the diagnostic *records* instead). This guard pins both ends of that live -contract; it stays valid after ``/policy/check`` was retired (#2500) and the diagnostic engine was -re-homed as an internal library — the live ``/apply`` raise path is unaffected by that move: - - 1. **Builder level** -- with ``graph._structured_call`` patched so the auditor returns a genuine - ``Contradiction``, ``build_role_rules`` RAISES ``PolicyContradictionError`` and returns no rule - set (fail-closed). Mirrors ``test_graph.py``'s genuine-overlap slice. - 2. **Route level** -- ``POST /apply/service/{id}`` maps that ``PolicyContradictionError`` to HTTP - 422 and never reaches the PCE. ``onboard_service`` is patched to raise (so no cluster / LLM is - needed), mirroring how ``test/agent/controller/test_routes.py`` drives ``/apply``. +"""Apply-conflict guards: two disjoint mechanisms, both leaving persisted state untouched. + +Deterministic (NOT ``integration`` / ``llm``) so it runs under ``-m "not integration"``. This +file pins BOTH grant/deny mechanisms and proves each one raises **before** the PCE, so a policy +problem never mutates persisted state: + + A. **Intra-pass ``PolicyContradictionError``** (the LLM auditor, ``graph.py``) — a self- + contradicting single pass fails **closed** (raises, withholds its whole rule set). Kept + valid unchanged: it is a *separate* mechanism from the structural detector (#2502), and the + existing route handler still maps it to HTTP 422 without reaching the PCE. + B. **Cross-pass structural ``PolicyConflictError``** (#2502) — after ``ServicePolicyBuilder.build`` + assembles every pass's rules, the pure ``detect_conflicts`` allow∩deny intersection surfaces + a ``(role, scope)`` that is both granted and prohibited and **raises inside the build**, + before the Orchestrator/Controller reach ``compute_and_apply`` (atomic-by-construction). This + is the seed repurposed from the former apply-vs-diagnostic guard: its old premise (the #154 + read-only diagnostic doesn't touch ``/apply``) is moot after ``/policy/check`` was retired + (#2500); it now guards the inline structural raise instead. + +Both are proved with the apply seam (``compute_and_apply`` / the PCE) patched and asserted +**never called** on a conflict — the atomic proof — and the LLM/cluster stubbed out. """ from contextlib import ExitStack -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest from fastapi.testclient import TestClient from aiac.agent.controller.routes import app +from aiac.agent.policy_rules_builder.conflict_detection import PolicyConflictError +from aiac.agent.policy_rules_builder.diagnostic_models import ConflictStatus, FocalType from aiac.agent.policy_rules_builder.graph import ( AuditVerdict, Contradiction, @@ -30,9 +36,32 @@ RoleSelection, build_role_rules, ) -from aiac.idp.configuration.models import Role, Scope +from aiac.agent.uc.onboarding.policy_builder.builder import ServicePolicyBuilder +from aiac.idp.configuration.models import Role, RoleKind, Scope, ServiceType +from aiac.agent.shared.focal_entities import FocalEntitySet +from aiac.policy.model.models import PolicyRule, RuleEffect client = TestClient(app) +# Server-side exceptions with no route handler surface as 500 rather than propagating, so the +# atomic proof can assert on the PCE seam regardless of how #2503 later wires the 422 body. +tolerant_client = TestClient(app, raise_server_exceptions=False) + +_BUILDER = "aiac.agent.uc.onboarding.policy_builder.builder" + +_TESTER = Role(id="r-tester", name="tester", composite=False, kind=RoleKind.USER) +_ISSUES = Scope(id="s-iss", name="issues") + + +def _focal_own_scope() -> FocalEntitySet: + """A minimal focal set for a Tool onboarding: one own scope, one kind=User candidate role, + no own roles / other scopes — enough to drive the scope-focal grant + Door B deny passes.""" + return FocalEntitySet( + own_scopes=[_ISSUES], + own_roles=[], + candidate_roles=[_TESTER], + other_scopes=[], + service_type=ServiceType.TOOL, + ) class _Source: @@ -101,3 +130,76 @@ def test_apply_service_maps_policy_contradiction_to_422_and_skips_pce(): assert resp.status_code == 422 pce.assert_not_called() + + +# --- Mechanism B: cross-pass structural PolicyConflictError (#2502) -------------------------- + + +def test_build_raises_structural_conflict_from_assembled_passes(): + # The scope-focal pass grants (tester, issues) and the Door B deny pass prohibits the SAME + # (tester, issues): the assembled list carries both an Allow and a Deny on one pair. build() + # must run the inline detector and RAISE PolicyConflictError — never reconcile (ADR 0001). + with ( + patch(f"{_BUILDER}._config", return_value=MagicMock()), + patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), + patch( + f"{_BUILDER}.build_scope_rules", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + ), + patch( + f"{_BUILDER}.build_role_denies", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], + ), + ): + with pytest.raises(PolicyConflictError) as exc: + ServicePolicyBuilder.build("svc-tool", ServiceType.TOOL) + + report = exc.value.report + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert len(report.conflicts) == 1 + c = report.conflicts[0] + assert (c.role.id, c.scope.id) == ("r-tester", "s-iss") + assert c.focal.type is FocalType.SCOPE + assert c.quotes_verified is False + + +def _drive_apply_with_passes(scope_rules, deny_rules) -> MagicMock: + """Drive ``POST /apply/service/{id}`` through the REAL onboarding sequence (provision graph + stubbed to a Tool, the two PRB passes stubbed to the given rule lists, LLM/cluster untouched) + and return the patched ``compute_and_apply`` mock so the caller can assert on the PCE seam.""" + provision = MagicMock() + provision.invoke.return_value = {"service_type": ServiceType.TOOL} + with ( + patch( + "aiac.agent.uc.onboarding.orchestrator.build_provision_graph", return_value=provision + ), + patch(f"{_BUILDER}._config", return_value=MagicMock()), + patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), + patch(f"{_BUILDER}.build_scope_rules", return_value=scope_rules), + patch(f"{_BUILDER}.build_role_denies", return_value=deny_rules), + patch("aiac.agent.controller.routes.compute_and_apply") as pce, + ): + tolerant_client.post("/apply/service/svc-tool") + return pce + + +def test_conflict_raises_before_compute_and_apply_is_atomic(): + # ATOMIC PROOF: a conflicting build (Allow + Deny on the same pair) short-circuits inside + # build() — the PCE (the persistence seam) is provably NEVER reached, so a conflict leaves + # persisted state untouched. This exercises the real detect_conflicts, not a patched raise. + pce = _drive_apply_with_passes( + scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + deny_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], + ) + pce.assert_not_called() + + +def test_clean_build_reaches_compute_and_apply(): + # Control: the SAME path with no allow∩deny overlap (Door B contributes no deny) is clean — + # the build returns and the PCE IS reached. Proves the raise is conditional on a real conflict + # and that clean policies still apply. + pce = _drive_apply_with_passes( + scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + deny_rules=[], + ) + pce.assert_called_once() diff --git a/aiac/test/agent/policy_rules_builder/test_conflict_detection.py b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py new file mode 100644 index 000000000..3b85fae26 --- /dev/null +++ b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py @@ -0,0 +1,104 @@ +"""Unit tests for the pure, deterministic structural conflict detector (#2502). + +``detect_conflicts`` is a pure ``(role.id, scope.id)`` allow∩deny set-intersection over the +assembled ``list[PolicyRule]`` — **no LLM**. Per ADR 0001 it surfaces every overlap as a +``Conflict`` and never reconciles. These tests pin: clean → NO_CONFLICT, overlap → one DIRECT +scope-focal ``Conflict`` (real ids, no quotes), order-independence (keyed on ids), id-vs-name +discrimination, and the empty case. They are deterministic (NOT ``integration``/``llm``). +""" + +from aiac.agent.policy_rules_builder.conflict_detection import ( + PolicyConflictError, + detect_conflicts, +) +from aiac.agent.policy_rules_builder.diagnostic_models import ( + ConflictKind, + ConflictStatus, + FocalType, +) +from aiac.idp.configuration.models import Role, Scope +from aiac.policy.model.models import PolicyRule, RuleEffect + +_TESTER = Role(id="r-tester", name="tester", composite=False) +_DEV = Role(id="r-dev", name="developer", composite=False) +_ISSUES = Scope(id="s-iss", name="issues") +_SOURCE = Scope(id="s-src", name="source") + + +def _allow(role: Role, scope: Scope) -> PolicyRule: + return PolicyRule(role=role, scope=scope, effect=RuleEffect.ALLOW) + + +def _deny(role: Role, scope: Scope) -> PolicyRule: + return PolicyRule(role=role, scope=scope, effect=RuleEffect.DENY) + + +def test_clean_ruleset_has_no_conflict(): + # Distinct (role, scope) pairs across allow and deny -> no overlap -> NO_CONFLICT. + report = detect_conflicts([_allow(_DEV, _ISSUES), _deny(_TESTER, _SOURCE)]) + assert report.conflicts == [] + assert report.status is ConflictStatus.NO_CONFLICT + + +def test_empty_ruleset_reports_no_conflicts(): + report = detect_conflicts([]) + assert report.conflicts == [] + # Nothing to grant/prohibit means nothing collides; the raise decision reads ``conflicts``. + + +def test_direct_overlap_is_surfaced_as_scope_focal_conflict(): + report = detect_conflicts([_allow(_TESTER, _ISSUES), _deny(_TESTER, _ISSUES)]) + + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert len(report.conflicts) == 1 + c = report.conflicts[0] + # Real ids from the colliding rules, classified DIRECT, focal anchored on the SCOPE side (Q16). + assert (c.role.id, c.role.name) == ("r-tester", "tester") + assert (c.scope.id, c.scope.name) == ("s-iss", "issues") + assert c.focal.type is FocalType.SCOPE + assert (c.focal.id, c.focal.name) == ("s-iss", "issues") + assert c.kind is ConflictKind.DIRECT + # Structural form for this ticket: no quotes, unverified (enrichment is #2503). + assert c.granting_quotes == [] and c.prohibiting_quotes == [] + assert c.quotes_verified is False + assert "tester" in c.explanation and "issues" in c.explanation + + +def test_detection_is_order_independent(): + # Same rules, opposite orders (mirrors tool-first vs agent-first assembly) -> identical result. + rules = [_allow(_TESTER, _ISSUES), _deny(_TESTER, _ISSUES), _allow(_DEV, _SOURCE)] + forward = detect_conflicts(rules) + reverse = detect_conflicts(list(reversed(rules))) + key = lambda rep: sorted((c.role.id, c.scope.id) for c in rep.conflicts) + assert key(forward) == key(reverse) == [("r-tester", "s-iss")] + + +def test_overlap_keyed_on_ids_not_names(): + # Same name, different ids => NOT the same pair => no conflict (id-keyed, never name-keyed). + other_tester = Role(id="r-tester-2", name="tester", composite=False) + report = detect_conflicts([_allow(_TESTER, _ISSUES), _deny(other_tester, _ISSUES)]) + assert report.conflicts == [] + assert report.status is ConflictStatus.NO_CONFLICT + + +def test_multiple_conflicts_each_surfaced(): + report = detect_conflicts( + [ + _allow(_TESTER, _ISSUES), + _deny(_TESTER, _ISSUES), + _allow(_DEV, _SOURCE), + _deny(_DEV, _SOURCE), + ] + ) + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert sorted((c.role.id, c.scope.id) for c in report.conflicts) == [ + ("r-dev", "s-src"), + ("r-tester", "s-iss"), + ] + + +def test_error_carries_report(): + report = detect_conflicts([_allow(_TESTER, _ISSUES), _deny(_TESTER, _ISSUES)]) + err = PolicyConflictError(report) + assert err.report is report + assert "tester" in str(err) and "issues" in str(err) From abbc4f82450e17374a226885f49615144b5f82ee Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 10:41:32 +0000 Subject: [PATCH 05/13] Feat: Surface rich ConflictReport on /apply with unified 422 (#2503) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold the re-homed diagnostic explain/quote engine into /apply's conflict path. When the deterministic detect_conflicts (#2502) finds a structural allow∩deny overlap -- and ONLY then -- ServicePolicyBuilder.build() runs an LLM explain pass (conflict_enrichment.enrich_report) over exactly those pairs, classifying each kind (direct/coarse_scope) and extracting verbatim, substring-validated quotes from the candidate policy text. A clean apply stays fully deterministic and LLM-free (the explain seam never fires). Unify the 422 boundary on one ConflictReport shape (Q15): PolicyConflictError carries the enriched report directly; PolicyContradictionError is re-shaped by report_from_contradictions (no LLM, lower-fidelity: no ids, quotes_verified= false). Both handlers return 422 whose body is the structured report JSON. On any quote-validation failure the conflict is kept with quotes_verified=false and a description fallback -- surface, never reconcile (ADR 0001). Adds an ADR addendum recording the reversal of handoff-07 Q15/Q16 (quote-less / no-LLM). Tests: enrichment + verbatim/fallback + clean-apply-never-calls-explain-seam (deterministic), a route test asserting the ConflictReport 422 body for both exceptions, and one live-LLM case (-m integration -m llm) asserting containment plus substring-validity only. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../docs/adr/0001-identify-never-reconcile.md | 61 ++++++++ aiac/src/aiac/agent/controller/routes.py | 35 ++++- .../conflict_detection.py | 54 ++++++- .../conflict_enrichment.py | 70 +++++++++ .../uc/onboarding/policy_builder/builder.py | 8 + aiac/test/agent/controller/test_routes.py | 50 ++++++- .../test_apply_conflict_regression.py | 140 +++++++++++++++--- .../test_apply_enrichment_live_llm.py | 78 ++++++++++ 8 files changed, 458 insertions(+), 38 deletions(-) create mode 100644 aiac/docs/adr/0001-identify-never-reconcile.md create mode 100644 aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py create mode 100644 aiac/test/agent/policy_rules_builder/test_apply_enrichment_live_llm.py diff --git a/aiac/docs/adr/0001-identify-never-reconcile.md b/aiac/docs/adr/0001-identify-never-reconcile.md new file mode 100644 index 000000000..b3cee0015 --- /dev/null +++ b/aiac/docs/adr/0001-identify-never-reconcile.md @@ -0,0 +1,61 @@ +# 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 — + are a pre-existing gap left as a follow-up, not reconciled here. +- 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. diff --git a/aiac/src/aiac/agent/controller/routes.py b/aiac/src/aiac/agent/controller/routes.py index 85dc4e870..27582b925 100644 --- a/aiac/src/aiac/agent/controller/routes.py +++ b/aiac/src/aiac/agent/controller/routes.py @@ -18,6 +18,10 @@ from fastapi.responses import JSONResponse, Response from aiac.agent.eventbus.consumer import lifespan +from aiac.agent.policy_rules_builder.conflict_detection import ( + PolicyConflictError, + report_from_contradictions, +) from aiac.agent.policy_rules_builder.graph import ( PolicyContradictionError, PolicyRulesBuilderError, @@ -34,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 diff --git a/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py index f8332bb1b..850f01976 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py +++ b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py @@ -60,14 +60,56 @@ def detect_conflicts(rules: list[PolicyRule]) -> ConflictReport: key = (rule.role.id, rule.scope.id) (deny if rule.effect is RuleEffect.DENY else allow)[key] = rule - conflicts = [ - _to_conflict(allow[key]) for key in sorted(allow.keys() & deny.keys()) - ] + conflicts = [_to_conflict(allow[key]) for key in sorted(allow.keys() & deny.keys())] # evaluated_count = distinct (role, scope) pairs examined; non-zero when any pair exists so a # clean list derives NO_CONFLICT (not INCOMPLETE). The raise decision only reads ``conflicts``. - return ConflictReport.from_survey( - conflicts, [], evaluated_count=len(allow.keys() | deny.keys()) - ) + return ConflictReport.from_survey(conflicts, [], evaluated_count=len(allow.keys() | deny.keys())) + + +def report_from_contradictions(focal: str, contradictions) -> ConflictReport: + """Map an intra-pass ``PolicyContradictionError`` (the LLM auditor, ``graph.py``) into the SAME + :class:`ConflictReport` shape the structural detector produces, so the 422 boundary has ONE + report shape for both mechanisms (settled design Q15). **No LLM** — a shallow, deterministic + re-shape at the handler. + + Lower-fidelity by nature: an auditor :class:`Contradiction` carries only name-strings (no ids, + no quotes, no kind). So each colliding pair gets empty ids, ``kind=DIRECT``, empty quotes with + ``quotes_verified=False``, and the auditor ``description`` as the ``explanation``. ``focal`` is + parsed from the raise's focal string (``_role_focal`` / ``_scope_focal`` prefix) to recover the + axis and name; the candidate name goes on the opposite side.""" + if focal.startswith("role name="): + focal_type = FocalType.ROLE + focal_name = focal[len("role name=") :].split(":", 1)[0].strip() + elif focal.startswith("scope name="): + focal_type = FocalType.SCOPE + focal_name = focal[len("scope name=") :].split(":", 1)[0].strip() + else: + # Unrecognized focal string (e.g. a bare service token in a degenerate raise): anchor on + # the SCOPE side (Q16) and use the whole string as the focal name. + focal_type = FocalType.SCOPE + focal_name = focal + focal_ref = FocalRef(name=focal_name, id="", type=focal_type) + + conflicts: list[Conflict] = [] + for c in contradictions: + candidate = EntityRef(name=c.candidate_name, id="") + focal_entity = EntityRef(name=focal_name, id="") + role, scope = (focal_entity, candidate) if focal_type is FocalType.ROLE else (candidate, focal_entity) + conflicts.append( + Conflict( + focal=focal_ref, + role=role, + scope=scope, + kind=ConflictKind.DIRECT, + granting_quotes=[], + prohibiting_quotes=[], + explanation=c.description, + quotes_verified=False, + ) + ) + # evaluated_count=1: the focal entity WAS evaluated (the auditor ruled on it), so a non-empty + # mapping derives CONFLICTS_FOUND (conflicts non-empty wins the precedence regardless). + return ConflictReport.from_survey(conflicts, [], evaluated_count=1) def _to_conflict(rule: PolicyRule) -> Conflict: diff --git a/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py b/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py new file mode 100644 index 000000000..a19e52127 --- /dev/null +++ b/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py @@ -0,0 +1,70 @@ +"""LLM enrichment of a structural ``ConflictReport`` (#2503). + +``detect_conflicts`` (#2502, :mod:`conflict_detection`) finds allow∩deny ``(role, scope)`` overlaps +**deterministically** and returns a *structural* :class:`ConflictReport`: real ids, +``kind=DIRECT``, a synthesized ``explanation``, and no quotes (``quotes_verified=False``). This +module upgrades EXACTLY those already-found pairs — never a blind full re-survey — by running the +diagnostic ``explain``/quote pass over each one to (1) classify the ``kind`` (``direct`` vs +``coarse_scope``) and (2) extract verbatim, substring-validated ``granting_quotes`` / +``prohibiting_quotes`` from the candidate policy text. + +It is invoked by ``ServicePolicyBuilder.build()`` **only when** ``detect_conflicts`` returns +conflicts, so a clean apply makes ZERO explain-LLM calls (the explain seam is +:func:`_explain_pair`). The pairs are fixed (the structural detector's output); each is explained +once against the candidate ``policy_text``. On ANY quote-validation failure the conflict is KEPT +with ``quotes_verified=False`` and the explanation falls back to the structural synthesized +description (ADR 0001: surface, never drop — reversing handoff-07 Q15/Q16, which had decided the +on-``/apply`` report would be quote-less / no-LLM). +""" + +from aiac.policy.model.models import PolicyRule + +from .diagnostic import ExplainResult, _verify_quote +from .diagnostic_models import Conflict, ConflictReport +from .graph import _role_focal, _scope_focal, _structured_call +from .prompts import build_explain_messages + + +def _explain_pair(policy_text: str, role, scope, hint: str) -> ExplainResult: + """THE explain seam for enrichment — one structured LLM call per already-confirmed colliding + pair. Isolated so a deterministic test can patch it and assert it is NEVER called on a clean + apply, and so the same ``graph._structured_call`` transport-retry path drives it as the live + proposer/auditor. ``role`` / ``scope`` are the typed IdP objects (descriptions feed the prompt); + ``hint`` is the structural synthesized explanation used purely to locate/classify the collision.""" + return _structured_call( + ExplainResult, + build_explain_messages(policy_text, _role_focal(role), _scope_focal(scope), hint), + ) + + +def enrich_report(report: ConflictReport, rules: list[PolicyRule], policy_text: str) -> ConflictReport: + """Return ``report`` with every conflict enriched with a classified ``kind`` + validated quotes. + + ``rules`` is the assembled rule list — it supplies the full typed ``Role`` / ``Scope`` objects + (with descriptions) for each colliding ``(role.id, scope.id)`` pair; ``policy_text`` is the + candidate prose each quote is validated against with the engine's own :func:`_verify_quote` + (whitespace-normalized substring). Each conflict is explained exactly once (no full re-survey, + never abort on the first). Quotes are verified only when there is at least one quote AND every + quote is a verbatim substring; otherwise the conflict is kept with ``quotes_verified=False`` and + the explanation falls back to the structural synthesized description. ``status`` is unchanged + (the conflict set is neither grown nor shrunk — enrichment never reconciles).""" + typed = {(r.role.id, r.scope.id): (r.role, r.scope) for r in rules} + enriched: list[Conflict] = [] + for c in report.conflicts: + role, scope = typed[(c.role.id, c.scope.id)] + result = _explain_pair(policy_text, role, scope, c.explanation) + granting = list(result.granting_quotes) + prohibiting = list(result.prohibiting_quotes) + verified = bool(granting or prohibiting) and all(_verify_quote(q, policy_text) for q in granting + prohibiting) + enriched.append( + c.model_copy( + update={ + "kind": result.kind, + "granting_quotes": granting, + "prohibiting_quotes": prohibiting, + "explanation": result.explanation if verified else c.explanation, + "quotes_verified": verified, + } + ) + ) + return report.model_copy(update={"conflicts": enriched}) diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index 371edc10d..59ba86e21 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -30,7 +30,9 @@ """ from aiac.agent.policy_rules_builder.conflict_detection import PolicyConflictError, detect_conflicts +from aiac.agent.policy_rules_builder.conflict_enrichment import enrich_report from aiac.agent.policy_rules_builder.graph import build_role_denies, build_role_rules, build_scope_rules +from aiac.agent.policy_rules_builder.policy_source import get_policy_source from aiac.agent.shared.focal_entities import resolve_focal_entities from aiac.agent.shared.roles import flatten_role from aiac.idp.configuration.api import Configuration @@ -75,5 +77,11 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: # agent-first onboarding yields the identical outcome. report = detect_conflicts(rules) if report.conflicts: + # A conflict was found: NOW (and only now) run the LLM explain/quote survey over the + # exact pairs detect_conflicts surfaced -- classifying each kind and extracting verbatim, + # substring-validated quotes from the candidate policy text (#2503). Gating the LLM + # behind report.conflicts keeps a clean apply fully deterministic and LLM-free (the + # explain seam never fires). The policy source is read only on this path. + report = enrich_report(report, rules, get_policy_source().fetch()) raise PolicyConflictError(report) return rules diff --git a/aiac/test/agent/controller/test_routes.py b/aiac/test/agent/controller/test_routes.py index 2c4bb7fc4..f01eb6404 100644 --- a/aiac/test/agent/controller/test_routes.py +++ b/aiac/test/agent/controller/test_routes.py @@ -11,7 +11,13 @@ from fastapi.testclient import TestClient from aiac.agent.controller.routes import app +from aiac.agent.policy_rules_builder.conflict_detection import ( + PolicyConflictError, + detect_conflicts, +) +from aiac.agent.policy_rules_builder.diagnostic_models import ConflictStatus, FocalType from aiac.agent.policy_rules_builder.graph import ( + Contradiction, PolicyContradictionError, PolicyRulesBuilderError, ) @@ -214,12 +220,22 @@ def test_policy_rules_builder_error_surfaces_422_and_skips_pce(): pce.assert_not_called() -def test_policy_contradiction_error_surfaces_422_and_skips_pce(): - # A genuine grant/deny contradiction is likewise a policy finding surfaced as 422. +def test_policy_conflict_error_surfaces_422_with_conflict_report_body_and_skips_pce(): + # A structural PolicyConflictError (the cross-pass detector, already enriched) surfaces as 422 + # whose BODY IS the structured ConflictReport (not a bare {"detail": ...}), and the PCE is never + # reached. onboard_service raises the carrier exception so the route handler is exercised. + tester = Role(id="r-tester", name="tester", composite=False) + issues = Scope(id="s-iss", name="issues") + report = detect_conflicts( + [ + PolicyRule(role=tester, scope=issues, effect=RuleEffect.ALLOW), + PolicyRule(role=tester, scope=issues, effect=RuleEffect.DENY), + ] + ) with ( patch( "aiac.agent.controller.routes.onboard_service", - side_effect=PolicyContradictionError("focal-svc", []), + side_effect=PolicyConflictError(report), ), patch("aiac.agent.controller.routes.compute_and_apply") as pce, ): @@ -227,6 +243,34 @@ def test_policy_contradiction_error_surfaces_422_and_skips_pce(): assert resp.status_code == 422 pce.assert_not_called() + body = resp.json() + assert body["status"] == ConflictStatus.CONFLICTS_FOUND.value + assert len(body["conflicts"]) == 1 + c = body["conflicts"][0] + assert (c["role"]["id"], c["scope"]["id"]) == ("r-tester", "s-iss") + assert c["focal"]["type"] == FocalType.SCOPE.value + + +def test_policy_contradiction_error_surfaces_422_conflict_report_and_skips_pce(): + # A genuine intra-pass grant/deny contradiction (the LLM auditor) is likewise a policy finding + # surfaced as 422 — and mapped into the SAME ConflictReport body shape (Q15), with no LLM at the + # boundary (lower fidelity: no ids, quotes_verified=False, auditor description as explanation). + err = PolicyContradictionError( + "scope name=issues: the issue tracker", + [Contradiction(candidate_name="tester", description="granted and prohibited")], + ) + with ( + patch("aiac.agent.controller.routes.onboard_service", side_effect=err), + patch("aiac.agent.controller.routes.compute_and_apply") as pce, + ): + resp = client.post("/apply/service/svc-conflict") + + assert resp.status_code == 422 + pce.assert_not_called() + body = resp.json() + assert body["status"] == ConflictStatus.CONFLICTS_FOUND.value + assert body["conflicts"][0]["scope"]["name"] == "issues" + assert body["conflicts"][0]["quotes_verified"] is False # --------------------------------------------------------------------------- # diff --git a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py index 690b5fad2..0762ae380 100644 --- a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py +++ b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py @@ -28,7 +28,8 @@ from aiac.agent.controller.routes import app from aiac.agent.policy_rules_builder.conflict_detection import PolicyConflictError -from aiac.agent.policy_rules_builder.diagnostic_models import ConflictStatus, FocalType +from aiac.agent.policy_rules_builder.diagnostic import ExplainResult +from aiac.agent.policy_rules_builder.diagnostic_models import ConflictKind, ConflictStatus, FocalType from aiac.agent.policy_rules_builder.graph import ( AuditVerdict, Contradiction, @@ -36,9 +37,9 @@ RoleSelection, build_role_rules, ) +from aiac.agent.shared.focal_entities import FocalEntitySet from aiac.agent.uc.onboarding.policy_builder.builder import ServicePolicyBuilder from aiac.idp.configuration.models import Role, RoleKind, Scope, ServiceType -from aiac.agent.shared.focal_entities import FocalEntitySet from aiac.policy.model.models import PolicyRule, RuleEffect client = TestClient(app) @@ -47,6 +48,9 @@ tolerant_client = TestClient(app, raise_server_exceptions=False) _BUILDER = "aiac.agent.uc.onboarding.policy_builder.builder" +# The explain/LLM seam the enrichment pass runs through — patched here so the deterministic suite +# never touches a live endpoint, and asserted NEVER called on a clean apply. +_EXPLAIN_SEAM = "aiac.agent.policy_rules_builder.conflict_enrichment._explain_pair" _TESTER = Role(id="r-tester", name="tester", composite=False, kind=RoleKind.USER) _ISSUES = Scope(id="s-iss", name="issues") @@ -82,9 +86,7 @@ def test_live_build_role_rules_still_raises_policy_contradiction(): issues = Scope(id="s-iss", name="issues") with ExitStack() as stack: - stack.enter_context( - patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source()) - ) + stack.enter_context(patch("aiac.agent.policy_rules_builder.graph.get_policy_source", return_value=_Source())) stack.enter_context( patch( "aiac.agent.policy_rules_builder.graph._structured_call", @@ -115,21 +117,32 @@ def test_live_build_role_rules_still_raises_policy_contradiction(): assert [c.candidate_name for c in exc.value.contradictions] == ["issues"] -def test_apply_service_maps_policy_contradiction_to_422_and_skips_pce(): +def test_apply_service_maps_policy_contradiction_to_422_conflict_report_and_skips_pce(): # The Controller maps a PolicyContradictionError raised inside the onboarding handler to HTTP 422 - # (a policy finding, not a 500), and the PCE is never reached. onboard_service is patched to raise - # directly so the route mapping is exercised without a cluster or the LLM. + # (a policy finding, not a 500), and the PCE is never reached. The 422 body is now the SAME + # ConflictReport shape as the structural detector (Q15) — re-shaped from the auditor's name-string + # contradictions with NO LLM (lower fidelity: no ids, quotes_verified=False). onboard_service is + # patched to raise directly so the route mapping is exercised without a cluster or the LLM. + err = PolicyContradictionError( + "scope name=issues: the issue tracker", + [Contradiction(candidate_name="tester", description="granted and prohibited")], + ) with ( - patch( - "aiac.agent.controller.routes.onboard_service", - side_effect=PolicyContradictionError("focal-svc", []), - ), + patch("aiac.agent.controller.routes.onboard_service", side_effect=err), patch("aiac.agent.controller.routes.compute_and_apply") as pce, ): resp = client.post("/apply/service/svc-conflict") assert resp.status_code == 422 pce.assert_not_called() + # Body is a structured ConflictReport, not a bare {"detail": ...}. + body = resp.json() + assert body["status"] == ConflictStatus.CONFLICTS_FOUND.value + assert len(body["conflicts"]) == 1 + c = body["conflicts"][0] + assert c["focal"]["type"] == FocalType.SCOPE.value + assert c["scope"]["name"] == "issues" and c["role"]["name"] == "tester" + assert c["quotes_verified"] is False and c["explanation"] == "granted and prohibited" # --- Mechanism B: cross-pass structural PolicyConflictError (#2502) -------------------------- @@ -139,6 +152,8 @@ def test_build_raises_structural_conflict_from_assembled_passes(): # The scope-focal pass grants (tester, issues) and the Door B deny pass prohibits the SAME # (tester, issues): the assembled list carries both an Allow and a Deny on one pair. build() # must run the inline detector and RAISE PolicyConflictError — never reconcile (ADR 0001). + # Enrichment is stubbed to identity here (the explain seam / policy source are exercised by the + # dedicated enrichment test below); this pins the STRUCTURAL raise + report shape. with ( patch(f"{_BUILDER}._config", return_value=MagicMock()), patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), @@ -150,6 +165,8 @@ def test_build_raises_structural_conflict_from_assembled_passes(): f"{_BUILDER}.build_role_denies", return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], ), + patch(f"{_BUILDER}.get_policy_source", return_value=_Source()), + patch(f"{_BUILDER}.enrich_report", side_effect=lambda report, rules, text: report), ): with pytest.raises(PolicyConflictError) as exc: ServicePolicyBuilder.build("svc-tool", ServiceType.TOOL) @@ -163,43 +180,120 @@ def test_build_raises_structural_conflict_from_assembled_passes(): assert c.quotes_verified is False -def _drive_apply_with_passes(scope_rules, deny_rules) -> MagicMock: +def test_conflicting_build_enriches_report_with_kind_and_verbatim_quotes(): + # On a detected conflict, build() runs the enrichment pass over the structural report: the + # explain seam classifies the kind and returns quotes, which the engine substring-validates + # against the candidate policy text. The raised report carries the enriched conflict. + policy = "Testers may access issues. Testers must not access issues." + explained = ExplainResult( + kind=ConflictKind.COARSE_SCOPE, + granting_quotes=["Testers may access issues."], + prohibiting_quotes=["Testers must not access issues."], + explanation="issues is both granted and prohibited for tester", + ) + with ( + patch(f"{_BUILDER}._config", return_value=MagicMock()), + patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), + patch( + f"{_BUILDER}.build_scope_rules", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + ), + patch( + f"{_BUILDER}.build_role_denies", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], + ), + patch(f"{_BUILDER}.get_policy_source", return_value=_Source(policy)), + patch(_EXPLAIN_SEAM, return_value=explained) as explain, + ): + with pytest.raises(PolicyConflictError) as exc: + ServicePolicyBuilder.build("svc-tool", ServiceType.TOOL) + + explain.assert_called_once() # exactly one explain call for the one conflicting pair + c = exc.value.report.conflicts[0] + assert c.kind is ConflictKind.COARSE_SCOPE + assert c.granting_quotes == ["Testers may access issues."] + assert c.prohibiting_quotes == ["Testers must not access issues."] + assert c.quotes_verified is True + assert c.explanation == "issues is both granted and prohibited for tester" + + +def test_conflicting_build_falls_back_when_quotes_not_verbatim(): + # A non-verbatim quote (not a substring of the policy) fails validation: the conflict is KEPT + # with quotes_verified=False and the explanation falls back to the structural synthesized one. + policy = "Testers may access issues. Testers must not access issues." + explained = ExplainResult( + kind=ConflictKind.DIRECT, + granting_quotes=["Testers are allowed full access"], # paraphrase — NOT in the policy + prohibiting_quotes=[], + explanation="fabricated wording", + ) + with ( + patch(f"{_BUILDER}._config", return_value=MagicMock()), + patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), + patch( + f"{_BUILDER}.build_scope_rules", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + ), + patch( + f"{_BUILDER}.build_role_denies", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], + ), + patch(f"{_BUILDER}.get_policy_source", return_value=_Source(policy)), + patch(_EXPLAIN_SEAM, return_value=explained), + ): + with pytest.raises(PolicyConflictError) as exc: + ServicePolicyBuilder.build("svc-tool", ServiceType.TOOL) + + c = exc.value.report.conflicts[0] + assert c.quotes_verified is False + assert c.explanation != "fabricated wording" # fell back to the structural description + assert "tester" in c.explanation and "issues" in c.explanation + + +def _drive_apply_with_passes(scope_rules, deny_rules) -> tuple[MagicMock, MagicMock]: """Drive ``POST /apply/service/{id}`` through the REAL onboarding sequence (provision graph stubbed to a Tool, the two PRB passes stubbed to the given rule lists, LLM/cluster untouched) - and return the patched ``compute_and_apply`` mock so the caller can assert on the PCE seam.""" + and return ``(compute_and_apply, explain_seam)`` mocks so the caller can assert on the PCE seam + AND that the explain/LLM enrichment seam fired only when a conflict was detected. The policy + source is stubbed and the explain seam returns a fixed ExplainResult, so no live endpoint or + on-disk policy file is touched even on the conflicting path.""" provision = MagicMock() provision.invoke.return_value = {"service_type": ServiceType.TOOL} + explained = ExplainResult(kind=ConflictKind.DIRECT, granting_quotes=[], prohibiting_quotes=[]) with ( - patch( - "aiac.agent.uc.onboarding.orchestrator.build_provision_graph", return_value=provision - ), + patch("aiac.agent.uc.onboarding.orchestrator.build_provision_graph", return_value=provision), patch(f"{_BUILDER}._config", return_value=MagicMock()), patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), patch(f"{_BUILDER}.build_scope_rules", return_value=scope_rules), patch(f"{_BUILDER}.build_role_denies", return_value=deny_rules), + patch(f"{_BUILDER}.get_policy_source", return_value=_Source()), + patch(_EXPLAIN_SEAM, return_value=explained) as explain, patch("aiac.agent.controller.routes.compute_and_apply") as pce, ): tolerant_client.post("/apply/service/svc-tool") - return pce + return pce, explain def test_conflict_raises_before_compute_and_apply_is_atomic(): # ATOMIC PROOF: a conflicting build (Allow + Deny on the same pair) short-circuits inside # build() — the PCE (the persistence seam) is provably NEVER reached, so a conflict leaves # persisted state untouched. This exercises the real detect_conflicts, not a patched raise. - pce = _drive_apply_with_passes( + pce, explain = _drive_apply_with_passes( scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], deny_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], ) pce.assert_not_called() + explain.assert_called_once() # enrichment fired for the one conflicting pair -def test_clean_build_reaches_compute_and_apply(): +def test_clean_build_reaches_compute_and_apply_without_touching_the_llm_seam(): # Control: the SAME path with no allow∩deny overlap (Door B contributes no deny) is clean — - # the build returns and the PCE IS reached. Proves the raise is conditional on a real conflict - # and that clean policies still apply. - pce = _drive_apply_with_passes( + # the build returns and the PCE IS reached. Proves the raise is conditional on a real conflict, + # that clean policies still apply, AND that the explain/LLM enrichment seam is NEVER called on a + # clean apply (enrichment is gated strictly behind a detected structural conflict). + pce, explain = _drive_apply_with_passes( scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], deny_rules=[], ) pce.assert_called_once() + explain.assert_not_called() diff --git a/aiac/test/agent/policy_rules_builder/test_apply_enrichment_live_llm.py b/aiac/test/agent/policy_rules_builder/test_apply_enrichment_live_llm.py new file mode 100644 index 000000000..41f40176e --- /dev/null +++ b/aiac/test/agent/policy_rules_builder/test_apply_enrichment_live_llm.py @@ -0,0 +1,78 @@ +"""Live-LLM case for the ``/apply`` conflict-report enrichment (#2503). + +Mirrors ``test_conflict_check_live_llm.py`` / ``test_graph_live_llm.py``: it runs the **real** LLM +end-to-end through :func:`enrich_report` — the pass ``ServicePolicyBuilder.build()`` invokes ONLY +when the deterministic ``detect_conflicts`` finds a structural conflict — and asserts **structural** +properties of the enriched ``ConflictReport``: the planted colliding pair is present, its ``kind`` +is one of the two recognized kinds, and every extracted quote is a verbatim (whitespace-normalized) +substring of the candidate policy text. It deliberately does NOT assert exact quote strings or +explanation wording (model nondeterminism) — only containment + substring-validity. + +Nothing but the LLM endpoint is needed: ``enrich_report`` takes the assembled rules + policy text +directly, so there is no catalog / cluster / Keycloak to stub. The structural report is produced by +the real ``detect_conflicts`` over a hand-built allow∩deny overlap, exactly as the build would hand +it to enrichment. + +Gating: marked **both** ``integration`` and ``llm`` so the routine ``-m "not integration"`` run +deselects it, while ``-m llm`` selects it cluster-free. The autouse ``require_env_or_skip`` fixture +makes it **skip cleanly** (never crash, never false-pass) when the endpoint is unset. +""" + +import pytest + +from aiac.agent.policy_rules_builder.conflict_detection import detect_conflicts +from aiac.agent.policy_rules_builder.conflict_enrichment import enrich_report +from aiac.agent.policy_rules_builder.diagnostic import _verify_quote +from aiac.agent.policy_rules_builder.diagnostic_models import ConflictKind, ConflictStatus +from aiac.idp.configuration.models import Role, RoleKind, Scope +from aiac.policy.model.models import PolicyRule, RuleEffect +from test.integration.launcher import require_env_or_skip + +pytestmark = [pytest.mark.integration, pytest.mark.llm] + + +@pytest.fixture(autouse=True) +def _require_llm_env(): + """Skip the whole suite cleanly unless a real LLM endpoint is configured.""" + require_env_or_skip("LLM_BASE_URL", "LLM_MODEL", "LLM_API_KEY") + + +def test_enrichment_produces_verbatim_substring_quotes_for_the_conflicting_pair(): + # A genuine direct conflict on (tester, issues): the policy both grants and prohibits it. The + # structural detector surfaces the pair with no quotes; enrichment runs the real explain LLM and + # must return quotes that are verbatim substrings of the policy (containment + substring-valid). + tester = Role( + id="r-tester", + name="tester", + composite=False, + kind=RoleKind.USER, + description="A member of the QA team who tests the product.", + ) + issues = Scope( + id="s-iss", + name="issues", + description="Read and manage entries in the issue tracker.", + ) + policy = "Testers may access the issue tracker. Testers must not access the issue tracker." + + rules = [ + PolicyRule(role=tester, scope=issues, effect=RuleEffect.ALLOW), + PolicyRule(role=tester, scope=issues, effect=RuleEffect.DENY), + ] + structural = detect_conflicts(rules) + assert structural.status is ConflictStatus.CONFLICTS_FOUND # pre-condition: detector fired + + report = enrich_report(structural, rules, policy) + + # Containment: the planted pair is still present after enrichment (never dropped/reconciled). + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert ("r-tester", "s-iss") in {(c.role.id, c.scope.id) for c in report.conflicts} + + c = next(c for c in report.conflicts if (c.role.id, c.scope.id) == ("r-tester", "s-iss")) + assert c.kind in (ConflictKind.DIRECT, ConflictKind.COARSE_SCOPE) + # Substring-validity: whatever quotes the model returned must be verbatim substrings, and when + # it did return quotes the engine must have marked them verified. + for quote in c.granting_quotes + c.prohibiting_quotes: + assert _verify_quote(quote, policy), f"quote not a verbatim substring: {quote!r}" + if c.granting_quotes or c.prohibiting_quotes: + assert c.quotes_verified is True From fa6ce096e9597c84566f38fe0a27c34b6ca46288 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 12:31:47 +0000 Subject: [PATCH 06/13] Feat: Detect cross-service policy conflicts + integration OPA-loop test (#2504) Extend the inline structural conflict detector (#2502) to see across services. Today detect_conflicts only sees the rules one build() assembles, so an ALLOW derived by one service's build and a DENY another service has already applied for the SAME (role.id, scope.id) go unseen. Add a use-case layer gatherer, applied_rules_for_scopes, that reads the already-applied inbound rules (allow AND deny) of the services owning the scopes a build touches, from the Policy Store. ServicePolicyBuilder.build now unions those with its own rules and runs the SAME #2502 intersection over the combined (about-to-be-persisted) state, and the SAME #2503 enrichment over the union only on a hit. No new report shape: cross-service overlaps surface as the same ConflictReport/Conflict (real ids, FocalRef, kind, verbatim quotes). Identify-never-reconcile (ADR 0001) is preserved: the gatherer only widens the detector's input, never merges/dedupes/picks a winner, and is read-only, so build() still raises PolicyConflictError before compute_and_apply (atomic-by-construction). The LLM enrichment stays gated behind a detected conflict, so a clean multi-service apply is fully deterministic and LLM-free. The gatherer lives in the onboarding use-case layer (next to builder.py), NOT the PRB package, keeping the PRB store-free (test_isolation). Tests: - Unit: applied_rules_for_scopes reads both inbound lists of each distinct scope owner once, tolerates a brand-new empty SPM, skips owner-less scopes. - Detection: the #2502 core surfaces an allow-deny overlap spanning the two sides (combined input), clean when disjoint, order-independent. - Regression: build() raises on a cross-service overlap read from the store (enrichment fires, PCE never reached) and a clean cross-service apply reaches the PCE with the LLM seam untouched; store seam patched to [] in the within-service cases so they are unchanged. - Integration (-m integration + llm, deselected by -m "not integration"): drive two colliding onboardings through the real in-cluster Controller and assert the second POST /apply/service returns 422 with a ConflictReport; skips cleanly when the pipeline/env is absent. Deterministic suite: 684 passed, 161 deselected. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../uc/onboarding/policy_builder/builder.py | 30 ++- .../policy_builder/cross_service.py | 49 +++++ .../test_apply_conflict_regression.py | 93 ++++++++- .../test_conflict_detection.py | 47 +++++ .../onboarding/policy_builder/test_builder.py | 5 + .../policy_builder/test_cross_service.py | 94 +++++++++ ...test_uc1_onboard_cross_service_conflict.py | 194 ++++++++++++++++++ 7 files changed, 502 insertions(+), 10 deletions(-) create mode 100644 aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py create mode 100644 aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py create mode 100644 aiac/test/integration/test_uc1_onboard_cross_service_conflict.py diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index 59ba86e21..9f26ee8dc 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -35,6 +35,7 @@ from aiac.agent.policy_rules_builder.policy_source import get_policy_source from aiac.agent.shared.focal_entities import resolve_focal_entities from aiac.agent.shared.roles import flatten_role +from aiac.agent.uc.onboarding.policy_builder.cross_service import applied_rules_for_scopes from aiac.idp.configuration.api import Configuration from aiac.idp.configuration.models import RoleKind, ServiceType from aiac.policy.model.models import PolicyRule @@ -69,19 +70,32 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: for own_role in focal.own_roles: for role in flatten_role(own_role): rules.extend(build_role_rules(role, focal.other_scopes)) - # Inline, deterministic (non-LLM) cross-pass conflict detection over the fully assembled - # rule list (scope-focal grants + Door B denies). Per ADR 0001 we surface, never reconcile: - # a (role, scope) carrying both an Allow and a Deny raises HERE -- before the Orchestrator/ - # Controller reach ``compute_and_apply`` -- so a conflict leaves persisted state untouched - # (atomic-by-construction). detection is order-independent (keyed on ids), so tool-first vs + # Inline, deterministic (non-LLM) conflict detection over the COMBINED state (#2504): this + # build's fully assembled rules (scope-focal grants + Door B denies) PLUS the already-applied + # inbound rules of the OTHER services on the scopes this build touches, read from the Policy + # Store. Widening the input this way lets the SAME #2502 allow∩deny intersection surface a + # cross-service overlap -- an Allow here and a Deny another service already applied on the + # same (role.id, scope.id) -- that a single build's own rules could never reveal. Onboarding + # appends (override=False), so ``combined`` is exactly the post-apply persisted state. + # + # Per ADR 0001 we surface, never reconcile: a (role, scope) carrying both an Allow and a Deny + # raises HERE -- before the Orchestrator/Controller reach ``compute_and_apply`` -- so a + # conflict leaves persisted state untouched (atomic-by-construction; the store read above is + # side-effect-free). Detection is order-independent (keyed on ids), so tool-first vs # agent-first onboarding yields the identical outcome. - report = detect_conflicts(rules) + combined = rules + applied_rules_for_scopes(rules) + report = detect_conflicts(combined) if report.conflicts: # A conflict was found: NOW (and only now) run the LLM explain/quote survey over the # exact pairs detect_conflicts surfaced -- classifying each kind and extracting verbatim, # substring-validated quotes from the candidate policy text (#2503). Gating the LLM # behind report.conflicts keeps a clean apply fully deterministic and LLM-free (the - # explain seam never fires). The policy source is read only on this path. - report = enrich_report(report, rules, get_policy_source().fetch()) + # explain seam never fires) -- true for a clean cross-service apply too. ``combined`` is + # passed so enrichment can resolve the typed Role/Scope of a pair whose sides came from + # different services (a conflict may join a new rule to a stored one). Policy source is + # read only on this path. + report = enrich_report(report, combined, get_policy_source().fetch()) raise PolicyConflictError(report) + # Only this build's own rules are applied; the OTHER services' rules read above are already + # persisted and are used solely to widen detection, never re-emitted. return rules diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py new file mode 100644 index 000000000..6d7ac54e3 --- /dev/null +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py @@ -0,0 +1,49 @@ +"""Cross-service conflict awareness for the assembled service policy (#2504). + +``detect_conflicts`` (#2502, :mod:`aiac.agent.policy_rules_builder.conflict_detection`) is a pure +allow∩deny set-intersection over the rules **one** ``build()`` assembles — so it sees only the +service currently being applied. An ``Allow`` produced by one build and a ``Deny`` already applied +by another for the SAME ``(role.id, scope.id)`` pair therefore go unseen: each build looks only at +its own output. This module supplies the missing half — the **already-applied** rules of the OTHER +services, read from the Policy Store — so the builder can run the very same #2502 intersection over +the COMBINED (about-to-be-persisted) state and surface a cross-service overlap with the SAME +:class:`ConflictReport` shape. + +It lives in the onboarding **use-case** layer (next to ``builder.py``), NOT in the PRB package: +the PRB stays pure and store-free (guarded by ``test_isolation``), and reading already-applied +state is a use-case concern — the same layer that afterwards drives ``compute_and_apply``. + +It never merges, dedupes, or picks a winner (ADR 0001 — *identify, never reconcile*): it only +**widens the input** the deterministic detector sees. There is no LLM here and no new report shape +— the combined list feeds ``detect_conflicts`` (#2502) and, only on a hit, ``enrich_report`` +(#2503) unchanged. It is read-only (it mutates no store state), so the atomicity guarantee holds: +the builder still raises **before** ``compute_and_apply``. +""" + +from aiac.policy.model.models import PolicyRule +from aiac.policy.model_store.library.api import get_service_policy + + +def applied_rules_for_scopes(rules: list[PolicyRule]) -> list[PolicyRule]: + """Read every already-applied inbound rule (``Allow`` AND ``Deny``) on the scopes the + about-to-be-applied ``rules`` touch, from the Policy Store — the OTHER services' contribution + to the combined state the detector must see. + + The PCE persists every rule as an inbound edge on ``SPM(scope.serviceId)`` — the service that + *owns* the rule's scope. So the already-applied rules that could collide with a new + ``(role, scope)`` rule are exactly the inbound edges of the SPMs owning the scopes this build + touches. We fetch each such SPM **once** (deduped, sorted for determinism) and return its + combined inbound allow+deny edges. Onboarding applies append-only (``override=False``), so those + edges are precisely what persists alongside the new rules — making the union the honest + post-apply state to detect over. + + A brand-new scope owner has no stored SPM (``get_service_policy`` returns a fresh empty SPM on + 404), contributing nothing. Order-independent and side-effect-free (reads only) — it never + mutates the store, preserving the build's atomic-before-``compute_and_apply`` guarantee.""" + owners = sorted({rule.scope.serviceId for rule in rules if rule.scope.serviceId}) + applied: list[PolicyRule] = [] + for owner in owners: + spm = get_service_policy(owner) + applied.extend(spm.inbound_allow_rules) + applied.extend(spm.inbound_deny_rules) + return applied diff --git a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py index 0762ae380..23d06d715 100644 --- a/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py +++ b/aiac/test/agent/policy_rules_builder/test_apply_conflict_regression.py @@ -51,6 +51,10 @@ # The explain/LLM seam the enrichment pass runs through — patched here so the deterministic suite # never touches a live endpoint, and asserted NEVER called on a clean apply. _EXPLAIN_SEAM = "aiac.agent.policy_rules_builder.conflict_enrichment._explain_pair" +# The Policy-Store read #2504 added to widen detection across services. Patched to [] in the +# within-service cases below (no other services applied) so they need no live store and their +# structural assertions are unchanged; the cross-service cases patch it to return applied rules. +_APPLIED_SEAM = f"{_BUILDER}.applied_rules_for_scopes" _TESTER = Role(id="r-tester", name="tester", composite=False, kind=RoleKind.USER) _ISSUES = Scope(id="s-iss", name="issues") @@ -166,6 +170,7 @@ def test_build_raises_structural_conflict_from_assembled_passes(): return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], ), patch(f"{_BUILDER}.get_policy_source", return_value=_Source()), + patch(_APPLIED_SEAM, return_value=[]), patch(f"{_BUILDER}.enrich_report", side_effect=lambda report, rules, text: report), ): with pytest.raises(PolicyConflictError) as exc: @@ -203,6 +208,7 @@ def test_conflicting_build_enriches_report_with_kind_and_verbatim_quotes(): return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], ), patch(f"{_BUILDER}.get_policy_source", return_value=_Source(policy)), + patch(_APPLIED_SEAM, return_value=[]), patch(_EXPLAIN_SEAM, return_value=explained) as explain, ): with pytest.raises(PolicyConflictError) as exc: @@ -239,6 +245,7 @@ def test_conflicting_build_falls_back_when_quotes_not_verbatim(): return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY)], ), patch(f"{_BUILDER}.get_policy_source", return_value=_Source(policy)), + patch(_APPLIED_SEAM, return_value=[]), patch(_EXPLAIN_SEAM, return_value=explained), ): with pytest.raises(PolicyConflictError) as exc: @@ -250,11 +257,13 @@ def test_conflicting_build_falls_back_when_quotes_not_verbatim(): assert "tester" in c.explanation and "issues" in c.explanation -def _drive_apply_with_passes(scope_rules, deny_rules) -> tuple[MagicMock, MagicMock]: +def _drive_apply_with_passes(scope_rules, deny_rules, applied=None) -> tuple[MagicMock, MagicMock]: """Drive ``POST /apply/service/{id}`` through the REAL onboarding sequence (provision graph stubbed to a Tool, the two PRB passes stubbed to the given rule lists, LLM/cluster untouched) and return ``(compute_and_apply, explain_seam)`` mocks so the caller can assert on the PCE seam - AND that the explain/LLM enrichment seam fired only when a conflict was detected. The policy + AND that the explain/LLM enrichment seam fired only when a conflict was detected. ``applied`` is + the OTHER services' already-applied rules the #2504 store read returns (default ``[]`` — a + single-service apply); pass a conflicting rule to exercise the cross-service path. The policy source is stubbed and the explain seam returns a fixed ExplainResult, so no live endpoint or on-disk policy file is touched even on the conflicting path.""" provision = MagicMock() @@ -267,6 +276,7 @@ def _drive_apply_with_passes(scope_rules, deny_rules) -> tuple[MagicMock, MagicM patch(f"{_BUILDER}.build_scope_rules", return_value=scope_rules), patch(f"{_BUILDER}.build_role_denies", return_value=deny_rules), patch(f"{_BUILDER}.get_policy_source", return_value=_Source()), + patch(_APPLIED_SEAM, return_value=applied or []), patch(_EXPLAIN_SEAM, return_value=explained) as explain, patch("aiac.agent.controller.routes.compute_and_apply") as pce, ): @@ -297,3 +307,82 @@ def test_clean_build_reaches_compute_and_apply_without_touching_the_llm_seam(): ) pce.assert_called_once() explain.assert_not_called() + + +# --- Cross-service structural conflict (#2504) ----------------------------------------------- +# +# The SAME structural mechanism (B), but the colliding Deny comes from ANOTHER service's already- +# applied rules (the store read the #2504 gatherer performs), not from this build's Door B pass. +# This build grants (tester, issues); the store already carries a Deny another service applied on +# the SAME (role.id, scope.id). Neither side alone reveals it — only the union that build() feeds +# to detect_conflicts does. These pin that cross-service detection raises with the same shape, +# fires enrichment on the joined pair, and stays atomic (PCE never reached), while a clean cross- +# service apply reaches the PCE with the LLM seam untouched. + +# Another service's already-applied Deny on the same scope id (returned by the store read seam). +_APPLIED_DENY = PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.DENY) + + +def test_build_raises_cross_service_conflict_from_applied_store_rules(): + # This build's ONLY rule is an Allow (Door B contributes no deny), so its own rule set is clean. + # The store read returns another service's applied Deny on the same (tester, issues): build() + # unions the two and the #2502 core surfaces the overlap and RAISES — proving detection sees + # across services, not just within one build. Enrichment fires over the joined pair. + policy = "Testers may access issues." + explained = ExplainResult( + kind=ConflictKind.DIRECT, + granting_quotes=["Testers may access issues."], + prohibiting_quotes=[], + explanation="issues is granted here but already prohibited for tester", + ) + with ( + patch(f"{_BUILDER}._config", return_value=MagicMock()), + patch(f"{_BUILDER}.resolve_focal_entities", return_value=_focal_own_scope()), + patch( + f"{_BUILDER}.build_scope_rules", + return_value=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + ), + patch(f"{_BUILDER}.build_role_denies", return_value=[]), + patch(f"{_BUILDER}.get_policy_source", return_value=_Source(policy)), + patch(_APPLIED_SEAM, return_value=[_APPLIED_DENY]), + patch(_EXPLAIN_SEAM, return_value=explained) as explain, + ): + with pytest.raises(PolicyConflictError) as exc: + ServicePolicyBuilder.build("svc-tool", ServiceType.TOOL) + + explain.assert_called_once() # enrichment fired for the one cross-service pair + report = exc.value.report + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert len(report.conflicts) == 1 + c = report.conflicts[0] + assert (c.role.id, c.scope.id) == ("r-tester", "s-iss") + assert c.focal.type is FocalType.SCOPE + + +def test_cross_service_conflict_raises_before_compute_and_apply_is_atomic(): + # ATOMIC PROOF (cross-service): build() grants (tester, issues) with no own deny, but the store + # already carries another service's Deny on the same pair. The union conflicts, so build() short- + # circuits and the PCE is provably NEVER reached — a cross-service conflict leaves persisted state + # untouched. Exercises the real detect_conflicts over the combined set, not a patched raise. + pce, explain = _drive_apply_with_passes( + scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + deny_rules=[], + applied=[_APPLIED_DENY], + ) + pce.assert_not_called() + explain.assert_called_once() # enrichment fired for the one cross-service pair + + +def test_clean_cross_service_apply_reaches_pce_without_touching_the_llm_seam(): + # Control (cross-service): this build grants (tester, issues) and another service's applied rules + # are disjoint (a deny on a DIFFERENT role). The union has no allow∩deny overlap — the build + # returns, the PCE IS reached, and the explain/LLM seam is NEVER called. Proves a clean multi- + # service apply stays deterministic and LLM-free. + other_role = Role(id="r-dev", name="developer", composite=False, kind=RoleKind.USER) + pce, explain = _drive_apply_with_passes( + scope_rules=[PolicyRule(role=_TESTER, scope=_ISSUES, effect=RuleEffect.ALLOW)], + deny_rules=[], + applied=[PolicyRule(role=other_role, scope=_ISSUES, effect=RuleEffect.DENY)], + ) + pce.assert_called_once() + explain.assert_not_called() diff --git a/aiac/test/agent/policy_rules_builder/test_conflict_detection.py b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py index 3b85fae26..f62b79026 100644 --- a/aiac/test/agent/policy_rules_builder/test_conflict_detection.py +++ b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py @@ -102,3 +102,50 @@ def test_error_carries_report(): err = PolicyConflictError(report) assert err.report is report assert "tester" in str(err) and "issues" in str(err) + + +# --- Cross-service (#2504): the SAME core over a combined rule set ----------------------------- +# +# Cross-service detection reuses this exact pure core (#2502): the builder simply widens the input +# to ``this build's rules + the OTHER services' already-applied rules`` (see ``cross_service``) and +# calls ``detect_conflicts`` over the union. These tests pin that the core surfaces an allow∩deny +# that spans the two sides with the SAME ``ConflictReport`` shape (real ids, DIRECT, scope-focal) — +# no new report type is needed for the cross-service case. + + +def test_combined_ruleset_surfaces_cross_service_overlap(): + # service_a's build grants (tester, issues); service_b already applied a deny on the SAME + # (role.id, scope.id). Neither build's own rules alone reveal it — only the union does. The core + # surfaces exactly one DIRECT, scope-focal Conflict with the real ids, identical to a within- + # service overlap: cross-service differs only in which side each rule came from. + service_a_rules = [_allow(_TESTER, _ISSUES)] + service_b_applied = [_deny(_TESTER, _ISSUES)] + report = detect_conflicts(service_a_rules + service_b_applied) + + assert report.status is ConflictStatus.CONFLICTS_FOUND + assert len(report.conflicts) == 1 + c = report.conflicts[0] + assert (c.role.id, c.scope.id) == ("r-tester", "s-iss") + assert c.focal.type is FocalType.SCOPE + assert c.kind is ConflictKind.DIRECT + assert c.quotes_verified is False + + +def test_combined_ruleset_clean_when_sides_disjoint(): + # A clean cross-service apply: service_b's applied rules touch different pairs, so the union has + # no allow∩deny — NO_CONFLICT, and the builder would never fire the LLM enrichment seam. + service_a_rules = [_allow(_TESTER, _ISSUES)] + service_b_applied = [_allow(_DEV, _SOURCE), _deny(_DEV, _ISSUES)] + report = detect_conflicts(service_a_rules + service_b_applied) + assert report.conflicts == [] + assert report.status is ConflictStatus.NO_CONFLICT + + +def test_combined_detection_is_order_independent_across_sides(): + # Whether this build's rules or the applied rules come first, the union yields the identical + # conflict set (keyed on ids) — onboarding order / read order cannot change the outcome. + build_rules = [_allow(_TESTER, _ISSUES)] + applied = [_deny(_TESTER, _ISSUES), _allow(_DEV, _SOURCE)] + key = lambda rep: sorted((c.role.id, c.scope.id) for c in rep.conflicts) + assert key(detect_conflicts(build_rules + applied)) == key(detect_conflicts(applied + build_rules)) + assert key(detect_conflicts(build_rules + applied)) == [("r-tester", "s-iss")] diff --git a/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py b/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py index a45b1f951..dc6a31e4a 100644 --- a/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py +++ b/aiac/test/agent/uc/onboarding/policy_builder/test_builder.py @@ -101,6 +101,11 @@ def _invoke( patch.object(builder, "build_scope_rules") as bsr, patch.object(builder, "build_role_rules") as brr, patch.object(builder, "build_role_denies") as brd, + # #2504: build() now reads the OTHER services' already-applied rules from the Policy Store to + # widen cross-service conflict detection. These unit tests exercise a single build in + # isolation (no other services applied), so stub the store read to [] — no store required and + # the assembled-rule assertions below are unchanged. The cross-service path has its own tests. + patch.object(builder, "applied_rules_for_scopes", return_value=[]), ): conf = MagicMock() if get_services_exc is not None: diff --git a/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py b/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py new file mode 100644 index 000000000..564784c75 --- /dev/null +++ b/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py @@ -0,0 +1,94 @@ +"""Unit tests for the cross-service applied-rule gatherer (#2504). + +``applied_rules_for_scopes`` supplies the OTHER half of cross-service conflict detection: the +already-applied inbound rules (allow AND deny) of the services that OWN the scopes a build touches, +read from the Policy Store. The builder unions them with its own rules and runs the #2502 core over +the combined state, so this gatherer must (a) read exactly the owning SPMs, once each, (b) return +BOTH inbound lists, (c) never mutate the store, and (d) tolerate a brand-new owner (empty SPM). +These are deterministic (NOT ``integration``/``llm``): the store read is patched, no HTTP happens. + +It lives in the onboarding use-case layer (next to ``builder.py``), NOT the PRB package, so the +PRB stays store-free (``test_isolation``) — this test mirrors that source location. +""" + +from unittest.mock import patch + +from aiac.agent.uc.onboarding.policy_builder.cross_service import applied_rules_for_scopes +from aiac.idp.configuration.models import Role, Scope +from aiac.policy.model.models import PolicyRule, RuleEffect, ServicePolicyModel, ServiceType + +_CROSS = "aiac.agent.uc.onboarding.policy_builder.cross_service" + +_TESTER = Role(id="r-tester", name="tester", composite=False) +_DEV = Role(id="r-dev", name="developer", composite=False) +# Two scopes owned by two different services (serviceId is the SPM key / owner). +_ISSUES = Scope(id="s-iss", name="issues", serviceId="svc-tool") +_SOURCE = Scope(id="s-src", name="source", serviceId="svc-other") + + +def _allow(role: Role, scope: Scope) -> PolicyRule: + return PolicyRule(role=role, scope=scope, effect=RuleEffect.ALLOW) + + +def _deny(role: Role, scope: Scope) -> PolicyRule: + return PolicyRule(role=role, scope=scope, effect=RuleEffect.DENY) + + +def _spm(service_id: str, *, allow=None, deny=None) -> ServicePolicyModel: + return ServicePolicyModel( + service_id=service_id, + service_type=ServiceType.TOOL, + owned_roles=[], + owned_scopes=[], + inbound_allow_rules=allow or [], + inbound_deny_rules=deny or [], + ) + + +def test_reads_both_inbound_lists_of_the_scope_owner(): + # The owning SPM already carries a deny on (tester, issues); the gatherer returns it (and any + # allow) so the builder can intersect it against a fresh allow on the same pair. + stored = _spm("svc-tool", allow=[_allow(_DEV, _ISSUES)], deny=[_deny(_TESTER, _ISSUES)]) + with patch(f"{_CROSS}.get_service_policy", return_value=stored) as get: + applied = applied_rules_for_scopes([_allow(_TESTER, _ISSUES)]) + + get.assert_called_once_with("svc-tool") # exactly the scope owner, once + assert {(r.role.id, r.scope.id, r.effect) for r in applied} == { + ("r-dev", "s-iss", RuleEffect.ALLOW), + ("r-tester", "s-iss", RuleEffect.DENY), + } + + +def test_reads_each_distinct_owner_once(): + # Rules touching two scopes owned by two services -> one read per distinct owner (deduped), + # deterministic (sorted) order. + def _by_id(service_id: str) -> ServicePolicyModel: + return _spm(service_id, deny=[_deny(_TESTER, _ISSUES if service_id == "svc-tool" else _SOURCE)]) + + with patch(f"{_CROSS}.get_service_policy", side_effect=_by_id) as get: + applied = applied_rules_for_scopes( + [_allow(_TESTER, _ISSUES), _allow(_DEV, _SOURCE), _deny(_DEV, _ISSUES)] + ) + + assert sorted(c.args[0] for c in get.call_args_list) == ["svc-other", "svc-tool"] + assert len(applied) == 2 + + +def test_new_owner_with_empty_spm_contributes_nothing(): + # A brand-new scope owner has no stored edges (the store returns a fresh empty SPM on 404). + with patch(f"{_CROSS}.get_service_policy", return_value=_spm("svc-tool")): + assert applied_rules_for_scopes([_allow(_TESTER, _ISSUES)]) == [] + + +def test_scopes_without_serviceid_are_skipped(): + # A scope with no resolved owner has no SPM to read — it is simply skipped, never a store call. + orphan = Scope(id="s-orphan", name="orphan", serviceId="") + with patch(f"{_CROSS}.get_service_policy") as get: + assert applied_rules_for_scopes([_allow(_TESTER, orphan)]) == [] + get.assert_not_called() + + +def test_empty_rules_reads_nothing(): + with patch(f"{_CROSS}.get_service_policy") as get: + assert applied_rules_for_scopes([]) == [] + get.assert_not_called() diff --git a/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py new file mode 100644 index 000000000..5b384485d --- /dev/null +++ b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py @@ -0,0 +1,194 @@ +"""Cross-service conflict integration test (#2504) — the live-cluster twin of the deterministic +cross-service unit/regression tests. + +The deterministic suite proves the #2504 machinery in isolation (``applied_rules_for_scopes`` reads +the OTHER services' already-applied rules from the Policy Store, the builder unions them with its own +and runs the #2502 detector, and a cross-service overlap raises ``PolicyConflictError`` **before** +``compute_and_apply``). This test closes the loop end-to-end against the **real in-cluster UC-1 +Controller**: it drives two onboardings whose derived rules genuinely collide **across services**, +and asserts the second ``POST /apply/service/{id}`` is refused with **HTTP 422 whose body is a +``ConflictReport``** — the same boundary shape the deterministic ``routes`` test pins. + +Why the conflict is caught at ONBOARDING, before OPA. A cross-service conflict short-circuits inside +``ServicePolicyBuilder.build`` — the atomicity guarantee (ADR 0001): the builder raises before the +Orchestrator/Controller reach the PCE, so **no ``AuthorizationPolicy`` CR is ever upserted** and the +conflicting policy never reaches the deployed OPA plugin. The "OPA loop" here is therefore exercised +only to the extent that this is the *same live pipeline / same Controller* the OPA-loop rungs drive +(``uc1_onboard`` harness) — the assertion is the pre-apply 422, which is precisely the observable +proof that a conflict leaves the enforced OPA state untouched. + +How the two onboardings collide **across services** (the #2504 store read is the load-bearing seam): + + * **Phase 1 — onboard the TOOL under an exclusivity policy** ("testers may access only issues; they + may not access source"). The tool's Door B deny pass emits ``DENY (tester -> github-tool.source-*)`` + and the PCE persists it as an **inbound deny edge on ``SPM(github-tool)``** — the service that owns + those scopes. Tool onboarding itself is clean (its own scope-focal grants and Door B denies are + disjoint), so it returns 200 and the deny is now **applied state**. + * **Phase 2 — onboard the AGENT under a policy that GRANTS testers source** ("testers may read and + write source"). The agent's outbound *subject* gate derives ``ALLOW (tester -> github-tool.source-*)`` + — a ``(user role -> tool scope)`` rule routed onto the **same** ``SPM(github-tool)``, on the **same** + ``(role.id, scope.id)`` the tool already denied. The agent's own build carries no deny (the policy + states no prohibition), so this is invisible within the agent's rules alone. #2504's + ``applied_rules_for_scopes`` reads ``SPM(github-tool)``'s already-applied inbound deny, the builder + unions it with the fresh allow, and the #2502 detector surfaces the overlap -> ``PolicyConflictError`` + -> the Controller maps it to **422 + ``ConflictReport``**. + +The store is cleared **once** before phase 1 and NOT between the phases — the whole point is that the +agent's build sees the tool's already-persisted rule (append-only, ``override=False``). This is why the +flow composes the harness primitives directly instead of ``uc1.onboarded_stack`` (which clears the +store on entry). + +The exact ``(role, scope)`` the LLM-driven PRB lands on depends on live provisioning, so the assertion +is **structural**: HTTP 422, ``status == conflicts_found``, at least one conflict carrying a real role +and scope and the ``ConflictReport`` quote fields — not a hardcoded id (which would make the test +brittle against the live role/scope universe). That is enough to prove the cross-service overlap was +surfaced through the real pipeline in the shared ``ConflictReport`` shape. + +Run (needs a live rossoctl/Kind cluster with the AuthBridge OPA pipeline wired in — see +``k8s/opa-kind-runbook.md`` / ``k8s/opa-kind-enable.sh`` — the demo workloads deployed + registered +into ``AIAC_TEST_REALM``, a real LLM in-pod, and ``test/integration/.env`` sourced). It also drives the +real PRB LLM, so it is marked both ``integration`` and ``llm``: + + .venv/bin/pytest test/integration/test_uc1_onboard_cross_service_conflict.py -m integration -v + +Without ``-m integration`` the suite is not collected; without a wired cluster / env it skips cleanly. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest +import requests + +# ``integration`` so ``-m "not integration"`` deselects it (no external services in the routine run); +# ``llm`` because phase 2 drives the real PRB LLM end-to-end, so it can also be selected on its own. +pytestmark = [pytest.mark.integration, pytest.mark.llm] + +HERE = Path(__file__).resolve().parent # test/integration/ +REPO_ROOT = HERE.parents[1] # -> aiac/ +sys.path.insert(0, str(REPO_ROOT)) # so ``import test.integration.*`` resolves + +from test.integration import scenario_uc1 as scn # noqa: E402 +from test.integration import uc1_onboard as uc1 # noqa: E402 + +TEST_REALM = uc1.TEST_REALM +NAMESPACE = uc1.NAMESPACE + + +# --- The two colliding policies (mounted one at a time, like the denyworld harness) ---------- +# +# Phase 1: the tool's exclusivity deny — "only issues" prohibits testers from source, so the tool's +# Door B pass emits DENY(tester -> github-tool.source-*), persisted on SPM(github-tool). +POLICY_TOOL_EXCLUSIVE = """\ +Grant access narrowly and state the exclusive scoping that constrains it. + +- Testers may access only issues; they may not access source. +""" + +# Phase 2: the agent grants testers source — the outbound subject gate derives +# ALLOW(tester -> github-tool.source-*) on the SAME tool scopes the tool already denied. This +# directly contradicts the tool's phase-1 applied deny (the cross-service overlap #2504 surfaces). +POLICY_AGENT_GRANTS_SOURCE = """\ +Grant access on the basis of what each role does. + +- Testers may read and write source. +""" + + +def _onboard_expect_conflict(base_url: str, service_id: str) -> dict: + """``POST /apply/service/{service_id}`` and assert it is refused with HTTP 422 whose body is a + ``ConflictReport``; return the parsed body. The cross-service twin of ``uc1.onboard`` (which asserts + 200): here the agent's fresh allow collides with the tool's already-applied deny, so the build must + raise ``PolicyConflictError`` and the Controller must map it to 422 — never 200 (which would mean the + conflict slipped through and a CR was upserted), never 500 (an unhandled error).""" + resp = requests.post(f"{base_url}/apply/service/{service_id}", timeout=uc1.ONBOARD_TIMEOUT) + assert resp.status_code == 422, ( + f"cross-service onboard of {service_id!r}: expected HTTP 422 (conflict surfaced before apply), " + f"got {resp.status_code} — {resp.text[:800]}" + ) + return resp.json() + + +def _onboard_via_fresh_controller(policy_md: str, workload: str, *, expect_conflict: bool) -> dict | None: + """Mount ``policy_md`` on the Controller (rolling it so the PRB reads the new prose), then onboard + ``workload`` against the freshly-resolved live Controller pod. + + Each phase re-mounts the policy and re-resolves the Controller pod because ``ensure_agent_policy`` + rolls the Deployment on a prose change: binding the port-forward to the current live pod (not the + Service, which can still route to a lingering ``Terminating`` pod) avoids dropping the long onboard + POST mid-flight — the same rationale as ``uc1.resolve_controller_pod``. Returns the parsed 422 + ``ConflictReport`` body when ``expect_conflict`` (else ``None`` after asserting a clean 200).""" + uc1.ensure_agent_policy(uc1.CONTROLLER_NAMESPACE, policy_md=policy_md) + service_id = uc1.resolve_service_id(uc1.connect_admin(), TEST_REALM, f"{NAMESPACE}/{workload}") + controller_target = ( + uc1.CONTROLLER_TARGET + if os.environ.get("AIAC_CONTROLLER_TARGET") + else f"pod/{uc1.resolve_controller_pod()}" + ) + with uc1.port_forward( + controller_target, + namespace=uc1.CONTROLLER_NAMESPACE, + local_port=uc1.CONTROLLER_LOCAL_PORT, + remote_port=uc1.CONTROLLER_REMOTE_PORT, + ready_url=f"http://127.0.0.1:{uc1.CONTROLLER_LOCAL_PORT}/health", + ) as base_url: + if expect_conflict: + return _onboard_expect_conflict(base_url, service_id) + uc1.onboard(base_url, service_id) # phase 1 must be clean (asserts 200) + return None + + +def test_cross_service_conflict_is_surfaced_as_422_conflict_report() -> None: + """End-to-end #2504: onboard the tool under an exclusivity policy (persisting a tool-side deny), + then onboard the agent under a policy that grants the same testers source — the agent's outbound + subject allow collides across services with the tool's already-applied deny. The real Controller + must refuse the second onboarding with 422 + a ``ConflictReport``, and the atomicity guarantee holds + (the conflicting policy never reaches OPA — no CR is upserted on the raising path). + + Skips cleanly (never false-passes) when the pipeline is not wired or the integration env is unset. + The store is cleared once up front and NOT between phases, so the agent's build genuinely reads the + tool's persisted rule (the #2504 seam under test).""" + # Skip gates first — before any cluster mutation (acceptance: skip, never false-pass). + uc1.require_pipeline(namespace=NAMESPACE, workloads=[scn.AGENT_WORKLOAD, scn.TOOL_WORKLOAD]) + creds = uc1.require_env_or_skip("KEYCLOAK_URL", "KEYCLOAK_ADMIN_USERNAME", "KEYCLOAK_ADMIN_PASSWORD") + keycloak_url = creds["KEYCLOAK_URL"] + + admin = uc1.connect_admin() + uc1.delete_agent_cr() # clean policy slate (drop any prior run's CR) + uc1.cleanup_provisioned(admin, TEST_REALM) # clean slate (Keycloak) + uc1.clear_policy_store() # clean slate ONCE — NOT between phases (the agent must see the tool's rule) + uc1.provision_realm_and_users(admin, TEST_REALM) # PRB reads the role universe + uc1.verify_subject_mapper( + keycloak_url=keycloak_url, realm=TEST_REALM, user="test-user", password=scn.USER_PASSWORD + ) + + try: + # Phase 1 — tool onboarding under the exclusivity policy: clean (200), persists the tool-side + # DENY(tester -> github-tool.source-*) on SPM(github-tool). + _onboard_via_fresh_controller(POLICY_TOOL_EXCLUSIVE, scn.TOOL_WORKLOAD, expect_conflict=False) + + # Phase 2 — agent onboarding under the source-granting policy: the outbound subject gate's fresh + # ALLOW(tester -> github-tool.source-*) collides with the tool's already-applied DENY on the same + # (role, scope). The #2504 store read surfaces it -> 422 + ConflictReport. + report = _onboard_via_fresh_controller( + POLICY_AGENT_GRANTS_SOURCE, scn.AGENT_WORKLOAD, expect_conflict=True + ) + finally: + uc1.delete_agent_cr() # after — drop any CR (there should be none on the raising path) + uc1.cleanup_provisioned(admin, TEST_REALM) # restore the pre-run Keycloak state + + # The 422 body is the shared ConflictReport shape (same as the deterministic routes test), carrying + # at least one cross-service conflict with a real role + scope. Structural (not id-pinned) so it is + # robust against the live role/scope universe. + assert report is not None + assert report["status"] == "conflicts_found", report + assert report["conflicts"], f"expected >=1 surfaced conflict, got: {report}" + c = report["conflicts"][0] + assert c["role"]["id"] and c["role"]["name"], c + assert c["scope"]["id"] and c["scope"]["name"], c + # The report always carries the enrichment quote fields (possibly empty / unverified), proving the + # boundary emitted the full ConflictReport, not a bare {"detail": ...}. + assert "granting_quotes" in c and "prohibiting_quotes" in c and "quotes_verified" in c, c From fdda9b5d2a7a3777de602c4895d0b2c81c48bea2 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 13:02:56 +0000 Subject: [PATCH 07/13] Docs: Add AIAC domain glossary (CONTEXT.md) Adds the AIAC feature glossary referenced by docs/adr/0001-identify-never-reconcile.md: fixes the vocabulary for focal entities, the scope-focal grant authority, Door B denies, contradiction (intra-pass) vs conflict (cross-pass), within-batch vs cross-service conflicts, and the identify-never-reconcile principle. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- aiac/CONTEXT.md | 66 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 aiac/CONTEXT.md diff --git a/aiac/CONTEXT.md b/aiac/CONTEXT.md new file mode 100644 index 000000000..5fb4dd2ca --- /dev/null +++ b/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 deny and the Door B deny (and any collision +with a grant) 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. From 52db817306e843a5c3041c80a3662f08bbb81dc5 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 14:43:31 +0000 Subject: [PATCH 08/13] Fix: Address CodeRabbit review on #842 (enrichment fallback, empty-quote guard, docs) Applies the actionable CodeRabbit findings from PR #842: - builder.py: enrichment (policy fetch + LLM explain/quote) is now best-effort on the conflict path. A fetch/enrich failure was escaping past the PolicyConflictError raise and returning 500 instead of the required 422 ConflictReport; it now falls back to the structural report (ADR 0001: surface, never drop). - conflict_enrichment.py + diagnostic.py: a blank/whitespace-only quote no longer verifies (guard q.strip() before _verify_quote, which is trivially true for the empty string), so an empty quote can no longer set quotes_verified=True with no evidence. - CONTEXT.md: within-batch example said 'scope-focal deny'; the scope-focal pass emits grants -> 'scope-focal grant'. - ADR 0001: add a #2504 addendum recording that cross-service conflicts are now detected pre-persistence (closing the Q13 follow-up gap, still identify-never-reconcile). - policy-conflict-check.md: mark the spec retired/superseded by /apply (the standalone POST /policy/check route was removed in #2500). - test_uc1_onboard_cross_service_conflict.py: open the cleanup guard before the first shared-state mutation and clear the Policy Store in teardown. - test_conflict_detection.py: lambda-assignment -> local def key(...). Deferred: the concurrent-apply TOCTOU (builder.py detect/persist atomicity) is pre-existing and needs Policy Store CAS; tracked as a follow-up. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- aiac/CONTEXT.md | 4 ++-- .../docs/adr/0001-identify-never-reconcile.md | 24 ++++++++++++++++++- .../aiac-agent/policy-conflict-check.md | 10 ++++++++ .../conflict_enrichment.py | 4 +++- .../agent/policy_rules_builder/diagnostic.py | 6 +++-- .../uc/onboarding/policy_builder/builder.py | 11 ++++++++- .../test_conflict_detection.py | 6 +++-- ...test_uc1_onboard_cross_service_conflict.py | 22 ++++++++++------- 8 files changed, 70 insertions(+), 17 deletions(-) diff --git a/aiac/CONTEXT.md b/aiac/CONTEXT.md index 5fb4dd2ca..db1114847 100644 --- a/aiac/CONTEXT.md +++ b/aiac/CONTEXT.md @@ -47,8 +47,8 @@ _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 deny and the Door B deny (and any collision -with a grant) are in hand in the same build. In 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**): diff --git a/aiac/docs/adr/0001-identify-never-reconcile.md b/aiac/docs/adr/0001-identify-never-reconcile.md index b3cee0015..9b21f6d9a 100644 --- a/aiac/docs/adr/0001-identify-never-reconcile.md +++ b/aiac/docs/adr/0001-identify-never-reconcile.md @@ -23,7 +23,8 @@ accepted 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 — - are a pre-existing gap left as a follow-up, not reconciled here. + 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 @@ -59,3 +60,24 @@ never-reconcile principle above, and Q15's *shape* unification — both 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.) diff --git a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md index bc6d0ecdb..da5018c36 100644 --- a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md +++ b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md @@ -1,5 +1,15 @@ # 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 when — and only when — a structural conflict is +> detected. 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. diff --git a/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py b/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py index a19e52127..fcdc94c97 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py +++ b/aiac/src/aiac/agent/policy_rules_builder/conflict_enrichment.py @@ -55,7 +55,9 @@ def enrich_report(report: ConflictReport, rules: list[PolicyRule], policy_text: result = _explain_pair(policy_text, role, scope, c.explanation) granting = list(result.granting_quotes) prohibiting = list(result.prohibiting_quotes) - verified = bool(granting or prohibiting) and all(_verify_quote(q, policy_text) for q in granting + prohibiting) + verified = bool(granting or prohibiting) and all( + q.strip() and _verify_quote(q, policy_text) for q in granting + prohibiting + ) enriched.append( c.model_copy( update={ diff --git a/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py b/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py index a4ebd2814..7068f0c94 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py +++ b/aiac/src/aiac/agent/policy_rules_builder/diagnostic.py @@ -265,9 +265,11 @@ def _explain( ) granting = list(result.granting_quotes) prohibiting = list(result.prohibiting_quotes) - # Verified only when there is at least one quote AND every quote is a verbatim substring. + # Verified only when there is at least one non-empty quote AND every quote is a verbatim + # substring. A blank/whitespace-only quote must NOT verify (``_verify_quote("", ...)`` is + # trivially true), so guard it with ``q.strip()`` before the substring check. verified = bool(granting or prohibiting) and all( - _verify_quote(q, policy_text) for q in granting + prohibiting + q.strip() and _verify_quote(q, policy_text) for q in granting + prohibiting ) explanation = result.explanation if verified else contradiction.description out.append( diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index 9f26ee8dc..f97ee3580 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -94,7 +94,16 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: # passed so enrichment can resolve the typed Role/Scope of a pair whose sides came from # different services (a conflict may join a new rule to a stored one). Policy source is # read only on this path. - report = enrich_report(report, combined, get_policy_source().fetch()) + # + # Enrichment (policy fetch + LLM explain/quote) is best-effort: if the policy source is + # unreadable/missing or the explain pass errors, we still raise with the STRUCTURAL + # report (ADR 0001: surface, never drop). Letting the error escape here would bypass the + # controller's PolicyConflictError handler and return a 500 instead of the required 422 + # ConflictReport. + try: + report = enrich_report(report, combined, get_policy_source().fetch()) + except Exception: + pass raise PolicyConflictError(report) # Only this build's own rules are applied; the OTHER services' rules read above are already # persisted and are used solely to widen detection, never re-emitted. diff --git a/aiac/test/agent/policy_rules_builder/test_conflict_detection.py b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py index f62b79026..6c1258e40 100644 --- a/aiac/test/agent/policy_rules_builder/test_conflict_detection.py +++ b/aiac/test/agent/policy_rules_builder/test_conflict_detection.py @@ -69,7 +69,8 @@ def test_detection_is_order_independent(): rules = [_allow(_TESTER, _ISSUES), _deny(_TESTER, _ISSUES), _allow(_DEV, _SOURCE)] forward = detect_conflicts(rules) reverse = detect_conflicts(list(reversed(rules))) - key = lambda rep: sorted((c.role.id, c.scope.id) for c in rep.conflicts) + def key(rep): + return sorted((c.role.id, c.scope.id) for c in rep.conflicts) assert key(forward) == key(reverse) == [("r-tester", "s-iss")] @@ -146,6 +147,7 @@ def test_combined_detection_is_order_independent_across_sides(): # conflict set (keyed on ids) — onboarding order / read order cannot change the outcome. build_rules = [_allow(_TESTER, _ISSUES)] applied = [_deny(_TESTER, _ISSUES), _allow(_DEV, _SOURCE)] - key = lambda rep: sorted((c.role.id, c.scope.id) for c in rep.conflicts) + def key(rep): + return sorted((c.role.id, c.scope.id) for c in rep.conflicts) assert key(detect_conflicts(build_rules + applied)) == key(detect_conflicts(applied + build_rules)) assert key(detect_conflicts(build_rules + applied)) == [("r-tester", "s-iss")] diff --git a/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py index 5b384485d..471b70c52 100644 --- a/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py +++ b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py @@ -157,15 +157,18 @@ def test_cross_service_conflict_is_surfaced_as_422_conflict_report() -> None: keycloak_url = creds["KEYCLOAK_URL"] admin = uc1.connect_admin() - uc1.delete_agent_cr() # clean policy slate (drop any prior run's CR) - uc1.cleanup_provisioned(admin, TEST_REALM) # clean slate (Keycloak) - uc1.clear_policy_store() # clean slate ONCE — NOT between phases (the agent must see the tool's rule) - uc1.provision_realm_and_users(admin, TEST_REALM) # PRB reads the role universe - uc1.verify_subject_mapper( - keycloak_url=keycloak_url, realm=TEST_REALM, user="test-user", password=scn.USER_PASSWORD - ) - + # Open the cleanup guard BEFORE the first shared-state mutation: the clean-slate steps below + # (Agent CR delete, Keycloak cleanup, Policy Store clear, provisioning) already mutate shared + # cluster state, so a failure mid-setup must still hit the ``finally`` teardown. try: + uc1.delete_agent_cr() # clean policy slate (drop any prior run's CR) + uc1.cleanup_provisioned(admin, TEST_REALM) # clean slate (Keycloak) + uc1.clear_policy_store() # clean slate ONCE — NOT between phases (the agent must see the tool's rule) + uc1.provision_realm_and_users(admin, TEST_REALM) # PRB reads the role universe + uc1.verify_subject_mapper( + keycloak_url=keycloak_url, realm=TEST_REALM, user="test-user", password=scn.USER_PASSWORD + ) + # Phase 1 — tool onboarding under the exclusivity policy: clean (200), persists the tool-side # DENY(tester -> github-tool.source-*) on SPM(github-tool). _onboard_via_fresh_controller(POLICY_TOOL_EXCLUSIVE, scn.TOOL_WORKLOAD, expect_conflict=False) @@ -179,6 +182,9 @@ def test_cross_service_conflict_is_surfaced_as_422_conflict_report() -> None: finally: uc1.delete_agent_cr() # after — drop any CR (there should be none on the raising path) uc1.cleanup_provisioned(admin, TEST_REALM) # restore the pre-run Keycloak state + # Phase 1 persisted a tool-side deny; clear it so later integration tests don't read stale + # inbound rules and become order-dependent. + uc1.clear_policy_store() # The 422 body is the shared ConflictReport shape (same as the deterministic routes test), carrying # at least one cross-service conflict with a real role + scope. Structural (not id-pinned) so it is From 94c97d98d2b791448f88199bba497672f12df0bb Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 15:29:51 +0000 Subject: [PATCH 09/13] Fix: Address clawgenti review on #842 (log enrichment failures, doc policy_text init) - builder.py: the best-effort enrichment fallback no longer swallows errors silently. It now logs at WARNING with exc_info so a genuine enrich_report bug (TypeError/AttributeError) is distinguishable from an expected "policy source unavailable" in production. Still raises the structural report (ADR 0001: surface, never drop). - graph.py: document that the "policy_text": "" state init is a placeholder the graph's fetch node (START -> fetch -> propose) populates via get_policy_source() before propose reads it, so a future reader does not add a redundant fetch. The line-length nit on conflict_detection.py:97 is a non-issue: ruff is enforced at line-length=120 and the line is 110 chars. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- aiac/src/aiac/agent/policy_rules_builder/graph.py | 6 ++++++ .../aiac/agent/uc/onboarding/policy_builder/builder.py | 10 +++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/aiac/src/aiac/agent/policy_rules_builder/graph.py b/aiac/src/aiac/agent/policy_rules_builder/graph.py index 9b231b3d4..79fca613c 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/graph.py +++ b/aiac/src/aiac/agent/policy_rules_builder/graph.py @@ -402,6 +402,8 @@ def build_role_rules(role: Role, scopes: list[Scope]) -> list[PolicyRule]: state: RoleRulesState = { "role": role, "scopes": scopes, + # placeholder: the graph's ``fetch`` node (START -> fetch -> propose) populates + # this via get_policy_source() before ``propose`` reads it -- do not fetch here. "policy_text": "", "selected_names": [], "denied_names": [], @@ -428,6 +430,8 @@ def build_role_denies(role: Role, scopes: list[Scope]) -> list[PolicyRule]: state: RoleRulesState = { "role": role, "scopes": scopes, + # placeholder: the graph's ``fetch`` node (START -> fetch -> propose) populates + # this via get_policy_source() before ``propose`` reads it -- do not fetch here. "policy_text": "", "selected_names": [], "denied_names": [], @@ -446,6 +450,8 @@ def build_scope_rules(roles: list[Role], scope: Scope) -> list[PolicyRule]: state: ScopeRulesState = { "roles": roles, "scope": scope, + # placeholder: the graph's ``fetch`` node (START -> fetch -> propose) populates + # this via get_policy_source() before ``propose`` reads it -- do not fetch here. "policy_text": "", "selected_names": [], "denied_names": [], diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py index f97ee3580..6039cf84c 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/builder.py @@ -29,6 +29,8 @@ before. """ +import logging + from aiac.agent.policy_rules_builder.conflict_detection import PolicyConflictError, detect_conflicts from aiac.agent.policy_rules_builder.conflict_enrichment import enrich_report from aiac.agent.policy_rules_builder.graph import build_role_denies, build_role_rules, build_scope_rules @@ -40,6 +42,8 @@ from aiac.idp.configuration.models import RoleKind, ServiceType from aiac.policy.model.models import PolicyRule +logger = logging.getLogger(__name__) + def _config() -> Configuration: return Configuration.for_default_realm() @@ -103,7 +107,11 @@ def build(service_id: str, service_type: ServiceType) -> list[PolicyRule]: try: report = enrich_report(report, combined, get_policy_source().fetch()) except Exception: - pass + # Best-effort only (ADR 0001: surface, never drop). Log at WARNING with the + # traceback so a genuine bug in enrich_report (TypeError/AttributeError) is + # distinguishable in production from an expected "policy source unavailable", + # rather than being silently swallowed. We still raise the STRUCTURAL report. + logger.warning("policy conflict enrichment failed; falling back to structural report", exc_info=True) raise PolicyConflictError(report) # Only this build's own rules are applied; the OTHER services' rules read above are already # persisted and are used solely to widen detection, never re-emitted. From eca35eda165eb4f0da88a3ed0a43a1ad0abb8857 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 16:18:58 +0000 Subject: [PATCH 10/13] Docs: Correct /apply 422 banner to cover both conflict paths (#842) The retired-route banner said the ConflictReport 422 fires 'only when a structural conflict is detected', which excludes the auditor path. Both PolicyConflictError (structural) and PolicyContradictionError (intra-pass LLM auditor) map to HTTP 422 with the same ConflictReport shape (routes.py:59-67). Reword to cover both. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../specs/components/aiac-agent/policy-conflict-check.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md index da5018c36..1191cc8d6 100644 --- a/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md +++ b/aiac/docs/specs/components/aiac-agent/policy-conflict-check.md @@ -3,8 +3,10 @@ > **⚠️ 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 when — and only when — a structural conflict is -> detected. The `PolicyCheckRequest` model and the `check_policy_conflicts(...)` function are +> 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 From 5907ea8a5b3bd56e68951eda795a1564d12cefaa Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 17:04:25 +0000 Subject: [PATCH 11/13] Fix: Address clawgenti review on #842 (F2 focal-prefix coupling, F4 fail-closed store read) - F2 (conflict_detection.py focal parsing): the role name=/scope name= prefixes are now single-sourced as ROLE_FOCAL_PREFIX/SCOPE_FOCAL_PREFIX constants in graph.py (the producer, via _role_focal/_scope_focal) and imported by the parser, so a format change can no longer silently drift the parse into its SCOPE fallback. - F4 (cross_service.py store read): a get_service_policy failure (network/auth/ malformed row) no longer escapes as an unhandled 500. It is now fail-closed -- a store-read failure means we are blind to another service's applied rules, so aborting is correct (best-effort would suppress cross-service detection and let a real overlap persist). Surfaced as 502 (dependency fault, mirrors the IdP boundary in resolve_focal_entities), not the 422 reserved for genuine conflicts. F1 (test_graph.py _build_llm import) skipped: not dead -- still used by the transport-retry and request-timeout tests. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../conflict_detection.py | 11 +++++---- .../aiac/agent/policy_rules_builder/graph.py | 13 ++++++++-- .../policy_builder/cross_service.py | 24 ++++++++++++++++++- 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py index 850f01976..bd711d385 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py +++ b/aiac/src/aiac/agent/policy_rules_builder/conflict_detection.py @@ -26,6 +26,7 @@ FocalRef, FocalType, ) +from .graph import ROLE_FOCAL_PREFIX, SCOPE_FOCAL_PREFIX class PolicyConflictError(Exception): @@ -77,12 +78,14 @@ def report_from_contradictions(focal: str, contradictions) -> ConflictReport: ``quotes_verified=False``, and the auditor ``description`` as the ``explanation``. ``focal`` is parsed from the raise's focal string (``_role_focal`` / ``_scope_focal`` prefix) to recover the axis and name; the candidate name goes on the opposite side.""" - if focal.startswith("role name="): + # Prefixes are imported from graph (the producer of this string) so the parse cannot silently + # drift from ``_role_focal`` / ``_scope_focal`` into the SCOPE fallback if the format changes. + if focal.startswith(ROLE_FOCAL_PREFIX): focal_type = FocalType.ROLE - focal_name = focal[len("role name=") :].split(":", 1)[0].strip() - elif focal.startswith("scope name="): + focal_name = focal[len(ROLE_FOCAL_PREFIX) :].split(":", 1)[0].strip() + elif focal.startswith(SCOPE_FOCAL_PREFIX): focal_type = FocalType.SCOPE - focal_name = focal[len("scope name=") :].split(":", 1)[0].strip() + focal_name = focal[len(SCOPE_FOCAL_PREFIX) :].split(":", 1)[0].strip() else: # Unrecognized focal string (e.g. a bare service token in a degenerate raise): anchor on # the SCOPE side (Q16) and use the whole string as the focal name. diff --git a/aiac/src/aiac/agent/policy_rules_builder/graph.py b/aiac/src/aiac/agent/policy_rules_builder/graph.py index 79fca613c..c171cbe2b 100644 --- a/aiac/src/aiac/agent/policy_rules_builder/graph.py +++ b/aiac/src/aiac/agent/policy_rules_builder/graph.py @@ -223,12 +223,21 @@ def _route(state: _PRBWorking) -> str: return "approved" if state["approved"] else "rejected" +# Focal-string format contract. The auditor raise carries the focal entity as a plain string +# built here; ``conflict_detection.report_from_contradictions`` parses it back to recover the axis +# and name. These prefixes are the single source of truth for both sides -- the producer here and +# the consumer there import the SAME constants, so the coupling is explicit and a format change +# cannot silently drift the parser into its SCOPE fallback. +ROLE_FOCAL_PREFIX = "role name=" +SCOPE_FOCAL_PREFIX = "scope name=" + + def _role_focal(r: Role) -> str: - return f"role name={r.name}: {r.description or ''}" + return f"{ROLE_FOCAL_PREFIX}{r.name}: {r.description or ''}" def _scope_focal(s: Scope) -> str: - return f"scope name={s.name}: {s.description or ''}" + return f"{SCOPE_FOCAL_PREFIX}{s.name}: {s.description or ''}" def _scope_cands(ss: list[Scope]) -> str: diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py index 6d7ac54e3..7011d1e92 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py @@ -20,9 +20,15 @@ the builder still raises **before** ``compute_and_apply``. """ +import logging + +from fastapi import HTTPException + from aiac.policy.model.models import PolicyRule from aiac.policy.model_store.library.api import get_service_policy +logger = logging.getLogger(__name__) + def applied_rules_for_scopes(rules: list[PolicyRule]) -> list[PolicyRule]: """Read every already-applied inbound rule (``Allow`` AND ``Deny``) on the scopes the @@ -43,7 +49,23 @@ def applied_rules_for_scopes(rules: list[PolicyRule]) -> list[PolicyRule]: owners = sorted({rule.scope.serviceId for rule in rules if rule.scope.serviceId}) applied: list[PolicyRule] = [] for owner in owners: - spm = get_service_policy(owner) + try: + spm = get_service_policy(owner) + except Exception as exc: + # Fail CLOSED, never best-effort. A store-read failure (network/auth/malformed row; + # a genuine 404 is already absorbed as a fresh empty SPM inside get_service_policy) + # means we are BLIND to this owner's already-applied rules. Silently treating that as + # "no other services applied" would suppress cross-service detection and let a real + # ALLOW∩DENY overlap persist -- a false-negative that defeats #2504's whole purpose. + # So we abort the apply instead. An unreachable Policy Store is a dependency fault, not + # a policy-input problem, so it surfaces as 502 (mirrors resolve_focal_entities' IdP + # boundary) rather than the 422 ConflictReport reserved for genuine conflicts, and never + # as an unhandled 500. + logger.warning("cross-service store read failed for owner %s; aborting apply (fail-closed)", owner) + raise HTTPException( + status_code=502, + detail=f"policy store unreachable while reading cross-service rules for {owner!r}: {exc}", + ) from exc applied.extend(spm.inbound_allow_rules) applied.extend(spm.inbound_deny_rules) return applied From 9ab13349793e448f6f23e4a64740c733957ff8c0 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 17:11:19 +0000 Subject: [PATCH 12/13] Test: xfail the unconfirmed cross-service integration scenario (#842 F5) Addresses review finding F5. The live cross-service OPA-loop scenario is under revision and not yet confirmed to fire the 422 on a live cluster (per the PR description), and it drives the real PRB LLM, so the derived rule is model-dependent. Marks it @pytest.mark.xfail(strict=False) so a live run cannot false-green while the scenario is validated, and reports XPASS the moment it fires (the signal to drop the marker). Deselected from the routine -m "not integration" run regardless. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../integration/test_uc1_onboard_cross_service_conflict.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py index 471b70c52..b1d40c0ae 100644 --- a/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py +++ b/aiac/test/integration/test_uc1_onboard_cross_service_conflict.py @@ -141,6 +141,12 @@ def _onboard_via_fresh_controller(policy_md: str, workload: str, *, expect_confl return None +# This live scenario is under revision and NOT yet confirmed to fire the 422 on a live cluster (see the +# PR description and the follow-ups it references). It also drives the real PRB LLM in phase 2, so the +# exact derived rule is model-dependent. ``xfail`` (non-strict) keeps a live run from false-greening +# while the scenario is being validated, and reports XPASS the moment it does fire — the signal to drop +# this marker. It stays deselected from the routine ``-m "not integration"`` run regardless. +@pytest.mark.xfail(strict=False, reason="live cross-service scenario under revision; not yet confirmed to fire 422") def test_cross_service_conflict_is_surfaced_as_422_conflict_report() -> None: """End-to-end #2504: onboard the tool under an exclusivity policy (persisting a tool-side deny), then onboard the agent under a policy that grants the same testers source — the agent's outbound From 6216baefa9452e36db216f73e1e334c0c5c544a7 Mon Sep 17 00:00:00 2001 From: Anatoly Koyfman Date: Tue, 1 Sep 2026 17:42:41 +0000 Subject: [PATCH 13/13] Fix: Redact store-read exception from cross-service 502 detail (CWE-209) The #2504 cross-service read fails closed with a 502 when the Policy Store is unreachable, but the client-facing detail echoed the raw exception (and the owner id). A store error can carry an internal URL, host, or credential fragment, so surfacing it in the HTTP body is an information-disclosure leak (CWE-209). Log the exception (and owner) server-side with exc_info=True, and return a stable, generic detail with no exc/owner text. 'from exc' keeps the cause in the server-side traceback. Adds a regression test asserting the sensitive text is absent from the response but present in the server log. Assisted-By: Claude (Anthropic AI) Signed-off-by: Anatoly Koyfman --- .../policy_builder/cross_service.py | 11 ++++++-- .../policy_builder/test_cross_service.py | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py index 7011d1e92..6679e2488 100644 --- a/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py +++ b/aiac/src/aiac/agent/uc/onboarding/policy_builder/cross_service.py @@ -61,10 +61,17 @@ def applied_rules_for_scopes(rules: list[PolicyRule]) -> list[PolicyRule]: # a policy-input problem, so it surfaces as 502 (mirrors resolve_focal_entities' IdP # boundary) rather than the 422 ConflictReport reserved for genuine conflicts, and never # as an unhandled 500. - logger.warning("cross-service store read failed for owner %s; aborting apply (fail-closed)", owner) + # The exception (and the owner id) are logged server-side only. The client-facing 502 + # detail is a STABLE, generic string with no ``exc`` text and no id -- a store error can + # carry an internal URL, host, or credential fragment, so echoing it into the HTTP body + # would be an information-disclosure leak (CWE-209). ``from exc`` keeps the cause in the + # server-side traceback without surfacing it to the caller. + logger.warning( + "cross-service store read failed for owner %s; aborting apply (fail-closed)", owner, exc_info=True + ) raise HTTPException( status_code=502, - detail=f"policy store unreachable while reading cross-service rules for {owner!r}: {exc}", + detail="policy store unreachable while resolving cross-service rules", ) from exc applied.extend(spm.inbound_allow_rules) applied.extend(spm.inbound_deny_rules) diff --git a/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py b/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py index 564784c75..067236f55 100644 --- a/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py +++ b/aiac/test/agent/uc/onboarding/policy_builder/test_cross_service.py @@ -13,6 +13,9 @@ from unittest.mock import patch +import pytest +from fastapi import HTTPException + from aiac.agent.uc.onboarding.policy_builder.cross_service import applied_rules_for_scopes from aiac.idp.configuration.models import Role, Scope from aiac.policy.model.models import PolicyRule, RuleEffect, ServicePolicyModel, ServiceType @@ -92,3 +95,26 @@ def test_empty_rules_reads_nothing(): with patch(f"{_CROSS}.get_service_policy") as get: assert applied_rules_for_scopes([]) == [] get.assert_not_called() + + +def test_store_read_failure_aborts_502_without_leaking_exception(caplog): + # A store-read failure fails CLOSED as 502 (never best-effort), but must NOT echo the raw + # exception into the client-facing detail (CWE-209): a store error can carry an internal URL, + # host, or credential fragment. The sensitive text is logged server-side only; the HTTP body + # is a stable, generic string. + secret = "postgres://admin:s3cr3t@internal-db.svc:5432 connection refused" + with patch(f"{_CROSS}.get_service_policy", side_effect=RuntimeError(secret)): + with caplog.at_level("WARNING"): + with pytest.raises(HTTPException) as ei: + applied_rules_for_scopes([_allow(_TESTER, _ISSUES)]) + + assert ei.value.status_code == 502 + # Client-facing detail is stable and carries none of the exception / owner text. + assert secret not in str(ei.value.detail) + assert "s3cr3t" not in str(ei.value.detail) + assert "svc-tool" not in str(ei.value.detail) + assert ei.value.detail == "policy store unreachable while resolving cross-service rules" + # The cause is preserved for the server-side traceback (chained via ``from exc``). + assert isinstance(ei.value.__cause__, RuntimeError) + # And the sensitive detail IS captured server-side (exc_info logging), where operators need it. + assert secret in caplog.text