Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ func BuildRoot() *cobra.Command {
rootCmd.AddCommand(NewExecCmd(globalFlags))
rootCmd.AddCommand(NewOutdatedCmd(globalFlags))
rootCmd.AddCommand(NewSetUpCmd(globalFlags))
rootCmd.AddCommand(NewRunUserCommandsCmd(globalFlags))
rootCmd.AddCommand(NewRunUserCommandsCmdAlias(globalFlags))

inheritCommandFlagsFromEnvironment(rootCmd)

Expand Down
212 changes: 212 additions & 0 deletions cmd/runusercommands.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
package cmd

import (
"context"
"fmt"
"os"
"sort"

"github.com/devsy-org/devsy/cmd/flags"
"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/types"
workspace2 "github.com/devsy-org/devsy/pkg/workspace"
"github.com/spf13/cobra"
)

// RunUserCommandsCmd holds the run-user-commands command flags.
type RunUserCommandsCmd struct {
*flags.GlobalFlags

WorkspaceFolder string
IDLabels []string
}

// NewRunUserCommandsCmd creates a new run-user-commands command.
func NewRunUserCommandsCmd(f *flags.GlobalFlags) *cobra.Command {
cmd := &RunUserCommandsCmd{GlobalFlags: f}
runE := func(cobraCmd *cobra.Command, _ []string) error {
return cmd.Run(cobraCmd.Context())
}

runCmd := &cobra.Command{
Use: "run-user-commands",
Short: "Executes lifecycle commands in a running workspace container",
RunE: runE,
}

runCmd.Flags().
StringVar(
&cmd.WorkspaceFolder,
"workspace-folder",
"",
"Path to the workspace folder",
)
_ = runCmd.MarkFlagRequired("workspace-folder")
runCmd.Flags().
StringArrayVar(
&cmd.IDLabels,
"id-label",
[]string{},
"Override the default container identification labels (format: key=value, can be specified multiple times)",
)

return runCmd
}

// NewRunUserCommandsCmdAlias creates the hidden camelCase alias for devcontainer CLI compat.
func NewRunUserCommandsCmdAlias(f *flags.GlobalFlags) *cobra.Command {
primary := NewRunUserCommandsCmd(f)
primary.Use = "runUserCommands"
primary.Hidden = true
return primary
}

type lifecycleExecParams struct {
ctx context.Context
helper *docker.DockerHelper
containerID string
envArgs []string
workdir string
}

// Run executes the run-user-commands logic.
func (cmd *RunUserCommandsCmd) Run(ctx context.Context) error {
if err := devcconfig.ValidateIDLabels(cmd.IDLabels); err != nil {
return err
}

params, result, err := cmd.resolveContainer(ctx)
if err != nil {
return err
}

if err := cmd.runLifecycleHooks(params, result); err != nil {
return err
}

user := devcconfig.GetRemoteUser(result)
log.Infof("lifecycle commands completed for container %s", params.containerID)
_ = devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir)
return nil
Comment on lines +91 to +94

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle WriteResultJSON failures instead of discarding them.

If writing the result envelope fails, the command still returns success, which leaves callers with no machine-readable result and masks the real failure.

💡 Suggested fix
-	user := devcconfig.GetRemoteUser(result)
-	log.Infof("lifecycle commands completed for container %s", params.containerID)
-	_ = devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir)
-	return nil
+	user := devcconfig.GetRemoteUser(result)
+	log.Infof("lifecycle commands completed for container %s", params.containerID)
+	if err := devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir); err != nil {
+		return err
+	}
+	return nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
user := devcconfig.GetRemoteUser(result)
log.Infof("lifecycle commands completed for container %s", params.containerID)
_ = devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir)
return nil
user := devcconfig.GetRemoteUser(result)
log.Infof("lifecycle commands completed for container %s", params.containerID)
if err := devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir); err != nil {
return err
}
return nil
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/runusercommands.go` around lines 91 - 94, The call to
devcconfig.WriteResultJSON in the end of the command currently discards its
error; change the code in the function containing user :=
devcconfig.GetRemoteUser(result) so that the returned error from
devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir)
is checked and propagated (or logged and returned) instead of being
ignored—i.e., capture the error, log a clear message with params.containerID
using log.Infof/log.Errorf as appropriate, and return the error (or wrap it) so
callers receive a failure when writing the result envelope fails.

}

func (cmd *RunUserCommandsCmd) resolveContainer(
ctx context.Context,
) (*lifecycleExecParams, *devcconfig.Result, error) {
devsyConfig, err := config.LoadConfig(cmd.Context, cmd.Provider)
if err != nil {
return nil, nil, err
}

client, err := workspace2.Get(ctx, workspace2.GetOptions{
DevsyConfig: devsyConfig,
Args: []string{cmd.WorkspaceFolder},
Owner: cmd.Owner,
})
if err != nil {
_ = devcconfig.WriteErrorJSON(os.Stderr, err.Error())
return nil, nil, fmt.Errorf("resolve workspace: %w", err)
}

workspaceConfig := client.WorkspaceConfig()
dockerCommand := resolveDockerCommand(workspaceConfig)

containerDetails, err := findRunningContainer(
ctx, dockerCommand, devcontainer.GetRunnerIDFromWorkspace(workspaceConfig), cmd.IDLabels,
)
if err != nil {
_ = devcconfig.WriteErrorJSON(os.Stderr, err.Error())
return nil, nil, err
}

result := loadExecResult(workspaceConfig, containerDetails)
if result == nil || result.MergedConfig == nil {
_ = devcconfig.WriteErrorJSON(
os.Stderr,
"no workspace result found; lifecycle commands unavailable",
)
return nil, nil, fmt.Errorf("no workspace result found; lifecycle commands unavailable")
}

params := &lifecycleExecParams{
ctx: ctx,
helper: &docker.DockerHelper{DockerCommand: dockerCommand},
containerID: containerDetails.ID,
envArgs: buildLifecycleEnvArgs(result),
workdir: resolveExecWorkdir(result, client.Workspace()),
}
return params, result, nil
}

func (cmd *RunUserCommandsCmd) runLifecycleHooks(
params *lifecycleExecParams,
result *devcconfig.Result,
) error {
hooks := []struct {
name string
cmds []types.LifecycleHook
}{
{"postCreateCommand", result.MergedConfig.PostCreateCommands},
{"postStartCommand", result.MergedConfig.PostStartCommands},
{"postAttachCommand", result.MergedConfig.PostAttachCommands},
}

for _, hook := range hooks {
for _, h := range hook.cmds {
if err := execLifecycleHook(params, hook.name, h); err != nil {
_ = devcconfig.WriteErrorJSON(os.Stderr, err.Error())
return fmt.Errorf("lifecycle hooks: %s: %w", hook.name, err)
}
}
}
return nil
}

func execLifecycleHook(params *lifecycleExecParams, name string, hook types.LifecycleHook) error {
if len(hook) == 0 {
return nil
}

for key, command := range hook {
if len(command) == 0 {
continue
}
log.Infof("running %s: %s %v", name, key, command)

args := buildDockerExecArgs(params.containerID, params.envArgs, params.workdir, command)
if err := params.helper.Run(params.ctx, args, os.Stdin, os.Stdout, os.Stderr); err != nil {
return fmt.Errorf("command %q failed: %w", key, err)
}
}
Comment on lines +181 to +184

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep hook stderr separate from the JSON envelope.

execLifecycleHook() forwards each lifecycle command's stderr to os.Stderr, and Run() later writes the result envelope to the same stream. Any hook that emits diagnostics will corrupt the envelope and break parsers downstream.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/runusercommands.go` around lines 181 - 184, The hook's stderr is
currently forwarded to os.Stderr inside execLifecycleHook which mixes
diagnostics with the JSON envelope written later by params.helper.Run; change
execLifecycleHook (and where it's invoked) to capture hook stderr into a
separate io.Writer (e.g., a bytes.Buffer or a dedicated params.hookStderr)
instead of passing os.Stderr, pass that buffer into params.helper.Run
invocations (the call site using params.helper.Run(params.ctx, args, os.Stdin,
os.Stdout, os.Stderr)), and ensure the JSON envelope is written only to
os.Stdout; emit any captured hook stderr to os.Stderr (or into a separate
diagnostics channel/field) outside or before/after the envelope so it cannot
corrupt the JSON output.


return nil
}

func buildLifecycleEnvArgs(result *devcconfig.Result) []string {
if result == nil || result.MergedConfig == nil {
return nil
}

env := result.MergedConfig.RemoteEnv
if len(env) == 0 {
return nil
}

keys := make([]string, 0, len(env))
for k, v := range env {
if v != nil {
keys = append(keys, k)
}
}
sort.Strings(keys)

args := make([]string, 0, len(keys)*2)
for _, k := range keys {
args = append(args, "-e", k+"="+*env[k])
}
return args
}
117 changes: 117 additions & 0 deletions cmd/runusercommands_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package cmd

import (
"testing"

"github.com/devsy-org/devsy/cmd/flags"
devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestNewRunUserCommandsCmd_CommandName(t *testing.T) {
cmd := NewRunUserCommandsCmd(&flags.GlobalFlags{})
assert.Equal(t, "run-user-commands", cmd.Use)
}

func TestNewRunUserCommandsCmdAlias_IsHidden(t *testing.T) {
cmd := NewRunUserCommandsCmdAlias(&flags.GlobalFlags{})
assert.Equal(t, "runUserCommands", cmd.Use)
assert.True(t, cmd.Hidden, "camelCase alias should be hidden")
}

func TestNewRunUserCommandsCmdAlias_RegisteredInRoot(t *testing.T) {
rootCmd := BuildRoot()
found := false
for _, sub := range rootCmd.Commands() {
if sub.Use == "runUserCommands" {
found = true
assert.True(t, sub.Hidden)
break
}
}
assert.True(t, found, "runUserCommands alias should be registered in root")
}

func TestNewRunUserCommandsCmd_WorkspaceFolderRequired(t *testing.T) {
cmd := NewRunUserCommandsCmd(&flags.GlobalFlags{})
cmd.SetArgs([]string{})
err := cmd.Execute()
assert.Error(t, err)
assert.Contains(t, err.Error(), "required flag")
}

func TestNewRunUserCommandsCmd_IDLabelFlag(t *testing.T) {
cmd := NewRunUserCommandsCmd(&flags.GlobalFlags{})
f := cmd.Flags().Lookup("id-label")
require.NotNil(t, f)
assert.Equal(t, "stringArray", f.Value.Type())
}

func TestBuildLifecycleEnvArgs_Nil(t *testing.T) {
args := buildLifecycleEnvArgs(nil)
assert.Nil(t, args)
}

func TestBuildLifecycleEnvArgs_NilMergedConfig(t *testing.T) {
result := &devcconfig.Result{}
args := buildLifecycleEnvArgs(result)
assert.Nil(t, args)
}

func TestBuildLifecycleEnvArgs_EmptyEnv(t *testing.T) {
result := &devcconfig.Result{
MergedConfig: &devcconfig.MergedDevContainerConfig{
DevContainerConfigBase: devcconfig.DevContainerConfigBase{
RemoteEnv: map[string]*string{},
},
},
}
args := buildLifecycleEnvArgs(result)
assert.Nil(t, args)
}

func TestBuildLifecycleEnvArgs_WithValues(t *testing.T) {
val := "bar"
result := &devcconfig.Result{
MergedConfig: &devcconfig.MergedDevContainerConfig{
DevContainerConfigBase: devcconfig.DevContainerConfigBase{
RemoteEnv: map[string]*string{
"FOO": &val,
},
},
},
}
args := buildLifecycleEnvArgs(result)
assert.Equal(t, []string{"-e", "FOO=bar"}, args)
}

func TestBuildLifecycleEnvArgs_NilValueSkipped(t *testing.T) {
val := "keep"
result := &devcconfig.Result{
MergedConfig: &devcconfig.MergedDevContainerConfig{
DevContainerConfigBase: devcconfig.DevContainerConfigBase{
RemoteEnv: map[string]*string{
"KEEP": &val,
"REMOVE": nil,
},
},
},
}
args := buildLifecycleEnvArgs(result)
assert.Contains(t, args, "-e")
assert.Contains(t, args, "KEEP=keep")
assert.NotContains(t, args, "REMOVE")
}

func TestRunUserCommandsCmd_RegisteredInRoot(t *testing.T) {
rootCmd := BuildRoot()
found := false
for _, sub := range rootCmd.Commands() {
if sub.Use == "run-user-commands" {
found = true
break
}
}
assert.True(t, found, "run-user-commands should be registered in root")
}
Loading