BCH-1290: Include certificate domain name in evidence title#5
Merged
Conversation
Suffixes each evidence title with the certificate domain name so users can identify which ACM certificate a piece of evidence relates to. Example: "ACM certificate must carry all required tags [api.example.com]" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCompliancePlugin.Eval builds a request-scoped merged policyData (l.policyData plus bundle root data for the first policy path). PolicyEvaluator.Eval consumes the provided policyData (no per-policy bundle load) and appends the certificate DomainName to each evidence Title. The bundle loader was exported and now applies overrides last or returns overrides when no bundle data exists. ChangesPolicy evaluation and bundle data loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/eval.go`:
- Line 137: The code reads the protobuf field directly via ev.Title; change it
to use the proto getter to avoid nil panics and follow protobuf conventions:
replace reading ev.Title with ev.GetTitle() when building the new title (e.g.,
in the expression used with fmt.Sprintf), so use ev.GetTitle() and keep the rest
of the logic that appends cert.DomainName 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2816d0b3-f571-4811-be9d-b1bf4e8364eb
📒 Files selected for processing (1)
internal/eval.go
loadBundleRootData (now exported as LoadBundleRootData) was called inside PolicyEvaluator.Eval which is invoked once per certificate, causing N_certs x N_policies redundant disk reads per evaluation cycle. Move the load+merge step to main.go's Eval, before the cert loop, so data.json is read once per policy path per evaluation. Eval now takes pre-merged data keyed by policy path instead of raw operator overrides. Add eval_test.go with three tests: - operator overrides win over bundle defaults (config primacy) - no data.json returns overrides unchanged - data.json one directory up from policyPath is found correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use ev.GetTitle() instead of ev.Title when reading the proto field to follow protobuf conventions and handle nil cases correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All policy paths share one bundle so per-path data maps were unnecessary. Load bundle data.json once using the first path, pass the single merged map through to Eval and directly into NewPolicyProcessor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
internal/eval.go (1)
163-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
policyPath/data.jsonbefore the parent directory.The current candidate order loads the parent
data.jsonfirst, so a local source tree can pick up unrelated defaults whenever both locations exist. That inverts the behavior described in the comment here. Swap the order so the file insidepolicyPathwins, then fall back to the parent only for OCI bundles.Suggested fix
candidates := []string{ - filepath.Join(filepath.Dir(policyPath), "data.json"), filepath.Join(policyPath, "data.json"), + filepath.Join(filepath.Dir(policyPath), "data.json"), }🤖 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 `@internal/eval.go` around lines 163 - 166, The candidate list currently checks the parent directory before the local policy directory; update the candidates construction so filepath.Join(policyPath, "data.json") comes first and the parent (filepath.Join(filepath.Dir(policyPath), "data.json")) is only appended as a fallback when handling OCI bundles (check the bundle/OCI flag used by this code path). Modify the creation of the candidates slice in internal/eval.go (the candidates variable that uses policyPath) to prefer the local data.json and conditionally include the parent entry only for OCI scenarios.
🤖 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 `@internal/eval_test.go`:
- Around line 53-73: Add a regression test to cover the precedence when both
policyPath/data.json and its parent directory's data.json exist: create both
files (one at filepath.Join(policiesDir, "data.json") and one at
filepath.Join(root, "data.json")) inside
TestLoadBundleRootData_FindsDataJsonOneDirectoryUp (or a new test named e.g.
TestLoadBundleRootData_LocalOverridesParent) and call
LoadBundleRootData(policiesDir, nil); assert that the returned map uses the
value from the policyPath/data.json (the local file) rather than the parent file
by checking the specific key (expiry_warning_days) matches the local file's
value. Ensure file creation, permissions, and cleanup mirror existing test
patterns and keep error checks identical to the current test.
---
Outside diff comments:
In `@internal/eval.go`:
- Around line 163-166: The candidate list currently checks the parent directory
before the local policy directory; update the candidates construction so
filepath.Join(policyPath, "data.json") comes first and the parent
(filepath.Join(filepath.Dir(policyPath), "data.json")) is only appended as a
fallback when handling OCI bundles (check the bundle/OCI flag used by this code
path). Modify the creation of the candidates slice in internal/eval.go (the
candidates variable that uses policyPath) to prefer the local data.json and
conditionally include the parent entry only for OCI scenarios.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: bf12d4be-e286-4668-bfaf-f40dce4c9973
📒 Files selected for processing (3)
internal/eval.gointernal/eval_test.gomain.go
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)
internal/eval.go (1)
162-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer the local bundle
data.jsonbefore the parent directory.Parent-first lookup makes
policyPath/data.jsonunreachable whenever both locations exist. In a local source tree, that can load an unrelated parentdata.jsonand evaluate policies with the wrong defaults.Suggested fix
candidates := []string{ - filepath.Join(filepath.Dir(policyPath), "data.json"), filepath.Join(policyPath, "data.json"), + filepath.Join(filepath.Dir(policyPath), "data.json"), }🤖 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 `@internal/eval.go` around lines 162 - 165, The candidate paths for data.json are currently parent-first which can hide a local bundle; in internal/eval.go update the candidates slice (the variable named candidates that uses policyPath) to list filepath.Join(policyPath, "data.json") before filepath.Join(filepath.Dir(policyPath), "data.json") so the local policyPath/data.json is preferred over the parent directory file.
🤖 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 `@internal/eval.go`:
- Around line 162-165: The candidate paths for data.json are currently
parent-first which can hide a local bundle; in internal/eval.go update the
candidates slice (the variable named candidates that uses policyPath) to list
filepath.Join(policyPath, "data.json") before
filepath.Join(filepath.Dir(policyPath), "data.json") so the local
policyPath/data.json is preferred over the parent directory file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8eec3b61-a8b0-4010-bff1-4365e3494398
📒 Files selected for processing (2)
internal/eval.gomain.go
…recedence test LoadBundleRootData was checking the parent directory first, meaning a local source tree (where data.json lives inside policies/) would silently fall through to an unrelated parent data.json when both exist. Swap the order so policyPath/data.json wins, with parent as the OCI-bundle fallback. Add TestLoadBundleRootData_PolicyPathDataJsonWinsOverParent to lock this precedence: when both locations exist, the file inside policyPath is used. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merged
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
1 participant
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.
Suffixes each evidence title with the certificate domain name so users can identify which ACM certificate a piece of evidence relates to.
Example: "ACM certificate must carry all required tags [api.example.com]"
Summary by CodeRabbit
Bug Fixes
New Features
Tests