lisa/feat/plugin-implementation#1
Conversation
|
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 (4)
📝 WalkthroughWalkthroughThis PR implements an AWS Secrets Manager compliance plugin: configuration parsing, concurrent secret collection across accounts/regions, normalization into ResourceRecords, CloudTrail correlation (Secrets Manager and IAM), Rego policy evaluation via a HashiCorp runner plugin, tests, README replacement, and CI workflow updates. ChangesAWS Secrets Manager Compliance Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Findings inline. Headline: CI is blocked by a non-existent actions/checkout SHA pinned in both test and release workflows; otherwise the implementation is in good shape, with three low-severity correctness/maintainability notes on IAM event attribution and friendly-name suffix handling. Local go build, go vet, go test -race, and gofmt are all clean. The pass-1/2/3 self-review fixes (planned-deletion listing, us-east-1 CloudTrail for IAM, duplicate-friendly-name index, go mod verify in release hooks) all look correctly applied and tested.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@collector.go`:
- Around line 102-104: The code in collector.go currently masks a missing AWS
region by setting regions = []string{""}; instead, detect when len(regions)==0
and return a clear configuration error (e.g., fmt.Errorf or a package
ErrInvalidConfig) from the function that constructs targets (the function that
owns the 'regions' variable) so failure is fast and descriptive; replace the
empty-string fallback with a returned error and update any callers to
propagate/handle that error accordingly.
In `@main_test.go`:
- Around line 37-43: The test currently calls p.Eval(nil, nil) and dereferences
resp without guarding for a nil response; update the nil-request test around
p.Eval to explicitly check if resp == nil after the call and t.Fatalf with a
clear message (e.g., "expected non-nil resp when error returned" or include the
err) before calling resp.GetStatus(), and only then assert resp.GetStatus() ==
proto.ExecutionStatus_FAILURE so the test cannot panic when Eval returns (nil,
err).
In `@main.go`:
- Line 227: The logger is currently created with debug enabled unconditionally
(logger := hclog.New(&hclog.LoggerOptions{Level: hclog.Debug, JSONFormat:
true})); change this to use Info as the default level and make Debug opt‑in via
configuration or an environment variable (e.g., read LOG_LEVEL or DEBUG at
startup), map that value to an hclog level (using hclog parsing or a small
switch) and pass the resolved level into hclog.New so the runtime defaults to
Info but can be elevated to Debug when explicitly set.
- Line 102: When building the Collector you must guard against a nil logger on
the CompliancePlugin receiver (l) before calling l.logger.Named; if l.logger is
nil assign a safe default no-op logger to l.logger (e.g., a zap.NewNop() or
equivalent no-op logger used in this project) and then call l.logger.Named to
construct the Collector: update the code around the Collector creation
(referencing Collector, CompliancePlugin receiver l, and l.logger.Named) to
ensure l.logger is non-nil first.
🪄 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: 22c450bd-1eae-4600-b4c4-40fc86c365e8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/build-and-upload.yml.github/workflows/release.yml.github/workflows/test.ymlREADME.mdcollector.gocollector_test.goconfig.goconfig_test.gogo.modinput.gointernal/util.gomain.gomain_test.gorego_fixture_test.go
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
Approving. All five of my prior threads are addressed (CI checkout SHA, one-way principal match, role-side IAM events, friendly-name suffix), and the four CodeRabbit threads from the previous cycle are now resolved with code in commit 4379717: regions-resolution returns a clear error instead of falling through to an empty region; Eval guards a nil logger; main reads LOG_LEVEL (defaulting to info); the nil-request test guards resp before GetStatus. Three new focused tests (TestDefaultAWSClientFactoryResolveTargetsRequiresRegion, TestEvalNilLoggerDoesNotPanic, TestDefaultLogLevel) cover those changes. CI is green (test / test passing on both runs), and local go build, go vet, go test -race, and gofmt -l are clean on commit 4379717.
|
PR approved. Marking as ready for e2e. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/build-and-upload.yml (2)
17-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPin the GoReleaser CLI to an exact release.
The action is SHA-pinned, but
version: "~> v2"still downloads a moving GoReleaser binary. The action docs allow a fixed version, and using a range here means release behavior can change without any workflow diff. Pin the exactv2.x.yyou have validated in CI. (github.com)🤖 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 @.github/workflows/build-and-upload.yml around lines 17 - 20, The workflow uses the goreleaser action 'goreleaser/goreleaser-action' with a moving version specifier version: "~> v2"; change this to an exact validated tag (e.g. v2.x.y) so the action installs a fixed GoReleaser binary and release behavior remains deterministic—update the version field in the same step that references goreleaser/goreleaser-action and keep args: release --clean untouched.
17-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit
permissionsfor the reusablebuild-and-uploadjob (contents+packages)This is a
workflow_callreusable workflow and it currently has no job/workflowpermissions, so theGITHUB_TOKENscopes depend on the caller/default. The steps usinggoreleaser/goreleaser-actionandgoocito push to GHCR needcontents: writeandpackages: writefor the providedGITHUB_TOKENto publish successfully.Suggested change
jobs: build-and-upload: runs-on: ubuntu-latest + permissions: + contents: write + packages: write steps:Pin
goreleaser/goreleaser-action’swith.versionto an exact GoReleaser v2.x.y release (avoid the floating~> v2) to improve release reproducibility.🤖 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 @.github/workflows/build-and-upload.yml around lines 17 - 22, This reusable workflow needs explicit workflow permissions and a fixed GoReleaser action version: add a top-level permissions block granting contents: write and packages: write for the workflow_call so the GITHUB_TOKEN can push to GHCR, and replace the floating with: "~> v2" in the goreleaser/goreleaser-action step's with.version with an exact v2.x.y tag (pin to a concrete GoReleaser v2 release) to ensure reproducible releases; locate the goreleaser step (goreleaser/goreleaser-action and its with.version) and the workflow declaration to add the permissions..github/workflows/test.yml (1)
10-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict the reusable
testjob to read-onlyGITHUB_TOKENpermissions.
.github/workflows/test.ymlis invoked viaworkflow_call, andjobs.testhas nopermissionsblock; addpermissions: contents: readso the workflow doesn’t inherit broader repository defaults.Suggested change
jobs: test: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 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 @.github/workflows/test.yml around lines 10 - 18, The reusable workflow's test job lacks a permissions block, so add a permissions entry for the job named "test" (jobs.test) to restrict GITHUB_TOKEN to read-only access: add a permissions section with contents: read under jobs.test in .github/workflows/test.yml so the workflow_call cannot inherit broader repo permissions; update the workflow YAML to include this permissions block adjacent to the job definition that contains the steps (actions/checkout, setup-go, go mod download/verify, go test).
🤖 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 @.github/workflows/build-and-upload.yml:
- Around line 17-20: The workflow uses the goreleaser action
'goreleaser/goreleaser-action' with a moving version specifier version: "~> v2";
change this to an exact validated tag (e.g. v2.x.y) so the action installs a
fixed GoReleaser binary and release behavior remains deterministic—update the
version field in the same step that references goreleaser/goreleaser-action and
keep args: release --clean untouched.
- Around line 17-22: This reusable workflow needs explicit workflow permissions
and a fixed GoReleaser action version: add a top-level permissions block
granting contents: write and packages: write for the workflow_call so the
GITHUB_TOKEN can push to GHCR, and replace the floating with: "~> v2" in the
goreleaser/goreleaser-action step's with.version with an exact v2.x.y tag (pin
to a concrete GoReleaser v2 release) to ensure reproducible releases; locate the
goreleaser step (goreleaser/goreleaser-action and its with.version) and the
workflow declaration to add the permissions.
In @.github/workflows/test.yml:
- Around line 10-18: The reusable workflow's test job lacks a permissions block,
so add a permissions entry for the job named "test" (jobs.test) to restrict
GITHUB_TOKEN to read-only access: add a permissions section with contents: read
under jobs.test in .github/workflows/test.yml so the workflow_call cannot
inherit broader repo permissions; update the workflow YAML to include this
permissions block adjacent to the job definition that contains the steps
(actions/checkout, setup-go, go mod download/verify, go test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 70c8fab3-324f-4870-af9f-bc508e6966b6
📒 Files selected for processing (5)
.github/workflows/build-and-upload.yml.github/workflows/test.ymlREADME.mdcollector.gocollector_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
collector_test.go (2)
204-218:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid order-dependent assertions for record content.
Line 204 assumes
result.Records[0]is thearn1record. If record ordering changes, this test can become flaky.Suggested fix
- rec := result.Records[0] + var rec ResourceRecord + found := false + for _, r := range result.Records { + if r.Labels["resource_arn"] == arn1 { + rec = r + found = true + break + } + } + if !found { + t.Fatalf("record for %s not found", arn1) + }🤖 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 `@collector_test.go` around lines 204 - 218, The test is making order-dependent assertions by indexing result.Records[0]; modify the test to find the specific record for "arn1" instead of assuming position: iterate result.Records to locate the record whose identifier (e.g., rec.Input.ARN or rec.Input.Arn) equals the expected arn string, assign that to rec, and then run the existing checks against rec.Input.Config["kms_key_id"], ["deprecated_version_count"], ["replication_status"], and rec.Input.Dynamic["cloudtrail_events"]/["iam_credential_removal_events"] so the assertions are stable regardless of record ordering.
298-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
deleted_dateassertion type-safe to avoid false positives.Line 298 currently passes if
deleted_dateis missing (nil), becausenil != "". This can hide regressions.Suggested fix
- if rec.Input.Config["deleted_date"] == "" { - t.Fatalf("deleted_date not exposed") - } + deletedDate, ok := rec.Input.Config["deleted_date"].(string) + if !ok || deletedDate == "" { + t.Fatalf("deleted_date not exposed: %#v", rec.Input.Config["deleted_date"]) + }🤖 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 `@collector_test.go` around lines 298 - 300, The current test compares rec.Input.Config["deleted_date"] to "" which passes if the key is missing or nil; change the assertion to be type-safe by first checking the key exists and the value is a string: retrieve v, ok := rec.Input.Config["deleted_date"], fail if !ok; then assert s, ok := v.(string) and fail if !ok or s == "" so you ensure the key is present and holds a non-empty string (use the same rec.Input.Config lookup and "deleted_date" identifier).
🤖 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 `@collector_test.go`:
- Around line 204-218: The test is making order-dependent assertions by indexing
result.Records[0]; modify the test to find the specific record for "arn1"
instead of assuming position: iterate result.Records to locate the record whose
identifier (e.g., rec.Input.ARN or rec.Input.Arn) equals the expected arn
string, assign that to rec, and then run the existing checks against
rec.Input.Config["kms_key_id"], ["deprecated_version_count"],
["replication_status"], and
rec.Input.Dynamic["cloudtrail_events"]/["iam_credential_removal_events"] so the
assertions are stable regardless of record ordering.
- Around line 298-300: The current test compares
rec.Input.Config["deleted_date"] to "" which passes if the key is missing or
nil; change the assertion to be type-safe by first checking the key exists and
the value is a string: retrieve v, ok := rec.Input.Config["deleted_date"], fail
if !ok; then assert s, ok := v.(string) and fail if !ok or s == "" so you ensure
the key is present and holds a non-empty string (use the same rec.Input.Config
lookup and "deleted_date" identifier).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f2b6552-2fda-4af9-afb9-f22418fbec1d
📒 Files selected for processing (5)
README.mdcollector.gocollector_test.gomain.gomain_test.go
|
No new automated feedback in the past 62 minutes. Waiting for a human review before continuing. |
1 participant
automated implementation by lisa.
Summary by CodeRabbit
New Features
Configuration
Documentation
Utilities
Tests
Chores