Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAPI key validation now returns a resolved Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIKeyPolicy
participant KeyStore
participant PolicyContext
rect rgba(200,150,100,0.5)
Note over Client,PolicyContext: Old flow — Boolean validation
Client->>APIKeyPolicy: Request (with API key)
APIKeyPolicy->>KeyStore: validateAPIKey(apiId,...,apiKey,issuer)
KeyStore-->>APIKeyPolicy: bool (true/false)
alt valid
APIKeyPolicy->>PolicyContext: handleAuthSuccess(ctx)
PolicyContext->>PolicyContext: Set minimal AuthContext
else invalid
APIKeyPolicy->>PolicyContext: return auth error
end
end
rect rgba(100,150,200,0.5)
Note over Client,PolicyContext: New flow — Resolved key object
Client->>APIKeyPolicy: Request (with API key)
APIKeyPolicy->>KeyStore: resolveValidatedAPIKey(apiId,...,apiKey,issuer)
KeyStore-->>APIKeyPolicy: *store.APIKey (or nil)
alt resolved key present
APIKeyPolicy->>PolicyContext: handleAuthSuccess(ctx, resolvedKey)
PolicyContext->>PolicyContext: Set CredentialID = resolvedKey.ID
PolicyContext->>PolicyContext: Set Properties (ApplicationName, ApplicationID)
PolicyContext->>PolicyContext: Optionally add AnalyticsMetadata to upstream
else no key
APIKeyPolicy->>PolicyContext: return auth error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
policies/aws-bedrock-guardrail/awsbedrockguardrail.go (1)
863-866: Consider using kebab-case forguardrailNameconsistency.The
guardrailNamevalue"AWS Bedrock Guardrail"uses Title Case with spaces, while most other guardrails use kebab-case (e.g.,"regex-guardrail","url-guardrail"). This inconsistency may complicate analytics filtering and aggregation.If preserving AWS branding is intentional, consider documenting the naming conventions for analytics consumers.
♻️ Alternative for consistency
analyticsMetadata := map[string]interface{}{ "isGuardrailHit": true, - "guardrailName": "AWS Bedrock Guardrail", + "guardrailName": "aws-bedrock-guardrail", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/aws-bedrock-guardrail/awsbedrockguardrail.go` around lines 863 - 866, The analyticsMetadata map sets "guardrailName" to "AWS Bedrock Guardrail" which is inconsistent with kebab-case used elsewhere; update the value associated with the "guardrailName" key in analyticsMetadata (in awsbedrockguardrail.go) to use kebab-case (e.g., "aws-bedrock-guardrail") for consistent analytics filtering, or if AWS branding must be preserved add a short documented mapping of this guardrail name to the kebab-case identifier for analytics consumers.policies/semantic-prompt-guard/semanticpromptguard.go (1)
420-423: Consider using kebab-case forguardrailNameconsistency.Other guardrail policies use kebab-case naming (e.g.,
"regex-guardrail","word-count-guardrail","sentence-count-guardrail"), but this uses PascalCase"SemanticPromptGuard". This inconsistency may complicate analytics queries filtering byguardrailName.♻️ Suggested change for consistency
analyticsMetadata := map[string]interface{}{ "isGuardrailHit": true, - "guardrailName": "SemanticPromptGuard", + "guardrailName": "semantic-prompt-guard", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/semantic-prompt-guard/semanticpromptguard.go` around lines 420 - 423, The analyticsMetadata map sets "guardrailName" to "SemanticPromptGuard" which is inconsistent with other policies; update the value in the analyticsMetadata declaration to use kebab-case ("semantic-prompt-guard") so it matches names like "regex-guardrail" and "word-count-guardrail" and keeps analytics filtering consistent (modify the map where analyticsMetadata is constructed and replace "SemanticPromptGuard" with "semantic-prompt-guard").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@policies/aws-bedrock-guardrail/awsbedrockguardrail.go`:
- Around line 863-866: The analyticsMetadata map sets "guardrailName" to "AWS
Bedrock Guardrail" which is inconsistent with kebab-case used elsewhere; update
the value associated with the "guardrailName" key in analyticsMetadata (in
awsbedrockguardrail.go) to use kebab-case (e.g., "aws-bedrock-guardrail") for
consistent analytics filtering, or if AWS branding must be preserved add a short
documented mapping of this guardrail name to the kebab-case identifier for
analytics consumers.
In `@policies/semantic-prompt-guard/semanticpromptguard.go`:
- Around line 420-423: The analyticsMetadata map sets "guardrailName" to
"SemanticPromptGuard" which is inconsistent with other policies; update the
value in the analyticsMetadata declaration to use kebab-case
("semantic-prompt-guard") so it matches names like "regex-guardrail" and
"word-count-guardrail" and keeps analytics filtering consistent (modify the map
where analyticsMetadata is constructed and replace "SemanticPromptGuard" with
"semantic-prompt-guard").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 033f9325-062b-4fef-aa79-3161c4deb641
📒 Files selected for processing (11)
policies/api-key-auth/apikey.gopolicies/aws-bedrock-guardrail/awsbedrockguardrail.gopolicies/azure-content-safety-content-moderation/azurecontentsafetycontentmoderation.gopolicies/content-length-guardrail/contentlengthguardrail.gopolicies/json-schema-guardrail/jsonschemaguardrail.gopolicies/llm-cost/llm_cost.gopolicies/regex-guardrail/regexguardrail.gopolicies/semantic-prompt-guard/semanticpromptguard.gopolicies/sentence-count-guardrail/sentencecountguardrail.gopolicies/url-guardrail/urlguardrail.gopolicies/word-count-guardrail/wordcountguardrail.go
Arshardh
force-pushed
the
analytics-events
branch
from
649a6da to
7e3b6bd
Compare
Arshardh
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
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 (1)
policies/api-key-auth/apikey.go (1)
195-229:⚠️ Potential issue | 🟠 MajorShare this resolved-key success path with
OnRequestHeaders().
handleAuthSuccess()now enriches successful auth with the resolved application data, but the v1alpha2authenticate()flow still succeeds from a boolean-only validation result and builds a bare success context. Requests entering throughGetPolicyV2()will therefore miss the application analytics this PR is adding.Please route the header-phase path through
resolveValidatedAPIKey(...)as well, and reuse a common success helper so both entry points stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/api-key-auth/apikey.go` around lines 195 - 229, The header-phase success path currently bypasses the new enriched success logic; modify OnRequestHeaders (and any v1alpha2 authenticate flow used by GetPolicyV2) to call resolveValidatedAPIKey(...) on a validated key and then invoke the common helper handleAuthSuccess(ctx, resolvedKey) instead of returning a bare boolean success; update authenticate() to reuse resolveValidatedAPIKey and/or call handleAuthSuccess so both request headers and v1alpha2 flows populate ctx.SharedContext.AuthContext and return the same UpstreamRequestModifications/AnalyticsMetadata as handleAuthSuccess.
🧹 Nitpick comments (3)
policies/word-count-guardrail/wordcountguardrail.go (1)
427-430: Avoid duplicated guardrail name literals
"word-count-guardrail"is now duplicated across analytics metadata and assessment construction. Consider a singleconstto prevent drift.Proposed small refactor
const ( GuardrailErrorCode = 422 + GuardrailName = "word-count-guardrail" TextCleanRegex = "^\"|\"$" @@ analyticsMetadata := map[string]interface{}{ "isGuardrailHit": true, - "guardrailName": "word-count-guardrail", + "guardrailName": GuardrailName, } @@ assessment := map[string]interface{}{ "action": "GUARDRAIL_INTERVENED", - "interveningGuardrail": "word-count-guardrail", + "interveningGuardrail": GuardrailName, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/word-count-guardrail/wordcountguardrail.go` around lines 427 - 430, Replace the duplicated literal "word-count-guardrail" with a single const (e.g., const guardrailName = "word-count-guardrail") and use that const wherever the name is referenced (for example in the analyticsMetadata map assignment and in the assessment construction code that currently repeats the string); update symbols analyticsMetadata and the assessment builder/constructor to reference guardrailName so the value is maintained in one place.policies/aws-bedrock-guardrail/awsbedrockguardrail.go (1)
882-885: StandardizeguardrailNameformat for analytics aggregationAt Line 884,
"AWS Bedrock Guardrail"uses title case while other guardrails use identifier-style names. This can split dashboards/queries by inconsistent label values.Suggested diff
analyticsMetadata := map[string]interface{}{ "isGuardrailHit": true, - "guardrailName": "AWS Bedrock Guardrail", + "guardrailName": "aws-bedrock-guardrail", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/aws-bedrock-guardrail/awsbedrockguardrail.go` around lines 882 - 885, The analytics metadata uses a title-cased guardrailName ("AWS Bedrock Guardrail") which is inconsistent with other guardrails; update the analyticsMetadata map's "guardrailName" value in the analyticsMetadata variable to the standardized identifier-style string used across the repo (e.g., "aws_bedrock_guardrail" or "aws-bedrock-guardrail") so dashboards/queries aggregate correctly; locate the analyticsMetadata map declaration and replace the guardrailName value while preserving the surrounding keys ("isGuardrailHit" etc.).policies/azure-content-safety-content-moderation/azurecontentsafetycontentmoderation.go (1)
657-660: NormalizeguardrailNameto match cross-policy conventionAt Line 659,
guardrailNameis CamelCase. Aligning this to the same identifier format used by other guardrails helps keep analytics filtering/grouping consistent.Suggested diff
analyticsMetadata := map[string]interface{}{ "isGuardrailHit": true, - "guardrailName": "AzureContentSafetyContentModeration", + "guardrailName": "azure-content-safety-content-moderation", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@policies/azure-content-safety-content-moderation/azurecontentsafetycontentmoderation.go` around lines 657 - 660, The analyticsMetadata map sets "guardrailName" to a CamelCase value ("AzureContentSafetyContentModeration"); change that value to the project’s cross-policy normalized identifier (e.g., "azure_content_safety_content_moderation") so analytics/filtering is consistent — update the analyticsMetadata map where "guardrailName" is assigned within azurecontentsafetycontentmoderation.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@policies/json-schema-guardrail/jsonschemaguardrail.go`:
- Around line 298-301: The v1alpha2 error builder buildErrorResponseV2 is
missing AnalyticsMetadata, causing lost telemetry; create the same
analyticsMetadata map used in buildErrorResponse (map with "isGuardrailHit":
true and "guardrailName": "json-schema-guardrail") and include it in the
returned policyv1alpha2.DownstreamResponseModifications by populating its
AnalyticsMetadata field; apply the same change pattern to other v1alpha2 error
builders (word-count, regex, url, sentence-count, content-length, etc.) so all
v1alpha2 guardrail error responses carry the analytics payload.
---
Outside diff comments:
In `@policies/api-key-auth/apikey.go`:
- Around line 195-229: The header-phase success path currently bypasses the new
enriched success logic; modify OnRequestHeaders (and any v1alpha2 authenticate
flow used by GetPolicyV2) to call resolveValidatedAPIKey(...) on a validated key
and then invoke the common helper handleAuthSuccess(ctx, resolvedKey) instead of
returning a bare boolean success; update authenticate() to reuse
resolveValidatedAPIKey and/or call handleAuthSuccess so both request headers and
v1alpha2 flows populate ctx.SharedContext.AuthContext and return the same
UpstreamRequestModifications/AnalyticsMetadata as handleAuthSuccess.
---
Nitpick comments:
In `@policies/aws-bedrock-guardrail/awsbedrockguardrail.go`:
- Around line 882-885: The analytics metadata uses a title-cased guardrailName
("AWS Bedrock Guardrail") which is inconsistent with other guardrails; update
the analyticsMetadata map's "guardrailName" value in the analyticsMetadata
variable to the standardized identifier-style string used across the repo (e.g.,
"aws_bedrock_guardrail" or "aws-bedrock-guardrail") so dashboards/queries
aggregate correctly; locate the analyticsMetadata map declaration and replace
the guardrailName value while preserving the surrounding keys ("isGuardrailHit"
etc.).
In
`@policies/azure-content-safety-content-moderation/azurecontentsafetycontentmoderation.go`:
- Around line 657-660: The analyticsMetadata map sets "guardrailName" to a
CamelCase value ("AzureContentSafetyContentModeration"); change that value to
the project’s cross-policy normalized identifier (e.g.,
"azure_content_safety_content_moderation") so analytics/filtering is consistent
— update the analyticsMetadata map where "guardrailName" is assigned within
azurecontentsafetycontentmoderation.go.
In `@policies/word-count-guardrail/wordcountguardrail.go`:
- Around line 427-430: Replace the duplicated literal "word-count-guardrail"
with a single const (e.g., const guardrailName = "word-count-guardrail") and use
that const wherever the name is referenced (for example in the analyticsMetadata
map assignment and in the assessment construction code that currently repeats
the string); update symbols analyticsMetadata and the assessment
builder/constructor to reference guardrailName so the value is maintained in one
place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a799c58d-77fc-4fd4-91f1-b66cc55b6523
📒 Files selected for processing (12)
policies/api-key-auth/apikey.gopolicies/api-key-auth/go.modpolicies/aws-bedrock-guardrail/awsbedrockguardrail.gopolicies/azure-content-safety-content-moderation/azurecontentsafetycontentmoderation.gopolicies/content-length-guardrail/contentlengthguardrail.gopolicies/json-schema-guardrail/jsonschemaguardrail.gopolicies/llm-cost/llm_cost.gopolicies/regex-guardrail/regexguardrail.gopolicies/semantic-prompt-guard/semanticpromptguard.gopolicies/sentence-count-guardrail/sentencecountguardrail.gopolicies/url-guardrail/urlguardrail.gopolicies/word-count-guardrail/wordcountguardrail.go
✅ Files skipped from review due to trivial changes (1)
- policies/api-key-auth/go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- policies/llm-cost/llm_cost.go
- policies/url-guardrail/urlguardrail.go
- policies/content-length-guardrail/contentlengthguardrail.go
- policies/sentence-count-guardrail/sentencecountguardrail.go
Thushani-Jayasekera
approved these changes
Mar 26, 2026
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
2 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.
Summary by CodeRabbit
New Features
Improvements
Chores