lisa/feat/plugin-implementation - #1
Merged
Merged
Conversation
![lisa[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements an AWS ELBv2 compliance plugin: config parsing, normalized record models, a concurrent ELBv2+CloudTrail collector that batches tags and associates events, Rego-based policy evaluation in a RunnerV2 gRPC plugin, tests, CI/workflows, docs, and a license change. ChangesAWS ELBv2 Compliance Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
LICENSE (1)
1-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace this with the exact canonical Apache-2.0 license text.
This file is a paraphrased/abridged variant, not the official Apache License 2.0 text. That creates legal ambiguity around redistribution and notice obligations. Please replace it verbatim with the full canonical Apache-2.0 license content.
🤖 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 `@LICENSE` around lines 1 - 42, The LICENSE file currently contains a paraphrased/abridged variant of the Apache License 2.0; replace the entire contents of the LICENSE file with the exact canonical Apache-2.0 text (verbatim) as published by the Apache Foundation (including the full header, terms, conditions, and NOTICE requirements) so there is no legal ambiguity; locate the current block beginning with "Apache License Version 2.0, January 2004" and overwrite it with the official Apache-2.0 license text..github/workflows/test.yml (1)
1-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine explicit minimal permissions for reusable test workflow.
Because this is reusable (
workflow_call), setting explicit read-only permissions prevents accidental privilege expansion in callers.Suggested fix
name: test on: workflow_call: + +permissions: + contents: read jobs: test:🤖 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 1 - 15, The reusable workflow named "test" currently exposes default permissions; add an explicit minimal permissions block to the workflow to restrict callers (e.g., set permissions: contents: read) at the top-level of the workflow YAML so the "workflow_call" reusable workflow only has read-only access to repository contents when running the test job; update the .github/workflows/test.yml workflow metadata accordingly..github/workflows/push.yml (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit workflow permissions (least privilege).
push.ymlcurrently relies on repository defaults. Lock this down explicitly so behavior is deterministic across org/repo settings.Suggested fix
name: push on: pull_request: push: branches: - "**" + +permissions: + contents: read jobs: test: uses: ./.github/workflows/test.yml🤖 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/push.yml around lines 1 - 12, Add an explicit permissions block to the top-level "push" workflow to enforce least-privilege instead of relying on repo defaults: declare only the minimal scopes the test job needs (for example contents: read and any specific scopes required by the ./.github/workflows/test.yml composite like pull-requests: write), or override permissions at the job level for the "test" job that uses ./.github/workflows/test.yml so the workflow runs with deterministic, minimal access.
🤖 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/build-and-upload.yml:
- Line 14: Update the checkout step using actions/checkout@v4 to disable
persisted Git credentials by adding the input persist-credentials: false on that
step; locate the workflow step that uses "uses: actions/checkout@v4" and add the
persist-credentials key so the job does not retain repository credentials in the
workspace.
- Around line 14-18: Update the workflow uses lines so the actions are pinned to
specific commit SHAs instead of moving tags: replace occurrences of
"actions/checkout@v4", "actions/setup-go@v5", and
"goreleaser/goreleaser-action@v6" in .github/workflows/build-and-upload.yml (and
the "actions/checkout@v4" / "actions/setup-go@v5" usages in
.github/workflows/test.yml) with the corresponding full commit SHA refs (e.g.,
actions/checkout@<full-sha>, actions/setup-go@<full-sha>,
goreleaser/goreleaser-action@<full-sha>) so the workflows reference immutable
commits rather than floating tags.
In `@collector.go`:
- Around line 308-330: Listeners and target-groups are emitted without their
resource tags; for each listener ARN and target-group ARN call
c.collectTags(ctx, client, arn) (similar to how LB tags are collected) and store
them in result.tags[arn], append any returned CollectionError into
result.errors[arn], then pass those tags into newListenerRecord and
newTargetGroupRecord (update those constructors to accept a tags parameter) so
listener and target-group records include their own tags for policy evaluation.
- Around line 210-231: The worker pool can deadlock if c.Config.MaxConcurrency
<= 0 because workerCount becomes 0 and the job sender blocks on the unbuffered
jobs channel; ensure workerCount is at least 1 by setting workerCount = max(1,
c.Config.MaxConcurrency) (or similar) before creating workers and sending into
jobs, or alternatively make jobs buffered with capacity len(targets); update the
code around workerCount, jobs, and the goroutine spawn (function literal using
targetCtx and cancel, c.collectTarget, results channel) to handle zero/negative
MaxConcurrency safely so the for _, target := range targets { jobs <- target }
loop cannot block forever.
In `@main.go`:
- Around line 40-46: The struct CompliancePlugin stores mutable per-request
state (rawConfig, parsedConfig, policyData) and Configure/Eval access them
concurrently; add a sync.RWMutex (e.g., mu) to CompliancePlugin and use
mu.Lock()/mu.Unlock() around writes in Configure and mu.RLock()/mu.RUnlock()
around reads in Eval (or lock exclusively if Eval mutates), and ensure you copy
maps/structures when storing or returning them to avoid aliasing; update
references to rawConfig, parsedConfig, and policyData in Configure and Eval to
use the mutex-protected access.
- Around line 81-87: requestWithDefaultPolicyBehavior can return nil when req is
nil, so before calling PolicyPathsForBehavior on policyRequest you must guard
against a nil Eval request: check if policyRequest == nil (or policyRequest.Eval
== nil if relevant) and handle that case (return early with an error/empty paths
map or construct a safe default) so you avoid dereferencing a nil pointer when
building pathsByType; update the code that builds pathsByType (references:
requestWithDefaultPolicyBehavior, policyRequest, PolicyPathsForBehavior,
resourceTypeLoadBalancer/resourceTypeListener/resourceTypeTargetGroup/resourceTypeTargetHealth)
to use the nil-checked/initialized policyRequest or fallback empty slices.
In `@Makefile`:
- Around line 1-5: The Makefile's build and test targets should be declared
phony to avoid being skipped when files named "build" or "test" exist; update
the Makefile to add a .PHONY declaration that includes the build and test
targets (reference: targets "build" and "test") so Make always runs these
recipes.
---
Outside diff comments:
In @.github/workflows/push.yml:
- Around line 1-12: Add an explicit permissions block to the top-level "push"
workflow to enforce least-privilege instead of relying on repo defaults: declare
only the minimal scopes the test job needs (for example contents: read and any
specific scopes required by the ./.github/workflows/test.yml composite like
pull-requests: write), or override permissions at the job level for the "test"
job that uses ./.github/workflows/test.yml so the workflow runs with
deterministic, minimal access.
In @.github/workflows/test.yml:
- Around line 1-15: The reusable workflow named "test" currently exposes default
permissions; add an explicit minimal permissions block to the workflow to
restrict callers (e.g., set permissions: contents: read) at the top-level of the
workflow YAML so the "workflow_call" reusable workflow only has read-only access
to repository contents when running the test job; update the
.github/workflows/test.yml workflow metadata accordingly.
In `@LICENSE`:
- Around line 1-42: The LICENSE file currently contains a paraphrased/abridged
variant of the Apache License 2.0; replace the entire contents of the LICENSE
file with the exact canonical Apache-2.0 text (verbatim) as published by the
Apache Foundation (including the full header, terms, conditions, and NOTICE
requirements) so there is no legal ambiguity; locate the current block beginning
with "Apache License Version 2.0, January 2004" and overwrite it with the
official Apache-2.0 license text.
🪄 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: 25d7fe2c-b250-45eb-9923-1288191b7aac
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.github/workflows/build-and-upload.yml.github/workflows/push.yml.github/workflows/release.yml.github/workflows/test.yml.goreleaser.yamlLICENSEMakefileREADME.mdcollector.gocollector_test.goconfig.goconfig_test.gogo.modinput.gointernal/util.gomain.gorego_fixture_test.go
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 @.github/workflows/test.yml:
- Line 10: The checkout step currently uses "uses:
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5"; update that step to
disable persisting Git credentials by adding the input persist-credentials:
false to the actions/checkout step so the job does not leave repository write
credentials in the workspace.
In `@collector.go`:
- Around line 320-326: The per-resource calls to collectTags cause N remote
DescribeTags requests; instead gather all listener and target group ARNs in the
collector loop and call DescribeTags with ResourceArns in configurable-size
chunks, then map the returned TagDescriptions back into result.tags and record
any per-ARN errors as CollectionError (use the same CollectionError type).
Update the current usage sites that call collectTags(listenerARN) and
collectTags(targetGroupARN) (and the similar block at 336-340) to call a new
batch helper (or extend collectTags to accept []string) that performs chunked
DescribeTagsInput.ResourceArns requests and fans results/errors back into
result.tags and result.errors.
🪄 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: 585ef3a5-ed8d-4714-8859-ad3fd22e0b0d
📒 Files selected for processing (8)
.github/workflows/build-and-upload.yml.github/workflows/test.ymlMakefilecollector.gocollector_test.goinput.gomain.gorego_fixture_test.go
There was a problem hiding this comment.
Pull request overview
This PR introduces an AWS ELBv2 compliance plugin for the Compliance Framework agent, including collection of ELBv2 configuration and CloudTrail evidence, normalization into a consistent Rego input schema, and supporting CI/release automation.
Changes:
- Added ELBv2 collector + normalized input schema + policy evaluation plumbing for RunnerV2.
- Added unit tests (config parsing, collection behavior) and a Rego fixture test.
- Updated CI/release workflows, GoReleaser config, README documentation, and switched licensing to Apache 2.0.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
main.go |
Implements RunnerV2 plugin entrypoint, config handling, subject templates, and policy evaluation loop. |
collector.go |
Adds ELBv2 + CloudTrail collection with concurrency and record normalization. |
input.go |
Defines the normalized Rego input schema and per-resource record builders. |
config.go |
Adds parsing/validation for flat-string plugin configuration. |
internal/util.go |
Adds small shared helpers (map merge, string pointer, time formatting). |
collector_test.go |
Tests multi-record collection, pagination behavior, and error accumulation. |
config_test.go |
Tests config defaults, parsing, alias handling, and validation errors. |
rego_fixture_test.go |
Validates Rego evaluation against multiple record shapes via a generated fixture policy. |
README.md |
Documents plugin behavior, configuration keys, input schema, and coverage. |
LICENSE |
Updates repository license text (currently incomplete vs standard Apache 2.0 text). |
Makefile |
Adds basic build/test targets. |
go.mod / go.sum |
Introduces module definition and dependencies for AWS SDK v2 + CCF agent. |
.goreleaser.yaml |
Configures builds/archives/changelog for releasing. |
.github/workflows/test.yml |
Adds pinned-actions Go test workflow with go mod tidy + go test. |
.github/workflows/push.yml |
Runs tests on PRs and branch pushes. |
.github/workflows/release.yml |
Triggers the unified build/upload workflow on tag pushes. |
.github/workflows/build-and-upload.yml |
Builds/releases with GoReleaser and uploads artifact OCI images to GHCR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gusfcarvalho
requested changes
May 26, 2026
gusfcarvalho
left a comment
gusfcarvalho
left a comment
Contributor
There was a problem hiding this comment.
Some correctness/design issues and a few smells — left inline on the relevant lines. Main ones: incomplete go.sum masked by go mod tidy in CI, the per-target timeout conflation, and the substring-based CloudTrail-to-LB matching.
gusfcarvalho
requested changes
May 26, 2026
gusfcarvalho
left a comment
gusfcarvalho
left a comment
Contributor
There was a problem hiding this comment.
Round 2 — full re-check after the fix commit. The previous round's fixes are correctly applied (go.sum complete, readonly build/test/vet/-race all pass, CI green), and the two documentation-only conflicts got the requested comments. Three remaining items inline: a README tag-coverage inaccuracy (A), goreleaser still running go mod tidy (B), and an optional note on unbounded max_concurrency (C). All low severity.
gusfcarvalho
approved these changes
May 26, 2026
gusfcarvalho
left a comment
gusfcarvalho
left a comment
Contributor
There was a problem hiding this comment.
Approving. All prior review threads have been addressed and verified across rounds:
- Module integrity: go.sum is complete; build/vet/test/-race all pass under -mod=readonly; CI green; goreleaser and CI both use read-only go mod download + go mod verify.
- Round-1 fixes confirmed: listener-error scoping, maxLookbackDays split, behavior/resourceType constant consolidation, Eval RLock fast-path, clonePolicyInputs doc; the two deliberate-design items (raw-payload CloudTrail matching, per-target timeout budget) got clarifying comments.
- Round-2 fixes confirmed: README tag-coverage corrected, goreleaser go mod tidy removed, max_concurrency capped at 32 with validation + test.
Security: AssumeRole with external_id/session handled and external_id never emitted; configured account_id is verified against the STS-resolved identity; CloudTrail raw payloads excluded from output; actions pinned, persist-credentials disabled.
Two minor, pre-existing maintainability smells remain (the three near-identical pagination loops and the redundant GetCallerIdentity fallback) plus a hardcoded hclog.Debug level — all non-blocking and fine to defer.
Contributor
Author
|
PR approved. Marking as ready for e2e. |
Contributor
Author
|
PR approved. Marking as ready for e2e. |
gusfcarvalho
requested changes
May 27, 2026
Contributor
Author
|
No new automated feedback in the past 99 minutes. Waiting for a human review before continuing. |
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
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.
automated implementation by lisa.
Summary by CodeRabbit
New Features
CI / Chores
Documentation
Tests
License
Developer tooling