BCH-1296: Address PR review comments on plugin-aws-ecr (004)#3
Merged
Conversation
- Risk templates: each policy now defines a risk_templates Rego rule so the policy manager can upsert risks from policy metadata via InitWithSubjectsAndRisksFromPolicies - Release workflow: add .github/workflows/release.yml triggered on v* tags; runs GoReleaser and pushes the OCI binary image via gooci - Policy scoping: Eval uses WithDefaultPolicyBehavior + PolicyPathsForBehavior to route CONFIG policies to repository/registry evaluators and DYNAMIC policies to the image evaluator; single-bundle deployments continue to work unchanged while operators can scope by supplying two named bundles - FetchRegistryConfig error handling: error is now accumulated in evalErrors instead of silently logged, so the agent run status reflects the failure - Account filtering: FilterByAccounts helper added; repos are filtered to l.config.Accounts when the list is non-empty Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces three independent changes: a new ChangesAccount Repository Filtering
Policy Path Scoping by Resource Type
Release Automation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. 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 |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
158-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid fetching images when no image-scoped policy paths exist.
For config-only runs (
imagePathsempty), this still fetches images and can hard-fail the run onFetchImageserrors even though no image policy is being evaluated.Proposed fix
- // DYNAMIC — image scan checks - if len(repos) > 0 { + // DYNAMIC — image scan checks + if len(imagePaths) > 0 && len(repos) > 0 {🤖 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 `@main.go` around lines 158 - 171, The code currently fetches images and calls policyEvaluator.EvalImage even when imagePaths is empty (config-only runs); add a guard so image fetching and image evaluation only occur when len(imagePaths) > 0: inside the DYNAMIC image-scan block check if len(imagePaths) == 0 and skip the repoNames/accountID setup, the call to dataFetcher.FetchImages(...) and the loop over images/EvalImage; keep existing error returns only for the image path branch so config-only runs do not fail on FetchImages errors. Reference symbols: repos, repoNames, accountID, dataFetcher.FetchImages, images, policyEvaluator.EvalImage, imagePaths.
🤖 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 @.github/workflows/release.yml:
- Line 29: The workflow step uses "go install
github.com/compliance-framework/gooci@latest", which makes releases
non-reproducible; change that invocation to pin a specific release tag (e.g.,
replace "`@latest`" with a concrete version like "`@vX.Y.Z`") so the release job
installs an immutable, audited version of gooci—check the gooci releases page to
pick the correct tag and update the run command accordingly.
- Around line 18-20: Replace the tag reference actions/setup-go@v5 with a commit
SHA to pin the setup-go action for supply-chain security: locate the workflow
step that currently uses actions/setup-go@v5 (the step setting go-version-file:
go.mod) and change the action ref to the corresponding commit SHA for the
desired v5 release (obtain the exact SHA from the setup-go GitHub releases),
then commit the updated release workflow.
- Line 22: Replace the tag reference goreleaser/goreleaser-action@v6 with a
pinned commit SHA to improve supply-chain security: locate the workflow step
that uses goreleaser/goreleaser-action@v6 and change it to the full commit SHA
for the desired v6.x.x release (obtain the SHA from the
goreleaser/goreleaser-action GitHub releases), ensuring the action reference is
immutable.
- Around line 35-36: The conditional currently checks
"!github.event.release.prerelease" (which only exists on release events) —
replace that with a tag-name check using github.ref_name to detect prereleases;
e.g., change the if expression that reads "!github.event.release.prerelease" to
something like "!contains(github.ref_name, '-')" so tags with hyphenated
prerelease identifiers (v1.0.0-alpha, -beta, -rc) are skipped while stable tags
(v1.0.0) run the upload; update the same block that contains the run: gooci
upload ... line and use contains(github.ref_name, '-') or a more specific regex
via startsWith/matches if you need a different prerelease convention.
In `@internal/config_test.go`:
- Around line 8-37: Subtests reuse the shared repos slice so FilterByAccounts
may mutate input and leak state; update each t.Run to create its own copy of the
initial repos (or reconstruct the slice inline) before calling FilterByAccounts
and after calling it assert the original copy's contents (e.g., original
AccountIDs/RepositoryNames) are unchanged; reference the repos variable and the
FilterByAccounts function to locate where to make the per-subtest copy and add
the post-call equality check to ensure no input-slice mutation occurred.
In `@internal/util.go`:
- Around line 19-25: The current in-place filtering uses out := repos[:0], which
aliases and mutates the caller-owned repos backing array; change this to
allocate a new slice so the original repos is not modified (e.g., create out
with make(..., 0, len(repos)) or start with a nil slice) and then append
matching entries from repos where allowed[r.AccountID] is true; update the block
that iterates over repos and appends to out (using the same allowed and
r.AccountID symbols) and return out so callers receive a new filtered slice
without mutating the input.
In `@main.go`:
- Around line 135-137: The bug is that after calling FilterByAccounts(repos,
l.config.Accounts) the code still uses a single accountID := repos[0].AccountID
for all image fetches, which causes missing/misrouted queries for repos
belonging to other accounts; update the image-fetching logic (the block that
currently reads accountID := repos[0].AccountID and the subsequent fetch loop
around that area and the similar code later) to iterate per-repo (or group by
repo.AccountID and region) and use each repo.AccountID when calling the
image-fetch function(s), ensuring every repo uses its own AccountID rather than
the first element; adjust any helper signatures if needed so functions
referenced (e.g., the image fetcher method) accept an accountID parameter per
call.
- Around line 146-152: The code calls dataFetcher.FetchRegistryConfig and
evaluates registry policies even when registryPaths is empty, causing unrelated
fetch errors to be added to evalErrors during image-only runs; update the logic
in the block handling registry processing (where FetchRegistryConfig, registry,
registryPaths, policyEvaluator.EvalRegistry, evalErrors, and allEvidences are
referenced) to skip fetching and EvalRegistry entirely when len(registryPaths)
== 0 (or equivalent empty check) so registry config is only fetched and
evaluated when there are selected registry policies/paths.
---
Outside diff comments:
In `@main.go`:
- Around line 158-171: The code currently fetches images and calls
policyEvaluator.EvalImage even when imagePaths is empty (config-only runs); add
a guard so image fetching and image evaluation only occur when len(imagePaths) >
0: inside the DYNAMIC image-scan block check if len(imagePaths) == 0 and skip
the repoNames/accountID setup, the call to dataFetcher.FetchImages(...) and the
loop over images/EvalImage; keep existing error returns only for the image path
branch so config-only runs do not fail on FetchImages errors. Reference symbols:
repos, repoNames, accountID, dataFetcher.FetchImages, images,
policyEvaluator.EvalImage, imagePaths.
🪄 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: 2ea47b60-c40e-405d-9a22-6d1ee697c07b
📒 Files selected for processing (4)
.github/workflows/release.ymlinternal/config_test.gointernal/util.gomain.go
release.yml: - Pin actions/setup-go to 40f1582b (v5 SHA) - Pin goreleaser/goreleaser-action to e435ccd7 (v6 SHA) - Pin gooci to v0.0.7 instead of @latest - Fix prerelease guard: use !contains(github.ref_name, '-') instead of !github.event.release.prerelease (which is always empty on tag-push events) internal/util.go: - FilterByAccounts: use make([]RepositoryContext, 0, len(repos)) instead of repos[:0] so the function never aliases or mutates the caller-owned slice internal/config_test.go: - Per-subtest copy of the input repos slice so subtests are independent - Post-call assertion that the original slice was not mutated main.go: - Guard FetchRegistryConfig/EvalRegistry behind len(registryPaths) > 0 so registry config is not fetched during image-only runs - Guard image fetching behind len(imagePaths) > 0 so image APIs are not called during config-only runs - Group repos by AccountID and call FetchImages per-account so each repo uses its own AccountID instead of always using repos[0].AccountID Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests