Merge pull request #2388 from dgageot/board/fix-docker-agent-issue-2383-81b49d7b

fix: prevent panic in code mode when tool handler is nil

Author: dgageot

pkg/toolinstall/resolver.go

@@ -25,23 +25,14 @@ func EnsureCommand(ctx context.Context, command, version string) (string, error)
 		return command, nil
 	}
 
-	version = strings.TrimSpace(version)
-	lower := strings.ToLower(version)
+	lower := strings.ToLower(strings.TrimSpace(version))
 	if lower == "false" || lower == "off" {
 		return command, nil
 	}
 
 	resolvedPath, err := resolve(ctx, command, version)
 	if err != nil {
-		if version != "" {
-			return "", fmt.Errorf("auto-installing command %q: %w", command, err)
-		}
-
-		slog.Warn("Auto-install failed, falling back to original command",
-			"command", command,
-			"error", err,
-		)
-		return command, nil
+		return "", fmt.Errorf("auto-installing command %q: %w", command, err)
 	}
 
 	return resolvedPath, nil

pkg/toolinstall/resolver_test.go

@@ -50,16 +50,7 @@ func TestEnsureCommand_DisabledPerToolset(t *testing.T) {
 	}
 }
 
-func TestEnsureCommand_AutoDetectFailureFallsBackToOriginalCommand(t *testing.T) {
-	t.Setenv("DOCKER_AGENT_TOOLS_DIR", t.TempDir())
-	t.Setenv("DOCKER_AGENT_AUTO_INSTALL", "")
-
-	result, err := EnsureCommand(t.Context(), "nonexistent-tool", "")
-	require.NoError(t, err)
-	assert.Equal(t, "nonexistent-tool", result)
-}
-
-func TestEnsureCommand_ExplicitVersionFailureStillErrors(t *testing.T) {
+func TestEnsureCommand_SoftFail(t *testing.T) {
 	t.Setenv("DOCKER_AGENT_TOOLS_DIR", t.TempDir())
 	t.Setenv("DOCKER_AGENT_AUTO_INSTALL", "")
 
@@ -82,7 +73,7 @@ func TestEnsureCommand_FoundInBinDir(t *testing.T) {
 	assert.Equal(t, fakeBin, result)
 }
 
-func TestEnsureCommand_NonExecutableInBinDirFallsBackToOriginalCommand(t *testing.T) {
+func TestEnsureCommand_NonExecutableInBinDirIsSkipped(t *testing.T) {
 	toolsDir := t.TempDir()
 	t.Setenv("DOCKER_AGENT_TOOLS_DIR", toolsDir)
 	t.Setenv("DOCKER_AGENT_AUTO_INSTALL", "")
@@ -91,9 +82,9 @@ func TestEnsureCommand_NonExecutableInBinDirFallsBackToOriginalCommand(t *testin
 	require.NoError(t, os.MkdirAll(binDir, 0o755))
 	require.NoError(t, os.WriteFile(filepath.Join(binDir, "not-executable"), []byte("data"), 0o644))
 
-	result, err := EnsureCommand(t.Context(), "not-executable", "")
-	require.NoError(t, err)
-	assert.Equal(t, "not-executable", result)
+	// Falls through to auto-install → fails → returns error.
+	_, err := EnsureCommand(t.Context(), "not-executable", "")
+	require.Error(t, err)
 }
 
 // --- resolve tests ---

pkg/tools/codemode/exec.go

@@ -89,57 +89,62 @@ func (c *codeModeTool) runJavascript(ctx context.Context, script string) (Script
 
 func callTool(ctx context.Context, tool tools.Tool, tracker *toolCallTracker) func(args map[string]any) (string, error) {
 	return func(args map[string]any) (string, error) {
-		var toolArgs struct {
-			Required []string `json:"required"`
-		}
-
-		if err := tools.ConvertSchema(tool.Parameters, &toolArgs); err != nil {
-			tracker.record(ToolCallInfo{
-				Name:      tool.Name,
-				Arguments: args,
-				Error:     err.Error(),
-			})
-			return "", err
-		}
+		output, filtered, err := invokeTool(ctx, tool, args)
 
-		nonNilArgs := make(map[string]any)
-		for k, v := range args {
-			if slices.Contains(toolArgs.Required, k) || v != nil {
-				nonNilArgs[k] = v
-			}
+		info := ToolCallInfo{
+			Name:      tool.Name,
+			Arguments: filtered,
 		}
-
-		arguments, err := json.Marshal(nonNilArgs)
 		if err != nil {
-			tracker.record(ToolCallInfo{
-				Name:      tool.Name,
-				Arguments: nonNilArgs,
-				Error:     err.Error(),
-			})
-			return "", err
+			info.Error = err.Error()
+		} else {
+			info.Result = output
 		}
+		tracker.record(info)
 
-		result, err := tool.Handler(ctx, tools.ToolCall{
-			Function: tools.FunctionCall{
-				Name:      tool.Name,
-				Arguments: string(arguments),
-			},
-		})
-		if err != nil {
-			tracker.record(ToolCallInfo{
-				Name:      tool.Name,
-				Arguments: nonNilArgs,
-				Error:     err.Error(),
-			})
-			return "", err
+		return output, err
+	}
+}
+
+// invokeTool calls a single tool handler, filtering out nil optional arguments.
+// It returns the output, the filtered arguments actually sent, and any error.
+func invokeTool(ctx context.Context, tool tools.Tool, args map[string]any) (string, map[string]any, error) {
+	if tool.Handler == nil {
+		return "", args, fmt.Errorf("tool %q is not available in code mode", tool.Name)
+	}
+
+	var schema struct {
+		Required []string `json:"required"`
+	}
+	if err := tools.ConvertSchema(tool.Parameters, &schema); err != nil {
+		return "", args, err
+	}
+
+	// Strip nil optional arguments that goja passes for omitted parameters.
+	filtered := make(map[string]any)
+	for k, v := range args {
+		if slices.Contains(schema.Required, k) || v != nil {
+			filtered[k] = v
 		}
+	}
 
-		tracker.record(ToolCallInfo{
+	arguments, err := json.Marshal(filtered)
+	if err != nil {
+		return "", filtered, err
+	}
+
+	result, err := tool.Handler(ctx, tools.ToolCall{
+		Function: tools.FunctionCall{
 			Name:      tool.Name,
-			Arguments: nonNilArgs,
-			Result:    result.Output,
-		})
+			Arguments: string(arguments),
+		},
+	})
+	if err != nil {
+		return "", filtered, err
+	}
 
-		return result.Output, nil
+	if result == nil {
+		return "", filtered, nil
 	}
+	return result.Output, filtered, nil
 }