diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 2e27bd3b9..25b7d522d 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,48 @@ 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) + + // Verify the gap flagged in PR review: even if ToolsApproved is true (yolo flag), + // the inherited Deny should correctly override the yolo flag 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() @@ -364,7 +410,8 @@ func TestRunAgent_InheritsParentPermissions(t *testing.T) { agent.WithSubAgents(worker)(root) tm := team.New(team.WithAgents(root, worker)) - rt, err := NewLocalRuntime(t.Context(), tm, + rt, err := NewLocalRuntime( + t.Context(), tm, WithSessionCompaction(false), WithModelStore(mockModelStore{}), ) @@ -409,6 +456,60 @@ 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), + ) + + rt.RunAgent(t.Context(), agenttool.RunParams{ + AgentName: "worker", + Task: "do something", + ParentSession: parentSession, + }) + + 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() @@ -420,7 +521,8 @@ func TestTransferTask_PropagatesPermissions(t *testing.T) { agent.WithSubAgents(librarian)(root) tm := team.New(team.WithAgents(root, librarian)) - rt, err := NewLocalRuntime(t.Context(), tm, + rt, err := NewLocalRuntime( + t.Context(), tm, WithSessionCompaction(false), WithModelStore(mockModelStore{}), ) diff --git a/pkg/runtime/runtime_test.go b/pkg/runtime/runtime_test.go index 858b556b3..130c3ed41 100644 --- a/pkg/runtime/runtime_test.go +++ b/pkg/runtime/runtime_test.go @@ -2517,7 +2517,8 @@ 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 @@ -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 require.True(t, sess.ToolsApproved) calls := []tools.ToolCall{{ @@ -2609,11 +2612,13 @@ 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 @@ -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..4ad1c521e 100644 --- a/pkg/runtime/toolexec/permissions_test.go +++ b/pkg/runtime/toolexec/permissions_test.go @@ -18,13 +18,13 @@ 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: OutcomeAllow, Reason: ReasonYolo}, d) + assert.Equal(t, PermissionDecision{Outcome: OutcomeDeny, Reason: ReasonChecker, Source: "team"}, d) } func TestDecide_DenyFromCheckerWins(t *testing.T) { 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,