From 8f210803ce780845831aace80e2b0ca5036fef90 Mon Sep 17 00:00:00 2001 From: piyush0049 Date: Mon, 6 Jul 2026 23:28:47 +0530 Subject: [PATCH 1/5] fix(runtime): propagate session permissions to background agents and skills This removes the hardcoded ToolsApproved bypass and ensures background tasks inherit the Allow/Deny/Ask permission configuration from their parent session. When tools fall back to 'Ask' in a background non-interactive context, the dispatcher correctly auto-denies them. --- pkg/runtime/agent_delegation.go | 23 ++++++++++++----------- pkg/runtime/skill_runner.go | 1 + 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/runtime/agent_delegation.go b/pkg/runtime/agent_delegation.go index bcb1cab425..85437d38cb 100644 --- a/pkg/runtime/agent_delegation.go +++ b/pkg/runtime/agent_delegation.go @@ -93,6 +93,8 @@ type SubSessionConfig struct { Title string // ToolsApproved overrides whether tools are pre-approved in the child session. ToolsApproved bool + // Permissions defines session-level tool permission overrides. + Permissions *session.PermissionsConfig // NonInteractive marks the child session as running without a user present // (e.g. MCP server, A2A adapter, background agent). This causes the runtime // to auto-stop on max iterations instead of blocking for user input. @@ -184,6 +186,9 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag if cfg.PinAgent { opts = append(opts, session.WithAgentName(cfg.AgentName)) } + if cfg.Permissions != nil { + opts = append(opts, session.WithPermissions(cfg.Permissions)) + } // Merge parent's excluded tools with config's excluded tools so that // nested sub-sessions (e.g. skill → transfer_task → child) inherit // exclusions from all ancestors and don't re-introduce filtered tools. @@ -476,23 +481,18 @@ func (r *LocalRuntime) CurrentAgentSubAgentNames() []string { // RunAgent implements agenttool.Runner. It starts a sub-agent synchronously // and blocks until completion or cancellation. // -// Background tasks run with tools pre-approved because there is no user -// present to respond to interactive approval prompts during async -// execution. This is a deliberate design trade-off: the user implicitly -// authorises all tool calls made by the sub-agent when they approve -// run_background_agent. Callers should be aware that prompt injection in -// the sub-agent's context could exploit this gate-bypass. -// -// TODO: propagate the parent session's per-tool permission rules once the -// runtime supports per-session permission scoping rather than a single -// shared ToolsApproved flag. +// Background tasks inherit the parent session's permissions because there is no user +// present to respond to interactive approval prompts during async execution. +// Tool calls that result in an "Ask" outcome will be auto-denied by the dispatcher +// due to the non-interactive context. func (r *LocalRuntime) RunAgent(ctx context.Context, params agenttool.RunParams) *agenttool.RunResult { return r.runCollecting(ctx, params.ParentSession, SubSessionConfig{ Task: params.Task, ExpectedOutput: params.ExpectedOutput, AgentName: params.AgentName, Title: "Background agent task", - ToolsApproved: true, + ToolsApproved: params.ParentSession.ToolsApproved, + Permissions: params.ParentSession.Permissions, NonInteractive: true, PinAgent: true, }, params.OnContent) @@ -552,6 +552,7 @@ func (r *LocalRuntime) handleTaskTransfer(ctx context.Context, sess *session.Ses AgentName: params.Agent, Title: "Transferred task", ToolsApproved: sess.ToolsApproved, + Permissions: sess.Permissions, NonInteractive: sess.NonInteractive, }, SwitchCurrentAgent: true, diff --git a/pkg/runtime/skill_runner.go b/pkg/runtime/skill_runner.go index 4152a04d3a..a8a9247afe 100644 --- a/pkg/runtime/skill_runner.go +++ b/pkg/runtime/skill_runner.go @@ -110,6 +110,7 @@ func (r *LocalRuntime) RunSkillFork(ctx context.Context, sess *session.Session, AgentName: ca, Title: "Skill: " + prepared.SkillName, ToolsApproved: sess.ToolsApproved, + Permissions: sess.Permissions, NonInteractive: sess.NonInteractive, ExcludedTools: []string{skills.ToolNameRunSkill}, AllowedTools: prepared.AllowedTools, From fd89580bfda42d07a093c0132c21a678b75c53da Mon Sep 17 00:00:00 2001 From: piyush0049 Date: Tue, 7 Jul 2026 19:58:50 +0530 Subject: [PATCH 2/5] fix: address PR feedback for sub-session permission isolation --- docs/tools/background-agents/index.md | 2 +- pkg/runtime/agent_delegation.go | 13 +- pkg/runtime/agent_delegation_test.go | 246 ++++++++++++++++++++++++- pkg/runtime/session_compaction_test.go | 9 + pkg/session/session.go | 25 ++- pkg/session/session_options_test.go | 56 +++--- pkg/session/store_test.go | 8 + 7 files changed, 322 insertions(+), 37 deletions(-) diff --git a/docs/tools/background-agents/index.md b/docs/tools/background-agents/index.md index ccdb66f2d1..4ea266d4cf 100644 --- a/docs/tools/background-agents/index.md +++ b/docs/tools/background-agents/index.md @@ -30,7 +30,7 @@ The background agents tool lets an orchestrator dispatch work to sub-agents conc | `task` | string | ✓ | Clear, concise description of the task the sub-agent should achieve. | | `expected_output` | string | ✗ | Optional description of the result format the caller expects. | -`run_background_agent` returns a **task ID** string. Tools run by the sub-agent are pre-approved, so only dispatch to trusted sub-agents with well-scoped tasks. +`run_background_agent` returns a **task ID** string. Tools run by the sub-agent inherit the parent session's permissions. Because background tasks run non-interactively, any tool call that would normally prompt the user for approval will be automatically denied. To allow background agents to run mutating tools, you must explicitly approve them in the parent session (e.g. via YOLO mode or explicit allow rules). ### `view_background_agent` and `stop_background_agent` parameters diff --git a/pkg/runtime/agent_delegation.go b/pkg/runtime/agent_delegation.go index 85437d38cb..4b9c526f30 100644 --- a/pkg/runtime/agent_delegation.go +++ b/pkg/runtime/agent_delegation.go @@ -186,9 +186,7 @@ func newSubSession(parent *session.Session, cfg SubSessionConfig, childAgent *ag if cfg.PinAgent { opts = append(opts, session.WithAgentName(cfg.AgentName)) } - if cfg.Permissions != nil { - opts = append(opts, session.WithPermissions(cfg.Permissions)) - } + opts = append(opts, session.WithPermissions(cfg.Permissions)) // Merge parent's excluded tools with config's excluded tools so that // nested sub-sessions (e.g. skill → transfer_task → child) inherit // exclusions from all ancestors and don't re-introduce filtered tools. @@ -324,11 +322,12 @@ func (r *LocalRuntime) runForwarding(ctx context.Context, parent *session.Sessio return nil, subSessionErr } - // Only propagate ToolsApproved on success. A failed sub-session must not - // silently escalate the parent's tool-approval gate: the user approved + // 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.ToolsApproved = s.ToolsApproved + parent.SetPermissions(s.ClonePermissions()) span.SetStatus(codes.Ok, "sub-session completed") return tools.ResultSuccess(s.GetLastAssistantMessageContent()), nil } @@ -491,8 +490,8 @@ func (r *LocalRuntime) RunAgent(ctx context.Context, params agenttool.RunParams) ExpectedOutput: params.ExpectedOutput, AgentName: params.AgentName, Title: "Background agent task", - ToolsApproved: params.ParentSession.ToolsApproved, - Permissions: params.ParentSession.Permissions, + ToolsApproved: params.ParentSession.IsToolsApproved(), + Permissions: params.ParentSession.ClonePermissions(), NonInteractive: true, PinAgent: true, }, params.OnContent) diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 6c1df4affe..1eae38b2e1 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -1,6 +1,8 @@ package runtime import ( + "os" + "path/filepath" "strings" "testing" @@ -9,6 +11,9 @@ import ( "github.com/docker/docker-agent/pkg/agent" "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/team" + agenttool "github.com/docker/docker-agent/pkg/tools/builtin/agent" + "github.com/docker/docker-agent/pkg/tools" ) func TestBuildTaskSystemMessage(t *testing.T) { @@ -29,9 +34,11 @@ func TestBuildTaskSystemMessage(t *testing.T) { }) t.Run("with attached files", func(t *testing.T) { - msg := buildTaskSystemMessage("do the thing", "", []string{"/abs/foo.go", "/abs/bar.go"}) + fooPath := filepath.Join(os.TempDir(), "foo.go") + barPath := filepath.Join(os.TempDir(), "bar.go") + msg := buildTaskSystemMessage("do the thing", "", []string{fooPath, barPath}) assert.Contains(t, msg, "\ndo the thing\n") - assert.Contains(t, msg, "\n- /abs/foo.go\n- /abs/bar.go\n") + assert.Contains(t, msg, "\n- "+fooPath+"\n- "+barPath+"\n") }) } @@ -219,9 +226,11 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { t.Parallel() parent := session.New(session.WithUserMessage("hello")) - parent.AddAttachedFile("/abs/foo.go") - parent.AddAttachedFile("/abs/bar.go") - parent.AddAttachedFile("/abs/foo.go") // duplicate, should be ignored + fooPath := filepath.Join(os.TempDir(), "foo.go") + barPath := filepath.Join(os.TempDir(), "bar.go") + parent.AddAttachedFile(fooPath) + parent.AddAttachedFile(barPath) + parent.AddAttachedFile(fooPath) // duplicate, should be ignored childAgent := agent.New("worker", "") cfg := SubSessionConfig{ @@ -233,7 +242,7 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { s := newSubSession(parent, cfg, childAgent) // Child session inherits parent's attached files (deduplicated, ordered). - assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) // The system message lists them so the sub-agent sees them up-front. sysMsg := s.GetMessages(childAgent) @@ -243,7 +252,7 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { joined.WriteString(m.Content) joined.WriteString("\n") } - assert.Contains(t, joined.String(), "\n- /abs/foo.go\n- /abs/bar.go\n") + assert.Contains(t, joined.String(), "\n- "+fooPath+"\n- "+barPath+"\n") } func TestSubSessionWithoutAttachedFilesOmitsBlock(t *testing.T) { @@ -266,3 +275,226 @@ func TestSubSessionWithoutAttachedFilesOmitsBlock(t *testing.T) { assert.NotContains(t, m.Content, "") } } + +func TestNewSubSession_PermissionsIsolation(t *testing.T) { + t.Parallel() + + parent := session.New(session.WithUserMessage("hello")) + childAgent := agent.New("worker", "") + + t.Run("cloned from config", func(t *testing.T) { + perms := &session.PermissionsConfig{ + Allow: []string{"read_file"}, + } + + cfg := SubSessionConfig{ + Task: "isolated work", + AgentName: "worker", + Title: "Task", + Permissions: perms, + } + + s := newSubSession(parent, cfg, childAgent) + + require.NotNil(t, s.Permissions) + assert.Equal(t, []string{"read_file"}, s.Permissions.Allow) + + // Mutate the original permissions config + perms.Allow = append(perms.Allow, "write_file") + + // The sub-session's permissions should remain isolated + assert.Equal(t, []string{"read_file"}, s.Permissions.Allow) + }) + + t.Run("nil permissions", func(t *testing.T) { + cfg := SubSessionConfig{ + Task: "work without permissions", + AgentName: "worker", + Title: "Task", + // Permissions is nil — the nil guard removal must not panic + } + + s := newSubSession(parent, cfg, childAgent) + assert.Nil(t, s.Permissions) + }) +} + +func TestSession_ClonePermissions(t *testing.T) { + t.Parallel() + + t.Run("returns deep copy", func(t *testing.T) { + perms := &session.PermissionsConfig{ + Allow: []string{"read_file"}, + Deny: []string{"write_file"}, + } + s := session.New(session.WithPermissions(perms)) + + cloned := s.ClonePermissions() + require.NotNil(t, cloned) + assert.Equal(t, perms.Allow, cloned.Allow) + assert.Equal(t, perms.Deny, cloned.Deny) + + // Mutating the clone should not affect the session + cloned.Allow = append(cloned.Allow, "exec_command") + original := s.ClonePermissions() + assert.Equal(t, []string{"read_file"}, original.Allow) + }) + + t.Run("returns nil when unset", func(t *testing.T) { + s := session.New() + assert.Nil(t, s.ClonePermissions()) + }) +} + +func TestSession_SetPermissions(t *testing.T) { + t.Parallel() + + s := session.New() + assert.Nil(t, s.ClonePermissions()) + + perms := &session.PermissionsConfig{ + Allow: []string{"read_file"}, + } + s.SetPermissions(perms) + + got := s.ClonePermissions() + require.NotNil(t, got) + assert.Equal(t, []string{"read_file"}, got.Allow) +} + +// TestRunAgent_InheritsParentPermissions verifies that RunAgent correctly +// inherits the parent session's ToolsApproved flag and Permissions via +// the thread-safe accessors, and that the child session is isolated from +// later mutations of the parent. +func TestRunAgent_InheritsParentPermissions(t *testing.T) { + t.Parallel() + + workerStream := newStreamBuilder().AddContent("done").AddStopWithUsage(10, 5).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)) + 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{"read_file", "list_dir"}, + Deny: []string{"shell:cmd=rm*"}, + } + 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") + + // The parent session must have a sub-session recorded. + 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") + + // The child must have inherited ToolsApproved. + assert.True(t, childSession.ToolsApproved, + "child session must inherit ToolsApproved from parent") + + // The child must have inherited permissions. + require.NotNil(t, childSession.Permissions) + assert.Equal(t, []string{"read_file", "list_dir"}, childSession.Permissions.Allow) + assert.Equal(t, []string{"shell:cmd=rm*"}, childSession.Permissions.Deny) + + // Mutating the child's permissions must NOT affect the parent (isolation). + childSession.Permissions.Allow = append(childSession.Permissions.Allow, "write_file") + parentClone := parentSession.ClonePermissions() + assert.Equal(t, []string{"read_file", "list_dir"}, parentClone.Allow, + "parent permissions must be isolated from child mutations") +} + +// TestTransferTask_PropagatesPermissions verifies that handleTaskTransfer +// passes the parent session's Permissions to the child sub-session, and +// that the child's permissions are isolated from the parent's. +func TestTransferTask_PropagatesPermissions(t *testing.T) { + t.Parallel() + + childStream := newStreamBuilder().AddContent("transferred").AddStopWithUsage(10, 5).Build() + prov := &mockProvider{id: "test/mock-model", stream: childStream} + + librarian := agent.New("librarian", "Library agent", agent.WithModel(prov)) + root := agent.New("root", "Root agent", agent.WithModel(prov)) + agent.WithSubAgents(librarian)(root) + + tm := team.New(team.WithAgents(root, librarian)) + 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"}, + } + sess := session.New( + session.WithUserMessage("Test"), + session.WithToolsApproved(true), + session.WithPermissions(parentPerms), + ) + evts := make(chan Event, 128) + + toolCall := tools.ToolCall{ + ID: "call_1", + Type: "function", + Function: tools.FunctionCall{ + Name: "transfer_task", + Arguments: `{"agent":"librarian","task":"find a book","expected_output":"book title"}`, + }, + } + + result, err := rt.handleTaskTransfer(t.Context(), sess, toolCall, NewChannelSink(evts)) + require.NoError(t, err) + require.NotNil(t, result) + assert.False(t, result.IsError, "transfer to valid sub-agent should succeed") + + // Find the sub-session that was created. + var childSession *session.Session + for _, item := range sess.Messages { + if item.SubSession != nil { + childSession = item.SubSession + break + } + } + require.NotNil(t, childSession, "parent must have a sub-session after transfer_task") + + // The child must have inherited the parent's permissions. + require.NotNil(t, childSession.Permissions) + assert.Equal(t, []string{"safe_tool"}, childSession.Permissions.Allow) + assert.Equal(t, []string{"dangerous_tool"}, childSession.Permissions.Deny) + + // The child must have inherited ToolsApproved. + assert.True(t, childSession.ToolsApproved, + "child session must inherit ToolsApproved from parent") + + // Mutating the child's permissions must NOT affect the parent (isolation). + childSession.Permissions.Allow = append(childSession.Permissions.Allow, "exploit") + parentClone := sess.ClonePermissions() + assert.Equal(t, []string{"safe_tool"}, parentClone.Allow, + "parent permissions must remain isolated from child mutations after transfer_task") +} + diff --git a/pkg/runtime/session_compaction_test.go b/pkg/runtime/session_compaction_test.go index cd631da4e8..5db33940da 100644 --- a/pkg/runtime/session_compaction_test.go +++ b/pkg/runtime/session_compaction_test.go @@ -2,6 +2,7 @@ package runtime import ( "os" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -108,6 +109,10 @@ func TestSessionGetMessages_SummaryWithoutFirstKeptEntry(t *testing.T) { func TestDoCompactBeforeHookDeniesSkipsCompaction(t *testing.T) { t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows: relies on Unix shell semantics") + } + denyingHooks := &latest.HooksConfig{ BeforeCompaction: []latest.HookDefinition{ {Type: "command", Command: "echo 'denied for safety' >&2; exit 2", Timeout: 5}, @@ -232,6 +237,10 @@ func TestDoCompactBeforeHookSuppliesSummary(t *testing.T) { func TestDoCompactAfterHookFires(t *testing.T) { t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows: relies on Unix shell semantics and jq") + } + dir := t.TempDir() logFile := dir + "/after.log" diff --git a/pkg/session/session.go b/pkg/session/session.go index 150fab456d..761d5d548e 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -888,7 +888,7 @@ func WithSendUserMessage(sendUserMessage bool) Opt { func WithPermissions(perms *PermissionsConfig) Opt { return func(s *Session) { - s.Permissions = perms + s.Permissions = clonePermissionsConfig(perms) } } @@ -1033,6 +1033,29 @@ func (s *Session) OwnCost() float64 { return cost } +// IsToolsApproved returns a consistent snapshot of the ToolsApproved flag. +// This is safe to call concurrently with session mutations. +func (s *Session) IsToolsApproved() bool { + s.mu.RLock() + defer s.mu.RUnlock() + return s.ToolsApproved +} + +// ClonePermissions returns a deep copy of the session's PermissionsConfig. +// This is safe to call concurrently with session mutations. +func (s *Session) ClonePermissions() *PermissionsConfig { + s.mu.RLock() + defer s.mu.RUnlock() + return clonePermissionsConfig(s.Permissions) +} + +// SetPermissions safely updates the session's PermissionsConfig. +func (s *Session) SetPermissions(perms *PermissionsConfig) { + s.mu.Lock() + defer s.mu.Unlock() + s.Permissions = perms +} + // now returns the session's current time, falling back to time.Now for // sessions created without a clock (e.g. JSON deserialization). func (s *Session) now() time.Time { diff --git a/pkg/session/session_options_test.go b/pkg/session/session_options_test.go index 3d9a65cd12..fb140660b2 100644 --- a/pkg/session/session_options_test.go +++ b/pkg/session/session_options_test.go @@ -1,6 +1,8 @@ package session import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -77,11 +79,13 @@ func TestAddAttachedFile(t *testing.T) { t.Parallel() t.Run("deduplicates and preserves order", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") + barPath := filepath.Join(os.TempDir(), "bar.go") s := New() - s.AddAttachedFile("/abs/foo.go") - s.AddAttachedFile("/abs/bar.go") - s.AddAttachedFile("/abs/foo.go") // duplicate - assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) + s.AddAttachedFile(fooPath) + s.AddAttachedFile(barPath) + s.AddAttachedFile(fooPath) // duplicate + assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) }) t.Run("ignores empty paths", func(t *testing.T) { @@ -102,11 +106,12 @@ func TestAddAttachedFile(t *testing.T) { t.Run("snapshot is independent of session storage", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - s.AddAttachedFile("/abs/foo.go") + s.AddAttachedFile(fooPath) snap := s.AttachedFilesSnapshot() snap[0] = "mutated" - assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) }) } @@ -114,43 +119,52 @@ func TestRemoveAttachedFile(t *testing.T) { t.Parallel() t.Run("removes and reports presence", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") + barPath := filepath.Join(os.TempDir(), "bar.go") + bazPath := filepath.Join(os.TempDir(), "baz.go") s := New() - s.AddAttachedFile("/abs/foo.go") - s.AddAttachedFile("/abs/bar.go") - s.AddAttachedFile("/abs/baz.go") + s.AddAttachedFile(fooPath) + s.AddAttachedFile(barPath) + s.AddAttachedFile(bazPath) - assert.True(t, s.RemoveAttachedFile("/abs/bar.go")) - assert.Equal(t, []string{"/abs/foo.go", "/abs/baz.go"}, s.AttachedFilesSnapshot()) + assert.True(t, s.RemoveAttachedFile(barPath)) + assert.Equal(t, []string{fooPath, bazPath}, s.AttachedFilesSnapshot()) }) t.Run("reports absent paths", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") + otherPath := filepath.Join(os.TempDir(), "other.go") s := New() - s.AddAttachedFile("/abs/foo.go") - assert.False(t, s.RemoveAttachedFile("/abs/other.go")) + s.AddAttachedFile(fooPath) + assert.False(t, s.RemoveAttachedFile(otherPath)) assert.False(t, s.RemoveAttachedFile("")) - assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) }) t.Run("no-op on empty list", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - assert.False(t, s.RemoveAttachedFile("/abs/foo.go")) + assert.False(t, s.RemoveAttachedFile(fooPath)) assert.Empty(t, s.AttachedFilesSnapshot()) }) t.Run("file can be re-attached after removal", func(t *testing.T) { t.Parallel() + fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - s.AddAttachedFile("/abs/foo.go") - require.True(t, s.RemoveAttachedFile("/abs/foo.go")) - s.AddAttachedFile("/abs/foo.go") - assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) + s.AddAttachedFile(fooPath) + require.True(t, s.RemoveAttachedFile(fooPath)) + s.AddAttachedFile(fooPath) + assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) }) } func TestWithAttachedFiles(t *testing.T) { t.Parallel() - s := New(WithAttachedFiles([]string{"/abs/foo.go", "", "relative/path.go", "/abs/bar.go", "/abs/foo.go"})) - assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) + fooPath := filepath.Join(os.TempDir(), "foo.go") + barPath := filepath.Join(os.TempDir(), "bar.go") + s := New(WithAttachedFiles([]string{fooPath, "", "relative/path.go", barPath, fooPath})) + assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) } diff --git a/pkg/session/store_test.go b/pkg/session/store_test.go index 7cefbf1ddc..ca66d6bfdb 100644 --- a/pkg/session/store_test.go +++ b/pkg/session/store_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "testing" "time" @@ -387,9 +388,16 @@ func TestStoreAgentNameJSON(t *testing.T) { func TestNewSQLiteSessionStore_DirectoryNotWritable(t *testing.T) { t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("Skipping test on Windows: os.Mkdir ignores Unix permission bits") + } + readOnlyDir := filepath.Join(t.TempDir(), "readonly") err := os.Mkdir(readOnlyDir, 0o555) require.NoError(t, err) + t.Cleanup(func() { + os.Chmod(readOnlyDir, 0o777) + }) _, err = NewSQLiteSessionStore(t.Context(), filepath.Join(readOnlyDir, "session.db")) require.Error(t, err) From 08fe4e631227bcd474278142687e648b1293aaa7 Mon Sep 17 00:00:00 2001 From: piyush0049 Date: Tue, 7 Jul 2026 20:36:15 +0530 Subject: [PATCH 3/5] fix: address PR feedback for sub-session permission isolation --- pkg/runtime/agent_delegation_test.go | 3 +-- pkg/session/store_test.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 1eae38b2e1..ca6b5fdb87 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -12,8 +12,8 @@ import ( "github.com/docker/docker-agent/pkg/agent" "github.com/docker/docker-agent/pkg/session" "github.com/docker/docker-agent/pkg/team" - agenttool "github.com/docker/docker-agent/pkg/tools/builtin/agent" "github.com/docker/docker-agent/pkg/tools" + agenttool "github.com/docker/docker-agent/pkg/tools/builtin/agent" ) func TestBuildTaskSystemMessage(t *testing.T) { @@ -497,4 +497,3 @@ func TestTransferTask_PropagatesPermissions(t *testing.T) { assert.Equal(t, []string{"safe_tool"}, parentClone.Allow, "parent permissions must remain isolated from child mutations after transfer_task") } - diff --git a/pkg/session/store_test.go b/pkg/session/store_test.go index ca66d6bfdb..0d7da34763 100644 --- a/pkg/session/store_test.go +++ b/pkg/session/store_test.go @@ -396,7 +396,7 @@ func TestNewSQLiteSessionStore_DirectoryNotWritable(t *testing.T) { err := os.Mkdir(readOnlyDir, 0o555) require.NoError(t, err) t.Cleanup(func() { - os.Chmod(readOnlyDir, 0o777) + _ = os.Chmod(readOnlyDir, 0o777) }) _, err = NewSQLiteSessionStore(t.Context(), filepath.Join(readOnlyDir, "session.db")) From 3d6fbcbe50adf90036ac322040dd5dcdf75a9bee Mon Sep 17 00:00:00 2001 From: piyush0049 Date: Wed, 8 Jul 2026 19:33:11 +0530 Subject: [PATCH 4/5] fix: resolve sub-session permission propagation data races and locking issues --- pkg/runtime/agent_delegation.go | 6 +-- pkg/runtime/agent_delegation_test.go | 40 ++++-------------- pkg/runtime/session_compaction_test.go | 9 ----- pkg/runtime/skill_runner.go | 4 +- pkg/runtime/tool_dispatch.go | 8 ++-- pkg/runtime/toolexec/dispatcher.go | 20 ++------- pkg/session/session.go | 27 +++++++++++++ pkg/session/session_options_test.go | 56 ++++++++++---------------- pkg/session/store_test.go | 8 ---- 9 files changed, 68 insertions(+), 110 deletions(-) diff --git a/pkg/runtime/agent_delegation.go b/pkg/runtime/agent_delegation.go index 4b9c526f30..4de78d9e06 100644 --- a/pkg/runtime/agent_delegation.go +++ b/pkg/runtime/agent_delegation.go @@ -326,7 +326,7 @@ func (r *LocalRuntime) runForwarding(ctx context.Context, parent *session.Sessio // 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.ToolsApproved = s.ToolsApproved + parent.SetToolsApproved(s.IsToolsApproved()) parent.SetPermissions(s.ClonePermissions()) span.SetStatus(codes.Ok, "sub-session completed") return tools.ResultSuccess(s.GetLastAssistantMessageContent()), nil @@ -550,8 +550,8 @@ func (r *LocalRuntime) handleTaskTransfer(ctx context.Context, sess *session.Ses ExpectedOutput: params.ExpectedOutput, AgentName: params.Agent, Title: "Transferred task", - ToolsApproved: sess.ToolsApproved, - Permissions: sess.Permissions, + ToolsApproved: sess.IsToolsApproved(), + Permissions: sess.ClonePermissions(), NonInteractive: sess.NonInteractive, }, SwitchCurrentAgent: true, diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index ca6b5fdb87..502689940d 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -1,8 +1,6 @@ package runtime import ( - "os" - "path/filepath" "strings" "testing" @@ -34,11 +32,9 @@ func TestBuildTaskSystemMessage(t *testing.T) { }) t.Run("with attached files", func(t *testing.T) { - fooPath := filepath.Join(os.TempDir(), "foo.go") - barPath := filepath.Join(os.TempDir(), "bar.go") - msg := buildTaskSystemMessage("do the thing", "", []string{fooPath, barPath}) + msg := buildTaskSystemMessage("do the thing", "", []string{"/abs/foo.go", "/abs/bar.go"}) assert.Contains(t, msg, "\ndo the thing\n") - assert.Contains(t, msg, "\n- "+fooPath+"\n- "+barPath+"\n") + assert.Contains(t, msg, "\n- /abs/foo.go\n- /abs/bar.go\n") }) } @@ -226,11 +222,9 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { t.Parallel() parent := session.New(session.WithUserMessage("hello")) - fooPath := filepath.Join(os.TempDir(), "foo.go") - barPath := filepath.Join(os.TempDir(), "bar.go") - parent.AddAttachedFile(fooPath) - parent.AddAttachedFile(barPath) - parent.AddAttachedFile(fooPath) // duplicate, should be ignored + parent.AddAttachedFile("/abs/foo.go") + parent.AddAttachedFile("/abs/bar.go") + parent.AddAttachedFile("/abs/foo.go") // duplicate, should be ignored childAgent := agent.New("worker", "") cfg := SubSessionConfig{ @@ -242,7 +236,7 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { s := newSubSession(parent, cfg, childAgent) // Child session inherits parent's attached files (deduplicated, ordered). - assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) // The system message lists them so the sub-agent sees them up-front. sysMsg := s.GetMessages(childAgent) @@ -252,7 +246,7 @@ func TestSubSessionInheritsAttachedFiles(t *testing.T) { joined.WriteString(m.Content) joined.WriteString("\n") } - assert.Contains(t, joined.String(), "\n- "+fooPath+"\n- "+barPath+"\n") + assert.Contains(t, joined.String(), "\n- /abs/foo.go\n- /abs/bar.go\n") } func TestSubSessionWithoutAttachedFilesOmitsBlock(t *testing.T) { @@ -299,10 +293,8 @@ func TestNewSubSession_PermissionsIsolation(t *testing.T) { require.NotNil(t, s.Permissions) assert.Equal(t, []string{"read_file"}, s.Permissions.Allow) - // Mutate the original permissions config perms.Allow = append(perms.Allow, "write_file") - // The sub-session's permissions should remain isolated assert.Equal(t, []string{"read_file"}, s.Permissions.Allow) }) @@ -311,7 +303,6 @@ func TestNewSubSession_PermissionsIsolation(t *testing.T) { Task: "work without permissions", AgentName: "worker", Title: "Task", - // Permissions is nil — the nil guard removal must not panic } s := newSubSession(parent, cfg, childAgent) @@ -334,7 +325,6 @@ func TestSession_ClonePermissions(t *testing.T) { assert.Equal(t, perms.Allow, cloned.Allow) assert.Equal(t, perms.Deny, cloned.Deny) - // Mutating the clone should not affect the session cloned.Allow = append(cloned.Allow, "exec_command") original := s.ClonePermissions() assert.Equal(t, []string{"read_file"}, original.Allow) @@ -362,10 +352,6 @@ func TestSession_SetPermissions(t *testing.T) { assert.Equal(t, []string{"read_file"}, got.Allow) } -// TestRunAgent_InheritsParentPermissions verifies that RunAgent correctly -// inherits the parent session's ToolsApproved flag and Permissions via -// the thread-safe accessors, and that the child session is isolated from -// later mutations of the parent. func TestRunAgent_InheritsParentPermissions(t *testing.T) { t.Parallel() @@ -401,7 +387,6 @@ func TestRunAgent_InheritsParentPermissions(t *testing.T) { }) require.Empty(t, result.ErrMsg, "RunAgent should succeed") - // The parent session must have a sub-session recorded. var childSession *session.Session for _, item := range parentSession.Messages { if item.SubSession != nil { @@ -411,25 +396,19 @@ func TestRunAgent_InheritsParentPermissions(t *testing.T) { } require.NotNil(t, childSession, "parent must have a sub-session") - // The child must have inherited ToolsApproved. assert.True(t, childSession.ToolsApproved, "child session must inherit ToolsApproved from parent") - // The child must have inherited permissions. require.NotNil(t, childSession.Permissions) assert.Equal(t, []string{"read_file", "list_dir"}, childSession.Permissions.Allow) assert.Equal(t, []string{"shell:cmd=rm*"}, childSession.Permissions.Deny) - // Mutating the child's permissions must NOT affect the parent (isolation). childSession.Permissions.Allow = append(childSession.Permissions.Allow, "write_file") parentClone := parentSession.ClonePermissions() assert.Equal(t, []string{"read_file", "list_dir"}, parentClone.Allow, "parent permissions must be isolated from child mutations") } -// TestTransferTask_PropagatesPermissions verifies that handleTaskTransfer -// passes the parent session's Permissions to the child sub-session, and -// that the child's permissions are isolated from the parent's. func TestTransferTask_PropagatesPermissions(t *testing.T) { t.Parallel() @@ -472,7 +451,6 @@ func TestTransferTask_PropagatesPermissions(t *testing.T) { require.NotNil(t, result) assert.False(t, result.IsError, "transfer to valid sub-agent should succeed") - // Find the sub-session that was created. var childSession *session.Session for _, item := range sess.Messages { if item.SubSession != nil { @@ -482,18 +460,16 @@ func TestTransferTask_PropagatesPermissions(t *testing.T) { } require.NotNil(t, childSession, "parent must have a sub-session after transfer_task") - // The child must have inherited the parent's permissions. require.NotNil(t, childSession.Permissions) assert.Equal(t, []string{"safe_tool"}, childSession.Permissions.Allow) assert.Equal(t, []string{"dangerous_tool"}, childSession.Permissions.Deny) - // The child must have inherited ToolsApproved. assert.True(t, childSession.ToolsApproved, "child session must inherit ToolsApproved from parent") - // Mutating the child's permissions must NOT affect the parent (isolation). childSession.Permissions.Allow = append(childSession.Permissions.Allow, "exploit") parentClone := sess.ClonePermissions() assert.Equal(t, []string{"safe_tool"}, parentClone.Allow, "parent permissions must remain isolated from child mutations after transfer_task") } + diff --git a/pkg/runtime/session_compaction_test.go b/pkg/runtime/session_compaction_test.go index 5db33940da..cd631da4e8 100644 --- a/pkg/runtime/session_compaction_test.go +++ b/pkg/runtime/session_compaction_test.go @@ -2,7 +2,6 @@ package runtime import ( "os" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -109,10 +108,6 @@ func TestSessionGetMessages_SummaryWithoutFirstKeptEntry(t *testing.T) { func TestDoCompactBeforeHookDeniesSkipsCompaction(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("Skipping test on Windows: relies on Unix shell semantics") - } - denyingHooks := &latest.HooksConfig{ BeforeCompaction: []latest.HookDefinition{ {Type: "command", Command: "echo 'denied for safety' >&2; exit 2", Timeout: 5}, @@ -237,10 +232,6 @@ func TestDoCompactBeforeHookSuppliesSummary(t *testing.T) { func TestDoCompactAfterHookFires(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("Skipping test on Windows: relies on Unix shell semantics and jq") - } - dir := t.TempDir() logFile := dir + "/after.log" diff --git a/pkg/runtime/skill_runner.go b/pkg/runtime/skill_runner.go index a8a9247afe..ba58f51e4b 100644 --- a/pkg/runtime/skill_runner.go +++ b/pkg/runtime/skill_runner.go @@ -109,8 +109,8 @@ func (r *LocalRuntime) RunSkillFork(ctx context.Context, sess *session.Session, ImplicitUserMessage: skills.BuildSkillUserMessage(prepared), AgentName: ca, Title: "Skill: " + prepared.SkillName, - ToolsApproved: sess.ToolsApproved, - Permissions: sess.Permissions, + ToolsApproved: sess.IsToolsApproved(), + Permissions: sess.ClonePermissions(), NonInteractive: sess.NonInteractive, ExcludedTools: []string{skills.ToolNameRunSkill}, AllowedTools: prepared.AllowedTools, diff --git a/pkg/runtime/tool_dispatch.go b/pkg/runtime/tool_dispatch.go index f0b96c8408..3f78bcf6a2 100644 --- a/pkg/runtime/tool_dispatch.go +++ b/pkg/runtime/tool_dispatch.go @@ -58,12 +58,12 @@ func (r *LocalRuntime) processToolCalls(ctx context.Context, sess *session.Sessi // evaluate (session-level first, then team-level). func (r *LocalRuntime) permissionCheckers(sess *session.Session) []toolexec.NamedChecker { var checkers []toolexec.NamedChecker - if sess.Permissions != nil { + if perms := sess.ClonePermissions(); perms != nil { checkers = append(checkers, toolexec.NamedChecker{ Checker: permissions.NewChecker(&latest.PermissionsConfig{ - Allow: sess.Permissions.Allow, - Ask: sess.Permissions.Ask, - Deny: sess.Permissions.Deny, + Allow: perms.Allow, + Ask: perms.Ask, + Deny: perms.Deny, }), Source: "session permissions", }) diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index cfee6682f0..75379c0948 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -7,7 +7,6 @@ import ( "fmt" "log/slog" "maps" - "slices" "strings" "sync" "time" @@ -175,7 +174,6 @@ type Dispatcher struct { Recall func(ctx context.Context, sess *session.Session, a *agent.Agent, message string) error confirmationMu sync.Mutex - approvalMu sync.Mutex } var ( @@ -434,15 +432,12 @@ func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) Ca } func (c *call) permissionDecision(readOnlyHint bool) PermissionDecision { - c.d.approvalMu.Lock() - defer c.d.approvalMu.Unlock() - var checkers []NamedChecker if c.d.Permissions != nil { checkers = c.d.Permissions(c.sess) } return Decide( - c.sess.ToolsApproved, + c.sess.IsToolsApproved(), checkers, c.tc.Function.Name, ParseToolInput(c.tc.Function.Arguments), @@ -789,9 +784,7 @@ func (c *call) handleResume(ctx context.Context, req ResumeRequest, runTool func return runTool() case ResumeTypeApproveSession: slog.DebugContext(ctx, "Resume signal received, approving session", "tool", c.tc.Function.Name, "session_id", c.sess.ID) - c.d.approvalMu.Lock() - c.sess.ToolsApproved = true - c.d.approvalMu.Unlock() + c.sess.SetToolsApproved(true) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedSession) return runTool() case ResumeTypeApproveTool: @@ -799,14 +792,7 @@ func (c *call) handleResume(ctx context.Context, req ResumeRequest, runTool func if approvedTool == "" { approvedTool = c.tc.Function.Name } - c.d.approvalMu.Lock() - if c.sess.Permissions == nil { - c.sess.Permissions = &session.PermissionsConfig{} - } - if !slices.Contains(c.sess.Permissions.Allow, approvedTool) { - c.sess.Permissions.Allow = append(c.sess.Permissions.Allow, approvedTool) - } - c.d.approvalMu.Unlock() + c.sess.AppendPermissionAllow(approvedTool) slog.DebugContext(ctx, "Resume signal received, approving tool permanently", "tool", approvedTool, "session_id", c.sess.ID) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceUserApprovedTool) return runTool() diff --git a/pkg/session/session.go b/pkg/session/session.go index 761d5d548e..7aeeaef700 100644 --- a/pkg/session/session.go +++ b/pkg/session/session.go @@ -1056,6 +1056,33 @@ func (s *Session) SetPermissions(perms *PermissionsConfig) { s.Permissions = perms } +// SetToolsApproved updates ToolsApproved under s.mu so concurrent readers +// (e.g. background-agent goroutines calling IsToolsApproved) observe a +// consistent value. It mirrors WithToolsApproved's SafetyPolicy sync. +func (s *Session) SetToolsApproved(approved bool) { + s.mu.Lock() + defer s.mu.Unlock() + s.ToolsApproved = approved + if approved && s.SafetyPolicy == "" { + s.SafetyPolicy = SafetyPolicyUnsafe + } +} + +// AppendPermissionAllow adds toolName to the session's Allow list if not +// already present, initializing Permissions when nil. Guarded by s.mu so +// concurrent readers (ClonePermissions, permissionCheckers) see a +// consistent snapshot. +func (s *Session) AppendPermissionAllow(toolName string) { + s.mu.Lock() + defer s.mu.Unlock() + if s.Permissions == nil { + s.Permissions = &PermissionsConfig{} + } + if !slices.Contains(s.Permissions.Allow, toolName) { + s.Permissions.Allow = append(s.Permissions.Allow, toolName) + } +} + // now returns the session's current time, falling back to time.Now for // sessions created without a clock (e.g. JSON deserialization). func (s *Session) now() time.Time { diff --git a/pkg/session/session_options_test.go b/pkg/session/session_options_test.go index fb140660b2..3d9a65cd12 100644 --- a/pkg/session/session_options_test.go +++ b/pkg/session/session_options_test.go @@ -1,8 +1,6 @@ package session import ( - "os" - "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -79,13 +77,11 @@ func TestAddAttachedFile(t *testing.T) { t.Parallel() t.Run("deduplicates and preserves order", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") - barPath := filepath.Join(os.TempDir(), "bar.go") s := New() - s.AddAttachedFile(fooPath) - s.AddAttachedFile(barPath) - s.AddAttachedFile(fooPath) // duplicate - assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) + s.AddAttachedFile("/abs/foo.go") + s.AddAttachedFile("/abs/bar.go") + s.AddAttachedFile("/abs/foo.go") // duplicate + assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) }) t.Run("ignores empty paths", func(t *testing.T) { @@ -106,12 +102,11 @@ func TestAddAttachedFile(t *testing.T) { t.Run("snapshot is independent of session storage", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - s.AddAttachedFile(fooPath) + s.AddAttachedFile("/abs/foo.go") snap := s.AttachedFilesSnapshot() snap[0] = "mutated" - assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) }) } @@ -119,52 +114,43 @@ func TestRemoveAttachedFile(t *testing.T) { t.Parallel() t.Run("removes and reports presence", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") - barPath := filepath.Join(os.TempDir(), "bar.go") - bazPath := filepath.Join(os.TempDir(), "baz.go") s := New() - s.AddAttachedFile(fooPath) - s.AddAttachedFile(barPath) - s.AddAttachedFile(bazPath) + s.AddAttachedFile("/abs/foo.go") + s.AddAttachedFile("/abs/bar.go") + s.AddAttachedFile("/abs/baz.go") - assert.True(t, s.RemoveAttachedFile(barPath)) - assert.Equal(t, []string{fooPath, bazPath}, s.AttachedFilesSnapshot()) + assert.True(t, s.RemoveAttachedFile("/abs/bar.go")) + assert.Equal(t, []string{"/abs/foo.go", "/abs/baz.go"}, s.AttachedFilesSnapshot()) }) t.Run("reports absent paths", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") - otherPath := filepath.Join(os.TempDir(), "other.go") s := New() - s.AddAttachedFile(fooPath) - assert.False(t, s.RemoveAttachedFile(otherPath)) + s.AddAttachedFile("/abs/foo.go") + assert.False(t, s.RemoveAttachedFile("/abs/other.go")) assert.False(t, s.RemoveAttachedFile("")) - assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) + assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) }) t.Run("no-op on empty list", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - assert.False(t, s.RemoveAttachedFile(fooPath)) + assert.False(t, s.RemoveAttachedFile("/abs/foo.go")) assert.Empty(t, s.AttachedFilesSnapshot()) }) t.Run("file can be re-attached after removal", func(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") s := New() - s.AddAttachedFile(fooPath) - require.True(t, s.RemoveAttachedFile(fooPath)) - s.AddAttachedFile(fooPath) - assert.Equal(t, []string{fooPath}, s.AttachedFilesSnapshot()) + s.AddAttachedFile("/abs/foo.go") + require.True(t, s.RemoveAttachedFile("/abs/foo.go")) + s.AddAttachedFile("/abs/foo.go") + assert.Equal(t, []string{"/abs/foo.go"}, s.AttachedFilesSnapshot()) }) } func TestWithAttachedFiles(t *testing.T) { t.Parallel() - fooPath := filepath.Join(os.TempDir(), "foo.go") - barPath := filepath.Join(os.TempDir(), "bar.go") - s := New(WithAttachedFiles([]string{fooPath, "", "relative/path.go", barPath, fooPath})) - assert.Equal(t, []string{fooPath, barPath}, s.AttachedFilesSnapshot()) + s := New(WithAttachedFiles([]string{"/abs/foo.go", "", "relative/path.go", "/abs/bar.go", "/abs/foo.go"})) + assert.Equal(t, []string{"/abs/foo.go", "/abs/bar.go"}, s.AttachedFilesSnapshot()) } diff --git a/pkg/session/store_test.go b/pkg/session/store_test.go index 0d7da34763..7cefbf1ddc 100644 --- a/pkg/session/store_test.go +++ b/pkg/session/store_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "os" "path/filepath" - "runtime" "testing" "time" @@ -388,16 +387,9 @@ func TestStoreAgentNameJSON(t *testing.T) { func TestNewSQLiteSessionStore_DirectoryNotWritable(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("Skipping test on Windows: os.Mkdir ignores Unix permission bits") - } - readOnlyDir := filepath.Join(t.TempDir(), "readonly") err := os.Mkdir(readOnlyDir, 0o555) require.NoError(t, err) - t.Cleanup(func() { - _ = os.Chmod(readOnlyDir, 0o777) - }) _, err = NewSQLiteSessionStore(t.Context(), filepath.Join(readOnlyDir, "session.db")) require.Error(t, err) From a9d44c4175d9d31637e6e295e490b9c7f8151dae Mon Sep 17 00:00:00 2001 From: piyush0049 Date: Wed, 8 Jul 2026 19:34:46 +0530 Subject: [PATCH 5/5] style: format agent_delegation_test.go --- pkg/runtime/agent_delegation_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/runtime/agent_delegation_test.go b/pkg/runtime/agent_delegation_test.go index 502689940d..2e27bd3b92 100644 --- a/pkg/runtime/agent_delegation_test.go +++ b/pkg/runtime/agent_delegation_test.go @@ -472,4 +472,3 @@ func TestTransferTask_PropagatesPermissions(t *testing.T) { assert.Equal(t, []string{"safe_tool"}, parentClone.Allow, "parent permissions must remain isolated from child mutations after transfer_task") } -