feat(authz): make v2 request limits configurable#3508
Merged
Conversation
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds configurable request limits for authorization v2, relaxes proto upper-bound validations, enforces limits at service runtime via new validators called early in RPC handlers, updates config and tests, and refreshes gRPC/OpenAPI/docs to reflect the new semantics. ChangesRequest Limits Configuration and Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates request limit validation for Authorization v2 from static Protocol Buffer CEL expressions to a configurable service-side implementation. This change enhances flexibility by allowing administrators to adjust request limits via configuration rather than requiring code changes, while maintaining strict validation of incoming requests. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. CEL was strict and hard to change, Now limits have a wider range. Configured now with ease and grace, The service runs at better pace. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request transitions the authorization service from hardcoded request limits to configurable ones. Changes include removing CEL-based size constraints in the Protobuf definitions, adding a new request_limits configuration section, and implementing a dedicated validation layer to enforce these limits. Documentation and tests have been updated to reflect the shift from static to dynamic limits. Review feedback identified potential nil pointer dereferences in the new validation functions for entity identifiers and resources, which could lead to runtime panics if not addressed.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
jakedoublev
changed the title
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Make DecisionRequestFulfillableObligationFqnsMax require > 0 consistent with all other request limits. Add boundary tests verifying exactly-at-limit values pass validation. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Add ErrInvalidRequestLimitConfig sentinel error and extract requestLimitConfigError helper to reduce duplication. Also replace context.Background() with t.Context() in validation tests. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@service/authorization/v2/authorization_test.go`:
- Around line 661-677: Add a parallel test case to cover
GetDecisionMultiResourceRequest with 51 obligations: create a test entry
mirroring the existing single-request case (name like "multi-entity: registered
resource, action: create, resource: registered, obligations - 51") that builds a
&authzV2.GetDecisionMultiResourceRequest using the same
sampleRegisteredResourceFQN for the entity and resource, sampleActionCreate for
Action, and fiftyOneObligations for FulfillableObligationFqns; place it
alongside the other table-driven cases in authorization_test.go so the
multi-resource proto path (GetDecisionMultiResourceRequest and its
FulfillableObligationFqns) is exercised and the 51-obligation success behavior
is validated.
In `@service/authorization/v2/validation_test.go`:
- Around line 60-67: The tests in
Test_validateGetDecisionMultiResourceRequest_DefaultRequestLimit (and the other
ranges) only assert resource count limits for GetDecisionMultiResourceRequest
and miss exercising the FulfillableObligationFqns branch, so add assertions and
test cases that validate obligation FQNs are checked against the configured
limit and that the branch is bound to the correct config key; update the test
helper newDecisionMultiResourceRequestWithResourceCount (or add new fixtures) to
produce requests with non-empty FulfillableObligationFqns and add cases for
valid/invalid counts, and verify validateGetDecisionMultiResourceRequest returns
connect.CodeInvalidArgument with the expected error message when obligation FQNs
exceed the limit and passes when within limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d29f2ca-57fb-488c-b7b6-98ec86fa7fb5
⛔ Files ignored due to path filters (1)
protocol/go/authorization/v2/authorization.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
docs/Configuring.mddocs/grpc/index.htmldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/entityresolution/v2/entity_resolution.openapi.yamlservice/authorization/v2/authorization.goservice/authorization/v2/authorization.protoservice/authorization/v2/authorization_test.goservice/authorization/v2/config.goservice/authorization/v2/config_test.goservice/authorization/v2/validation.goservice/authorization/v2/validation_test.go
Contributor
Add proto and service-side tests for FulfillableObligationFqns on GetDecisionMultiResourceRequest, which was previously untested. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
elizabethhealy
previously approved these changes
May 21, 2026
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/openapi/policy/namespaces/namespaces.openapi.yaml (1)
1038-1055:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark
idas required onUpdateNamespaceRequest.The schema currently allows
{}even though this RPC cannot identify a target namespace withoutid.Suggested fix
policy.namespaces.UpdateNamespaceRequest: type: object properties: id: type: string title: id format: uuid description: Required metadata: title: metadata description: Optional $ref: '`#/components/schemas/common.MetadataMutable`' metadataUpdateBehavior: title: metadata_update_behavior $ref: '`#/components/schemas/common.MetadataUpdateEnum`' + required: + - id title: UpdateNamespaceRequest additionalProperties: false🤖 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 `@docs/openapi/policy/namespaces/namespaces.openapi.yaml` around lines 1038 - 1055, The UpdateNamespaceRequest schema allows empty objects because the id field isn't marked required; update the policy.namespaces.UpdateNamespaceRequest schema to include a required list containing "id" so the RPC cannot be called without an id (refer to the UpdateNamespaceRequest schema and its id property) and keep additionalProperties: false unchanged.docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml (3)
665-670:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
google.protobuf.Timestampexamples regressed to invaliddate-timevalues. The shared root cause is the example generator/template now emitting duration literals (1s,1.000340012s) for a schema that still documents RFC3339 timestamps.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 665 - 670, The examples for the OpenAPI schema component google.protobuf.Timestamp are wrong (they show duration literals like "1s" and "1.000340012s") even though the schema declares type: string and format: date-time; update the examples to valid RFC3339 timestamp strings (e.g. "1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") in the google.protobuf.Timestamp examples array so they match the declared date-time format and validate correctly.
2255-2287:⚠️ Potential issue | 🟠 MajorFix Connect Error schema: use
detailsarray, not singulardetail.
Indocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml, theconnect.errorschema defines a singledetail(#/components/schemas/google.protobuf.Any) and never definesdetails; Connect’s JSON HTTP error body usesdetails(plural) as an array, so multi-detail errors are mis-modeled and can be lost for clients.
Update the Connect/OpenAPI generator/template to emitdetails: [{ type, value, ... }]per Connect.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 2255 - 2287, The OpenAPI schema for the Connect error object (schema name connect.error) incorrectly defines a singular "detail" property; change this to "details" as an array of google.protobuf.Any (e.g., details: type: array, items: $ref '`#/components/schemas/google.protobuf.Any`') so multiple error details are preserved, and update the OpenAPI generator/template that emits the connect.error schema to produce "details" (array) instead of "detail" (singular).
1315-1473:⚠️ Potential issue | 🟠 MajorFix
oneOf+ parentadditionalProperties: false(branch-only fields get rejected)In
openapi: 3.1.0schemas, the parent object hasadditionalProperties: falsewhile theoneOf-selected fields are only declared inside theoneOfbranches (parent lists onlyid/base fields). This causes validators to treat those branch-only fields as forbidden additional properties.
docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml:policy.kasregistry.GetKeyAccessServerRequest(kasId/name/uriinoneOf, onlyidin parent)docs/openapi/policy/namespaces/namespaces.openapi.yaml:policy.namespaces.GetNamespaceRequest(fqn/namespace_idinoneOf, onlyidin parent)docs/openapi/policy/actions/actions.openapi.yaml(and other policy bundles):policy.Action(custom/standardinoneOf, onlyid/name/namespace/metadatain parent)Fix by moving union fields onto the parent
properties(as optional) or switching the generator tounevaluatedProperties: falseinstead ofadditionalProperties: false.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 1315 - 1473, The OpenAPI schemas use oneOf branches that declare fields (e.g., kasId/name/uri in policy.kasregistry.GetKeyAccessServerRequest and KasKeyIdentifier, fqn/namespace_id in policy.namespaces.GetNamespaceRequest, and custom/standard in policy.Action) while the parent objects set additionalProperties: false, causing branch-only properties to be rejected; fix by moving the union fields from each oneOf branch into the parent properties as optional entries (so the parent lists kasId, name, uri, kid, fqn, namespace_id, custom, standard, etc.) or alternatively replace additionalProperties: false with unevaluatedProperties: false on these parent schemas so the oneOf-evaluated properties are allowed — update policy.kasregistry.GetKeyAccessServerRequest, policy.kasregistry.KasKeyIdentifier, policy.namespaces.GetNamespaceRequest, and affected policy.Action schemas accordingly.
🤖 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.
Inline comments:
In `@docs/openapi/kas/kas.openapi.yaml`:
- Line 238: The OpenAPI enum text only documents 'hybrid-wrapped' as X-Wing but
omits the newly exposed ML-KEM hybrid algorithms; update the enum/value
documentation to include brief descriptions for
ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024, state
that these are ML-KEM hybrid-wrapped modes (alongside the existing X-Wing
explanation), and clarify any behavioral differences or mapping to the
'hybrid-wrapped' value so clients can map request/response behavior to those
algorithm enums.
---
Outside diff comments:
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 665-670: The examples for the OpenAPI schema component
google.protobuf.Timestamp are wrong (they show duration literals like "1s" and
"1.000340012s") even though the schema declares type: string and format:
date-time; update the examples to valid RFC3339 timestamp strings (e.g.
"1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") in the
google.protobuf.Timestamp examples array so they match the declared date-time
format and validate correctly.
- Around line 2255-2287: The OpenAPI schema for the Connect error object (schema
name connect.error) incorrectly defines a singular "detail" property; change
this to "details" as an array of google.protobuf.Any (e.g., details: type:
array, items: $ref '`#/components/schemas/google.protobuf.Any`') so multiple error
details are preserved, and update the OpenAPI generator/template that emits the
connect.error schema to produce "details" (array) instead of "detail"
(singular).
- Around line 1315-1473: The OpenAPI schemas use oneOf branches that declare
fields (e.g., kasId/name/uri in policy.kasregistry.GetKeyAccessServerRequest and
KasKeyIdentifier, fqn/namespace_id in policy.namespaces.GetNamespaceRequest, and
custom/standard in policy.Action) while the parent objects set
additionalProperties: false, causing branch-only properties to be rejected; fix
by moving the union fields from each oneOf branch into the parent properties as
optional entries (so the parent lists kasId, name, uri, kid, fqn, namespace_id,
custom, standard, etc.) or alternatively replace additionalProperties: false
with unevaluatedProperties: false on these parent schemas so the oneOf-evaluated
properties are allowed — update policy.kasregistry.GetKeyAccessServerRequest,
policy.kasregistry.KasKeyIdentifier, policy.namespaces.GetNamespaceRequest, and
affected policy.Action schemas accordingly.
In `@docs/openapi/policy/namespaces/namespaces.openapi.yaml`:
- Around line 1038-1055: The UpdateNamespaceRequest schema allows empty objects
because the id field isn't marked required; update the
policy.namespaces.UpdateNamespaceRequest schema to include a required list
containing "id" so the RPC cannot be called without an id (refer to the
UpdateNamespaceRequest schema and its id property) and keep
additionalProperties: false unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 009d0fce-2bb4-42fb-a552-d463285abdcb
📒 Files selected for processing (18)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/entityresolution/entity_resolution.openapi.yamldocs/openapi/kas/kas.openapi.yamldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamldocs/openapi/wellknownconfiguration/wellknown_configuration.openapi.yamlservice/authorization/v2/authorization_test.goservice/authorization/v2/validation_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/openapi/policy/namespaces/namespaces.openapi.yaml (1)
1038-1055:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark
idas required onUpdateNamespaceRequest.The schema currently allows
{}even though this RPC cannot identify a target namespace withoutid.Suggested fix
policy.namespaces.UpdateNamespaceRequest: type: object properties: id: type: string title: id format: uuid description: Required metadata: title: metadata description: Optional $ref: '`#/components/schemas/common.MetadataMutable`' metadataUpdateBehavior: title: metadata_update_behavior $ref: '`#/components/schemas/common.MetadataUpdateEnum`' + required: + - id title: UpdateNamespaceRequest additionalProperties: false🤖 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 `@docs/openapi/policy/namespaces/namespaces.openapi.yaml` around lines 1038 - 1055, The UpdateNamespaceRequest schema allows empty objects because the id field isn't marked required; update the policy.namespaces.UpdateNamespaceRequest schema to include a required list containing "id" so the RPC cannot be called without an id (refer to the UpdateNamespaceRequest schema and its id property) and keep additionalProperties: false unchanged.docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml (3)
665-670:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
google.protobuf.Timestampexamples regressed to invaliddate-timevalues. The shared root cause is the example generator/template now emitting duration literals (1s,1.000340012s) for a schema that still documents RFC3339 timestamps.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 665 - 670, The examples for the OpenAPI schema component google.protobuf.Timestamp are wrong (they show duration literals like "1s" and "1.000340012s") even though the schema declares type: string and format: date-time; update the examples to valid RFC3339 timestamp strings (e.g. "1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") in the google.protobuf.Timestamp examples array so they match the declared date-time format and validate correctly.
2255-2287:⚠️ Potential issue | 🟠 MajorFix Connect Error schema: use
detailsarray, not singulardetail.
Indocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml, theconnect.errorschema defines a singledetail(#/components/schemas/google.protobuf.Any) and never definesdetails; Connect’s JSON HTTP error body usesdetails(plural) as an array, so multi-detail errors are mis-modeled and can be lost for clients.
Update the Connect/OpenAPI generator/template to emitdetails: [{ type, value, ... }]per Connect.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 2255 - 2287, The OpenAPI schema for the Connect error object (schema name connect.error) incorrectly defines a singular "detail" property; change this to "details" as an array of google.protobuf.Any (e.g., details: type: array, items: $ref '`#/components/schemas/google.protobuf.Any`') so multiple error details are preserved, and update the OpenAPI generator/template that emits the connect.error schema to produce "details" (array) instead of "detail" (singular).
1315-1473:⚠️ Potential issue | 🟠 MajorFix
oneOf+ parentadditionalProperties: false(branch-only fields get rejected)In
openapi: 3.1.0schemas, the parent object hasadditionalProperties: falsewhile theoneOf-selected fields are only declared inside theoneOfbranches (parent lists onlyid/base fields). This causes validators to treat those branch-only fields as forbidden additional properties.
docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml:policy.kasregistry.GetKeyAccessServerRequest(kasId/name/uriinoneOf, onlyidin parent)docs/openapi/policy/namespaces/namespaces.openapi.yaml:policy.namespaces.GetNamespaceRequest(fqn/namespace_idinoneOf, onlyidin parent)docs/openapi/policy/actions/actions.openapi.yaml(and other policy bundles):policy.Action(custom/standardinoneOf, onlyid/name/namespace/metadatain parent)Fix by moving union fields onto the parent
properties(as optional) or switching the generator tounevaluatedProperties: falseinstead ofadditionalProperties: false.🤖 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 `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml` around lines 1315 - 1473, The OpenAPI schemas use oneOf branches that declare fields (e.g., kasId/name/uri in policy.kasregistry.GetKeyAccessServerRequest and KasKeyIdentifier, fqn/namespace_id in policy.namespaces.GetNamespaceRequest, and custom/standard in policy.Action) while the parent objects set additionalProperties: false, causing branch-only properties to be rejected; fix by moving the union fields from each oneOf branch into the parent properties as optional entries (so the parent lists kasId, name, uri, kid, fqn, namespace_id, custom, standard, etc.) or alternatively replace additionalProperties: false with unevaluatedProperties: false on these parent schemas so the oneOf-evaluated properties are allowed — update policy.kasregistry.GetKeyAccessServerRequest, policy.kasregistry.KasKeyIdentifier, policy.namespaces.GetNamespaceRequest, and affected policy.Action schemas accordingly.
🤖 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.
Inline comments:
In `@docs/openapi/kas/kas.openapi.yaml`:
- Line 238: The OpenAPI enum text only documents 'hybrid-wrapped' as X-Wing but
omits the newly exposed ML-KEM hybrid algorithms; update the enum/value
documentation to include brief descriptions for
ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024, state
that these are ML-KEM hybrid-wrapped modes (alongside the existing X-Wing
explanation), and clarify any behavioral differences or mapping to the
'hybrid-wrapped' value so clients can map request/response behavior to those
algorithm enums.
---
Outside diff comments:
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 665-670: The examples for the OpenAPI schema component
google.protobuf.Timestamp are wrong (they show duration literals like "1s" and
"1.000340012s") even though the schema declares type: string and format:
date-time; update the examples to valid RFC3339 timestamp strings (e.g.
"1970-01-01T00:00:01Z" and "1970-01-01T00:00:01.000340012Z") in the
google.protobuf.Timestamp examples array so they match the declared date-time
format and validate correctly.
- Around line 2255-2287: The OpenAPI schema for the Connect error object (schema
name connect.error) incorrectly defines a singular "detail" property; change
this to "details" as an array of google.protobuf.Any (e.g., details: type:
array, items: $ref '`#/components/schemas/google.protobuf.Any`') so multiple error
details are preserved, and update the OpenAPI generator/template that emits the
connect.error schema to produce "details" (array) instead of "detail"
(singular).
- Around line 1315-1473: The OpenAPI schemas use oneOf branches that declare
fields (e.g., kasId/name/uri in policy.kasregistry.GetKeyAccessServerRequest and
KasKeyIdentifier, fqn/namespace_id in policy.namespaces.GetNamespaceRequest, and
custom/standard in policy.Action) while the parent objects set
additionalProperties: false, causing branch-only properties to be rejected; fix
by moving the union fields from each oneOf branch into the parent properties as
optional entries (so the parent lists kasId, name, uri, kid, fqn, namespace_id,
custom, standard, etc.) or alternatively replace additionalProperties: false
with unevaluatedProperties: false on these parent schemas so the oneOf-evaluated
properties are allowed — update policy.kasregistry.GetKeyAccessServerRequest,
policy.kasregistry.KasKeyIdentifier, policy.namespaces.GetNamespaceRequest, and
affected policy.Action schemas accordingly.
In `@docs/openapi/policy/namespaces/namespaces.openapi.yaml`:
- Around line 1038-1055: The UpdateNamespaceRequest schema allows empty objects
because the id field isn't marked required; update the
policy.namespaces.UpdateNamespaceRequest schema to include a required list
containing "id" so the RPC cannot be called without an id (refer to the
UpdateNamespaceRequest schema and its id property) and keep
additionalProperties: false unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 009d0fce-2bb4-42fb-a552-d463285abdcb
📒 Files selected for processing (18)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/authorization/v2/authorization.openapi.yamldocs/openapi/entityresolution/entity_resolution.openapi.yamldocs/openapi/kas/kas.openapi.yamldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamldocs/openapi/wellknownconfiguration/wellknown_configuration.openapi.yamlservice/authorization/v2/authorization_test.goservice/authorization/v2/validation_test.go
🛑 Comments failed to post (1)
docs/openapi/kas/kas.openapi.yaml (1)
238-238:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the new ML-KEM hybrid modes here too.
These updates explain
hybrid-wrappedonly as X-Wing, but the same PR also addsALGORITHM_HPQT_SECP256R1_MLKEM768andALGORITHM_HPQT_SECP384R1_MLKEM1024to the shared algorithm enums in the authorization/policy specs. As written, the KAS contract now documents one hybrid mode while leaving the other newly exposed hybrid algorithms undefined for clients mapping request/response behavior to supported algorithms.Also applies to: 285-285, 475-475
🤖 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 `@docs/openapi/kas/kas.openapi.yaml` at line 238, The OpenAPI enum text only documents 'hybrid-wrapped' as X-Wing but omits the newly exposed ML-KEM hybrid algorithms; update the enum/value documentation to include brief descriptions for ALGORITHM_HPQT_SECP256R1_MLKEM768 and ALGORITHM_HPQT_SECP384R1_MLKEM1024, state that these are ML-KEM hybrid-wrapped modes (alongside the existing X-Wing explanation), and clarify any behavioral differences or mapping to the 'hybrid-wrapped' value so clients can map request/response behavior to those algorithm enums.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
pflynn-virtru
approved these changes
Jun 10, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/openapi/policy/objects.openapi.yaml (3)
134-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
date-timeexamples that match the declared format.
1sand1.000340012sare duration literals, not RFC3339 timestamps. They contradict bothformat: date-timeand the schema description, and they can trip OpenAPI example validation/tooling.🤖 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 134 - 139, The examples for the schema symbol google.protobuf.Timestamp are currently duration literals ("1s", "1.000340012s") but the schema declares type: string and format: date-time; replace those examples with valid RFC3339/ISO8601 timestamp strings (e.g., "2023-05-01T12:34:56Z", "2023-05-01T12:34:56.000340012Z") so they match the declared format and pass OpenAPI/example validation.
230-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
policy.Actionbecame invalid under its own schema.
additionalProperties: falseonly recognizes siblingproperties, butcustomandstandardare declared only inside theoneOfbranches. That means any Action payload containing either variant field now fails validation. The same copied block appears in the service-specific policy specs, so this breaks the published contract broadly.🤖 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 230 - 266, policy.Action is invalid because additionalProperties: false is applied at the root while the discriminating fields (custom, standard) are only declared inside the oneOf branches; update the schema so the root properties include the variant keys or move additionalProperties inside each oneOf branch. Specifically, modify the policy.Action schema (the object with title "Action") to either add top-level properties for "custom" and "standard" (so additionalProperties: false will allow them) or remove/add additionalProperties: false inside each oneOf subschema and declare required/properties for custom and standard there (referencing the existing oneOf branches and the schema names policy.Action.StandardAction and policy.Namespace as needed) so payloads with the variant fields validate correctly.
448-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
algguard no longer excludes the UNSPECIFIED enum member.
policy.KasPublicKeyAlgEnumis modeled as strings, sonot.enum: [0]never matches and the restriction becomes a no-op. If this field is still meant to reject the unspecified value, the exclusion has to use the string enum member 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 448 - 457, The `alg` schema's not.enum currently uses the numeric literal 0 which is ineffective because policy.KasPublicKeyAlgEnum is a string enum; update the exclusion to use the enum's actual string member for the unspecified value (e.g., replace not.enum: [0] with not.enum: ["UNSPECIFIED"] or the exact string name used in policy.KasPublicKeyAlgEnum) so the `alg` guard rejects the unspecified enum; locate the `alg` entry and modify the not.enum value accordingly.docs/openapi/policy/attributes/attributes.openapi.yaml (1)
1943-1984:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new attribute lookup schemas are unsatisfiable.
GetAttributeRequestandGetAttributeValueRequestboth combineadditionalProperties: falsewith branch-only identifier fields, soattributeId/valueId/fqnare rejected at the outer object. The deprecatedidfallback also never satisfies the currentoneOf, so even{id: ...}is invalid. Move the identifier fields to top-levelpropertiesand model the allowed alternatives there.Also applies to: 1992-2036
🤖 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 `@docs/openapi/policy/attributes/attributes.openapi.yaml` around lines 1943 - 1984, GetAttributeRequest and GetAttributeValueRequest are unsatisfiable because identifier fields (attributeId/valueId/fqn) are only declared under oneOf branches while additionalProperties: false is set at the outer object, and the deprecated id doesn't satisfy any branch; fix by moving identifier fields to the top-level properties (declare attributeId, valueId, fqn and id alongside other top-level props), then use oneOf (or anyOf) to enforce alternatives by referencing those top-level property names via branch-required arrays (e.g., oneOf entries that only set required: ["attributeId"] or required: ["fqn"] and another for deprecated id), keep additionalProperties: false, and ensure titles/descriptions/format remain on the moved properties so the schema accepts {id:...} or {attributeId:...} or {fqn:...} as intended.docs/openapi/policy/obligations/obligations.openapi.yaml (1)
1629-1642:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSingle-resource obligation requests lost their identifier constraint.
These get/delete schemas now accept
{}and{id, fqn}because neither identifier is required and no one-of rule remains. That broadens the API contract beyond what a single-resource lookup/delete can satisfy. Keepid/fqnas top-level properties, but restore aoneOfon the required alternatives.Also applies to: 1651-1664, 1673-1686, 1715-1728
🤖 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 `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 1629 - 1642, The single-resource request schema policy.obligations.DeleteObligationRequest currently allows neither identifier and therefore accepts {} — restore the identifier constraint by adding a oneOf with two alternatives requiring either id or fqn (e.g., oneOf: - required: [id] - required: [fqn]) while keeping the existing id/fqn property definitions and additionalProperties: false; apply the same change to the other single-resource request schemas referenced in the diff (the corresponding schemas at the other ranges) so each requires either id or fqn as a top-level required alternative.docs/openapi/policy/actions/actions.openapi.yaml (1)
1148-1190:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetActionRequestcurrently rejects both lookup forms.With
additionalProperties: false, the outer object only permitsnamespaceIdandnamespaceFqn. Becauseidandnameexist only inside theoneOfbranches, both{id: ...}and{name: ...}fail validation. Declare the identifier fields at the top level and useoneOfonly to express the required choice.🤖 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 `@docs/openapi/policy/actions/actions.openapi.yaml` around lines 1148 - 1190, GetActionRequest currently has additionalProperties: false which prevents id or name from appearing because they're only defined inside the oneOf branches; move the identifier schemas for id (uuid format) and name (string with maxLength/regex/description) into the top-level properties of GetActionRequest and keep additionalProperties: false, then change the oneOf to two branches that only assert required: ["id"] and required: ["name"] respectively (so the choice is enforced but the actual property definitions live at top level); preserve namespaceId and namespaceFqn properties and their metadata and keep the same formats/constraints when you copy the id and name definitions to the top-level properties.
🤖 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.
Inline comments:
In `@docs/grpc/index.html`:
- Around line 15256-15264: The generated gRPC docs are missing descriptions for
the new namespace fields—update the documentation entries for namespace_fqn (and
the related namespace/context fields) to include a concise usage note: mark them
as "Optional" and describe their purpose and format (e.g., "Optional.
Fully-qualified namespace identifier used to scope the request; when omitted
defaults to the service/project namespace"), mirroring the style and detail of
nearby resource-mapping request field descriptions; ensure the same text is
added to both occurrences of the namespace_fqn/namespace context entries so the
docs are consistent.
In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml`:
- Around line 1298-1302: The CreateResourceMappingGroupRequest schema currently
allows neither namespaceId nor namespaceFqn which conflicts with
policy.ResourceMappingGroup; update the schema for
CreateResourceMappingGroupRequest to require exactly one namespace selector by
replacing the optional fields with a oneOf containing two object variants: one
that requires ["namespaceId"] and one that requires ["namespaceFqn"] (each
variant keeping the existing types/format validation for namespaceId and
namespaceFqn), and add a short description clarifying that exactly one selector
must be provided; alternatively, if server-side defaulting is intended, document
the default/derivation in the schema description and ensure the server
implements it.
---
Outside diff comments:
In `@docs/openapi/policy/actions/actions.openapi.yaml`:
- Around line 1148-1190: GetActionRequest currently has additionalProperties:
false which prevents id or name from appearing because they're only defined
inside the oneOf branches; move the identifier schemas for id (uuid format) and
name (string with maxLength/regex/description) into the top-level properties of
GetActionRequest and keep additionalProperties: false, then change the oneOf to
two branches that only assert required: ["id"] and required: ["name"]
respectively (so the choice is enforced but the actual property definitions live
at top level); preserve namespaceId and namespaceFqn properties and their
metadata and keep the same formats/constraints when you copy the id and name
definitions to the top-level properties.
In `@docs/openapi/policy/attributes/attributes.openapi.yaml`:
- Around line 1943-1984: GetAttributeRequest and GetAttributeValueRequest are
unsatisfiable because identifier fields (attributeId/valueId/fqn) are only
declared under oneOf branches while additionalProperties: false is set at the
outer object, and the deprecated id doesn't satisfy any branch; fix by moving
identifier fields to the top-level properties (declare attributeId, valueId, fqn
and id alongside other top-level props), then use oneOf (or anyOf) to enforce
alternatives by referencing those top-level property names via branch-required
arrays (e.g., oneOf entries that only set required: ["attributeId"] or required:
["fqn"] and another for deprecated id), keep additionalProperties: false, and
ensure titles/descriptions/format remain on the moved properties so the schema
accepts {id:...} or {attributeId:...} or {fqn:...} as intended.
In `@docs/openapi/policy/objects.openapi.yaml`:
- Around line 134-139: The examples for the schema symbol
google.protobuf.Timestamp are currently duration literals ("1s", "1.000340012s")
but the schema declares type: string and format: date-time; replace those
examples with valid RFC3339/ISO8601 timestamp strings (e.g.,
"2023-05-01T12:34:56Z", "2023-05-01T12:34:56.000340012Z") so they match the
declared format and pass OpenAPI/example validation.
- Around line 230-266: policy.Action is invalid because additionalProperties:
false is applied at the root while the discriminating fields (custom, standard)
are only declared inside the oneOf branches; update the schema so the root
properties include the variant keys or move additionalProperties inside each
oneOf branch. Specifically, modify the policy.Action schema (the object with
title "Action") to either add top-level properties for "custom" and "standard"
(so additionalProperties: false will allow them) or remove/add
additionalProperties: false inside each oneOf subschema and declare
required/properties for custom and standard there (referencing the existing
oneOf branches and the schema names policy.Action.StandardAction and
policy.Namespace as needed) so payloads with the variant fields validate
correctly.
- Around line 448-457: The `alg` schema's not.enum currently uses the numeric
literal 0 which is ineffective because policy.KasPublicKeyAlgEnum is a string
enum; update the exclusion to use the enum's actual string member for the
unspecified value (e.g., replace not.enum: [0] with not.enum: ["UNSPECIFIED"] or
the exact string name used in policy.KasPublicKeyAlgEnum) so the `alg` guard
rejects the unspecified enum; locate the `alg` entry and modify the not.enum
value accordingly.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 1629-1642: The single-resource request schema
policy.obligations.DeleteObligationRequest currently allows neither identifier
and therefore accepts {} — restore the identifier constraint by adding a oneOf
with two alternatives requiring either id or fqn (e.g., oneOf: - required: [id]
- required: [fqn]) while keeping the existing id/fqn property definitions and
additionalProperties: false; apply the same change to the other single-resource
request schemas referenced in the diff (the corresponding schemas at the other
ranges) so each requires either id or fqn as a top-level required alternative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13a33a63-ffe9-44ba-b91e-638648c8d807
📒 Files selected for processing (9)
docs/grpc/index.htmldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/openapi/policy/objects.openapi.yaml (3)
134-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
date-timeexamples that match the declared format.
1sand1.000340012sare duration literals, not RFC3339 timestamps. They contradict bothformat: date-timeand the schema description, and they can trip OpenAPI example validation/tooling.🤖 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 134 - 139, The examples for the schema symbol google.protobuf.Timestamp are currently duration literals ("1s", "1.000340012s") but the schema declares type: string and format: date-time; replace those examples with valid RFC3339/ISO8601 timestamp strings (e.g., "2023-05-01T12:34:56Z", "2023-05-01T12:34:56.000340012Z") so they match the declared format and pass OpenAPI/example validation.
230-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
policy.Actionbecame invalid under its own schema.
additionalProperties: falseonly recognizes siblingproperties, butcustomandstandardare declared only inside theoneOfbranches. That means any Action payload containing either variant field now fails validation. The same copied block appears in the service-specific policy specs, so this breaks the published contract broadly.🤖 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 230 - 266, policy.Action is invalid because additionalProperties: false is applied at the root while the discriminating fields (custom, standard) are only declared inside the oneOf branches; update the schema so the root properties include the variant keys or move additionalProperties inside each oneOf branch. Specifically, modify the policy.Action schema (the object with title "Action") to either add top-level properties for "custom" and "standard" (so additionalProperties: false will allow them) or remove/add additionalProperties: false inside each oneOf subschema and declare required/properties for custom and standard there (referencing the existing oneOf branches and the schema names policy.Action.StandardAction and policy.Namespace as needed) so payloads with the variant fields validate correctly.
448-457:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
algguard no longer excludes the UNSPECIFIED enum member.
policy.KasPublicKeyAlgEnumis modeled as strings, sonot.enum: [0]never matches and the restriction becomes a no-op. If this field is still meant to reject the unspecified value, the exclusion has to use the string enum member 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 `@docs/openapi/policy/objects.openapi.yaml` around lines 448 - 457, The `alg` schema's not.enum currently uses the numeric literal 0 which is ineffective because policy.KasPublicKeyAlgEnum is a string enum; update the exclusion to use the enum's actual string member for the unspecified value (e.g., replace not.enum: [0] with not.enum: ["UNSPECIFIED"] or the exact string name used in policy.KasPublicKeyAlgEnum) so the `alg` guard rejects the unspecified enum; locate the `alg` entry and modify the not.enum value accordingly.docs/openapi/policy/attributes/attributes.openapi.yaml (1)
1943-1984:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new attribute lookup schemas are unsatisfiable.
GetAttributeRequestandGetAttributeValueRequestboth combineadditionalProperties: falsewith branch-only identifier fields, soattributeId/valueId/fqnare rejected at the outer object. The deprecatedidfallback also never satisfies the currentoneOf, so even{id: ...}is invalid. Move the identifier fields to top-levelpropertiesand model the allowed alternatives there.Also applies to: 1992-2036
🤖 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 `@docs/openapi/policy/attributes/attributes.openapi.yaml` around lines 1943 - 1984, GetAttributeRequest and GetAttributeValueRequest are unsatisfiable because identifier fields (attributeId/valueId/fqn) are only declared under oneOf branches while additionalProperties: false is set at the outer object, and the deprecated id doesn't satisfy any branch; fix by moving identifier fields to the top-level properties (declare attributeId, valueId, fqn and id alongside other top-level props), then use oneOf (or anyOf) to enforce alternatives by referencing those top-level property names via branch-required arrays (e.g., oneOf entries that only set required: ["attributeId"] or required: ["fqn"] and another for deprecated id), keep additionalProperties: false, and ensure titles/descriptions/format remain on the moved properties so the schema accepts {id:...} or {attributeId:...} or {fqn:...} as intended.docs/openapi/policy/obligations/obligations.openapi.yaml (1)
1629-1642:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSingle-resource obligation requests lost their identifier constraint.
These get/delete schemas now accept
{}and{id, fqn}because neither identifier is required and no one-of rule remains. That broadens the API contract beyond what a single-resource lookup/delete can satisfy. Keepid/fqnas top-level properties, but restore aoneOfon the required alternatives.Also applies to: 1651-1664, 1673-1686, 1715-1728
🤖 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 `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 1629 - 1642, The single-resource request schema policy.obligations.DeleteObligationRequest currently allows neither identifier and therefore accepts {} — restore the identifier constraint by adding a oneOf with two alternatives requiring either id or fqn (e.g., oneOf: - required: [id] - required: [fqn]) while keeping the existing id/fqn property definitions and additionalProperties: false; apply the same change to the other single-resource request schemas referenced in the diff (the corresponding schemas at the other ranges) so each requires either id or fqn as a top-level required alternative.docs/openapi/policy/actions/actions.openapi.yaml (1)
1148-1190:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetActionRequestcurrently rejects both lookup forms.With
additionalProperties: false, the outer object only permitsnamespaceIdandnamespaceFqn. Becauseidandnameexist only inside theoneOfbranches, both{id: ...}and{name: ...}fail validation. Declare the identifier fields at the top level and useoneOfonly to express the required choice.🤖 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 `@docs/openapi/policy/actions/actions.openapi.yaml` around lines 1148 - 1190, GetActionRequest currently has additionalProperties: false which prevents id or name from appearing because they're only defined inside the oneOf branches; move the identifier schemas for id (uuid format) and name (string with maxLength/regex/description) into the top-level properties of GetActionRequest and keep additionalProperties: false, then change the oneOf to two branches that only assert required: ["id"] and required: ["name"] respectively (so the choice is enforced but the actual property definitions live at top level); preserve namespaceId and namespaceFqn properties and their metadata and keep the same formats/constraints when you copy the id and name definitions to the top-level properties.
🤖 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.
Inline comments:
In `@docs/grpc/index.html`:
- Around line 15256-15264: The generated gRPC docs are missing descriptions for
the new namespace fields—update the documentation entries for namespace_fqn (and
the related namespace/context fields) to include a concise usage note: mark them
as "Optional" and describe their purpose and format (e.g., "Optional.
Fully-qualified namespace identifier used to scope the request; when omitted
defaults to the service/project namespace"), mirroring the style and detail of
nearby resource-mapping request field descriptions; ensure the same text is
added to both occurrences of the namespace_fqn/namespace context entries so the
docs are consistent.
In `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml`:
- Around line 1298-1302: The CreateResourceMappingGroupRequest schema currently
allows neither namespaceId nor namespaceFqn which conflicts with
policy.ResourceMappingGroup; update the schema for
CreateResourceMappingGroupRequest to require exactly one namespace selector by
replacing the optional fields with a oneOf containing two object variants: one
that requires ["namespaceId"] and one that requires ["namespaceFqn"] (each
variant keeping the existing types/format validation for namespaceId and
namespaceFqn), and add a short description clarifying that exactly one selector
must be provided; alternatively, if server-side defaulting is intended, document
the default/derivation in the schema description and ensure the server
implements it.
---
Outside diff comments:
In `@docs/openapi/policy/actions/actions.openapi.yaml`:
- Around line 1148-1190: GetActionRequest currently has additionalProperties:
false which prevents id or name from appearing because they're only defined
inside the oneOf branches; move the identifier schemas for id (uuid format) and
name (string with maxLength/regex/description) into the top-level properties of
GetActionRequest and keep additionalProperties: false, then change the oneOf to
two branches that only assert required: ["id"] and required: ["name"]
respectively (so the choice is enforced but the actual property definitions live
at top level); preserve namespaceId and namespaceFqn properties and their
metadata and keep the same formats/constraints when you copy the id and name
definitions to the top-level properties.
In `@docs/openapi/policy/attributes/attributes.openapi.yaml`:
- Around line 1943-1984: GetAttributeRequest and GetAttributeValueRequest are
unsatisfiable because identifier fields (attributeId/valueId/fqn) are only
declared under oneOf branches while additionalProperties: false is set at the
outer object, and the deprecated id doesn't satisfy any branch; fix by moving
identifier fields to the top-level properties (declare attributeId, valueId, fqn
and id alongside other top-level props), then use oneOf (or anyOf) to enforce
alternatives by referencing those top-level property names via branch-required
arrays (e.g., oneOf entries that only set required: ["attributeId"] or required:
["fqn"] and another for deprecated id), keep additionalProperties: false, and
ensure titles/descriptions/format remain on the moved properties so the schema
accepts {id:...} or {attributeId:...} or {fqn:...} as intended.
In `@docs/openapi/policy/objects.openapi.yaml`:
- Around line 134-139: The examples for the schema symbol
google.protobuf.Timestamp are currently duration literals ("1s", "1.000340012s")
but the schema declares type: string and format: date-time; replace those
examples with valid RFC3339/ISO8601 timestamp strings (e.g.,
"2023-05-01T12:34:56Z", "2023-05-01T12:34:56.000340012Z") so they match the
declared format and pass OpenAPI/example validation.
- Around line 230-266: policy.Action is invalid because additionalProperties:
false is applied at the root while the discriminating fields (custom, standard)
are only declared inside the oneOf branches; update the schema so the root
properties include the variant keys or move additionalProperties inside each
oneOf branch. Specifically, modify the policy.Action schema (the object with
title "Action") to either add top-level properties for "custom" and "standard"
(so additionalProperties: false will allow them) or remove/add
additionalProperties: false inside each oneOf subschema and declare
required/properties for custom and standard there (referencing the existing
oneOf branches and the schema names policy.Action.StandardAction and
policy.Namespace as needed) so payloads with the variant fields validate
correctly.
- Around line 448-457: The `alg` schema's not.enum currently uses the numeric
literal 0 which is ineffective because policy.KasPublicKeyAlgEnum is a string
enum; update the exclusion to use the enum's actual string member for the
unspecified value (e.g., replace not.enum: [0] with not.enum: ["UNSPECIFIED"] or
the exact string name used in policy.KasPublicKeyAlgEnum) so the `alg` guard
rejects the unspecified enum; locate the `alg` entry and modify the not.enum
value accordingly.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 1629-1642: The single-resource request schema
policy.obligations.DeleteObligationRequest currently allows neither identifier
and therefore accepts {} — restore the identifier constraint by adding a oneOf
with two alternatives requiring either id or fqn (e.g., oneOf: - required: [id]
- required: [fqn]) while keeping the existing id/fqn property definitions and
additionalProperties: false; apply the same change to the other single-resource
request schemas referenced in the diff (the corresponding schemas at the other
ranges) so each requires either id or fqn as a top-level required alternative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13a33a63-ffe9-44ba-b91e-638648c8d807
📒 Files selected for processing (9)
docs/grpc/index.htmldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yaml
🛑 Comments failed to post (2)
docs/grpc/index.html (1)
15256-15264:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFill in blank descriptions for newly documented namespace fields.
Line 15256 and Line 15875 currently render empty field descriptions (
<p> </p>), which makes the newnamespace_fqn/namespace context behavior unclear in generated gRPC docs. Add explicit “Optional”/usage text, matching the detail level used in nearby resource-mapping request fields.Also applies to: 15871-15876
🤖 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 `@docs/grpc/index.html` around lines 15256 - 15264, The generated gRPC docs are missing descriptions for the new namespace fields—update the documentation entries for namespace_fqn (and the related namespace/context fields) to include a concise usage note: mark them as "Optional" and describe their purpose and format (e.g., "Optional. Fully-qualified namespace identifier used to scope the request; when omitted defaults to the service/project namespace"), mirroring the style and detail of nearby resource-mapping request field descriptions; ensure the same text is added to both occurrences of the namespace_fqn/namespace context entries so the docs are consistent.docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml (1)
1298-1302:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire one namespace selector for group creation.
CreateResourceMappingGroupRequestnow exposes bothnamespaceIdandnamespaceFqn, but the schema still allows neither even thoughpolicy.ResourceMappingGrouprequires a namespace. That makes the public contract accept create requests that cannot deterministically produce a valid group. Require exactly one selector, or document the server-side default/derivation.🩹 Suggested schema fix
policy.resourcemapping.CreateResourceMappingGroupRequest: type: object + oneOf: + - required: + - namespaceId + - required: + - namespaceFqn properties: namespaceId: type: string title: namespace_id format: uuid namespaceFqn: type: string title: namespace_fqn minLength: 1 format: uri🤖 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 `@docs/openapi/policy/resourcemapping/resource_mapping.openapi.yaml` around lines 1298 - 1302, The CreateResourceMappingGroupRequest schema currently allows neither namespaceId nor namespaceFqn which conflicts with policy.ResourceMappingGroup; update the schema for CreateResourceMappingGroupRequest to require exactly one namespace selector by replacing the optional fields with a oneOf containing two object variants: one that requires ["namespaceId"] and one that requires ["namespaceFqn"] (each variant keeping the existing types/format validation for namespaceId and namespaceFqn), and add a short description clarifying that exactly one selector must be provided; alternatively, if server-side defaulting is intended, document the default/derivation in the schema description and ensure the server implements it.
This was referenced Jun 10, 2026
Contributor
Author
|
/backport |
Contributor
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-3508-to-release/service/v0.11
git worktree add --checkout .worktree/backport-3508-to-release/service/v0.11 backport-3508-to-release/service/v0.11
cd .worktree/backport-3508-to-release/service/v0.11
git reset --hard HEAD^
git cherry-pick -x 9d16f8062e6164748a6a27c497272209b743339f
git push --force-with-lease |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make Authorization v2 request limits configurable by moving max-count validation from proto CEL to service-side config.
Summary by CodeRabbit
New Features
Documentation
Tests