fix: address reviewer feedback on working_dir feature

B1: pass resolved CWD to NewGatewayToolset; gateway-based MCPs now
honour working_dir. Remote MCP toolsets reject working_dir at
validation time (no local subprocess).

B2: call path.ExpandPath in resolveToolsetWorkingDir so ~ and
${VAR}/$VAR are expanded before path operations. Use filepath.Abs
when joining a relative path with the agent dir (fixes file://./backend
LSP root URI).

S1: add checkDirExists helper; createMCPTool and createLSPTool now
surface a clear error if working_dir does not exist at tool-creation
time.

S2: doc comment on resolveToolsetWorkingDir explains no-containment-
check posture (working_dir is treated like command/args).

S3: filepath.Abs on relative+non-empty-agentDir path ensures LSP
rootURI is always absolute.

S4: validation rejects working_dir on remote MCP toolsets.

S5: comment in applyMCPDefaults explains empty-string inheritance
semantics.

S6: doc comment covers the empty-agent-dir+relative edge case.

Nits: example model → gpt-5-mini; test name → 'bare relative dir';
schema descriptions aligned; import groups tidied; hard-coded
non-existent path in test → t.TempDir()+'/missing'.

N5: integration tests added:
- TestCreateMCPTool_WorkingDir_ReachesSubprocess
- TestCreateMCPTool_RelativeWorkingDir_ResolvedAgainstAgentDir
- TestCreateMCPTool_NonexistentWorkingDir_ReturnsError
- TestCreateLSPTool_WorkingDir_ReachesHandler

Also add WorkingDir() accessors on *mcp.Toolset and *builtin.LSPTool
to support the above integration tests.

Assisted-By: docker-agent

Author: simonferquel-clanker

agent-schema.json

@@ -904,7 +904,7 @@
         },
         "working_dir": {
           "type": "string",
-          "description": "Optional working directory for the MCP server process. Relative paths are resolved relative to the agent's working directory."
+          "description": "Optional working directory for the MCP server process. Relative paths are resolved relative to the agent's working directory. Only valid for subprocess-based MCP types (command or ref); not supported for remote MCP toolsets."
         }
       },
       "anyOf": [
@@ -1148,7 +1148,7 @@
         },
         "working_dir": {
           "type": "string",
-          "description": "Optional working directory for MCP/LSP toolset processes. Relative paths are resolved relative to the agent's working directory. Only valid for type 'mcp' or 'lsp'."
+          "description": "Optional working directory for MCP/LSP toolset processes. Relative paths are resolved relative to the agent's working directory. Only valid for type 'mcp' or 'lsp', and not supported for remote MCP toolsets (no local subprocess)."
         }
       },
       "additionalProperties": false,

examples/toolset-working-dir.yaml

@@ -12,7 +12,7 @@
 
 agents:
   root:
-    model: openai/gpt-4o
+    model: openai/gpt-5-mini
     description: Example agent demonstrating working_dir for MCP and LSP toolsets
     instruction: |
       You are a helpful coding assistant with access to language server and MCP tools

pkg/config/latest/validate.go

@@ -117,6 +117,10 @@ func (t *Toolset) validate() error {
 	if t.WorkingDir != "" && t.Type != "mcp" && t.Type != "lsp" {
 		return errors.New("working_dir can only be used with type 'mcp' or 'lsp'")
 	}
+	// working_dir requires a local subprocess; it is meaningless for remote MCP toolsets.
+	if t.WorkingDir != "" && t.Type == "mcp" && t.Remote.URL != "" {
+		return errors.New("working_dir is not valid for remote MCP toolsets (no local subprocess)")
+	}
 
 	switch t.Type {
 	case "shell":

pkg/config/latest/validate_test.go

@@ -181,6 +181,22 @@ agents:
 			wantErr:   "",
 			wantValue: "",
 		},
+		{
+			name: "working_dir on remote mcp is rejected",
+			config: `
+version: "8"
+agents:
+  root:
+    model: "openai/gpt-4"
+    toolsets:
+      - type: mcp
+        remote:
+          url: https://mcp.example.com/sse
+        working_dir: ./tools
+`,
+			wantErr:   "working_dir is not valid for remote MCP toolsets",
+			wantValue: "",
+		},
 	}
 
 	for _, tt := range tests {

pkg/config/mcps.go

@@ -87,6 +87,10 @@ func applyMCPDefaults(ts, def *latest.Toolset) {
 		ts.Defer = def.Defer
 	}
 	if ts.WorkingDir == "" {
+		// An empty working_dir in the referencing toolset is treated as "unset":
+		// inherit the definition's value. This matches the semantics of all other
+		// string fields in this function. An explicit `working_dir: ""` in YAML
+		// is indistinguishable from omission and will therefore be overridden.
 		ts.WorkingDir = def.WorkingDir
 	}
 	if len(def.Env) > 0 {

pkg/teamloader/registry.go

@@ -86,19 +86,59 @@ func NewDefaultToolsetRegistry() *ToolsetRegistry {
 	return r
 }
 
+// checkDirExists returns an error if the given directory does not exist or is
+// not a directory. toolsetType is used only in the error message.
+func checkDirExists(dir, toolsetType string) error {
+	info, err := os.Stat(dir)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("working_dir %q for %s toolset does not exist", dir, toolsetType)
+		}
+		return fmt.Errorf("working_dir %q for %s toolset: %w", dir, toolsetType, err)
+	}
+	if !info.IsDir() {
+		return fmt.Errorf("working_dir %q for %s toolset is not a directory", dir, toolsetType)
+	}
+	return nil
+}
+
 // resolveToolsetWorkingDir returns the effective working directory for a toolset process.
-// If toolsetWorkingDir is set, it is resolved relative to agentWorkingDir (when relative).
-// If toolsetWorkingDir is empty, agentWorkingDir is returned unchanged.
+//
+// Resolution rules:
+//   - If toolsetWorkingDir is empty, agentWorkingDir is returned unchanged.
+//   - Shell patterns (~ and ${VAR}/$VAR) are expanded before any further processing.
+//   - If the expanded path is absolute, it is returned as-is.
+//   - If the expanded path is relative and agentWorkingDir is non-empty,
+//     it is joined with agentWorkingDir and made absolute via filepath.Abs.
+//   - If the expanded path is relative and agentWorkingDir is empty,
+//     the relative path is returned unchanged (caller will inherit the process cwd).
+//
+// Note: unlike resolveToolsetPath, this helper does not enforce containment
+// within the agent working directory. working_dir is treated like command/args —
+// a trusted, operator-authored value where cross-tree references (e.g. a sibling
+// module root in a monorepo) are intentional and must not be silently blocked.
 func resolveToolsetWorkingDir(toolsetWorkingDir, agentWorkingDir string) string {
 	if toolsetWorkingDir == "" {
 		return agentWorkingDir
 	}
+	// Expand ~ and environment variables before path operations.
+	toolsetWorkingDir = path.ExpandPath(toolsetWorkingDir)
 	if filepath.IsAbs(toolsetWorkingDir) {
 		return toolsetWorkingDir
 	}
 	if agentWorkingDir != "" {
+		// filepath.Abs cleans the result and anchors the URI correctly
+		// (avoids file://./backend-style LSP root URIs when the agent dir
+		// is itself absolute, which is the normal case).
+		abs, err := filepath.Abs(filepath.Join(agentWorkingDir, toolsetWorkingDir))
+		if err == nil {
+			return abs
+		}
+		// Fallback: return the joined path without Abs (should not happen in practice).
 		return filepath.Join(agentWorkingDir, toolsetWorkingDir)
 	}
+	// agentWorkingDir is empty and path is relative: return as-is.
+	// The child process will inherit the OS working directory.
 	return toolsetWorkingDir
 }
 
@@ -257,6 +297,19 @@ func createFetchTool(_ context.Context, toolset latest.Toolset, _ string, _ *con
 func createMCPTool(ctx context.Context, toolset latest.Toolset, _ string, runConfig *config.RuntimeConfig, _ string) (tools.ToolSet, error) {
 	envProvider := runConfig.EnvProvider()
 
+	// Resolve the working directory once; used for all subprocess-based branches.
+	// The remote branch never reaches here because working_dir is rejected by
+	// validation for toolsets with a remote.url.
+	cwd := resolveToolsetWorkingDir(toolset.WorkingDir, runConfig.WorkingDir)
+
+	// S1: validate the resolved directory exists (if one was specified) so we
+	// surface a clear error now rather than a cryptic exec failure later.
+	if toolset.WorkingDir != "" {
+		if err := checkDirExists(cwd, "mcp"); err != nil {
+			return nil, err
+		}
+	}
+
 	switch {
 	// MCP Server from the MCP Catalog, running with the MCP Gateway
 	case toolset.Ref != "":
@@ -281,7 +334,8 @@ func createMCPTool(ctx context.Context, toolset latest.Toolset, _ string, runCon
 			envProvider,
 		)
 
-		return mcp.NewGatewayToolset(ctx, toolset.Name, mcpServerName, serverSpec.Secrets, toolset.Config, envProvider, runConfig.WorkingDir)
+		// Pass the resolved cwd so gateway-based MCPs also honour working_dir.
+		return mcp.NewGatewayToolset(ctx, toolset.Name, mcpServerName, serverSpec.Secrets, toolset.Config, envProvider, cwd)
 
 	// STDIO MCP Server from shell command
 	case toolset.Command != "":
@@ -305,10 +359,9 @@ func createMCPTool(ctx context.Context, toolset latest.Toolset, _ string, runCon
 		// Prepend tools bin dir to PATH so child processes can find installed tools
 		env = toolinstall.PrependBinDirToEnv(env)
 
-		cwd := resolveToolsetWorkingDir(toolset.WorkingDir, runConfig.WorkingDir)
 		return mcp.NewToolsetCommand(toolset.Name, resolvedCommand, toolset.Args, env, cwd), nil
 
-	// Remote MCP Server
+	// Remote MCP Server — working_dir is rejected at validation time for this branch.
 	case toolset.Remote.URL != "":
 		expander := js.NewJsExpander(envProvider)
 
@@ -347,6 +400,15 @@ func createLSPTool(ctx context.Context, toolset latest.Toolset, _ string, runCon
 	env = toolinstall.PrependBinDirToEnv(env)
 
 	cwd := resolveToolsetWorkingDir(toolset.WorkingDir, runConfig.WorkingDir)
+
+	// S1: validate the resolved directory exists (if one was specified) so we
+	// surface a clear error now rather than a cryptic exec failure later.
+	if toolset.WorkingDir != "" {
+		if err := checkDirExists(cwd, "lsp"); err != nil {
+			return nil, err
+		}
+	}
+
 	tool := builtin.NewLSPTool(resolvedCommand, toolset.Args, env, cwd)
 	if len(toolset.FileTypes) > 0 {
 		tool.SetFileTypes(toolset.FileTypes)

pkg/teamloader/registry_test.go

@@ -1,6 +1,8 @@
 package teamloader
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -10,6 +12,8 @@ import (
 	"github.com/docker/docker-agent/pkg/config/latest"
 	"github.com/docker/docker-agent/pkg/environment"
 	"github.com/docker/docker-agent/pkg/tools"
+	"github.com/docker/docker-agent/pkg/tools/builtin"
+	mcptool "github.com/docker/docker-agent/pkg/tools/mcp"
 )
 
 func TestCreateShellTool(t *testing.T) {
@@ -74,6 +78,9 @@ func TestCreateMCPTool_BareCommandNotFound_CreatesToolsetAnyway(t *testing.T) {
 func TestResolveToolsetWorkingDir(t *testing.T) {
 	t.Parallel()
 
+	home, err := os.UserHomeDir()
+	require.NoError(t, err)
+
 	tests := []struct {
 		name              string
 		toolsetWorkingDir string
@@ -99,7 +106,7 @@ func TestResolveToolsetWorkingDir(t *testing.T) {
 			want:              "/workspace/backend",
 		},
 		{
-			name:              "relative toolset dir without leading dot is joined with agent dir",
+			name:              "bare relative dir joined with agent dir",
 			toolsetWorkingDir: "tools/mcp",
 			agentWorkingDir:   "/workspace",
 			want:              "/workspace/tools/mcp",
@@ -116,6 +123,19 @@ func TestResolveToolsetWorkingDir(t *testing.T) {
 			agentWorkingDir:   "",
 			want:              "",
 		},
+		// Tilde expansion tests (B2)
+		{
+			name:              "tilde expands to home dir",
+			toolsetWorkingDir: "~/projects/app",
+			agentWorkingDir:   "/workspace",
+			want:              filepath.Join(home, "projects/app"),
+		},
+		{
+			name:              "bare tilde expands to home dir",
+			toolsetWorkingDir: "~",
+			agentWorkingDir:   "/workspace",
+			want:              home,
+		},
 	}
 
 	for _, tt := range tests {
@@ -126,3 +146,130 @@ func TestResolveToolsetWorkingDir(t *testing.T) {
 		})
 	}
 }
+
+// TestResolveToolsetWorkingDir_EnvVarExpansion tests env-var expansion separately
+// because t.Setenv is incompatible with t.Parallel on the parent test.
+func TestResolveToolsetWorkingDir_EnvVarExpansion(t *testing.T) {
+	t.Setenv("TEST_REGISTRY_CWD_VAR", "/custom/path")
+
+	got := resolveToolsetWorkingDir("${TEST_REGISTRY_CWD_VAR}/app", "/workspace")
+	assert.Equal(t, "/custom/path/app", got)
+}
+
+// TestCreateMCPTool_WorkingDir_ReachesSubprocess verifies that working_dir is
+// wired all the way through createMCPTool to the underlying stdio command (N5).
+func TestCreateMCPTool_WorkingDir_ReachesSubprocess(t *testing.T) {
+	t.Setenv("DOCKER_AGENT_TOOLS_DIR", t.TempDir())
+
+	// Create a real temporary directory so the existence check passes.
+	customDir := t.TempDir()
+	agentDir := t.TempDir()
+
+	toolset := latest.Toolset{
+		Type:       "mcp",
+		Command:    "some-nonexistent-mcp-binary",
+		WorkingDir: customDir,
+	}
+
+	registry := NewDefaultToolsetRegistry()
+	runConfig := &config.RuntimeConfig{
+		Config:              config.Config{WorkingDir: agentDir},
+		EnvProviderForTests: environment.NewOsEnvProvider(),
+	}
+
+	rawTool, err := registry.CreateTool(t.Context(), toolset, ".", runConfig, "test-agent")
+	require.NoError(t, err)
+	require.NotNil(t, rawTool)
+
+	// Assert the CWD reached the inner stdio command.
+	ts, ok := rawTool.(*mcptool.Toolset)
+	require.True(t, ok, "expected *mcp.Toolset")
+	assert.Equal(t, customDir, ts.WorkingDir())
+}
+
+// TestCreateMCPTool_RelativeWorkingDir_ResolvedAgainstAgentDir verifies that a
+// relative working_dir is resolved against the agent's working directory.
+func TestCreateMCPTool_RelativeWorkingDir_ResolvedAgainstAgentDir(t *testing.T) {
+	t.Setenv("DOCKER_AGENT_TOOLS_DIR", t.TempDir())
+
+	agentDir := t.TempDir()
+	// Create the subdirectory so the existence check passes.
+	subDir := filepath.Join(agentDir, "tools", "mcp")
+	require.NoError(t, os.MkdirAll(subDir, 0o700))
+
+	toolset := latest.Toolset{
+		Type:       "mcp",
+		Command:    "some-nonexistent-mcp-binary",
+		WorkingDir: "tools/mcp",
+	}
+
+	registry := NewDefaultToolsetRegistry()
+	runConfig := &config.RuntimeConfig{
+		Config:              config.Config{WorkingDir: agentDir},
+		EnvProviderForTests: environment.NewOsEnvProvider(),
+	}
+
+	rawTool, err := registry.CreateTool(t.Context(), toolset, ".", runConfig, "test-agent")
+	require.NoError(t, err)
+	require.NotNil(t, rawTool)
+
+	ts, ok := rawTool.(*mcptool.Toolset)
+	require.True(t, ok, "expected *mcp.Toolset")
+	assert.Equal(t, subDir, ts.WorkingDir())
+}
+
+// TestCreateMCPTool_NonexistentWorkingDir_ReturnsError verifies that a
+// non-existent working_dir surfaces a clear error at tool-creation time (S1).
+func TestCreateMCPTool_NonexistentWorkingDir_ReturnsError(t *testing.T) {
+	t.Setenv("DOCKER_AGENT_TOOLS_DIR", t.TempDir())
+
+	// Use a path that is guaranteed not to exist by creating a tempdir and
+	// appending a non-existent subdir (avoids flakes on hosts where a
+	// hard-coded path might coincidentally exist).
+	nonExistent := filepath.Join(t.TempDir(), "missing")
+
+	toolset := latest.Toolset{
+		Type:       "mcp",
+		Command:    "some-nonexistent-mcp-binary",
+		WorkingDir: nonExistent,
+	}
+
+	registry := NewDefaultToolsetRegistry()
+	runConfig := &config.RuntimeConfig{
+		Config:              config.Config{WorkingDir: t.TempDir()},
+		EnvProviderForTests: environment.NewOsEnvProvider(),
+	}
+
+	_, err := registry.CreateTool(t.Context(), toolset, ".", runConfig, "test-agent")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "working_dir")
+	assert.Contains(t, err.Error(), "does not exist")
+}
+
+// TestCreateLSPTool_WorkingDir_ReachesHandler verifies that working_dir is
+// wired all the way through createLSPTool to the LSP handler (N5).
+func TestCreateLSPTool_WorkingDir_ReachesHandler(t *testing.T) {
+	// Create a real temporary directory so the existence check passes.
+	customDir := t.TempDir()
+	agentDir := t.TempDir()
+
+	toolset := latest.Toolset{
+		Type:       "lsp",
+		Command:    "gopls",
+		WorkingDir: customDir,
+	}
+
+	registry := NewDefaultToolsetRegistry()
+	runConfig := &config.RuntimeConfig{
+		Config:              config.Config{WorkingDir: agentDir},
+		EnvProviderForTests: environment.NewOsEnvProvider(),
+	}
+
+	rawTool, err := registry.CreateTool(t.Context(), toolset, ".", runConfig, "test-agent")
+	require.NoError(t, err)
+	require.NotNil(t, rawTool)
+
+	lsp, ok := rawTool.(*builtin.LSPTool)
+	require.True(t, ok, "expected *builtin.LSPTool")
+	assert.Equal(t, customDir, lsp.WorkingDir())
+}

pkg/tools/builtin/lsp.go

@@ -346,6 +346,12 @@ func (t *LSPTool) SetFileTypes(fileTypes []string) {
 	t.handler.fileTypes = fileTypes
 }
 
+// WorkingDir returns the working directory of the LSP server process.
+// This is intended for testing only.
+func (t *LSPTool) WorkingDir() string {
+	return t.handler.workingDir
+}
+
 // HandlesFile checks if this LSP handles the given file based on its extension.
 func (t *LSPTool) HandlesFile(path string) bool {
 	return t.handler.handlesFile(path)