feat: add runtime permission decisions - #95
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. WalkthroughThis PR implements an interactive permission request and decision workflow. It introduces new permission event types (request vs. decision), adds sandbox-based permission gating to the agent, surfaces in-view permission prompts in the TUI, and updates the session/exec event pipelines to properly classify and persist these permission events. ChangesPermission Request/Decision Workflow
Sequence Diagram(s)sequenceDiagram
participant Agent as agent.Run
participant ToolExec as executeToolCall
participant SandboxDecider as sandbox.Engine
participant UserCaller as OnPermissionRequest
participant SessionLog as session recorder
participant OnPermissionEvent as OnPermission
Agent->>ToolExec: execute write_file
ToolExec->>SandboxDecider: compute preflight decision
SandboxDecider-->>ToolExec: decision (requires approval)
ToolExec->>UserCaller: request permission with metadata
alt User grants permission
UserCaller-->>ToolExec: PermissionDecisionAllow
ToolExec->>SessionLog: append permission_decision event
else User always-allows
UserCaller-->>ToolExec: PermissionDecisionAlwaysAllow
ToolExec->>SandboxDecider: persist grant via Grant()
ToolExec->>SessionLog: append permission_decision event
else User denies
UserCaller-->>ToolExec: PermissionDecisionDeny
ToolExec->>SessionLog: append permission_decision event (denied)
ToolExec-->>Agent: ToolResult with error
end
ToolExec->>OnPermissionEvent: emit with DecisionReason
OnPermissionEvent-->>Agent: PermissionEvent recorded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/agent/loop.go (2)
387-396: 💤 Low valueShallow clone of args map
cloneArgscreates a shallow copy—ifargscontains nested maps or slices (e.g., from JSON arrays/objects), mutations to the clone's nested values would affect the original. In practice this is unlikely since JSON-decoded tool arguments are typically primitives, but worth noting if the args structure becomes more complex.🤖 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 `@internal/agent/loop.go` around lines 387 - 396, cloneArgs currently performs a shallow copy which will allow nested maps/slices in args to remain shared; update cloneArgs to perform a deep copy of the args map (recursively clone maps and slices and copy primitive values) so mutations to nested structures in the returned map do not affect the original; you can implement this by writing a helper that inspects values and recursively clones map[string]any and []any, or by using JSON marshal/unmarshal as a simple deep-clone fallback, ensuring cloneArgs returns an independent structure.
266-293: 💤 Low valueDead code in event construction
Lines 274-283 copy
requestEventinto a localeventand modify itsRiskandToolNamefields, but neither is used—onlyevent.Actionis read for theMetamap. TheRiskandToolNameassignments have no effect on the returnedToolResult.Simplify by removing unused assignments
func deniedPermissionResult(call ToolCall, reason string, requestEvent PermissionEvent) ToolResult { reason = strings.TrimSpace(reason) if reason == "" { reason = requestEvent.Reason } if reason == "" { reason = "tool requires approval before execution" } - event := requestEvent - event.Action = PermissionActionDeny - event.PermissionGranted = false - event.DecisionReason = reason - if event.Risk.Level == "" { - event.Risk = sandbox.Risk{Level: sandbox.RiskMedium, Reason: reason} - } - if requestEvent.ToolName == "" { - event.ToolName = call.Name - } return ToolResult{ ToolCallID: call.ID, Name: call.Name, Status: tools.StatusError, Output: "Error: Permission denied for " + call.Name + ": " + reason, Meta: map[string]string{ - "permission_action": string(event.Action), + "permission_action": string(PermissionActionDeny), }, } }🤖 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 `@internal/agent/loop.go` around lines 266 - 293, The helper deniedPermissionResult constructs a local copy event := requestEvent and mutates event.Risk and event.ToolName even though only event.Action is read for the Meta map and the mutated fields are never used; remove the unused assignments (event.Risk = ... and event.ToolName = ...) and the unnecessary copy if you only need to set Action for the meta, ensuring the Meta still contains permission_action using either requestEvent.Action or setting event.Action directly before building the ToolResult so the function remains correct and side-effect free.internal/sandbox/engine.go (1)
107-112: ⚡ Quick winAdd context parameter for cancellation and timeout support.
Grant storage may involve I/O (disk, database). Without a context parameter, the operation cannot be cancelled if the parent context is cancelled, and no deadline/timeout can be enforced. This risks resource leaks if storage blocks.
The
Evaluatemethod (line 36) takes a context, and theOnPermissionRequestcallback (types.go:94) receives one—Grantshould follow the same pattern.🔧 Proposed fix to add context parameter
-func (engine *Engine) Grant(input GrantInput) (Grant, error) { +func (engine *Engine) Grant(ctx context.Context, input GrantInput) (Grant, error) { if engine == nil || engine.store == nil { return Grant{}, errors.New("sandbox grant store is not configured") } - return engine.store.Grant(input) + return engine.store.Grant(ctx, input) }Note: This assumes
GrantStore.Grantwill also be updated to accept a context. If the store interface doesn't yet support context, update it as well for consistency.🤖 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 `@internal/sandbox/engine.go` around lines 107 - 112, The Grant method lacks a context parameter—update Engine.Grant to accept a context.Context (e.g., func (engine *Engine) Grant(ctx context.Context, input GrantInput) (Grant, error)), update any call sites, and propagate that context into the underlying store call by changing the store interface method Grant to accept a context (GrantStore.Grant(ctx context.Context, input GrantInput) (Grant, error)) and calling engine.store.Grant(ctx, input); also mirror the pattern used by Evaluate and the OnPermissionRequest callback (types.go:94) so cancellations/timeouts are honored end-to-end.internal/tui/model.go (1)
524-552: 💤 Low valueSession logging skips permission request when custom handler provided.
If
onPermissionRequestis not nil (line 526), the early return at line 527 bypasses thesessionEventsappend at lines 539-542, so the session won't record anEventPermissionRequest. TheOnPermissioncallback (line 609) will still log the permission event, but session replay won't show the request phase—only the decision outcome.This is acceptable for custom handlers (testing or embedding scenarios), but means session event streams differ based on whether a custom handler is active. If session replay fidelity is critical, consider always logging the request before delegating, or document this behavior clearly.
🤖 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 `@internal/tui/model.go` around lines 524 - 552, The permission request is not being appended to sessionEvents when a custom handler (onPermissionRequest) is present; change the flow in the OnPermissionRequest wrapper so the pendingSessionEvent for sessions.EventPermissionRequest is recorded unconditionally before delegating to onPermissionRequest or performing the default UI flow. Specifically, ensure the sessionEvents append of pendingSessionEvent{Type: sessions.EventPermissionRequest, Payload: request} occurs before the onPermissionRequest != nil check (or duplicate the append into both branches) and keep existing references to runID, m.sendPermissionRequest, decisionCh, and the permission decision handling intact.
🤖 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.
Nitpick comments:
In `@internal/agent/loop.go`:
- Around line 387-396: cloneArgs currently performs a shallow copy which will
allow nested maps/slices in args to remain shared; update cloneArgs to perform a
deep copy of the args map (recursively clone maps and slices and copy primitive
values) so mutations to nested structures in the returned map do not affect the
original; you can implement this by writing a helper that inspects values and
recursively clones map[string]any and []any, or by using JSON marshal/unmarshal
as a simple deep-clone fallback, ensuring cloneArgs returns an independent
structure.
- Around line 266-293: The helper deniedPermissionResult constructs a local copy
event := requestEvent and mutates event.Risk and event.ToolName even though only
event.Action is read for the Meta map and the mutated fields are never used;
remove the unused assignments (event.Risk = ... and event.ToolName = ...) and
the unnecessary copy if you only need to set Action for the meta, ensuring the
Meta still contains permission_action using either requestEvent.Action or
setting event.Action directly before building the ToolResult so the function
remains correct and side-effect free.
In `@internal/sandbox/engine.go`:
- Around line 107-112: The Grant method lacks a context parameter—update
Engine.Grant to accept a context.Context (e.g., func (engine *Engine) Grant(ctx
context.Context, input GrantInput) (Grant, error)), update any call sites, and
propagate that context into the underlying store call by changing the store
interface method Grant to accept a context (GrantStore.Grant(ctx
context.Context, input GrantInput) (Grant, error)) and calling
engine.store.Grant(ctx, input); also mirror the pattern used by Evaluate and the
OnPermissionRequest callback (types.go:94) so cancellations/timeouts are honored
end-to-end.
In `@internal/tui/model.go`:
- Around line 524-552: The permission request is not being appended to
sessionEvents when a custom handler (onPermissionRequest) is present; change the
flow in the OnPermissionRequest wrapper so the pendingSessionEvent for
sessions.EventPermissionRequest is recorded unconditionally before delegating to
onPermissionRequest or performing the default UI flow. Specifically, ensure the
sessionEvents append of pendingSessionEvent{Type:
sessions.EventPermissionRequest, Payload: request} occurs before the
onPermissionRequest != nil check (or duplicate the append into both branches)
and keep existing references to runID, m.sendPermissionRequest, decisionCh, and
the permission decision handling intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5988602b-0165-4cd9-b9d9-699c1d62e196
📒 Files selected for processing (18)
docs/STREAM_JSON_PROTOCOL.mdinternal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_writer.gointernal/sandbox/engine.gointernal/sessions/replay.gointernal/sessions/store.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/rendering.gointernal/tui/session.gointernal/tui/session_test.gointernal/tui/transcript.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
-
Clean separation of
PermissionRequestvsPermissionEventvsPermissionDecision. The request is the prompt context (args, risk, violation, side effect); the event is the post-decision record (action, decision reason, persisted grant); the decision is the responder's reply (action + reason). Three distinct types with three distinct roles — exactly what the protocol and the session store need. -
OnPermissionRequestis a well-typed callback seam. Signaturefunc(context.Context, PermissionRequest) (PermissionDecision, error)is right: context for cancellation, request for context, decision for the result, error for responder failure. The TUI returns synchronously, the headlessexecreturns a deny-by-default, tests inject a fake. Clean. -
Synchronous TUI decision via buffered channel. The TUI's
OnPermissionRequestwrapper creates a size-1 channel, sends apermissionRequestMsgto the runtime sink, and blocks on the channel until either a decision arrives or the context is canceled. The buffer size of 1 prevents the channel send from blocking if the agent runtime has given up.case <-ctx.Done():returns a deny with the cancellation error. The pattern is correct and leak-free (the wrapper owns the channel lifetime). -
Default deny when sink is unavailable.
if m.runtimeMessageSink == nil { return ...deny... }ensures the agent runtime never blocks forever. The TUI always has a sink in practice, but the safety net is real. -
pendingPermissionis cleared on bothcancelRunandagentResponseMsg. No prompt state leaks across runs. -
KeyEnteris blocked when pending, and all other keys route throughhandlePermissionKey. The prompt is modal — onlya/d/y(case-insensitive) are valid; everything else is a no-op. TheTestPermissionPromptBlocksNormalSubmittest locks the Enter-blocking behavior. -
Dedup key update allows both request and decision rows for the same
ToolCallID. Old key waskind:ToolCallID; new iskind:ToolCallID:Action. Without this, the decision row would be silently dropped. The change is correct because a prompt and an allow are different events. -
Stream JSON protocol gets explicit event types.
permission_requestandpermission_decisionare distinct from the genericpermission(which is preserved for backward compatibility). Protocol clients can branch on the type. The docs example shows both new event shapes withpermission: "prompt"andaction: "allow". -
Headless behavior is documented. The protocol doc explicitly states "Headless
exechas no interactive permission responder. If a prompt-gated tool is not pre-approved, Zero may emitpermission_requestfollowed by a deniedtool_result". Critical for automation consumers who would otherwise think the run is hung. -
Session events mirror the protocol.
EventPermissionRequestandEventPermissionDecisionjoin the existingEventPermission. The TUI'stuiPermissionEventTypeand the CLI'ssessionPermissionEventTypeapply the same Action→EventType mapping.transcriptRowsFromSessionEventshydrate handles all three types. -
shouldRequestPermissioncorrectly gates the prompt. No request is made if the tool is notPermissionPrompt, or already granted (unsafe mode / grant match), or the sandbox preflight already decided (allow/deny). Only when the sandbox is silent AND the tool is a prompt tool AND the user hasn't granted it does the TUI get asked. -
normalizePermissionDecisionActiondefaults unknown actions todeny. Safe default; the three valid actions are explicitly allowed. -
deniedPermissionResultreturns aStatusErrortool result with a clear message. The LLM sees "Error: Permission denied for write_file: " on the next turn and can adapt. -
OnPermissionis called withDecisionReasonpopulated on the final event. The TUI'srenderPermissionRowshows the reason ("denied in TUI" / "approved in TUI" / "persistently approved in TUI"). Good. -
persistPermissionGrantdefaultsMaxAutonomytoMediumifoptions.Autonomy == "". Reasonable default. The grant is bound to the current autonomy ceiling, which is correct semantics. -
Engine.Grant(input)is a thin wrapper overstore.Grant(input). Keeps the engine as the only entry point for grant operations. Tested transitively byTestRunPersistsAlwaysAllowPermissionDecision. -
TestPermissionPromptAlwaysPersistsGrantAndSkipsLaterPromptis the strongest test. Full flow: prompt →y→ grant persisted → tool runs → second turn → grant matches → no prompt → tool runs. The session events assert only the first run requested permission (EventPermissionRequestcount = 1) but both recorded decisions (EventPermissionDecisioncount = 2). This is the user's mental model of "always allow" and the test locks the contract. -
receiveRuntimeMessage/receiveFinalMessagetest helpers usetime.After(5 * time.Second). Prevents test deadlock on broken wiring. Defensive test pattern. -
Callback chains are preserved.
OnPermissionRequestandOnPermissionboth wrap the previous option callback (if onPermissionRequest != nil { onPermissionRequest(...) }/if onPermission != nil { onPermission(event) }), so future test seams can observe the same events without being silently swallowed. -
TUI prompt rendering uses theme colors. Amber for "permission required", muted for details, text for choice labels.
renderFocusedPermissionPromptreuses the existingborderedBlockhelper.
Observations (non-blocking)
-
Duplicated Action→EventType mapping in three places.
sessionPermissionEventType(CLI exec),tuiPermissionEventType(TUI), andstreamJSONPermissionEventType(CLI stream JSON writer) all do the same mapping. A shared helper inagent(e.g.,agent.EventTypeForPermissionAction(action)) and a parallel one for stream JSON would deduplicate. Not blocking. -
Headless
exechas no--non-interactive-permissions=allow|denyflag. The TUI's "always allow" path is unavailable in headless mode — prompt tools just get denied. Automation users would benefit from a CLI flag that pre-approves prompt tools (with the same grant persistence) or denies them explicitly. A follow-up slice, not this one. -
renderFocusedPermissionPromptdoes not show the tool call args. The TUI shows the reason and side effect, but not the path/content forwrite_fileor the command forbash.request.Argsis now available (added in this PR) but not rendered. A future TUI improvement could surface a detail block with the args for triage. -
renderFocusedPermissionPromptis shown only whenm.pendingis true. Defensive invariant:pendingPermission != nil => pending == true. The current code maintains this (the wrapper sets both), but a future refactor that togglespendingwithout clearingpendingPermissioncould break it. -
Esc/Ctrl+Ccancel the prompt silently. The TUI's existing cancel button works (clearspendingPermission), but the user doesn't see a confirmation that the decision was dropped. A small "permission request canceled" line in the transcript would help trust. -
cloneArgsis a shallow copy. A nested map or slice in the args would be shared. For the current tool set (read_file, write_file, bash) the args are flat JSON values, so shallow copy is fine. A future tool with nested objects would need deep copy. -
deniedPermissionResultandemitDeniedPermissionshare the "set action to deny, set permissionGranted to false, set decision reason" pattern. They could share a helper. Minor. -
PermissionRequest.GrantMatchedandGrantfields are present but unused in the current request path. The preflight decision either grants (skipping the request) or doesn't (request is built without grant context). The fields are there for completeness — a doc comment on the field would explain. -
No test for
Engine.Grantdirectly. Covered transitively byTestRunPersistsAlwaysAllowPermissionDecision. A focused unit test forEngine.Grantwould assert the nil-engine and nil-store error paths. -
No test for
OnPermissionRequestreturning an error. The loop converts an error into a deny, but no test exercises that path. A test that injects a callback returning(PermissionDecision{}, errors.New("test"))and asserts the deny is emitted would lock the safety net. -
Protocol naming is slightly asymmetric. The
permission_requestexample showspermission: "prompt"(the type) and thepermission_decisionexample showsaction: "allow"(the action). One field is the type, the other is the action. A doc note explaining the asymmetry would help consumers parsing the JSON. -
TestPermissionPromptBlocksNormalSubmitcould also assert the input is preserved.m.inputstill contains "second prompt" after the blocked Enter — confirming the user's typed text isn't lost. Minor tightening. -
m.cancelRunclearspendingPermissionwithout calling thedecidecallback. The pending decision is silently dropped. The agent runtime sees a context cancellation via the channel'sctx.Done()case. Consistent and correct, but worth a doc comment oncancelRunto explain the contract. -
No benchmark for the channel-based decision flow. A microbenchmark would confirm the channel is fast enough for typical use. Not a concern in practice.
-
OnPermissionRequestblocks the agent loop. That's the design, but a slow responder would block the entire agent run. The TUI's channel pattern is correct; thectx.Done()case handles cancellation. Worth noting for future async-responder work.
Approving — the slice is well-architected. The new types are clean, the callback seam is well-typed, the synchronous TUI decision via channel is correct, the dedup key update allows both request and decision rows, the stream JSON protocol gets explicit event types, the session events mirror the protocol, and the headless behavior is documented. The tests cover the full flow including the "always allow" path with grant persistence. The follow-ups are minor (duplicated event-type helpers, args display in TUI prompt, missing --non-interactive-permissions flag, error path tests).
2 participants
Summary
permission_requestandpermission_decisionTests
Notes
Summary by CodeRabbit
New Features
permission_requestandpermission_decisionevents to the protocol.Documentation