fix(cli): match TS secrets precedence and --remove-existing-container failure

docs/parity/parity-matrix.yaml

@@ -3061,6 +3061,20 @@ initial_cases:
     class: lifecycle-runtime
     current_status: match
 
+  - id: run-user-commands.secrets-override-remote-env-success
+    lane: runtime
+    command: run-user-commands
+    priority: p1
+    docker_required: true
+    ts_cmd: "run-user-commands --workspace-folder ${WORKSPACE} --secrets-file ${SECRETS_FILE} --remote-env TOKEN=cli --remote-env CONFIG_WINS=cli --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}"
+    setup_cmd: "WORKSPACE=/tmp/parity-secrets-env-run-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf ${WORKSPACE}; mkdir -p ${WORKSPACE}; printf '%s' '{\"image\":\"alpine:3.20\",\"overrideCommand\":true,\"remoteEnv\":{\"TOKEN\":\"config\",\"CONFIG_WINS\":\"config\"},\"postCreateCommand\":\"printenv TOKEN CONFIG_WINS > env.postCreateCommand.testMarker\"}' > ${WORKSPACE}/.devcontainer.json; SECRETS_FILE=${WORKSPACE}/test-secrets-temp.json; printf '%s' '{\"TOKEN\":\"SecretValue1\"}' > ${SECRETS_FILE}; if [ \"${PARITY_SIDE}\" = \"ts\" ]; then ${PARITY_CLI_TS} up --workspace-folder ${WORKSPACE} --skip-post-create --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container >/dev/null; else ${PARITY_CLI_GO} up --workspace-folder ${WORKSPACE} --skip-post-create --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container >/dev/null; fi; echo WORKSPACE=$WORKSPACE; echo SECRETS_FILE=$SECRETS_FILE"
+    verify_cmd: "cat ${WORKSPACE}/env.postCreateCommand.testMarker"
+    cleanup_cmd: "IDS=$(docker ps -aq --filter label=parity.case=${PARITY_CASE_ID}-${PARITY_SIDE}); [ -n \"$IDS\" ] && docker rm -f $IDS >/dev/null 2>&1 || true; rm -rf ${WORKSPACE}"
+    asserts: [exit_code, stdout_normalized]
+    class: lifecycle-runtime
+    current_status: match
+    notes: "Same precedence as up.secrets-override-remote-env-success: run-user-commands hands runLifecycleHooks the probed remoteEnv and the secrets, and each hook runs with {...remoteEnv, ...secrets}."
+
   - id: run-user-commands.workspace-subfolder-config-success
     lane: runtime
     command: run-user-commands
@@ -3794,6 +3808,20 @@ initial_cases:
     current_status: match
     arm64_required: true
 
+  - id: up.secrets-override-remote-env-success
+    lane: runtime
+    command: up
+    priority: p1
+    docker_required: true
+    ts_cmd: "up --workspace-folder ${WORKSPACE} --secrets-file ${SECRETS_FILE} --remote-env TOKEN=cli --remote-env CONFIG_WINS=cli --id-label parity.case=${PARITY_CASE_ID}-${PARITY_SIDE} --remove-existing-container"
+    setup_cmd: "WORKSPACE=/tmp/parity-secrets-env-up-${PARITY_CASE_ID}-${PARITY_SIDE}; rm -rf ${WORKSPACE}; mkdir -p ${WORKSPACE}; printf '%s' '{\"image\":\"alpine:3.20\",\"overrideCommand\":true,\"remoteEnv\":{\"TOKEN\":\"config\",\"CONFIG_WINS\":\"config\"},\"postCreateCommand\":\"printenv TOKEN CONFIG_WINS > env.postCreateCommand.testMarker\"}' > ${WORKSPACE}/.devcontainer.json; SECRETS_FILE=${WORKSPACE}/test-secrets-temp.json; printf '%s' '{\"TOKEN\":\"SecretValue1\"}' > ${SECRETS_FILE}; echo WORKSPACE=$WORKSPACE; echo SECRETS_FILE=$SECRETS_FILE"
+    verify_cmd: "cat ${WORKSPACE}/env.postCreateCommand.testMarker"
+    cleanup_cmd: "docker rm -f ${CONTAINER_ID} >/dev/null 2>&1 || true; rm -rf ${WORKSPACE}"
+    asserts: [exit_code, stdout_normalized]
+    class: up-runtime
+    current_status: match
+    notes: "Lifecycle hooks see {...remoteEnv, ...secrets} (injectHeadless.ts runLifecycleCommand), where remoteEnv is the probed env, then --remote-env, then the config's remoteEnv (probeRemoteEnv): a --secrets-file value beats both remoteEnv sources of the same name, and the config's remoteEnv beats --remote-env. Go put the secrets before both, so remoteEnv won."
+
   - id: up.workspace-secrets-success
     lane: runtime
     command: up

internal/cli/run_user_commands.go

@@ -222,7 +222,11 @@ func runUserCommandsInContainer(ctx context.Context, out Output, opts *runUserCo
 
 	remoteUser := lifecycleRemoteUser(merged, cfg)
 	probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, lifecycleUserEnvProbe(opts.defaultUserEnvProbe, cfg), opts.containerSessionDataFolder)
-	remoteEnv := lifecycleRemoteEnv(probed, opts.secrets.env(), opts.remoteEnvs, cfg)
+	var cfgRemoteEnv map[string]*string
+	if cfg != nil {
+		cfgRemoteEnv = cfg.RemoteEnv
+	}
+	remoteEnv := hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv, opts.secrets.env())
 
 	shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, remoteEnv...)
 	defer shellServer.Close()
@@ -280,19 +284,3 @@ func lifecycleUserEnvProbe(defaultProbe string, cfg *config.DevContainer) lifecy
 	return lifecycle.UserEnvProbeStrategy(defaultProbe)
 }
 
-func lifecycleRemoteEnv(probed map[string]string, secrets, remoteEnvs []string, cfg *config.DevContainer) []string {
-	var env []string
-	for k, v := range probed {
-		env = append(env, fmt.Sprintf("%s=%s", k, v))
-	}
-	env = append(env, secrets...)
-	env = append(env, remoteEnvs...)
-	if cfg != nil {
-		for k, v := range cfg.RemoteEnv {
-			if v != nil {
-				env = append(env, fmt.Sprintf("%s=%s", k, *v))
-			}
-		}
-	}
-	return env
-}

internal/cli/run_user_commands_held_test.go

@@ -3,7 +3,6 @@ package cli
 import (
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"testing"
 
@@ -183,18 +182,3 @@ func TestLifecycleUserEnvProbe(t *testing.T) {
 		}
 	}
 }
-
-func TestLifecycleRemoteEnvOrder(t *testing.T) {
-	probed := map[string]string{"PATH": "/probed"}
-	secrets := []string{"TOKEN=s"}
-	cli := []string{"PATH=/cli"}
-	value := "/config"
-	cfg := &config.DevContainer{RemoteEnv: map[string]*string{"PATH": &value, "UNSET": nil}}
-
-	if got, want := lifecycleRemoteEnv(probed, secrets, cli, cfg), []string{"PATH=/probed", "TOKEN=s", "PATH=/cli", "PATH=/config"}; !slices.Equal(got, want) {
-		t.Errorf("with config: %v, want %v", got, want)
-	}
-	if got, want := lifecycleRemoteEnv(probed, secrets, cli, nil), []string{"PATH=/probed", "TOKEN=s", "PATH=/cli"}; !slices.Equal(got, want) {
-		t.Errorf("without config: %v, want %v", got, want)
-	}
-}

internal/cli/setup.go

@@ -159,7 +159,7 @@ func setUpContainer(ctx context.Context, out Output, engine *docker.EngineClient
 	}
 	probeStrategy := lifecycle.UserEnvProbeStrategy(cmp.Or(cfgProbe, opts.defaultUserEnvProbe))
 	probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, merged.RemoteUser, probeStrategy, opts.containerSessionDataFolder)
-	shellServer := lifecycle.NewShellServer(ctx, engine, containerID, merged.RemoteUser, hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv)...)
+	shellServer := lifecycle.NewShellServer(ctx, engine, containerID, merged.RemoteUser, hookRemoteEnv(probed, opts.remoteEnvs, cfgRemoteEnv, nil)...)
 	defer shellServer.Close()
 
 	executor := &lifecycle.ShellExecutor{Server: shellServer, Log: logger}

internal/cli/up.go

@@ -321,8 +321,9 @@ func runUp(ctx context.Context, out Output, opts *upOpts) error {
 
 	existingIDs, _ := engine.ListContainers(ctx, true, idLabels)
 	if len(existingIDs) > 0 && opts.removeExisting {
-		logger.Write(fmt.Sprintf("Removing existing container %s...", existingIDs[0]), log.LevelInfo)
-		_ = engine.RemoveContainer(ctx, existingIDs[0])
+		if err := removeExistingContainer(ctx, engine, logger, cfg, opts.dockerPath, existingIDs[0]); err != nil {
+			return writeErrorJSON(out, coreerrors.ToErrorOutput(err))
+		}
 		existingIDs = nil
 	}
 
@@ -986,8 +987,7 @@ func (r *upRunner) runLifecycleForUp(ctx context.Context, containerID string, cf
 	logger, engine, opts := r.log, r.engine, r.opts
 	probeStrategy := lifecycle.UserEnvProbeStrategy(cmp.Or(cfg.UserEnvProbe, opts.defaultUserEnvProbe))
 	probed := lifecycle.ProbedRemoteEnv(ctx, engine, logger, containerID, remoteUser, probeStrategy, opts.containerSessionDataFolder)
-	extra := append(opts.secrets.env(), opts.remoteEnvs...)
-	mergedRemoteEnv := hookRemoteEnv(probed, extra, cfg.RemoteEnv)
+	mergedRemoteEnv := hookRemoteEnv(probed, opts.remoteEnvs, cfg.RemoteEnv, opts.secrets.env())
 
 	shellServer := lifecycle.NewShellServer(ctx, engine, containerID, remoteUser, mergedRemoteEnv...)
 	defer shellServer.Close()
@@ -1033,18 +1033,54 @@ func (r *upRunner) checkContainerSubstitution(inspectResp container.InspectRespo
 	return checkContainerSubstitution(containerEnv, cfg.ConfigFilePath, raw, imagemeta.MergeConfiguration(entries))
 }
 
-func hookRemoteEnv(probed map[string]string, extra []string, cfgRemoteEnv map[string]*string) []string {
+func hookRemoteEnv(probed map[string]string, remoteEnvs []string, cfgRemoteEnv map[string]*string, secrets []string) []string {
 	var env []string
 	for k, v := range probed {
 		env = append(env, fmt.Sprintf("%s=%s", k, v))
 	}
-	env = append(env, extra...)
+	env = append(env, remoteEnvs...)
 	for k, v := range cfgRemoteEnv {
 		if v != nil {
 			env = append(env, fmt.Sprintf("%s=%s", k, *v))
 		}
 	}
-	return env
+	return append(env, secrets...)
+}
+
+type containerRemover interface {
+	RemoveContainer(ctx context.Context, id string) error
+}
+
+type containerRemoveError struct {
+	dockerPath string
+	id         string
+	err        error
+}
+
+func (e *containerRemoveError) Error() string {
+	return fmt.Sprintf("Command failed: %s rm -f %s", e.dockerPath, e.id)
+}
+
+func (e *containerRemoveError) Unwrap() error { return e.err }
+
+func removeExistingContainer(ctx context.Context, engine containerRemover, logger log.Logger, cfg *config.DevContainer, dockerPath, id string) error {
+	text := "Removing Existing Container"
+	if cfg.IsComposeConfig() {
+		text = "Removing existing container."
+	}
+	start := logger.Start(text)
+	if err := engine.RemoveContainer(ctx, id); err != nil {
+		removeErr := &coreerrors.ContainerError{
+			Description:   "An error occurred setting up the container.",
+			OriginalError: &containerRemoveError{dockerPath: cmp.Or(dockerPath, "docker"), id: id, err: err},
+		}
+		if cfg.IsComposeConfig() {
+			removeErr.ContainerID = id
+		}
+		return removeErr
+	}
+	logger.Stop(text, start)
+	return nil
 }
 
 func hookContainerError(hookErr error, containerID, fallback string) *coreerrors.ContainerError {

internal/cli/up_held_test.go

@@ -23,6 +23,7 @@ import (
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 
 	"github.com/devcontainers/cli/internal/config"
+	coreerrors "github.com/devcontainers/cli/internal/errors"
 	"github.com/devcontainers/cli/internal/docker"
 	"github.com/devcontainers/cli/internal/imagemeta"
 	"github.com/devcontainers/cli/internal/lifecycle"
@@ -467,12 +468,16 @@ func TestParseComposeServiceBuildReadsTheServicesBuild(t *testing.T) {
 func TestHookRemoteEnvOrdersLaterSourcesLast(t *testing.T) {
 	got := hookRemoteEnv(
 		map[string]string{"P": "probed"},
-		[]string{"S=secret", "R=cli"},
-		map[string]*string{"C": strPtr("config"), "UNSET": nil},
+		[]string{"R=cli", "TOKEN=cli"},
+		map[string]*string{"TOKEN": strPtr("config"), "UNSET": nil},
+		[]string{"TOKEN=secret"},
 	)
-	if want := []string{"P=probed", "S=secret", "R=cli", "C=config"}; !reflect.DeepEqual(got, want) {
+	if want := []string{"P=probed", "R=cli", "TOKEN=cli", "TOKEN=config", "TOKEN=secret"}; !reflect.DeepEqual(got, want) {
 		t.Errorf("env = %q, want %q", got, want)
 	}
+	if got, want := hookRemoteEnv(nil, []string{"R=cli"}, nil, nil), []string{"R=cli"}; !reflect.DeepEqual(got, want) {
+		t.Errorf("without config or secrets: env = %q, want %q", got, want)
+	}
 }
 
 func TestDropRanHooksAsksEveryMarkerAndDropsWhatRan(t *testing.T) {
@@ -652,3 +657,74 @@ func TestUpFeatureBuildOptionsCarryTheContainerEnv(t *testing.T) {
 		t.Errorf("ContainerEnv = %v, want the config's %v", got, cfg.ContainerEnv)
 	}
 }
+
+type fakeRemover struct {
+	removed []string
+	err     error
+}
+
+func (f *fakeRemover) RemoveContainer(_ context.Context, id string) error {
+	f.removed = append(f.removed, id)
+	return f.err
+}
+
+func TestRemoveExistingContainer(t *testing.T) {
+	daemonErr := errors.New("removal of container abc is already in progress")
+	image := &config.DevContainer{}
+	compose := &config.DevContainer{DockerComposeFile: config.StringOrStrings{"docker-compose.yml"}}
+	for _, tc := range []struct {
+		name       string
+		cfg        *config.DevContainer
+		dockerPath string
+		removeErr  error
+		wantEvents []string
+		wantErr    *coreerrors.ErrorOutput
+	}{
+		{"image config", image, "", nil, []string{"start:Removing Existing Container", "stop:Removing Existing Container"}, nil},
+		{"compose config", compose, "", nil, []string{"start:Removing existing container.", "stop:Removing existing container."}, nil},
+		{"image config fails", image, "", daemonErr, []string{"start:Removing Existing Container"}, &coreerrors.ErrorOutput{
+			Outcome: "error", Message: "Command failed: docker rm -f abc", Description: "An error occurred setting up the container.",
+		}},
+		{"compose config fails", compose, "/usr/bin/podman", daemonErr, []string{"start:Removing existing container."}, &coreerrors.ErrorOutput{
+			Outcome: "error", Message: "Command failed: /usr/bin/podman rm -f abc", Description: "An error occurred setting up the container.", ContainerID: "abc",
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			var logged bytes.Buffer
+			logger := log.New(log.Options{Writer: &logged, Format: "json", Level: log.LevelTrace})
+			engine := &fakeRemover{err: tc.removeErr}
+
+			err := removeExistingContainer(context.Background(), engine, logger, tc.cfg, tc.dockerPath, "abc")
+
+			if !slices.Equal(engine.removed, []string{"abc"}) {
+				t.Errorf("removed %q, want [abc]", engine.removed)
+			}
+			var events []string
+			for line := range strings.Lines(logged.String()) {
+				var e log.Event
+				if err := json.Unmarshal([]byte(line), &e); err != nil {
+					t.Fatalf("log line %q: %v", line, err)
+				}
+				events = append(events, e.Type+":"+e.Text)
+			}
+			if !slices.Equal(events, tc.wantEvents) {
+				t.Errorf("events = %q, want %q", events, tc.wantEvents)
+			}
+			if tc.wantErr == nil {
+				if err != nil {
+					t.Fatalf("err = %v, want nil", err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatal("err = nil, want the removal failure")
+			}
+			if got := coreerrors.ToErrorOutput(err); !reflect.DeepEqual(got, *tc.wantErr) {
+				t.Errorf("error output = %+v, want %+v", got, *tc.wantErr)
+			}
+			if !errors.Is(err, daemonErr) {
+				t.Errorf("err = %v, want it to wrap the daemon error", err)
+			}
+		})
+	}
+}