diff --git a/docs/configuration/hooks/index.md b/docs/configuration/hooks/index.md index 4ec0c8c6f..05454ec94 100644 --- a/docs/configuration/hooks/index.md +++ b/docs/configuration/hooks/index.md @@ -57,7 +57,7 @@ docker-agent dispatches the following hook events: | `on_max_iterations` | When the runtime reaches its configured `max_iterations` limit | No | | `on_agent_switch` | When the runtime moves the active agent (transfer_task, handoff, return) | No | | `on_session_resume` | When the user explicitly approves continuation past `max_iterations` | No | -| `on_tool_approval_decision` | After the runtime's approval chain (yolo / permissions / readonly / ask) resolves | No | +| `on_tool_approval_decision` | After the runtime's approval chain (permissions / yolo / readonly / ask) resolves | No | | `worktree_create` | After `docker agent run --worktree` creates a git worktree, before the session | Yes | > [!NOTE] @@ -713,7 +713,7 @@ At every transfer the runtime ships a snapshot of the previous agent's model end ### Tool-Approval-Decision: who-approved-what audit trail -`on_tool_approval_decision` fires after the runtime's tool-approval chain (yolo / permissions / readonly / pre_tool_use hooks / interactive prompt) has resolved a verdict for a tool call. `approval_decision` is `allow`, `deny`, or `canceled`; `approval_source` is a stable classifier of which step produced the verdict. Observational only — it gives audit pipelines a single, structured "who approved what" record without re-implementing the chain. +`on_tool_approval_decision` fires after the runtime's tool-approval chain (permissions / yolo / readonly / pre_tool_use hooks / interactive prompt) has resolved a verdict for a tool call. `approval_decision` is `allow`, `deny`, or `canceled`; `approval_source` is a stable classifier of which step produced the verdict. Observational only — it gives audit pipelines a single, structured "who approved what" record without re-implementing the chain. ### Worktree-Create: prepare an isolated checkout diff --git a/docs/features/cli/index.md b/docs/features/cli/index.md index e3052e047..307231d84 100644 --- a/docs/features/cli/index.md +++ b/docs/features/cli/index.md @@ -28,7 +28,7 @@ $ docker agent run [config] [message...] [flags] | Flag | Description | | --------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------- | | `-a, --agent ` | Run a specific agent from the config | -| `--yolo` | Auto-approve all tool calls | +| `--yolo` | Auto-approve tool calls (unless explicitly denied) | | `--model ` | Override model(s). Use `provider/model` for all agents, or `agent=provider/model` for specific agents. Comma-separate multiple overrides. | | `--session ` | Resume a previous session. Supports relative refs (`-1` = last, `-2` = second to last). An explicit ID that does not exist yet is created with that ID, so a supervisor can own the session ID upfront and reuse it across runs. | | `-s, --session-db ` | Path to the SQLite session database (default: `/session.db`, so `~/.cagent/session.db` unless `--data-dir` is set) | @@ -479,7 +479,7 @@ $ docker agent run yolo-coder **Alias Options:** Aliases can include runtime options that apply automatically when used: -- `--yolo` — Auto-approve all tool calls when running the alias +- `--yolo` — Auto-approve tool calls (unless explicitly denied) when running the alias - `--model ` — Override the model for the alias - `--hide-tool-results` — Hide tool call results in the TUI when running the alias - `--sandbox` — Always run the alias inside a [Docker sandbox](../../configuration/sandbox/index.md) diff --git a/pkg/runtime/agent_delegation.go b/pkg/runtime/agent_delegation.go index 4de78d9e0..1ce322a94 100644 --- a/pkg/runtime/agent_delegation.go +++ b/pkg/runtime/agent_delegation.go @@ -322,12 +322,6 @@ func (r *LocalRuntime) runForwarding(ctx context.Context, parent *session.Sessio return nil, subSessionErr } - // Only propagate ToolsApproved and Permissions on success. A failed sub-session - // must not silently escalate the parent's tool-approval gate: the user approved - // tools within a sub-session scope that ended in error, and that approval - // should not carry over to the parent's remaining turns. - parent.SetToolsApproved(s.IsToolsApproved()) - parent.SetPermissions(s.ClonePermissions()) span.SetStatus(codes.Ok, "sub-session completed") return tools.ResultSuccess(s.GetLastAssistantMessageContent()), nil } diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 2e27bd3b9..00c197b3f 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -1,6 +1,7 @@ package runtime import ( + "context" "strings" "testing" @@ -8,6 +9,9 @@ import ( "github.com/stretchr/testify/require" "github.com/docker/docker-agent/pkg/agent" + "github.com/docker/docker-agent/pkg/config/latest" + "github.com/docker/docker-agent/pkg/permissions" + "github.com/docker/docker-agent/pkg/runtime/toolexec" "github.com/docker/docker-agent/pkg/session" "github.com/docker/docker-agent/pkg/team" "github.com/docker/docker-agent/pkg/tools" @@ -270,6 +274,47 @@ func TestSubSessionWithoutAttachedFilesOmitsBlock(t *testing.T) { } } +func TestSubSessionInheritsPermissions(t *testing.T) { + t.Parallel() + + perms := &session.PermissionsConfig{ + Allow: []string{"read_*"}, + Deny: []string{"write_*"}, + Ask: []string{"edit_*"}, + } + parent := session.New(session.WithPermissions(perms)) + + childAgent := agent.New("worker", "") + cfg := SubSessionConfig{ + Task: "refactor", + AgentName: "worker", + Title: "Refactor", + Permissions: parent.ClonePermissions(), + } + + s := newSubSession(parent, cfg, childAgent) + + require.NotNil(t, s.Permissions) + assert.Equal(t, perms.Allow, s.Permissions.Allow) + assert.Equal(t, perms.Deny, s.Permissions.Deny) + assert.Equal(t, perms.Ask, s.Permissions.Ask) + + // Even with ToolsApproved set (yolo), an inherited Deny must win during dispatch. + s.ToolsApproved = true + + checker := permissions.NewChecker(&latest.PermissionsConfig{ + Allow: s.Permissions.Allow, + Ask: s.Permissions.Ask, + Deny: s.Permissions.Deny, + }) + namedCheckers := []toolexec.NamedChecker{ + {Checker: checker, Source: "session permissions"}, + } + + decision := toolexec.Decide(s.ToolsApproved, namedCheckers, "write_file", map[string]any{"path": "foo"}, false) + assert.Equal(t, toolexec.OutcomeDeny, decision.Outcome, "Inherited Deny should override ToolsApproved: true (yolo)") +} + func TestNewSubSession_PermissionsIsolation(t *testing.T) { t.Parallel() @@ -409,6 +454,70 @@ func TestRunAgent_InheritsParentPermissions(t *testing.T) { "parent permissions must be isolated from child mutations") } +func TestRunAgent_EndToEndPermissions(t *testing.T) { + t.Parallel() + + var executed bool + agentTools := []tools.Tool{{ + Name: "dangerous_tool", + Parameters: map[string]any{}, + Handler: func(_ context.Context, _ tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + executed = true + return tools.ResultSuccess("executed"), nil + }, + }} + + workerStream := newStreamBuilder(). + AddToolCallName("call_1", "dangerous_tool"). + AddToolCallArguments("call_1", "{}"). + Build() + parentProv := &mockProvider{id: "test/mock-model", stream: &mockStream{}} + workerProv := &mockProvider{id: "test/mock-model", stream: workerStream} + + worker := agent.New("worker", "Worker agent", + agent.WithModel(workerProv), + agent.WithToolSets(newStubToolSet(nil, agentTools, nil)), + ) + root := agent.New("root", "Root agent", agent.WithModel(parentProv)) + agent.WithSubAgents(worker)(root) + + tm := team.New(team.WithAgents(root, worker)) + rt, err := NewLocalRuntime( + t.Context(), tm, + WithSessionCompaction(false), + WithModelStore(mockModelStore{}), + ) + require.NoError(t, err) + + parentPerms := &session.PermissionsConfig{ + Allow: []string{"safe_tool"}, + Deny: []string{"dangerous_tool"}, + } + parentSession := session.New( + session.WithUserMessage("Test"), + session.WithToolsApproved(true), + session.WithPermissions(parentPerms), + ) + + result := rt.RunAgent(t.Context(), agenttool.RunParams{ + AgentName: "worker", + Task: "do something", + ParentSession: parentSession, + }) + require.Empty(t, result.ErrMsg, "RunAgent should succeed") + + var childSession *session.Session + for _, item := range parentSession.Messages { + if item.SubSession != nil { + childSession = item.SubSession + break + } + } + require.NotNil(t, childSession, "parent must have a sub-session") + + require.False(t, executed, "expected dangerous_tool to NOT be executed because it is denied by inherited permissions") +} + func TestTransferTask_PropagatesPermissions(t *testing.T) { t.Parallel() diff --git a/pkg/runtime/runtime_test.go b/pkg/runtime/runtime_test.go index 858b556b3..bd9cfa39b 100644 --- a/pkg/runtime/runtime_test.go +++ b/pkg/runtime/runtime_test.go @@ -2517,10 +2517,11 @@ func TestTransferTaskPersistsSubSessionOnError(t *testing.T) { "SubSessionCompletedEvent must fire on the error path so observers persist the sub-session") } -func TestYoloMode_OverridesPermissionsDeny(t *testing.T) { +// TestDenyOverridesYoloMode verifies that Deny permissions take precedence over the yolo flag. +func TestDenyOverridesYoloMode(t *testing.T) { t.Parallel() - // Test that --yolo flag takes precedence over deny permissions + // Test that deny permissions take precedence over the --yolo flag permChecker := permissions.NewChecker(&latest.PermissionsConfig{ Deny: []string{"dangerous_tool"}, }) @@ -2561,10 +2562,11 @@ func TestYoloMode_OverridesPermissionsDeny(t *testing.T) { rt.processToolCalls(t.Context(), sess, calls, agentTools, NewChannelSink(events)) close(events) - // With --yolo, the tool should execute despite deny permission - require.True(t, executed, "expected tool to be executed in --yolo mode despite deny permission") + // With --yolo and Deny/ForceAsk precedence, the tool should NOT execute. + require.False(t, executed, "expected tool to NOT be executed in --yolo mode because Deny wins") } +// TestYoloMode_OverridesForceAsk verifies that the yolo flag takes precedence over ForceAsk permissions. func TestYoloMode_OverridesForceAsk(t *testing.T) { t.Parallel() @@ -2597,6 +2599,7 @@ func TestYoloMode_OverridesForceAsk(t *testing.T) { require.NoError(t, err) sess := session.New(session.WithUserMessage("Test"), session.WithToolsApproved(true)) + sess.NonInteractive = true // fail fast instead of hanging on askUser if this regresses require.True(t, sess.ToolsApproved) calls := []tools.ToolCall{{ @@ -2609,14 +2612,16 @@ func TestYoloMode_OverridesForceAsk(t *testing.T) { rt.processToolCalls(t.Context(), sess, calls, agentTools, NewChannelSink(events)) close(events) - // With --yolo, the tool should execute without asking - require.True(t, executed, "expected tool to be executed in --yolo mode despite ForceAsk permission") + // YOLO overrides ForceAsk: the checker's ForceAsk verdict is bypassed + // and the tool executes automatically. + require.True(t, executed, "expected tool to be executed in --yolo mode because YOLO wins over ForceAsk") } -func TestYoloMode_OverridesSessionDeny(t *testing.T) { +// TestSessionDenyOverridesYoloMode verifies that session-level Deny permissions take precedence over the yolo flag. +func TestSessionDenyOverridesYoloMode(t *testing.T) { t.Parallel() - // Test that --yolo flag takes precedence over session-level deny + // Test that session-level deny takes precedence over the --yolo flag var executed bool agentTools := []tools.Tool{{ Name: "blocked_tool", @@ -2656,8 +2661,8 @@ func TestYoloMode_OverridesSessionDeny(t *testing.T) { rt.processToolCalls(t.Context(), sess, calls, agentTools, NewChannelSink(events)) close(events) - // With --yolo, the tool should execute despite session deny - require.True(t, executed, "expected tool to be executed in --yolo mode despite session deny permission") + // With --yolo and Deny/ForceAsk precedence, the tool should NOT execute. + require.False(t, executed, "expected tool to NOT be executed in --yolo mode because session Deny wins") } func TestStripImageContent(t *testing.T) { diff --git a/pkg/runtime/toolexec/permissions.go b/pkg/runtime/toolexec/permissions.go index e872bfd41..dbf8fdda4 100644 --- a/pkg/runtime/toolexec/permissions.go +++ b/pkg/runtime/toolexec/permissions.go @@ -58,11 +58,11 @@ type PermissionDecision struct { // Decide resolves the final permission outcome for a tool call by walking // the configured pipeline in priority order: // -// 1. yoloApproved (--yolo) — auto-allow everything. -// 2. checkers (in order; typically session-level first, then team-level) +// 1. checkers (in order; typically session-level first, then team-level) // — the first checker that returns Allow / Deny / ForceAsk wins. -// ForceAsk produces [OutcomeAsk]: an explicit ask pattern always -// overrides the read-only fast path below. +// However, if the outcome is ForceAsk and yoloApproved is true, +// YOLO overrides ForceAsk and auto-allows the call. +// 2. yoloApproved (--yolo) — auto-allow everything else. // 3. readOnlyHint — auto-allow. // 4. default — Ask. // @@ -75,10 +75,6 @@ func Decide( toolArgs map[string]any, readOnlyHint bool, ) PermissionDecision { - if yoloApproved { - return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo} - } - for _, pc := range checkers { switch pc.Checker.CheckWithArgs(toolName, toolArgs) { case permissions.Deny: @@ -86,12 +82,19 @@ func Decide( case permissions.Allow: return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonChecker, Source: pc.Source} case permissions.ForceAsk: + if yoloApproved { + return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo} + } return PermissionDecision{Outcome: OutcomeAsk, Reason: ReasonChecker, Source: pc.Source} case permissions.Ask: // No explicit match at this level; fall through to next checker. } } + if yoloApproved { + return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo} + } + if readOnlyHint { return PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonReadOnlyHint} } diff --git a/pkg/runtime/toolexec/permissions_test.go b/pkg/runtime/toolexec/permissions_test.go index 16bf6c769..1803f95f0 100644 --- a/pkg/runtime/toolexec/permissions_test.go +++ b/pkg/runtime/toolexec/permissions_test.go @@ -18,12 +18,26 @@ func newChecker(t *testing.T, allow, ask, deny []string) *permissions.Checker { }) } -func TestDecide_YoloShortCircuits(t *testing.T) { +func TestDecide_DenyOverridesYolo(t *testing.T) { t.Parallel() d := Decide(true, []NamedChecker{ {Checker: newChecker(t, nil, nil, []string{"shell"}), Source: "team"}, }, "shell", nil, false) + assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "team"}, d) +} + +func TestDecide_YoloAllowsWhenNoCheckerMatches(t *testing.T) { + t.Parallel() + d := Decide(true, nil, "shell", nil, false) + assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d) +} + +func TestDecide_YoloOverridesForceAsk(t *testing.T) { + t.Parallel() + d := Decide(true, []NamedChecker{ + {Checker: newChecker(t, nil, []string{"shell"}, nil), Source: "team"}, + }, "shell", nil, false) assert.Equal(t, PermissionDecision{Outcome: OutcomeAllow, Reason: ReasonYolo}, d) } diff --git a/pkg/session/branch.go b/pkg/session/branch.go index 0113d9e2c..bd1d13a23 100644 --- a/pkg/session/branch.go +++ b/pkg/session/branch.go @@ -91,7 +91,7 @@ func (s *Session) Clone() *Session { InputTokens: s.InputTokens, OutputTokens: s.OutputTokens, Cost: s.Cost, - Permissions: clonePermissionsConfig(s.Permissions), + Permissions: s.Permissions.Clone(), AgentModelOverrides: cloneStringMap(s.AgentModelOverrides), CustomModelsUsed: cloneStringSlice(s.CustomModelsUsed), AttachedFiles: cloneStringSlice(s.AttachedFiles), @@ -190,7 +190,7 @@ func copySessionMetadata(dst, src *Session, title string) { dst.MaxConsecutiveToolCalls = src.MaxConsecutiveToolCalls dst.MaxOldToolCallTokens = src.MaxOldToolCallTokens dst.Starred = src.Starred - dst.Permissions = clonePermissionsConfig(src.Permissions) + dst.Permissions = src.Permissions.Clone() dst.AgentModelOverrides = cloneStringMap(src.AgentModelOverrides) dst.CustomModelsUsed = cloneStringSlice(src.CustomModelsUsed) dst.AttachedFiles = src.AttachedFilesSnapshot() @@ -319,17 +319,6 @@ func cloneEvalResultChecks(src EvalResultChecks) EvalResultChecks { return cp } -func clonePermissionsConfig(src *PermissionsConfig) *PermissionsConfig { - if src == nil { - return nil - } - return &PermissionsConfig{ - Allow: cloneStringSlice(src.Allow), - Ask: cloneStringSlice(src.Ask), - Deny: cloneStringSlice(src.Deny), - } -} - func cloneStringMap(src map[string]string) map[string]string { if len(src) == 0 { return nil diff --git a/pkg/session/session.go b/pkg/session/session.go index 7aeeaef70..78c592eb0 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -270,6 +270,18 @@ type PermissionsConfig struct { Deny []string `json:"deny,omitempty"` } +// Clone returns a deep copy of the permissions configuration. +func (c *PermissionsConfig) Clone() *PermissionsConfig { + if c == nil { + return nil + } + return &PermissionsConfig{ + Allow: slices.Clone(c.Allow), + Ask: slices.Clone(c.Ask), + Deny: slices.Clone(c.Deny), + } +} + // Message is a message from an agent type Message struct { // ID is the database ID of the message (used for persistence tracking) @@ -888,7 +900,7 @@ func WithSendUserMessage(sendUserMessage bool) Opt { func WithPermissions(perms *PermissionsConfig) Opt { return func(s *Session) { - s.Permissions = clonePermissionsConfig(perms) + s.Permissions = perms.Clone() } } @@ -1046,7 +1058,7 @@ func (s *Session) IsToolsApproved() bool { func (s *Session) ClonePermissions() *PermissionsConfig { s.mu.RLock() defer s.mu.RUnlock() - return clonePermissionsConfig(s.Permissions) + return s.Permissions.Clone() } // SetPermissions safely updates the session's PermissionsConfig. diff --git a/pkg/session/store.go b/pkg/session/store.go index 8b9d70f0e..8cb7a6963 100644 --- a/pkg/session/store.go +++ b/pkg/session/store.go @@ -232,7 +232,7 @@ func (s *InMemorySessionStore) UpdateSession(_ context.Context, session *Session InputTokens: session.InputTokens, OutputTokens: session.OutputTokens, Cost: session.Cost, - Permissions: clonePermissionsConfig(session.Permissions), + Permissions: session.Permissions.Clone(), AgentModelOverrides: cloneStringMap(session.AgentModelOverrides), CustomModelsUsed: cloneStringSlice(session.CustomModelsUsed), AttachedFiles: slices.Clone(session.AttachedFiles), @@ -954,7 +954,7 @@ func (s *SQLiteSessionStore) UpdateSession(ctx context.Context, session *Session InputTokens: session.InputTokens, OutputTokens: session.OutputTokens, Cost: session.Cost, - Permissions: clonePermissionsConfig(session.Permissions), + Permissions: session.Permissions.Clone(), AgentModelOverrides: cloneStringMap(session.AgentModelOverrides), CustomModelsUsed: cloneStringSlice(session.CustomModelsUsed), ParentID: session.ParentID,