Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive tool policy system enabling granular control over bash and tool execution. It adds policy schema types to the API, implements a policy evaluation engine with bash command validation against rules, integrates policy checks into tool execution hooks, includes audit logging for policy decisions, and provides a UI for administrators to configure tool permissions and bash command patterns. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin UI
participant API as Agent Config API
participant Service as Frontend Service
participant Engine as Policy Engine
participant Audit as Audit Service
participant Executor as Tool Executor
Admin->>API: Update tool policy (enable/disable tools, bash rules)
API->>Service: UpdateAgentConfig with toolPolicy
Service->>Engine: ValidateToolPolicy
Engine-->>Service: Valid/Invalid result
alt Policy Invalid
Service-->>API: Error response
else Policy Valid
Service->>Service: Store policy in config
Service-->>API: Success response
API-->>Admin: Updated policy confirmed
end
Note over Admin,Executor: Later: Command Execution
Admin->>Executor: Execute bash command
Executor->>Service: OnBeforeToolExec hook
Service->>Engine: ResolveToolPolicy
Service->>Engine: EvaluateBashPolicy (match rules, check behaviors)
alt Rule Matches (Allow)
Engine-->>Service: Allowed decision
Service-->>Executor: Proceed with execution
else Rule Matches (Deny) or Default Deny
Engine-->>Service: Denied decision with reason
Service->>Executor: RequestCommandApproval callback
Executor->>Admin: Prompt user (approve/reject)
alt User Approves
Admin-->>Executor: Approval response
Executor->>Service: Policy override recorded
Service->>Audit: Log approval override
Service-->>Executor: Allow execution
else User Rejects
Admin-->>Executor: Rejection response
Service->>Audit: Log denial decision
Service-->>Executor: Block execution
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@internal/agent/policy.go`:
- Around line 53-69: The cloneBashRules function constructs copies of BashRule
but incorrectly uses new(*rules[i].Enabled) which is a compile error; change the
Enabled copy to produce a *bool by either calling the existing boolPtr helper
with the value (e.g., boolPtr(*rules[i].Enabled)) or by creating a local bool
variable v := *rules[i].Enabled and assigning &v to out[i].Enabled; update the
Enabled handling in cloneBashRules accordingly so it safely copies the pointer
when rules[i].Enabled != nil.
- Around line 372-414: The function hasUnsupportedShellConstructs fails to
detect process substitution patterns "<(" and ">("; update
hasUnsupportedShellConstructs to return true when encountering a '<' followed by
'(' or a '>' followed by '(' outside single-quoted contexts (honoring escaped
and inDouble state the same way other checks do), i.e., add checks similar to
the existing '`' and '$(' checks that verify escaped/inSingle/inDouble before
matching and then return true for "<(" and ">(" so calls like cat <(cmd) or cmd
>(out) are treated as unsupported.
In `@internal/service/frontend/api/v1/agent_config_test.go`:
- Around line 38-40: The tests use assert.Contains and mixed assertions; change
these to require calls for consistency: replace assert.Contains(t,
*getResp.ToolPolicy.Tools, "bash") with require.Contains and make the
surrounding nil checks (require.NotNil on getResp.ToolPolicy and
getResp.ToolPolicy.Tools) all use require (they already do) and similarly update
the assertions referenced at lines 163-167 to require.* variants; also remove
duplicated mock/test fixtures and import/use the shared fixtures from
internal/test instead of creating new mocks so tests follow the repo test
guidelines.
- Around line 16-18: The test uses invalid new() calls with values; update the
helper and call sites: change strPtr to return the address of its parameter (use
&v) so it compiles, and replace any direct new("allow_git_status") usages with a
string-pointer produced by calling strPtr("allow_git_status") or by creating a
local variable and taking its address; ensure all references to strPtr and the
literal pointer usage are updated accordingly.
In `@internal/service/frontend/api/v1/agent_config.go`:
- Around line 47-51: Rename the package-level error identifier
errInvalidToolPolicy to ErrInvalidToolPolicy (exported "Err..." style) and do
the same for the other package-level errors defined later in this file (the ones
around the 90-92 region); update all usages/imports in this package to the new
names so references compile, but leave the Error struct fields unchanged. Ensure
you update any tests or callers that reference the old names and run go
vet/build to catch any remaining references.
In `@ui/src/pages/agent-settings/index.tsx`:
- Around line 349-391: The four functions addBashRule, updateBashRule,
removeBashRule, and moveBashRule currently read toolPolicy from the render
closure and can produce stale updates; refactor each to compute the new rules
inside updateBashPolicy's functional updater (i.e., call updateBashPolicy((prev)
=> { const rules = [...(normalizeToolPolicy(prev).bash?.rules||[])]; /*
add/update/remove/move logic on rules */ return { ...prev, bash: { ...prev.bash,
rules } } })) and likewise update setBashRuleIds using its functional updater
deriving ids from prev (and call nextBashRuleId inside that updater for
addBashRule) so all mutations are based on the latest state rather than a stale
snapshot.
🧹 Nitpick comments (10)
internal/service/frontend/agent_policy_test.go (1)
30-111: Good test coverage of the core policy hook paths.The four subtests cover the main decision branches well. A couple of gaps to consider for completeness:
- No test for
configStore.Loadreturning an error (should yield"policy unavailable").- No test for a
nilconfigStore(should yield"agent policy unavailable").- No test for
BashDenyBehaviorBlock(i.e., deny withoutRequestCommandApprovalavailable or with block behavior), which exercises the fall-through path at the bottom of the hook.These are low-risk gaps since the production code is straightforward, but adding at least the "block" behavior case would strengthen coverage of policy enforcement semantics.
internal/agent/approval.go (1)
12-58: Clean approval flow implementation.Logic is sound. One minor observation:
The prompt is emitted (Line 29) before the
nilparentCtx guard (Line 41). IfparentCtxis nil, the prompt will have already been sent to the user before the fallbackcontext.Background()is assigned. This is harmless sinceemitdoesn't use the context, but reordering the nil-check beforeemitwould be slightly more defensive.internal/agent/policy_test.go (1)
51-133: Thorough bash policy evaluation tests.Good coverage of allow/deny rules, default behaviors, and unsupported constructs. One gap: no test exercises a
BashRulewithEnabledset tofalse(*bool). SincecompileEnabledBashRulespresumably skips disabled rules, a test case confirming that a disabled deny rule doesn't block execution would be valuable.internal/service/frontend/agent_policy.go (3)
43-64: Double policy resolution —EvaluateBashPolicyresolves internally.
ResolveToolPolicyis called here at Line 43, producingpolicy. That resolved config is then passed toEvaluateBashPolicyat Line 57, which internally callsResolveToolPolicyagain (seeinternal/agent/policy.goLine 185). This is redundant work on every bash tool invocation.Either pass the raw
cfg.ToolPolicyand letEvaluateBashPolicyhandle resolution entirely, or introduce a variant that accepts an already-resolved policy.
103-120: Audit logging uses a detachedcontext.Background()— consider propagating the request context.Line 119 creates a new
context.Background()instead of using the caller's context. If the audit service respects cancellation or has tracing/correlation propagation, this disconnects audit logs from the request lifecycle. Using the request context (or at least documenting why it's detached) would be more robust.
53-55: Use a constant or helper function for the"bash"tool name string.The string
"bash"is hardcoded in the comparison. Theagentpackage defines the private constanttoolNameBash, but it cannot be accessed from thefrontendpackage. Consider either exporting this constant (e.g.,ToolNameBash) or adding an exported helper function likeIsBashTool(toolName string) boolin theagentpackage to avoid string duplication.internal/agent/loop.go (1)
329-338:PolicyCheckedis set based on hook presence, not actual execution.
PolicyCheckedis set totruewheneverHasBeforeToolExecHooks()returns true (i.e., hooks are registered), regardless of whether the hook actually ran successfully for this specific call. SinceRunBeforeToolExecat Line 325 returns an error and short-circuits on failure (Line 326), a tool only reaches Line 329 if the hook succeeded. So in practice this is correct — but the field namePolicyCheckedcombined with the value sourceHasBeforeToolExecHooks()is a bit misleading. The value really means "policy hooks are registered and they passed."internal/agent/store.go (1)
97-115: Default tool list and test coverage mismatch.
DefaultToolPolicydefines 8 tools (bash,read,patch,think,navigate,read_schema,ask_user,web_search), butTestResolveToolPolicy_Defaultsinpolicy_test.goonly asserts 5 of them (missingthink,navigate,read_schema). If any of those defaults are accidentally removed, the test won't catch it.ui/src/pages/agent-settings/index.tsx (2)
91-122: Inconsistent fallback defaults betweennormalizeToolPolicyandcanonicalizeToolPolicy.
normalizeToolPolicydefaultsdefaultBehaviortoallow(viacreateDefaultToolPolicy), butcanonicalizeToolPolicyat line 118 falls back todeny. After normalization the value is always populated so this won't bite at runtime, but the mismatch is misleading for future maintainers.♻️ Align the fallback or remove the redundant guard
Since
canonicalizeToolPolicycallsnormalizeToolPolicyfirst, the||fallback on lines 118-119 is dead code. Remove it for clarity:return { tools, bash: { rules, - defaultBehavior: normalized.bash?.defaultBehavior || AgentBashPolicyDefaultBehavior.deny, - denyBehavior: normalized.bash?.denyBehavior || AgentBashPolicyDenyBehavior.ask_user, + defaultBehavior: normalized.bash!.defaultBehavior!, + denyBehavior: normalized.bash!.denyBehavior!, }, };Or, if you want to keep a safety net, use the same defaults as
createDefaultToolPolicy(allow/ask_user).
252-264: Redundant state updates beforefetchConfig()re-fetch.Lines 252-260 manually set
toolPolicy,savedConfig, etc. from the PATCH response, then line 264 callsfetchConfig()which overwrites the same state. The manual set is unnecessary—fetchConfigis the authoritative source. Additionally,fetchConfigregeneratesbashRuleIdsviabuildBashRuleIDs, which will assign new IDs and force React to remount all rule editors (losing any transient focus).Consider either removing the manual state updates (lines 252-260) or removing the
fetchConfig()re-call.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1667 +/- ##
==========================================
- Coverage 70.24% 69.80% -0.45%
==========================================
Files 345 347 +2
Lines 38740 39064 +324
==========================================
+ Hits 27213 27268 +55
- Misses 9363 9425 +62
- Partials 2164 2371 +207
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes