feat: add support for policy definition configuration#85
Merged
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 adds policy behavior configuration support throughout the agent's plugin lifecycle. It extends the runner proto contract with policy behavior mappings, implements helper methods for behavior manipulation and filtering, integrates configuration threading from agent startup through init/configure/eval phases, and updates tests to verify the new behavior flows. ChangesPolicy Behavior Configuration and Threading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for configuring “policy behavior” alongside policy paths so the agent can pass policy categorization/behavior metadata to runner plugins and provide helper logic for mapping policies to behaviors.
Changes:
- Extend runner protobuf API (Configure/Init/Eval requests) to include a
policyBehaviormap and introduce aStringListwrapper message. - Add
EvalRequesthelpers to merge default behavior mappings, fill undefined mappings, and filter policy paths by behavior (with unit tests). - Wire
policy_behaviorfrom agent plugin configuration through to runnerConfigure,Init, andEvalcalls.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runner/proto/runner.proto | Adds StringList and policyBehavior fields to request messages. |
| runner/proto/runner.pb.go | Regenerates Go protobuf bindings to reflect the new API fields/types. |
| runner/proto/policy_behavior.go | Introduces helper methods for default/undefined behavior mapping and behavior-based path filtering. |
| runner/proto/policy_behavior_test.go | Adds unit tests covering behavior merging, undefined mapping, and filtering logic. |
| cmd/agent.go | Plumbs policy_behavior config into runner Configure/Init/Eval requests and adds a conversion helper. |
| cmd/agent_test.go | Updates tests to cover policyBehavior being sent on Configure and signature changes. |
Files not reviewed (1)
- runner/proto/runner.pb.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+975
to
+980
| if policyBehavior == nil { | ||
| return nil | ||
| } | ||
| result := make(map[string]*proto.StringList) | ||
| for key, values := range policyBehavior { | ||
| result[key] = &proto.StringList{Values: values} |
Comment on lines
+100
to
+105
| func pathCoveredByPolicyBehavior(path string, behavior map[string]*StringList) bool { | ||
| for key := range behavior { | ||
| if strings.Contains(path, key) { | ||
| return true | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmd/agent_test.go`:
- Around line 1018-1020: The test directly indexes proto fields which can
nil-deref; change the assertion to use the generated getters: fetch the policy
bundle via pb :=
testRunner.configureRequest.GetPolicyBehavior()["policy-bundle"] (or check for
nil first), then obtain values with pb.GetValues() (guarding pb == nil) and
compare that slice to the expected []string{"vpc","sg"} using reflect.DeepEqual;
update the test to handle the nil pb case explicitly so no direct field access
occurs.
In `@runner/proto/policy_behavior.go`:
- Around line 25-29: The local variable named copy shadows Go's predeclared copy
builtin; rename that variable (e.g., change copy to cloned or clonedReq) where
the EvalRequest is constructed so references to EvalRequest{PolicyPaths:
slices.Clone(r.PolicyPaths), ApiServer: r.ApiServer, PolicyBehavior:
mergePolicyBehavior(nil, r.PolicyBehavior)} no longer use the identifier copy;
update all subsequent uses of that variable in the function to the new name to
avoid shadowing and lint failures.
In `@runner/proto/runner.proto`:
- Line 20: Rename the protobuf field policyBehavior to policy_behavior in all
affected message definitions (ConfigureRequest, InitRequest, and EvalRequest) so
it follows lower_snake_case; keep the value type as map<string, StringList> so
the generated Go field remains PolicyBehavior. Update all references/usages in
proto comments or options accordingly and then regenerate the Go artifacts
(protoc/buf) to pick up the renamed field. Ensure any code that referenced the
old proto field name is updated to use the regenerated Go field or the new proto
field name where applicable.
🪄 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: 2d8f1c00-34fa-4574-af8c-bc07608b5993
⛔ Files ignored due to path filters (1)
runner/proto/runner.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
cmd/agent.gocmd/agent_test.gorunner/proto/policy_behavior.gorunner/proto/policy_behavior_test.gorunner/proto/runner.proto
| message ConfigureRequest { | ||
| map<string, string> config = 1; | ||
| google.protobuf.Struct policy_data = 2; | ||
| map<string, StringList> policyBehavior = 3; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced proto file around the mentioned lines
echo "== runner/proto/runner.proto (context around lines ~1-60) =="
nl -ba runner/proto/runner.proto | sed -n '1,120p'
echo
echo "== Search for policyBehavior in the repo =="
rg -n "policyBehavior" -S .
echo
echo "== Search for policy_behavior in the repo =="
rg -n "policy_behavior" -S .Repository: compliance-framework/agent
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Confirm file exists =="
ls -la runner/proto/runner.proto || true
echo
echo "== runner/proto/runner.proto (first 120 lines with line numbers) =="
python3 - <<'PY'
import itertools
path = "runner/proto/runner.proto"
with open(path, "r", encoding="utf-8") as f:
for i, line in zip(range(1, 121), f):
print(f"{i:4d}\t{line.rstrip()}")
PY
echo
echo "== Search for policyBehavior in the repo =="
rg -n "policyBehavior" -S .
echo
echo "== Search for policy_behavior in the repo =="
rg -n "policy_behavior" -S .Repository: compliance-framework/agent
Length of output: 5023
Rename proto field policyBehavior to policy_behavior to satisfy Buf/protobuf naming lint
runner/proto/runner.proto defines policyBehavior in ConfigureRequest, InitRequest, and EvalRequest; protobuf field names should be lower_snake_case. Renaming to policy_behavior keeps the Go generated field name as PolicyBehavior.
Suggested fix
message ConfigureRequest {
map<string, string> config = 1;
google.protobuf.Struct policy_data = 2;
- map<string, StringList> policyBehavior = 3;
+ map<string, StringList> policy_behavior = 3;
}
@@
message InitRequest {
repeated string policyPaths = 1;
uint32 apiServer = 2;
- map<string, StringList> policyBehavior = 3;
+ map<string, StringList> policy_behavior = 3;
}
@@
message EvalRequest {
repeated string policyPaths = 1;
uint32 apiServer = 2;
- map<string, StringList> policyBehavior = 3;
+ map<string, StringList> policy_behavior = 3;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| map<string, StringList> policyBehavior = 3; | |
| map<string, StringList> policy_behavior = 3; |
🧰 Tools
🪛 Buf (1.69.0)
[error] 20-20: Field name "policyBehavior" should be lower_snake_case, such as "policy_behavior".
(FIELD_LOWER_SNAKE_CASE)
🤖 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 `@runner/proto/runner.proto` at line 20, Rename the protobuf field
policyBehavior to policy_behavior in all affected message definitions
(ConfigureRequest, InitRequest, and EvalRequest) so it follows lower_snake_case;
keep the value type as map<string, StringList> so the generated Go field remains
PolicyBehavior. Update all references/usages in proto comments or options
accordingly and then regenerate the Go artifacts (protoc/buf) to pick up the
renamed field. Ensure any code that referenced the old proto field name is
updated to use the regenerated Go field or the new proto field name where
applicable.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/agent_test.go (1)
1065-1066:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse proto getters in this new assertion to satisfy protogetter and avoid nil dereference.
Line 1065 still directly accesses
got["bundle"].Values. Switch toGetValues()in both the assertion and failure message.Suggested fix
- if !reflect.DeepEqual(got["bundle"].Values, want) { - t.Fatalf("policyBehaviorToProto() values = %#v, want %#v", got["bundle"].Values, want) + bundle := got["bundle"] + if !reflect.DeepEqual(bundle.GetValues(), want) { + t.Fatalf("policyBehaviorToProto() values = %#v, want %#v", bundle.GetValues(), want) }🤖 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 `@cmd/agent_test.go` around lines 1065 - 1066, The test assertion directly accesses got["bundle"].Values which can nil-deref; replace both uses with the proto getter: call got["bundle"].GetValues() in the reflect.DeepEqual check and in the t.Fatalf message (e.g., change policyBehaviorToProto() values = %#v to use got["bundle"].GetValues()), so the test uses the proto getter API and avoids nil dereference.
🤖 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.
Duplicate comments:
In `@cmd/agent_test.go`:
- Around line 1065-1066: The test assertion directly accesses
got["bundle"].Values which can nil-deref; replace both uses with the proto
getter: call got["bundle"].GetValues() in the reflect.DeepEqual check and in the
t.Fatalf message (e.g., change policyBehaviorToProto() values = %#v to use
got["bundle"].GetValues()), so the test uses the proto getter API and avoids nil
dereference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: adee60d8-b117-44e0-9e3a-1591865848dd
📒 Files selected for processing (4)
cmd/agent.gocmd/agent_test.gorunner/proto/policy_behavior.gorunner/proto/policy_behavior_test.go
| return nil | ||
| } | ||
| result := make(map[string]*proto.StringList, len(policyBehavior)) | ||
| for key, values := range policyBehavior { |
Comment on lines
+65
to
+73
| filtered := make([]string, 0, len(r.PolicyPaths)) | ||
| outer: | ||
| for _, path := range r.PolicyPaths { | ||
| for _, key := range matchingKeys { | ||
| if pathCoveredByPolicyBehavior(path, map[string]*StringList{key: nil}) { | ||
| filtered = append(filtered, path) | ||
| continue outer | ||
| } | ||
| } |
Comment on lines
950
to
966
| @@ -961,6 +962,7 @@ func TestInitRunner(t *testing.T) { | |||
| &initTestRunner{initErr: status.Error(codes.Unimplemented, "not implemented")}, | |||
| nil, | |||
| nil, | |||
| nil, | |||
| ) | |||
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.
Summary by CodeRabbit
New Features
Documentation
Tests