Docs: Add AIAC specification documents#661
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe PR adds AIAC architecture, component, demo, broker-evaluation, and integration-test specifications. The documents define system boundaries, policy workflows, service contracts, deployment behavior, event delivery, RAG processing, and validation scenarios. ChangesAIAC documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (33)
aiac/docs/specs/ARCHITECTURE-SUMMARY.md-101-101 (1)
101-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDocument at-least-once delivery accurately.
The competing-consumer design does not guarantee exactly-once processing. Replace the claim with at-least-once delivery plus idempotent handlers/reconciliation, and keep that wording consistent in both documents.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` at line 101, Update the Event Broker descriptions in aiac/docs/specs/ARCHITECTURE-SUMMARY.md:101-101 and aiac/docs/specs/PRD.md:116-116 to replace the exactly-once processing claim with at-least-once delivery and explicitly mention idempotent handlers/reconciliation, keeping the wording consistent across both documents.aiac/docs/specs/ARCHITECTURE-SUMMARY.md-161-163 (1)
161-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the canonical library module names.
This section names
aiac.idp.libraryandaiac.pdp.library, but the documented integration surfaces areaiac.idp.configurationandaiac.pdp.policy.library. Correct these references before engineers copy invalid imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` around lines 161 - 163, Update the service onboarding documentation to use the canonical integration module names: replace aiac.idp.library with aiac.idp.configuration and aiac.pdp.library with aiac.pdp.policy.library. Keep the surrounding description unchanged.aiac/docs/specs/ARCHITECTURE-SUMMARY.md-174-177 (1)
174-177: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRename the undeclared “Policy Management Service.”
The architecture defines a Policy Store, not a Policy Management Service. This stale name obscures the owner of SQLite state and conflicts with the component and PRD contracts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` around lines 174 - 177, Rename “Policy Management Service” to the established “Policy Store” in the AIAC architecture description, including the heading and any associated ownership wording, while preserving the existing SQLite persistence and policy-state flow.aiac/docs/specs/ARCHITECTURE-SUMMARY.md-204-204 (1)
204-204: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoute every policy update through the PCE and reset both stores during rebuild.
The Architecture Summary’s flows still post directly to the PDP Policy Writer and omit the Policy Store, while the PRD defines
compute_and_applyas the coordination point. The full rebuild must clear both the Policy Store and OPA state before recomputing, otherwise stale structured mappings can be reintroduced.
aiac/docs/specs/ARCHITECTURE-SUMMARY.md#L204-L204: replace the direct PDP write with the PCE flow.aiac/docs/specs/ARCHITECTURE-SUMMARY.md#L225-L225: use the PCE for role updates.aiac/docs/specs/ARCHITECTURE-SUMMARY.md#L248-L248: use the PCE for policy builds.aiac/docs/specs/ARCHITECTURE-SUMMARY.md#L261-L265: show Policy Store deletion before recomputation.aiac/docs/specs/components/aiac-agent/uc2-policy-update.md#L50-L56: specify the same full-reset behavior for Rebuild.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` at line 204, Update aiac/docs/specs/ARCHITECTURE-SUMMARY.md at lines 204, 225, and 248 to route agent policy writes, role updates, and policy builds through the PCE compute_and_apply coordination flow instead of directly to the PDP Policy Writer; update lines 261-265 to delete both Policy Store data and OPA state before recomputation. Apply the same full-reset-before-rebuild behavior to the Rebuild flow in aiac/docs/specs/components/aiac-agent/uc2-policy-update.md lines 50-56.aiac/docs/specs/ARCHITECTURE-SUMMARY.md-53-61 (1)
53-61: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine the offboarding trigger and cleanup path.
The use-case text claims role/client removal is reconciled, but the documented subjects and dispatch table only cover creation and updates. Either remove “removed” from the supported behavior or add the delete event, consumer mapping, handler, and policy cleanup semantics.
aiac/docs/specs/ARCHITECTURE-SUMMARY.md#L53-L61: align UC-1 trigger claims with implemented events.aiac/docs/specs/PRD.md#L66-L74: define removal reconciliation or remove it from the use case.aiac/docs/specs/PRD.md#L437-L443: add the missing Keycloak deletion mapping.aiac/docs/specs/components/aiac-agent/uc3-role-update.md#L7-L12: document the role-deletion trigger and behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` around lines 53 - 61, Define the offboarding reconciliation contract consistently across all four sites: in aiac/docs/specs/ARCHITECTURE-SUMMARY.md lines 53-61, retain removal only if supported and describe its cleanup path; in aiac/docs/specs/PRD.md lines 66-74, document removal reconciliation behavior; in aiac/docs/specs/PRD.md lines 437-443, add the Keycloak deletion event mapping; and in aiac/docs/specs/components/aiac-agent/uc3-role-update.md lines 7-12, document the role-deletion trigger, consumer handling, and policy cleanup semantics.aiac/docs/specs/PRD.md-114-114 (1)
114-114: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine one consistent PCE API and merge contract.
The PRD describes additive-only merging and omits
override, while UC1/UC3 requirecompute_and_apply(..., override=...)with authoritative replacement semantics. Update every affected section to distinguish append mode from role-keyed replacement mode.
aiac/docs/specs/PRD.md#L114-L114: document the override-aware PCE contract.aiac/docs/specs/PRD.md#L298-L299: remove or qualify the “additive only” decision.aiac/docs/specs/PRD.md#L365-L365: include override behavior.aiac/docs/specs/components/aiac-agent.md#L16-L24: return and forward the override flag.aiac/docs/specs/components/aiac-agent.md#L116-L116: call the override-aware API.aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md#L21-L21: retain append mode explicitly.aiac/docs/specs/components/aiac-agent/uc2-policy-update.md#L54-L56: define Build versus Rebuild mode.aiac/docs/specs/components/aiac-agent/uc3-role-update.md#L68-L70: retain authoritative replacement semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/PRD.md` at line 114, Define one consistent override-aware PCE contract across all cited documentation: in aiac/docs/specs/PRD.md lines 114-114 and 365-365 document compute_and_apply(rules, override=...) with append behavior when false and role-keyed authoritative replacement when true, while qualifying the additive-only decision at lines 298-299. In aiac/docs/specs/components/aiac-agent.md lines 16-24 and 116-116, return and forward the override flag and call the override-aware API. Explicitly retain append mode in aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md lines 21-21, define Build versus Rebuild mode in aiac/docs/specs/components/aiac-agent/uc2-policy-update.md lines 54-56, and preserve authoritative replacement semantics in aiac/docs/specs/components/aiac-agent/uc3-role-update.md lines 68-70.aiac/docs/specs/event-broker-redhat-amq-evaluation.md-7-9 (1)
7-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not call this evaluation technology-neutral while retaining implementation-specific requirements.
The abstract says technology-specific constraints were removed, but FR5–FR7 still require Python 3.12/FastAPI/asyncio and Java. Remove those details from the functional requirements or explicitly state that they remain binding; otherwise the candidate comparison is not technology-neutral.
Also applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/event-broker-redhat-amq-evaluation.md` around lines 7 - 9, Reconcile the technology-neutral claim in the evaluation abstract with FR5–FR7: remove references to Python 3.12, FastAPI, asyncio, and Java from those functional requirements, or explicitly identify them as binding technology constraints. Ensure the abstract accurately reflects the resulting requirements.aiac/docs/specs/event-broker-redhat-amq-evaluation.md-27-30 (1)
27-30: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefine FR2 as work-queue assignment, not exactly-once processing.
FR1 and FR3 explicitly allow redelivery and retries, so one event may be handled by multiple Agent instances over its lifetime. Clarify that each delivery is assigned to one active consumer, and distinguish that from exactly-once processing to avoid incorrect idempotency and delivery guarantees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/event-broker-redhat-amq-evaluation.md` around lines 27 - 30, Update FR2 in the requirements table to define work-queue assignment per delivery: each delivery is assigned to one active Agent consumer across competing replicas. Remove the “exactly one Agent instance” processing guarantee and explicitly distinguish single-consumer delivery from exactly-once processing, while preserving FR1 redelivery and FR3 retry semantics.aiac/docs/specs/event-broker-redhat-amq-evaluation.md-12-16 (1)
12-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the conclusion with the FR9 verdict.
AMQ Broker is described as satisfying all ten requirements, while FR9 is marked
Partialand⚠️ in the assessment and scorecard. Either change the conclusion to acknowledge that FR9 is an accepted trade-off, or change the verdict and justify why it satisfies the stated footprint requirement.Also applies to: 177-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/event-broker-redhat-amq-evaluation.md` around lines 12 - 16, The conclusion in the event-broker evaluation conflicts with the FR9 Partial verdict. Update the Conclusion to state that AMQ Broker meets the functional requirements with FR9 accepted as a documented trade-off, or revise FR9 to fully satisfied only if the assessment and scorecard provide justification against the stated footprint requirement.aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md-222-228 (1)
222-228: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClarify the phase-1 authorization contract.
This section says the agent holds all tool scopes, then says real UC-1 emits an empty
target_ok, making the generatedallowdeny every request. Because the test probessubject_okonly, it does not validate the documented outboundallow. State whether “holds all scopes” is an intended future invariant or current behavior, and align the oracle and expected claims accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md` around lines 222 - 228, Clarify the phase-1 authorization contract in the onboarding pipeline specification by explicitly distinguishing the intended future “agent holds all github-tool scopes” invariant from current UC-1 behavior, where target_ok is empty. Align the probe’s oracle and expected claims to validate subject_ok/user gating only, and document that the generated outbound allow is not asserted until agent-role gating is implemented.aiac/docs/specs/integration-test/policy-pipeline.md-62-64 (1)
62-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPin the runtime contracts instead of deferring them to test implementation.
These specifications leave required import paths, environment-variable names, ports, and output/deployment selectors as “confirm later” placeholders. That makes the test plans non-actionable and permits implementations to drift from the documented contracts.
aiac/docs/specs/integration-test/policy-pipeline.md#L62-L64: specify the canonical Policy Store ASGI import path and database environment variable.aiac/docs/specs/integration-test/policy-pipeline.md#L221-L224: replaceAGENTPOLICY_DB_PATHwith the verified contract or link the authoritative component specification.aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md#L264-L266: define the Controller ports and deterministic OPA pod/PVC/output-path selection.aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md#L270-L274: remove the “confirm” placeholder once those deployment contracts are specified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/integration-test/policy-pipeline.md` around lines 62 - 64, Pin the runtime contracts in aiac/docs/specs/integration-test/policy-pipeline.md:62-64 and :221-224 by documenting the canonical Policy Store ASGI import path and verified database environment variable, replacing AGENTPOLICY_DB_PATH or linking its authoritative specification. In aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md:264-266, define Controller ports and deterministic OPA pod, PVC, and output-path selectors; then remove the related “confirm” placeholder at :270-274. Ensure all referenced ports, environment variables, import paths, and deployment selectors are actionable and consistent with the authoritative component contracts.aiac/docs/specs/components/pdp-policy-writer-opa.md-9-11 (1)
9-11: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the IdP library namespace with the other specifications.
This document references
aiac.idp.library.configuration, while the PCE and integration references useaiac.idp.configuration.library. Choose the canonical import path and update all component and test plans consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/pdp-policy-writer-opa.md` around lines 9 - 11, Update the IdP library namespace in the PDP policy writer specification from aiac.idp.library.configuration to the canonical aiac.idp.configuration.library path, and apply the same namespace consistently across all related component and test plan references.aiac/docs/specs/components/policy-computation-engine.md-166-170 (1)
166-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the full-rebuild scope with the documented
override=Truebehavior.Earlier sections state that UC2 Rebuild uses
override=True, while this section says full policy rebuilds occur outside the PCE. Clarify whether “rebuild” means role-level replacement inside the PCE or a whole-store clear-and-reapply workflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-computation-engine.md` around lines 166 - 170, The Out of Scope section ambiguously excludes full policy rebuilds despite UC2 Rebuild using override=True. Clarify the distinction: document role-level replacement via override=True as supported within the PCE, while whole-store clear-and-reapply rebuilds remain the responsibility of higher-level orchestration.aiac/docs/specs/components/policy-model.md-86-90 (1)
86-90: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStandardize subject map keys on one identifier.
The policy model and PDP descriptions use subject IDs, but the PCE writes
subject_roles[subject.username]. If a username differs from the Keycloak entity ID used in request input, authorization lookups fail.
aiac/docs/specs/components/policy-model.md#L86-L90: retain eitheridorusernameas the canonical key.aiac/docs/specs/components/policy-computation-engine.md#L88-L93: use the same key when recording subjects.aiac/docs/specs/components/policy-computation-engine.md#L148-L154: align the test expectation.aiac/docs/specs/components/pdp-policy-writer-opa.md#L41-L46: align the model field description.aiac/docs/specs/components/pdp-policy-writer-opa.md#L102-L109: align the Rego input and map lookup contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-model.md` around lines 86 - 90, Standardize the subject-role map on the canonical subject ID across all listed documentation sites: update the policy model description, the PCE subject recording flow and test expectation, and the OPA model, Rego input, and lookup contract. Ensure every reference uses the same identifier consistently and no longer records or expects username-based keys.aiac/docs/specs/components/policy-store.md-149-154 (1)
149-154: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake the network-only authorization boundary an enforced deployment requirement.
Every policy mutation endpoint is unauthenticated, so any workload that can reach port 7074 can alter or delete authorization state. Either add application-layer authentication or explicitly require and test restrictive NetworkPolicies as a release prerequisite, including the trusted-caller threat model.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-store.md` around lines 149 - 154, The Policy Store specification must make its unauthenticated mutation surface a mandatory deployment security requirement. Update the “In-cluster mTLS between Policy Computation Engine and Policy Store” out-of-scope item to explicitly require restrictive Kubernetes NetworkPolicies, define the trusted-caller threat model, and require release validation/tests proving only authorized callers can reach port 7074; alternatively, specify application-layer authentication for every policy mutation endpoint.aiac/docs/specs/components/policy-model.md-46-53 (1)
46-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the package import contract consistent.
The package layout declares an empty
aiac.policy.model.__init__.py, but other specifications import symbols fromaiac.policy.modelinstead ofaiac.policy.model.models. Either re-export the models from__init__.pyor change all consumers to the explicit.modelspath.
aiac/docs/specs/components/policy-model.md#L46-L53: document the chosen export behavior in the package structure.aiac/docs/specs/components/policy-model.md#L168-L171: update the Agent API import.aiac/docs/specs/components/policy-store.md#L105-L107: update the Policy Store import.aiac/docs/specs/components/policy-computation-engine.md#L175-L178: update the Agent/PCE import guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-model.md` around lines 46 - 53, Choose one consistent package import contract and apply it across the specifications: either document model re-exports from __init__.py or use the explicit .models path everywhere. Update aiac/docs/specs/components/policy-model.md lines 46-53 to document the choice, aiac/docs/specs/components/policy-model.md lines 168-171 for the Agent API import, aiac/docs/specs/components/policy-store.md lines 105-107 for the Policy Store import, and aiac/docs/specs/components/policy-computation-engine.md lines 175-178 for the Agent/PCE import guidance.aiac/docs/specs/components/pdp-policy-writer-opa.md-289-304 (1)
289-304: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine atomicity or recovery for partial
/policyupdates.
POST /policyiterates through agents and patches the CR individually. If a later Kubernetes write fails, earlier packages remain updated while the endpoint returns502, leaving a partially applied policy. Specify rollback/staging behavior or an explicit retry/reconciliation contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/pdp-policy-writer-opa.md` around lines 289 - 304, Define the failure contract for POST /policy and its per-agent _upsert_agent flow: ensure a failed multi-agent update either stages and applies the complete policy atomically, rolls back successful earlier patches, or explicitly documents retry/reconciliation behavior for partial application. Update the behavior notes to state the selected recovery semantics, including what clients should expect after a Kubernetes write failure.aiac/docs/specs/components/policy-computation-engine.md-56-64 (1)
56-64: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve the synchronous versus fire-and-forget contract.
compute_and_apply(...) -> Noneis documented as a regular direct call, but the user story promises that callers are not blocked. The integration plan also calls it directly and expects store/PDP effects to be complete afterward. Specify either synchronous best-effort behavior or an actual background/async contract, then align the tests and callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-computation-engine.md` around lines 56 - 64, Clarify the contract for compute_and_apply: choose either synchronous best-effort execution or genuine background/async execution, then update its documentation, integration callers, and tests consistently. If retaining fire-and-forget, define how callers observe completion before relying on Policy Store/PDP effects; otherwise document direct synchronous completion while preserving exception logging and non-propagation.aiac/docs/specs/components/policy-computation-engine.md-98-104 (1)
98-104: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine recovery when the PDP push fails after store commits.
The store becomes updated before
apply_policyruns, and exceptions are intentionally swallowed. A PDP failure therefore leaves the durable policy store and derived OPA policy inconsistent indefinitely unless retries, reconciliation, or an outbox-style mechanism is specified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-computation-engine.md` around lines 98 - 104, Define the recovery behavior for failures in the PDP push after apply_agent_policy commits, including how the system retries or reconciles the durable Policy Store with OPA when apply_policy fails. Update the “PDP push” step to specify the failure handling and prevent indefinite inconsistency while preserving the fire-and-forget behavior.aiac/docs/specs/components/idp-configuration-service.md-94-94 (1)
94-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the GET response-shape contract.
Line 94 says every GET except assignments returns an array, but
/services/{service_id}returns a single client object and/healthreturns server information. Clients following the generalized statement may deserialize these responses incorrectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/idp-configuration-service.md` at line 94, Update the GET response-shape contract to explicitly exclude `/services/{service_id}` and `/health` alongside `/subjects/{subject_id}/assignments`; document that the service endpoint returns a single client object and the health endpoint returns server information, while retaining the array response for the remaining GET endpoints.aiac/docs/specs/components/library-idp.md-128-128 (1)
128-128: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not retry non-idempotent writes blindly.
Line 128 applies automatic retries to every HTTP call, including scope/role creation and mappings. If Keycloak commits a write but the response is lost, the retry can return
409and surface a false failure or repeat a side effect. Restrict retries to safe operations, or reconcile ambiguous write failures before retrying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/library-idp.md` at line 128, The transport retry policy described for the private _request helper must not blindly retry non-idempotent writes such as service role, scope, and mapping creation. Restrict automatic retries to safe/idempotent operations, or add reconciliation for ambiguous write failures before retrying, while preserving retries for transient failures on eligible requests and avoiding compounded retries in composite methods.aiac/docs/specs/components/idp-configuration-service.md-119-119 (1)
119-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the canonical IdP library import path.
This line references
aiac.idp.library.configuration, while the companion library specification definesaiac.idp.configuration.modelsandaiac.idp.configuration.api. Replace this path or explicitly document a compatibility alias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/idp-configuration-service.md` at line 119, Update the Python library reference in the IdP configuration service specification to use the canonical `aiac.idp.configuration.models` and `aiac.idp.configuration.api` paths defined by the companion specification, or explicitly document `aiac.idp.library.configuration` as a compatibility alias.aiac/docs/specs/components/library-idp.md-235-250 (1)
235-250: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn an actually enriched
Serviceafter mapping.The configuration service documents
GET /services/{service_id}as returning raw client JSON, while these methods re-fetch that endpoint and parse it directly. The returned model therefore may have emptyrolesandscopes, contrary to the enrichment contract ofget_service(). Specify that these methods callget_service(service.id)or perform equivalent enrichment before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/library-idp.md` around lines 235 - 250, The mapping methods map_scope_to_service and map_role_to_service must return an enriched Service rather than parsing the raw GET response directly. After the mapping POST succeeds, call get_service(service.id) or apply the same enrichment behavior before returning, while preserving the existing error handling and create_role behavior.aiac/docs/specs/components/pdp-policy-keycloak-service.md-19-19 (1)
19-19: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not describe the two-step scope workflow as atomic without rollback.
The implementation creates the scope and then assigns it in a separate call. If assignment fails, the newly created realm scope remains orphaned. Add compensating cleanup/transactional behavior, or change the contract to document partial-failure recovery.
Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/pdp-policy-keycloak-service.md` at line 19, The POST /services/{service_id}/scopes contract must not claim the two-step Keycloak scope creation and assignment is atomic. Either implement compensating deletion of the newly created scope when default-scope assignment fails, or revise the contract to document partial failures and the required orphan-scope recovery behavior.aiac/docs/specs/components/pdp-policy-keycloak-service.md-71-77 (1)
71-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHonor
aiac.managedwhen selecting and deleting policy entities.The marker is documented as the boundary between AIAC-provisioned entities and Keycloak built-ins, but these flows ignore it.
aiac/docs/specs/components/pdp-policy-keycloak-service.md#L71-L77: filter rebuilds to AIAC-managed roles before removing composites; otherwise built-in mappings such asdefault-roles-<realm>can be revoked.aiac/docs/specs/components/library-idp.md#L252-L260: reuse only AIAC-managed existing roles/scopes, or explicitly handle same-name unmanaged conflicts instead of attaching them silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/pdp-policy-keycloak-service.md` around lines 71 - 77, Honor the aiac.managed marker when selecting policy entities: in aiac/docs/specs/components/pdp-policy-keycloak-service.md lines 71-77, filter roles to AIAC-managed roles before removing composites, preserving built-in mappings; in aiac/docs/specs/components/library-idp.md lines 252-260, reuse existing roles and scopes only when they are AIAC-managed, and explicitly handle same-name unmanaged conflicts instead of attaching them silently.aiac/docs/specs/components/rag-ingest-service.md-18-18 (1)
18-18: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftConstrain URL ingestion to prevent SSRF.
The service fetches caller-supplied URLs, potentially including redirects to cluster-internal services or cloud metadata endpoints. Define an allowlist or egress policy, block private/link-local destinations, restrict redirects, and enforce connection, response-size, and read timeouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/rag-ingest-service.md` at line 18, Update the POST /ingest/{collection}/url contract to require SSRF protections for caller-supplied URLs: enforce an allowlist or equivalent egress policy, reject private and link-local destinations including cloud metadata endpoints, restrict redirects to approved destinations, and apply connection, response-size, and read timeouts.aiac/docs/specs/demo/github-agent.md-91-92 (1)
91-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdvertise a reachable AgentCard endpoint.
The fallback
http://host:port/is a placeholder, and the deployment section does not setAGENT_ENDPOINT. Clients following the card will call the wrong host. RequireAGENT_ENDPOINTin deployment or default it to the deployed Service URL and fail startup when it is absent.Also applies to: 183-185, 201-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/demo/github-agent.md` around lines 91 - 92, Update the AgentCard endpoint configuration in the deployment examples and related sections to require AGENT_ENDPOINT, or derive it from the deployed Service URL when unset. Remove the http://host:port/ placeholder and ensure startup fails when neither a valid AGENT_ENDPOINT nor Service URL is available, so advertised supported_interfaces always points to a reachable endpoint.aiac/docs/specs/components/rag-ingest-service.md-16-20 (1)
16-20: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the previous collection until replacement succeeds.
Dropping and recreating the collection before all documents are embedded and written means a partial failure leaves the previous data gone and an empty collection behind. That is not an atomic rollback. Stage into a temporary collection and swap only after success, or remove the atomicity claim.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/rag-ingest-service.md` around lines 16 - 20, Update the Replace semantics documentation to preserve the existing collection until ingestion completes successfully: describe staging documents in a temporary collection and swapping it into place only after all embedding and writes succeed, with failures leaving the previous collection unchanged. Keep the empty-docs behavior explicit and ensure the atomicity claim matches this implementation.aiac/docs/specs/components/rag-ingest-service.md-40-44 (1)
40-44: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not silently drop policy-refresh events.
A successful ingest followed by a failed NATS publish leaves ChromaDB updated while the Agent never recomputes policy. Add a durable outbox/retry/reconciliation mechanism, or return an error when the trigger cannot be queued.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/rag-ingest-service.md` around lines 40 - 44, The post-ingest notification flow must not silently lose policy-refresh events when publishing to NATS fails. Update the documented behavior around successful replace, update, and delete operations to either durably queue and retry/reconcile the event before reporting success, or propagate a failure when the trigger cannot be queued; remove the fire-and-forget guarantee and ensure ChromaDB changes cannot be reported as fully successful without a recoverable policy-refresh trigger.aiac/docs/specs/demo/github-agent.md-66-70 (1)
66-70: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftFail closed instead of allowing unauthenticated MCP access.
If neither
GITHUB_TOKENnor an inboundAuthorizationheader exists, the agent still proceeds. A warning is not an authentication control; require credentials or make this an explicit local-only mode that cannot be selected in deployment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/demo/github-agent.md` around lines 66 - 70, Update the Auth flow documentation for GithubExecutor.execute so the third tier no longer permits unauthenticated MCP access: require either GITHUB_TOKEN or the inbound Authorization header, and fail closed when both are absent. Only describe an unauthenticated path if it is explicitly restricted to a local-only mode that cannot be enabled in deployment.aiac/docs/specs/components/event-broker.md-51-56 (1)
51-56: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDocument at-least-once processing, not exactly-once processing.
A queue group selects one active consumer for each delivery, but a handler can run, perform side effects, crash before acknowledging, and then run again. Require idempotent handlers or event deduplication and replace “Exactly-one processing” with accurate at-least-once semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/event-broker.md` around lines 51 - 56, Update the Delivery Guarantees section to remove the “Exactly-one processing” claim and describe queue-group delivery as at-least-once: a handler may execute again after side effects if it crashes before acknowledgment. Document that Agent handlers must be idempotent or use event deduplication, while preserving the existing queue-group behavior description.aiac/docs/specs/components/event-broker.md-15-22 (1)
15-22: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude the DLQ from the Agent consumer’s subject filter.
aiac.apply.>also matchesaiac.apply.dlq, while the document assigns DLQ messages to manual operator inspection. Without a consumer filter or separate DLQ stream, the Agent can receive dead-letter messages and attempt normal dispatch.Also applies to: 28-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/event-broker.md` around lines 15 - 22, Update the Agent consumer subject filter in the event-broker specification to exclude aiac.apply.dlq while still matching normal aiac.apply subjects. Apply the same correction to the additional consumer configuration referenced by the comment, ensuring DLQ messages remain available for manual operator inspection rather than Agent dispatch.aiac/docs/specs/components/event-broker.md-21-22 (1)
21-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake DLQ routing and delivery semantics explicit. JetStream won’t automatically publish failed messages to
aiac.apply.dlq; it only emits a max-deliver advisory, so this spec needs the republisher/advisory consumer (or other DLQ mechanism) spelled out. Also,5means total deliveries including the first attempt, not 5 redeliveries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/event-broker.md` around lines 21 - 22, Update the event-broker specification to explicitly define the mechanism that consumes the JetStream max-deliver advisory and republishes the failed message to aiac.apply.dlq, or document the alternative DLQ mechanism used. Correct the delivery limit wording so 5 represents total deliveries, including the initial attempt, rather than 5 unacknowledged redeliveries.
🟡 Minor comments (8)
aiac/docs/specs/PRD.md-451-459 (1)
451-459: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the manifest count.
The section says “Four separate manifest files” but lists five entries. Either include only four files or change the count to five and clarify which entries are pending.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/PRD.md` around lines 451 - 459, Update the manifest count heading to five and clarify that event-broker-deployment.yaml and rag-statefulset.yaml are pending, while the other three manifests are currently defined.aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md-111-124 (1)
111-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken issue link The
../../issues/...reference resolves toaiac/docs/specs/issues/..., so it won’t navigate from this file. Point it at the actual issue-doc location instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md` around lines 111 - 124, Update the issue reference in the analyze_tool documentation to use the actual issue-document location rather than the resolving path under aiac/docs/specs/issues. Preserve the link target and surrounding hybrid Keycloak→K8s strategy text unchanged.aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md-146-146 (1)
146-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken intra-document link.
The
#the-agent-tool-gate-degenerate-by-designfragment does not match the heading containing the Unicode→character. Normalize the heading/anchor or update the fragment so the link resolves correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md` at line 146, Fix the intra-document link in the onboarding pipeline specification by making the `The agent→tool gate` heading anchor match `#the-agent-tool-gate-degenerate-by-design`. Update either the heading text or the link fragment, preserving the intended visible wording and ensuring the link resolves correctly.Source: Linters/SAST tools
aiac/docs/specs/components/policy-computation-engine.md-131-135 (1)
131-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the bootstrap contradiction.
This section says the PCE is not responsible for bootstrapping new agents, then immediately says the engine creates a fresh model after a store 404. State clearly whether fresh-model creation belongs to the PCE or another component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-computation-engine.md` around lines 131 - 135, Update the “Not Responsible For” section to remove the contradiction around bootstrapping AgentPolicyModel records: explicitly assign fresh-model creation after a store 404 to the correct component, and ensure the responsibility statement matches the described behavior.aiac/docs/specs/components/policy-computation-engine.md-106-113 (1)
106-113: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument
outbound_subject_rulesin additive and override semantics.The algorithm purges and de-duplicates
outbound_subject_rules, but the merge-semantics summary only names inbound and outbound rules. This omission can cause an implementation to preserve stale user-to-tool permissions duringoverride=True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-computation-engine.md` around lines 106 - 113, Update the Merge Semantics documentation to explicitly include outbound_subject_rules in both additive and override modes, covering preservation/appending and de-duplication for override=False and purging plus replacement during override=True. Ensure the summary matches the algorithm’s existing handling so stale user-to-tool permissions are removed in authoritative merges.aiac/docs/specs/demo/github-tool.md-266-268 (1)
266-268: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
localhostwhen testing through port-forward.
kubectl port-forwardexposes the service at the operator’s local address, whilegithub-tool.team1.svc.cluster.localonly resolves inside the cluster. Provide separate commands for in-cluster access and port-forward access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/demo/github-tool.md` around lines 266 - 268, Update the testing instructions around the tools/list POST request to provide separate in-cluster and port-forward commands: retain the cluster DNS URL for in-cluster access, but use the localhost address and forwarded port when running through kubectl port-forward.aiac/docs/specs/components/event-broker.md-41-47 (1)
41-47: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the policy-build event payload contract.
The Event Broker spec says the payload is
{}, while the RAG Ingest spec says it publishes{"id": ""}. Choose one wire format and update the producer, consumer, and tests consistently.
aiac/docs/specs/components/event-broker.md#L41-L47: define the canonical payload.aiac/docs/specs/components/rag-ingest-service.md#L40-L44: publish exactly that payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/event-broker.md` around lines 41 - 47, The policy-build event payload contract is inconsistent across the specifications. In aiac/docs/specs/components/event-broker.md lines 41-47, define the canonical payload format; then update aiac/docs/specs/components/rag-ingest-service.md lines 40-44 to publish exactly that same format, and align the related producer, consumer, and tests with the chosen contract.aiac/docs/specs/components/rag-knowledge-base.md-16-16 (1)
16-16: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify whether adding collections is configuration-only.
Both documents require a new slug-to-ChromaDB-name map entry, which is a code/configuration change unless that map is itself externalized. State the actual change procedure consistently.
aiac/docs/specs/components/rag-knowledge-base.md#L16-L16: correct the “configuration-only” claim or identify the external map.aiac/docs/specs/components/rag-ingest-service.md#L71-L71: use the same extension procedure and terminology.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/rag-knowledge-base.md` at line 16, Clarify the collection-extension procedure consistently in both aiac/docs/specs/components/rag-knowledge-base.md:16-16 and aiac/docs/specs/components/rag-ingest-service.md:71-71: either identify the externalized slug-to-ChromaDB-name map that makes adding a collection configuration-only, or state that adding a slug and map entry requires a code/configuration change. Use identical terminology and procedure in both documents.
🧹 Nitpick comments (9)
aiac/docs/specs/components/aiac-agent/policy-rules-builder.md (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the graph fence.
Mark the ASCII-art graph as
text; markdownlint reports the untyped fence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent/policy-rules-builder.md` at line 68, Update the ASCII-art graph code fence in the policy-rules-builder documentation to specify the text language, changing the untyped fence to a text-labeled fence while preserving the graph content.Source: Linters/SAST tools
aiac/docs/specs/components/aiac-agent.md (1)
203-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd languages to the fenced text blocks.
Mark the file tree and dependency list fences as
text; markdownlint reports both blocks.Also applies to: 233-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent.md` at line 203, Update the fenced code blocks around the file tree and dependency list in aiac-agent.md to specify the text language, including both locations referenced by the comment, so markdownlint recognizes them correctly.Source: Linters/SAST tools
aiac/docs/specs/ARCHITECTURE-SUMMARY.md (1)
105-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint errors in the diagrams.
Add
textto ASCII-art fences and change#### UC-1ato### UC-1aso heading levels do not jump. These are reported by markdownlint.Also applies to: 187-189, 212-212, 233-233, 256-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/ARCHITECTURE-SUMMARY.md` at line 105, Fix the Markdown lint issues in ARCHITECTURE-SUMMARY.md by adding the text language tag to each ASCII-art code fence in the affected diagram sections and changing the UC-1a heading from level four to level three. Apply the heading correction to all occurrences and preserve the diagram content.Source: Linters/SAST tools
aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint errors in this spec.
Remove blank lines inside the dependency blockquote and mark the ASCII-art fences as
text; these are reported by markdownlint.Also applies to: 78-78, 225-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md` around lines 3 - 5, Fix the Markdown lint issues in the service-onboarding spec: remove blank lines within the dependency blockquote, and annotate each ASCII-art fenced code block with the text language marker, including the occurrences at the referenced locations.Source: Linters/SAST tools
aiac/docs/specs/components/aiac-agent/uc2-policy-update.md (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the dependency blockquote contiguous.
The blank lines inside the blockquote trigger MD028 and can split the rendered note into separate blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent/uc2-policy-update.md` around lines 3 - 7, Remove the blank lines between the consecutive blockquote lines in the dependency note, keeping the Status, Depends on, and IdP access lines contiguous as one Markdown blockquote.Source: Linters/SAST tools
aiac/docs/specs/PRD.md (1)
122-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint errors in the architecture diagrams.
Add
textto the ASCII-art fenced blocks. These missing language identifiers are reported by markdownlint.Also applies to: 176-176, 202-202, 226-226, 252-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/PRD.md` at line 122, Update the fenced ASCII-art blocks in PRD.md, including the blocks near the referenced sections, to specify the text language identifier after each opening fence. Preserve the diagram contents and formatting while ensuring all affected fences satisfy markdownlint.Source: Linters/SAST tools
aiac/docs/specs/components/aiac-agent/uc3-role-update.md (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint errors in this spec.
Remove the blank line inside the dependency blockquote and mark the ASCII-art file tree as
text.Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/aiac-agent/uc3-role-update.md` around lines 3 - 5, Fix Markdown linting in the role-update specification by removing the blank line within the dependency blockquote and labeling the ASCII-art file-tree code fence as text. Preserve the dependency content and tree formatting otherwise.Source: Linters/SAST tools
aiac/docs/specs/components/policy-model.md (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace “dependency-free” with “transport-independent.”
This module still depends on
aiac.idp.configuration.modelsforRoleandScope; describe it as transport-independent or dependency-light to avoid contradicting the dependency table and misleading layer-boundary documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/policy-model.md` around lines 17 - 21, Update the Solution description in the policy model specification to replace “dependency-free” with “transport-independent” (or “dependency-light”), accurately reflecting its Role and Scope dependencies while preserving the existing description of its lack of HTTP or service-layer code.aiac/docs/specs/components/event-broker.md (1)
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the NATS image version.
Replace
nats:latestwith an immutable version or digest so JetStream behavior and the deployment image remain reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/components/event-broker.md` around lines 70 - 75, Update the Runtime section’s NATS Image entry to replace nats:latest with a pinned immutable version or image digest, preserving the JetStream-enabled configuration and the existing deployment details.
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Review summary
Docs-only PR adding the full AIAC (AI Access Control) specification set — 26 new Markdown files, +5422/-0. No security, secret, or supply-chain concerns (no .claude//.vscode/ changes; credential references are all env-var names / Secret pointers, no real values). The substance is internal consistency across the specs, which I reviewed with a cross-doc pass. All findings are documentation-consistency suggestions — none block merge.
Beyond the inline comments, worth a pass:
- IdP library module path has ~4 spellings across docs —
aiac.idp.configurationvsaiac.idp.library.configuration(idp-configuration-service.md:119) vsaiac.idp.configuration.library(policy-computation-engine.md:170) vsaiac.idp.library(ARCHITECTURE-SUMMARY.md:161). Settle on one (canonical looks likeaiac.idp.configuration). - Same component named two ways — "IdP Configuration Service" vs "PDP Configuration Service" (event-broker.md:47/90, pdp-policy-keycloak-service.md:9/21/47).
- Broken heading anchor uc1-onboarding-pipeline.md:146 — the
→in the target heading breaks the GitHub slug. Wrong link depth policy-pipeline.md:382 (../../src/...should be../../../src/...). - Confirm the incomplete
TBDtable cells are intended to ship: PRD.md:544 (| AIAC Agent | TBD | TBD |), pdp-policy-writer-opa.md:227-228 (AUTHORIZATION_POLICY_NAME/_NAMESPACE).
Thorough, well-structured spec set overall. Approving; the inline items are cleanups that can land here or as a quick follow-up.
Assisted-By: Claude Code
| | Layer | Component | Responsibility | | ||
| |---|---|---| | ||
| | **Policy Management** | AIAC Agent | Translates natural-language policy into PDP configuration on every trigger | | ||
| | **Policy Decision (PDP)** | OPA | Evaluates LLM-generated Rego rules; issues scoped tokens | |
There was a problem hiding this comment.
suggestion: "Evaluates LLM-generated Rego rules; issues scoped tokens" — OPA is the PDP: it returns an allow/deny decision (or entitlements), it does not mint OAuth tokens. RFC 8693 token issuance is the authorization server's (Keycloak) role, initiated by the PEP (AuthBridge) — which the very next row (:39) states correctly. This also contradicts the parallel table in PRD.md:55 ("decides what a caller may access"). Suggest rewording to "...returns the caller's entitlements/decision".
| | **Policy Decision (PDP)** | OPA | Evaluates LLM-generated Rego rules; decides what a caller may access | | ||
| | **Policy Enforcement (PEP)** | AuthBridge | Intercepts traffic; exchanges tokens; carries no policy knowledge | | ||
|
|
||
| The PEP (AuthBridge) is a pure enforcement layer. It performs RFC 8693 token exchanges sending only the target `audience` — no `scope` parameter. OPA evaluates the caller's role against the Rego rules and issues a token containing exactly the entitlements that role grants on the target service. |
There was a problem hiding this comment.
suggestion: Same issue as ARCHITECTURE-SUMMARY.md:38 — "OPA ... issues a token containing exactly the entitlements" is inaccurate and self-contradicts this file's own table row at :55 ("OPA ... decides what a caller may access"). OPA returns the decision/entitlements; the exchanged RFC 8693 token is issued by Keycloak via AuthBridge. Reword so OPA decides and the IdP issues the token.
| ## Description | ||
| A FastAPI web service that translates a **Policy Model** into OPA Rego packages and writes them to an `AuthorizationPolicy` Kubernetes Custom Resource. The OPA plugin embedded in each AuthBridge instance fetches the Rego packages relevant to its pod from this CR at startup. | ||
|
|
||
| The service is deployed as a container in the **Kagenti Interface Pod** alongside the IdP Configuration Service, behind the `aiac-pdp-policy-writer-service:7072` ClusterIP. |
There was a problem hiding this comment.
suggestion: Service name aiac-pdp-policy-writer-service:7072 here (and at :244) disagrees with aiac-pdp-policy-service:7072 used in every other doc (ARCHITECTURE-SUMMARY.md:97, PRD.md:112, components/event-broker.md:103, pdp-policy-keycloak-service.md:46). It also breaks the stability promise in pdp-policy-keycloak-service.md:9 ("The service name and port remain stable"). Pick one name.
|
|
||
| --- | ||
|
|
||
| ## Library: `aiac.pdp.library.policy` |
There was a problem hiding this comment.
suggestion: This ## Library: aiac.pdp.library.policy heading and the import at :213 use exactly the path that library-pdp-policy.md:111 marks deprecated: "The aiac.pdp.library.policy module (old path) is deprecated. All consumers must update imports to aiac.pdp.policy.library.api." Update this doc to the canonical path.
|
|
||
| **Dependencies:** `fastapi`, `uvicorn[standard]`, `pydantic`. `sqlite3` is stdlib (no new dependency). | ||
|
|
||
| **Imports:** `from aiac.policy.model import PolicyModel, AgentPolicyModel` |
There was a problem hiding this comment.
suggestion: from aiac.policy.model import PolicyModel, AgentPolicyModel imports from the package root, but PRD.md:310 states all aiac.* __init__.py are empty and callers must use explicit submodules (from aiac.policy.model.models import ...), which the other docs do (library-pdp-policy.md:21, policy-model.md:119). As written this import would fail.
|
|
||
| > K8s access: `list` on `agentcards.agent.kagenti.dev` in the target namespace. | ||
|
|
||
| - **`analyze_tool`**: non-LLM node; discovers MCP tools. `namespace` + `workload_name` are already resolved by `classify_service` (from the `client.name` split). MCP endpoint lookup uses the **hybrid Keycloak→K8s strategy** decided in issue [`6.2`](../../issues/agent/service-onboarding/6.2-analyze-tool-lookup-strategy.md): the Keycloak client name supplied the key `{namespace, workload_name}`; K8s supplies the reachable endpoint. |
There was a problem hiding this comment.
nit: Link [6.2](../../issues/agent/service-onboarding/6.2-analyze-tool-lookup-strategy.md) resolves inside the new specs/ tree to a file not added in this PR (dangling). Also issues/ here vs gh-issues/ elsewhere (github-tool.md:16, uc1-onboarding-pipeline.md:38) — pick one directory name.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md (1)
180-187: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not label the subject-only probe as outbound authorization.
The documented production rule is
allow = subject_ok AND target_ok, and this section explicitly saystarget_okis empty. Therefore the real outboundallowdecision is deny for every subject/function; the displayed ✅ values only representsubject_ok. Rename the table to “Outbound user-gate” and separately assert the fullallowresult, or maketarget_oknon-degenerate before treating these as authorization outcomes.Also applies to: 220-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md` around lines 180 - 187, Correct the “Outbound allow(subject, function)” documentation to avoid presenting subject-only results as full authorization decisions. Rename the table to “Outbound user-gate” and describe its values as subject_ok, then separately assert that the production allow result is denied for every subject/function while target_ok is empty; alternatively, make target_ok non-degenerate before documenting authorization outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@aiac/docs/specs/integration-test/uc1-onboarding-pipeline.md`:
- Around line 180-187: Correct the “Outbound allow(subject, function)”
documentation to avoid presenting subject-only results as full authorization
decisions. Rename the table to “Outbound user-gate” and describe its values as
subject_ok, then separately assert that the production allow result is denied
for every subject/function while target_ok is empty; alternatively, make
target_ok non-degenerate before documenting authorization outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02b653ac-0d2b-4ab7-8880-9042fe0e0483
📒 Files selected for processing (9)
aiac/docs/specs/ARCHITECTURE-SUMMARY.mdaiac/docs/specs/PRD.mdaiac/docs/specs/components/aiac-agent/uc1-service-onboarding.mdaiac/docs/specs/components/event-broker.mdaiac/docs/specs/components/pdp-policy-keycloak-service.mdaiac/docs/specs/components/pdp-policy-writer-opa.mdaiac/docs/specs/components/policy-store.mdaiac/docs/specs/integration-test/policy-pipeline.mdaiac/docs/specs/integration-test/uc1-onboarding-pipeline.md
🚧 Files skipped from review as they are similar to previous changes (8)
- aiac/docs/specs/ARCHITECTURE-SUMMARY.md
- aiac/docs/specs/components/pdp-policy-keycloak-service.md
- aiac/docs/specs/components/event-broker.md
- aiac/docs/specs/components/policy-store.md
- aiac/docs/specs/components/aiac-agent/uc1-service-onboarding.md
- aiac/docs/specs/components/pdp-policy-writer-opa.md
- aiac/docs/specs/integration-test/policy-pipeline.md
- aiac/docs/specs/PRD.md
Import the AIAC PRD and per-component specs under aiac/docs/specs/ as the basis for the upstream contribution. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Oleg Blinder <olegb@il.ibm.com> (cherry picked from commit a81869a)
…equisites §7 Deployment: - Add protocol.kagenti.io/a2a label requirement on both Deployment metadata.labels and pod template; explain it gates AgentCardSyncReconciler.shouldSyncWorkload() (operator stamps kagenti.io/type=agent but not the protocol label) - Replace single Service port 8080→8000 with two-port shape: agent:8001→8001 first (direct path past authbridge proxy, used by AgentCardReconciler for card fetch), proxy:8080→8000 second (authenticated path for A2A clients); document why port order matters (getServicePort() always takes Ports[0]) §8 Verification step 8: add AgentCard CR auto-creation check Signed-off-by: Oleg Blinder <olegb@il.ibm.com> (cherry picked from commit dd5691b)
(cherry picked from commit 634c42d) Signed-off-by: Oleg Blinder <oblinder@gmail.com>
(cherry picked from commit dac6b08) Signed-off-by: Oleg Blinder <oblinder@gmail.com>
OPA is the PDP and returns an allow/deny decision or entitlements; it does not mint OAuth tokens. Token issuance under RFC 8693 is Keycloak's role as authorization server, initiated by AuthBridge as PEP. Signed-off-by: Oleg Blinder <olegb@il.ibm.com> (cherry picked from commit 2c8df4d)
(cherry picked from commit a511c6f) Signed-off-by: Oleg Blinder <oblinder@gmail.com>
(cherry picked from commit d7d5d3f) Signed-off-by: Oleg Blinder <oblinder@gmail.com>
Signed-off-by: Oleg Blinder <olegb@il.ibm.com> (cherry picked from commit 35fea8e)
Signed-off-by: Oleg Blinder <olegb@il.ibm.com> (cherry picked from commit 65b97ad)
github-project-automation
Bot
moved this from New/ToDo
to Done
in Kagenti Issue Prioritization
3 participants
Summary
Delivers the AIAC end-goal documentation requested in #644 (sub-issue of the AIAC epic #643):
a human-readable overview, the per-component behavioral specifications, and the test plan — all
placed under
aiac/docs/specs/. A reader can understand what AIAC is, what each component does,and how a request flows through the system without reading source code.
Major Use-Cases
UC-1 · Continuous Access Reconciliation (On-boarding / Off-boarding)
Trigger: A Role or Keycloak Client is created, updated, or removed.
The Keycloak SPI listener publishes a scoped event to the Event Broker. The AIAC Agent retrieves
relevant context from the RAG store, reads the current OPA policy state, and asks the LLM to
compute the minimal permission diff scoped to the affected entity. The diff is validated by a
second LLM pass and applied to OPA as updated Rego rules. Supports both auto-apply (fully
automated, least-privilege) and recommendation + human review modes.
UC-2 · Policy Update Reconciliation
Trigger: An operator ingests updated documents into the RAG store.
After ingestion the RAG Ingest Service publishes a build event. The AIAC Agent retrieves all
relevant context, computes a full policy diff against current OPA state, and applies the delta.
A
rebuildvariant (operator-only, direct HTTP) first clears all OPA policy rules beforerecomputing from scratch — used when policy changes are too broad for incremental diff.
UC-3 · Entitlements Review
Trigger: Operator request (on-demand or scheduled).
The agent evaluates all current OPA policy rules — including manually added ones that AIAC did
not create — against the natural-language policy. It reports compliant, non-compliant, and
policy-agnostic entitlements, enabling audit and remediation workflows.
UC-4 · Access Request
Trigger: User request via chatbot.
A user requests an entitlement grant. The agent verifies the request against the policy
(permissive approach) and either auto-grants or routes to a human approver (man-in-the-loop).
Manually granted entitlements are flagged as policy-agnostic and surfaced during UC-3 reviews.
AIAC Component Architecture
Eight components across five Kubernetes Pods plus a Python library layer, all implemented in Python 3.12. External dependencies: Keycloak Admin API, an LLM API, and an embedding API. The Keycloak SPI listener is defined in a separate PRD.
aiac.idp.configuration.AuthorizationPolicyKubernetes CR. Exposed as ClusterIP serviceaiac-pdp-policy-service:7072. Python library:aiac.pdp.policy.library.PolicyModelcache backed by SQLite as the authoritative structured policy store. Enables the Policy Computation Engine to read currentAgentPolicyModelstate for additive merging. Deployed as a dedicated single-replica StatefulSet (aiac-policy-store) at:7074. Python library:aiac.policy.store.library.aiac.policy.computation). No service, no Kubernetes deployment. Receiveslist[PolicyRule]from AIAC Agent sub-agents, queries IdP to resolve owning services, additively merges rules intoAgentPolicyModelobjects in the Policy Store, and pushes the updatedPolicyModelto the PDP Policy Writer. Single entry point:compute_and_apply(rules).aiac.apply.>subjects) and directly by the operator (rebuildonly). Retrieves the current policy from the RAG store, interprets it against live PDP state, and applies the required policy changes immediately.aiac.idp.configuration,aiac.policy.model,aiac.policy.store.library,aiac.pdp.policy.library, andaiac.policy.computationmodules backed by generic Pydantic models.All inter-pod traffic is Kubernetes ClusterIP. External access is exclusively via
kubectl port-forward(operator/developer) or NATS publish (Keycloak SPI, RAG Ingest).What's included
Architectural Summary — end-to-end, plain-language orientation
aiac/docs/specs/ARCHITECTURE-SUMMARY.mdMaster PRD — technical reference (ports, schemas, libraries)
aiac/docs/specs/PRD.mdComponent specifications — intended behavior of each part, cross-referenced
components/aiac-agent.md,components/aiac-agent/policy-rules-builder.md,components/aiac-agent/uc1-service-onboarding.md,uc2-policy-update.md,uc3-role-update.mdcomponents/idp-configuration-service.md,components/library-idp.md,components/keycloak-service.mdcomponents/policy-model.md,components/policy-store.md,components/policy-computation-engine.md,components/library-policy-store.md,components/library-pdp-policy.md,components/pdp-policy-writer-opa.md,components/pdp-policy-keycloak-service.mdcomponents/event-broker.md(+
event-broker-redhat-amq-evaluation.md),components/rag-ingest-service.md,components/rag-knowledge-base.mddemo/github-agent.md,demo/github-tool.mdTest plan — how each phase's behavior is validated
integration-test/uc1-onboarding-pipeline.md,integration-test/policy-pipeline.md,integration-test/pdp-policy-writer.mdAcceptance criteria (#644)
cross-reference one another and the Architectural Summary
components) is available ahead of implementation
Closes #644
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit