From f939ec604925667ea8281e1fa74600688d482706 Mon Sep 17 00:00:00 2001 From: Toni Alatalo Date: Thu, 23 Apr 2026 02:56:14 +0300 Subject: [PATCH] dev_container: Align compose project name with reference CLI (#54302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > **Draft / open question for maintainers.** The failure mode this fixes is narrow — a new-Zed-created container exists under the `name`-field project while a CLI-derivation tool (`@devcontainers/cli`, VS Code) operates on the same folder (the container persists in Docker, so the originating Zed session doesn't need to still be open). See issue #54255 failure mode 3 and the fixture's step 6. > > I'd like to pose this as a question rather than a claim: is matching `@devcontainers/cli`'s `getProjectName` precedence something the project wants to take on, given the narrowness of the bug? I wrote this implementation mostly as a way to explore what parity would actually cost — happy to close it if you'd rather leave it as-is, or pare it down (e.g. just rule 4) if a partial match is preferable. > > The broader value beyond this specific bug: devcontainer impls agreeing on the same project name means containers created by Zed, the devcontainer CLI, and VS Code are interchangeable for the same folder, which feels worth it to me — but you know the project's priorities better. > > Folds in #54068 (detection) — closing that PR unmerged; its `MultipleMatchingContainers` error lands here. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #54255 ## Summary **Match `@devcontainers/cli`'s full `getProjectName` precedence.** Replaces `safe_id_lower(devcontainer.json's name)` with the five-step chain the reference CLI walks (see [`src/spec-node/dockerCompose.ts` in devcontainers/cli](https://github.com/devcontainers/cli/blob/main/src/spec-node/dockerCompose.ts)): 1. `COMPOSE_PROJECT_NAME` from the local environment. 2. `COMPOSE_PROJECT_NAME=` in the workspace `.env` file. 3. Top-level `name:` on the merged compose config, when at least one fragment declared it explicitly. 4. `${workspaceFolderBasename}_devcontainer` — only when the first compose file's directory is `/.devcontainer/`. 5. Otherwise, the plain basename of the first compose file's directory (no suffix). The old Zed implementation diverged at every one of those inputs: any user setting `COMPOSE_PROJECT_NAME`, shipping a `.env` with one, declaring a top-level compose `name:`, or pointing `dockerComposeFile` outside `.devcontainer/` (e.g. `"../docker-compose.yml"`) got a different project namespace than the CLI and VS Code, producing two compose projects for the same folder. Adds a small `sanitize_compose_project_name()` helper implementing the CLI's rules (lowercase + strip `[^-_a-z0-9]`) — notably preserving hyphens, which `safe_id_lower` would have replaced with underscores. Adds two helpers used by the precedence walk: - `parse_dotenv_compose_project_name` — line scan extracting `COMPOSE_PROJECT_NAME=…` from the workspace `.env`, matching the subset the CLI's regex dotenv reader recognizes. - `compose_fragment_declares_name` — parses each compose fragment with `yaml-rust2` (already a transitive workspace dep; slated to become a direct dep via #53922) and checks for a `name` key on the root mapping (block, quoted, or flow style all work), matching the CLI's own `yaml.load`. `docker compose config` always injects `name: devcontainer` into its merged output when no fragment declared one, so rule 3 needs to distinguish the user-provided case from the injected default — this helper supplies that signal. On YAML parse failure it returns "not declared" (rule 4 applies), matching the CLI's fallback. `project_name()` becomes async and fallible (`async fn project_name(&self) -> Result`) so it can load the `.env` file and each compose fragment via `self.fs.load`. Four call sites now `.await?` the derivation. Real I/O errors on the `.env` read propagate as `FilesystemError` (matching the CLI's narrow `ENOENT`/`EISDIR` swallow); fragment-rescan read errors are logged and skipped (matching the CLI's broader try/catch over its fragment read + parse). The `name` field is still used as the features image-tag prefix (`generate_features_image_tag`); only the compose project namespace is decoupled from it. **Duplicate-container detection (from #54068).** When `check_for_existing_container`'s label-based lookup returns more than one match, propagate `MultipleMatchingContainers(ids)` with instructions to clean up the stale one(s). This covers the mixed-version upgrade edge case where a pre-fix Zed left a container under the legacy project name alongside a CLI-style one — transparent to users in the common case (one tool, one container), explicit error when two legacy siblings need manual cleanup. ## Why Full write-up with verified fixtures and captured output: #54255. Three failure modes from the same root cause, all resolved by this change: 1. **Interop** — opening a folder in both Zed and `devcontainer up` (or Zed and VS Code) creates two compose projects with identical `devcontainer.local_folder` + `devcontainer.config_file` labels, breaking the spec's uniqueness invariant. 2. **Cross-worktree silent db/volume reuse** — if multiple git worktrees share a `devcontainer.json` with the same `name`, Zed uses the same compose project for all of them; Compose reuses stateful siblings (db, cache, localstack) by config-hash, so worktree B silently inherits worktree A's database. Fixture + captured output: [antont/zed-devcontainer-db-share-repro](https://github.com/antont/zed-devcontainer-db-share-repro). 3. **Mixed-version Zed sessions** — the Rust impl landed in stable v0.232.2 (2026-04-15, #52338). Older Zed (≤v0.231.x) shelled out to `@devcontainers/cli` so it used the reference derivation. The collision shows up when a new-Zed-created container exists under the name-field project while a CLI-derivation tool (old Zed, `devcontainer up`, VS Code) operates on the same folder. ## Migration / compatibility Existing Zed-created containers (under the old `safe_id_lower(name)` project) continue to be found via `check_for_existing_container`'s label-based lookup — they're looked up by `devcontainer.local_folder` + `devcontainer.config_file`, not by project name. A user with duplicate legacy containers from a prior Zed session sees `MultipleMatchingContainers` with cleanup instructions. ## Revision — 2026-04-22 Revised per @KyleBarton review on the prior version: - Swapped the YAML parser from `serde_yaml_ng` to `yaml-rust2` (already transitive via `tree-sitter-yaml`; net reduction of one direct workspace dep; also what #53922 will pull in). - Dropped the mixed-version tiebreak (`pick_canonical_container`) and its `com.docker.compose.project` serde label. The edge case it covered is transient enough to address via the explicit `MultipleMatchingContainers` error rather than permanent tiebreaking code. - Folded #54068's detection commit into this PR; #54068 closed unmerged. - Rebased onto `main`. ## Test plan - [x] `cargo test -p dev_container --lib` — 89 passed, including: - `sanitize_compose_project_name_matches_cli_rules` - `--project-name` assertion added to `test_spawns_devcontainer_with_docker_compose` - `check_for_existing_container_errors_when_multiple_match` - `derive_project_name_env_wins_over_everything` - `derive_project_name_dotenv_wins_over_compose_and_fallback` - `derive_project_name_compose_name_wins_over_fallback` - `derive_project_name_skips_compose_name_when_not_explicitly_declared` - `derive_project_name_omits_suffix_when_compose_file_outside_devcontainer_dir` - `derive_project_name_normalizes_compose_path_for_rule_4` - `compose_fragment_declares_name_detects_top_level_name_key` (covers block, quoted-key, and flow-style roots, plus parse failure → not-declared) - `is_missing_file_error_only_accepts_notfound_and_isadirectory` - [x] `cargo fmt --all` — clean - [x] `./script/clippy -p dev_container` — clean - [x] **End-to-end with fixture** [antont/zed-devcontainer-compose-test](https://github.com/antont/zed-devcontainer-compose-test): - Build `zed` from this branch. - Clean slate: `docker ps -a --filter "label=devcontainer.local_folder=$PWD" -q | xargs -r docker rm -f` - `zed --dev-container /path/to/devcontainer-compose-test` → Zed creates container under project `devcontainer-compose-test_devcontainer` (was `compose_duplicate_repro` before the fix). - `devcontainer up --workspace-folder $PWD` → CLI reports the same `containerId` Zed created; no second compose project is introduced. - Captured: `devcontainer-compose-test_devcontainer-app-1`, `composeProjectName: "devcontainer-compose-test_devcontainer"` reported by both tools. Release Notes: - Fixed dev container Docker Compose project name now matches the full `getProjectName` precedence from the reference devcontainer CLI (`COMPOSE_PROJECT_NAME` in the environment, then in the workspace `.env`, then an explicit top-level `name:` on the merged compose config, then the basename of the first compose file's directory — with the `_devcontainer` suffix only when that directory is `/.devcontainer`). This prevents duplicate containers when the same folder is opened with both Zed and the devcontainer CLI / VS Code. --------- Co-authored-by: Claude Opus 4.7 --- Cargo.lock | 2 + Cargo.toml | 1 + crates/dev_container/Cargo.toml | 2 + crates/dev_container/src/command_json.rs | 49 +- crates/dev_container/src/devcontainer_api.rs | 12 + .../src/devcontainer_manifest.rs | 537 +++++++++++++++++- crates/dev_container/src/docker.rs | 81 ++- 7 files changed, 665 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b388c6437ac..021ea9c72ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4894,6 +4894,7 @@ dependencies = [ name = "dev_container" version = "0.1.0" dependencies = [ + "anyhow", "async-tar", "async-trait", "env_logger 0.11.8", @@ -4918,6 +4919,7 @@ dependencies = [ "walkdir", "workspace", "worktree", + "yaml-rust2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 27f612605ff..5d403c48c6f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -806,6 +806,7 @@ web-time = "1.1.0" webrtc-sys = "0.3.23" wgpu = { git = "https://github.com/zed-industries/wgpu.git", branch = "v29" } windows-core = "0.61" +yaml-rust2 = "0.8" yawc = "0.2.5" zeroize = "1.8" zstd = "0.11" diff --git a/crates/dev_container/Cargo.toml b/crates/dev_container/Cargo.toml index 92c42f97a29..d051b51e8bf 100644 --- a/crates/dev_container/Cargo.toml +++ b/crates/dev_container/Cargo.toml @@ -5,11 +5,13 @@ publish.workspace = true edition.workspace = true [dependencies] +anyhow.workspace = true async-tar.workspace = true async-trait.workspace = true serde.workspace = true serde_json.workspace = true serde_json_lenient.workspace = true +yaml-rust2.workspace = true shlex.workspace = true http_client.workspace = true http.workspace = true diff --git a/crates/dev_container/src/command_json.rs b/crates/dev_container/src/command_json.rs index 9823fec4068..8226767f579 100644 --- a/crates/dev_container/src/command_json.rs +++ b/crates/dev_container/src/command_json.rs @@ -52,9 +52,8 @@ where if raw.is_empty() || raw.trim() == "[]" || raw.trim() == "{}" { return Ok(None); } - let value = serde_json_lenient::from_str(&raw) - .map_err(|e| format!("Error deserializing from raw json: {e}")); - value + serde_json_lenient::from_str(&raw) + .map_err(|e| format!("Error deserializing from raw json: {e}")) } else { let std_err = String::from_utf8_lossy(&output.stderr); Err(format!( @@ -62,3 +61,47 @@ where )) } } + +#[cfg(test)] +mod tests { + use std::process::ExitStatus; + + use super::*; + + fn success_output(stdout: &str) -> Output { + Output { + status: ExitStatus::default(), + stdout: stdout.as_bytes().to_vec(), + stderr: Vec::new(), + } + } + + #[derive(Debug, Deserialize, PartialEq)] + struct TestItem { + id: String, + } + + #[test] + fn test_deserialize_newline_delimited_json_rejected() { + // Strict single-value contract: NDJSON must be rejected. Commands that + // may legitimately return multiple rows (e.g. `docker ps`) parse their + // output themselves rather than routing through this helper. + let output = success_output("{\"id\":\"first\"}\n{\"id\":\"second\"}\n"); + let result: Result, String> = deserialize_json_output(output); + assert!(result.is_err(), "expected parse error, got {result:?}"); + } + + #[test] + fn test_deserialize_empty_output() { + let output = success_output(""); + let result: Option = deserialize_json_output(output).unwrap(); + assert_eq!(result, None); + } + + #[test] + fn test_deserialize_empty_object() { + let output = success_output("{}"); + let result: Option = deserialize_json_output(output).unwrap(); + assert_eq!(result, None); + } +} diff --git a/crates/dev_container/src/devcontainer_api.rs b/crates/dev_container/src/devcontainer_api.rs index 385185aacc1..a5ba5edd6f8 100644 --- a/crates/dev_container/src/devcontainer_api.rs +++ b/crates/dev_container/src/devcontainer_api.rs @@ -79,6 +79,11 @@ pub enum DevContainerError { FilesystemError, ResourceFetchFailed, NotInValidProject, + /// Multiple existing containers match this project's identifying labels + /// (`devcontainer.local_folder` + `devcontainer.config_file`). The spec + /// expects those labels to be unique per project, so Zed can't choose + /// which one to connect to. The user must remove the duplicate(s). + MultipleMatchingContainers(Vec), } impl Display for DevContainerError { @@ -112,6 +117,12 @@ impl Display for DevContainerError { DevContainerError::ResourceFetchFailed => "Failed to fetch resources from template or feature repository".to_string(), DevContainerError::DevContainerValidationFailed(failure) => failure.to_string(), + DevContainerError::MultipleMatchingContainers(ids) => format!( + "Multiple containers match this project's dev container labels ({}). \ + Zed can't decide which to connect to. Stop and remove the stale one(s) with \ + `docker stop ` and `docker rm `, then try again.", + ids.join(", ") + ), } ) } @@ -285,6 +296,7 @@ pub async fn start_dev_container_with_config( Ok((connection, remote_workspace_folder)) } + Err(err @ DevContainerError::MultipleMatchingContainers(_)) => Err(err), Err(err) => { let message = format!("Failed with nested error: {:?}", err); Err(DevContainerError::DevContainerUpFailed(message)) diff --git a/crates/dev_container/src/devcontainer_manifest.rs b/crates/dev_container/src/devcontainer_manifest.rs index 952b48bd9b2..79fa05b83bb 100644 --- a/crates/dev_container/src/devcontainer_manifest.rs +++ b/crates/dev_container/src/devcontainer_manifest.rs @@ -848,9 +848,14 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true let Some(docker_compose_files) = dev_container.docker_compose_file.clone() else { return Err(DevContainerError::DevContainerParseFailed); }; + // Normalize upfront so every downstream consumer of + // `DockerComposeResources.files` (compose fragment reads, project-name + // derivation, `docker compose -f` invocations, …) sees resolved paths. + // `dockerComposeFile` entries are joined verbatim with + // `config_directory`, so raw entries can carry `..` components. let docker_compose_full_paths = docker_compose_files .iter() - .map(|relative| self.config_directory.join(relative)) + .map(|relative| normalize_path(&self.config_directory.join(relative))) .collect::>(); let Some(config) = self @@ -984,8 +989,9 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true docker_compose_resources.files.push(config_location); + let project_name = self.project_name().await?; self.docker_client - .docker_compose_build(&docker_compose_resources.files, &self.project_name()) + .docker_compose_build(&docker_compose_resources.files, &project_name) .await?; ( self.docker_client @@ -1076,8 +1082,9 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true docker_compose_resources.files.push(config_location); + let project_name = self.project_name().await?; self.docker_client - .docker_compose_build(&docker_compose_resources.files, &self.project_name()) + .docker_compose_build(&docker_compose_resources.files, &project_name) .await?; ( @@ -1701,7 +1708,8 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true resources: DockerComposeResources, ) -> Result { let mut command = Command::new(self.docker_client.docker_cli()); - command.args(&["compose", "--project-name", &self.project_name()]); + let project_name = self.project_name().await?; + command.args(&["compose", "--project-name", &project_name]); for docker_compose_file in resources.files { command.args(&["-f", &docker_compose_file.display().to_string()]); } @@ -2096,15 +2104,80 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true .await } - fn project_name(&self) -> String { - if let Some(name) = &self.dev_container().name { - safe_id_lower(name) - } else { - let alternate_name = &self - .local_workspace_base_name() - .unwrap_or(self.local_workspace_folder()); - safe_id_lower(alternate_name) + /// Matches `@devcontainers/cli`'s `getProjectName` in + /// `src/spec-node/dockerCompose.ts`. See `derive_project_name` for the + /// full precedence. Using the devcontainer.json `name` field here + /// diverges from the reference CLI and creates duplicate compose + /// projects when the same folder is opened by both tools — see #54255. + /// + /// Async because the derivation reads both the workspace `.env` file + /// and the merged compose config — neither of which is available + /// synchronously. + async fn project_name(&self) -> Result { + let workspace_fallback = self + .local_workspace_base_name() + .unwrap_or_else(|_| self.local_workspace_folder()); + let compose_resources = self.docker_compose_manifest().await.ok(); + let first_compose_file = compose_resources + .as_ref() + .and_then(|r| r.files.first()) + .map(PathBuf::as_path); + let compose_config_name = compose_resources + .as_ref() + .and_then(|r| r.config.name.as_deref()); + let mut compose_name_explicitly_declared = false; + if let Some(resources) = &compose_resources { + for file in &resources.files { + // Mirrors the CLI's fragment re-parse (dockerCompose.ts 663-673): + // the whole readFile+yaml.load pair is wrapped in a single + // try/catch that swallows every failure. The comment there + // calls out `!reset` custom tags; the behavior is "on any + // failure, treat the fragment as not-declared and keep + // scanning." Propagating an I/O error here would diverge + // from that policy and fail the whole devcontainer flow for + // a fragment the CLI would have silently skipped. + let contents = match self.fs.load(file).await { + Ok(contents) => contents, + Err(err) => { + log::warn!( + "Ignoring unreadable compose fragment `{}` while deriving project name: {err:?}", + file.display() + ); + continue; + } + }; + if compose_fragment_declares_name(&contents) { + compose_name_explicitly_declared = true; + break; + } + } } + let dotenv_path = self.local_project_directory.join(".env"); + let dotenv_contents = match self.fs.load(&dotenv_path).await { + Ok(contents) => Some(contents), + Err(err) if is_missing_file_error(&err) => None, + Err(err) => { + // Mirrors the CLI: `getProjectName` only swallows `ENOENT`/ + // `EISDIR` on the `.env` read. Any other error (permission + // denied, I/O failure, …) must surface so we don't silently + // fall back to a non-canonical project name and create a + // second compose project for the same repo. + log::error!( + "Failed to read workspace .env `{}` while deriving project name: {err:?}", + dotenv_path.display() + ); + return Err(DevContainerError::FilesystemError); + } + }; + Ok(derive_project_name( + &self.local_environment, + dotenv_contents.as_deref(), + compose_config_name, + compose_name_explicitly_declared, + first_compose_file, + &self.local_project_directory, + &workspace_fallback, + )) } async fn expanded_dockerfile_content(&self) -> Result { @@ -2341,6 +2414,132 @@ fn escape_regex_chars(input: &str) -> String { result } +/// Sanitize a string for use as a Docker Compose project name, matching +/// `@devcontainers/cli`'s `toProjectName` (modern Compose branch): lowercase +/// the input and strip any character outside `[-_a-z0-9]`. +fn sanitize_compose_project_name(input: &str) -> String { + input + .chars() + .flat_map(|c| c.to_lowercase()) + .filter(|c| c.is_ascii_digit() || c.is_ascii_lowercase() || *c == '-' || *c == '_') + .collect() +} + +/// Derive the Docker Compose project name, mirroring `getProjectName` in +/// `@devcontainers/cli`'s `src/spec-node/dockerCompose.ts`. Precedence: +/// +/// 1. `COMPOSE_PROJECT_NAME` from the local environment. +/// 2. `COMPOSE_PROJECT_NAME` from the workspace `.env` file. +/// 3. The top-level `name:` field of the merged compose config, but only +/// when at least one compose fragment explicitly declared `name:`. +/// Compose injects a default `name: devcontainer` into its merged +/// output whenever no fragment declared one — that default must NOT be +/// treated as a user-provided name, so rule 4 applies instead. +/// 4. Basename of the first compose file's directory, appending +/// `_devcontainer` only when that directory is +/// `/.devcontainer`. +/// +/// The caller is responsible for computing `compose_name_explicitly_declared` +/// by scanning the original compose fragments for a top-level `name:` key +/// (the reference CLI does the same). This keeps the helper a pure function +/// of its inputs. +/// +/// All branches pass through `sanitize_compose_project_name` — the CLI's +/// final normalization step. +fn derive_project_name( + local_environment: &HashMap, + workspace_dotenv_contents: Option<&str>, + compose_config_name: Option<&str>, + compose_name_explicitly_declared: bool, + first_compose_file: Option<&Path>, + workspace_root: &Path, + workspace_fallback: &str, +) -> String { + if let Some(env_name) = local_environment.get("COMPOSE_PROJECT_NAME") + && !env_name.is_empty() + { + return sanitize_compose_project_name(env_name); + } + if let Some(contents) = workspace_dotenv_contents + && let Some(dotenv_name) = parse_dotenv_compose_project_name(contents) + && !dotenv_name.is_empty() + { + return sanitize_compose_project_name(&dotenv_name); + } + if let Some(name) = compose_config_name + && !name.is_empty() + && compose_name_explicitly_declared + { + return sanitize_compose_project_name(name); + } + let compose_dir = first_compose_file.and_then(Path::parent); + let canonical_devcontainer_dir = normalize_path(&workspace_root.join(".devcontainer")); + let raw = match compose_dir { + Some(dir) if dir == canonical_devcontainer_dir => { + // Matches the CLI's `configDir/.devcontainer` branch: use the + // *workspace root's* basename with the `_devcontainer` suffix, + // NOT the `.devcontainer` dir's basename. + format!("{workspace_fallback}_devcontainer") + } + Some(dir) => dir + .file_name() + .map(|f| f.to_string_lossy().into_owned()) + .unwrap_or_else(|| workspace_fallback.to_string()), + None => format!("{workspace_fallback}_devcontainer"), + }; + sanitize_compose_project_name(&raw) +} + +/// Classify an anyhow error from `Fs::load` as "file does not exist" vs a +/// real I/O failure. Used on the `.env` read in `project_name()`, where the +/// CLI's `getProjectName` catches only `ENOENT`/`EISDIR` and rethrows +/// everything else; any other error must propagate so callers can surface +/// the problem instead of silently falling back to a non-canonical project +/// name. (The fragment-rescan loop uses a different, broader swallow — +/// the CLI wraps its fragment read+parse in one try/catch that ignores +/// every failure.) +fn is_missing_file_error(err: &anyhow::Error) -> bool { + err.downcast_ref::().is_some_and(|e| { + matches!( + e.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::IsADirectory + ) + }) +} + +/// Extract `COMPOSE_PROJECT_NAME` from a `.env` file's contents. Matches +/// the subset of dotenv syntax that `@devcontainers/cli`'s regex parser +/// recognizes: a bare `COMPOSE_PROJECT_NAME=value` line (no `export` prefix, +/// no quoting, no line continuation). Comment lines are skipped. +fn parse_dotenv_compose_project_name(contents: &str) -> Option { + for line in contents.lines() { + let trimmed = line.trim_start(); + if trimmed.starts_with('#') { + continue; + } + if let Some(value) = trimmed.strip_prefix("COMPOSE_PROJECT_NAME=") { + return Some(value.trim().to_string()); + } + } + None +} + +/// Detect whether a compose-file fragment declares a top-level `name:` key. +/// Matches the reference CLI's approach: parse the fragment as YAML and check +/// for a `name` key on the root mapping. This handles all valid styles — +/// block mappings, quoted keys (`"name":`), flow-style root mappings, anchors, +/// etc. On parse failure we fall through (return `false`), matching the CLI's +/// own behavior when fragment parsing errors. +fn compose_fragment_declares_name(contents: &str) -> bool { + let Ok(docs) = yaml_rust2::YamlLoader::load_from_str(contents) else { + return false; + }; + let Some(yaml_rust2::Yaml::Hash(h)) = docs.into_iter().next() else { + return false; + }; + h.contains_key(&yaml_rust2::Yaml::String("name".to_string())) +} + /// Extracts the short feature ID from a full feature reference string. /// /// Examples: @@ -2881,7 +3080,9 @@ mod test { image: DockerInspect { id: "mcr.microsoft.com/devcontainers/base:ubuntu".to_string(), config: DockerInspectConfig { - labels: DockerConfigLabels { metadata: None }, + labels: DockerConfigLabels { + metadata: None, + }, image_user: None, env: Vec::new(), }, @@ -3719,6 +3920,28 @@ RUN apt-get update && export DEBIAN_FRONTEND=noninteractive \ let _devcontainer_up = devcontainer_manifest.build_and_run().await.unwrap(); + let docker_commands = test_dependencies + .command_runner + .commands_by_program("docker"); + let compose_up = docker_commands + .iter() + .find(|c| { + c.args.first().map(String::as_str) == Some("compose") + && c.args.iter().any(|a| a == "up") + }) + .expect("docker compose up command recorded"); + let project_name_idx = compose_up + .args + .iter() + .position(|a| a == "--project-name") + .expect("compose command has --project-name flag"); + assert_eq!( + compose_up.args[project_name_idx + 1], + "project_devcontainer", + "compose project name should match @devcontainers/cli derivation \ + (${{folderBasename}}_devcontainer), ignoring devcontainer.json `name`" + ); + let files = test_dependencies.fs.files(); let feature_dockerfile = files .iter() @@ -3932,6 +4155,252 @@ ENV DOCKER_BUILDKIT=1 ) } + #[test] + fn derive_project_name_env_wins_over_everything() { + // CLI precedence rule 1: `COMPOSE_PROJECT_NAME` env var short-circuits + // every later source (.env, compose name:, basename fallback). + use crate::devcontainer_manifest::derive_project_name; + + let env = HashMap::from([("COMPOSE_PROJECT_NAME".to_string(), "from_env".to_string())]); + let got = derive_project_name( + &env, + Some("COMPOSE_PROJECT_NAME=from_dotenv\n"), + Some("from_compose_name"), + true, + Some(Path::new( + "/path/to/local/project/.devcontainer/docker-compose.yml", + )), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got, "from_env"); + } + + #[test] + fn derive_project_name_dotenv_wins_over_compose_and_fallback() { + // CLI precedence rule 2: when no env var is set, the workspace .env's + // `COMPOSE_PROJECT_NAME=` line wins over the compose config's `name:` + // field and the basename fallback. + use crate::devcontainer_manifest::derive_project_name; + + let got = derive_project_name( + &HashMap::new(), + Some("# comment\nCOMPOSE_PROJECT_NAME=from_dotenv\n"), + Some("from_compose_name"), + true, + Some(Path::new( + "/path/to/local/project/.devcontainer/docker-compose.yml", + )), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got, "from_dotenv"); + } + + #[test] + fn derive_project_name_compose_name_wins_over_fallback() { + // CLI precedence rule 3: when neither env nor .env provide a name, + // the merged compose config's top-level `name:` field takes precedence + // over the basename fallback. Also covers sanitization (spaces + // stripped, uppercase lowercased). + use crate::devcontainer_manifest::derive_project_name; + + let got = derive_project_name( + &HashMap::new(), + None, + Some("My Compose Project"), + true, + Some(Path::new( + "/path/to/local/project/.devcontainer/docker-compose.yml", + )), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got, "mycomposeproject"); + } + + #[test] + fn derive_project_name_skips_compose_name_when_not_explicitly_declared() { + // CLI precedence rule 3 edge case: `docker compose config` injects a + // default `name: devcontainer` into the merged output whenever no + // compose fragment declared one. `@devcontainers/cli` ignores that + // default by tracking per-fragment whether `name:` was declared and + // skipping rule 3 if none was. The caller conveys that signal via + // `compose_name_explicitly_declared`; when it's `false`, even a + // non-empty `compose_config_name` must be skipped so rule 4 applies. + use crate::devcontainer_manifest::derive_project_name; + + let got = derive_project_name( + &HashMap::new(), + None, + Some("devcontainer"), + false, + Some(Path::new( + "/path/to/myworkspace/.devcontainer/docker-compose.yml", + )), + Path::new("/path/to/myworkspace"), + "myworkspace", + ); + assert_eq!(got, "myworkspace_devcontainer"); + } + + #[test] + fn derive_project_name_omits_suffix_when_compose_file_outside_devcontainer_dir() { + // CLI precedence rule 4: when falling back to the first compose file's + // directory basename, the `_devcontainer` suffix is only appended when + // that directory IS `/.devcontainer`. A compose file at the + // workspace root (as `"dockerComposeFile": "../docker-compose.yml"` + // produces) must derive to the plain dir basename, not + // `project_devcontainer` — otherwise Zed diverges from the CLI. + use crate::devcontainer_manifest::derive_project_name; + + let got = derive_project_name( + &HashMap::new(), + None, + None, + false, + Some(Path::new("/path/to/local/project/docker-compose.yml")), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got, "project"); + } + + #[test] + fn derive_project_name_handles_resolved_paths_from_docker_compose_manifest() { + // `docker_compose_manifest()` normalizes compose file paths upfront + // (resolving `..` components from raw `dockerComposeFile` entries like + // `"subdir/../docker-compose.yml"`) before populating + // `DockerComposeResources.files`. This test pins the resulting + // rule-4/rule-5 behavior on those normalized paths: a file + // semantically under `/.devcontainer` takes rule 4, and + // one that resolves outside it takes rule 5. + use crate::devcontainer_manifest::derive_project_name; + + // Normalized equivalent of `.devcontainer/subdir/../docker-compose.yml`: + // rule 4 applies → `${ws}_devcontainer`. + let got_under = derive_project_name( + &HashMap::new(), + None, + None, + false, + Some(Path::new( + "/path/to/local/project/.devcontainer/docker-compose.yml", + )), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got_under, "project_devcontainer"); + + // Normalized equivalent of `.devcontainer/../docker-compose.yml`: + // the file sits at the workspace root, so rule 5 applies — plain + // basename of the parent dir, no suffix. + let got_escaped = derive_project_name( + &HashMap::new(), + None, + None, + false, + Some(Path::new("/path/to/local/project/docker-compose.yml")), + Path::new("/path/to/local/project"), + "project", + ); + assert_eq!(got_escaped, "project"); + } + + #[test] + fn compose_fragment_declares_name_detects_top_level_name_key() { + // Block-style top-level key — declared. + use crate::devcontainer_manifest::compose_fragment_declares_name; + + assert!(compose_fragment_declares_name( + "name: my-project\nservices:\n app:\n image: foo\n" + )); + // Indented `name:` belongs to a nested mapping (here a service) and + // must NOT count as a top-level declaration. + assert!(!compose_fragment_declares_name( + "services:\n app:\n name: inner\n image: foo\n" + )); + // Comment lines are ignored. + assert!(!compose_fragment_declares_name( + "# name: commented-out\nservices: {}\n" + )); + // Empty fragment — no declaration. + assert!(!compose_fragment_declares_name("")); + // Quoted key — still a top-level declaration. A line scanner that + // looks for bare `name:` at column 0 would miss this. + assert!(compose_fragment_declares_name( + "\"name\": my-project\nservices: {}\n" + )); + // Flow-style root mapping — also a top-level declaration. Again a + // line scanner keyed on block-style layout would miss it. + assert!(compose_fragment_declares_name( + "{name: my-project, services: {app: {image: foo}}}\n" + )); + // Unparsable fragment falls through to "not declared" (matches the + // CLI's behavior on parse failure). + assert!(!compose_fragment_declares_name(": : :\n- - -\n")); + } + + #[test] + fn is_missing_file_error_only_accepts_notfound_and_isadirectory() { + // Mirrors the CLI's narrow `ENOENT`/`EISDIR` swallow in + // `getProjectName`'s `.env` read. Any other `io::Error` — permission + // denied, I/O failure, `ENOTDIR`, etc. — must not be classified as + // "missing" so callers surface the problem instead of silently + // falling back to a non-canonical project name. Non-`io::Error` + // anyhow errors must also not be classified as missing. + use crate::devcontainer_manifest::is_missing_file_error; + + let notfound = anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::NotFound)); + assert!(is_missing_file_error(¬found)); + + // EISDIR — `.env` exists as a directory; CLI swallows, so must we. + let is_a_dir = anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::IsADirectory)); + assert!(is_missing_file_error(&is_a_dir)); + + // ENOTDIR — a path component isn't a directory; CLI does NOT + // swallow this (its catch is narrow to ENOENT/EISDIR), so we must + // propagate it as a real failure. + let not_a_dir = anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::NotADirectory)); + assert!(!is_missing_file_error(¬_a_dir)); + + let permission_denied = + anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::PermissionDenied)); + assert!(!is_missing_file_error(&permission_denied)); + + let other_io = anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::Other)); + assert!(!is_missing_file_error(&other_io)); + + let non_io: anyhow::Error = anyhow::anyhow!("something else"); + assert!(!is_missing_file_error(&non_io)); + } + + #[test] + fn sanitize_compose_project_name_matches_cli_rules() { + use crate::devcontainer_manifest::sanitize_compose_project_name; + + // Plain lowercase alnum passes through. + assert_eq!( + sanitize_compose_project_name("project_devcontainer"), + "project_devcontainer" + ); + // Hyphens survive (unlike safe_id_lower which would replace them with _). + assert_eq!( + sanitize_compose_project_name("devcontainer-compose-test_devcontainer"), + "devcontainer-compose-test_devcontainer" + ); + // Uppercase letters are lowercased. + assert_eq!( + sanitize_compose_project_name("Makermint-Studio_devcontainer"), + "makermint-studio_devcontainer" + ); + // Characters outside [-_a-z0-9] are stripped. + assert_eq!( + sanitize_compose_project_name("Rust & PostgreSQL_devcontainer"), + "rustpostgresql_devcontainer" + ); + } + #[test] fn test_resolve_compose_dockerfile() { let compose = Path::new("/project/.devcontainer/docker-compose.yml"); @@ -5118,6 +5587,28 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de assert_eq!(base_image, "test_image:latest"); } + #[cfg(not(target_os = "windows"))] + #[gpui::test] + async fn check_for_existing_container_errors_when_multiple_match(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + let (test_dependencies, devcontainer_manifest) = + init_default_devcontainer_manifest(cx, r#"{"image": "image"}"#) + .await + .unwrap(); + test_dependencies + .docker + .set_duplicate_container_ids(vec!["abc123".to_string(), "def456".to_string()]); + + let result = devcontainer_manifest + .check_for_existing_devcontainer() + .await; + + let Err(DevContainerError::MultipleMatchingContainers(ids)) = result else { + panic!("expected MultipleMatchingContainers, got {result:?}"); + }; + assert_eq!(ids, vec!["abc123".to_string(), "def456".to_string()]); + } + #[test] fn test_aliases_dockerfile_with_pre_existing_aliases_for_build() {} @@ -5139,6 +5630,10 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de exec_commands_recorded: Mutex>, podman: bool, has_buildx: bool, + /// When `Some`, `find_process_by_filters` returns + /// `MultipleMatchingContainers` with these IDs. Used to exercise the + /// duplicate-container error path. + duplicate_container_ids: Mutex>>, } impl FakeDocker { @@ -5147,12 +5642,20 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de podman: false, has_buildx: true, exec_commands_recorded: Mutex::new(Vec::new()), + duplicate_container_ids: Mutex::new(None), } } #[cfg(not(target_os = "windows"))] fn set_podman(&mut self, podman: bool) { self.podman = podman; } + #[cfg(not(target_os = "windows"))] + fn set_duplicate_container_ids(&self, ids: Vec) { + *self + .duplicate_container_ids + .lock() + .expect("should be available") = Some(ids); + } } #[async_trait] @@ -5445,6 +5948,14 @@ FROM docker.io/hexpm/elixir:1.21-erlang-28.4.1-debian-trixie-20260316-slim AS de &self, _filters: Vec, ) -> Result, DevContainerError> { + if let Some(ids) = self + .duplicate_container_ids + .lock() + .expect("should be available") + .clone() + { + return Err(DevContainerError::MultipleMatchingContainers(ids)); + } Ok(Some(DockerPs { id: "found_docker_ps".to_string(), })) diff --git a/crates/dev_container/src/docker.rs b/crates/dev_container/src/docker.rs index 6fb5c88c1ff..be0fe0ed81b 100644 --- a/crates/dev_container/src/docker.rs +++ b/crates/dev_container/src/docker.rs @@ -379,8 +379,28 @@ impl DockerClient for Docker { &self, filters: Vec, ) -> Result, DevContainerError> { - let command = self.create_docker_query_containers(filters); - evaluate_json_command(command).await + let mut command = self.create_docker_query_containers(filters); + let output = command.output().await.map_err(|e| { + log::error!("Error running command {:?}: {e}", command); + DevContainerError::CommandFailed(command.get_program().display().to_string()) + })?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + log::error!("Non-success status from docker ps: {stderr}"); + return Err(DevContainerError::CommandFailed( + command.get_program().display().to_string(), + )); + } + let raw = String::from_utf8_lossy(&output.stdout); + parse_find_process_output(&raw).map_err(|e| { + // Preserve the dedicated multi-match error; log and re-wrap other parse failures. + if let DevContainerError::MultipleMatchingContainers(_) = &e { + e + } else { + log::error!("Error parsing docker ps output: {e}"); + DevContainerError::CommandFailed(command.get_program().display().to_string()) + } + }) } fn docker_cli(&self) -> String { @@ -392,6 +412,33 @@ impl DockerClient for Docker { } } +/// Parses output of `docker ps -a --format={{ json . }}`. When a single +/// container matches the label filters, docker emits one JSON object; when +/// multiple match, it emits newline-delimited JSON (one object per line). +/// +/// Returns `Ok(None)` for no matches, `Ok(Some(_))` for exactly one match, +/// and `DevContainerError::MultipleMatchingContainers` for ≥2 matches — the +/// spec expects identifying labels to be unique per project, so the caller +/// can't silently pick one. +fn parse_find_process_output(raw: &str) -> Result, DevContainerError> { + if raw.trim().is_empty() { + return Ok(None); + } + let containers: Vec = serde_json_lenient::Deserializer::from_str(raw) + .into_iter::() + .collect::>() + .map_err(|e| { + DevContainerError::CommandFailed(format!("failed to parse docker ps output: {e}")) + })?; + match containers.len() { + 0 => Ok(None), + 1 => Ok(containers.into_iter().next()), + _ => Err(DevContainerError::MultipleMatchingContainers( + containers.into_iter().map(|c| c.id).collect(), + )), + } +} + #[async_trait] pub(crate) trait DockerClient { async fn inspect(&self, id: &String) -> Result; @@ -532,10 +579,11 @@ mod test { use crate::{ command_json::deserialize_json_output, + devcontainer_api::DevContainerError, devcontainer_json::MountDefinition, docker::{ Docker, DockerComposeConfig, DockerComposeService, DockerComposeServicePort, - DockerComposeVolume, DockerInspect, DockerPs, + DockerComposeVolume, DockerInspect, DockerPs, parse_find_process_output, }, }; @@ -718,6 +766,33 @@ mod test { assert_eq!(result.id, "abdb6ab59573".to_string()); } + #[test] + fn parse_find_process_output_none() { + assert!(matches!(parse_find_process_output(""), Ok(None))); + assert!(matches!(parse_find_process_output(" \n\n"), Ok(None))); + } + + #[test] + fn parse_find_process_output_single() { + let raw = r#"{"ID":"abc123"}"#; + let result = parse_find_process_output(raw).expect("single match must parse"); + assert_eq!(result.unwrap().id, "abc123"); + } + + #[test] + fn parse_find_process_output_multiple_errors() { + // `docker ps --format={{ json . }}` emits newline-delimited JSON when + // multiple containers match the filters. The spec expects the + // identifying labels to be unique per project, so this is an error. + let raw = "{\"ID\":\"abc\"}\n{\"ID\":\"def\"}\n"; + match parse_find_process_output(raw) { + Err(DevContainerError::MultipleMatchingContainers(ids)) => { + assert_eq!(ids, vec!["abc".to_string(), "def".to_string()]); + } + other => panic!("expected MultipleMatchingContainers, got {other:?}"), + } + } + #[test] fn should_deserialize_object_metadata_from_docker_compose_container() { // The devcontainer CLI writes metadata as a bare JSON object (not an array)