Skip to content

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

Merged
aledbf merged 2 commits into
mainfrom
secrets-remoteenv-and-removal
Nov 13, 2026
Merged

aledbf merged 2 commits into
mainfrom
secrets-remoteenv-and-removal

Conversation

@aledbf

@aledbf aledbf commented Nov 13, 2026

Copy link
Copy Markdown
Member

Two drifts from the reference Dev Container CLI v0.90.0.

1. --secrets-file values vs remoteEnv

TS: every lifecycle hook runs with { ...remoteEnv, ...secrets } (injectHeadless.ts runLifecycleCommand), where remoteEnv is probeRemoteEnv's { ...shellEnv, ...params.remoteEnv, ...config.remoteEnv }. Precedence, lowest to highest: probed env, --remote-env, config remoteEnv, secrets. This applies to up and run-user-commands (dotfiles also get { ...remoteEnv, ...secrets }). exec and set-up take no secrets file.

  • Before: Go put the secrets before --remote-env and the config's remoteEnv, so either one hid a secret of the same name.
  • After: secrets go last, so they win. The two near-identical builders (hookRemoteEnv in up.go and lifecycleRemoteEnv in run_user_commands.go) are now one, hookRemoteEnv(probed, remoteEnvs, cfgRemoteEnv, secrets). set-up passes no secrets, as before.

2. up --remove-existing-container

TS: findExistingContainer (singleContainer.ts) and openDockerComposeDevContainer (dockerCompose.ts) wrap removeContainer in output.start/output.stop with the text Removing Existing Container (Compose: Removing existing container.). A failure rejects with Command failed: <docker> rm -f <id>, which the setup catch turns into ContainerError("An error occurred setting up the container."). It carries the container ID only for Compose, where container is already assigned. up prints the error JSON and exits 1.

  • Before: Go wrote an info line Removing existing container <id>..., ignored the removal error (_ = …), and went on.
  • After: Go emits the same start/stop pair (no stop on failure, as in TS) and fails up with the same error JSON and exit 1. The engine error stays wrapped for errors.Is/As.
  • The documented divergence (#844, docs/DIVERGENCES.md) is unchanged: the removal still happens before initializeCommand and the disallowed-Features check.

Tests

  • TestHookRemoteEnvOrdersLaterSourcesLast (updated): a secret beats --remote-env and the config's remoteEnv of the same name, and the config's remoteEnv beats --remote-env. Replaces TestLifecycleRemoteEnvOrder, whose function was folded in. Before this change both tests asserted the old order.
  • TestRemoveExistingContainer: for image and Compose configs, success emits the start/stop pair. Failure emits only start and returns the TS error output (message, description, containerId only for Compose, --docker-path honored), wrapping the daemon error.
  • New runtime parity cases up.secrets-override-remote-env-success and run-user-commands.secrets-override-remote-env-success: a hook prints TOKEN (set by secrets, --remote-env, and config remoteEnv) and CONFIG_WINS (set by --remote-env and config remoteEnv). Against an origin/main binary both cases fail with config/config vs TS SecretValue1/config. With this change both match.
  • No parity case covers a failing removal: Go removes through the Engine API, so a --docker-path wrapper cannot make only Go's removal fail. The unit test covers it.

Checks (all run from the branch worktree)

  • go build ./...: ok
  • go tool task lint: ok
  • BASE=origin/main go tool task mutate: 3 refused, 0 survived
  • TMPDIR=/var/tmp/<unique> go tool task test:race: ok
  • go tool task parity:contract: 114 matched. The 2 failures, read-configuration.features-configuration-local-feature-{key-order,metadata}-success, come from a stray /tmp/.git on the dev machine and are unrelated.
  • go tool task test:e2e: ok
  • PARITY_COMMAND=up,run-user-commands go tool task parity:runtime: 109 matched, 22 failed, 4 arm64 skipped. Locally the stray /tmp/.git changes remoteWorkspaceFolder for workspaces under /tmp:
    • 17 of the 21 pre-existing failures also fail with an origin/main binary (same /tmp symptom, plus up.compose-with-name-success).
    • 2 passed when rerun alone. One was a Go timeout under load, the other a cleanup race.
    • up.image-reuses-base-image-success also passed when rerun alone.
    • up.compose-without-workspace-folder-uses-root-success is flaky on both binaries. The TS and Go sides share a Compose project name, and Go sometimes finds the TS side's container while it is being removed ("container is marked for removal"). It reproduces with the origin/main binary.
    • The two new cases hit the same /tmp symptom here. With their workspace moved to /var/tmp they match, and they fail against origin/main as described above.

🤖 Generated with Claude Code

aledbf and others added 2 commits November 13, 2026 01:42
The reference runs every lifecycle hook with {...remoteEnv, ...secrets},
where remoteEnv is the probed env, then --remote-env, then the config's
remoteEnv. up and run-user-commands put the secrets before both remoteEnv
sources, so a remoteEnv entry of the same name hid the secret.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
up ignored the error removing the existing container and went on. The
reference fails up with "An error occurred setting up the container."
and the message "Command failed: <docker> rm -f <id>" (with the
container ID for a Compose configuration), exit 1. The removal is now
logged as the reference's start/stop pair, "Removing Existing Container"
("Removing existing container." for Compose), instead of an info line.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aledbf
aledbf merged commit 3f5d6e5 into main Nov 13, 2026
13 checks passed
@aledbf
aledbf deleted the secrets-remoteenv-and-removal branch November 13, 2026 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant