From e6e82e8eedd45fb060a781610e15bc180ea3386f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:34:09 -0500 Subject: [PATCH 01/48] chore(deps): add modelcontextprotocol/go-sdk --- go.mod | 1 + go.sum | 2 ++ 2 files changed, 3 insertions(+) diff --git a/go.mod b/go.mod index 2204e857d..6d794d910 100644 --- a/go.mod +++ b/go.mod @@ -41,6 +41,7 @@ require ( github.com/moby/buildkit v0.29.0 github.com/moby/patternmatcher v0.6.1 github.com/moby/term v0.5.2 + github.com/modelcontextprotocol/go-sdk v1.6.1 github.com/onsi/ginkgo/v2 v2.28.1 github.com/onsi/gomega v1.39.1 github.com/pkg/sftp v1.13.10 diff --git a/go.sum b/go.sum index a3f9b073b..cf90afc2b 100644 --- a/go.sum +++ b/go.sum @@ -535,6 +535,8 @@ github.com/moby/sys/userns v0.1.0 h1:tVLXkFOxVu9A64/yh59slHVv9ahO9UIev4JZusOLG/g github.com/moby/sys/userns v0.1.0/go.mod h1:IHUYgu/kao6N8YZlp9Cf444ySSvCmDlmzUcYfDHOl28= github.com/moby/term v0.5.2 h1:6qk3FJAFDs6i/q3W/pQ97SX192qKfZgGjCQqfCJkgzQ= github.com/moby/term v0.5.2/go.mod h1:d3djjFCrjnB+fl8NJux+EJzu0msscUP+f8it8hPkFLc= +github.com/modelcontextprotocol/go-sdk v1.6.1 h1:0zOSupjKUxPKSocPT1Wtago+mUHU2/uZ4xSOY0FGReU= +github.com/modelcontextprotocol/go-sdk v1.6.1/go.mod h1:kzm3kzFL1/+AziGOE0nUs3gvPoNxMCvkxokMkuFapXQ= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= From 910ca3b9a2954501d042691f7ccdf16e1eb27231 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:38:11 -0500 Subject: [PATCH 02/48] feat(mcp): add mcp serve command skeleton --- cmd/mcp/mcp.go | 16 ++++++++++++++++ cmd/mcp/serve.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ cmd/root.go | 4 ++++ 3 files changed, 68 insertions(+) create mode 100644 cmd/mcp/mcp.go create mode 100644 cmd/mcp/serve.go diff --git a/cmd/mcp/mcp.go b/cmd/mcp/mcp.go new file mode 100644 index 000000000..d1629410e --- /dev/null +++ b/cmd/mcp/mcp.go @@ -0,0 +1,16 @@ +package mcp + +import ( + "github.com/devsy-org/devsy/cmd/flags" + "github.com/spf13/cobra" +) + +// NewMCPCmd builds the 'devsy mcp' parent command. +func NewMCPCmd(globalFlags *flags.GlobalFlags) *cobra.Command { + cmd := &cobra.Command{ + Use: "mcp", + Short: "Run Devsy as a Model Context Protocol server", + } + cmd.AddCommand(NewServeCmd(globalFlags)) + return cmd +} diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go new file mode 100644 index 000000000..281a3f06e --- /dev/null +++ b/cmd/mcp/serve.go @@ -0,0 +1,48 @@ +package mcp + +import ( + "context" + "time" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/log" + "github.com/spf13/cobra" +) + +// ServeCmd holds configuration for `devsy mcp serve`. +type ServeCmd struct { + *flags.GlobalFlags + + ExecTimeoutDefault time.Duration + ExecTimeoutMax time.Duration + ExecOutputCap int +} + +// NewServeCmd builds the `serve` subcommand. +func NewServeCmd(globalFlags *flags.GlobalFlags) *cobra.Command { + cmd := &ServeCmd{GlobalFlags: globalFlags} + cobraCmd := &cobra.Command{ + Use: "serve", + Short: "Run an MCP server over stdio", + Args: cobra.NoArgs, + RunE: func(cobraCmd *cobra.Command, _ []string) error { + return cmd.Run(cobraCmd.Context()) + }, + } + cobraCmd.Flags().DurationVar(&cmd.ExecTimeoutDefault, "exec-timeout-default", 5*time.Minute, + "Default timeout for workspace_exec calls") + cobraCmd.Flags().DurationVar(&cmd.ExecTimeoutMax, "exec-timeout-max", 30*time.Minute, + "Maximum timeout for workspace_exec calls (caller values are clamped)") + cobraCmd.Flags().IntVar(&cmd.ExecOutputCap, "exec-output-cap", 100*1024, + "Per-stream byte cap for workspace_exec output; excess is replaced with a truncation marker") + return cobraCmd +} + +// Run wires up the MCP server and serves over stdio until ctx is cancelled. +func (cmd *ServeCmd) Run(ctx context.Context) error { + log.Debugf("starting MCP server (timeout default=%s max=%s cap=%dB)", + cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) + // Tool registration and stdio loop are added in later tasks. + _ = ctx + return nil +} diff --git a/cmd/root.go b/cmd/root.go index ce6bc7f6a..83147f858 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -14,6 +14,7 @@ import ( "github.com/devsy-org/devsy/cmd/ide" cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/machine" + "github.com/devsy-org/devsy/cmd/mcp" "github.com/devsy-org/devsy/cmd/pro" "github.com/devsy-org/devsy/cmd/provider" "github.com/devsy-org/devsy/cmd/self" @@ -177,6 +178,9 @@ func registerSubcommands(rootCmd *cobra.Command, globalFlags *flags.GlobalFlags) selfCmd := self.NewSelfCmd(globalFlags) selfCmd.GroupID = groupMeta rootCmd.AddCommand(selfCmd) + mcpCmd := mcp.NewMCPCmd(globalFlags) + mcpCmd.GroupID = groupMeta + rootCmd.AddCommand(mcpCmd) configCmd := cliconfig.NewConfigCmd(globalFlags) configCmd.GroupID = groupDevcontainer rootCmd.AddCommand(configCmd) From 4304a4d9332957dfd7e936d4516a8928104276af Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:40:03 -0500 Subject: [PATCH 03/48] feat(mcp): add BoundedBuffer for exec output capture --- cmd/mcp/buffer.go | 55 ++++++++++++++++++++++++++++++++++++++++++ cmd/mcp/buffer_test.go | 42 ++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 cmd/mcp/buffer.go create mode 100644 cmd/mcp/buffer_test.go diff --git a/cmd/mcp/buffer.go b/cmd/mcp/buffer.go new file mode 100644 index 000000000..405caf042 --- /dev/null +++ b/cmd/mcp/buffer.go @@ -0,0 +1,55 @@ +package mcp + +import "fmt" + +// BoundedBuffer is an io.Writer that retains at most Cap bytes by keeping the +// first Cap/2 and last Cap/2 bytes written. String() reports the contents +// joined with a truncation marker when more than Cap bytes were written. +type BoundedBuffer struct { + cap int + head []byte + tail []byte + written int64 +} + +func NewBoundedBuffer(cap int) *BoundedBuffer { + if cap < 8 { + cap = 8 + } + return &BoundedBuffer{cap: cap} +} + +func (b *BoundedBuffer) Write(p []byte) (int, error) { + n := len(p) + b.written += int64(n) + half := b.cap / 2 + + if len(b.head) < half { + take := min(half-len(b.head), n) + b.head = append(b.head, p[:take]...) + p = p[take:] + } + if len(p) == 0 { + return n, nil + } + + b.tail = append(b.tail, p...) + if len(b.tail) > half { + b.tail = b.tail[len(b.tail)-half:] + } + return n, nil +} + +func (b *BoundedBuffer) Truncated() bool { + return b.written > int64(b.cap) +} + +func (b *BoundedBuffer) BytesWritten() int64 { return b.written } + +func (b *BoundedBuffer) String() string { + if !b.Truncated() { + return string(b.head) + string(b.tail) + } + dropped := b.written - int64(len(b.head)) - int64(len(b.tail)) + return fmt.Sprintf("%s\n... [%d bytes truncated] ...\n%s", b.head, dropped, b.tail) +} diff --git a/cmd/mcp/buffer_test.go b/cmd/mcp/buffer_test.go new file mode 100644 index 000000000..16858b946 --- /dev/null +++ b/cmd/mcp/buffer_test.go @@ -0,0 +1,42 @@ +package mcp + +import ( + "strings" + "testing" +) + +func TestBoundedBuffer_NoTruncation(t *testing.T) { + b := NewBoundedBuffer(100) + _, _ = b.Write([]byte("hello")) + if got := b.String(); got != "hello" { + t.Fatalf("got %q", got) + } + if b.Truncated() { + t.Fatal("expected not truncated") + } +} + +func TestBoundedBuffer_TruncatesMiddle(t *testing.T) { + b := NewBoundedBuffer(20) + _, _ = b.Write([]byte(strings.Repeat("a", 50))) + s := b.String() + if !b.Truncated() { + t.Fatal("expected truncated") + } + if !strings.Contains(s, "bytes truncated") { + t.Fatalf("missing marker: %q", s) + } + if !strings.HasPrefix(s, "aaaa") || !strings.HasSuffix(s, "aaaa") { + t.Fatalf("head/tail not preserved: %q", s) + } +} + +func TestBoundedBuffer_MultipleWritesAccumulate(t *testing.T) { + b := NewBoundedBuffer(10) + for range 5 { + _, _ = b.Write([]byte("xxxx")) + } + if !b.Truncated() { + t.Fatal("expected truncated after 20 bytes into cap 10") + } +} From 1f19642b42f2cfe7ec528a094b25c5929057a496 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:46:26 -0500 Subject: [PATCH 04/48] refactor(workspace): move exec helpers into pkg/workspace for reuse Moves FindRunningContainer, LoadExecResult, ResolveExecWorkdir, ResolveDockerCommand, BuildExecEnv, ProbeContainerEnv, ContainerTarget, DefaultDockerCommand, and ContainerStatusRunning out of cmd/workspace into a new pkg/workspace/exec_helpers.go so the MCP server can call them without depending on the cmd tree. Re-exports are provided in cmd/workspace/exec.go for backward compatibility with existing callers in cmd/config and cmd/internal. --- cmd/config/apply.go | 5 +- cmd/config/read.go | 8 +- cmd/internal/runusercommands.go | 12 +- cmd/workspace/exec.go | 288 +++++--------------------------- pkg/workspace/exec_helpers.go | 242 +++++++++++++++++++++++++++ 5 files changed, 295 insertions(+), 260 deletions(-) create mode 100644 pkg/workspace/exec_helpers.go diff --git a/cmd/config/apply.go b/cmd/config/apply.go index 3cef7c5bf..b59316881 100644 --- a/cmd/config/apply.go +++ b/cmd/config/apply.go @@ -18,6 +18,7 @@ import ( "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/output" "github.com/devsy-org/devsy/pkg/types" + pkgworkspace "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -143,7 +144,7 @@ func (cmd *ApplyCmd) resolveDockerPath() string { if cmd.DockerPath != "" { return cmd.DockerPath } - return workspace.DefaultDockerCommand + return pkgworkspace.DefaultDockerCommand } func (cmd *ApplyCmd) inspectRunningContainer( @@ -159,7 +160,7 @@ func (cmd *ApplyCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace.ContainerStatusRunning) { + if !strings.EqualFold(containerDetails.State.Status, pkgworkspace.ContainerStatusRunning) { return nil, fmt.Errorf( "container %s is not running (status: %s)", cmd.Container, diff --git a/cmd/config/read.go b/cmd/config/read.go index 579889be6..c46369b35 100644 --- a/cmd/config/read.go +++ b/cmd/config/read.go @@ -8,10 +8,10 @@ import ( "path/filepath" "github.com/devsy-org/devsy/cmd/flags" - "github.com/devsy-org/devsy/cmd/workspace" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/devcontainer/metadata" "github.com/devsy-org/devsy/pkg/docker" + pkgworkspace "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" ) @@ -216,7 +216,7 @@ func (cmd *ReadCmd) resolveConfigFromContainer(ctx context.Context) ( string, error, ) { - dockerCommand := workspace.DefaultDockerCommand + dockerCommand := pkgworkspace.DefaultDockerCommand if cmd.DockerPath != "" { dockerCommand = cmd.DockerPath } @@ -262,8 +262,8 @@ func (cmd *ReadCmd) resolveConfigFromIDLabels(ctx context.Context) ( string, error, ) { - containerDetails, err := workspace.FindRunningContainer( - ctx, workspace.DefaultDockerCommand, "", cmd.IDLabels, + containerDetails, err := pkgworkspace.FindRunningContainer( + ctx, pkgworkspace.DefaultDockerCommand, "", cmd.IDLabels, ) if err != nil { return nil, "", err diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index c3981b1af..caa7e8e37 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -233,7 +233,7 @@ func (cmd *RunUserCommandsCmd) resolveDockerPath() string { if cmd.DockerPath != "" { return cmd.DockerPath } - return workspace.DefaultDockerCommand + return workspace2.DefaultDockerCommand } func (cmd *RunUserCommandsCmd) inspectRunningContainer( @@ -252,7 +252,7 @@ func (cmd *RunUserCommandsCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace.ContainerStatusRunning) { + if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { errMsg := fmt.Sprintf( "container %s is not running (status: %s)", cmd.ContainerID, @@ -332,12 +332,12 @@ func (cmd *RunUserCommandsCmd) resolveContainer( } workspaceConfig := client.WorkspaceConfig() - dockerCommand := workspace.ResolveDockerCommand(workspaceConfig) + dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig) if cmd.DockerPath != "" { dockerCommand = cmd.DockerPath } - containerDetails, err := workspace.FindRunningContainer( + containerDetails, err := workspace2.FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, ) if err != nil { @@ -345,7 +345,7 @@ func (cmd *RunUserCommandsCmd) resolveContainer( return nil, nil, err } - result := workspace.LoadExecResult(workspaceConfig, containerDetails) + result := workspace2.LoadExecResult(workspaceConfig, containerDetails) if result == nil || result.MergedConfig == nil { _ = devcconfig.WriteErrorJSON( os.Stderr, @@ -372,7 +372,7 @@ func (cmd *RunUserCommandsCmd) resolveContainer( Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, ContainerID: containerDetails.ID, EnvArgs: envArgs, - Workdir: workspace.ResolveExecWorkdir(result, client.Workspace()), + Workdir: workspace2.ResolveExecWorkdir(result, client.Workspace()), User: devcconfig.GetRemoteUser(result), } return params, result, nil diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index 363e6aea5..abc58f6e6 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -1,13 +1,9 @@ package workspace import ( - "bytes" "context" "fmt" - "io" - "maps" "os" - "path" "strings" "github.com/devsy-org/devsy/cmd/flags" @@ -17,17 +13,30 @@ import ( "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/output" - provider2 "github.com/devsy-org/devsy/pkg/provider" workspace2 "github.com/devsy-org/devsy/pkg/workspace" "github.com/spf13/cobra" "golang.org/x/term" ) +// Re-exports so that code within this package (including tests) can reference +// symbols that have moved to pkg/workspace without an import alias. const ( - DefaultDockerCommand = "docker" - ContainerStatusRunning = "running" + DefaultDockerCommand = workspace2.DefaultDockerCommand + ContainerStatusRunning = workspace2.ContainerStatusRunning ) +// ResolveDockerCommand is a re-export of the pkg/workspace function. +var ResolveDockerCommand = workspace2.ResolveDockerCommand + +// FindRunningContainer is a re-export of the pkg/workspace function. +var FindRunningContainer = workspace2.FindRunningContainer + +// LoadExecResult is a re-export of the pkg/workspace function. +var LoadExecResult = workspace2.LoadExecResult + +// ResolveExecWorkdir is a re-export of the pkg/workspace function. +var ResolveExecWorkdir = workspace2.ResolveExecWorkdir + type ExecCmd struct { *flags.GlobalFlags @@ -155,27 +164,27 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { } workspaceConfig := client.WorkspaceConfig() - dockerCommand := ResolveDockerCommand(workspaceConfig) + dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig) - containerDetails, err := FindRunningContainer( + containerDetails, err := workspace2.FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, ) if err != nil { return err } - result := LoadExecResult(workspaceConfig, containerDetails) - workdir := ResolveExecWorkdir(result, client.Workspace()) + result := workspace2.LoadExecResult(workspaceConfig, containerDetails) + workdir := workspace2.ResolveExecWorkdir(result, client.Workspace()) user := devcconfig.GetRemoteUser(result) userEnvProbe := resolveUserEnvProbe(result, cmd.DefaultUserEnvProbe) - target := containerTarget{ - helper: &docker.DockerHelper{DockerCommand: dockerCommand}, - containerID: containerDetails.ID, - user: user, + target := workspace2.ContainerTarget{ + Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, + ContainerID: containerDetails.ID, + User: user, } - probedEnv := probeContainerEnv(ctx, target, userEnvProbe) - envMap := buildExecEnv(result, cmd.RemoteEnv, probedEnv) + probedEnv := workspace2.ProbeContainerEnv(ctx, target, userEnvProbe) + envMap := workspace2.BuildExecEnv(result, cmd.RemoteEnv, probedEnv) mode, err := output.ResolveMode(cmd.ResultFormat) if err != nil { @@ -206,7 +215,7 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { } func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error { - dockerCommand := DefaultDockerCommand + dockerCommand := workspace2.DefaultDockerCommand if cmd.DockerPath != "" { dockerCommand = cmd.DockerPath } @@ -221,7 +230,7 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, ContainerStatusRunning) { + if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { return fmt.Errorf( "container %s is not running (status: %s)", cmd.ContainerID, @@ -230,13 +239,13 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error } userEnvProbe := cmd.DefaultUserEnvProbe - target := containerTarget{ - helper: helper, - containerID: containerDetails.ID, - user: "", + target := workspace2.ContainerTarget{ + Helper: helper, + ContainerID: containerDetails.ID, + User: "", } - probedEnv := probeContainerEnv(ctx, target, userEnvProbe) - envMap := buildExecEnv(nil, cmd.RemoteEnv, probedEnv) + probedEnv := workspace2.ProbeContainerEnv(ctx, target, userEnvProbe) + envMap := workspace2.BuildExecEnv(nil, cmd.RemoteEnv, probedEnv) workdir := containerDetails.Config.WorkingDir @@ -277,83 +286,6 @@ func (cmd *ExecCmd) validateRemoteEnv() error { return nil } -func ResolveDockerCommand( - workspace *provider2.Workspace, -) string { - if workspace == nil || workspace.Context == "" { - return DefaultDockerCommand - } - - providerConfig, err := provider2.LoadProviderConfig( - workspace.Context, - workspace.Provider.Name, - ) - if err != nil { - log.Debugf("Failed to load provider config, defaulting to 'docker': %v", err) - return DefaultDockerCommand - } - - if providerConfig.Agent.Docker.Path != "" { - if expanded := os.ExpandEnv(providerConfig.Agent.Docker.Path); expanded != "" { - return expanded - } - } - - return DefaultDockerCommand -} - -func FindRunningContainer( - ctx context.Context, - dockerCommand string, - workspaceID string, - idLabels []string, -) (*devcconfig.ContainerDetails, error) { - dockerHelper := &docker.DockerHelper{ - DockerCommand: dockerCommand, - } - - labels := devcconfig.GetIDLabels(workspaceID, idLabels) - container, err := dockerHelper.FindDevContainer(ctx, labels) - if err != nil { - return nil, fmt.Errorf("find container: %w", err) - } - if container == nil { - return nil, fmt.Errorf( - "no running container found for workspace %q", - workspaceID, - ) - } - - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { - return nil, fmt.Errorf( - "container %s is not running (status: %s)", - container.ID, - container.State.Status, - ) - } - - return container, nil -} - -func LoadExecResult( - workspaceConfig *provider2.Workspace, - containerDetails *devcconfig.ContainerDetails, -) *devcconfig.Result { - if workspaceConfig == nil || workspaceConfig.Context == "" || workspaceConfig.ID == "" { - return nil - } - - result, err := provider2.LoadWorkspaceResult(workspaceConfig.Context, workspaceConfig.ID) - if err != nil { - log.Warnf("Error loading workspace result: %v", err) - return nil - } - if result != nil { - result.ContainerDetails = containerDetails - } - return result -} - func resolveUserEnvProbe(result *devcconfig.Result, cliOverride string) string { if cliOverride != "" { return cliOverride @@ -364,57 +296,8 @@ func resolveUserEnvProbe(result *devcconfig.Result, cliOverride string) string { return "" } -func ResolveExecWorkdir(result *devcconfig.Result, workspaceName string) string { - if result != nil && result.MergedConfig != nil && result.MergedConfig.WorkspaceFolder != "" { - return result.MergedConfig.WorkspaceFolder - } - return path.Join("/workspaces", workspaceName) -} - -func buildExecEnv( - result *devcconfig.Result, - cliEnv []string, - probedEnv map[string]string, -) map[string]string { - env := make(map[string]string, len(probedEnv)) - maps.Copy(env, probedEnv) - - if result != nil { - applyRemoteEnv(env, mergedRemoteEnv(result)) - } - - for _, e := range cliEnv { - if k, v, ok := strings.Cut(e, "="); ok { - env[k] = v - } - } - - return env -} - -func mergedRemoteEnv(result *devcconfig.Result) map[string]*string { - merged := map[string]*string{} - if result.MergedConfig != nil { - maps.Copy(merged, result.MergedConfig.RemoteEnv) - } - if result.DevContainerConfigWithPath != nil && result.DevContainerConfigWithPath.Config != nil { - maps.Copy(merged, result.DevContainerConfigWithPath.Config.RemoteEnv) - } - return merged -} - -func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { - for k, v := range remoteEnv { - if v == nil { - delete(env, k) - } else { - env[k] = *v - } - } -} - type execOpts struct { - target containerTarget + target workspace2.ContainerTarget workdir string envMap map[string]string } @@ -430,106 +313,15 @@ func (cmd *ExecCmd) execInContainer(ctx context.Context, opts execOpts, args []s if opts.workdir != "" { execArgs = append(execArgs, "--workdir", opts.workdir) } - if opts.target.user != "" { - execArgs = append(execArgs, "--user", opts.target.user) + if opts.target.User != "" { + execArgs = append(execArgs, "--user", opts.target.User) } - execArgs = append(execArgs, opts.target.containerID) + execArgs = append(execArgs, opts.target.ContainerID) execArgs = append(execArgs, args...) redacted := strings.Join(redactExecArgs(execArgs), " ") - log.Debugf("Executing in container: %s %s", opts.target.helper.DockerCommand, redacted) - return opts.target.helper.Run(ctx, execArgs, os.Stdin, os.Stdout, os.Stderr) -} - -func parseEnvOutput(out []byte, sep byte) map[string]string { - entries := bytes.Split(out, []byte{sep}) - env := make(map[string]string, len(entries)) - for _, e := range entries { - if len(e) == 0 { - continue - } - name, value, ok := bytes.Cut(e, []byte{'='}) - if !ok || len(name) == 0 { - continue - } - env[string(name)] = string(value) - } - delete(env, "PWD") - return env -} - -type containerTarget struct { - helper *docker.DockerHelper - containerID string - user string -} - -func probeContainerEnv( - ctx context.Context, - target containerTarget, - probe string, -) map[string]string { - userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) - if err != nil { - log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) - userEnvProbe = devcconfig.DefaultUserEnvProbe - } - if userEnvProbe == devcconfig.NoneProbe { - return map[string]string{} - } - - shellFlag := probeShellFlag(userEnvProbe) - - out, sep, err := runProbeCommand(ctx, target, shellFlag) - if err != nil { - log.Warnf("Failed to probe user env: %v", err) - return map[string]string{} - } - return parseEnvOutput(out, sep) -} - -func probeShellFlag(probe devcconfig.UserEnvProbe) string { - switch probe { - case devcconfig.LoginInteractiveShellProbe: - return "-lic" - case devcconfig.LoginShellProbe: - return "-lc" - case devcconfig.InteractiveShellProbe: - return "-ic" - default: - return "-c" - } -} - -func runProbeCommand( - ctx context.Context, - target containerTarget, - shellFlag string, -) ([]byte, byte, error) { - args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") - var stdout bytes.Buffer - err := target.helper.Run(ctx, args, nil, &stdout, io.Discard) - if err == nil { - return stdout.Bytes(), 0, nil - } - - log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) - args = buildProbeArgs(target, shellFlag, "printenv") - stdout.Reset() - err = target.helper.Run(ctx, args, nil, &stdout, io.Discard) - if err != nil { - return nil, 0, fmt.Errorf("probe user env: %w", err) - } - return stdout.Bytes(), '\n', nil -} - -func buildProbeArgs(target containerTarget, shellFlag string, cmd string) []string { - args := []string{"exec"} - if target.user != "" { - args = append(args, "--user", target.user) - } - args = append(args, target.containerID, "sh", shellFlag, cmd) - return args + log.Debugf("Executing in container: %s %s", opts.target.Helper.DockerCommand, redacted) + return opts.target.Helper.Run(ctx, execArgs, os.Stdin, os.Stdout, os.Stderr) } func redactExecArgs(args []string) []string { diff --git a/pkg/workspace/exec_helpers.go b/pkg/workspace/exec_helpers.go new file mode 100644 index 000000000..d7a6e68c7 --- /dev/null +++ b/pkg/workspace/exec_helpers.go @@ -0,0 +1,242 @@ +package workspace + +import ( + "bytes" + "context" + "fmt" + "io" + "maps" + "os" + "path" + "strings" + + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/log" + provider2 "github.com/devsy-org/devsy/pkg/provider" +) + +const ( + DefaultDockerCommand = "docker" + ContainerStatusRunning = "running" +) + +// ContainerTarget bundles the docker helper, container ID, and user for exec operations. +type ContainerTarget struct { + Helper *docker.DockerHelper + ContainerID string + User string +} + +func ResolveDockerCommand( + workspace *provider2.Workspace, +) string { + if workspace == nil || workspace.Context == "" { + return DefaultDockerCommand + } + + providerConfig, err := provider2.LoadProviderConfig( + workspace.Context, + workspace.Provider.Name, + ) + if err != nil { + log.Debugf("Failed to load provider config, defaulting to 'docker': %v", err) + return DefaultDockerCommand + } + + if providerConfig.Agent.Docker.Path != "" { + if expanded := os.ExpandEnv(providerConfig.Agent.Docker.Path); expanded != "" { + return expanded + } + } + + return DefaultDockerCommand +} + +func FindRunningContainer( + ctx context.Context, + dockerCommand string, + workspaceID string, + idLabels []string, +) (*devcconfig.ContainerDetails, error) { + dockerHelper := &docker.DockerHelper{ + DockerCommand: dockerCommand, + } + + labels := devcconfig.GetIDLabels(workspaceID, idLabels) + container, err := dockerHelper.FindDevContainer(ctx, labels) + if err != nil { + return nil, fmt.Errorf("find container: %w", err) + } + if container == nil { + return nil, fmt.Errorf( + "no running container found for workspace %q", + workspaceID, + ) + } + + if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + return nil, fmt.Errorf( + "container %s is not running (status: %s)", + container.ID, + container.State.Status, + ) + } + + return container, nil +} + +func LoadExecResult( + workspaceConfig *provider2.Workspace, + containerDetails *devcconfig.ContainerDetails, +) *devcconfig.Result { + if workspaceConfig == nil || workspaceConfig.Context == "" || workspaceConfig.ID == "" { + return nil + } + + result, err := provider2.LoadWorkspaceResult(workspaceConfig.Context, workspaceConfig.ID) + if err != nil { + log.Warnf("Error loading workspace result: %v", err) + return nil + } + if result != nil { + result.ContainerDetails = containerDetails + } + return result +} + +func ResolveExecWorkdir(result *devcconfig.Result, workspaceName string) string { + if result != nil && result.MergedConfig != nil && result.MergedConfig.WorkspaceFolder != "" { + return result.MergedConfig.WorkspaceFolder + } + return path.Join("/workspaces", workspaceName) +} + +// BuildExecEnv merges probed env, result remote env, and caller-supplied env slices. +func BuildExecEnv( + result *devcconfig.Result, + cliEnv []string, + probedEnv map[string]string, +) map[string]string { + env := make(map[string]string, len(probedEnv)) + maps.Copy(env, probedEnv) + + if result != nil { + applyRemoteEnv(env, mergedRemoteEnv(result)) + } + + for _, e := range cliEnv { + if k, v, ok := strings.Cut(e, "="); ok { + env[k] = v + } + } + + return env +} + +func mergedRemoteEnv(result *devcconfig.Result) map[string]*string { + merged := map[string]*string{} + if result.MergedConfig != nil { + maps.Copy(merged, result.MergedConfig.RemoteEnv) + } + if result.DevContainerConfigWithPath != nil && result.DevContainerConfigWithPath.Config != nil { + maps.Copy(merged, result.DevContainerConfigWithPath.Config.RemoteEnv) + } + return merged +} + +func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { + for k, v := range remoteEnv { + if v == nil { + delete(env, k) + } else { + env[k] = *v + } + } +} + +// ProbeContainerEnv probes the container's environment via the given probe strategy. +func ProbeContainerEnv( + ctx context.Context, + target ContainerTarget, + probe string, +) map[string]string { + userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) + if err != nil { + log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) + userEnvProbe = devcconfig.DefaultUserEnvProbe + } + if userEnvProbe == devcconfig.NoneProbe { + return map[string]string{} + } + + shellFlag := probeShellFlag(userEnvProbe) + + out, sep, err := runProbeCommand(ctx, target, shellFlag) + if err != nil { + log.Warnf("Failed to probe user env: %v", err) + return map[string]string{} + } + return parseEnvOutput(out, sep) +} + +func probeShellFlag(probe devcconfig.UserEnvProbe) string { + switch probe { + case devcconfig.LoginInteractiveShellProbe: + return "-lic" + case devcconfig.LoginShellProbe: + return "-lc" + case devcconfig.InteractiveShellProbe: + return "-ic" + default: + return "-c" + } +} + +func runProbeCommand( + ctx context.Context, + target ContainerTarget, + shellFlag string, +) ([]byte, byte, error) { + args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") + var stdout bytes.Buffer + err := target.Helper.Run(ctx, args, nil, &stdout, io.Discard) + if err == nil { + return stdout.Bytes(), 0, nil + } + + log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) + args = buildProbeArgs(target, shellFlag, "printenv") + stdout.Reset() + err = target.Helper.Run(ctx, args, nil, &stdout, io.Discard) + if err != nil { + return nil, 0, fmt.Errorf("probe user env: %w", err) + } + return stdout.Bytes(), '\n', nil +} + +func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []string { + args := []string{"exec"} + if target.User != "" { + args = append(args, "--user", target.User) + } + args = append(args, target.ContainerID, "sh", shellFlag, cmd) + return args +} + +func parseEnvOutput(out []byte, sep byte) map[string]string { + entries := bytes.Split(out, []byte{sep}) + env := make(map[string]string, len(entries)) + for _, e := range entries { + if len(e) == 0 { + continue + } + name, value, ok := bytes.Cut(e, []byte{'='}) + if !ok || len(name) == 0 { + continue + } + env[string(name)] = string(value) + } + delete(env, "PWD") + return env +} From baad611d0da7b9392e0d2b89baa4dbe4b426578a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:49:22 -0500 Subject: [PATCH 05/48] feat(workspace): add ExecOneShot capture-based exec entry point Adds ExecOneShot to pkg/workspace for programmatic (non-TTY) command execution inside workspace containers. Supports configurable timeouts with clamping, captures stdout/stderr via io.Writer, and returns a structured result with exit code, duration, and timeout status. --- pkg/workspace/exec_oneshot.go | 219 +++++++++++++++++++++++++++++ pkg/workspace/exec_oneshot_test.go | 44 ++++++ 2 files changed, 263 insertions(+) create mode 100644 pkg/workspace/exec_oneshot.go create mode 100644 pkg/workspace/exec_oneshot_test.go diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go new file mode 100644 index 000000000..7426c9554 --- /dev/null +++ b/pkg/workspace/exec_oneshot.go @@ -0,0 +1,219 @@ +package workspace + +import ( + "context" + "errors" + "fmt" + "io" + "os/exec" + "time" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer" + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/platform" +) + +// ExecOneShotOptions configures a single non-interactive command execution +// inside a workspace's running container. +type ExecOneShotOptions struct { + WorkspaceName string + Command []string + Workdir string + Env map[string]string + TimeoutSeconds int + TimeoutSecondsDefault int + TimeoutSecondsMax int + Owner platform.OwnerFilter + Context string + Provider string + Stdout io.Writer + Stderr io.Writer +} + +// ExecOneShotResult is the structured outcome of an exec. +type ExecOneShotResult struct { + ExitCode int + DurationMS int64 + TimedOut bool + TimeoutSecond int + Clamped bool +} + +// ResolveTimeout returns the effective timeout and whether the caller's +// requested value was clamped down by Max. Precedence: +// 1. TimeoutSeconds if > 0 +// 2. TimeoutSecondsDefault if > 0 +// 3. fallbackDefault +// +// The result is clamped by TimeoutSecondsMax if > 0. +func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, bool) { + want := o.TimeoutSeconds + if want <= 0 { + want = o.TimeoutSecondsDefault + } + if want <= 0 { + want = fallbackDefault + } + if o.TimeoutSecondsMax > 0 && want > o.TimeoutSecondsMax { + return time.Duration(o.TimeoutSecondsMax) * time.Second, true + } + return time.Duration(want) * time.Second, false +} + +// ExecOneShot runs Command inside the workspace's container, captures +// stdout/stderr via the provided writers, and returns a structured result. +// It never reads stdin and never allocates a TTY. +func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResult, error) { + if opts.WorkspaceName == "" { + return nil, fmt.Errorf("WorkspaceName is required") + } + if len(opts.Command) == 0 { + return nil, fmt.Errorf("command is required") + } + + timeout, clamped := opts.ResolveTimeout(300) + execCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + resolved, err := resolveExecTarget(execCtx, opts) + if err != nil { + return nil, err + } + + start := time.Now() + exitCode, runErr := runCapture(execCtx, captureArgs{ + target: resolved.target, + workdir: resolved.workdir, + env: resolved.envMap, + command: opts.Command, + stdout: opts.Stdout, + stderr: opts.Stderr, + }) + duration := time.Since(start) + + res := &ExecOneShotResult{ + ExitCode: exitCode, + DurationMS: duration.Milliseconds(), + Clamped: clamped, + TimeoutSecond: int(timeout.Seconds()), + } + if errors.Is(execCtx.Err(), context.DeadlineExceeded) { + res.TimedOut = true + res.ExitCode = -1 + return res, nil + } + if runErr != nil { + return res, runErr + } + return res, nil +} + +// resolvedExecTarget bundles the resolved container target, workdir, and env for an exec. +type resolvedExecTarget struct { + target ContainerTarget + workdir string + envMap map[string]string +} + +// resolveExecTarget resolves the container target, workdir, and env map from options. +func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedExecTarget, error) { + devsyConfig, err := config.LoadConfig(opts.Context, opts.Provider) + if err != nil { + return resolvedExecTarget{}, fmt.Errorf("load config: %w", err) + } + + client, err := Get(ctx, GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{opts.WorkspaceName}, + Owner: opts.Owner, + }) + if err != nil { + return resolvedExecTarget{}, fmt.Errorf("resolve workspace: %w", err) + } + + workspaceConfig := client.WorkspaceConfig() + dockerCommand := ResolveDockerCommand(workspaceConfig) + + containerDetails, err := FindRunningContainer( + ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), nil, + ) + if err != nil { + return resolvedExecTarget{}, err + } + + execResult := LoadExecResult(workspaceConfig, containerDetails) + workdir := opts.Workdir + if workdir == "" { + workdir = ResolveExecWorkdir(execResult, client.Workspace()) + } + + user := "" + if execResult != nil { + user = devcconfig.GetRemoteUser(execResult) + } + + target := ContainerTarget{ + Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, + ContainerID: containerDetails.ID, + User: user, + } + probedEnv := ProbeContainerEnv(ctx, target, "") + envSlice := envMapToSlice(opts.Env) + envMap := BuildExecEnv(execResult, envSlice, probedEnv) + + return resolvedExecTarget{target: target, workdir: workdir, envMap: envMap}, nil +} + +// captureArgs bundles the arguments for runCapture. +type captureArgs struct { + target ContainerTarget + workdir string + env map[string]string + command []string + stdout io.Writer + stderr io.Writer +} + +func runCapture(ctx context.Context, args captureArgs) (int, error) { + execArgs := []string{"exec", "-i"} + for k, v := range args.env { + execArgs = append(execArgs, "-e", k+"="+v) + } + if args.workdir != "" { + execArgs = append(execArgs, "--workdir", args.workdir) + } + if args.target.User != "" { + execArgs = append(execArgs, "--user", args.target.User) + } + execArgs = append(execArgs, args.target.ContainerID) + execArgs = append(execArgs, args.command...) + + stdout := args.stdout + if stdout == nil { + stdout = io.Discard + } + stderr := args.stderr + if stderr == nil { + stderr = io.Discard + } + + err := args.target.Helper.Run(ctx, execArgs, nil, stdout, stderr) + if err == nil { + return 0, nil + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode(), nil + } + return -1, err +} + +func envMapToSlice(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k, v := range m { + out = append(out, k+"="+v) + } + return out +} diff --git a/pkg/workspace/exec_oneshot_test.go b/pkg/workspace/exec_oneshot_test.go new file mode 100644 index 000000000..58c6bc993 --- /dev/null +++ b/pkg/workspace/exec_oneshot_test.go @@ -0,0 +1,44 @@ +package workspace + +import ( + "testing" +) + +func TestExecOneShotOptions_ResolveTimeout_Clamp(t *testing.T) { + opts := ExecOneShotOptions{ + TimeoutSeconds: 10000, + TimeoutSecondsMax: 60, + } + clamped, wasClamped := opts.ResolveTimeout(5) + if !wasClamped { + t.Fatal("expected clamp=true") + } + if clamped.Seconds() != 60 { + t.Fatalf("expected 60s, got %s", clamped) + } +} + +func TestExecOneShotOptions_ResolveTimeout_Default(t *testing.T) { + opts := ExecOneShotOptions{TimeoutSecondsMax: 600} + clamped, wasClamped := opts.ResolveTimeout(300) + if wasClamped { + t.Fatal("expected clamp=false") + } + if clamped.Seconds() != 300 { + t.Fatalf("expected 300s, got %s", clamped) + } +} + +func TestExecOneShotOptions_ResolveTimeout_CallerExplicit(t *testing.T) { + opts := ExecOneShotOptions{ + TimeoutSeconds: 120, + TimeoutSecondsMax: 600, + } + clamped, wasClamped := opts.ResolveTimeout(300) + if wasClamped { + t.Fatal("expected clamp=false") + } + if clamped.Seconds() != 120 { + t.Fatalf("expected 120s, got %s", clamped) + } +} From ddfe75e9ea4254dae8ef1c4631aac1e00bf0fe36 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:51:23 -0500 Subject: [PATCH 06/48] feat(mcp): map CLI errors to MCP structured payloads --- cmd/mcp/errors.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 cmd/mcp/errors.go diff --git a/cmd/mcp/errors.go b/cmd/mcp/errors.go new file mode 100644 index 000000000..1691c4297 --- /dev/null +++ b/cmd/mcp/errors.go @@ -0,0 +1,42 @@ +package mcp + +import ( + "errors" + + cliErrors "github.com/devsy-org/devsy/pkg/errors" + "github.com/devsy-org/devsy/pkg/workspace" +) + +// ErrorPayload is the JSON shape attached to MCP tool errors so the agent gets +// the same structured information the Devsy CLI shows humans (code, hint, doc URL). +type ErrorPayload struct { + Code string `json:"code"` + Message string `json:"message"` + Hint string `json:"hint,omitempty"` + DocURL string `json:"doc_url,omitempty"` +} + +// ClassifyError converts any error returned by an MCP handler into a structured +// payload using the same classifier the CLI uses. +func ClassifyError(err error) ErrorPayload { + if err == nil { + return ErrorPayload{} + } + if errors.Is(err, workspace.ErrWorkspaceNotFound) { + return ErrorPayload{ + Code: "workspace_not_found", + Message: err.Error(), + } + } + classified := cliErrors.Classify(err, cliErrors.ClassifyContext{}) + code := "internal_error" + if classified.Code != "" { + code = string(classified.Code) + } + return ErrorPayload{ + Code: code, + Message: classified.Message, + Hint: classified.Hint, + DocURL: classified.DocURL, + } +} From 8289db14d12ec2f8f5da653f3df0686d6dcea580 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 16:56:17 -0500 Subject: [PATCH 07/48] feat(mcp): register workspace_list and workspace_status tools Wire up the MCP server with stdio transport in ServeCmd.Run and register workspace_list / workspace_status tools; add stub registration functions for exec and provider tools (Tasks 7-9) so the package compiles. --- cmd/mcp/serve.go | 21 ++++++- cmd/mcp/tools_exec.go | 6 ++ cmd/mcp/tools_provider.go | 9 +++ cmd/mcp/tools_workspace.go | 112 +++++++++++++++++++++++++++++++++++++ go.mod | 4 ++ go.sum | 8 +++ 6 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 cmd/mcp/tools_exec.go create mode 100644 cmd/mcp/tools_provider.go create mode 100644 cmd/mcp/tools_workspace.go diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index 281a3f06e..ae1adcc97 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -6,6 +6,8 @@ import ( "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/version" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/cobra" ) @@ -42,7 +44,20 @@ func NewServeCmd(globalFlags *flags.GlobalFlags) *cobra.Command { func (cmd *ServeCmd) Run(ctx context.Context) error { log.Debugf("starting MCP server (timeout default=%s max=%s cap=%dB)", cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) - // Tool registration and stdio loop are added in later tasks. - _ = ctx - return nil + + server := sdkmcp.NewServer(&sdkmcp.Implementation{ + Name: "devsy", + Version: version.GetVersion(), + }, nil) + + cmd.registerTools(server) + + transport := &sdkmcp.StdioTransport{} + return server.Run(ctx, transport) +} + +func (cmd *ServeCmd) registerTools(s *sdkmcp.Server) { + registerWorkspaceTools(s, cmd.GlobalFlags) + registerExecTool(s, cmd) + registerProviderTools(s, cmd.GlobalFlags) } diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go new file mode 100644 index 000000000..a2a10b77a --- /dev/null +++ b/cmd/mcp/tools_exec.go @@ -0,0 +1,6 @@ +package mcp + +import sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + +// registerExecTool is filled in by Task 8. +func registerExecTool(_ *sdkmcp.Server, _ *ServeCmd) {} diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go new file mode 100644 index 000000000..acdffb979 --- /dev/null +++ b/cmd/mcp/tools_provider.go @@ -0,0 +1,9 @@ +package mcp + +import ( + "github.com/devsy-org/devsy/cmd/flags" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// registerProviderTools is filled in by Task 9. +func registerProviderTools(_ *sdkmcp.Server, _ *flags.GlobalFlags) {} diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go new file mode 100644 index 000000000..55b846d1a --- /dev/null +++ b/cmd/mcp/tools_workspace.go @@ -0,0 +1,112 @@ +package mcp + +import ( + "context" + "fmt" + "time" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/workspace" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +type workspaceSummary struct { + Name string `json:"name"` + Provider string `json:"provider,omitempty"` + IDE string `json:"ide,omitempty"` + Source string `json:"source,omitempty"` + LastUsed string `json:"last_used,omitempty"` +} + +type ( + workspaceListInput struct{} + workspaceListOutput struct { + Workspaces []workspaceSummary `json:"workspaces"` + } +) + +type workspaceStatusInput struct { + Name string `json:"name" jsonschema:"required"` +} + +func registerWorkspaceTools(s *sdkmcp.Server, g *flags.GlobalFlags) { + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_list", + Description: "List all Devsy workspaces.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ workspaceListInput, + ) (*sdkmcp.CallToolResult, workspaceListOutput, error) { + out, err := handleWorkspaceList(ctx, g) + if err != nil { + return errorResult(err), workspaceListOutput{}, nil + } + return nil, out, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_status", + Description: "Get detailed status for a workspace by name.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in workspaceStatusInput) (*sdkmcp.CallToolResult, any, error) { + out, err := handleWorkspaceStatus(ctx, g, in.Name) + if err != nil { + return errorResult(err), nil, nil + } + return nil, out, nil + }) + + registerWorkspaceLifecycleTools(s, g) +} + +func handleWorkspaceList(ctx context.Context, g *flags.GlobalFlags) (workspaceListOutput, error) { + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return workspaceListOutput{}, err + } + entries, err := workspace.List(ctx, devsyConfig, false, g.Owner) + if err != nil { + return workspaceListOutput{}, err + } + summaries := make([]workspaceSummary, 0, len(entries)) + for _, e := range entries { + summaries = append(summaries, workspaceSummary{ + Name: e.ID, + Provider: e.Provider.Name, + IDE: e.IDE.Name, + Source: e.Source.String(), + LastUsed: e.LastUsedTimestamp.Format(time.RFC3339), + }) + } + return workspaceListOutput{Workspaces: summaries}, nil +} + +func handleWorkspaceStatus(ctx context.Context, g *flags.GlobalFlags, name string) (any, error) { + if name == "" { + return nil, fmt.Errorf("name is required") + } + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return nil, err + } + client, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{name}, + Owner: g.Owner, + }) + if err != nil { + return nil, err + } + return client.WorkspaceConfig(), nil +} + +// errorResult builds an isError CallToolResult carrying our structured payload. +func errorResult(err error) *sdkmcp.CallToolResult { + payload := ClassifyError(err) + return &sdkmcp.CallToolResult{ + IsError: true, + Content: []sdkmcp.Content{&sdkmcp.TextContent{Text: payload.Message}}, + StructuredContent: payload, + } +} + +// Stub — Task 7 replaces this. +func registerWorkspaceLifecycleTools(_ *sdkmcp.Server, _ *flags.GlobalFlags) {} diff --git a/go.mod b/go.mod index 6d794d910..3e858a6b2 100644 --- a/go.mod +++ b/go.mod @@ -186,6 +186,7 @@ require ( github.com/google/gnostic-models v0.7.1 // indirect github.com/google/go-github/v74 v74.0.0 // indirect github.com/google/go-querystring v1.1.0 // indirect + github.com/google/jsonschema-go v0.4.3 // indirect github.com/google/pprof v0.0.0-20260115054156-294ebfa9ad83 // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect @@ -249,6 +250,8 @@ require ( github.com/safchain/ethtool v0.3.0 // indirect github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect github.com/secure-systems-lab/go-securesystemslib v0.10.0 // indirect + github.com/segmentio/asm v1.1.3 // indirect + github.com/segmentio/encoding v0.5.4 // indirect github.com/shibumi/go-pathspec v1.3.0 // indirect github.com/sirupsen/logrus v1.9.4 // indirect github.com/stoewer/go-strcase v1.3.1 // indirect @@ -270,6 +273,7 @@ require ( github.com/x448/float16 v0.8.4 // indirect github.com/xhit/go-str2duration/v2 v2.1.0 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect gitlab.com/gitlab-org/api/client-go v1.9.1 // indirect go.etcd.io/etcd/api/v3 v3.6.5 // indirect diff --git a/go.sum b/go.sum index cf90afc2b..6d47dd858 100644 --- a/go.sum +++ b/go.sum @@ -386,6 +386,8 @@ github.com/google/go-tpm v0.9.4/go.mod h1:h9jEsEECg7gtLis0upRBQU+GhYVH6jMjrFxI8u github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/jsonschema-go v0.4.3 h1:/DBOLZTfDow7pe2GmaJNhltueGTtDKICi8V8p+DQPd0= +github.com/google/jsonschema-go v0.4.3/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/nftables v0.2.1-0.20240414091927-5e242ec57806 h1:wG8RYIyctLhdFk6Vl1yPGtSRtwGpVkWyZww1OCil2MI= github.com/google/nftables v0.2.1-0.20240414091927-5e242ec57806/go.mod h1:Beg6V6zZ3oEn0JuiUQ4wqwuyqqzasOltcoXPtgLbFp4= github.com/google/pprof v0.0.0-20260115054156-294ebfa9ad83 h1:z2ogiKUYzX5Is6zr/vP9vJGqPwcdqsWjOt+V8J7+bTc= @@ -609,6 +611,10 @@ github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 h1:KRzFb2m7YtdldCEkzs6KqmJw4nqEV github.com/santhosh-tekuri/jsonschema/v6 v6.0.2/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU= github.com/secure-systems-lab/go-securesystemslib v0.10.0 h1:l+H5ErcW0PAehBNrBxoGv1jjNpGYdZ9RcheFkB2WI14= github.com/secure-systems-lab/go-securesystemslib v0.10.0/go.mod h1:MRKONWmRoFzPNQ9USRF9i1mc7MvAVvF1LlW8X5VWDvk= +github.com/segmentio/asm v1.1.3 h1:WM03sfUOENvvKexOLp+pCqgb/WDjsi7EK8gIsICtzhc= +github.com/segmentio/asm v1.1.3/go.mod h1:Ld3L4ZXGNcSLRg4JBsZ3//1+f/TjYl0Mzen/DQy1EJg= +github.com/segmentio/encoding v0.5.4 h1:OW1VRern8Nw6ITAtwSZ7Idrl3MXCFwXHPgqESYfvNt0= +github.com/segmentio/encoding v0.5.4/go.mod h1:HS1ZKa3kSN32ZHVZ7ZLPLXWvOVIiZtyJnO1gPH1sKt0= github.com/shibumi/go-pathspec v1.3.0 h1:QUyMZhFo0Md5B8zV8x2tesohbb5kfbpTi9rBnKh5dkI= github.com/shibumi/go-pathspec v1.3.0/go.mod h1:Xutfslp817l2I1cZvgcfeMQJG5QnU2lh5tVaaMCl3jE= github.com/shirou/gopsutil/v4 v4.26.4 h1:B4SXVbcwTyrocPHEmWBC4uCYr4Xcu3MK1TXqbprAOWY= @@ -711,6 +717,8 @@ github.com/xiang90/probing v0.0.0-20221125231312-a49e3df8f510 h1:S2dVYn90KE98chq github.com/xiang90/probing v0.0.0-20221125231312-a49e3df8f510/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= From 40f595448414a25808850f3f5b2ab27d98105b01 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:00:15 -0500 Subject: [PATCH 08/48] feat(mcp): add workspace start/stop/delete/create tools --- cmd/mcp/tools_workspace.go | 148 ++++++++++++++++++++++++++++++++++++- 1 file changed, 146 insertions(+), 2 deletions(-) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 55b846d1a..858f2d6ed 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -6,6 +6,8 @@ import ( "time" "github.com/devsy-org/devsy/cmd/flags" + up "github.com/devsy-org/devsy/cmd/workspace/up" + client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" @@ -108,5 +110,147 @@ func errorResult(err error) *sdkmcp.CallToolResult { } } -// Stub — Task 7 replaces this. -func registerWorkspaceLifecycleTools(_ *sdkmcp.Server, _ *flags.GlobalFlags) {} +type nameInput struct { + Name string `json:"name" jsonschema:"required"` + Force bool `json:"force,omitempty"` +} + +type opOK struct { + OK bool `json:"ok"` + Message string `json:"message,omitempty"` +} + +type createInput struct { + Source string `json:"source" jsonschema:"required"` + Name string `json:"name,omitempty"` + Provider string `json:"provider,omitempty"` + IDE string `json:"ide,omitempty"` + DevcontainerPath string `json:"devcontainer_path,omitempty"` +} + +func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_start", + Description: "Start (or resume) an existing workspace by name.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := startWorkspace(ctx, g, in.Name); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_stop", + Description: "Stop a running workspace by name.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := stopWorkspace(ctx, g, in.Name); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_delete", + Description: "Delete a workspace by name. Pass force=true to force-delete even if not found remotely.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := deleteWorkspace(ctx, g, in.Name, in.Force); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_create", + Description: "Create and start a new workspace from a git URL, local path, or container image.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in createInput) (*sdkmcp.CallToolResult, any, error) { + out, err := createWorkspace(ctx, g, in) + if err != nil { + return errorResult(err), nil, nil + } + return nil, out, nil + }) +} + +func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { + return runUp(ctx, g, createInput{Source: name}) +} + +func stopWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return err + } + client, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{name}, + Owner: g.Owner, + }) + if err != nil { + return err + } + return client.Stop(ctx, client2.StopOptions{}) +} + +func deleteWorkspace(ctx context.Context, g *flags.GlobalFlags, name string, force bool) error { + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return err + } + _, err = workspace.Delete(ctx, workspace.DeleteOptions{ + DevsyConfig: devsyConfig, + Args: []string{name}, + Force: force, + Owner: g.Owner, + }) + return err +} + +func createWorkspace(ctx context.Context, g *flags.GlobalFlags, in createInput) (any, error) { + if in.Source == "" { + return nil, fmt.Errorf("source is required") + } + if err := runUp(ctx, g, in); err != nil { + return nil, err + } + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return nil, err + } + lookup := in.Name + if lookup == "" { + lookup = in.Source + } + client, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{lookup}, + Owner: g.Owner, + }) + if err != nil { + return nil, err + } + return client.WorkspaceConfig(), nil +} + +func runUp(ctx context.Context, g *flags.GlobalFlags, in createInput) error { + args := []string{} + if in.Provider != "" { + args = append(args, "--provider", in.Provider) + } + if in.IDE != "" { + args = append(args, "--ide", in.IDE) + } else { + args = append(args, "--ide=none") + } + if in.DevcontainerPath != "" { + args = append(args, "--devcontainer-path", in.DevcontainerPath) + } + if in.Name != "" { + args = append(args, "--id", in.Name) + } + args = append(args, in.Source) + + cobraCmd := up.NewUpCmd(g) + cobraCmd.SetArgs(args) + cobraCmd.SetContext(ctx) + return cobraCmd.Execute() +} From e0da0835b725817f2e034304722c151c29b804e1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:02:28 -0500 Subject: [PATCH 09/48] feat(mcp): add workspace_exec tool --- cmd/mcp/tools_exec.go | 71 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index a2a10b77a..9ab14d408 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -1,6 +1,71 @@ package mcp -import sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +import ( + "context" + "fmt" -// registerExecTool is filled in by Task 8. -func registerExecTool(_ *sdkmcp.Server, _ *ServeCmd) {} + "github.com/devsy-org/devsy/pkg/workspace" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +type execInput struct { + Name string `json:"name" jsonschema:"required"` + Command []string `json:"command" jsonschema:"required"` + Workdir string `json:"workdir,omitempty"` + Env map[string]string `json:"env,omitempty"` + TimeoutSeconds int `json:"timeout_seconds,omitempty"` +} + +type execOutput struct { + Stdout string `json:"stdout"` + Stderr string `json:"stderr"` + ExitCode int `json:"exit_code"` + DurationMS int64 `json:"duration_ms"` + Truncated bool `json:"truncated"` + TimedOut bool `json:"timed_out,omitempty"` + Clamped bool `json:"clamped,omitempty"` +} + +func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "workspace_exec", + Description: "Run a one-shot command in a workspace container. Output is capped " + + "per stream; excess is truncated in the middle. The command is argv, not a shell string.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in execInput) (*sdkmcp.CallToolResult, execOutput, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), execOutput{}, nil + } + if len(in.Command) == 0 { + return errorResult(fmt.Errorf("command is required")), execOutput{}, nil + } + stdout := NewBoundedBuffer(cmd.ExecOutputCap) + stderr := NewBoundedBuffer(cmd.ExecOutputCap) + + res, err := workspace.ExecOneShot(ctx, workspace.ExecOneShotOptions{ + WorkspaceName: in.Name, + Command: in.Command, + Workdir: in.Workdir, + Env: in.Env, + TimeoutSeconds: in.TimeoutSeconds, + TimeoutSecondsDefault: int(cmd.ExecTimeoutDefault.Seconds()), + TimeoutSecondsMax: int(cmd.ExecTimeoutMax.Seconds()), + Owner: cmd.Owner, + Context: cmd.Context, + Provider: cmd.Provider, + Stdout: stdout, + Stderr: stderr, + }) + if err != nil { + return errorResult(err), execOutput{}, nil + } + return nil, execOutput{ + Stdout: stdout.String(), + Stderr: stderr.String(), + ExitCode: res.ExitCode, + DurationMS: res.DurationMS, + Truncated: stdout.Truncated() || stderr.Truncated(), + TimedOut: res.TimedOut, + Clamped: res.Clamped, + }, nil + }) +} From b3864787fa635cb5c43e6dfd1367d3a5469dc20f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:05:20 -0500 Subject: [PATCH 10/48] feat(mcp): add provider list/add/delete/use tools --- cmd/mcp/tools_provider.go | 134 +++++++++++++++++++++++++++++++++++++- 1 file changed, 132 insertions(+), 2 deletions(-) diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index acdffb979..02015869c 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -1,9 +1,139 @@ package mcp import ( + "context" + "fmt" + "sort" + "github.com/devsy-org/devsy/cmd/flags" + cmdprovider "github.com/devsy-org/devsy/cmd/provider" + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) -// registerProviderTools is filled in by Task 9. -func registerProviderTools(_ *sdkmcp.Server, _ *flags.GlobalFlags) {} +type providerSummary struct { + Name string `json:"name"` + Version string `json:"version,omitempty"` + Default bool `json:"default,omitempty"` +} + +type providerListOutput struct { + Providers []providerSummary `json:"providers"` +} + +type providerAddInput struct { + Source string `json:"source" jsonschema:"required"` + Name string `json:"name,omitempty"` + Options map[string]string `json:"options,omitempty"` + Use bool `json:"use,omitempty"` +} + +type providerNameInput struct { + Name string `json:"name" jsonschema:"required"` +} + +func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "provider_list", + Description: "List configured Devsy providers.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ struct{}, + ) (*sdkmcp.CallToolResult, providerListOutput, error) { + out, err := handleProviderList(ctx, g) + if err != nil { + return errorResult(err), providerListOutput{}, nil + } + return nil, out, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "provider_add", + Description: "Add a provider from a source (registry name, URL, or local path).", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerAddInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := runProviderAdd(ctx, g, in); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "provider_delete", + Description: "Delete a configured provider.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := runProviderDelete(ctx, g, in.Name); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) + + sdkmcp.AddTool(s, &sdkmcp.Tool{ + Name: "provider_use", + Description: "Set a provider as the default for new workspaces.", + }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput) (*sdkmcp.CallToolResult, opOK, error) { + if err := runProviderUse(ctx, g, in.Name); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil + }) +} + +func handleProviderList(_ context.Context, g *flags.GlobalFlags) (providerListOutput, error) { + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return providerListOutput{}, err + } + + providers, err := workspace.LoadAllProviders(devsyConfig) + if err != nil { + return providerListOutput{}, err + } + + defaultProvider := devsyConfig.Current().DefaultProvider + + summaries := make([]providerSummary, 0, len(providers)) + for _, entry := range providers { + summaries = append(summaries, providerSummary{ + Name: entry.Config.Name, + Version: entry.Config.Version, + Default: entry.Config.Name == defaultProvider, + }) + } + sort.Slice(summaries, func(i, j int) bool { + return summaries[i].Name < summaries[j].Name + }) + + return providerListOutput{Providers: summaries}, nil +} + +func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInput) error { + args := []string{} + if in.Name != "" { + args = append(args, "--name", in.Name) + } + if in.Use { + args = append(args, "--use") + } + for k, v := range in.Options { + args = append(args, "--option", fmt.Sprintf("%s=%s", k, v)) + } + args = append(args, in.Source) + + cobraCmd := cmdprovider.NewAddCmd(g) + cobraCmd.SetArgs(args) + cobraCmd.SetContext(ctx) + return cobraCmd.Execute() +} + +func runProviderDelete(ctx context.Context, g *flags.GlobalFlags, name string) error { + cobraCmd := cmdprovider.NewDeleteCmd(g) + cobraCmd.SetArgs([]string{name}) + cobraCmd.SetContext(ctx) + return cobraCmd.Execute() +} + +func runProviderUse(ctx context.Context, g *flags.GlobalFlags, name string) error { + cobraCmd := cmdprovider.NewUseCmd(g) + cobraCmd.SetArgs([]string{name}) + cobraCmd.SetContext(ctx) + return cobraCmd.Execute() +} From 70351a1b4eae3cddc5252deb432ddb59f76cc649 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:06:59 -0500 Subject: [PATCH 11/48] test(mcp): add integration test for tool registration --- cmd/mcp/serve_test.go | 60 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 cmd/mcp/serve_test.go diff --git a/cmd/mcp/serve_test.go b/cmd/mcp/serve_test.go new file mode 100644 index 000000000..0b006a863 --- /dev/null +++ b/cmd/mcp/serve_test.go @@ -0,0 +1,60 @@ +package mcp + +import ( + "context" + "testing" + "time" + + "github.com/devsy-org/devsy/cmd/flags" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +func TestServer_ListsAllTools(t *testing.T) { + home := t.TempDir() + t.Setenv("DEVSY_HOME", home) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + server := sdkmcp.NewServer(&sdkmcp.Implementation{Name: "devsy-test", Version: "test"}, nil) + g := &flags.GlobalFlags{} + registerWorkspaceTools(server, g) + registerProviderTools(server, g) + registerExecTool(server, &ServeCmd{GlobalFlags: g, ExecOutputCap: 1024}) + + clientTransport, serverTransport := sdkmcp.NewInMemoryTransports() + + serverErr := make(chan error, 1) + go func() { + serverErr <- server.Run(ctx, serverTransport) + }() + + client := sdkmcp.NewClient(&sdkmcp.Implementation{Name: "test-client", Version: "0"}, nil) + session, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = session.Close() }) + + tools, err := session.ListTools(ctx, nil) + if err != nil { + t.Fatalf("list tools: %v", err) + } + wantNames := []string{ + "workspace_list", "workspace_status", "workspace_start", "workspace_stop", + "workspace_delete", "workspace_create", "workspace_exec", + "provider_list", "provider_add", "provider_delete", "provider_use", + } + have := map[string]bool{} + for _, tool := range tools.Tools { + have[tool.Name] = true + } + for _, name := range wantNames { + if !have[name] { + t.Errorf("missing tool: %s", name) + } + } + if len(tools.Tools) != len(wantNames) { + t.Errorf("expected %d tools, got %d: %+v", len(wantNames), len(tools.Tools), have) + } +} From f1a38ca9b7ce33301f18b7844f309dee275cb515 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:17:26 -0500 Subject: [PATCH 12/48] refactor(workspace): drop re-export shims and tidy ExecOneShot Removes the six re-export const/var aliases in cmd/workspace/exec.go that existed only to keep two tests working without a real import. The tests in cmd/workspace and cmd/config now reference pkg/workspace directly. Renames ExecOneShotResult.TimeoutSecond to TimeoutSeconds for consistency with the matching input field, extracts the 300s fallback into a named constant, and lowercases the workspace-name validation error to match Go style and the sibling error. --- cmd/config/apply_test.go | 2 +- cmd/workspace/exec.go | 19 ------------------- cmd/workspace/exec_test.go | 3 ++- pkg/workspace/exec_oneshot.go | 27 ++++++++++++++++----------- 4 files changed, 19 insertions(+), 32 deletions(-) diff --git a/cmd/config/apply_test.go b/cmd/config/apply_test.go index 015dd9e12..7969e6994 100644 --- a/cmd/config/apply_test.go +++ b/cmd/config/apply_test.go @@ -4,7 +4,7 @@ import ( "testing" "github.com/devsy-org/devsy/cmd/flags" - "github.com/devsy-org/devsy/cmd/workspace" + "github.com/devsy-org/devsy/pkg/workspace" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index abc58f6e6..06eb84c40 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -18,25 +18,6 @@ import ( "golang.org/x/term" ) -// Re-exports so that code within this package (including tests) can reference -// symbols that have moved to pkg/workspace without an import alias. -const ( - DefaultDockerCommand = workspace2.DefaultDockerCommand - ContainerStatusRunning = workspace2.ContainerStatusRunning -) - -// ResolveDockerCommand is a re-export of the pkg/workspace function. -var ResolveDockerCommand = workspace2.ResolveDockerCommand - -// FindRunningContainer is a re-export of the pkg/workspace function. -var FindRunningContainer = workspace2.FindRunningContainer - -// LoadExecResult is a re-export of the pkg/workspace function. -var LoadExecResult = workspace2.LoadExecResult - -// ResolveExecWorkdir is a re-export of the pkg/workspace function. -var ResolveExecWorkdir = workspace2.ResolveExecWorkdir - type ExecCmd struct { *flags.GlobalFlags diff --git a/cmd/workspace/exec_test.go b/cmd/workspace/exec_test.go index 3cfc96092..db2b4e2c3 100644 --- a/cmd/workspace/exec_test.go +++ b/cmd/workspace/exec_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/devsy-org/devsy/cmd/flags" + workspace2 "github.com/devsy-org/devsy/pkg/workspace" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -69,7 +70,7 @@ func TestNewExecCmd_RequiresArgs(t *testing.T) { } func TestResolveDockerCommand_NilWorkspace(t *testing.T) { - result := ResolveDockerCommand(nil) + result := workspace2.ResolveDockerCommand(nil) assert.Equal(t, "docker", result) } diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 7426c9554..9cb04e09b 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -32,13 +32,18 @@ type ExecOneShotOptions struct { Stderr io.Writer } +// defaultExecTimeoutSeconds is used when neither the caller nor the configured +// default supplies a positive timeout. Kept short enough to surface hung calls +// quickly while long enough to allow typical build/test commands to complete. +const defaultExecTimeoutSeconds = 300 + // ExecOneShotResult is the structured outcome of an exec. type ExecOneShotResult struct { - ExitCode int - DurationMS int64 - TimedOut bool - TimeoutSecond int - Clamped bool + ExitCode int + DurationMS int64 + TimedOut bool + TimeoutSeconds int + Clamped bool } // ResolveTimeout returns the effective timeout and whether the caller's @@ -67,13 +72,13 @@ func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, // It never reads stdin and never allocates a TTY. func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResult, error) { if opts.WorkspaceName == "" { - return nil, fmt.Errorf("WorkspaceName is required") + return nil, fmt.Errorf("workspace name is required") } if len(opts.Command) == 0 { return nil, fmt.Errorf("command is required") } - timeout, clamped := opts.ResolveTimeout(300) + timeout, clamped := opts.ResolveTimeout(defaultExecTimeoutSeconds) execCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -94,10 +99,10 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu duration := time.Since(start) res := &ExecOneShotResult{ - ExitCode: exitCode, - DurationMS: duration.Milliseconds(), - Clamped: clamped, - TimeoutSecond: int(timeout.Seconds()), + ExitCode: exitCode, + DurationMS: duration.Milliseconds(), + Clamped: clamped, + TimeoutSeconds: int(timeout.Seconds()), } if errors.Is(execCtx.Err(), context.DeadlineExceeded) { res.TimedOut = true From 471eaacbd397d9b8646649d4af2f4a4cc9ac2f8f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:31:07 -0500 Subject: [PATCH 13/48] fix(mcp): protect stdio framing, flag injection, error logging, panic recovery, and buffer fixes - redirect os.Stdout to os.Stderr during MCP serve via IOTransport so accidental writes cannot corrupt the JSON-RPC frame (fixes: Fix 1) - append "--" terminator before user-controlled positional cobra args in runUp, runProviderAdd, runProviderDelete, runProviderUse to prevent flag injection (fixes: Fix 2) - log raw errors in errorResult before classification for operator visibility (fixes: Fix 4) - add safeHandler generic wrapper that recovers panics in tool handlers and returns a structured tool error instead of crashing the server; wrap all sdkmcp.AddTool calls (fixes: Fix 5) - route ErrWorkspaceNotFound through the classifier and preserve Hint and DocURL; only override the code field (fixes: Fix 6) - round BoundedBuffer cap up to even in NewBoundedBuffer to avoid dropping the middle byte at odd-cap boundaries; add test (fixes: Fix 7) --- cmd/mcp/buffer.go | 3 +++ cmd/mcp/buffer_test.go | 15 +++++++++++++++ cmd/mcp/errors.go | 11 ++++------- cmd/mcp/recover.go | 32 ++++++++++++++++++++++++++++++++ cmd/mcp/serve.go | 15 ++++++++++++++- cmd/mcp/tools_exec.go | 6 ++++-- cmd/mcp/tools_provider.go | 27 +++++++++++++++++---------- cmd/mcp/tools_workspace.go | 38 ++++++++++++++++++++++++++------------ 8 files changed, 115 insertions(+), 32 deletions(-) create mode 100644 cmd/mcp/recover.go diff --git a/cmd/mcp/buffer.go b/cmd/mcp/buffer.go index 405caf042..a2e691010 100644 --- a/cmd/mcp/buffer.go +++ b/cmd/mcp/buffer.go @@ -16,6 +16,9 @@ func NewBoundedBuffer(cap int) *BoundedBuffer { if cap < 8 { cap = 8 } + if cap%2 != 0 { + cap++ + } return &BoundedBuffer{cap: cap} } diff --git a/cmd/mcp/buffer_test.go b/cmd/mcp/buffer_test.go index 16858b946..7e41cc8cd 100644 --- a/cmd/mcp/buffer_test.go +++ b/cmd/mcp/buffer_test.go @@ -40,3 +40,18 @@ func TestBoundedBuffer_MultipleWritesAccumulate(t *testing.T) { t.Fatal("expected truncated after 20 bytes into cap 10") } } + +func TestBoundedBuffer_OddCap(t *testing.T) { + // Odd cap should be rounded up so head+tail covers everything when + // written exactly cap bytes. + b2 := NewBoundedBuffer(101) // odd, > floor; rounded up to 102 + for range 101 { + _, _ = b2.Write([]byte("a")) + } + if b2.Truncated() { + t.Fatal("at exactly 101 bytes into cap 102, should not be truncated") + } + if got := b2.String(); len(got) != 101 { + t.Fatalf("expected 101 bytes, got %d", len(got)) + } +} diff --git a/cmd/mcp/errors.go b/cmd/mcp/errors.go index 1691c4297..a06ee802b 100644 --- a/cmd/mcp/errors.go +++ b/cmd/mcp/errors.go @@ -22,20 +22,17 @@ func ClassifyError(err error) ErrorPayload { if err == nil { return ErrorPayload{} } - if errors.Is(err, workspace.ErrWorkspaceNotFound) { - return ErrorPayload{ - Code: "workspace_not_found", - Message: err.Error(), - } - } classified := cliErrors.Classify(err, cliErrors.ClassifyContext{}) code := "internal_error" if classified.Code != "" { code = string(classified.Code) } + if errors.Is(err, workspace.ErrWorkspaceNotFound) { + code = "workspace_not_found" + } return ErrorPayload{ Code: code, - Message: classified.Message, + Message: err.Error(), Hint: classified.Hint, DocURL: classified.DocURL, } diff --git a/cmd/mcp/recover.go b/cmd/mcp/recover.go new file mode 100644 index 000000000..78881b69c --- /dev/null +++ b/cmd/mcp/recover.go @@ -0,0 +1,32 @@ +package mcp + +import ( + "context" + "fmt" + "runtime/debug" + + "github.com/devsy-org/devsy/pkg/log" + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// safeHandler wraps a typed MCP handler with panic recovery so a single broken +// tool cannot tear down the server. A recovered panic is logged with a stack +// trace and returned as a structured tool error. +func safeHandler[In any, Out any]( + inner func(context.Context, *sdkmcp.CallToolRequest, In) (*sdkmcp.CallToolResult, Out, error), +) func(context.Context, *sdkmcp.CallToolRequest, In) (*sdkmcp.CallToolResult, Out, error) { + return func( + ctx context.Context, req *sdkmcp.CallToolRequest, in In, + ) (result *sdkmcp.CallToolResult, out Out, err error) { + defer func() { + if r := recover(); r != nil { + log.Errorf("mcp handler panic: %v\n%s", r, debug.Stack()) + result = errorResult(fmt.Errorf("handler panicked: %v", r)) + var zero Out + out = zero + err = nil + } + }() + return inner(ctx, req, in) + } +} diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index ae1adcc97..49a8e9742 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "os" "time" "github.com/devsy-org/devsy/cmd/flags" @@ -45,6 +46,19 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { log.Debugf("starting MCP server (timeout default=%s max=%s cap=%dB)", cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) + // Capture the real stdout before any redirection. StdioTransport.Connect + // reads os.Stdin/os.Stdout lazily (at server.Run time), so we use IOTransport + // with explicit file references instead. Then we redirect os.Stdout → os.Stderr + // so any code path that writes to os.Stdout cannot corrupt the MCP JSON-RPC frame. + realStdout := os.Stdout + os.Stdout = os.Stderr + defer func() { os.Stdout = realStdout }() + + transport := &sdkmcp.IOTransport{ + Reader: os.Stdin, + Writer: realStdout, + } + server := sdkmcp.NewServer(&sdkmcp.Implementation{ Name: "devsy", Version: version.GetVersion(), @@ -52,7 +66,6 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { cmd.registerTools(server) - transport := &sdkmcp.StdioTransport{} return server.Run(ctx, transport) } diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index 9ab14d408..211bd08f6 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -31,7 +31,9 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { Name: "workspace_exec", Description: "Run a one-shot command in a workspace container. Output is capped " + "per stream; excess is truncated in the middle. The command is argv, not a shell string.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in execInput) (*sdkmcp.CallToolResult, execOutput, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in execInput, + ) (*sdkmcp.CallToolResult, execOutput, error) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), execOutput{}, nil } @@ -67,5 +69,5 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { TimedOut: res.TimedOut, Clamped: res.Clamped, }, nil - }) + })) } diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index 02015869c..7f4ac6b06 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -37,44 +37,50 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "provider_list", Description: "List configured Devsy providers.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ struct{}, + }, safeHandler(func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ struct{}, ) (*sdkmcp.CallToolResult, providerListOutput, error) { out, err := handleProviderList(ctx, g) if err != nil { return errorResult(err), providerListOutput{}, nil } return nil, out, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "provider_add", Description: "Add a provider from a source (registry name, URL, or local path).", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerAddInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in providerAddInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := runProviderAdd(ctx, g, in); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "provider_delete", Description: "Delete a configured provider.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := runProviderDelete(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "provider_use", Description: "Set a provider as the default for new workspaces.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := runProviderUse(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) } func handleProviderList(_ context.Context, g *flags.GlobalFlags) (providerListOutput, error) { @@ -116,6 +122,7 @@ func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInp for k, v := range in.Options { args = append(args, "--option", fmt.Sprintf("%s=%s", k, v)) } + args = append(args, "--") args = append(args, in.Source) cobraCmd := cmdprovider.NewAddCmd(g) @@ -126,14 +133,14 @@ func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInp func runProviderDelete(ctx context.Context, g *flags.GlobalFlags, name string) error { cobraCmd := cmdprovider.NewDeleteCmd(g) - cobraCmd.SetArgs([]string{name}) + cobraCmd.SetArgs([]string{"--", name}) cobraCmd.SetContext(ctx) return cobraCmd.Execute() } func runProviderUse(ctx context.Context, g *flags.GlobalFlags, name string) error { cobraCmd := cmdprovider.NewUseCmd(g) - cobraCmd.SetArgs([]string{name}) + cobraCmd.SetArgs([]string{"--", name}) cobraCmd.SetContext(ctx) return cobraCmd.Execute() } diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 858f2d6ed..ded8b5f55 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -9,6 +9,7 @@ import ( up "github.com/devsy-org/devsy/cmd/workspace/up" client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -36,25 +37,27 @@ func registerWorkspaceTools(s *sdkmcp.Server, g *flags.GlobalFlags) { sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_list", Description: "List all Devsy workspaces.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ workspaceListInput, + }, safeHandler(func(ctx context.Context, _ *sdkmcp.CallToolRequest, _ workspaceListInput, ) (*sdkmcp.CallToolResult, workspaceListOutput, error) { out, err := handleWorkspaceList(ctx, g) if err != nil { return errorResult(err), workspaceListOutput{}, nil } return nil, out, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_status", Description: "Get detailed status for a workspace by name.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in workspaceStatusInput) (*sdkmcp.CallToolResult, any, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in workspaceStatusInput, + ) (*sdkmcp.CallToolResult, any, error) { out, err := handleWorkspaceStatus(ctx, g, in.Name) if err != nil { return errorResult(err), nil, nil } return nil, out, nil - }) + })) registerWorkspaceLifecycleTools(s, g) } @@ -101,7 +104,9 @@ func handleWorkspaceStatus(ctx context.Context, g *flags.GlobalFlags, name strin } // errorResult builds an isError CallToolResult carrying our structured payload. +// The raw error is logged so operators can see the unclassified failure detail. func errorResult(err error) *sdkmcp.CallToolResult { + log.Errorf("mcp tool error: %v", err) payload := ClassifyError(err) return &sdkmcp.CallToolResult{ IsError: true, @@ -132,43 +137,51 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_start", Description: "Start (or resume) an existing workspace by name.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := startWorkspace(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_stop", Description: "Stop a running workspace by name.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := stopWorkspace(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_delete", Description: "Delete a workspace by name. Pass force=true to force-delete even if not found remotely.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput) (*sdkmcp.CallToolResult, opOK, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, + ) (*sdkmcp.CallToolResult, opOK, error) { if err := deleteWorkspace(ctx, g, in.Name, in.Force); err != nil { return errorResult(err), opOK{}, nil } return nil, opOK{OK: true}, nil - }) + })) sdkmcp.AddTool(s, &sdkmcp.Tool{ Name: "workspace_create", Description: "Create and start a new workspace from a git URL, local path, or container image.", - }, func(ctx context.Context, _ *sdkmcp.CallToolRequest, in createInput) (*sdkmcp.CallToolResult, any, error) { + }, safeHandler(func( + ctx context.Context, _ *sdkmcp.CallToolRequest, in createInput, + ) (*sdkmcp.CallToolResult, any, error) { out, err := createWorkspace(ctx, g, in) if err != nil { return errorResult(err), nil, nil } return nil, out, nil - }) + })) } func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { @@ -247,6 +260,7 @@ func runUp(ctx context.Context, g *flags.GlobalFlags, in createInput) error { if in.Name != "" { args = append(args, "--id", in.Name) } + args = append(args, "--") args = append(args, in.Source) cobraCmd := up.NewUpCmd(g) From cbda0de4edccb91649f67f8333cd6f4964d20758 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:31:17 -0500 Subject: [PATCH 14/48] fix(workspace): distinguish context cancellation from deadline in ExecOneShot Add a parallel branch for context.Canceled alongside the existing DeadlineExceeded check. Uses parent ctx (not execCtx) so cancellation from the caller is correctly surfaced rather than returned as a generic error. --- pkg/workspace/exec_oneshot.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 9cb04e09b..638425510 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -109,6 +109,10 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu res.ExitCode = -1 return res, nil } + if errors.Is(ctx.Err(), context.Canceled) { + res.ExitCode = -1 + return res, ctx.Err() + } if runErr != nil { return res, runErr } From 99fa02a2e4f9794b5188688b1939dfd949a639df Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:35:13 -0500 Subject: [PATCH 15/48] fix(mcp): use --flag=value form to prevent flag injection via values The -- terminator protected positional args, but values for --provider, --ide, --devcontainer-path, --id, and --option were still passed as separate tokens. A value beginning with -- (or an option key like --use) would be reparsed by pflag as a new flag, bypassing the protection. Using the single-token --flag=value form bundles the value with the flag name so pflag cannot misinterpret it regardless of the value contents. --- cmd/mcp/tools_provider.go | 4 +++- cmd/mcp/tools_workspace.go | 13 +++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index 7f4ac6b06..abfb88b54 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -120,7 +120,9 @@ func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInp args = append(args, "--use") } for k, v := range in.Options { - args = append(args, "--option", fmt.Sprintf("%s=%s", k, v)) + // Use --flag=value single-token form so a key starting with "-" cannot + // be reparsed by pflag as a separate flag. + args = append(args, fmt.Sprintf("--option=%s=%s", k, v)) } args = append(args, "--") args = append(args, in.Source) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index ded8b5f55..3d78673e3 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -245,23 +245,24 @@ func createWorkspace(ctx context.Context, g *flags.GlobalFlags, in createInput) } func runUp(ctx context.Context, g *flags.GlobalFlags, in createInput) error { + // Use --flag=value single-token form for every user-controlled value so a + // value starting with "-" cannot be reparsed by pflag as a separate flag. args := []string{} if in.Provider != "" { - args = append(args, "--provider", in.Provider) + args = append(args, fmt.Sprintf("--provider=%s", in.Provider)) } if in.IDE != "" { - args = append(args, "--ide", in.IDE) + args = append(args, fmt.Sprintf("--ide=%s", in.IDE)) } else { args = append(args, "--ide=none") } if in.DevcontainerPath != "" { - args = append(args, "--devcontainer-path", in.DevcontainerPath) + args = append(args, fmt.Sprintf("--devcontainer-path=%s", in.DevcontainerPath)) } if in.Name != "" { - args = append(args, "--id", in.Name) + args = append(args, fmt.Sprintf("--id=%s", in.Name)) } - args = append(args, "--") - args = append(args, in.Source) + args = append(args, "--", in.Source) cobraCmd := up.NewUpCmd(g) cobraCmd.SetArgs(args) From 5623d60c121142b1c4fc8a32e484d50acc077496 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:49:45 -0500 Subject: [PATCH 16/48] fix(workspace): stop WithSignals from leaking a goroutine per call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second goroutine — the "force shutdown on second signal" guard — waited on <-signals after ctx was cancelled. The returned cleanup called signal.Stop, so no further signals could ever arrive on that channel and the goroutine blocked forever, leaking one goroutine per call. Adds a done channel that the cleanup closes so both goroutines exit cleanly when the caller is finished. --- cmd/workspace/up/up.go | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index a03513386..6ea776030 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -328,23 +328,38 @@ func WithSignals(ctx context.Context) (context.Context, func()) { ctx, cancel := context.WithCancel(ctx) signals := make(chan os.Signal, 1) signal.Notify(signals, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGQUIT) + + // done is closed by the returned cleanup so both goroutines exit when the + // caller finishes — signal.Stop alone is not enough because a goroutine + // already blocked on <-signals will never unblock once Stop is called. + done := make(chan struct{}) + go func() { select { case <-signals: cancel() case <-ctx.Done(): + case <-done: } }() go func() { - <-ctx.Done() - <-signals - // force shutdown if context is done and another signal arrives - os.Exit(1) + select { + case <-ctx.Done(): + case <-done: + return + } + select { + case <-signals: + // force shutdown if context is done and another signal arrives + os.Exit(1) + case <-done: + } }() return ctx, func() { cancel() signal.Stop(signals) + close(done) } } From 3a7378283fa25c80f9944e6cbf1c0c7a0e3cf54a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 3 Jun 2026 17:49:50 -0500 Subject: [PATCH 17/48] style(mcp): tighten serve stdout-redirect comment Drop the SDK-implementation rationale and keep only the why-it-exists. --- cmd/mcp/serve.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index 49a8e9742..606f0627f 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -46,10 +46,8 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { log.Debugf("starting MCP server (timeout default=%s max=%s cap=%dB)", cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) - // Capture the real stdout before any redirection. StdioTransport.Connect - // reads os.Stdin/os.Stdout lazily (at server.Run time), so we use IOTransport - // with explicit file references instead. Then we redirect os.Stdout → os.Stderr - // so any code path that writes to os.Stdout cannot corrupt the MCP JSON-RPC frame. + // Reserve the real stdout for the JSON-RPC frame and redirect os.Stdout to + // stderr so stray writes elsewhere in the process cannot corrupt it. realStdout := os.Stdout os.Stdout = os.Stderr defer func() { os.Stdout = realStdout }() From 50d0f4be9e3f88b5b58b5323f25b092091478003 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 13:56:25 -0500 Subject: [PATCH 18/48] fix(mcp): round sub-second exec timeouts up instead of truncating to zero int(d.Seconds()) truncates a value like 500ms to 0, which then falls through ResolveTimeout's defaults and silently produces a 5-minute timeout instead of the value the operator configured. Round up via math.Ceil so any positive duration resolves to at least 1s. --- cmd/mcp/tools_exec.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index 211bd08f6..3d90569d8 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -3,11 +3,23 @@ package mcp import ( "context" "fmt" + "math" + "time" "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) +// durationToSeconds converts a Duration to a positive whole number of seconds, +// rounding up so that any non-zero sub-second value resolves to at least 1s +// rather than truncating to 0 (which would silently fall through to defaults). +func durationToSeconds(d time.Duration) int { + if d <= 0 { + return 0 + } + return int(math.Ceil(d.Seconds())) +} + type execInput struct { Name string `json:"name" jsonschema:"required"` Command []string `json:"command" jsonschema:"required"` @@ -49,8 +61,8 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { Workdir: in.Workdir, Env: in.Env, TimeoutSeconds: in.TimeoutSeconds, - TimeoutSecondsDefault: int(cmd.ExecTimeoutDefault.Seconds()), - TimeoutSecondsMax: int(cmd.ExecTimeoutMax.Seconds()), + TimeoutSecondsDefault: durationToSeconds(cmd.ExecTimeoutDefault), + TimeoutSecondsMax: durationToSeconds(cmd.ExecTimeoutMax), Owner: cmd.Owner, Context: cmd.Context, Provider: cmd.Provider, From 34de3eba9eb1df2779f508d7d3b2dd812d2ede37 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 13:56:37 -0500 Subject: [PATCH 19/48] fix(mcp): validate non-empty required strings in provider handlers jsonschema:"required" only enforces field presence, not non-empty contents. An empty string would currently flow into cobra as a positional arg and surface as a confusing pflag error. Match the explicit empty-string checks that workspace_status and workspace_exec already perform. --- cmd/mcp/tools_provider.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index abfb88b54..6c8f3b0ac 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -52,6 +52,9 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in providerAddInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Source == "" { + return errorResult(fmt.Errorf("source is required")), opOK{}, nil + } if err := runProviderAdd(ctx, g, in); err != nil { return errorResult(err), opOK{}, nil } @@ -64,6 +67,9 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), opOK{}, nil + } if err := runProviderDelete(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } @@ -76,6 +82,9 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in providerNameInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), opOK{}, nil + } if err := runProviderUse(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } From 6d65b9767a028dd0989878709dbee2250dce7468 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 13:56:43 -0500 Subject: [PATCH 20/48] fix(mcp): ensure workspace_start refuses to create a workspace up.NewUpCmd interprets its positional argument as either an existing workspace name or a source from which to create one. For workspace_start we want lookup-only semantics: verify the workspace exists via workspace.Get before delegating to up, so a typo or stale name surfaces as 'workspace not found' rather than silently creating a new workspace from a string that happens to look like a path. --- cmd/mcp/tools_workspace.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 3d78673e3..bfdcd9f2d 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -185,6 +185,20 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { } func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { + // up.NewUpCmd treats the positional argument as either an existing + // workspace name or a source from which to create one. workspace_start + // must never create, so confirm the workspace exists first. + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return err + } + if _, err := workspace.Get(ctx, workspace.GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{name}, + Owner: g.Owner, + }); err != nil { + return err + } return runUp(ctx, g, createInput{Source: name}) } From 0e77736c714502cf734cd0e53286aa12a34098e7 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 13:56:48 -0500 Subject: [PATCH 21/48] test(mcp): drive the integration test through ServeCmd.registerTools The test registered each tool group manually, which lets registration drift from production wiring. Use the same registerTools method the serve command uses. --- cmd/mcp/serve_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/mcp/serve_test.go b/cmd/mcp/serve_test.go index 0b006a863..d7ce8388c 100644 --- a/cmd/mcp/serve_test.go +++ b/cmd/mcp/serve_test.go @@ -18,9 +18,8 @@ func TestServer_ListsAllTools(t *testing.T) { server := sdkmcp.NewServer(&sdkmcp.Implementation{Name: "devsy-test", Version: "test"}, nil) g := &flags.GlobalFlags{} - registerWorkspaceTools(server, g) - registerProviderTools(server, g) - registerExecTool(server, &ServeCmd{GlobalFlags: g, ExecOutputCap: 1024}) + serveCmd := &ServeCmd{GlobalFlags: g, ExecOutputCap: 1024} + serveCmd.registerTools(server) clientTransport, serverTransport := sdkmcp.NewInMemoryTransports() From 21bcbeb63c09fd9cae107f0055cc5290bb718746 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:11:22 -0500 Subject: [PATCH 22/48] fix(mcp): close provider_add --name flag injection gap provider_add was the last user-controlled value still passed to cobra as two separate tokens. A value like '--use' would be reparsed by pflag as a boolean flag instead of being treated as the workspace name. Move to the --flag=value single-token form, matching the rest of the file. --- cmd/mcp/tools_provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index 6c8f3b0ac..3d831f203 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -123,7 +123,7 @@ func handleProviderList(_ context.Context, g *flags.GlobalFlags) (providerListOu func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInput) error { args := []string{} if in.Name != "" { - args = append(args, "--name", in.Name) + args = append(args, fmt.Sprintf("--name=%s", in.Name)) } if in.Use { args = append(args, "--use") From 2be2827c69c505f9cbd65449f4200a12c4eecb5f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:11:28 -0500 Subject: [PATCH 23/48] fix(workspace): don't claim TimedOut when the parent context cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit execCtx is derived from the caller's ctx, so when the caller's deadline or cancellation fires first, execCtx.Err() is also DeadlineExceeded. The previous code matched that branch unconditionally and reported TimedOut=true — telling the agent its exec timeout was exceeded when the real cause was the caller. Check the parent context first and only attribute the timeout to ours when the parent is still healthy. While here, honor the workspace's configured userEnvProbe. The CLI exec flow reads result.MergedConfig.UserEnvProbe; the MCP path was passing an empty string and always falling back to the default probe, ignoring workspace settings such as userEnvProbe: none. --- pkg/workspace/exec_oneshot.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 638425510..31ec0be91 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -104,15 +104,19 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu Clamped: clamped, TimeoutSeconds: int(timeout.Seconds()), } + // Distinguish "our timeout fired" from "the caller cancelled or expired". + // Check the parent context first — if it carries an error, execCtx + // inherits the same DeadlineExceeded/Canceled and we must not claim + // TimedOut, which means "the exec exceeded ITS own time budget". + if parentErr := ctx.Err(); parentErr != nil { + res.ExitCode = -1 + return res, parentErr + } if errors.Is(execCtx.Err(), context.DeadlineExceeded) { res.TimedOut = true res.ExitCode = -1 return res, nil } - if errors.Is(ctx.Err(), context.Canceled) { - res.ExitCode = -1 - return res, ctx.Err() - } if runErr != nil { return res, runErr } @@ -168,7 +172,11 @@ func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedEx ContainerID: containerDetails.ID, User: user, } - probedEnv := ProbeContainerEnv(ctx, target, "") + userEnvProbe := "" + if execResult != nil && execResult.MergedConfig != nil { + userEnvProbe = execResult.MergedConfig.UserEnvProbe + } + probedEnv := ProbeContainerEnv(ctx, target, userEnvProbe) envSlice := envMapToSlice(opts.Env) envMap := BuildExecEnv(execResult, envSlice, probedEnv) From f7ac471490bfacab9faf9600b21e00e2ee8492e7 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:11:34 -0500 Subject: [PATCH 24/48] fix(mcp): require non-empty name on workspace_stop and workspace_delete workspace_status and workspace_start already reject empty names with a clear error; the stop and delete handlers were passing the empty string through to workspace.Get/Delete and surfacing a confusing 'workspace not found' instead. Mirror the existing check. --- cmd/mcp/tools_workspace.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index bfdcd9f2d..c2fb4dd0d 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -140,6 +140,9 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), opOK{}, nil + } if err := startWorkspace(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } @@ -152,6 +155,9 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), opOK{}, nil + } if err := stopWorkspace(ctx, g, in.Name); err != nil { return errorResult(err), opOK{}, nil } @@ -164,6 +170,9 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { }, safeHandler(func( ctx context.Context, _ *sdkmcp.CallToolRequest, in nameInput, ) (*sdkmcp.CallToolResult, opOK, error) { + if in.Name == "" { + return errorResult(fmt.Errorf("name is required")), opOK{}, nil + } if err := deleteWorkspace(ctx, g, in.Name, in.Force); err != nil { return errorResult(err), opOK{}, nil } From 9b62676d4998a3b5aee6a9ebb76928ac9458006f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:45:09 -0500 Subject: [PATCH 25/48] refactor(mcp,workspace): tighten exec_oneshot internals and exec output flow - Preserve partial stdout/stderr on cancelled/timed-out exec (1.2) - Add IDLabels field to ExecOneShotOptions and execInput; propagate to FindRunningContainer so callers can pass --id-label style filters (1.3) - Split timeout budget: resolve container target with parent ctx, apply exec timeout only to runCapture so Docker lookup does not eat the user's exec budget (1.4) - Remove redundant zero-init in safeHandler panic recover (1.5) - Consolidate runProviderDelete/runProviderUse into runProviderCobraCmd helper (1.6) --- cmd/mcp/recover.go | 3 +-- cmd/mcp/tools_exec.go | 27 +++++++++++++++++---------- cmd/mcp/tools_provider.go | 18 ++++++++++++------ pkg/workspace/exec_oneshot.go | 13 +++++++++---- 4 files changed, 39 insertions(+), 22 deletions(-) diff --git a/cmd/mcp/recover.go b/cmd/mcp/recover.go index 78881b69c..bd4bbd379 100644 --- a/cmd/mcp/recover.go +++ b/cmd/mcp/recover.go @@ -22,8 +22,7 @@ func safeHandler[In any, Out any]( if r := recover(); r != nil { log.Errorf("mcp handler panic: %v\n%s", r, debug.Stack()) result = errorResult(fmt.Errorf("handler panicked: %v", r)) - var zero Out - out = zero + out = *new(Out) // reset to zero value; named returns are zero-initialized but may have been modified err = nil } }() diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index 3d90569d8..5a13d5a74 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -25,6 +25,7 @@ type execInput struct { Command []string `json:"command" jsonschema:"required"` Workdir string `json:"workdir,omitempty"` Env map[string]string `json:"env,omitempty"` + IDLabels []string `json:"id_labels,omitempty"` TimeoutSeconds int `json:"timeout_seconds,omitempty"` } @@ -60,6 +61,7 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { Command: in.Command, Workdir: in.Workdir, Env: in.Env, + IDLabels: in.IDLabels, TimeoutSeconds: in.TimeoutSeconds, TimeoutSecondsDefault: durationToSeconds(cmd.ExecTimeoutDefault), TimeoutSecondsMax: durationToSeconds(cmd.ExecTimeoutMax), @@ -69,17 +71,22 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { Stdout: stdout, Stderr: stderr, }) + // Populate output from whatever result we got, even on error (e.g. + // cancelled / timed-out exec may have written partial output that is + // useful to the caller). + out := execOutput{} + if res != nil { + out.Stdout = stdout.String() + out.Stderr = stderr.String() + out.ExitCode = res.ExitCode + out.DurationMS = res.DurationMS + out.Truncated = stdout.Truncated() || stderr.Truncated() + out.TimedOut = res.TimedOut + out.Clamped = res.Clamped + } if err != nil { - return errorResult(err), execOutput{}, nil + return errorResult(err), out, nil } - return nil, execOutput{ - Stdout: stdout.String(), - Stderr: stderr.String(), - ExitCode: res.ExitCode, - DurationMS: res.DurationMS, - Truncated: stdout.Truncated() || stderr.Truncated(), - TimedOut: res.TimedOut, - Clamped: res.Clamped, - }, nil + return nil, out, nil })) } diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index 3d831f203..1b2eed5dd 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -10,6 +10,7 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/spf13/cobra" ) type providerSummary struct { @@ -142,16 +143,21 @@ func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInp return cobraCmd.Execute() } -func runProviderDelete(ctx context.Context, g *flags.GlobalFlags, name string) error { - cobraCmd := cmdprovider.NewDeleteCmd(g) +func runProviderCobraCmd(ctx context.Context, name string, newCmd func() *cobra.Command) error { + cobraCmd := newCmd() cobraCmd.SetArgs([]string{"--", name}) cobraCmd.SetContext(ctx) return cobraCmd.Execute() } +func runProviderDelete(ctx context.Context, g *flags.GlobalFlags, name string) error { + return runProviderCobraCmd( + ctx, + name, + func() *cobra.Command { return cmdprovider.NewDeleteCmd(g) }, + ) +} + func runProviderUse(ctx context.Context, g *flags.GlobalFlags, name string) error { - cobraCmd := cmdprovider.NewUseCmd(g) - cobraCmd.SetArgs([]string{"--", name}) - cobraCmd.SetContext(ctx) - return cobraCmd.Execute() + return runProviderCobraCmd(ctx, name, func() *cobra.Command { return cmdprovider.NewUseCmd(g) }) } diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 31ec0be91..0f57c7300 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -22,6 +22,7 @@ type ExecOneShotOptions struct { Command []string Workdir string Env map[string]string + IDLabels []string // additional id-labels for container lookup; nil uses defaults TimeoutSeconds int TimeoutSecondsDefault int TimeoutSecondsMax int @@ -79,14 +80,18 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu } timeout, clamped := opts.ResolveTimeout(defaultExecTimeoutSeconds) - execCtx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - resolved, err := resolveExecTarget(execCtx, opts) + // Resolve the container target with the parent context so a slow Docker + // daemon lookup doesn't consume the user's exec time budget. + resolved, err := resolveExecTarget(ctx, opts) if err != nil { return nil, err } + // Apply the timeout only to the actual command execution. + execCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + start := time.Now() exitCode, runErr := runCapture(execCtx, captureArgs{ target: resolved.target, @@ -150,7 +155,7 @@ func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedEx dockerCommand := ResolveDockerCommand(workspaceConfig) containerDetails, err := FindRunningContainer( - ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), nil, + ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, ) if err != nil { return resolvedExecTarget{}, err From 91a375c34d12d61a319f2a30b380654b5124ae9e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:45:56 -0500 Subject: [PATCH 26/48] refactor(mcp): drop cobra round-trip for provider_delete and provider_use Extract UseProvider helper from use.go RunE and call DeleteProvider/UseProvider directly from tools_provider.go, eliminating the cobra command round-trip for these two operations. --- cmd/mcp/tools_provider.go | 26 +++++++++++--------------- cmd/provider/use.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index 1b2eed5dd..d22d660c9 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -10,7 +10,6 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/workspace" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/spf13/cobra" ) type providerSummary struct { @@ -143,21 +142,18 @@ func runProviderAdd(ctx context.Context, g *flags.GlobalFlags, in providerAddInp return cobraCmd.Execute() } -func runProviderCobraCmd(ctx context.Context, name string, newCmd func() *cobra.Command) error { - cobraCmd := newCmd() - cobraCmd.SetArgs([]string{"--", name}) - cobraCmd.SetContext(ctx) - return cobraCmd.Execute() -} - func runProviderDelete(ctx context.Context, g *flags.GlobalFlags, name string) error { - return runProviderCobraCmd( - ctx, - name, - func() *cobra.Command { return cmdprovider.NewDeleteCmd(g) }, - ) + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return err + } + return cmdprovider.DeleteProvider(ctx, devsyConfig, name, false, false) } -func runProviderUse(ctx context.Context, g *flags.GlobalFlags, name string) error { - return runProviderCobraCmd(ctx, name, func() *cobra.Command { return cmdprovider.NewUseCmd(g) }) +func runProviderUse(_ context.Context, g *flags.GlobalFlags, name string) error { + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return err + } + return cmdprovider.UseProvider(devsyConfig, name) } diff --git a/cmd/provider/use.go b/cmd/provider/use.go index d698b20d0..807601f86 100644 --- a/cmd/provider/use.go +++ b/cmd/provider/use.go @@ -11,6 +11,21 @@ import ( "github.com/spf13/cobra" ) +// UseProvider sets the named provider as the default for the active config context. +// It is the extracted core of the `provider use` RunE, exposed for non-CLI callers. +func UseProvider(devsyConfig *config.Config, name string) error { + p, err := workspace.FindProvider(devsyConfig, name) + if err != nil { + return err + } + devsyConfig.Current().DefaultProvider = p.Config.Name + if err := config.SaveConfig(devsyConfig); err != nil { + return fmt.Errorf("save config: %w", err) + } + log.Infof("default provider: %s", p.Config.Name) + return nil +} + // UseCmd holds the cmd flags. type UseCmd struct { *flags.GlobalFlags From d1450141755b8ff91d5bd32b6a443281c0ae6668 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:48:34 -0500 Subject: [PATCH 27/48] refactor(workspace,mcp): expose up.RunFromOptions and drop cobra round-trip Add up.Options struct and up.RunFromOptions for non-CLI callers to invoke the up command logic directly without marshaling arguments through cobra. Extract applyConfig helper shared by execute() and RunFromOptions(). Replace the MCP runUp cobra invocation with a direct up.RunFromOptions call, eliminating all --flag=value string construction. --- cmd/mcp/tools_workspace.go | 30 ++++------------- cmd/workspace/up/up.go | 67 +++++++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index c2fb4dd0d..a6011d2c3 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -268,27 +268,11 @@ func createWorkspace(ctx context.Context, g *flags.GlobalFlags, in createInput) } func runUp(ctx context.Context, g *flags.GlobalFlags, in createInput) error { - // Use --flag=value single-token form for every user-controlled value so a - // value starting with "-" cannot be reparsed by pflag as a separate flag. - args := []string{} - if in.Provider != "" { - args = append(args, fmt.Sprintf("--provider=%s", in.Provider)) - } - if in.IDE != "" { - args = append(args, fmt.Sprintf("--ide=%s", in.IDE)) - } else { - args = append(args, "--ide=none") - } - if in.DevcontainerPath != "" { - args = append(args, fmt.Sprintf("--devcontainer-path=%s", in.DevcontainerPath)) - } - if in.Name != "" { - args = append(args, fmt.Sprintf("--id=%s", in.Name)) - } - args = append(args, "--", in.Source) - - cobraCmd := up.NewUpCmd(g) - cobraCmd.SetArgs(args) - cobraCmd.SetContext(ctx) - return cobraCmd.Execute() + return up.RunFromOptions(ctx, g, up.Options{ + Source: in.Source, + Name: in.Name, + Provider: in.Provider, + IDE: in.IDE, + DevcontainerPath: in.DevcontainerPath, + }) } diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 6ea776030..40c71bb32 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -49,6 +49,58 @@ type UpCmd struct { DotfilesScriptEnvFile []string // Paths to files containing Key=Value pairs to pass to install script } +// Options is the structured input form of the up command, for non-CLI callers. +type Options struct { + Source string // git URL, local path, image, or workspace name + Name string // explicit workspace ID override + Provider string // provider name override + IDE string // ide name; "none" to skip launching + DevcontainerPath string // path to devcontainer.json, relative to project +} + +// RunFromOptions runs the up command's logic without going through cobra. +// This is the same code path execute() follows, exposed for callers (such as +// the MCP server) that have structured input instead of CLI args. +// NOTE: WithSignals is intentionally skipped — the caller controls cancellation +// via ctx. +func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) error { + cmd := buildUpCmd(g, opts) + if err := cmd.validate(); err != nil { + return err + } + devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + if err != nil { + return fmt.Errorf("load devsy config: %w", err) + } + cmd.applyConfig(devsyConfig) + + args := []string{opts.Source} + client, err := cmd.prepareClient(ctx, devsyConfig, args) + if err != nil { + return fmt.Errorf("prepare workspace client: %w", err) + } + if cmd.ExtraDevContainerPath != "" && client.Provider() != "docker" { + return fmt.Errorf("extra devcontainer file is only supported with local provider") + } + telemetry.CollectorCLI.SetClient(client) + return cmd.Run(ctx, devsyConfig, client, args) +} + +// buildUpCmd constructs an UpCmd from structured options for non-CLI callers. +func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { + ide := opts.IDE + if ide == "" { + ide = "none" + } + cmd := &UpCmd{GlobalFlags: g} + cmd.IDE = ide + cmd.DevContainerPath = opts.DevcontainerPath + if opts.Name != "" { + cmd.ID = opts.Name + } + return cmd +} + // NewUpCmd creates a new up command. func NewUpCmd(f *flags.GlobalFlags) *cobra.Command { cmd := &UpCmd{GlobalFlags: f} @@ -91,6 +143,15 @@ func (cmd *UpCmd) Run( }) } +// applyConfig sets config-derived fields after loading the devsy config. +// Used by both execute() and RunFromOptions(). +func (cmd *UpCmd) applyConfig(devsyConfig *config.Config) { + if devsyConfig.ContextOption(config.ContextOptionSSHStrictHostKeyChecking) == config.BoolTrue { + cmd.StrictHostKeyChecking = true + } + cmd.resolveDotfilesOptions(devsyConfig) +} + type finalizeUpArgs struct { devsyConfig *config.Config client client2.BaseWorkspaceClient @@ -191,11 +252,7 @@ func (cmd *UpCmd) execute(cobraCmd *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("load devsy config: %w", err) } - if devsyConfig.ContextOption(config.ContextOptionSSHStrictHostKeyChecking) == config.BoolTrue { - cmd.StrictHostKeyChecking = true - } - - cmd.resolveDotfilesOptions(devsyConfig) + cmd.applyConfig(devsyConfig) ctx, cancel := WithSignals(cobraCmd.Context()) defer cancel() From 56fc1c161f12d2ceb7a27c50d6e8e86b43afed08 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:51:41 -0500 Subject: [PATCH 28/48] refactor(workspace,mcp): plumb output writer through up.go instead of swapping os.Stdout Add UpCmd.Out io.Writer field and stdout() accessor (falls back to os.Stdout). Change reportErr and emitUpResult to accept an explicit writer. RunFromOptions/ buildUpCmd sets Out=io.Discard so JSON result envelopes do not reach stdout in non-CLI callers. The os.Stdout swap in serve.go is retained as a defensive backstop for remaining hardcoded os.Stdout sites outside up.go. --- cmd/mcp/serve.go | 3 +++ cmd/workspace/up/up.go | 46 +++++++++++++++++++++++++++++++++--------- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index 606f0627f..3c34ffc34 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -48,6 +48,9 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { // Reserve the real stdout for the JSON-RPC frame and redirect os.Stdout to // stderr so stray writes elsewhere in the process cannot corrupt it. + // The up.go result/error JSON envelopes are now written to io.Discard via + // the plumbed Out writer; this swap is a defensive backstop for remaining + // sites that still hardcode os.Stdout (e.g. pkg/workspace/workspace.go:480). realStdout := os.Stdout os.Stdout = os.Stderr defer func() { os.Stdout = realStdout }() diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 40c71bb32..a65e46b37 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -3,6 +3,7 @@ package up import ( "context" "fmt" + "io" "os" "os/signal" "path/filepath" @@ -47,6 +48,10 @@ type UpCmd struct { DotfilesTargetPath string DotfilesScriptEnv []string // Key=Value to pass to install script DotfilesScriptEnvFile []string // Paths to files containing Key=Value pairs to pass to install script + + // Out is the writer for structured JSON output (result/error envelopes). + // When nil, os.Stdout is used, matching the CLI default. + Out io.Writer } // Options is the structured input form of the up command, for non-CLI callers. @@ -92,7 +97,18 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { if ide == "" { ide = "none" } - cmd := &UpCmd{GlobalFlags: g} + // Use a shallow copy of GlobalFlags so we can override ResultFormat without + // mutating the caller's flags. + gCopy := *g + if gCopy.ResultFormat == "" { + gCopy.ResultFormat = "plain" + } + cmd := &UpCmd{ + GlobalFlags: &gCopy, + // MCP and other non-CLI callers do not have a structured JSON output channel; + // discard the result/error JSON so it does not corrupt stdout. + Out: io.Discard, + } cmd.IDE = ide cmd.DevContainerPath = opts.DevcontainerPath if opts.Name != "" { @@ -128,9 +144,10 @@ func (cmd *UpCmd) Run( } emitJSON := mode == output.ModeJSON + out := cmd.stdout() wctx, err := cmd.executeDevsyUp(ctx, devsyConfig, client) if err != nil { - return reportErr(err, emitJSON) + return reportErr(err, emitJSON, out) } if wctx == nil || cmd.Prebuild { return nil // Platform mode or prebuild-only run. @@ -140,6 +157,7 @@ func (cmd *UpCmd) Run( client: client, wctx: wctx, emitJSON: emitJSON, + out: out, }) } @@ -157,13 +175,14 @@ type finalizeUpArgs struct { client client2.BaseWorkspaceClient wctx *workspaceContext emitJSON bool + out io.Writer } // finalizeUp performs the post-up steps: workspace configuration, optional SSH // tunnel, IDE launch, and JSON envelope emission. Split out to keep Run small. func (cmd *UpCmd) finalizeUp(ctx context.Context, args *finalizeUpArgs) error { if err := cmd.configureWorkspace(args.devsyConfig, args.client, args.wctx); err != nil { - return reportErr(err, args.emitJSON) + return reportErr(err, args.emitJSON, args.out) } if cleanup := cmd.maybeStartTunnel( @@ -180,10 +199,10 @@ func (cmd *UpCmd) finalizeUp(ctx context.Context, args *finalizeUpArgs) error { ideURL, err := cmd.openIDE(ctx, args.devsyConfig, args.client, args.wctx) if err != nil { - return reportErr(err, args.emitJSON) + return reportErr(err, args.emitJSON, args.out) } if args.emitJSON { - emitUpResult(args.wctx, ideURL) + emitUpResult(args.wctx, ideURL, args.out) } if args.wctx.tunnelPort > 0 { log.Infof( @@ -217,16 +236,25 @@ func (cmd *UpCmd) maybeStartTunnel( return tunnelCleanup } +// stdout returns the output writer for structured JSON envelopes. +// Falls back to os.Stdout when no explicit writer is set, matching CLI behaviour. +func (cmd *UpCmd) stdout() io.Writer { + if cmd.Out != nil { + return cmd.Out + } + return os.Stdout +} + // reportErr writes the error to JSON output when requested and returns it for the caller. -func reportErr(err error, emitJSON bool) error { +func reportErr(err error, emitJSON bool, out io.Writer) error { if emitJSON { - _ = config2.WriteErrorJSON(os.Stdout, err.Error()) + _ = config2.WriteErrorJSON(out, err.Error()) } return err } // emitUpResult writes the JSON result envelope for a completed `up` invocation. -func emitUpResult(wctx *workspaceContext, ideURL string) { +func emitUpResult(wctx *workspaceContext, ideURL string, out io.Writer) { containerID := "" var warnings []string if wctx.result != nil { @@ -235,7 +263,7 @@ func emitUpResult(wctx *workspaceContext, ideURL string) { } warnings = wctx.result.HostWarnings } - _ = config2.WriteResultJSON(os.Stdout, config2.ResultEnvelope{ + _ = config2.WriteResultJSON(out, config2.ResultEnvelope{ ContainerID: containerID, RemoteUser: wctx.user, RemoteWorkspaceFolder: wctx.workdir, From 1c900898364cc4b26f1e22f81d9a765f10c1b55d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:52:57 -0500 Subject: [PATCH 29/48] refactor(mcp): switch BoundedBuffer from mid-truncation to tail-only Replace the head+tail split approach with a tail-only ring buffer. Output endings carry the final state (exit messages, errors, summaries) that LLMs need most; keeping only the tail is more useful than a middle-truncation split. Update buffer_test.go to verify tail retention and head dropping. --- cmd/mcp/buffer.go | 53 ++++++++++++++---------------- cmd/mcp/buffer_test.go | 74 +++++++++++++++++++++++++++++------------- 2 files changed, 76 insertions(+), 51 deletions(-) diff --git a/cmd/mcp/buffer.go b/cmd/mcp/buffer.go index a2e691010..6ae69653a 100644 --- a/cmd/mcp/buffer.go +++ b/cmd/mcp/buffer.go @@ -2,57 +2,52 @@ package mcp import "fmt" -// BoundedBuffer is an io.Writer that retains at most Cap bytes by keeping the -// first Cap/2 and last Cap/2 bytes written. String() reports the contents -// joined with a truncation marker when more than Cap bytes were written. +// BoundedBuffer is an io.Writer that retains at most cap bytes by keeping the +// tail (most recent bytes) of the written data. Tail-only retention is more +// useful to LLMs than a mid-truncation split because output endings carry +// the final state (exit messages, errors, summaries) rather than middles. type BoundedBuffer struct { cap int - head []byte - tail []byte + buf []byte written int64 } +// NewBoundedBuffer returns a BoundedBuffer with the given capacity. +// The minimum effective capacity is 64 bytes. func NewBoundedBuffer(cap int) *BoundedBuffer { - if cap < 8 { - cap = 8 + if cap < 64 { + cap = 64 } - if cap%2 != 0 { - cap++ - } - return &BoundedBuffer{cap: cap} + return &BoundedBuffer{cap: cap, buf: make([]byte, 0, cap)} } func (b *BoundedBuffer) Write(p []byte) (int, error) { n := len(p) b.written += int64(n) - half := b.cap / 2 - - if len(b.head) < half { - take := min(half-len(b.head), n) - b.head = append(b.head, p[:take]...) - p = p[take:] - } - if len(p) == 0 { + if len(p) >= b.cap { + // Incoming chunk fills or exceeds cap — keep only the last cap bytes. + b.buf = append(b.buf[:0], p[len(p)-b.cap:]...) return n, nil } - - b.tail = append(b.tail, p...) - if len(b.tail) > half { - b.tail = b.tail[len(b.tail)-half:] + if len(b.buf)+len(p) > b.cap { + drop := len(b.buf) + len(p) - b.cap + b.buf = b.buf[drop:] } + b.buf = append(b.buf, p...) return n, nil } -func (b *BoundedBuffer) Truncated() bool { - return b.written > int64(b.cap) -} +// Truncated reports whether more bytes were written than the buffer can hold. +func (b *BoundedBuffer) Truncated() bool { return b.written > int64(b.cap) } +// BytesWritten returns the total number of bytes written, including dropped ones. func (b *BoundedBuffer) BytesWritten() int64 { return b.written } +// String returns the buffered content. When truncated, a marker showing how +// many bytes were dropped is prepended so callers know output is incomplete. func (b *BoundedBuffer) String() string { if !b.Truncated() { - return string(b.head) + string(b.tail) + return string(b.buf) } - dropped := b.written - int64(len(b.head)) - int64(len(b.tail)) - return fmt.Sprintf("%s\n... [%d bytes truncated] ...\n%s", b.head, dropped, b.tail) + return fmt.Sprintf("... [%d bytes dropped] ...\n%s", b.written-int64(len(b.buf)), b.buf) } diff --git a/cmd/mcp/buffer_test.go b/cmd/mcp/buffer_test.go index 7e41cc8cd..94fd838e8 100644 --- a/cmd/mcp/buffer_test.go +++ b/cmd/mcp/buffer_test.go @@ -16,42 +16,72 @@ func TestBoundedBuffer_NoTruncation(t *testing.T) { } } -func TestBoundedBuffer_TruncatesMiddle(t *testing.T) { - b := NewBoundedBuffer(20) - _, _ = b.Write([]byte(strings.Repeat("a", 50))) +func TestBoundedBuffer_TruncatesHead(t *testing.T) { + // Write 130 bytes into a cap-64 buffer: only the last 64 bytes (tail) should survive. + b := NewBoundedBuffer(64) + _, _ = b.Write([]byte(strings.Repeat("a", 66) + strings.Repeat("b", 64))) s := b.String() if !b.Truncated() { t.Fatal("expected truncated") } - if !strings.Contains(s, "bytes truncated") { - t.Fatalf("missing marker: %q", s) + if !strings.Contains(s, "bytes dropped") { + t.Fatalf("missing drop marker: %q", s) } - if !strings.HasPrefix(s, "aaaa") || !strings.HasSuffix(s, "aaaa") { - t.Fatalf("head/tail not preserved: %q", s) + // The tail (last 64 bytes, all 'b') should be preserved. + if !strings.HasSuffix(s, strings.Repeat("b", 64)) { + t.Fatalf("tail not preserved: %q", s) + } + // The head ('a' bytes) should be dropped. + if strings.Contains(s, "aaaa") { + t.Fatalf("head should be dropped but was retained: %q", s) } } func TestBoundedBuffer_MultipleWritesAccumulate(t *testing.T) { - b := NewBoundedBuffer(10) + b := NewBoundedBuffer(64) // min cap is 64 for range 5 { - _, _ = b.Write([]byte("xxxx")) + _, _ = b.Write([]byte(strings.Repeat("x", 20))) } + // 100 bytes written into cap 64: should be truncated. if !b.Truncated() { - t.Fatal("expected truncated after 20 bytes into cap 10") + t.Fatal("expected truncated after 100 bytes into cap 64") + } +} + +func TestBoundedBuffer_TailPreservedAcrossSmallWrites(t *testing.T) { + // Write many small chunks; the buffer should hold the most recent cap bytes. + b := NewBoundedBuffer(64) + for i := range 200 { + _, _ = b.Write([]byte{byte('a' + (i % 26))}) + } + if !b.Truncated() { + t.Fatal("expected truncated") + } + s := b.String() + // The last 64 chars of the 200-char sequence. + var want strings.Builder + for i := 136; i < 200; i++ { + want.WriteString(string([]byte{byte('a' + (i % 26))})) + } + if !strings.HasSuffix(s, want.String()) { + t.Fatalf( + "tail not preserved: got suffix %q, want %q", + s[len(s)-len(want.String()):], + want.String(), + ) } } -func TestBoundedBuffer_OddCap(t *testing.T) { - // Odd cap should be rounded up so head+tail covers everything when - // written exactly cap bytes. - b2 := NewBoundedBuffer(101) // odd, > floor; rounded up to 102 - for range 101 { - _, _ = b2.Write([]byte("a")) - } - if b2.Truncated() { - t.Fatal("at exactly 101 bytes into cap 102, should not be truncated") - } - if got := b2.String(); len(got) != 101 { - t.Fatalf("expected 101 bytes, got %d", len(got)) +func TestBoundedBuffer_LargeChunkOverwrite(t *testing.T) { + // A single write larger than cap should keep only the last cap bytes. + b := NewBoundedBuffer(64) + data := strings.Repeat("a", 30) + strings.Repeat("b", 64) + _, _ = b.Write([]byte(data)) + s := b.String() + if !b.Truncated() { + t.Fatal("expected truncated") + } + if !strings.HasSuffix(s, strings.Repeat("b", 64)) { + t.Fatalf("last cap bytes not preserved: %q", s) } } From 20a1a5a5be97c98be58201281ed697f015820ee3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:53:53 -0500 Subject: [PATCH 30/48] refactor(mcp): collapse repeated opOK handler boilerplate Add opResultHandler helper that wraps a void function into the standard (errorResult | opOK{OK: true}) pattern. Replace the repeated three-line if-err-return blocks in workspace and provider lifecycle handlers with single opResultHandler calls. --- cmd/mcp/helpers.go | 12 ++++++++++++ cmd/mcp/tools_provider.go | 15 +++------------ cmd/mcp/tools_workspace.go | 15 +++------------ 3 files changed, 18 insertions(+), 24 deletions(-) create mode 100644 cmd/mcp/helpers.go diff --git a/cmd/mcp/helpers.go b/cmd/mcp/helpers.go new file mode 100644 index 000000000..fffea4d2a --- /dev/null +++ b/cmd/mcp/helpers.go @@ -0,0 +1,12 @@ +package mcp + +import sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + +// opResultHandler is the standard (errorResult | opOK) wrapper for void operations. +// It calls fn and returns either an error result or opOK{OK: true}. +func opResultHandler(fn func() error) (*sdkmcp.CallToolResult, opOK, error) { + if err := fn(); err != nil { + return errorResult(err), opOK{}, nil + } + return nil, opOK{OK: true}, nil +} diff --git a/cmd/mcp/tools_provider.go b/cmd/mcp/tools_provider.go index d22d660c9..ccc1667ef 100644 --- a/cmd/mcp/tools_provider.go +++ b/cmd/mcp/tools_provider.go @@ -55,10 +55,7 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Source == "" { return errorResult(fmt.Errorf("source is required")), opOK{}, nil } - if err := runProviderAdd(ctx, g, in); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return runProviderAdd(ctx, g, in) }) })) sdkmcp.AddTool(s, &sdkmcp.Tool{ @@ -70,10 +67,7 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), opOK{}, nil } - if err := runProviderDelete(ctx, g, in.Name); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return runProviderDelete(ctx, g, in.Name) }) })) sdkmcp.AddTool(s, &sdkmcp.Tool{ @@ -85,10 +79,7 @@ func registerProviderTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), opOK{}, nil } - if err := runProviderUse(ctx, g, in.Name); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return runProviderUse(ctx, g, in.Name) }) })) } diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index a6011d2c3..d4c3afd86 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -143,10 +143,7 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), opOK{}, nil } - if err := startWorkspace(ctx, g, in.Name); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return startWorkspace(ctx, g, in.Name) }) })) sdkmcp.AddTool(s, &sdkmcp.Tool{ @@ -158,10 +155,7 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), opOK{}, nil } - if err := stopWorkspace(ctx, g, in.Name); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return stopWorkspace(ctx, g, in.Name) }) })) sdkmcp.AddTool(s, &sdkmcp.Tool{ @@ -173,10 +167,7 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { if in.Name == "" { return errorResult(fmt.Errorf("name is required")), opOK{}, nil } - if err := deleteWorkspace(ctx, g, in.Name, in.Force); err != nil { - return errorResult(err), opOK{}, nil - } - return nil, opOK{OK: true}, nil + return opResultHandler(func() error { return deleteWorkspace(ctx, g, in.Name, in.Force) }) })) sdkmcp.AddTool(s, &sdkmcp.Tool{ From 556a22a9fa73402a80d6608e9a142cdd6c0d151d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:54:26 -0500 Subject: [PATCH 31/48] fix(workspace): tighten WithSignals cleanup to handle in-flight signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After ctx.Done() fires in goroutine-2, add a non-blocking check of done before waiting for a second signal. If cleanup has already started (done is ready), return immediately rather than potentially triggering os.Exit(1) on a signal that arrived during cleanup. WithSignals is now only called from execute() — RunFromOptions skips it since MCP callers control cancellation via ctx. --- cmd/workspace/up/up.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index a65e46b37..3408c5699 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -434,9 +434,17 @@ func WithSignals(ctx context.Context) (context.Context, func()) { case <-done: return } + // Check whether ctx.Done() was caused by cleanup (done is also ready) + // rather than by the first goroutine catching a signal. If cleanup is + // already in progress there is no need to wait for a second signal. + select { + case <-done: + return + default: + } select { case <-signals: - // force shutdown if context is done and another signal arrives + // A second signal arrived before cleanup finished — force shutdown. os.Exit(1) case <-done: } From acd644d052d3e224aaa1a40bdfc256de157aad76 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 16:54:59 -0500 Subject: [PATCH 32/48] docs(mcp): document TOCTOU window in workspace_start The Get+RunFromOptions sequence has a small race window where a concurrent delete could remove the workspace between the existence check and the up call. Document the acceptable window and the rationale for not closing it. --- cmd/mcp/tools_workspace.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index d4c3afd86..3722cc266 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -185,9 +185,15 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { } func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { - // up.NewUpCmd treats the positional argument as either an existing - // workspace name or a source from which to create one. workspace_start - // must never create, so confirm the workspace exists first. + // workspace_start must never create a new workspace. RunFromOptions treats + // the positional argument as either an existing workspace name or a new + // source, so we confirm the workspace exists before calling it. + // + // NOTE: There is an inherent TOCTOU window between the Get check and the + // RunFromOptions call — a concurrent delete could remove the workspace in + // that gap. This is acceptable: the window is small and closing it would + // require distributed locking. The resulting error from RunFromOptions is + // user-visible and actionable. devsyConfig, err := config.LoadConfig(g.Context, g.Provider) if err != nil { return err From 9e0200f61d1954da098c1c5691a9205d6317903a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:14:14 -0500 Subject: [PATCH 33/48] fix(workspace): propagate up.Options.Provider through LoadConfig buildUpCmd took opts.Provider and never assigned it to the GlobalFlags copy, so the subsequent config.LoadConfig call always used the caller's default provider. Apply the override on the copy and read it back from cmd.GlobalFlags so RunFromOptions honors the provider the caller asked for. --- cmd/workspace/up/up.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 3408c5699..42b4c926e 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -64,16 +64,16 @@ type Options struct { } // RunFromOptions runs the up command's logic without going through cobra. -// This is the same code path execute() follows, exposed for callers (such as -// the MCP server) that have structured input instead of CLI args. -// NOTE: WithSignals is intentionally skipped — the caller controls cancellation -// via ctx. +// Exposed for non-CLI callers that already have structured input and their own +// context cancellation; WithSignals is intentionally skipped. func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) error { cmd := buildUpCmd(g, opts) if err := cmd.validate(); err != nil { return err } - devsyConfig, err := config.LoadConfig(g.Context, g.Provider) + // Read overrides from cmd.GlobalFlags (the copy), not the caller's g — + // buildUpCmd may have applied opts.Provider on top of g.Provider. + devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider) if err != nil { return fmt.Errorf("load devsy config: %w", err) } @@ -97,16 +97,20 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { if ide == "" { ide = "none" } - // Use a shallow copy of GlobalFlags so we can override ResultFormat without - // mutating the caller's flags. + // Shallow-copy GlobalFlags so per-call overrides (ResultFormat, Provider) + // don't mutate the caller's flags. gCopy := *g if gCopy.ResultFormat == "" { gCopy.ResultFormat = "plain" } + if opts.Provider != "" { + gCopy.Provider = opts.Provider + } cmd := &UpCmd{ GlobalFlags: &gCopy, - // MCP and other non-CLI callers do not have a structured JSON output channel; - // discard the result/error JSON so it does not corrupt stdout. + // Non-CLI callers don't consume the result/error JSON envelopes and may + // share stdout with a transport (e.g. MCP stdio) that would be corrupted + // by them; discard by default. Out: io.Discard, } cmd.IDE = ide From aa1187bbc2da8355943dc47901b03ca86c498f2a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:14:20 -0500 Subject: [PATCH 34/48] fix(mcp): preserve classified error in execOutput on partial-output failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP SDK marshals the typed handler's return value and overwrites the CallToolResult.StructuredContent with it, so the ErrorPayload built by errorResult was destroyed whenever a cancelled or timed-out exec also produced partial stdout/stderr. Embed the classification inside execOutput.Error so it survives the round-trip alongside the partial output. Read the bounded buffers unconditionally as well — early failure paths that wrote to the buffers before res was constructed no longer drop that output silently. --- cmd/mcp/tools_exec.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index 5a13d5a74..4e9c3a22b 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -37,6 +37,11 @@ type execOutput struct { Truncated bool `json:"truncated"` TimedOut bool `json:"timed_out,omitempty"` Clamped bool `json:"clamped,omitempty"` + // Error carries the classified error payload when the exec failed with + // partial output already captured. The MCP SDK overwrites a result's + // StructuredContent with the marshalled typed output, so the classification + // has to ride inside execOutput itself to survive the round-trip. + Error *ErrorPayload `json:"error,omitempty"` } func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { @@ -71,20 +76,23 @@ func registerExecTool(s *sdkmcp.Server, cmd *ServeCmd) { Stdout: stdout, Stderr: stderr, }) - // Populate output from whatever result we got, even on error (e.g. - // cancelled / timed-out exec may have written partial output that is - // useful to the caller). - out := execOutput{} + // Populate output from whatever was captured. A cancelled or timed-out + // exec may still have written partial stdout/stderr that's useful to + // the caller, so read the buffers unconditionally. + out := execOutput{ + Stdout: stdout.String(), + Stderr: stderr.String(), + Truncated: stdout.Truncated() || stderr.Truncated(), + } if res != nil { - out.Stdout = stdout.String() - out.Stderr = stderr.String() out.ExitCode = res.ExitCode out.DurationMS = res.DurationMS - out.Truncated = stdout.Truncated() || stderr.Truncated() out.TimedOut = res.TimedOut out.Clamped = res.Clamped } if err != nil { + payload := ClassifyError(err) + out.Error = &payload return errorResult(err), out, nil } return nil, out, nil From 86f52c765eb873c0f39e0e15e84723b3c89e30e1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:14:25 -0500 Subject: [PATCH 35/48] docs(mcp): tighten comments that referenced rot-prone context Drop file:line citations, temporal markers ("now"), and specific caller names ("MCP server") from comments that explained WHY. Replace "distributed locking" in the startWorkspace TOCTOU note with an accurate description of the trade-off. --- cmd/mcp/helpers_test.go | 41 +++++++++++++++++++++++++++++++++++++ cmd/mcp/recover.go | 4 +++- cmd/mcp/serve.go | 8 ++++---- cmd/mcp/tools_workspace.go | 13 +++++------- cmd/workspace/up/up_test.go | 31 ++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 cmd/mcp/helpers_test.go diff --git a/cmd/mcp/helpers_test.go b/cmd/mcp/helpers_test.go new file mode 100644 index 000000000..1874bb7a8 --- /dev/null +++ b/cmd/mcp/helpers_test.go @@ -0,0 +1,41 @@ +package mcp + +import ( + "errors" + "testing" +) + +func TestOpResultHandler_Success(t *testing.T) { + result, ok, err := opResultHandler(func() error { return nil }) + if err != nil { + t.Fatalf("unexpected go error: %v", err) + } + if result != nil { + t.Fatalf("success path must return nil *CallToolResult, got %+v", result) + } + if !ok.OK { + t.Fatalf("success path must report ok.OK=true") + } +} + +func TestOpResultHandler_Error(t *testing.T) { + sentinel := errors.New("boom") + result, ok, err := opResultHandler(func() error { return sentinel }) + if err != nil { + t.Fatalf( + "opResultHandler must always return nil go-error so the SDK uses our *CallToolResult, got %v", + err, + ) + } + if result == nil { + t.Fatalf( + "error path must return a non-nil *CallToolResult so the SDK reports IsError to the client", + ) + } + if !result.IsError { + t.Fatalf("error path must mark the result IsError=true") + } + if ok.OK { + t.Fatalf("error path must leave ok zero") + } +} diff --git a/cmd/mcp/recover.go b/cmd/mcp/recover.go index bd4bbd379..75c438e2d 100644 --- a/cmd/mcp/recover.go +++ b/cmd/mcp/recover.go @@ -22,7 +22,9 @@ func safeHandler[In any, Out any]( if r := recover(); r != nil { log.Errorf("mcp handler panic: %v\n%s", r, debug.Stack()) result = errorResult(fmt.Errorf("handler panicked: %v", r)) - out = *new(Out) // reset to zero value; named returns are zero-initialized but may have been modified + // The panic may have left out half-populated; force the zero + // value so callers never see a partial result alongside an error. + out = *new(Out) err = nil } }() diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index 3c34ffc34..7e56f5e3c 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -47,10 +47,10 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) // Reserve the real stdout for the JSON-RPC frame and redirect os.Stdout to - // stderr so stray writes elsewhere in the process cannot corrupt it. - // The up.go result/error JSON envelopes are now written to io.Discard via - // the plumbed Out writer; this swap is a defensive backstop for remaining - // sites that still hardcode os.Stdout (e.g. pkg/workspace/workspace.go:480). + // stderr. Up.go's JSON envelopes are routed through an injected writer, but + // other code paths in pkg/workspace still write directly to os.Stdout for + // interactive prompts and progress. Belt-and-suspenders against any such + // site corrupting the MCP stdio transport. realStdout := os.Stdout os.Stdout = os.Stderr defer func() { os.Stdout = realStdout }() diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 3722cc266..2e9124e29 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -186,14 +186,11 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { // workspace_start must never create a new workspace. RunFromOptions treats - // the positional argument as either an existing workspace name or a new - // source, so we confirm the workspace exists before calling it. - // - // NOTE: There is an inherent TOCTOU window between the Get check and the - // RunFromOptions call — a concurrent delete could remove the workspace in - // that gap. This is acceptable: the window is small and closing it would - // require distributed locking. The resulting error from RunFromOptions is - // user-visible and actionable. + // its positional argument as either an existing workspace name or a new + // source, so confirm the workspace exists first. The Get→RunFromOptions + // sequence has a TOCTOU window if another caller deletes the workspace in + // between; RunFromOptions returns an actionable error in that case, which + // is acceptable for an interactive tool. devsyConfig, err := config.LoadConfig(g.Context, g.Provider) if err != nil { return err diff --git a/cmd/workspace/up/up_test.go b/cmd/workspace/up/up_test.go index d03533837..9756bc7da 100644 --- a/cmd/workspace/up/up_test.go +++ b/cmd/workspace/up/up_test.go @@ -277,3 +277,34 @@ func TestUpCmd_ValidateMounts(t *testing.T) { }) } } + +func TestBuildUpCmd_AppliesOptions(t *testing.T) { + g := &flags.GlobalFlags{Provider: "default-provider", ResultFormat: ""} + opts := Options{ + Source: "github.com/example/repo", + Name: "my-ws", + Provider: "k8s", + IDE: "vscode", + DevcontainerPath: ".devcontainer/devcontainer.json", + } + cmd := buildUpCmd(g, opts) + + assert.Equal(t, "vscode", cmd.IDE) + assert.Equal(t, ".devcontainer/devcontainer.json", cmd.DevContainerPath) + assert.Equal(t, "my-ws", cmd.ID) + assert.Equal(t, "k8s", cmd.Provider, "Provider override must reach LoadConfig via gCopy") + assert.Equal(t, "plain", cmd.ResultFormat, "default ResultFormat ensures human-readable output") + require.NotNil(t, cmd.Out, "Out must be set to suppress JSON envelope writes to stdout") + assert.Equal(t, "default-provider", g.Provider, "caller's GlobalFlags must not be mutated") +} + +func TestBuildUpCmd_DefaultsIDEToNone(t *testing.T) { + g := &flags.GlobalFlags{} + cmd := buildUpCmd(g, Options{Source: "src"}) + assert.Equal( + t, + "none", + cmd.IDE, + "MCP path must default IDE to none — there's no human to attach an IDE to", + ) +} From 913146db92e8e2edc617f2ec640844cb53dcf5ca Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:18:42 -0500 Subject: [PATCH 36/48] style(mcp,workspace): trim wordy comments Comments now explain the non-obvious WHY in one or two lines instead of narrating context. Removes 'extracted core', file:line citations, and restatements of what nearby code already says. --- cmd/mcp/buffer.go | 10 ++++------ cmd/mcp/recover.go | 8 +++----- cmd/mcp/serve.go | 7 ++----- cmd/mcp/tools_exec.go | 12 +++++------- cmd/mcp/tools_workspace.go | 9 +++------ cmd/provider/use.go | 1 - cmd/workspace/up/up.go | 31 +++++++++++-------------------- pkg/workspace/exec_oneshot.go | 25 ++++++++----------------- 8 files changed, 36 insertions(+), 67 deletions(-) diff --git a/cmd/mcp/buffer.go b/cmd/mcp/buffer.go index 6ae69653a..6557f2cf8 100644 --- a/cmd/mcp/buffer.go +++ b/cmd/mcp/buffer.go @@ -2,18 +2,16 @@ package mcp import "fmt" -// BoundedBuffer is an io.Writer that retains at most cap bytes by keeping the -// tail (most recent bytes) of the written data. Tail-only retention is more -// useful to LLMs than a mid-truncation split because output endings carry -// the final state (exit messages, errors, summaries) rather than middles. +// BoundedBuffer is an io.Writer that keeps only the last cap bytes written. +// Tail retention beats mid-truncation for command output because endings carry +// exit status, errors, and final state. type BoundedBuffer struct { cap int buf []byte written int64 } -// NewBoundedBuffer returns a BoundedBuffer with the given capacity. -// The minimum effective capacity is 64 bytes. +// NewBoundedBuffer returns a BoundedBuffer with the given capacity (minimum 64). func NewBoundedBuffer(cap int) *BoundedBuffer { if cap < 64 { cap = 64 diff --git a/cmd/mcp/recover.go b/cmd/mcp/recover.go index 75c438e2d..a382e384e 100644 --- a/cmd/mcp/recover.go +++ b/cmd/mcp/recover.go @@ -9,9 +9,8 @@ import ( sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) -// safeHandler wraps a typed MCP handler with panic recovery so a single broken -// tool cannot tear down the server. A recovered panic is logged with a stack -// trace and returned as a structured tool error. +// safeHandler wraps a typed MCP handler so a panic in one tool doesn't kill +// the server. Recovered panics are logged with a stack and returned as a tool error. func safeHandler[In any, Out any]( inner func(context.Context, *sdkmcp.CallToolRequest, In) (*sdkmcp.CallToolResult, Out, error), ) func(context.Context, *sdkmcp.CallToolRequest, In) (*sdkmcp.CallToolResult, Out, error) { @@ -22,8 +21,7 @@ func safeHandler[In any, Out any]( if r := recover(); r != nil { log.Errorf("mcp handler panic: %v\n%s", r, debug.Stack()) result = errorResult(fmt.Errorf("handler panicked: %v", r)) - // The panic may have left out half-populated; force the zero - // value so callers never see a partial result alongside an error. + // Reset in case the handler partially populated out before panicking. out = *new(Out) err = nil } diff --git a/cmd/mcp/serve.go b/cmd/mcp/serve.go index 7e56f5e3c..7f3ddcedb 100644 --- a/cmd/mcp/serve.go +++ b/cmd/mcp/serve.go @@ -46,11 +46,8 @@ func (cmd *ServeCmd) Run(ctx context.Context) error { log.Debugf("starting MCP server (timeout default=%s max=%s cap=%dB)", cmd.ExecTimeoutDefault, cmd.ExecTimeoutMax, cmd.ExecOutputCap) - // Reserve the real stdout for the JSON-RPC frame and redirect os.Stdout to - // stderr. Up.go's JSON envelopes are routed through an injected writer, but - // other code paths in pkg/workspace still write directly to os.Stdout for - // interactive prompts and progress. Belt-and-suspenders against any such - // site corrupting the MCP stdio transport. + // Reserve real stdout for the JSON-RPC frame; redirect os.Stdout to stderr + // so any stray write elsewhere in the process can't corrupt the transport. realStdout := os.Stdout os.Stdout = os.Stderr defer func() { os.Stdout = realStdout }() diff --git a/cmd/mcp/tools_exec.go b/cmd/mcp/tools_exec.go index 4e9c3a22b..c68ff5fb3 100644 --- a/cmd/mcp/tools_exec.go +++ b/cmd/mcp/tools_exec.go @@ -10,9 +10,8 @@ import ( sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" ) -// durationToSeconds converts a Duration to a positive whole number of seconds, -// rounding up so that any non-zero sub-second value resolves to at least 1s -// rather than truncating to 0 (which would silently fall through to defaults). +// durationToSeconds rounds up so any non-zero sub-second value becomes at +// least 1s instead of truncating to 0 and falling through to defaults. func durationToSeconds(d time.Duration) int { if d <= 0 { return 0 @@ -37,10 +36,9 @@ type execOutput struct { Truncated bool `json:"truncated"` TimedOut bool `json:"timed_out,omitempty"` Clamped bool `json:"clamped,omitempty"` - // Error carries the classified error payload when the exec failed with - // partial output already captured. The MCP SDK overwrites a result's - // StructuredContent with the marshalled typed output, so the classification - // has to ride inside execOutput itself to survive the round-trip. + // Error rides inside execOutput because the SDK overwrites + // CallToolResult.StructuredContent with the marshalled typed output, which + // would otherwise drop the classified payload on partial-output failures. Error *ErrorPayload `json:"error,omitempty"` } diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 2e9124e29..987e040b1 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -185,12 +185,9 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { } func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { - // workspace_start must never create a new workspace. RunFromOptions treats - // its positional argument as either an existing workspace name or a new - // source, so confirm the workspace exists first. The Get→RunFromOptions - // sequence has a TOCTOU window if another caller deletes the workspace in - // between; RunFromOptions returns an actionable error in that case, which - // is acceptable for an interactive tool. + // RunFromOptions creates the workspace if the name doesn't resolve, so we + // pre-check existence to keep workspace_start lookup-only. A TOCTOU race + // with a concurrent delete just surfaces as an actionable error. devsyConfig, err := config.LoadConfig(g.Context, g.Provider) if err != nil { return err diff --git a/cmd/provider/use.go b/cmd/provider/use.go index 807601f86..67903547b 100644 --- a/cmd/provider/use.go +++ b/cmd/provider/use.go @@ -12,7 +12,6 @@ import ( ) // UseProvider sets the named provider as the default for the active config context. -// It is the extracted core of the `provider use` RunE, exposed for non-CLI callers. func UseProvider(devsyConfig *config.Config, name string) error { p, err := workspace.FindProvider(devsyConfig, name) if err != nil { diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 42b4c926e..02c890850 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -49,8 +49,7 @@ type UpCmd struct { DotfilesScriptEnv []string // Key=Value to pass to install script DotfilesScriptEnvFile []string // Paths to files containing Key=Value pairs to pass to install script - // Out is the writer for structured JSON output (result/error envelopes). - // When nil, os.Stdout is used, matching the CLI default. + // Out receives result/error JSON envelopes; nil falls back to os.Stdout (CLI default). Out io.Writer } @@ -63,16 +62,14 @@ type Options struct { DevcontainerPath string // path to devcontainer.json, relative to project } -// RunFromOptions runs the up command's logic without going through cobra. -// Exposed for non-CLI callers that already have structured input and their own -// context cancellation; WithSignals is intentionally skipped. +// RunFromOptions runs the up logic without cobra, for callers with structured +// input and their own context cancellation. WithSignals is intentionally skipped. func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) error { cmd := buildUpCmd(g, opts) if err := cmd.validate(); err != nil { return err } - // Read overrides from cmd.GlobalFlags (the copy), not the caller's g — - // buildUpCmd may have applied opts.Provider on top of g.Provider. + // Read from cmd.GlobalFlags (the copy) so opts.Provider overrides take effect. devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider) if err != nil { return fmt.Errorf("load devsy config: %w", err) @@ -97,8 +94,7 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { if ide == "" { ide = "none" } - // Shallow-copy GlobalFlags so per-call overrides (ResultFormat, Provider) - // don't mutate the caller's flags. + // Shallow-copy so per-call overrides don't mutate the caller's flags. gCopy := *g if gCopy.ResultFormat == "" { gCopy.ResultFormat = "plain" @@ -108,9 +104,8 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { } cmd := &UpCmd{ GlobalFlags: &gCopy, - // Non-CLI callers don't consume the result/error JSON envelopes and may - // share stdout with a transport (e.g. MCP stdio) that would be corrupted - // by them; discard by default. + // Discard JSON envelopes so they can't corrupt a shared stdout (e.g. + // an MCP stdio transport). Out: io.Discard, } cmd.IDE = ide @@ -418,9 +413,8 @@ func WithSignals(ctx context.Context) (context.Context, func()) { signals := make(chan os.Signal, 1) signal.Notify(signals, os.Interrupt, syscall.SIGHUP, syscall.SIGTERM, syscall.SIGQUIT) - // done is closed by the returned cleanup so both goroutines exit when the - // caller finishes — signal.Stop alone is not enough because a goroutine - // already blocked on <-signals will never unblock once Stop is called. + // done lets cleanup unblock goroutines parked on <-signals; signal.Stop + // alone wouldn't wake them. done := make(chan struct{}) go func() { @@ -438,9 +432,7 @@ func WithSignals(ctx context.Context) (context.Context, func()) { case <-done: return } - // Check whether ctx.Done() was caused by cleanup (done is also ready) - // rather than by the first goroutine catching a signal. If cleanup is - // already in progress there is no need to wait for a second signal. + // Skip the second-signal wait if cleanup already closed done. select { case <-done: return @@ -448,8 +440,7 @@ func WithSignals(ctx context.Context) (context.Context, func()) { } select { case <-signals: - // A second signal arrived before cleanup finished — force shutdown. - os.Exit(1) + os.Exit(1) // second signal — force shutdown case <-done: } }() diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 0f57c7300..e7e988002 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -33,9 +33,9 @@ type ExecOneShotOptions struct { Stderr io.Writer } -// defaultExecTimeoutSeconds is used when neither the caller nor the configured -// default supplies a positive timeout. Kept short enough to surface hung calls -// quickly while long enough to allow typical build/test commands to complete. +// defaultExecTimeoutSeconds bounds an exec when no caller or configured +// default applies. Tuned to surface hung calls without truncating typical +// build/test commands. const defaultExecTimeoutSeconds = 300 // ExecOneShotResult is the structured outcome of an exec. @@ -47,13 +47,8 @@ type ExecOneShotResult struct { Clamped bool } -// ResolveTimeout returns the effective timeout and whether the caller's -// requested value was clamped down by Max. Precedence: -// 1. TimeoutSeconds if > 0 -// 2. TimeoutSecondsDefault if > 0 -// 3. fallbackDefault -// -// The result is clamped by TimeoutSecondsMax if > 0. +// ResolveTimeout picks the first positive of TimeoutSeconds, TimeoutSecondsDefault, +// fallbackDefault, then clamps by TimeoutSecondsMax. The bool reports a clamp. func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, bool) { want := o.TimeoutSeconds if want <= 0 { @@ -81,14 +76,12 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu timeout, clamped := opts.ResolveTimeout(defaultExecTimeoutSeconds) - // Resolve the container target with the parent context so a slow Docker - // daemon lookup doesn't consume the user's exec time budget. + // Resolve under the parent context so docker lookup doesn't eat the exec budget. resolved, err := resolveExecTarget(ctx, opts) if err != nil { return nil, err } - // Apply the timeout only to the actual command execution. execCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -109,10 +102,8 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu Clamped: clamped, TimeoutSeconds: int(timeout.Seconds()), } - // Distinguish "our timeout fired" from "the caller cancelled or expired". - // Check the parent context first — if it carries an error, execCtx - // inherits the same DeadlineExceeded/Canceled and we must not claim - // TimedOut, which means "the exec exceeded ITS own time budget". + // Parent error first — execCtx inherits its cancellation, so we'd otherwise + // misreport caller cancellation as our own timeout. if parentErr := ctx.Err(); parentErr != nil { res.ExitCode = -1 return res, parentErr From b9b11235575c58f604d48cba3bc082f8df549fc5 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:24:05 -0500 Subject: [PATCH 37/48] fix(workspace): preserve CLI defaults for *bool flags in RunFromOptions buildUpCmd left MountWorkspaceGitRoot nil, while the CLI registers it with a default of true via cobra. The MCP path therefore got a different mount behavior than the documented CLI default. Mirror the flag default inside buildUpCmd. Also surface 'no provider configured' as a clear error from RunFromOptions instead of letting the empty provider name fall through into a confusing downstream lookup failure. --- cmd/workspace/up/up.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 02c890850..ae2c39e6c 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -76,6 +76,13 @@ func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) err } cmd.applyConfig(devsyConfig) + // Surface the missing-provider case here so non-CLI callers don't see a + // confusing downstream lookup error. + if cmd.Provider == "" && devsyConfig.Current().DefaultProvider == "" { + return fmt.Errorf("no provider specified and no default provider configured for context %q", + cmd.Context) + } + args := []string{opts.Source} client, err := cmd.prepareClient(ctx, devsyConfig, args) if err != nil { @@ -113,6 +120,10 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { if opts.Name != "" { cmd.ID = opts.Name } + // Mirror CLI flag defaults that ship as *bool so non-CLI callers don't + // silently get nil-pointer or false-default behavior. + mountGitRootDefault := true + cmd.MountWorkspaceGitRoot = &mountGitRootDefault return cmd } From c7aab7b476e282c9dba78313a311be08c7e2f2c1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:24:10 -0500 Subject: [PATCH 38/48] fix(mcp): preserve panic visibility through SDK SetError safeHandler was returning a custom CallToolResult on panic and nil go-error. The SDK's typed handler path overwrites StructuredContent with the marshalled zero Out, so the classified error payload was lost exactly when it mattered. Return a non-nil go-error instead; the SDK's own SetError builds a result that survives the round-trip. --- cmd/mcp/recover.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cmd/mcp/recover.go b/cmd/mcp/recover.go index a382e384e..fdcf5a19a 100644 --- a/cmd/mcp/recover.go +++ b/cmd/mcp/recover.go @@ -20,10 +20,12 @@ func safeHandler[In any, Out any]( defer func() { if r := recover(); r != nil { log.Errorf("mcp handler panic: %v\n%s", r, debug.Stack()) - result = errorResult(fmt.Errorf("handler panicked: %v", r)) - // Reset in case the handler partially populated out before panicking. + // Return a non-nil Go error so the SDK takes over with SetError. + // Returning a custom result here would lose its StructuredContent + // when the SDK marshals the (zero) typed Out over it. + result = nil out = *new(Out) - err = nil + err = fmt.Errorf("handler panicked: %v", r) } }() return inner(ctx, req, in) From d63c82caad209306326881a5d57346b89d7396b3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:24:15 -0500 Subject: [PATCH 39/48] fix(workspace): wrap non-ExitError exec failures with container context --- cmd/workspace/up/up_test.go | 20 ++++++++++++++++++++ pkg/workspace/exec_oneshot.go | 2 +- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/cmd/workspace/up/up_test.go b/cmd/workspace/up/up_test.go index 9756bc7da..7c756d289 100644 --- a/cmd/workspace/up/up_test.go +++ b/cmd/workspace/up/up_test.go @@ -308,3 +308,23 @@ func TestBuildUpCmd_DefaultsIDEToNone(t *testing.T) { "MCP path must default IDE to none — there's no human to attach an IDE to", ) } + +func TestBuildUpCmd_DoesNotMutateCallerGlobalFlags(t *testing.T) { + g := &flags.GlobalFlags{Provider: "default-provider", ResultFormat: ""} + + // Two calls with different overrides must each see a clean copy. If the + // shallow-copy guard regressed, the second call would inherit the first + // call's override. + first := buildUpCmd(g, Options{Source: "src1", Provider: "alpha"}) + second := buildUpCmd(g, Options{Source: "src2", Provider: "beta"}) + + assert.Equal(t, "alpha", first.Provider, "first call applies its own override") + assert.Equal(t, "beta", second.Provider, "second call applies its own override") + assert.Equal(t, "default-provider", g.Provider, "caller's Provider must remain untouched") + assert.Equal( + t, + "", + g.ResultFormat, + "caller's ResultFormat must remain untouched even after copy defaulted it", + ) +} diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index e7e988002..559908e3d 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -220,7 +220,7 @@ func runCapture(ctx context.Context, args captureArgs) (int, error) { if errors.As(err, &exitErr) { return exitErr.ExitCode(), nil } - return -1, err + return -1, fmt.Errorf("exec in container %s: %w", args.target.ContainerID, err) } func envMapToSlice(m map[string]string) []string { From 55f67714b32720994f96d814960782bae7c79811 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:44:43 -0500 Subject: [PATCH 40/48] fix(workspace): honor --docker-path in workspace-folder exec branch The runWithContainerID branch already used cmd.DockerPath when set, but the workspace-folder branch always took whatever ResolveDockerCommand returned, so a user passing --docker-path with --workspace-folder got the wrong docker binary. --- cmd/workspace/exec.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index 06eb84c40..9cfcb3260 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -146,6 +146,9 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { workspaceConfig := client.WorkspaceConfig() dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig) + if cmd.DockerPath != "" { + dockerCommand = cmd.DockerPath + } containerDetails, err := workspace2.FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, From 4e845c95385f1b999c8991365e8f94d2b426831a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:45:33 -0500 Subject: [PATCH 41/48] fix(config): honor --docker-path in resolveConfigFromIDLabels Mirror the existing DockerPath-override pattern used elsewhere in ReadCmd so --id-label resolution uses the configured docker binary when --docker-path is set. --- cmd/config/read.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/config/read.go b/cmd/config/read.go index c46369b35..53f959ea2 100644 --- a/cmd/config/read.go +++ b/cmd/config/read.go @@ -262,8 +262,12 @@ func (cmd *ReadCmd) resolveConfigFromIDLabels(ctx context.Context) ( string, error, ) { + dockerCommand := pkgworkspace.DefaultDockerCommand + if cmd.DockerPath != "" { + dockerCommand = cmd.DockerPath + } containerDetails, err := pkgworkspace.FindRunningContainer( - ctx, pkgworkspace.DefaultDockerCommand, "", cmd.IDLabels, + ctx, dockerCommand, "", cmd.IDLabels, ) if err != nil { return nil, "", err From 446634f2bae0562d62a961ad890edf38091301f8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 17:50:14 -0500 Subject: [PATCH 42/48] refactor(workspace): fold --docker-path override into ResolveDockerCommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ResolveDockerCommand now takes an override parameter and consults it first, so the precedence (override → provider config → default) is the function's contract rather than something every caller must re-implement. Two recently-fixed bugs in this PR came from forgetting that override step at a new call site; making it part of the signature removes the foot-gun. --- cmd/internal/runusercommands.go | 5 +---- cmd/workspace/exec.go | 5 +---- cmd/workspace/exec_test.go | 7 ++++++- pkg/workspace/exec_helpers.go | 8 ++++++++ pkg/workspace/exec_oneshot.go | 2 +- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index caa7e8e37..8f15a11d1 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -332,10 +332,7 @@ func (cmd *RunUserCommandsCmd) resolveContainer( } workspaceConfig := client.WorkspaceConfig() - dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig) - if cmd.DockerPath != "" { - dockerCommand = cmd.DockerPath - } + dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig, cmd.DockerPath) containerDetails, err := workspace2.FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index 9cfcb3260..76ae13f6e 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -145,10 +145,7 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { } workspaceConfig := client.WorkspaceConfig() - dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig) - if cmd.DockerPath != "" { - dockerCommand = cmd.DockerPath - } + dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig, cmd.DockerPath) containerDetails, err := workspace2.FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, diff --git a/cmd/workspace/exec_test.go b/cmd/workspace/exec_test.go index db2b4e2c3..c601ccbd5 100644 --- a/cmd/workspace/exec_test.go +++ b/cmd/workspace/exec_test.go @@ -70,10 +70,15 @@ func TestNewExecCmd_RequiresArgs(t *testing.T) { } func TestResolveDockerCommand_NilWorkspace(t *testing.T) { - result := workspace2.ResolveDockerCommand(nil) + result := workspace2.ResolveDockerCommand(nil, "") assert.Equal(t, "docker", result) } +func TestResolveDockerCommand_OverrideBeatsDefault(t *testing.T) { + result := workspace2.ResolveDockerCommand(nil, "/usr/local/bin/podman") + assert.Equal(t, "/usr/local/bin/podman", result, "explicit override must win over the default") +} + func TestExecCmd_DockerPathFlag(t *testing.T) { execCmd := NewExecCmd(&flags.GlobalFlags{}) flag := execCmd.Flags().Lookup("docker-path") diff --git a/pkg/workspace/exec_helpers.go b/pkg/workspace/exec_helpers.go index d7a6e68c7..5e6fd69d7 100644 --- a/pkg/workspace/exec_helpers.go +++ b/pkg/workspace/exec_helpers.go @@ -28,9 +28,17 @@ type ContainerTarget struct { User string } +// ResolveDockerCommand returns the docker binary to invoke. Precedence: +// caller-supplied override → provider config (agent.docker.path) → default. +// Callers wired to a --docker-path style flag pass it as override; the override +// is honored even when workspace is nil so flag handling works at every call site. func ResolveDockerCommand( workspace *provider2.Workspace, + override string, ) string { + if override != "" { + return override + } if workspace == nil || workspace.Context == "" { return DefaultDockerCommand } diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index 559908e3d..e4e888cd8 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -143,7 +143,7 @@ func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedEx } workspaceConfig := client.WorkspaceConfig() - dockerCommand := ResolveDockerCommand(workspaceConfig) + dockerCommand := ResolveDockerCommand(workspaceConfig, "") containerDetails, err := FindRunningContainer( ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, From ef4c2f7840302adba4690b1a9f414449fddbfbbe Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:13:44 -0500 Subject: [PATCH 43/48] refactor(workspace): introduce ContainerRuntime interface to abstract docker ops Add ContainerRuntime interface (FindRunning, Exec, ProbeEnv) in pkg/workspace/runtime.go with ContainerTarget moved there (Helper field removed) and ExecRequest bundling exec parameters to satisfy the revive argument-limit rule. DockerRuntime implements the interface in docker_runtime.go, constructing from NewDockerRuntime(workspace, override). Remove FindRunningContainer and ProbeContainerEnv free functions; their bodies now live on DockerRuntime. Migrate exec_oneshot.go, cmd/workspace/exec.go, cmd/internal/runusercommands.go, and cmd/config/read.go to use NewDockerRuntime + runtime.FindRunning/ProbeEnv. Split ExecOneShot into a public entry point and a testable execOneShotWithRuntime inner function; add fakeRuntime test double and two new exec tests. --- cmd/config/read.go | 9 +- cmd/internal/runusercommands.go | 8 +- cmd/workspace/exec.go | 46 ++++----- pkg/workspace/docker_runtime.go | 148 +++++++++++++++++++++++++++++ pkg/workspace/exec_helpers.go | 96 +------------------ pkg/workspace/exec_oneshot.go | 95 +++++++----------- pkg/workspace/exec_oneshot_test.go | 106 +++++++++++++++++++++ pkg/workspace/runtime.go | 51 ++++++++++ 8 files changed, 373 insertions(+), 186 deletions(-) create mode 100644 pkg/workspace/docker_runtime.go create mode 100644 pkg/workspace/runtime.go diff --git a/cmd/config/read.go b/cmd/config/read.go index 53f959ea2..60d8ab343 100644 --- a/cmd/config/read.go +++ b/cmd/config/read.go @@ -262,13 +262,8 @@ func (cmd *ReadCmd) resolveConfigFromIDLabels(ctx context.Context) ( string, error, ) { - dockerCommand := pkgworkspace.DefaultDockerCommand - if cmd.DockerPath != "" { - dockerCommand = cmd.DockerPath - } - containerDetails, err := pkgworkspace.FindRunningContainer( - ctx, dockerCommand, "", cmd.IDLabels, - ) + runtime := pkgworkspace.NewDockerRuntime(nil, cmd.DockerPath) + containerDetails, err := runtime.FindRunning(ctx, "", cmd.IDLabels) if err != nil { return nil, "", err } diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 8f15a11d1..efbdfe464 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -332,10 +332,10 @@ func (cmd *RunUserCommandsCmd) resolveContainer( } workspaceConfig := client.WorkspaceConfig() - dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig, cmd.DockerPath) + runtime := workspace2.NewDockerRuntime(workspaceConfig, cmd.DockerPath) - containerDetails, err := workspace2.FindRunningContainer( - ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, + containerDetails, err := runtime.FindRunning( + ctx, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, ) if err != nil { _ = devcconfig.WriteErrorJSON(os.Stderr, err.Error()) @@ -366,7 +366,7 @@ func (cmd *RunUserCommandsCmd) resolveContainer( params := &workspace.LifecycleExecParams{ Ctx: ctx, - Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, + Helper: &docker.DockerHelper{DockerCommand: runtime.DockerCommand()}, ContainerID: containerDetails.ID, EnvArgs: envArgs, Workdir: workspace2.ResolveExecWorkdir(result, client.Workspace()), diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index 76ae13f6e..a62fe36f2 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -145,10 +145,10 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { } workspaceConfig := client.WorkspaceConfig() - dockerCommand := workspace2.ResolveDockerCommand(workspaceConfig, cmd.DockerPath) + runtime := workspace2.NewDockerRuntime(workspaceConfig, cmd.DockerPath) - containerDetails, err := workspace2.FindRunningContainer( - ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, + containerDetails, err := runtime.FindRunning( + ctx, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels, ) if err != nil { return err @@ -160,11 +160,10 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { userEnvProbe := resolveUserEnvProbe(result, cmd.DefaultUserEnvProbe) target := workspace2.ContainerTarget{ - Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, ContainerID: containerDetails.ID, User: user, } - probedEnv := workspace2.ProbeContainerEnv(ctx, target, userEnvProbe) + probedEnv := runtime.ProbeEnv(ctx, target, userEnvProbe) envMap := workspace2.BuildExecEnv(result, cmd.RemoteEnv, probedEnv) mode, err := output.ResolveMode(cmd.ResultFormat) @@ -174,9 +173,10 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { emitJSON := mode == output.ModeJSON err = cmd.execInContainer(ctx, execOpts{ - target: target, - workdir: workdir, - envMap: envMap, + dockerCmd: runtime.DockerCommand(), + target: target, + workdir: workdir, + envMap: envMap, }, args) if err != nil { if emitJSON { @@ -196,11 +196,8 @@ func (cmd *ExecCmd) Run(ctx context.Context, args []string) error { } func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error { - dockerCommand := workspace2.DefaultDockerCommand - if cmd.DockerPath != "" { - dockerCommand = cmd.DockerPath - } - helper := &docker.DockerHelper{DockerCommand: dockerCommand} + runtime := workspace2.NewDockerRuntime(nil, cmd.DockerPath) + helper := &docker.DockerHelper{DockerCommand: runtime.DockerCommand()} details, err := helper.InspectContainers(ctx, []string{cmd.ContainerID}) if err != nil { @@ -221,11 +218,10 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error userEnvProbe := cmd.DefaultUserEnvProbe target := workspace2.ContainerTarget{ - Helper: helper, ContainerID: containerDetails.ID, User: "", } - probedEnv := workspace2.ProbeContainerEnv(ctx, target, userEnvProbe) + probedEnv := runtime.ProbeEnv(ctx, target, userEnvProbe) envMap := workspace2.BuildExecEnv(nil, cmd.RemoteEnv, probedEnv) workdir := containerDetails.Config.WorkingDir @@ -237,9 +233,10 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error emitJSON := mode == output.ModeJSON err = cmd.execInContainer(ctx, execOpts{ - target: target, - workdir: workdir, - envMap: envMap, + dockerCmd: runtime.DockerCommand(), + target: target, + workdir: workdir, + envMap: envMap, }, args) if err != nil { if emitJSON { @@ -278,9 +275,10 @@ func resolveUserEnvProbe(result *devcconfig.Result, cliOverride string) string { } type execOpts struct { - target workspace2.ContainerTarget - workdir string - envMap map[string]string + dockerCmd string + target workspace2.ContainerTarget + workdir string + envMap map[string]string } func (cmd *ExecCmd) execInContainer(ctx context.Context, opts execOpts, args []string) error { @@ -301,8 +299,10 @@ func (cmd *ExecCmd) execInContainer(ctx context.Context, opts execOpts, args []s execArgs = append(execArgs, args...) redacted := strings.Join(redactExecArgs(execArgs), " ") - log.Debugf("Executing in container: %s %s", opts.target.Helper.DockerCommand, redacted) - return opts.target.Helper.Run(ctx, execArgs, os.Stdin, os.Stdout, os.Stderr) + log.Debugf("Executing in container: %s %s", opts.dockerCmd, redacted) + + helper := &docker.DockerHelper{DockerCommand: opts.dockerCmd} + return helper.Run(ctx, execArgs, os.Stdin, os.Stdout, os.Stderr) } func redactExecArgs(args []string) []string { diff --git a/pkg/workspace/docker_runtime.go b/pkg/workspace/docker_runtime.go new file mode 100644 index 000000000..272950560 --- /dev/null +++ b/pkg/workspace/docker_runtime.go @@ -0,0 +1,148 @@ +package workspace + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os/exec" + "strings" + + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/log" + provider2 "github.com/devsy-org/devsy/pkg/provider" +) + +// DockerRuntime is the production ContainerRuntime that shells out to a +// docker-compatible binary. +type DockerRuntime struct { + helper *docker.DockerHelper +} + +// NewDockerRuntime constructs a runtime that shells out to the docker-like +// binary chosen by ResolveDockerCommand. The override parameter is honored +// the same way ResolveDockerCommand honors it. +func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRuntime { + return &DockerRuntime{ + helper: &docker.DockerHelper{ + DockerCommand: ResolveDockerCommand(workspace, override), + }, + } +} + +// DockerCommand exposes the resolved docker binary path (for diagnostics or +// callers that still need the raw string). +func (r *DockerRuntime) DockerCommand() string { return r.helper.DockerCommand } + +// FindRunning implements ContainerRuntime. +func (r *DockerRuntime) FindRunning( + ctx context.Context, + workspaceID string, + idLabels []string, +) (*devcconfig.ContainerDetails, error) { + labels := devcconfig.GetIDLabels(workspaceID, idLabels) + container, err := r.helper.FindDevContainer(ctx, labels) + if err != nil { + return nil, fmt.Errorf("find container: %w", err) + } + if container == nil { + return nil, fmt.Errorf( + "no running container found for workspace %q", + workspaceID, + ) + } + + if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + return nil, fmt.Errorf( + "container %s is not running (status: %s)", + container.ID, + container.State.Status, + ) + } + + return container, nil +} + +// Exec implements ContainerRuntime. +func (r *DockerRuntime) Exec(ctx context.Context, req ExecRequest) (int, error) { + execArgs := []string{"exec", "-i"} + for k, v := range req.Env { + execArgs = append(execArgs, "-e", k+"="+v) + } + if req.Workdir != "" { + execArgs = append(execArgs, "--workdir", req.Workdir) + } + if req.Target.User != "" { + execArgs = append(execArgs, "--user", req.Target.User) + } + execArgs = append(execArgs, req.Target.ContainerID) + execArgs = append(execArgs, req.Argv...) + + stdout := req.Stdout + if stdout == nil { + stdout = io.Discard + } + stderr := req.Stderr + if stderr == nil { + stderr = io.Discard + } + + err := r.helper.Run(ctx, execArgs, nil, stdout, stderr) + if err == nil { + return 0, nil + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode(), nil + } + return -1, fmt.Errorf("exec in container %s: %w", req.Target.ContainerID, err) +} + +// ProbeEnv implements ContainerRuntime. +func (r *DockerRuntime) ProbeEnv( + ctx context.Context, + target ContainerTarget, + probe string, +) map[string]string { + userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) + if err != nil { + log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) + userEnvProbe = devcconfig.DefaultUserEnvProbe + } + if userEnvProbe == devcconfig.NoneProbe { + return map[string]string{} + } + + shellFlag := probeShellFlag(userEnvProbe) + + out, sep, probeErr := r.runProbeCommand(ctx, target, shellFlag) + if probeErr != nil { + log.Warnf("Failed to probe user env: %v", probeErr) + return map[string]string{} + } + return parseEnvOutput(out, sep) +} + +func (r *DockerRuntime) runProbeCommand( + ctx context.Context, + target ContainerTarget, + shellFlag string, +) ([]byte, byte, error) { + args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") + var stdout bytes.Buffer + err := r.helper.Run(ctx, args, nil, &stdout, io.Discard) + if err == nil { + return stdout.Bytes(), 0, nil + } + + log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) + args = buildProbeArgs(target, shellFlag, "printenv") + stdout.Reset() + err = r.helper.Run(ctx, args, nil, &stdout, io.Discard) + if err != nil { + return nil, 0, fmt.Errorf("probe user env: %w", err) + } + return stdout.Bytes(), '\n', nil +} diff --git a/pkg/workspace/exec_helpers.go b/pkg/workspace/exec_helpers.go index 5e6fd69d7..d807090fd 100644 --- a/pkg/workspace/exec_helpers.go +++ b/pkg/workspace/exec_helpers.go @@ -2,16 +2,12 @@ package workspace import ( "bytes" - "context" - "fmt" - "io" "maps" "os" "path" "strings" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/log" provider2 "github.com/devsy-org/devsy/pkg/provider" ) @@ -21,13 +17,6 @@ const ( ContainerStatusRunning = "running" ) -// ContainerTarget bundles the docker helper, container ID, and user for exec operations. -type ContainerTarget struct { - Helper *docker.DockerHelper - ContainerID string - User string -} - // ResolveDockerCommand returns the docker binary to invoke. Precedence: // caller-supplied override → provider config (agent.docker.path) → default. // Callers wired to a --docker-path style flag pass it as override; the override @@ -61,39 +50,6 @@ func ResolveDockerCommand( return DefaultDockerCommand } -func FindRunningContainer( - ctx context.Context, - dockerCommand string, - workspaceID string, - idLabels []string, -) (*devcconfig.ContainerDetails, error) { - dockerHelper := &docker.DockerHelper{ - DockerCommand: dockerCommand, - } - - labels := devcconfig.GetIDLabels(workspaceID, idLabels) - container, err := dockerHelper.FindDevContainer(ctx, labels) - if err != nil { - return nil, fmt.Errorf("find container: %w", err) - } - if container == nil { - return nil, fmt.Errorf( - "no running container found for workspace %q", - workspaceID, - ) - } - - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { - return nil, fmt.Errorf( - "container %s is not running (status: %s)", - container.ID, - container.State.Status, - ) - } - - return container, nil -} - func LoadExecResult( workspaceConfig *provider2.Workspace, containerDetails *devcconfig.ContainerDetails, @@ -163,31 +119,8 @@ func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { } } -// ProbeContainerEnv probes the container's environment via the given probe strategy. -func ProbeContainerEnv( - ctx context.Context, - target ContainerTarget, - probe string, -) map[string]string { - userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) - if err != nil { - log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) - userEnvProbe = devcconfig.DefaultUserEnvProbe - } - if userEnvProbe == devcconfig.NoneProbe { - return map[string]string{} - } - - shellFlag := probeShellFlag(userEnvProbe) - - out, sep, err := runProbeCommand(ctx, target, shellFlag) - if err != nil { - log.Warnf("Failed to probe user env: %v", err) - return map[string]string{} - } - return parseEnvOutput(out, sep) -} - +// probeShellFlag returns the shell flag for the given probe mode. +// Kept as a package-level helper so DockerRuntime can use it. func probeShellFlag(probe devcconfig.UserEnvProbe) string { switch probe { case devcconfig.LoginInteractiveShellProbe: @@ -201,28 +134,8 @@ func probeShellFlag(probe devcconfig.UserEnvProbe) string { } } -func runProbeCommand( - ctx context.Context, - target ContainerTarget, - shellFlag string, -) ([]byte, byte, error) { - args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") - var stdout bytes.Buffer - err := target.Helper.Run(ctx, args, nil, &stdout, io.Discard) - if err == nil { - return stdout.Bytes(), 0, nil - } - - log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) - args = buildProbeArgs(target, shellFlag, "printenv") - stdout.Reset() - err = target.Helper.Run(ctx, args, nil, &stdout, io.Discard) - if err != nil { - return nil, 0, fmt.Errorf("probe user env: %w", err) - } - return stdout.Bytes(), '\n', nil -} - +// buildProbeArgs constructs the docker exec arguments for env probing. +// Kept as a package-level helper so DockerRuntime can use it. func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []string { args := []string{"exec"} if target.User != "" { @@ -232,6 +145,7 @@ func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []stri return args } +// parseEnvOutput parses the output of an env probe command. func parseEnvOutput(out []byte, sep byte) map[string]string { entries := bytes.Split(out, []byte{sep}) env := make(map[string]string, len(entries)) diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go index e4e888cd8..bf57d238d 100644 --- a/pkg/workspace/exec_oneshot.go +++ b/pkg/workspace/exec_oneshot.go @@ -5,13 +5,11 @@ import ( "errors" "fmt" "io" - "os/exec" "time" "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/platform" ) @@ -86,13 +84,13 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu defer cancel() start := time.Now() - exitCode, runErr := runCapture(execCtx, captureArgs{ - target: resolved.target, - workdir: resolved.workdir, - env: resolved.envMap, - command: opts.Command, - stdout: opts.Stdout, - stderr: opts.Stderr, + exitCode, runErr := execOneShotWithRuntime(execCtx, resolved.runtime, ExecRequest{ + Target: resolved.target, + Workdir: resolved.workdir, + Env: resolved.envMap, + Argv: opts.Command, + Stdout: opts.Stdout, + Stderr: opts.Stderr, }) duration := time.Since(start) @@ -119,14 +117,28 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu return res, nil } -// resolvedExecTarget bundles the resolved container target, workdir, and env for an exec. +// execOneShotWithRuntime is the low-level exec path; it accepts an already- +// resolved runtime and request so tests can inject a fake runtime without +// touching workspace resolution. +func execOneShotWithRuntime( + ctx context.Context, + runtime ContainerRuntime, + req ExecRequest, +) (int, error) { + return runtime.Exec(ctx, req) +} + +// resolvedExecTarget bundles the resolved runtime, target, workdir, and env +// for an exec — kept as a struct to stay within revive's function-result-limit. type resolvedExecTarget struct { + runtime ContainerRuntime target ContainerTarget workdir string envMap map[string]string } -// resolveExecTarget resolves the container target, workdir, and env map from options. +// resolveExecTarget resolves the container runtime, target, workdir, and env +// map from options. func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedExecTarget, error) { devsyConfig, err := config.LoadConfig(opts.Context, opts.Provider) if err != nil { @@ -143,10 +155,10 @@ func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedEx } workspaceConfig := client.WorkspaceConfig() - dockerCommand := ResolveDockerCommand(workspaceConfig, "") + runtime := NewDockerRuntime(workspaceConfig, "") - containerDetails, err := FindRunningContainer( - ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, + containerDetails, err := runtime.FindRunning( + ctx, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, ) if err != nil { return resolvedExecTarget{}, err @@ -164,63 +176,24 @@ func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedEx } target := ContainerTarget{ - Helper: &docker.DockerHelper{DockerCommand: dockerCommand}, ContainerID: containerDetails.ID, User: user, } + userEnvProbe := "" if execResult != nil && execResult.MergedConfig != nil { userEnvProbe = execResult.MergedConfig.UserEnvProbe } - probedEnv := ProbeContainerEnv(ctx, target, userEnvProbe) + probedEnv := runtime.ProbeEnv(ctx, target, userEnvProbe) envSlice := envMapToSlice(opts.Env) envMap := BuildExecEnv(execResult, envSlice, probedEnv) - return resolvedExecTarget{target: target, workdir: workdir, envMap: envMap}, nil -} - -// captureArgs bundles the arguments for runCapture. -type captureArgs struct { - target ContainerTarget - workdir string - env map[string]string - command []string - stdout io.Writer - stderr io.Writer -} - -func runCapture(ctx context.Context, args captureArgs) (int, error) { - execArgs := []string{"exec", "-i"} - for k, v := range args.env { - execArgs = append(execArgs, "-e", k+"="+v) - } - if args.workdir != "" { - execArgs = append(execArgs, "--workdir", args.workdir) - } - if args.target.User != "" { - execArgs = append(execArgs, "--user", args.target.User) - } - execArgs = append(execArgs, args.target.ContainerID) - execArgs = append(execArgs, args.command...) - - stdout := args.stdout - if stdout == nil { - stdout = io.Discard - } - stderr := args.stderr - if stderr == nil { - stderr = io.Discard - } - - err := args.target.Helper.Run(ctx, execArgs, nil, stdout, stderr) - if err == nil { - return 0, nil - } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - return exitErr.ExitCode(), nil - } - return -1, fmt.Errorf("exec in container %s: %w", args.target.ContainerID, err) + return resolvedExecTarget{ + runtime: runtime, + target: target, + workdir: workdir, + envMap: envMap, + }, nil } func envMapToSlice(m map[string]string) []string { diff --git a/pkg/workspace/exec_oneshot_test.go b/pkg/workspace/exec_oneshot_test.go index 58c6bc993..68b2d4138 100644 --- a/pkg/workspace/exec_oneshot_test.go +++ b/pkg/workspace/exec_oneshot_test.go @@ -1,7 +1,12 @@ package workspace import ( + "bytes" + "context" + "io" "testing" + + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" ) func TestExecOneShotOptions_ResolveTimeout_Clamp(t *testing.T) { @@ -42,3 +47,104 @@ func TestExecOneShotOptions_ResolveTimeout_CallerExplicit(t *testing.T) { t.Fatalf("expected 120s, got %s", clamped) } } + +// fakeRuntime is a test double for ContainerRuntime. +type fakeRuntime struct { + findResult *devcconfig.ContainerDetails + findErr error + execExit int + execErr error + execStdout string + execStderr string + probeEnv map[string]string +} + +func (f *fakeRuntime) FindRunning( + _ context.Context, + _ string, + _ []string, +) (*devcconfig.ContainerDetails, error) { + return f.findResult, f.findErr +} + +func (f *fakeRuntime) Exec(_ context.Context, req ExecRequest) (int, error) { + stdout := req.Stdout + if stdout == nil { + stdout = io.Discard + } + stderr := req.Stderr + if stderr == nil { + stderr = io.Discard + } + if f.execStdout != "" { + _, _ = stdout.Write([]byte(f.execStdout)) + } + if f.execStderr != "" { + _, _ = stderr.Write([]byte(f.execStderr)) + } + return f.execExit, f.execErr +} + +func (f *fakeRuntime) ProbeEnv( + _ context.Context, + _ ContainerTarget, + _ string, +) map[string]string { + return f.probeEnv +} + +func TestExecOneShot_ExitCodeAndOutput(t *testing.T) { + var stdout, stderr bytes.Buffer + runtime := &fakeRuntime{ + execExit: 42, + execStdout: "hi", + } + + exitCode, err := execOneShotWithRuntime( + context.Background(), + runtime, + ExecRequest{ + Target: ContainerTarget{ContainerID: "ctr1"}, + Workdir: "/workdir", + Argv: []string{"echo", "hi"}, + Stdout: &stdout, + Stderr: &stderr, + }, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exitCode != 42 { + t.Fatalf("expected exit code 42, got %d", exitCode) + } + if stdout.String() != "hi" { + t.Fatalf("expected stdout %q, got %q", "hi", stdout.String()) + } +} + +func TestExecOneShot_PartialOutputOnError(t *testing.T) { + var stdout, stderr bytes.Buffer + runtime := &fakeRuntime{ + execExit: -1, + execErr: context.Canceled, + execStdout: "partial", + } + + _, err := execOneShotWithRuntime( + context.Background(), + runtime, + ExecRequest{ + Target: ContainerTarget{ContainerID: "ctr2"}, + Workdir: "/workdir", + Argv: []string{"long-running-cmd"}, + Stdout: &stdout, + Stderr: &stderr, + }, + ) + if err == nil { + t.Fatal("expected error, got nil") + } + if stdout.String() != "partial" { + t.Fatalf("expected partial stdout %q to be preserved, got %q", "partial", stdout.String()) + } +} diff --git a/pkg/workspace/runtime.go b/pkg/workspace/runtime.go new file mode 100644 index 000000000..590953047 --- /dev/null +++ b/pkg/workspace/runtime.go @@ -0,0 +1,51 @@ +package workspace + +import ( + "context" + "io" + + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" +) + +// ContainerRuntime abstracts container-runtime operations used for workspace +// exec and env-probe. It is runtime-agnostic: callers do not need to know +// whether Docker, Podman, or a test double is underneath. +type ContainerRuntime interface { + // FindRunning looks up a single running container for the given workspace. + // workspaceID may be empty when only idLabels are provided. + FindRunning( + ctx context.Context, + workspaceID string, + idLabels []string, + ) (*devcconfig.ContainerDetails, error) + + // Exec runs the command described by req inside a container. It writes + // stdout and stderr to req.Stdout/req.Stderr and returns the process exit + // code. A non-nil error means the exec machinery itself failed (e.g. the + // docker binary could not be found), not that the command exited non-zero. + Exec(ctx context.Context, req ExecRequest) (exitCode int, err error) + + // ProbeEnv queries the container's environment using the given probe mode + // string (values defined by devcconfig.UserEnvProbe). Returns an empty map + // on any error. + ProbeEnv(ctx context.Context, target ContainerTarget, probeMode string) map[string]string +} + +// ExecRequest bundles all parameters for a single non-interactive container +// exec. Using a struct keeps the ContainerRuntime.Exec signature within the +// revive argument-limit rule while remaining extensible. +type ExecRequest struct { + Target ContainerTarget + Workdir string + Env map[string]string + Argv []string + Stdout io.Writer + Stderr io.Writer +} + +// ContainerTarget identifies a single container exec target. Runtime-agnostic +// data; runtimes consume it. +type ContainerTarget struct { + ContainerID string + User string +} From 9c8ed3ad72b98761518eab7898c790f326813f44 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:17:53 -0500 Subject: [PATCH 44/48] refactor(workspace): collapse exec helpers, runtime, and ExecOneShot into exec.go Four files (exec_helpers.go, runtime.go, docker_runtime.go, exec_oneshot.go) covered one logical surface: container exec via a runtime abstraction. Merge them into a single exec.go organized by responsibility: 1. workspace metadata helpers (ResolveDockerCommand, LoadExecResult, ...) 2. ContainerRuntime interface + ExecRequest/ContainerTarget types 3. DockerRuntime implementation 4. ExecOneShot + ResolveTimeout high-level entry point Rename exec_oneshot_test.go to exec_test.go to match. --- pkg/workspace/docker_runtime.go | 148 ----- pkg/workspace/exec.go | 553 ++++++++++++++++++ pkg/workspace/exec_helpers.go | 164 ------ pkg/workspace/exec_oneshot.go | 205 ------- .../{exec_oneshot_test.go => exec_test.go} | 0 pkg/workspace/runtime.go | 51 -- 6 files changed, 553 insertions(+), 568 deletions(-) delete mode 100644 pkg/workspace/docker_runtime.go create mode 100644 pkg/workspace/exec.go delete mode 100644 pkg/workspace/exec_helpers.go delete mode 100644 pkg/workspace/exec_oneshot.go rename pkg/workspace/{exec_oneshot_test.go => exec_test.go} (100%) delete mode 100644 pkg/workspace/runtime.go diff --git a/pkg/workspace/docker_runtime.go b/pkg/workspace/docker_runtime.go deleted file mode 100644 index 272950560..000000000 --- a/pkg/workspace/docker_runtime.go +++ /dev/null @@ -1,148 +0,0 @@ -package workspace - -import ( - "bytes" - "context" - "errors" - "fmt" - "io" - "os/exec" - "strings" - - devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/docker" - "github.com/devsy-org/devsy/pkg/log" - provider2 "github.com/devsy-org/devsy/pkg/provider" -) - -// DockerRuntime is the production ContainerRuntime that shells out to a -// docker-compatible binary. -type DockerRuntime struct { - helper *docker.DockerHelper -} - -// NewDockerRuntime constructs a runtime that shells out to the docker-like -// binary chosen by ResolveDockerCommand. The override parameter is honored -// the same way ResolveDockerCommand honors it. -func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRuntime { - return &DockerRuntime{ - helper: &docker.DockerHelper{ - DockerCommand: ResolveDockerCommand(workspace, override), - }, - } -} - -// DockerCommand exposes the resolved docker binary path (for diagnostics or -// callers that still need the raw string). -func (r *DockerRuntime) DockerCommand() string { return r.helper.DockerCommand } - -// FindRunning implements ContainerRuntime. -func (r *DockerRuntime) FindRunning( - ctx context.Context, - workspaceID string, - idLabels []string, -) (*devcconfig.ContainerDetails, error) { - labels := devcconfig.GetIDLabels(workspaceID, idLabels) - container, err := r.helper.FindDevContainer(ctx, labels) - if err != nil { - return nil, fmt.Errorf("find container: %w", err) - } - if container == nil { - return nil, fmt.Errorf( - "no running container found for workspace %q", - workspaceID, - ) - } - - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { - return nil, fmt.Errorf( - "container %s is not running (status: %s)", - container.ID, - container.State.Status, - ) - } - - return container, nil -} - -// Exec implements ContainerRuntime. -func (r *DockerRuntime) Exec(ctx context.Context, req ExecRequest) (int, error) { - execArgs := []string{"exec", "-i"} - for k, v := range req.Env { - execArgs = append(execArgs, "-e", k+"="+v) - } - if req.Workdir != "" { - execArgs = append(execArgs, "--workdir", req.Workdir) - } - if req.Target.User != "" { - execArgs = append(execArgs, "--user", req.Target.User) - } - execArgs = append(execArgs, req.Target.ContainerID) - execArgs = append(execArgs, req.Argv...) - - stdout := req.Stdout - if stdout == nil { - stdout = io.Discard - } - stderr := req.Stderr - if stderr == nil { - stderr = io.Discard - } - - err := r.helper.Run(ctx, execArgs, nil, stdout, stderr) - if err == nil { - return 0, nil - } - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - return exitErr.ExitCode(), nil - } - return -1, fmt.Errorf("exec in container %s: %w", req.Target.ContainerID, err) -} - -// ProbeEnv implements ContainerRuntime. -func (r *DockerRuntime) ProbeEnv( - ctx context.Context, - target ContainerTarget, - probe string, -) map[string]string { - userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) - if err != nil { - log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) - userEnvProbe = devcconfig.DefaultUserEnvProbe - } - if userEnvProbe == devcconfig.NoneProbe { - return map[string]string{} - } - - shellFlag := probeShellFlag(userEnvProbe) - - out, sep, probeErr := r.runProbeCommand(ctx, target, shellFlag) - if probeErr != nil { - log.Warnf("Failed to probe user env: %v", probeErr) - return map[string]string{} - } - return parseEnvOutput(out, sep) -} - -func (r *DockerRuntime) runProbeCommand( - ctx context.Context, - target ContainerTarget, - shellFlag string, -) ([]byte, byte, error) { - args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") - var stdout bytes.Buffer - err := r.helper.Run(ctx, args, nil, &stdout, io.Discard) - if err == nil { - return stdout.Bytes(), 0, nil - } - - log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) - args = buildProbeArgs(target, shellFlag, "printenv") - stdout.Reset() - err = r.helper.Run(ctx, args, nil, &stdout, io.Discard) - if err != nil { - return nil, 0, fmt.Errorf("probe user env: %w", err) - } - return stdout.Bytes(), '\n', nil -} diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go new file mode 100644 index 000000000..bec1e806d --- /dev/null +++ b/pkg/workspace/exec.go @@ -0,0 +1,553 @@ +package workspace + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "maps" + "os" + "os/exec" + "path" + "strings" + "time" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer" + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/platform" + provider2 "github.com/devsy-org/devsy/pkg/provider" +) + +const ( + DefaultDockerCommand = "docker" + ContainerStatusRunning = "running" +) + +// defaultExecTimeoutSeconds bounds an exec when no caller or configured +// default applies. Tuned to surface hung calls without truncating typical +// build/test commands. +const defaultExecTimeoutSeconds = 300 + +// ---------------------------------------------------------------------------- +// Workspace metadata helpers (runtime-agnostic). +// ---------------------------------------------------------------------------- + +// ResolveDockerCommand returns the docker binary to invoke. Precedence: +// caller-supplied override → provider config (agent.docker.path) → default. +// Callers wired to a --docker-path style flag pass it as override; the override +// is honored even when workspace is nil so flag handling works at every call site. +func ResolveDockerCommand( + workspace *provider2.Workspace, + override string, +) string { + if override != "" { + return override + } + if workspace == nil || workspace.Context == "" { + return DefaultDockerCommand + } + + providerConfig, err := provider2.LoadProviderConfig( + workspace.Context, + workspace.Provider.Name, + ) + if err != nil { + log.Debugf("Failed to load provider config, defaulting to 'docker': %v", err) + return DefaultDockerCommand + } + + if providerConfig.Agent.Docker.Path != "" { + if expanded := os.ExpandEnv(providerConfig.Agent.Docker.Path); expanded != "" { + return expanded + } + } + + return DefaultDockerCommand +} + +func LoadExecResult( + workspaceConfig *provider2.Workspace, + containerDetails *devcconfig.ContainerDetails, +) *devcconfig.Result { + if workspaceConfig == nil || workspaceConfig.Context == "" || workspaceConfig.ID == "" { + return nil + } + + result, err := provider2.LoadWorkspaceResult(workspaceConfig.Context, workspaceConfig.ID) + if err != nil { + log.Warnf("Error loading workspace result: %v", err) + return nil + } + if result != nil { + result.ContainerDetails = containerDetails + } + return result +} + +func ResolveExecWorkdir(result *devcconfig.Result, workspaceName string) string { + if result != nil && result.MergedConfig != nil && result.MergedConfig.WorkspaceFolder != "" { + return result.MergedConfig.WorkspaceFolder + } + return path.Join("/workspaces", workspaceName) +} + +// BuildExecEnv merges probed env, result remote env, and caller-supplied env slices. +func BuildExecEnv( + result *devcconfig.Result, + cliEnv []string, + probedEnv map[string]string, +) map[string]string { + env := make(map[string]string, len(probedEnv)) + maps.Copy(env, probedEnv) + + if result != nil { + applyRemoteEnv(env, mergedRemoteEnv(result)) + } + + for _, e := range cliEnv { + if k, v, ok := strings.Cut(e, "="); ok { + env[k] = v + } + } + + return env +} + +func mergedRemoteEnv(result *devcconfig.Result) map[string]*string { + merged := map[string]*string{} + if result.MergedConfig != nil { + maps.Copy(merged, result.MergedConfig.RemoteEnv) + } + if result.DevContainerConfigWithPath != nil && result.DevContainerConfigWithPath.Config != nil { + maps.Copy(merged, result.DevContainerConfigWithPath.Config.RemoteEnv) + } + return merged +} + +func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { + for k, v := range remoteEnv { + if v == nil { + delete(env, k) + } else { + env[k] = *v + } + } +} + +// probeShellFlag returns the shell flag for the given probe mode. +func probeShellFlag(probe devcconfig.UserEnvProbe) string { + switch probe { + case devcconfig.LoginInteractiveShellProbe: + return "-lic" + case devcconfig.LoginShellProbe: + return "-lc" + case devcconfig.InteractiveShellProbe: + return "-ic" + default: + return "-c" + } +} + +// buildProbeArgs constructs the docker exec arguments for env probing. +func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []string { + args := []string{"exec"} + if target.User != "" { + args = append(args, "--user", target.User) + } + args = append(args, target.ContainerID, "sh", shellFlag, cmd) + return args +} + +// parseEnvOutput parses the output of an env probe command. +func parseEnvOutput(out []byte, sep byte) map[string]string { + entries := bytes.Split(out, []byte{sep}) + env := make(map[string]string, len(entries)) + for _, e := range entries { + if len(e) == 0 { + continue + } + name, value, ok := bytes.Cut(e, []byte{'='}) + if !ok || len(name) == 0 { + continue + } + env[string(name)] = string(value) + } + delete(env, "PWD") + return env +} + +// ---------------------------------------------------------------------------- +// ContainerRuntime — abstraction over docker / podman / test doubles. +// ---------------------------------------------------------------------------- + +// ContainerRuntime abstracts container-runtime operations used for workspace +// exec and env-probe. Callers do not need to know whether Docker, Podman, or a +// test double is underneath. +type ContainerRuntime interface { + // FindRunning looks up a single running container for the given workspace. + // workspaceID may be empty when only idLabels are provided. + FindRunning( + ctx context.Context, + workspaceID string, + idLabels []string, + ) (*devcconfig.ContainerDetails, error) + + // Exec runs the command described by req inside a container. It writes + // stdout and stderr to req.Stdout/req.Stderr and returns the process exit + // code. A non-nil error means the exec machinery itself failed (e.g. the + // docker binary could not be found), not that the command exited non-zero. + Exec(ctx context.Context, req ExecRequest) (exitCode int, err error) + + // ProbeEnv queries the container's environment using the given probe mode + // string (values defined by devcconfig.UserEnvProbe). Returns an empty map + // on any error. + ProbeEnv(ctx context.Context, target ContainerTarget, probeMode string) map[string]string +} + +// ExecRequest bundles all parameters for a single non-interactive container +// exec. Using a struct keeps the ContainerRuntime.Exec signature within the +// revive argument-limit rule while remaining extensible. +type ExecRequest struct { + Target ContainerTarget + Workdir string + Env map[string]string + Argv []string + Stdout io.Writer + Stderr io.Writer +} + +// ContainerTarget identifies a single container exec target. Runtime-agnostic +// data; runtimes consume it. +type ContainerTarget struct { + ContainerID string + User string +} + +// ---------------------------------------------------------------------------- +// DockerRuntime — production implementation of ContainerRuntime. +// ---------------------------------------------------------------------------- + +// DockerRuntime shells out to a docker-compatible binary. +type DockerRuntime struct { + helper *docker.DockerHelper +} + +// NewDockerRuntime constructs a runtime that shells out to the docker-like +// binary chosen by ResolveDockerCommand. The override parameter is honored +// the same way ResolveDockerCommand honors it. +func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRuntime { + return &DockerRuntime{ + helper: &docker.DockerHelper{ + DockerCommand: ResolveDockerCommand(workspace, override), + }, + } +} + +// DockerCommand exposes the resolved docker binary path (for diagnostics or +// callers that still need the raw string). +func (r *DockerRuntime) DockerCommand() string { return r.helper.DockerCommand } + +// FindRunning implements ContainerRuntime. +func (r *DockerRuntime) FindRunning( + ctx context.Context, + workspaceID string, + idLabels []string, +) (*devcconfig.ContainerDetails, error) { + labels := devcconfig.GetIDLabels(workspaceID, idLabels) + container, err := r.helper.FindDevContainer(ctx, labels) + if err != nil { + return nil, fmt.Errorf("find container: %w", err) + } + if container == nil { + return nil, fmt.Errorf( + "no running container found for workspace %q", + workspaceID, + ) + } + + if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + return nil, fmt.Errorf( + "container %s is not running (status: %s)", + container.ID, + container.State.Status, + ) + } + + return container, nil +} + +// Exec implements ContainerRuntime. +func (r *DockerRuntime) Exec(ctx context.Context, req ExecRequest) (int, error) { + execArgs := []string{"exec", "-i"} + for k, v := range req.Env { + execArgs = append(execArgs, "-e", k+"="+v) + } + if req.Workdir != "" { + execArgs = append(execArgs, "--workdir", req.Workdir) + } + if req.Target.User != "" { + execArgs = append(execArgs, "--user", req.Target.User) + } + execArgs = append(execArgs, req.Target.ContainerID) + execArgs = append(execArgs, req.Argv...) + + stdout := req.Stdout + if stdout == nil { + stdout = io.Discard + } + stderr := req.Stderr + if stderr == nil { + stderr = io.Discard + } + + err := r.helper.Run(ctx, execArgs, nil, stdout, stderr) + if err == nil { + return 0, nil + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode(), nil + } + return -1, fmt.Errorf("exec in container %s: %w", req.Target.ContainerID, err) +} + +// ProbeEnv implements ContainerRuntime. +func (r *DockerRuntime) ProbeEnv( + ctx context.Context, + target ContainerTarget, + probe string, +) map[string]string { + userEnvProbe, err := devcconfig.NewUserEnvProbe(probe) + if err != nil { + log.Warnf("Invalid userEnvProbe %q, using default: %v", probe, err) + userEnvProbe = devcconfig.DefaultUserEnvProbe + } + if userEnvProbe == devcconfig.NoneProbe { + return map[string]string{} + } + + shellFlag := probeShellFlag(userEnvProbe) + + out, sep, probeErr := r.runProbeCommand(ctx, target, shellFlag) + if probeErr != nil { + log.Warnf("Failed to probe user env: %v", probeErr) + return map[string]string{} + } + return parseEnvOutput(out, sep) +} + +func (r *DockerRuntime) runProbeCommand( + ctx context.Context, + target ContainerTarget, + shellFlag string, +) ([]byte, byte, error) { + args := buildProbeArgs(target, shellFlag, "cat /proc/self/environ") + var stdout bytes.Buffer + err := r.helper.Run(ctx, args, nil, &stdout, io.Discard) + if err == nil { + return stdout.Bytes(), 0, nil + } + + log.Debugf("Env probe with /proc/self/environ failed: %v, trying printenv", err) + args = buildProbeArgs(target, shellFlag, "printenv") + stdout.Reset() + err = r.helper.Run(ctx, args, nil, &stdout, io.Discard) + if err != nil { + return nil, 0, fmt.Errorf("probe user env: %w", err) + } + return stdout.Bytes(), '\n', nil +} + +// ---------------------------------------------------------------------------- +// ExecOneShot — the high-level non-interactive exec entry point. +// ---------------------------------------------------------------------------- + +// ExecOneShotOptions configures a single non-interactive command execution +// inside a workspace's running container. +type ExecOneShotOptions struct { + WorkspaceName string + Command []string + Workdir string + Env map[string]string + IDLabels []string // additional id-labels for container lookup; nil uses defaults + TimeoutSeconds int + TimeoutSecondsDefault int + TimeoutSecondsMax int + Owner platform.OwnerFilter + Context string + Provider string + Stdout io.Writer + Stderr io.Writer +} + +// ExecOneShotResult is the structured outcome of an exec. +type ExecOneShotResult struct { + ExitCode int + DurationMS int64 + TimedOut bool + TimeoutSeconds int + Clamped bool +} + +// ResolveTimeout picks the first positive of TimeoutSeconds, TimeoutSecondsDefault, +// fallbackDefault, then clamps by TimeoutSecondsMax. The bool reports a clamp. +func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, bool) { + want := o.TimeoutSeconds + if want <= 0 { + want = o.TimeoutSecondsDefault + } + if want <= 0 { + want = fallbackDefault + } + if o.TimeoutSecondsMax > 0 && want > o.TimeoutSecondsMax { + return time.Duration(o.TimeoutSecondsMax) * time.Second, true + } + return time.Duration(want) * time.Second, false +} + +// ExecOneShot runs Command inside the workspace's container, captures +// stdout/stderr via the provided writers, and returns a structured result. +// It never reads stdin and never allocates a TTY. +func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResult, error) { + if opts.WorkspaceName == "" { + return nil, fmt.Errorf("workspace name is required") + } + if len(opts.Command) == 0 { + return nil, fmt.Errorf("command is required") + } + + timeout, clamped := opts.ResolveTimeout(defaultExecTimeoutSeconds) + + // Resolve under the parent context so docker lookup doesn't eat the exec budget. + resolved, err := resolveExecTarget(ctx, opts) + if err != nil { + return nil, err + } + + execCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + start := time.Now() + exitCode, runErr := execOneShotWithRuntime(execCtx, resolved.runtime, ExecRequest{ + Target: resolved.target, + Workdir: resolved.workdir, + Env: resolved.envMap, + Argv: opts.Command, + Stdout: opts.Stdout, + Stderr: opts.Stderr, + }) + duration := time.Since(start) + + res := &ExecOneShotResult{ + ExitCode: exitCode, + DurationMS: duration.Milliseconds(), + Clamped: clamped, + TimeoutSeconds: int(timeout.Seconds()), + } + // Parent error first — execCtx inherits its cancellation, so we'd otherwise + // misreport caller cancellation as our own timeout. + if parentErr := ctx.Err(); parentErr != nil { + res.ExitCode = -1 + return res, parentErr + } + if errors.Is(execCtx.Err(), context.DeadlineExceeded) { + res.TimedOut = true + res.ExitCode = -1 + return res, nil + } + if runErr != nil { + return res, runErr + } + return res, nil +} + +// execOneShotWithRuntime is the low-level exec path; it accepts an already- +// resolved runtime and request so tests can inject a fake runtime without +// touching workspace resolution. +func execOneShotWithRuntime( + ctx context.Context, + runtime ContainerRuntime, + req ExecRequest, +) (int, error) { + return runtime.Exec(ctx, req) +} + +// resolvedExecTarget bundles the resolved runtime, target, workdir, and env +// for an exec — kept as a struct to stay within revive's function-result-limit. +type resolvedExecTarget struct { + runtime ContainerRuntime + target ContainerTarget + workdir string + envMap map[string]string +} + +// resolveExecTarget resolves the container runtime, target, workdir, and env +// map from options. +func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedExecTarget, error) { + devsyConfig, err := config.LoadConfig(opts.Context, opts.Provider) + if err != nil { + return resolvedExecTarget{}, fmt.Errorf("load config: %w", err) + } + + client, err := Get(ctx, GetOptions{ + DevsyConfig: devsyConfig, + Args: []string{opts.WorkspaceName}, + Owner: opts.Owner, + }) + if err != nil { + return resolvedExecTarget{}, fmt.Errorf("resolve workspace: %w", err) + } + + workspaceConfig := client.WorkspaceConfig() + runtime := NewDockerRuntime(workspaceConfig, "") + + containerDetails, err := runtime.FindRunning( + ctx, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, + ) + if err != nil { + return resolvedExecTarget{}, err + } + + execResult := LoadExecResult(workspaceConfig, containerDetails) + workdir := opts.Workdir + if workdir == "" { + workdir = ResolveExecWorkdir(execResult, client.Workspace()) + } + + user := "" + if execResult != nil { + user = devcconfig.GetRemoteUser(execResult) + } + + target := ContainerTarget{ + ContainerID: containerDetails.ID, + User: user, + } + + userEnvProbe := "" + if execResult != nil && execResult.MergedConfig != nil { + userEnvProbe = execResult.MergedConfig.UserEnvProbe + } + probedEnv := runtime.ProbeEnv(ctx, target, userEnvProbe) + envSlice := envMapToSlice(opts.Env) + envMap := BuildExecEnv(execResult, envSlice, probedEnv) + + return resolvedExecTarget{ + runtime: runtime, + target: target, + workdir: workdir, + envMap: envMap, + }, nil +} + +func envMapToSlice(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k, v := range m { + out = append(out, k+"="+v) + } + return out +} diff --git a/pkg/workspace/exec_helpers.go b/pkg/workspace/exec_helpers.go deleted file mode 100644 index d807090fd..000000000 --- a/pkg/workspace/exec_helpers.go +++ /dev/null @@ -1,164 +0,0 @@ -package workspace - -import ( - "bytes" - "maps" - "os" - "path" - "strings" - - devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/log" - provider2 "github.com/devsy-org/devsy/pkg/provider" -) - -const ( - DefaultDockerCommand = "docker" - ContainerStatusRunning = "running" -) - -// ResolveDockerCommand returns the docker binary to invoke. Precedence: -// caller-supplied override → provider config (agent.docker.path) → default. -// Callers wired to a --docker-path style flag pass it as override; the override -// is honored even when workspace is nil so flag handling works at every call site. -func ResolveDockerCommand( - workspace *provider2.Workspace, - override string, -) string { - if override != "" { - return override - } - if workspace == nil || workspace.Context == "" { - return DefaultDockerCommand - } - - providerConfig, err := provider2.LoadProviderConfig( - workspace.Context, - workspace.Provider.Name, - ) - if err != nil { - log.Debugf("Failed to load provider config, defaulting to 'docker': %v", err) - return DefaultDockerCommand - } - - if providerConfig.Agent.Docker.Path != "" { - if expanded := os.ExpandEnv(providerConfig.Agent.Docker.Path); expanded != "" { - return expanded - } - } - - return DefaultDockerCommand -} - -func LoadExecResult( - workspaceConfig *provider2.Workspace, - containerDetails *devcconfig.ContainerDetails, -) *devcconfig.Result { - if workspaceConfig == nil || workspaceConfig.Context == "" || workspaceConfig.ID == "" { - return nil - } - - result, err := provider2.LoadWorkspaceResult(workspaceConfig.Context, workspaceConfig.ID) - if err != nil { - log.Warnf("Error loading workspace result: %v", err) - return nil - } - if result != nil { - result.ContainerDetails = containerDetails - } - return result -} - -func ResolveExecWorkdir(result *devcconfig.Result, workspaceName string) string { - if result != nil && result.MergedConfig != nil && result.MergedConfig.WorkspaceFolder != "" { - return result.MergedConfig.WorkspaceFolder - } - return path.Join("/workspaces", workspaceName) -} - -// BuildExecEnv merges probed env, result remote env, and caller-supplied env slices. -func BuildExecEnv( - result *devcconfig.Result, - cliEnv []string, - probedEnv map[string]string, -) map[string]string { - env := make(map[string]string, len(probedEnv)) - maps.Copy(env, probedEnv) - - if result != nil { - applyRemoteEnv(env, mergedRemoteEnv(result)) - } - - for _, e := range cliEnv { - if k, v, ok := strings.Cut(e, "="); ok { - env[k] = v - } - } - - return env -} - -func mergedRemoteEnv(result *devcconfig.Result) map[string]*string { - merged := map[string]*string{} - if result.MergedConfig != nil { - maps.Copy(merged, result.MergedConfig.RemoteEnv) - } - if result.DevContainerConfigWithPath != nil && result.DevContainerConfigWithPath.Config != nil { - maps.Copy(merged, result.DevContainerConfigWithPath.Config.RemoteEnv) - } - return merged -} - -func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { - for k, v := range remoteEnv { - if v == nil { - delete(env, k) - } else { - env[k] = *v - } - } -} - -// probeShellFlag returns the shell flag for the given probe mode. -// Kept as a package-level helper so DockerRuntime can use it. -func probeShellFlag(probe devcconfig.UserEnvProbe) string { - switch probe { - case devcconfig.LoginInteractiveShellProbe: - return "-lic" - case devcconfig.LoginShellProbe: - return "-lc" - case devcconfig.InteractiveShellProbe: - return "-ic" - default: - return "-c" - } -} - -// buildProbeArgs constructs the docker exec arguments for env probing. -// Kept as a package-level helper so DockerRuntime can use it. -func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []string { - args := []string{"exec"} - if target.User != "" { - args = append(args, "--user", target.User) - } - args = append(args, target.ContainerID, "sh", shellFlag, cmd) - return args -} - -// parseEnvOutput parses the output of an env probe command. -func parseEnvOutput(out []byte, sep byte) map[string]string { - entries := bytes.Split(out, []byte{sep}) - env := make(map[string]string, len(entries)) - for _, e := range entries { - if len(e) == 0 { - continue - } - name, value, ok := bytes.Cut(e, []byte{'='}) - if !ok || len(name) == 0 { - continue - } - env[string(name)] = string(value) - } - delete(env, "PWD") - return env -} diff --git a/pkg/workspace/exec_oneshot.go b/pkg/workspace/exec_oneshot.go deleted file mode 100644 index bf57d238d..000000000 --- a/pkg/workspace/exec_oneshot.go +++ /dev/null @@ -1,205 +0,0 @@ -package workspace - -import ( - "context" - "errors" - "fmt" - "io" - "time" - - "github.com/devsy-org/devsy/pkg/config" - "github.com/devsy-org/devsy/pkg/devcontainer" - devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/platform" -) - -// ExecOneShotOptions configures a single non-interactive command execution -// inside a workspace's running container. -type ExecOneShotOptions struct { - WorkspaceName string - Command []string - Workdir string - Env map[string]string - IDLabels []string // additional id-labels for container lookup; nil uses defaults - TimeoutSeconds int - TimeoutSecondsDefault int - TimeoutSecondsMax int - Owner platform.OwnerFilter - Context string - Provider string - Stdout io.Writer - Stderr io.Writer -} - -// defaultExecTimeoutSeconds bounds an exec when no caller or configured -// default applies. Tuned to surface hung calls without truncating typical -// build/test commands. -const defaultExecTimeoutSeconds = 300 - -// ExecOneShotResult is the structured outcome of an exec. -type ExecOneShotResult struct { - ExitCode int - DurationMS int64 - TimedOut bool - TimeoutSeconds int - Clamped bool -} - -// ResolveTimeout picks the first positive of TimeoutSeconds, TimeoutSecondsDefault, -// fallbackDefault, then clamps by TimeoutSecondsMax. The bool reports a clamp. -func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, bool) { - want := o.TimeoutSeconds - if want <= 0 { - want = o.TimeoutSecondsDefault - } - if want <= 0 { - want = fallbackDefault - } - if o.TimeoutSecondsMax > 0 && want > o.TimeoutSecondsMax { - return time.Duration(o.TimeoutSecondsMax) * time.Second, true - } - return time.Duration(want) * time.Second, false -} - -// ExecOneShot runs Command inside the workspace's container, captures -// stdout/stderr via the provided writers, and returns a structured result. -// It never reads stdin and never allocates a TTY. -func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResult, error) { - if opts.WorkspaceName == "" { - return nil, fmt.Errorf("workspace name is required") - } - if len(opts.Command) == 0 { - return nil, fmt.Errorf("command is required") - } - - timeout, clamped := opts.ResolveTimeout(defaultExecTimeoutSeconds) - - // Resolve under the parent context so docker lookup doesn't eat the exec budget. - resolved, err := resolveExecTarget(ctx, opts) - if err != nil { - return nil, err - } - - execCtx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - - start := time.Now() - exitCode, runErr := execOneShotWithRuntime(execCtx, resolved.runtime, ExecRequest{ - Target: resolved.target, - Workdir: resolved.workdir, - Env: resolved.envMap, - Argv: opts.Command, - Stdout: opts.Stdout, - Stderr: opts.Stderr, - }) - duration := time.Since(start) - - res := &ExecOneShotResult{ - ExitCode: exitCode, - DurationMS: duration.Milliseconds(), - Clamped: clamped, - TimeoutSeconds: int(timeout.Seconds()), - } - // Parent error first — execCtx inherits its cancellation, so we'd otherwise - // misreport caller cancellation as our own timeout. - if parentErr := ctx.Err(); parentErr != nil { - res.ExitCode = -1 - return res, parentErr - } - if errors.Is(execCtx.Err(), context.DeadlineExceeded) { - res.TimedOut = true - res.ExitCode = -1 - return res, nil - } - if runErr != nil { - return res, runErr - } - return res, nil -} - -// execOneShotWithRuntime is the low-level exec path; it accepts an already- -// resolved runtime and request so tests can inject a fake runtime without -// touching workspace resolution. -func execOneShotWithRuntime( - ctx context.Context, - runtime ContainerRuntime, - req ExecRequest, -) (int, error) { - return runtime.Exec(ctx, req) -} - -// resolvedExecTarget bundles the resolved runtime, target, workdir, and env -// for an exec — kept as a struct to stay within revive's function-result-limit. -type resolvedExecTarget struct { - runtime ContainerRuntime - target ContainerTarget - workdir string - envMap map[string]string -} - -// resolveExecTarget resolves the container runtime, target, workdir, and env -// map from options. -func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedExecTarget, error) { - devsyConfig, err := config.LoadConfig(opts.Context, opts.Provider) - if err != nil { - return resolvedExecTarget{}, fmt.Errorf("load config: %w", err) - } - - client, err := Get(ctx, GetOptions{ - DevsyConfig: devsyConfig, - Args: []string{opts.WorkspaceName}, - Owner: opts.Owner, - }) - if err != nil { - return resolvedExecTarget{}, fmt.Errorf("resolve workspace: %w", err) - } - - workspaceConfig := client.WorkspaceConfig() - runtime := NewDockerRuntime(workspaceConfig, "") - - containerDetails, err := runtime.FindRunning( - ctx, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), opts.IDLabels, - ) - if err != nil { - return resolvedExecTarget{}, err - } - - execResult := LoadExecResult(workspaceConfig, containerDetails) - workdir := opts.Workdir - if workdir == "" { - workdir = ResolveExecWorkdir(execResult, client.Workspace()) - } - - user := "" - if execResult != nil { - user = devcconfig.GetRemoteUser(execResult) - } - - target := ContainerTarget{ - ContainerID: containerDetails.ID, - User: user, - } - - userEnvProbe := "" - if execResult != nil && execResult.MergedConfig != nil { - userEnvProbe = execResult.MergedConfig.UserEnvProbe - } - probedEnv := runtime.ProbeEnv(ctx, target, userEnvProbe) - envSlice := envMapToSlice(opts.Env) - envMap := BuildExecEnv(execResult, envSlice, probedEnv) - - return resolvedExecTarget{ - runtime: runtime, - target: target, - workdir: workdir, - envMap: envMap, - }, nil -} - -func envMapToSlice(m map[string]string) []string { - out := make([]string, 0, len(m)) - for k, v := range m { - out = append(out, k+"="+v) - } - return out -} diff --git a/pkg/workspace/exec_oneshot_test.go b/pkg/workspace/exec_test.go similarity index 100% rename from pkg/workspace/exec_oneshot_test.go rename to pkg/workspace/exec_test.go diff --git a/pkg/workspace/runtime.go b/pkg/workspace/runtime.go deleted file mode 100644 index 590953047..000000000 --- a/pkg/workspace/runtime.go +++ /dev/null @@ -1,51 +0,0 @@ -package workspace - -import ( - "context" - "io" - - devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" -) - -// ContainerRuntime abstracts container-runtime operations used for workspace -// exec and env-probe. It is runtime-agnostic: callers do not need to know -// whether Docker, Podman, or a test double is underneath. -type ContainerRuntime interface { - // FindRunning looks up a single running container for the given workspace. - // workspaceID may be empty when only idLabels are provided. - FindRunning( - ctx context.Context, - workspaceID string, - idLabels []string, - ) (*devcconfig.ContainerDetails, error) - - // Exec runs the command described by req inside a container. It writes - // stdout and stderr to req.Stdout/req.Stderr and returns the process exit - // code. A non-nil error means the exec machinery itself failed (e.g. the - // docker binary could not be found), not that the command exited non-zero. - Exec(ctx context.Context, req ExecRequest) (exitCode int, err error) - - // ProbeEnv queries the container's environment using the given probe mode - // string (values defined by devcconfig.UserEnvProbe). Returns an empty map - // on any error. - ProbeEnv(ctx context.Context, target ContainerTarget, probeMode string) map[string]string -} - -// ExecRequest bundles all parameters for a single non-interactive container -// exec. Using a struct keeps the ContainerRuntime.Exec signature within the -// revive argument-limit rule while remaining extensible. -type ExecRequest struct { - Target ContainerTarget - Workdir string - Env map[string]string - Argv []string - Stdout io.Writer - Stderr io.Writer -} - -// ContainerTarget identifies a single container exec target. Runtime-agnostic -// data; runtimes consume it. -type ContainerTarget struct { - ContainerID string - User string -} From aa7a29202f9f6b8b45bb38545494058616895446 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:21:40 -0500 Subject: [PATCH 45/48] style(workspace): drop section dividers and tighten exec.go comments Removes the four divider banners and trims WHAT-not-WHY docstrings on unexported helpers (probeShellFlag, buildProbeArgs, parseEnvOutput, 'X implements ContainerRuntime' tags). Compresses verbose docstrings on ResolveDockerCommand, the runtime interface, DockerRuntime, ExecOneShot, ResolveTimeout, and the resolved-target helpers. --- pkg/workspace/exec.go | 98 ++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 66 deletions(-) diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go index bec1e806d..2d06a8166 100644 --- a/pkg/workspace/exec.go +++ b/pkg/workspace/exec.go @@ -27,19 +27,13 @@ const ( ContainerStatusRunning = "running" ) -// defaultExecTimeoutSeconds bounds an exec when no caller or configured -// default applies. Tuned to surface hung calls without truncating typical -// build/test commands. +// defaultExecTimeoutSeconds bounds an exec when no caller or configured default +// applies. Long enough for typical build/test, short enough to surface hangs. const defaultExecTimeoutSeconds = 300 -// ---------------------------------------------------------------------------- -// Workspace metadata helpers (runtime-agnostic). -// ---------------------------------------------------------------------------- - // ResolveDockerCommand returns the docker binary to invoke. Precedence: -// caller-supplied override → provider config (agent.docker.path) → default. -// Callers wired to a --docker-path style flag pass it as override; the override -// is honored even when workspace is nil so flag handling works at every call site. +// override → provider config (agent.docker.path) → default. The override is +// honored even when workspace is nil. func ResolveDockerCommand( workspace *provider2.Workspace, override string, @@ -138,7 +132,6 @@ func applyRemoteEnv(env map[string]string, remoteEnv map[string]*string) { } } -// probeShellFlag returns the shell flag for the given probe mode. func probeShellFlag(probe devcconfig.UserEnvProbe) string { switch probe { case devcconfig.LoginInteractiveShellProbe: @@ -152,7 +145,6 @@ func probeShellFlag(probe devcconfig.UserEnvProbe) string { } } -// buildProbeArgs constructs the docker exec arguments for env probing. func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []string { args := []string{"exec"} if target.User != "" { @@ -162,7 +154,6 @@ func buildProbeArgs(target ContainerTarget, shellFlag string, cmd string) []stri return args } -// parseEnvOutput parses the output of an env probe command. func parseEnvOutput(out []byte, sep byte) map[string]string { entries := bytes.Split(out, []byte{sep}) env := make(map[string]string, len(entries)) @@ -180,37 +171,28 @@ func parseEnvOutput(out []byte, sep byte) map[string]string { return env } -// ---------------------------------------------------------------------------- -// ContainerRuntime — abstraction over docker / podman / test doubles. -// ---------------------------------------------------------------------------- - -// ContainerRuntime abstracts container-runtime operations used for workspace -// exec and env-probe. Callers do not need to know whether Docker, Podman, or a -// test double is underneath. +// ContainerRuntime abstracts find/exec/probe over a container runtime so +// callers and tests don't depend on a particular CLI (docker, podman, ...). type ContainerRuntime interface { - // FindRunning looks up a single running container for the given workspace. - // workspaceID may be empty when only idLabels are provided. + // FindRunning resolves a running container by workspace ID and/or labels. + // A non-nil error includes the not-found and not-running cases. FindRunning( ctx context.Context, workspaceID string, idLabels []string, ) (*devcconfig.ContainerDetails, error) - // Exec runs the command described by req inside a container. It writes - // stdout and stderr to req.Stdout/req.Stderr and returns the process exit - // code. A non-nil error means the exec machinery itself failed (e.g. the - // docker binary could not be found), not that the command exited non-zero. + // Exec runs req inside a container and returns the process exit code. + // A non-nil error means the exec machinery itself failed (e.g. binary + // missing), not a non-zero exit. Exec(ctx context.Context, req ExecRequest) (exitCode int, err error) - // ProbeEnv queries the container's environment using the given probe mode - // string (values defined by devcconfig.UserEnvProbe). Returns an empty map - // on any error. + // ProbeEnv reads the container's environment via shell. Returns an empty + // map on any failure; probeMode comes from devcconfig.UserEnvProbe. ProbeEnv(ctx context.Context, target ContainerTarget, probeMode string) map[string]string } -// ExecRequest bundles all parameters for a single non-interactive container -// exec. Using a struct keeps the ContainerRuntime.Exec signature within the -// revive argument-limit rule while remaining extensible. +// ExecRequest is the per-call input to ContainerRuntime.Exec. type ExecRequest struct { Target ContainerTarget Workdir string @@ -220,25 +202,20 @@ type ExecRequest struct { Stderr io.Writer } -// ContainerTarget identifies a single container exec target. Runtime-agnostic -// data; runtimes consume it. +// ContainerTarget identifies a container and the user to exec as. type ContainerTarget struct { ContainerID string User string } -// ---------------------------------------------------------------------------- -// DockerRuntime — production implementation of ContainerRuntime. -// ---------------------------------------------------------------------------- - -// DockerRuntime shells out to a docker-compatible binary. +// DockerRuntime is the production ContainerRuntime, shelling out to a +// docker-compatible binary. type DockerRuntime struct { helper *docker.DockerHelper } -// NewDockerRuntime constructs a runtime that shells out to the docker-like -// binary chosen by ResolveDockerCommand. The override parameter is honored -// the same way ResolveDockerCommand honors it. +// NewDockerRuntime builds a DockerRuntime using ResolveDockerCommand to pick +// the binary; override wins if set. func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRuntime { return &DockerRuntime{ helper: &docker.DockerHelper{ @@ -247,11 +224,9 @@ func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRu } } -// DockerCommand exposes the resolved docker binary path (for diagnostics or -// callers that still need the raw string). +// DockerCommand returns the resolved binary path for callers that need the raw string. func (r *DockerRuntime) DockerCommand() string { return r.helper.DockerCommand } -// FindRunning implements ContainerRuntime. func (r *DockerRuntime) FindRunning( ctx context.Context, workspaceID string, @@ -280,7 +255,6 @@ func (r *DockerRuntime) FindRunning( return container, nil } -// Exec implements ContainerRuntime. func (r *DockerRuntime) Exec(ctx context.Context, req ExecRequest) (int, error) { execArgs := []string{"exec", "-i"} for k, v := range req.Env { @@ -315,7 +289,6 @@ func (r *DockerRuntime) Exec(ctx context.Context, req ExecRequest) (int, error) return -1, fmt.Errorf("exec in container %s: %w", req.Target.ContainerID, err) } -// ProbeEnv implements ContainerRuntime. func (r *DockerRuntime) ProbeEnv( ctx context.Context, target ContainerTarget, @@ -362,12 +335,8 @@ func (r *DockerRuntime) runProbeCommand( return stdout.Bytes(), '\n', nil } -// ---------------------------------------------------------------------------- -// ExecOneShot — the high-level non-interactive exec entry point. -// ---------------------------------------------------------------------------- - -// ExecOneShotOptions configures a single non-interactive command execution -// inside a workspace's running container. +// ExecOneShotOptions configures a single non-interactive exec inside a +// workspace's running container. type ExecOneShotOptions struct { WorkspaceName string Command []string @@ -384,7 +353,7 @@ type ExecOneShotOptions struct { Stderr io.Writer } -// ExecOneShotResult is the structured outcome of an exec. +// ExecOneShotResult is the outcome of an ExecOneShot call. type ExecOneShotResult struct { ExitCode int DurationMS int64 @@ -393,8 +362,9 @@ type ExecOneShotResult struct { Clamped bool } -// ResolveTimeout picks the first positive of TimeoutSeconds, TimeoutSecondsDefault, -// fallbackDefault, then clamps by TimeoutSecondsMax. The bool reports a clamp. +// ResolveTimeout picks the first positive of TimeoutSeconds, +// TimeoutSecondsDefault, fallbackDefault, then clamps by TimeoutSecondsMax. +// The bool is true when clamping applied. func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, bool) { want := o.TimeoutSeconds if want <= 0 { @@ -409,9 +379,8 @@ func (o ExecOneShotOptions) ResolveTimeout(fallbackDefault int) (time.Duration, return time.Duration(want) * time.Second, false } -// ExecOneShot runs Command inside the workspace's container, captures -// stdout/stderr via the provided writers, and returns a structured result. -// It never reads stdin and never allocates a TTY. +// ExecOneShot runs opts.Command in the workspace's container, capturing +// stdout/stderr via the provided writers. Never reads stdin, never allocates a TTY. func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResult, error) { if opts.WorkspaceName == "" { return nil, fmt.Errorf("workspace name is required") @@ -465,9 +434,8 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu return res, nil } -// execOneShotWithRuntime is the low-level exec path; it accepts an already- -// resolved runtime and request so tests can inject a fake runtime without -// touching workspace resolution. +// execOneShotWithRuntime is the testable seam: takes an already-resolved +// runtime so fakes can be injected without touching workspace lookup. func execOneShotWithRuntime( ctx context.Context, runtime ContainerRuntime, @@ -476,8 +444,8 @@ func execOneShotWithRuntime( return runtime.Exec(ctx, req) } -// resolvedExecTarget bundles the resolved runtime, target, workdir, and env -// for an exec — kept as a struct to stay within revive's function-result-limit. +// resolvedExecTarget wraps the resolve step's return values; a struct +// keeps it under revive's function-result-limit. type resolvedExecTarget struct { runtime ContainerRuntime target ContainerTarget @@ -485,8 +453,6 @@ type resolvedExecTarget struct { envMap map[string]string } -// resolveExecTarget resolves the container runtime, target, workdir, and env -// map from options. func resolveExecTarget(ctx context.Context, opts ExecOneShotOptions) (resolvedExecTarget, error) { devsyConfig, err := config.LoadConfig(opts.Context, opts.Provider) if err != nil { From 2a761aec00e1ecd47fb7c40769e9808edf5eb896 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:27:00 -0500 Subject: [PATCH 46/48] style: drop first-person voice from comments --- cmd/mcp/tools_workspace.go | 6 +++--- pkg/workspace/exec.go | 2 -- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/cmd/mcp/tools_workspace.go b/cmd/mcp/tools_workspace.go index 987e040b1..c6d5af842 100644 --- a/cmd/mcp/tools_workspace.go +++ b/cmd/mcp/tools_workspace.go @@ -103,7 +103,7 @@ func handleWorkspaceStatus(ctx context.Context, g *flags.GlobalFlags, name strin return client.WorkspaceConfig(), nil } -// errorResult builds an isError CallToolResult carrying our structured payload. +// errorResult builds an isError CallToolResult carrying the classified payload. // The raw error is logged so operators can see the unclassified failure detail. func errorResult(err error) *sdkmcp.CallToolResult { log.Errorf("mcp tool error: %v", err) @@ -185,9 +185,9 @@ func registerWorkspaceLifecycleTools(s *sdkmcp.Server, g *flags.GlobalFlags) { } func startWorkspace(ctx context.Context, g *flags.GlobalFlags, name string) error { - // RunFromOptions creates the workspace if the name doesn't resolve, so we + // RunFromOptions creates the workspace if the name doesn't resolve; // pre-check existence to keep workspace_start lookup-only. A TOCTOU race - // with a concurrent delete just surfaces as an actionable error. + // with a concurrent delete surfaces as an actionable error. devsyConfig, err := config.LoadConfig(g.Context, g.Provider) if err != nil { return err diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go index 2d06a8166..4c221ef1d 100644 --- a/pkg/workspace/exec.go +++ b/pkg/workspace/exec.go @@ -417,8 +417,6 @@ func ExecOneShot(ctx context.Context, opts ExecOneShotOptions) (*ExecOneShotResu Clamped: clamped, TimeoutSeconds: int(timeout.Seconds()), } - // Parent error first — execCtx inherits its cancellation, so we'd otherwise - // misreport caller cancellation as our own timeout. if parentErr := ctx.Err(); parentErr != nil { res.ExitCode = -1 return res, parentErr From 0f6337d0581245b324303f2da6def6f0a7d0895d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:28:04 -0500 Subject: [PATCH 47/48] style(workspace): drop obvious comments on resolvedExecTarget and DockerCommand --- pkg/workspace/exec.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go index 4c221ef1d..198beefcd 100644 --- a/pkg/workspace/exec.go +++ b/pkg/workspace/exec.go @@ -224,7 +224,6 @@ func NewDockerRuntime(workspace *provider2.Workspace, override string) *DockerRu } } -// DockerCommand returns the resolved binary path for callers that need the raw string. func (r *DockerRuntime) DockerCommand() string { return r.helper.DockerCommand } func (r *DockerRuntime) FindRunning( @@ -442,8 +441,6 @@ func execOneShotWithRuntime( return runtime.Exec(ctx, req) } -// resolvedExecTarget wraps the resolve step's return values; a struct -// keeps it under revive's function-result-limit. type resolvedExecTarget struct { runtime ContainerRuntime target ContainerTarget From e7a1a3f97f4689dddf839bfcd61103f4ed1ddf60 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 4 Jun 2026 18:30:43 -0500 Subject: [PATCH 48/48] style(workspace): trim excessive comments in up.go Drop WHAT-not-WHY docstrings on unexported helpers (buildUpCmd, applyConfig, stdout, finalizeUp follow-up), redundant 'for non-CLI callers' tags that leak PR context, and a comment that just restated a field name. --- cmd/workspace/up/up.go | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 01713884e..631fe31d5 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -49,11 +49,11 @@ type UpCmd struct { DotfilesScriptEnv []string // Key=Value to pass to install script DotfilesScriptEnvFile []string // Paths to files containing Key=Value pairs to pass to install script - // Out receives result/error JSON envelopes; nil falls back to os.Stdout (CLI default). + // Out receives result/error JSON envelopes; nil falls back to os.Stdout. Out io.Writer } -// Options is the structured input form of the up command, for non-CLI callers. +// Options is the structured input form of the up command. type Options struct { Source string // git URL, local path, image, or workspace name Name string // explicit workspace ID override @@ -62,22 +62,20 @@ type Options struct { DevcontainerPath string // path to devcontainer.json, relative to project } -// RunFromOptions runs the up logic without cobra, for callers with structured -// input and their own context cancellation. WithSignals is intentionally skipped. +// RunFromOptions runs the up logic without cobra. Callers own ctx cancellation; +// WithSignals is intentionally skipped. func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) error { cmd := buildUpCmd(g, opts) if err := cmd.validate(); err != nil { return err } - // Read from cmd.GlobalFlags (the copy) so opts.Provider overrides take effect. + // Read from the copy in cmd.GlobalFlags so opts.Provider overrides take effect. devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider) if err != nil { return fmt.Errorf("load devsy config: %w", err) } cmd.applyConfig(devsyConfig) - // Surface the missing-provider case here so non-CLI callers don't see a - // confusing downstream lookup error. if cmd.Provider == "" && devsyConfig.Current().DefaultProvider == "" { return fmt.Errorf("no provider specified and no default provider configured for context %q", cmd.Context) @@ -95,7 +93,6 @@ func RunFromOptions(ctx context.Context, g *flags.GlobalFlags, opts Options) err return cmd.Run(ctx, devsyConfig, client, args) } -// buildUpCmd constructs an UpCmd from structured options for non-CLI callers. func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { ide := opts.IDE if ide == "" { @@ -111,17 +108,14 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { } cmd := &UpCmd{ GlobalFlags: &gCopy, - // Discard JSON envelopes so they can't corrupt a shared stdout (e.g. - // an MCP stdio transport). - Out: io.Discard, + Out: io.Discard, // any callers wanting envelopes can set this themselves. } cmd.IDE = ide cmd.DevContainerPath = opts.DevcontainerPath if opts.Name != "" { cmd.ID = opts.Name } - // Mirror CLI flag defaults that ship as *bool so non-CLI callers don't - // silently get nil-pointer or false-default behavior. + // *bool flags lose their CLI default when the cobra registration is skipped. mountGitRootDefault := true cmd.MountWorkspaceGitRoot = &mountGitRootDefault return cmd @@ -171,8 +165,6 @@ func (cmd *UpCmd) Run( }) } -// applyConfig sets config-derived fields after loading the devsy config. -// Used by both execute() and RunFromOptions(). func (cmd *UpCmd) applyConfig(devsyConfig *config.Config) { if devsyConfig.ContextOption(config.ContextOptionSSHStrictHostKeyChecking) == config.BoolTrue { cmd.StrictHostKeyChecking = true @@ -246,8 +238,6 @@ func (cmd *UpCmd) maybeStartTunnel( return tunnelCleanup } -// stdout returns the output writer for structured JSON envelopes. -// Falls back to os.Stdout when no explicit writer is set, matching CLI behaviour. func (cmd *UpCmd) stdout() io.Writer { if cmd.Out != nil { return cmd.Out