From a2aeaf1487dae306bfadb820c88a25ce08c92468 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 13 May 2026 23:33:03 +0200 Subject: [PATCH 1/8] Bump upstream devcontainers CLI to v0.87.0 --- upstream | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/upstream b/upstream index 6293ce587..65f98a518 160000 --- a/upstream +++ b/upstream @@ -1 +1 @@ -Subproject commit 6293ce5879399316f06287e42e710c0f8e5edfef +Subproject commit 65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9 From 428f0c872552bbbf79063763d04a584e60fc855f Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 13 May 2026 23:35:25 +0200 Subject: [PATCH 2/8] Add stable lockfile parity tests --- .../commands/configuration/tests/upgrade.rs | 46 +++++--- .../tests/runtime_build_smoke/features.rs | 100 +++++++++++++++++- 2 files changed, 125 insertions(+), 21 deletions(-) diff --git a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs index fb785c7c8..a6f8aad66 100644 --- a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs @@ -195,11 +195,34 @@ fn ensure_native_lockfile_uses_shared_lockfile_format() { fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); + ensure_native_lockfile( + &["--workspace-folder".to_string(), root.display().to_string()], + &config_file, + &json!({ + "image": "debian:bookworm", + "features": { + "ghcr.io/devcontainers/features/github-cli": {} + } + }), + ) + .expect("lockfile write"); + + let lockfile = fs::read_to_string(root.join(".devcontainer-lock.json")).expect("lockfile"); + assert!(lockfile.ends_with('\n')); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn ensure_native_lockfile_skips_generation_when_no_lockfile_is_set() { + let root = unique_temp_dir(); + fs::create_dir_all(&root).expect("failed to create root"); + let config_file = root.join(".devcontainer.json"); + ensure_native_lockfile( &[ "--workspace-folder".to_string(), root.display().to_string(), - "--experimental-lockfile".to_string(), + "--no-lockfile".to_string(), ], &config_file, &json!({ @@ -209,10 +232,9 @@ fn ensure_native_lockfile_uses_shared_lockfile_format() { } }), ) - .expect("lockfile write"); + .expect("lockfile skip"); - let lockfile = fs::read_to_string(root.join(".devcontainer-lock.json")).expect("lockfile"); - assert!(lockfile.ends_with('\n')); + assert!(!root.join(".devcontainer-lock.json").exists()); let _ = fs::remove_dir_all(root); } @@ -244,11 +266,7 @@ fn ensure_native_lockfile_rejects_corrupt_existing_lockfile_when_generating() { fs::write(&lockfile_path, "this is not json").expect("corrupt lockfile"); let error = ensure_native_lockfile( - &[ - "--workspace-folder".to_string(), - root.display().to_string(), - "--experimental-lockfile".to_string(), - ], + &["--workspace-folder".to_string(), root.display().to_string()], &config_file, &json!({ "image": "debian:bookworm", @@ -277,7 +295,7 @@ fn ensure_native_lockfile_reports_missing_frozen_lockfile() { &[ "--workspace-folder".to_string(), root.display().to_string(), - "--experimental-frozen-lockfile".to_string(), + "--frozen-lockfile".to_string(), ], &config_file, &json!({ @@ -299,11 +317,7 @@ fn ensure_native_lockfile_accepts_semantically_identical_existing_json() { fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); ensure_native_lockfile( - &[ - "--workspace-folder".to_string(), - root.display().to_string(), - "--experimental-lockfile".to_string(), - ], + &["--workspace-folder".to_string(), root.display().to_string()], &config_file, &json!({ "image": "debian:bookworm", @@ -322,7 +336,7 @@ fn ensure_native_lockfile_accepts_semantically_identical_existing_json() { &[ "--workspace-folder".to_string(), root.display().to_string(), - "--experimental-frozen-lockfile".to_string(), + "--frozen-lockfile".to_string(), ], &config_file, &json!({ diff --git a/cmd/devcontainer/tests/runtime_build_smoke/features.rs b/cmd/devcontainer/tests/runtime_build_smoke/features.rs index eb746f8d9..fcd7bf086 100644 --- a/cmd/devcontainer/tests/runtime_build_smoke/features.rs +++ b/cmd/devcontainer/tests/runtime_build_smoke/features.rs @@ -348,7 +348,7 @@ fn build_pushes_final_feature_image_instead_of_intermediate_base_image() { } #[test] -fn build_writes_feature_lockfile_when_requested() { +fn build_writes_feature_lockfile_by_default() { let harness = RuntimeHarness::new(); let workspace = harness.workspace(); fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); @@ -365,7 +365,6 @@ fn build_writes_feature_lockfile_when_requested() { fake_podman.as_str(), "--workspace-folder", workspace.to_string_lossy().as_ref(), - "--experimental-lockfile", ], &[], ); @@ -381,6 +380,99 @@ fn build_writes_feature_lockfile_when_requested() { assert!(lockfile.contains("\"resolved\":")); } +#[test] +fn build_no_lockfile_skips_feature_lockfile_write() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + write_devcontainer_config( + &workspace, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"ghcr.io/devcontainers/features/git:1.0\": {}\n }\n}\n", + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--no-lockfile", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + assert!(!workspace + .join(".devcontainer") + .join("devcontainer-lock.json") + .exists()); +} + +#[test] +fn build_rejects_mutually_exclusive_lockfile_flags() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + write_devcontainer_config( + &workspace, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"ghcr.io/devcontainers/features/git:1.0\": {}\n }\n}\n", + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--no-lockfile", + "--frozen-lockfile", + ], + &[], + ); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains("mutually exclusive"), "{stderr}"); + let invocations = + fs::read_to_string(harness.log_dir.join("invocations.log")).unwrap_or_default(); + assert!(!invocations.contains("build "), "{invocations}"); +} + +#[test] +fn build_experimental_lockfile_flag_emits_deprecation_warning() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + write_devcontainer_config( + &workspace, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"ghcr.io/devcontainers/features/git:1.0\": {}\n }\n}\n", + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--experimental-lockfile", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!( + stderr.contains("--experimental-lockfile is deprecated"), + "{stderr}" + ); +} + #[test] fn build_rejects_corrupt_existing_feature_lockfile_before_build_or_push() { let harness = RuntimeHarness::new(); @@ -408,7 +500,6 @@ fn build_rejects_corrupt_existing_feature_lockfile_before_build_or_push() { "--image-name", "example/native-build:corrupt-lockfile", "--push", - "--experimental-lockfile", ], &[], ); @@ -442,7 +533,6 @@ fn build_omits_additional_only_features_from_generated_lockfile() { workspace.to_string_lossy().as_ref(), "--additional-features", "{\"ghcr.io/devcontainers/features/github-cli\":{}}", - "--experimental-lockfile", ], &[], ); @@ -486,7 +576,7 @@ fn build_rejects_outdated_frozen_feature_lockfile() { fake_podman.as_str(), "--workspace-folder", workspace.to_string_lossy().as_ref(), - "--experimental-frozen-lockfile", + "--frozen-lockfile", ], &[], ); From 86f84d08c9a02d1e144b7799ff3621f09439a398 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 13 May 2026 23:38:12 +0200 Subject: [PATCH 3/8] Implement stable lockfile flags --- .../src/commands/configuration/mod.rs | 8 +++ .../src/commands/configuration/upgrade.rs | 64 +++++++++++++++---- cmd/devcontainer/src/runtime/mod.rs | 4 ++ 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/cmd/devcontainer/src/commands/configuration/mod.rs b/cmd/devcontainer/src/commands/configuration/mod.rs index 2ac451040..6d315c674 100644 --- a/cmd/devcontainer/src/commands/configuration/mod.rs +++ b/cmd/devcontainer/src/commands/configuration/mod.rs @@ -118,6 +118,14 @@ pub(crate) fn validate_native_lockfile( upgrade::validate_native_lockfile(args, config_file, configuration) } +pub(crate) fn validate_lockfile_options(args: &[String]) -> Result<(), String> { + upgrade::validate_lockfile_options(args) +} + +pub(crate) fn warn_deprecated_lockfile_flags(args: &[String]) { + upgrade::warn_deprecated_lockfile_flags(args); +} + pub(crate) fn should_use_native_read_configuration(args: &[String]) -> bool { read::should_use_native_read_configuration(args) } diff --git a/cmd/devcontainer/src/commands/configuration/upgrade.rs b/cmd/devcontainer/src/commands/configuration/upgrade.rs index c01746941..30393745f 100644 --- a/cmd/devcontainer/src/commands/configuration/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/upgrade.rs @@ -16,6 +16,11 @@ use crate::commands::collections::oci; use crate::commands::common; use crate::output::{CommandLogLevel, CommandLogger, LogFormat, TerminalDimensions}; +const NO_LOCKFILE_FLAG: &str = "--no-lockfile"; +const FROZEN_LOCKFILE_FLAG: &str = "--frozen-lockfile"; +const EXPERIMENTAL_LOCKFILE_FLAG: &str = "--experimental-lockfile"; +const EXPERIMENTAL_FROZEN_LOCKFILE_FLAG: &str = "--experimental-frozen-lockfile"; + pub(super) fn run_outdated(args: &[String]) -> ExitCode { let logger = outdated_logger(args); match validate_outdated_options(args) @@ -73,7 +78,8 @@ pub(super) fn ensure_native_lockfile( config_file: &Path, configuration: &Value, ) -> Result<(), String> { - if !wants_native_lockfile(args) { + validate_lockfile_options(args)?; + if lockfile_disabled(args) { return Ok(()); } @@ -83,7 +89,7 @@ pub(super) fn ensure_native_lockfile( let generated = generate_lockfile(configuration, workspace_folder.as_deref())?; let path = lockfile_path(config_file); let existing = existing_native_lockfile(args, &path)?; - if common::has_flag(args, "--experimental-frozen-lockfile") { + if lockfile_frozen(args) { let Some(existing) = existing else { return Err("Lockfile does not exist.".to_string()); }; @@ -93,11 +99,10 @@ pub(super) fn ensure_native_lockfile( path.display() )); } + return Ok(()); } - if common::has_flag(args, "--experimental-lockfile") { - let lockfile = serialized_lockfile(&generated)?; - fs::write(&path, lockfile).map_err(|error| error.to_string())?; - } + let lockfile = serialized_lockfile(&generated)?; + fs::write(&path, lockfile).map_err(|error| error.to_string())?; Ok(()) } @@ -106,13 +111,14 @@ pub(super) fn validate_native_lockfile( config_file: &Path, configuration: &Value, ) -> Result<(), String> { - if !wants_native_lockfile(args) { + validate_lockfile_options(args)?; + if lockfile_disabled(args) { return Ok(()); } let path = lockfile_path(config_file); let existing = existing_native_lockfile(args, &path)?; - if common::has_flag(args, "--experimental-frozen-lockfile") { + if lockfile_frozen(args) { let Some(existing) = existing else { return Err("Lockfile does not exist.".to_string()); }; @@ -130,13 +136,47 @@ pub(super) fn validate_native_lockfile( Ok(()) } -fn wants_native_lockfile(args: &[String]) -> bool { - common::has_flag(args, "--experimental-lockfile") - || common::has_flag(args, "--experimental-frozen-lockfile") +pub(super) fn validate_lockfile_options(args: &[String]) -> Result<(), String> { + if common::has_flag(args, NO_LOCKFILE_FLAG) { + for flag in [ + FROZEN_LOCKFILE_FLAG, + EXPERIMENTAL_FROZEN_LOCKFILE_FLAG, + EXPERIMENTAL_LOCKFILE_FLAG, + ] { + if common::has_flag(args, flag) { + return Err(format!( + "{NO_LOCKFILE_FLAG} and {flag} are mutually exclusive." + )); + } + } + } + Ok(()) +} + +pub(super) fn warn_deprecated_lockfile_flags(args: &[String]) { + if common::has_flag(args, EXPERIMENTAL_LOCKFILE_FLAG) { + eprintln!( + "Warning: {EXPERIMENTAL_LOCKFILE_FLAG} is deprecated. Lockfiles are now enabled by default." + ); + } + if common::has_flag(args, EXPERIMENTAL_FROZEN_LOCKFILE_FLAG) { + eprintln!( + "Warning: {EXPERIMENTAL_FROZEN_LOCKFILE_FLAG} is deprecated. Use {FROZEN_LOCKFILE_FLAG} instead." + ); + } +} + +fn lockfile_disabled(args: &[String]) -> bool { + common::has_flag(args, NO_LOCKFILE_FLAG) +} + +fn lockfile_frozen(args: &[String]) -> bool { + common::has_flag(args, FROZEN_LOCKFILE_FLAG) + || common::has_flag(args, EXPERIMENTAL_FROZEN_LOCKFILE_FLAG) } fn existing_native_lockfile(args: &[String], path: &Path) -> Result, String> { - if path.exists() || common::has_flag(args, "--experimental-frozen-lockfile") { + if path.exists() || lockfile_frozen(args) { read_lockfile(path.to_path_buf()) } else { Ok(None) diff --git a/cmd/devcontainer/src/runtime/mod.rs b/cmd/devcontainer/src/runtime/mod.rs index 0688face0..b624036b0 100644 --- a/cmd/devcontainer/src/runtime/mod.rs +++ b/cmd/devcontainer/src/runtime/mod.rs @@ -46,6 +46,8 @@ fn effective_up_resolved_config( } pub fn run_build(args: &[String]) -> Result { + configuration::validate_lockfile_options(args)?; + configuration::warn_deprecated_lockfile_flags(args); let resolved = context::load_required_config(args)?; let feature_support = configuration::resolve_feature_support( args, @@ -78,6 +80,8 @@ pub fn run_build(args: &[String]) -> Result { } pub fn run_up(args: &[String]) -> Result { + configuration::validate_lockfile_options(args)?; + configuration::warn_deprecated_lockfile_flags(args); let _ = mounts::cli_mount_values(args)?; let effective_resolved = effective_up_resolved_config(args, context::load_required_config(args)?)?; From 2517063d62dc0ce57294da054614bd931017c697 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 13 May 2026 23:40:22 +0200 Subject: [PATCH 4/8] Refresh upstream parity artifacts --- cmd/devcontainer/src/cli_metadata.json | 54 ++++++++++++++++++++++- docs/upstream/command-matrix.json | 6 ++- docs/upstream/command-reference.md | 6 ++- docs/upstream/compatibility-baseline.json | 4 +- docs/upstream/compatibility-dashboard.md | 4 +- docs/upstream/parity-inventory.json | 42 +++++++++++++++--- docs/upstream/parity-inventory.md | 12 ++--- docs/upstream/test-coverage-map.json | 6 +-- docs/upstream/test-coverage-map.md | 6 +-- 9 files changed, 114 insertions(+), 26 deletions(-) diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index 58e347cac..1e5872eb1 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -1,5 +1,5 @@ { - "upstreamCommit": "6293ce5879399316f06287e42e710c0f8e5edfef", + "upstreamCommit": "65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9", "sourcePath": "upstream/src/spec-node/devContainersSpecCLI.ts", "root": { "lines": [ @@ -428,6 +428,20 @@ ], "positionalNames": [] }, + { + "text": " --no-lockfile Disable lockfile generation and verification. [boolean] [default: false]", + "optionNames": [ + "no-lockfile" + ], + "positionalNames": [] + }, + { + "text": " --frozen-lockfile Ensure lockfile exists and remains unchanged; fail otherwise. [boolean] [default: false]", + "optionNames": [ + "frozen-lockfile" + ], + "positionalNames": [] + }, { "text": " --include-configuration Include configuration in result. [boolean] [default: false]", "optionNames": [ @@ -552,6 +566,12 @@ "description": null, "visible": false }, + { + "name": "frozen-lockfile", + "aliases": [], + "description": "Ensure lockfile exists and remains unchanged; fail otherwise. [boolean] [default: false]", + "visible": true + }, { "name": "gpu-availability", "aliases": [], @@ -606,6 +626,12 @@ "description": "Mount the workspace using its Git root. [boolean] [default: true]", "visible": true }, + { + "name": "no-lockfile", + "aliases": [], + "description": "Disable lockfile generation and verification. [boolean] [default: false]", + "visible": true + }, { "name": "omit-config-remote-env-from-metadata", "aliases": [], @@ -1218,6 +1244,20 @@ "additional-features" ], "positionalNames": [] + }, + { + "text": " --no-lockfile Disable lockfile generation and verification. [boolean] [default: false]", + "optionNames": [ + "no-lockfile" + ], + "positionalNames": [] + }, + { + "text": " --frozen-lockfile Ensure lockfile exists and remains unchanged; fail otherwise. [boolean] [default: false]", + "optionNames": [ + "frozen-lockfile" + ], + "positionalNames": [] } ], "options": [ @@ -1275,6 +1315,12 @@ "description": null, "visible": false }, + { + "name": "frozen-lockfile", + "aliases": [], + "description": "Ensure lockfile exists and remains unchanged; fail otherwise. [boolean] [default: false]", + "visible": true + }, { "name": "image-name", "aliases": [], @@ -1305,6 +1351,12 @@ "description": "Builds the image with `--no-cache`. [boolean] [default: false]", "visible": true }, + { + "name": "no-lockfile", + "aliases": [], + "description": "Disable lockfile generation and verification. [boolean] [default: false]", + "visible": true + }, { "name": "omit-syntax-directive", "aliases": [], diff --git a/docs/upstream/command-matrix.json b/docs/upstream/command-matrix.json index 25d6590e2..aa1695efa 100644 --- a/docs/upstream/command-matrix.json +++ b/docs/upstream/command-matrix.json @@ -1,5 +1,5 @@ { - "upstreamCommit": "6293ce5879399316f06287e42e710c0f8e5edfef", + "upstreamCommit": "65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9", "sourcePath": "upstream/src/spec-node/devContainersSpecCLI.ts", "topLevel": [ "up", @@ -38,6 +38,7 @@ "expect-existing-container", "experimental-frozen-lockfile", "experimental-lockfile", + "frozen-lockfile", "gpu-availability", "id-label", "include-configuration", @@ -47,6 +48,7 @@ "mount", "mount-git-worktree-common-dir", "mount-workspace-git-root", + "no-lockfile", "omit-config-remote-env-from-metadata", "omit-syntax-directive", "override-config", @@ -108,11 +110,13 @@ "docker-path", "experimental-frozen-lockfile", "experimental-lockfile", + "frozen-lockfile", "image-name", "label", "log-format", "log-level", "no-cache", + "no-lockfile", "omit-syntax-directive", "output", "platform", diff --git a/docs/upstream/command-reference.md b/docs/upstream/command-reference.md index e4c9d32f6..3a693d28a 100644 --- a/docs/upstream/command-reference.md +++ b/docs/upstream/command-reference.md @@ -2,7 +2,7 @@ Generated from the pinned upstream CLI command matrix. This is a compatibility baseline, not a native behavior reference. -- Upstream commit: `6293ce5879399316f06287e42e710c0f8e5edfef` +- Upstream commit: `65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9` - Source: `upstream/src/spec-node/devContainersSpecCLI.ts` ## Top-Level Commands @@ -45,6 +45,7 @@ Options: - `--expect-existing-container` - `--experimental-frozen-lockfile` - `--experimental-lockfile` +- `--frozen-lockfile` - `--gpu-availability` - `--id-label` - `--include-configuration` @@ -54,6 +55,7 @@ Options: - `--mount` - `--mount-git-worktree-common-dir` - `--mount-workspace-git-root` +- `--no-lockfile` - `--omit-config-remote-env-from-metadata` - `--omit-syntax-directive` - `--override-config` @@ -111,11 +113,13 @@ Options: - `--docker-path` - `--experimental-frozen-lockfile` - `--experimental-lockfile` +- `--frozen-lockfile` - `--image-name` - `--label` - `--log-format` - `--log-level` - `--no-cache` +- `--no-lockfile` - `--omit-syntax-directive` - `--output` - `--platform` diff --git a/docs/upstream/compatibility-baseline.json b/docs/upstream/compatibility-baseline.json index 68fa272d2..936a19caf 100644 --- a/docs/upstream/compatibility-baseline.json +++ b/docs/upstream/compatibility-baseline.json @@ -1,5 +1,5 @@ { "submodulePath": "upstream", - "pinnedCommit": "6293ce5879399316f06287e42e710c0f8e5edfef", - "compatibilityContract": "This repository targets upstream/ at commit 6293ce5879399316f06287e42e710c0f8e5edfef." + "pinnedCommit": "65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9", + "compatibilityContract": "This repository targets upstream/ at commit 65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9." } diff --git a/docs/upstream/compatibility-dashboard.md b/docs/upstream/compatibility-dashboard.md index cbfaf1f51..c716c010f 100644 --- a/docs/upstream/compatibility-dashboard.md +++ b/docs/upstream/compatibility-dashboard.md @@ -1,6 +1,6 @@ # Native Compatibility Dashboard -- Pinned upstream commit: `6293ce5879399316f06287e42e710c0f8e5edfef` +- Pinned upstream commit: `65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9` - Pinned spec commit: `c95ffeed1d059abfe9ffbe79762dc2fa4e7c2421` - Command matrix source: `docs/upstream/command-matrix.json` - Native parity inventory: `docs/upstream/parity-inventory.md` @@ -8,7 +8,7 @@ ## Current snapshot - Declared upstream command paths present natively: `20/20` -- Upstream options with a native source reference in mapped Rust sources: `200/200` +- Upstream options with a native source reference in mapped Rust sources: `204/204` - The parity inventory is a static source-evidence report. It is intended to identify obvious gaps and track drift, not to claim semantic parity by itself. ## Highest-Impact Gaps diff --git a/docs/upstream/parity-inventory.json b/docs/upstream/parity-inventory.json index b62a79dbc..c4f58b3b8 100644 --- a/docs/upstream/parity-inventory.json +++ b/docs/upstream/parity-inventory.json @@ -1,11 +1,11 @@ { - "upstreamCommit": "6293ce5879399316f06287e42e710c0f8e5edfef", + "upstreamCommit": "65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9", "sourcePath": "upstream/src/spec-node/devContainersSpecCLI.ts", "summary": { "commandPathsTotal": 20, "commandPathsDeclared": 20, - "optionsTotal": 200, - "optionsReferenced": 200, + "optionsTotal": 204, + "optionsReferenced": 204, "optionsMissing": 0 }, "commands": [ @@ -15,8 +15,8 @@ "description": "Create and run dev container", "declared": true, "optionSummary": { - "total": 43, - "referenced": 43, + "total": 45, + "referenced": 45, "missing": 0 }, "options": [ @@ -165,6 +165,13 @@ "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, + { + "name": "frozen-lockfile", + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] + }, { "name": "gpu-availability", "sourceReferenced": true, @@ -244,6 +251,13 @@ "cmd/devcontainer/src/runtime/exec.rs" ] }, + { + "name": "no-lockfile", + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] + }, { "name": "omit-config-remote-env-from-metadata", "sourceReferenced": true, @@ -550,8 +564,8 @@ "description": "Build a dev container image", "declared": true, "optionSummary": { - "total": 22, - "referenced": 22, + "total": 24, + "referenced": 24, "missing": 0 }, "options": [ @@ -633,6 +647,13 @@ "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, + { + "name": "frozen-lockfile", + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] + }, { "name": "image-name", "sourceReferenced": true, @@ -674,6 +695,13 @@ "cmd/devcontainer/src/runtime/compose/mod.rs" ] }, + { + "name": "no-lockfile", + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] + }, { "name": "omit-syntax-directive", "sourceReferenced": true, diff --git a/docs/upstream/parity-inventory.md b/docs/upstream/parity-inventory.md index d94773743..856b13278 100644 --- a/docs/upstream/parity-inventory.md +++ b/docs/upstream/parity-inventory.md @@ -2,10 +2,10 @@ Generated from the pinned upstream CLI command matrix and static source evidence in the Rust implementation. -- Upstream commit: `6293ce5879399316f06287e42e710c0f8e5edfef` +- Upstream commit: `65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9` - Source: `upstream/src/spec-node/devContainersSpecCLI.ts` - Declared upstream command paths present natively: `20/20` -- Upstream options with a native source reference in mapped files: `200/200` +- Upstream options with a native source reference in mapped files: `204/204` This report is a static inventory, not a semantic parity proof. A referenced option can still be only partially implemented, and command-level known gaps are called out explicitly below. @@ -13,9 +13,9 @@ This report is a static inventory, not a semantic parity proof. A referenced opt | Command | Declared | Option refs | Missing refs | Known gaps | | --- | --- | --- | --- | --- | -| `up` | yes | 43/43 | 0 | 2 | +| `up` | yes | 45/45 | 0 | 2 | | `set-up` | yes | 20/20 | 0 | 1 | -| `build` | yes | 22/22 | 0 | 2 | +| `build` | yes | 24/24 | 0 | 2 | | `run-user-commands` | yes | 27/27 | 0 | 1 | | `read-configuration` | yes | 18/18 | 0 | 2 | | `outdated` | yes | 8/8 | 0 | 1 | @@ -38,7 +38,7 @@ This report is a static inventory, not a semantic parity proof. A referenced opt - Description: Create and run dev container - Declared natively: yes -- Option source references: 43/43 +- Option source references: 45/45 - Missing option references: none - Known gaps: Native runtime now layers Features for image, dockerfile, and Docker Compose configs. Several upstream flags remain unimplemented or are only partially honored. @@ -54,7 +54,7 @@ This report is a static inventory, not a semantic parity proof. A referenced opt - Description: Build a dev container image - Declared natively: yes -- Option source references: 22/22 +- Option source references: 24/24 - Missing option references: none - Known gaps: Native runtime now layers Features for image, dockerfile, and Docker Compose configs. Several upstream build flags are still unimplemented or are only partially honored. diff --git a/docs/upstream/test-coverage-map.json b/docs/upstream/test-coverage-map.json index b3630c41d..d10fba9d1 100644 --- a/docs/upstream/test-coverage-map.json +++ b/docs/upstream/test-coverage-map.json @@ -1,5 +1,5 @@ { - "upstreamCommit": "6293ce5879399316f06287e42e710c0f8e5edfef", + "upstreamCommit": "65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9", "suites": [ { "upstreamTest": "upstream/src/test/cli.build.test.ts", @@ -172,7 +172,7 @@ "cmd/devcontainer/src/commands/configuration/tests/upgrade.rs", "cmd/devcontainer/tests/runtime_build_smoke/features.rs" ], - "notes": "Native lockfile generation coverage validates configured Feature filtering, additional-only Feature exclusion, trailing-newline writes, semantic frozen comparisons, corrupt lockfile failures, and build-path lockfile generation." + "notes": "Native lockfile generation coverage validates default generation, configured Feature filtering, additional-only Feature exclusion, trailing-newline writes, semantic frozen comparisons, corrupt lockfile failures, and build-path lockfile generation." }, { "upstreamTest": "upstream/src/test/container-features/lifecycleHooks.test.ts", @@ -191,7 +191,7 @@ "cmd/devcontainer/tests/cli_smoke/lockfile.rs", "cmd/devcontainer/src/commands/configuration/tests/upgrade.rs" ], - "notes": "Native lockfile coverage includes outdated, upgrade, dry-run, root-relative path handling, trailing-newline writes, missing frozen-lockfile errors, and workspace-local OCI layout mirrors for published Feature version and digest resolution." + "notes": "Native lockfile coverage includes stable and deprecated lockfile flags, no-lockfile skips, outdated, upgrade, dry-run, root-relative path handling, trailing-newline writes, missing frozen-lockfile errors, and workspace-local OCI layout mirrors for published Feature version and digest resolution." }, { "upstreamTest": "upstream/src/test/container-features/registryCompatibilityOCI.test.ts", diff --git a/docs/upstream/test-coverage-map.md b/docs/upstream/test-coverage-map.md index f503d188e..cb6ccea7b 100644 --- a/docs/upstream/test-coverage-map.md +++ b/docs/upstream/test-coverage-map.md @@ -2,7 +2,7 @@ Machine-readable upstream test coverage inventory for the native Rust CLI. -- Upstream commit: `6293ce5879399316f06287e42e710c0f8e5edfef` +- Upstream commit: `65f98a518a1f62355a08e6f38e9d6bfb9a0d8ac9` - Upstream tests inventoried: `36` - Covered: `16` - Partial: `20` @@ -29,9 +29,9 @@ Machine-readable upstream test coverage inventory for the native Rust CLI. | `upstream/src/test/container-features/featureHelpers.test.ts` | partial | `cmd/devcontainer/src/commands/collections/feature_tests/materialize.rs`
`cmd/devcontainer/src/commands/collections/tests/feature_tests.rs` | Native helper coverage focuses on test materialization, not the full upstream helper surface. | | `upstream/src/test/container-features/featuresCLICommands.test.ts` | partial | `cmd/devcontainer/tests/cli_smoke/collections.rs`
`cmd/devcontainer/src/commands/collections/tests/features.rs`
`cmd/devcontainer/src/commands/collections/tests/feature_tests.rs`
`cmd/devcontainer/src/commands/collections/tests/publish.rs` | CLI coverage exists for Features commands, but published flows remain substitute-based. | | `upstream/src/test/container-features/generateFeaturesConfig.test.ts` | covered | `cmd/devcontainer/src/commands/configuration/tests/read.rs`
`cmd/devcontainer/tests/runtime_build_smoke/features.rs` | Native read-configuration coverage validates generated local Feature sets, option values, published Feature customizations, metadata merge behavior, and runtime build smoke coverage validates install materialization. | -| `upstream/src/test/container-features/generateLockfile.test.ts` | covered | `cmd/devcontainer/src/commands/configuration/tests/upgrade.rs`
`cmd/devcontainer/tests/runtime_build_smoke/features.rs` | Native lockfile generation coverage validates configured Feature filtering, additional-only Feature exclusion, trailing-newline writes, semantic frozen comparisons, corrupt lockfile failures, and build-path lockfile generation. | +| `upstream/src/test/container-features/generateLockfile.test.ts` | covered | `cmd/devcontainer/src/commands/configuration/tests/upgrade.rs`
`cmd/devcontainer/tests/runtime_build_smoke/features.rs` | Native lockfile generation coverage validates default generation, configured Feature filtering, additional-only Feature exclusion, trailing-newline writes, semantic frozen comparisons, corrupt lockfile failures, and build-path lockfile generation. | | `upstream/src/test/container-features/lifecycleHooks.test.ts` | covered | `cmd/devcontainer/tests/runtime_lifecycle_smoke.rs`
`cmd/devcontainer/tests/runtime_lifecycle_smoke/commands.rs`
`cmd/devcontainer/tests/runtime_lifecycle_smoke/selection.rs` | Native lifecycle smoke coverage now includes Feature-contributed hooks merged before devcontainer-level hooks, install-order-sensitive hook ordering, resume/run-user-commands paths, and secrets propagation through lifecycle exec. | -| `upstream/src/test/container-features/lockfile.test.ts` | covered | `cmd/devcontainer/tests/cli_smoke/lockfile.rs`
`cmd/devcontainer/src/commands/configuration/tests/upgrade.rs` | Native lockfile coverage includes outdated, upgrade, dry-run, root-relative path handling, trailing-newline writes, missing frozen-lockfile errors, and workspace-local OCI layout mirrors for published Feature version and digest resolution. | +| `upstream/src/test/container-features/lockfile.test.ts` | covered | `cmd/devcontainer/tests/cli_smoke/lockfile.rs`
`cmd/devcontainer/src/commands/configuration/tests/upgrade.rs` | Native lockfile coverage includes stable and deprecated lockfile flags, no-lockfile skips, outdated, upgrade, dry-run, root-relative path handling, trailing-newline writes, missing frozen-lockfile errors, and workspace-local OCI layout mirrors for published Feature version and digest resolution. | | `upstream/src/test/container-features/registryCompatibilityOCI.test.ts` | partial | `cmd/devcontainer/src/commands/collections/tests/features.rs`
`cmd/devcontainer/tests/network_smoke/ghcr.rs` | Native coverage now includes OCI-manifest-shaped `features info manifest`, canonical ids, `publishedTags` output, and a dedicated anonymous GHCR manifest smoke check, but registry auth and broader live OCI pull flows are still missing. | | `upstream/src/test/container-templates/containerTemplatesOCI.test.ts` | partial | `cmd/devcontainer/src/commands/collections/tests/templates.rs`
`cmd/devcontainer/tests/cli_smoke/collections.rs` | Native coverage now includes workspace-local OCI metadata lookup and archive-backed template apply flows, but live registry template fetch behavior is still missing. | | `upstream/src/test/container-templates/templatesCLICommands.test.ts` | partial | `cmd/devcontainer/tests/cli_smoke/collections.rs`
`cmd/devcontainer/src/commands/collections/tests/templates.rs`
`cmd/devcontainer/src/commands/collections/tests/publish.rs` | Template CLI commands now cover workspace-local OCI metadata and apply flows, but published-template behavior is still partial without live registry fetches. | From b9cb4707eea99b2f1a996934634a21e362169b38 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Thu, 14 May 2026 07:56:23 +0200 Subject: [PATCH 5/8] Honor resolved feature lockfiles --- .../src/commands/collections/oci.rs | 11 + .../src/commands/configuration/catalog.rs | 10 - .../src/commands/configuration/features.rs | 1 + .../configuration/features/resolve.rs | 158 +++++++++++++- .../commands/configuration/features/types.rs | 11 + .../src/commands/configuration/mod.rs | 6 +- .../commands/configuration/tests/upgrade.rs | 26 ++- .../src/commands/configuration/upgrade.rs | 186 ++++++----------- cmd/devcontainer/src/runtime/build.rs | 4 + cmd/devcontainer/src/runtime/compose/mod.rs | 4 +- .../tests/runtime_build_smoke/features.rs | 196 ++++++++++++++++++ docs/upstream/parity-inventory.json | 9 +- 12 files changed, 471 insertions(+), 151 deletions(-) diff --git a/cmd/devcontainer/src/commands/collections/oci.rs b/cmd/devcontainer/src/commands/collections/oci.rs index c07978c76..0d6e2f197 100644 --- a/cmd/devcontainer/src/commands/collections/oci.rs +++ b/cmd/devcontainer/src/commands/collections/oci.rs @@ -135,6 +135,17 @@ pub(crate) fn resolve_feature_artifact( resolve_feature_artifact_for_reference(&parsed, workspace_folder, &CurlTransport) } +pub(crate) fn resolve_feature_artifact_with_digest( + reference: &str, + manifest_digest: &str, + workspace_folder: Option<&Path>, +) -> Result { + let mut parsed = parse_oci_reference(reference) + .ok_or_else(|| format!("Invalid OCI Feature reference: {reference}"))?; + parsed.digest = Some(manifest_digest.to_string()); + resolve_feature_artifact_for_reference(&parsed, workspace_folder, &CurlTransport) +} + pub(crate) fn list_feature_tags( reference: &str, workspace_folder: Option<&Path>, diff --git a/cmd/devcontainer/src/commands/configuration/catalog.rs b/cmd/devcontainer/src/commands/configuration/catalog.rs index 7a307146a..aab8dff15 100644 --- a/cmd/devcontainer/src/commands/configuration/catalog.rs +++ b/cmd/devcontainer/src/commands/configuration/catalog.rs @@ -178,16 +178,6 @@ pub(super) fn latest_version(base: &str, workspace_folder: Option<&Path>) -> Opt .map(|entry| entry.version) } -pub(super) fn catalog_entry_for_version( - base: &str, - version: &str, - workspace_folder: Option<&Path>, -) -> Option { - catalog_entries(base, workspace_folder)? - .into_iter() - .find(|entry| entry.version == version) -} - fn local_oci_layout_entries(base: &str, workspace_folder: Option<&Path>) -> Vec { let Some(layout_dir) = workspace_oci_layout_dir(base, workspace_folder) else { return Vec::new(); diff --git a/cmd/devcontainer/src/commands/configuration/features.rs b/cmd/devcontainer/src/commands/configuration/features.rs index e9cff28fc..20a7221eb 100644 --- a/cmd/devcontainer/src/commands/configuration/features.rs +++ b/cmd/devcontainer/src/commands/configuration/features.rs @@ -10,4 +10,5 @@ mod types; pub(crate) use install::{feature_installation_name, materialize_feature_installation}; pub(crate) use metadata::apply_feature_metadata; pub(crate) use resolve::resolve_feature_support; +pub(super) use resolve::resolve_feature_support_without_lockfile; pub(crate) use types::{FeatureInstallation, ResolvedFeatureSupport}; diff --git a/cmd/devcontainer/src/commands/configuration/features/resolve.rs b/cmd/devcontainer/src/commands/configuration/features/resolve.rs index 6a36677ef..ddc75899f 100644 --- a/cmd/devcontainer/src/commands/configuration/features/resolve.rs +++ b/cmd/devcontainer/src/commands/configuration/features/resolve.rs @@ -5,6 +5,7 @@ use std::collections::VecDeque; use std::path::{Path, PathBuf}; use serde_json::{json, Map, Value}; +use sha2::{Digest, Sha256}; use crate::commands::collections::oci; use crate::commands::collections::registry::{ @@ -13,12 +14,13 @@ use crate::commands::collections::registry::{ }; use crate::commands::common; +use super::super::{catalog::exact_catalog_entry, Lockfile}; use super::control::{ensure_no_disallowed_features, feature_advisories_for_oci_features}; use super::metadata::feature_metadata_entry; use super::options::{feature_object, feature_option_values_from_manifest, feature_options}; use super::types::{ FeatureInstallation, FeatureInstallationSource, FeatureRequest, FeatureSource, FeatureSpec, - ResolvedFeatureSummary, ResolvedFeatureSupport, + ResolvedFeatureSummary, ResolvedFeatureSupport, ResolvedLockfileFeature, }; #[derive(Clone)] @@ -40,6 +42,32 @@ pub(crate) fn resolve_feature_support( workspace_folder: &Path, config_file: &Path, configuration: &Value, +) -> Result, String> { + let lockfile = super::super::upgrade::lockfile_for_resolution(args, config_file)?; + resolve_feature_support_with_lockfile( + args, + workspace_folder, + config_file, + configuration, + lockfile.as_ref(), + ) +} + +pub(in crate::commands::configuration) fn resolve_feature_support_without_lockfile( + args: &[String], + workspace_folder: &Path, + config_file: &Path, + configuration: &Value, +) -> Result, String> { + resolve_feature_support_with_lockfile(args, workspace_folder, config_file, configuration, None) +} + +fn resolve_feature_support_with_lockfile( + args: &[String], + workspace_folder: &Path, + config_file: &Path, + configuration: &Value, + lockfile: Option<&Lockfile>, ) -> Result, String> { let declared = declared_features(args, configuration)?; if declared.is_empty() { @@ -55,8 +83,13 @@ pub(crate) fn resolve_feature_support( options: options.clone(), }) .collect::>(); - let graph = - build_dependency_graph(root_requests, configuration, config_root, workspace_folder)?; + let graph = build_dependency_graph( + root_requests, + configuration, + config_root, + workspace_folder, + lockfile, + )?; let ordered_nodes = compute_feature_install_order(graph)?; let mut feature_sets = Vec::new(); @@ -65,6 +98,7 @@ pub(crate) fn resolve_feature_support( let mut installations = Vec::new(); let mut ordered_features = Vec::new(); let mut ordered_feature_ids = Vec::new(); + let mut lockfile_features = Vec::new(); for node in ordered_nodes { let spec = node.spec; @@ -93,6 +127,9 @@ pub(crate) fn resolve_feature_support( id: spec.install_order_id.clone(), options: spec.value.clone(), }); + if let Some(lockfile_feature) = spec.lockfile_feature.clone() { + lockfile_features.push(lockfile_feature); + } installations.push(spec.installation); } let feature_advisories = feature_advisories_for_oci_features(args, &advisory_inputs)?; @@ -106,6 +143,7 @@ pub(crate) fn resolve_feature_support( installations, ordered_features, ordered_feature_ids, + lockfile_features, })) } @@ -132,12 +170,13 @@ fn build_dependency_graph( configuration: &Value, config_root: &Path, workspace_folder: &Path, + lockfile: Option<&Lockfile>, ) -> Result, String> { let mut worklist = VecDeque::from(root_requests); let mut resolved = Vec::new(); while let Some(request) = worklist.pop_front() { - let node = resolve_feature_node(&request, config_root, workspace_folder)?; + let node = resolve_feature_node(&request, config_root, workspace_folder, lockfile)?; if resolved.iter().any(|existing| nodes_equal(existing, &node)) { continue; } @@ -152,6 +191,7 @@ fn build_dependency_graph( configuration, config_root, workspace_folder, + lockfile, )?; Ok(resolved) } @@ -160,22 +200,28 @@ fn resolve_feature_node( request: &FeatureRequest, config_root: &Path, workspace_folder: &Path, + lockfile: Option<&Lockfile>, ) -> Result { let spec = resolve_feature_spec( &request.user_feature_id, &request.options, config_root, workspace_folder, + lockfile, )?; let depends_on = spec .depends_on .iter() - .map(|dependency| resolve_feature_dependency(dependency, config_root, workspace_folder)) + .map(|dependency| { + resolve_feature_dependency(dependency, config_root, workspace_folder, lockfile) + }) .collect::, _>>()?; let installs_after = spec .installs_after .iter() - .map(|dependency| resolve_feature_dependency(dependency, config_root, workspace_folder)) + .map(|dependency| { + resolve_feature_dependency(dependency, config_root, workspace_folder, lockfile) + }) .collect::, _>>()?; Ok(FeatureNode { @@ -190,12 +236,14 @@ fn resolve_feature_dependency( request: &FeatureRequest, config_root: &Path, workspace_folder: &Path, + lockfile: Option<&Lockfile>, ) -> Result { let spec = resolve_feature_spec( &request.user_feature_id, &request.options, config_root, workspace_folder, + lockfile, )?; Ok(FeatureDependency { request: request.clone(), @@ -208,6 +256,7 @@ fn apply_override_feature_install_order( configuration: &Value, config_root: &Path, workspace_folder: &Path, + lockfile: Option<&Lockfile>, ) -> Result<(), String> { let Some(overrides) = configuration .get("overrideFeatureInstallOrder") @@ -227,7 +276,8 @@ fn apply_override_feature_install_order( user_feature_id: override_id.to_string(), options: json!({}), }; - let dependency = resolve_feature_dependency(&request, config_root, workspace_folder)?; + let dependency = + resolve_feature_dependency(&request, config_root, workspace_folder, lockfile)?; for node in worklist.iter_mut() { if node_satisfies_soft_dependency(node, &dependency) { node.round_priority = node.round_priority.max(priority); @@ -456,6 +506,7 @@ fn resolve_feature_spec( value: &Value, config_root: &Path, workspace_folder: &Path, + lockfile: Option<&Lockfile>, ) -> Result { let (manifest, source_information, installation, source, install_order_id) = if is_local_feature_reference(feature_id) { @@ -538,7 +589,19 @@ fn resolve_feature_spec( feature_id.to_string(), ) } else { - let artifact = oci::resolve_feature_artifact(feature_id, Some(workspace_folder))?; + let locked_digest = lockfile + .and_then(|value| value.features.get(feature_id)) + .map(|entry| entry.integrity.as_str()) + .filter(|integrity| !integrity.is_empty()); + let artifact = if let Some(digest) = locked_digest { + oci::resolve_feature_artifact_with_digest( + feature_id, + digest, + Some(workspace_folder), + )? + } else { + oci::resolve_feature_artifact(feature_id, Some(workspace_folder))? + }; let manifest = artifact.metadata.clone(); let resource = artifact.resource.clone(); let tag = artifact.tag.clone(); @@ -569,6 +632,8 @@ fn resolve_feature_spec( ) }; + let lockfile_feature = + resolved_lockfile_feature(feature_id, &manifest, &source, workspace_folder)?; let options = feature_options(&manifest, value); let metadata_entry = feature_metadata_entry(&manifest); let aliases = feature_aliases(&manifest); @@ -588,9 +653,86 @@ fn resolve_feature_spec( aliases, depends_on, installs_after, + lockfile_feature, }) } +fn resolved_lockfile_feature( + feature_id: &str, + manifest: &Value, + source: &FeatureSource, + workspace_folder: &Path, +) -> Result, String> { + match source { + FeatureSource::Oci { + resource, + tag, + digest, + } => Ok(Some(ResolvedLockfileFeature { + user_feature_id: feature_id.to_string(), + version: manifest_version(manifest, tag.as_deref()), + resolved: format!("{resource}@{digest}"), + integrity: digest.clone(), + depends_on: manifest_depends_on_entries(manifest), + })), + FeatureSource::DirectTarball { uri } => { + if let Some(entry) = exact_catalog_entry(uri, Some(workspace_folder)) { + return Ok(Some(ResolvedLockfileFeature { + user_feature_id: feature_id.to_string(), + version: entry.version, + resolved: entry.resolved, + integrity: entry.integrity, + depends_on: entry.depends_on, + })); + } + Ok(Some(ResolvedLockfileFeature { + user_feature_id: feature_id.to_string(), + version: manifest_version(manifest, None), + resolved: uri.clone(), + integrity: synthetic_direct_tarball_integrity(uri, manifest)?, + depends_on: manifest_depends_on_entries(manifest), + })) + } + FeatureSource::Local { .. } | FeatureSource::GithubRepo { .. } => Ok(None), + } +} + +fn manifest_version(manifest: &Value, fallback: Option<&str>) -> String { + manifest + .get("version") + .and_then(Value::as_str) + .or(fallback) + .unwrap_or("latest") + .to_string() +} + +fn manifest_depends_on_entries(manifest: &Value) -> Option> { + let depends_on = manifest.get("dependsOn")?; + let entries = if let Some(object) = depends_on.as_object() { + object.keys().cloned().collect::>() + } else if let Some(array) = depends_on.as_array() { + array + .iter() + .filter_map(Value::as_str) + .map(str::to_string) + .collect::>() + } else { + Vec::new() + }; + (!entries.is_empty()).then_some(entries) +} + +fn synthetic_direct_tarball_integrity(uri: &str, manifest: &Value) -> Result { + let payload = json!({ + "uri": uri, + "manifest": manifest, + }); + let bytes = serde_json::to_vec(&payload).map_err(|error| error.to_string())?; + let mut hasher = Sha256::new(); + hasher.update(bytes); + Ok(format!("sha256:{:x}", hasher.finalize())) +} + fn feature_depends_on(manifest: &Value) -> Vec { manifest .get("dependsOn") diff --git a/cmd/devcontainer/src/commands/configuration/features/types.rs b/cmd/devcontainer/src/commands/configuration/features/types.rs index 6ffaf5935..456d3f62b 100644 --- a/cmd/devcontainer/src/commands/configuration/features/types.rs +++ b/cmd/devcontainer/src/commands/configuration/features/types.rs @@ -28,6 +28,7 @@ pub(crate) struct ResolvedFeatureSupport { pub(crate) installations: Vec, pub(crate) ordered_features: Vec, pub(crate) ordered_feature_ids: Vec, + pub(crate) lockfile_features: Vec, } #[derive(Clone)] @@ -44,6 +45,7 @@ pub(super) struct FeatureSpec { pub(super) aliases: Vec, pub(super) depends_on: Vec, pub(super) installs_after: Vec, + pub(super) lockfile_feature: Option, } #[derive(Clone, Debug)] @@ -52,6 +54,15 @@ pub(crate) struct ResolvedFeatureSummary { pub(crate) options: Value, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ResolvedLockfileFeature { + pub(crate) user_feature_id: String, + pub(crate) version: String, + pub(crate) resolved: String, + pub(crate) integrity: String, + pub(crate) depends_on: Option>, +} + #[derive(Clone, Debug)] pub(super) struct FeatureRequest { pub(super) user_feature_id: String, diff --git a/cmd/devcontainer/src/commands/configuration/mod.rs b/cmd/devcontainer/src/commands/configuration/mod.rs index 6d315c674..340082dc0 100644 --- a/cmd/devcontainer/src/commands/configuration/mod.rs +++ b/cmd/devcontainer/src/commands/configuration/mod.rs @@ -106,16 +106,18 @@ pub(crate) fn ensure_native_lockfile( args: &[String], config_file: &std::path::Path, configuration: &Value, + resolved_features: &features::ResolvedFeatureSupport, ) -> Result<(), String> { - upgrade::ensure_native_lockfile(args, config_file, configuration) + upgrade::ensure_native_lockfile(args, config_file, configuration, resolved_features) } pub(crate) fn validate_native_lockfile( args: &[String], config_file: &std::path::Path, configuration: &Value, + resolved_features: &features::ResolvedFeatureSupport, ) -> Result<(), String> { - upgrade::validate_native_lockfile(args, config_file, configuration) + upgrade::validate_native_lockfile(args, config_file, configuration, resolved_features) } pub(crate) fn validate_lockfile_options(args: &[String]) -> Result<(), String> { diff --git a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs index a6f8aad66..a10224a44 100644 --- a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs @@ -7,10 +7,10 @@ use serde_json::json; use sha2::{Digest, Sha256}; use super::support::unique_temp_dir; -use crate::commands::configuration::ensure_native_lockfile; use crate::commands::configuration::upgrade::{ build_outdated_payload, feature_id_without_version, lockfile_path, run_upgrade_lockfile, }; +use crate::commands::configuration::{ensure_native_lockfile, resolve_feature_support}; #[test] fn outdated_payload_reports_remote_feature_versions() { @@ -195,7 +195,7 @@ fn ensure_native_lockfile_uses_shared_lockfile_format() { fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); - ensure_native_lockfile( + ensure_native_lockfile_for_config( &["--workspace-folder".to_string(), root.display().to_string()], &config_file, &json!({ @@ -218,7 +218,7 @@ fn ensure_native_lockfile_skips_generation_when_no_lockfile_is_set() { fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); - ensure_native_lockfile( + ensure_native_lockfile_for_config( &[ "--workspace-folder".to_string(), root.display().to_string(), @@ -265,7 +265,7 @@ fn ensure_native_lockfile_rejects_corrupt_existing_lockfile_when_generating() { let lockfile_path = root.join(".devcontainer-lock.json"); fs::write(&lockfile_path, "this is not json").expect("corrupt lockfile"); - let error = ensure_native_lockfile( + let error = ensure_native_lockfile_for_config( &["--workspace-folder".to_string(), root.display().to_string()], &config_file, &json!({ @@ -291,7 +291,7 @@ fn ensure_native_lockfile_reports_missing_frozen_lockfile() { fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); - let error = ensure_native_lockfile( + let error = ensure_native_lockfile_for_config( &[ "--workspace-folder".to_string(), root.display().to_string(), @@ -316,7 +316,7 @@ fn ensure_native_lockfile_accepts_semantically_identical_existing_json() { let root = unique_temp_dir(); fs::create_dir_all(&root).expect("failed to create root"); let config_file = root.join(".devcontainer.json"); - ensure_native_lockfile( + ensure_native_lockfile_for_config( &["--workspace-folder".to_string(), root.display().to_string()], &config_file, &json!({ @@ -332,7 +332,7 @@ fn ensure_native_lockfile_accepts_semantically_identical_existing_json() { let reformatted = lockfile.trim_end_matches('\n').to_string(); fs::write(&lockfile_path, reformatted).expect("lockfile rewrite"); - ensure_native_lockfile( + ensure_native_lockfile_for_config( &[ "--workspace-folder".to_string(), root.display().to_string(), @@ -351,6 +351,18 @@ fn ensure_native_lockfile_accepts_semantically_identical_existing_json() { let _ = fs::remove_dir_all(root); } +fn ensure_native_lockfile_for_config( + args: &[String], + config_file: &Path, + configuration: &serde_json::Value, +) -> Result<(), String> { + let workspace_folder = config_file.parent().unwrap_or_else(|| Path::new(".")); + let resolved_features = + resolve_feature_support(args, workspace_folder, config_file, configuration)? + .expect("feature support"); + ensure_native_lockfile(args, config_file, configuration, &resolved_features) +} + fn write_workspace_layout_version( workspace_root: &Path, base: &str, diff --git a/cmd/devcontainer/src/commands/configuration/upgrade.rs b/cmd/devcontainer/src/commands/configuration/upgrade.rs index 30393745f..387ca474e 100644 --- a/cmd/devcontainer/src/commands/configuration/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/upgrade.rs @@ -1,18 +1,16 @@ //! Configuration upgrade, lockfile, and outdated command helpers. +use std::collections::BTreeSet; use std::fs; use std::path::{Path, PathBuf}; use std::process::ExitCode; use serde_json::{json, Map, Value}; -use super::catalog::{ - build_feature_version_info, catalog_entry_for_version, exact_catalog_entry, latest_version, - resolve_wanted_version, -}; +use super::catalog::build_feature_version_info; +use super::features::ResolvedFeatureSupport; use super::load::load_config; use super::{FeatureReference, Lockfile, LockfileEntry}; -use crate::commands::collections::oci; use crate::commands::common; use crate::output::{CommandLogLevel, CommandLogger, LogFormat, TerminalDimensions}; @@ -77,16 +75,14 @@ pub(super) fn ensure_native_lockfile( args: &[String], config_file: &Path, configuration: &Value, + resolved_features: &ResolvedFeatureSupport, ) -> Result<(), String> { validate_lockfile_options(args)?; if lockfile_disabled(args) { return Ok(()); } - let workspace_folder = common::parse_option_value(args, "--workspace-folder") - .map(PathBuf::from) - .or_else(|| config_file.parent().map(Path::to_path_buf)); - let generated = generate_lockfile(configuration, workspace_folder.as_deref())?; + let generated = generate_lockfile_from_resolved(args, configuration, resolved_features)?; let path = lockfile_path(config_file); let existing = existing_native_lockfile(args, &path)?; if lockfile_frozen(args) { @@ -110,6 +106,7 @@ pub(super) fn validate_native_lockfile( args: &[String], config_file: &Path, configuration: &Value, + resolved_features: &ResolvedFeatureSupport, ) -> Result<(), String> { validate_lockfile_options(args)?; if lockfile_disabled(args) { @@ -122,10 +119,7 @@ pub(super) fn validate_native_lockfile( let Some(existing) = existing else { return Err("Lockfile does not exist.".to_string()); }; - let workspace_folder = common::parse_option_value(args, "--workspace-folder") - .map(PathBuf::from) - .or_else(|| config_file.parent().map(Path::to_path_buf)); - let generated = generate_lockfile(configuration, workspace_folder.as_deref())?; + let generated = generate_lockfile_from_resolved(args, configuration, resolved_features)?; if existing != generated { return Err(format!( "Lockfile at {} is out of date for the current feature configuration", @@ -183,6 +177,17 @@ fn existing_native_lockfile(args: &[String], path: &Path) -> Result Result, String> { + validate_lockfile_options(args)?; + if lockfile_disabled(args) { + return Ok(None); + } + read_lockfile(lockfile_path(config_file)) +} + fn serialized_lockfile(lockfile: &Lockfile) -> Result { serde_json::to_string_pretty(lockfile) .map(|json| format!("{json}\n")) @@ -317,10 +322,19 @@ fn run_upgrade_lockfile_with_logger( "Generating lockfile for {feature_count} feature(s)" )); } - let generated = generate_lockfile( - &loaded.configuration, - Some(loaded.workspace_folder.as_path()), - )?; + let generated = if let Some(resolved_features) = + super::features::resolve_feature_support_without_lockfile( + args, + &loaded.workspace_folder, + &loaded.config_file, + &loaded.configuration, + )? { + generate_lockfile_from_resolved(args, &loaded.configuration, &resolved_features)? + } else { + Lockfile { + features: std::collections::BTreeMap::new(), + } + }; if !common::has_flag(args, "--dry-run") { let lockfile_path = lockfile_path(&loaded.config_file); if let Some(logger) = logger { @@ -406,120 +420,52 @@ fn validate_upgrade_options(args: &[String]) -> Result<(), String> { Ok(()) } -fn generate_lockfile( +fn generate_lockfile_from_resolved( + args: &[String], configuration: &Value, - workspace_folder: Option<&Path>, + resolved_features: &ResolvedFeatureSupport, ) -> Result { - let features = configuration - .get("features") - .and_then(Value::as_object) - .cloned() - .unwrap_or_default(); - + let excluded = additional_only_feature_ids(args, configuration)?; let mut resolved = std::collections::BTreeMap::new(); - for feature_id in features.keys() { - let Some(reference) = parse_feature_reference(feature_id) else { + for feature in &resolved_features.lockfile_features { + if excluded.contains(&feature.user_feature_id) { continue; - }; - - let (lockfile_key, entry) = generate_lockfile_entry(&reference, workspace_folder)? - .ok_or_else(|| { - format!("Unsupported feature for native lockfile generation: {feature_id}") - })?; - resolved.insert(lockfile_key, entry); - } - - Ok(Lockfile { features: resolved }) -} - -fn generate_lockfile_entry( - feature: &FeatureReference, - workspace_folder: Option<&Path>, -) -> Result, String> { - if oci::is_registry_qualified_reference(&feature.base) { - let artifact = oci::resolve_feature_artifact(&feature.original, workspace_folder)?; - return Ok(Some(( - feature.original.clone(), + } + resolved.insert( + feature.user_feature_id.clone(), LockfileEntry { - version: artifact - .metadata - .get("version") - .and_then(Value::as_str) - .or(artifact.tag.as_deref()) - .unwrap_or("latest") - .to_string(), - resolved: oci::canonical_feature_id(&artifact), - integrity: artifact.manifest_digest, - depends_on: feature_depends_on_entries(&artifact.metadata), + version: feature.version.clone(), + resolved: feature.resolved.clone(), + integrity: feature.integrity.clone(), + depends_on: feature.depends_on.clone(), }, - ))); - } - - if feature.digest.is_some() { - return Ok( - exact_catalog_entry(&feature.original, workspace_folder).map(|entry| { - ( - feature.original.clone(), - LockfileEntry { - version: entry.version.clone(), - resolved: entry.resolved.clone(), - integrity: entry.integrity.clone(), - depends_on: entry.depends_on.clone(), - }, - ) - }), ); } - let version = if let Some(tag) = feature.tag.as_deref() { - if tag == "latest" { - let Some(version) = latest_version(&feature.base, workspace_folder) else { - return Ok(None); - }; - version - } else if tag.matches('.').count() == 2 { - tag.to_string() - } else { - let Some(version) = resolve_wanted_version(feature, None, workspace_folder) else { - return Ok(None); - }; - version - } - } else { - let Some(version) = latest_version(&feature.base, workspace_folder) else { - return Ok(None); - }; - version - }; + Ok(Lockfile { features: resolved }) +} - let Some(entry) = catalog_entry_for_version(&feature.base, &version, workspace_folder) else { - return Ok(None); - }; - Ok(Some(( - feature.original.clone(), - LockfileEntry { - version, - resolved: entry.resolved.clone(), - integrity: entry.integrity.clone(), - depends_on: entry.depends_on.clone(), - }, - ))) -} - -fn feature_depends_on_entries(metadata: &Value) -> Option> { - let depends_on = metadata.get("dependsOn")?; - let entries = if let Some(object) = depends_on.as_object() { - object.keys().cloned().collect::>() - } else if let Some(array) = depends_on.as_array() { - array - .iter() - .filter_map(Value::as_str) - .map(str::to_string) - .collect::>() - } else { - Vec::new() +fn additional_only_feature_ids( + args: &[String], + configuration: &Value, +) -> Result, String> { + let config_feature_keys = configuration + .get("features") + .and_then(Value::as_object) + .map(|features| features.keys().cloned().collect::>()) + .unwrap_or_default(); + let Some(raw_additional) = common::parse_option_value(args, "--additional-features") else { + return Ok(BTreeSet::new()); }; - (!entries.is_empty()).then_some(entries) + let additional = crate::config::parse_jsonc_value(&raw_additional)?; + let additional = additional + .as_object() + .ok_or_else(|| "--additional-features must be a JSON object".to_string())?; + Ok(additional + .keys() + .filter(|key| !config_feature_keys.contains(*key)) + .cloned() + .collect()) } pub(super) fn lockfile_path(config_file: &Path) -> PathBuf { diff --git a/cmd/devcontainer/src/runtime/build.rs b/cmd/devcontainer/src/runtime/build.rs index 411f12f2a..3009f9dfc 100644 --- a/cmd/devcontainer/src/runtime/build.rs +++ b/cmd/devcontainer/src/runtime/build.rs @@ -64,6 +64,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< args, &resolved.config_file, &resolved.configuration, + &feature_support, )?; let image_name = common::parse_option_value(args, "--image-name") .unwrap_or_else(|| default_image_name(&resolved.workspace_folder)); @@ -74,6 +75,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< args, &resolved.config_file, &resolved.configuration, + &feature_support, )?; Ok(built) } else { @@ -88,6 +90,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< args, &resolved.config_file, &resolved.configuration, + &feature_support, )?; let base_image = format!("{image_name}-base"); build_base_image(resolved, args, &base_image)?; @@ -102,6 +105,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< args, &resolved.config_file, &resolved.configuration, + &feature_support, )?; return Ok(built); } diff --git a/cmd/devcontainer/src/runtime/compose/mod.rs b/cmd/devcontainer/src/runtime/compose/mod.rs index 0d24826a5..e6403f5b1 100644 --- a/cmd/devcontainer/src/runtime/compose/mod.rs +++ b/cmd/devcontainer/src/runtime/compose/mod.rs @@ -79,11 +79,12 @@ pub(crate) fn build_service(resolved: &ResolvedConfig, args: &[String]) -> Resul &resolved.config_file, &resolved.configuration, )?; - if feature_support.is_some() { + if let Some(feature_support) = &feature_support { configuration::validate_native_lockfile( args, &resolved.config_file, &resolved.configuration, + feature_support, )?; } @@ -131,6 +132,7 @@ pub(crate) fn build_service(resolved: &ResolvedConfig, args: &[String]) -> Resul args, &resolved.config_file, &resolved.configuration, + &feature_support, )?; return Ok(built_image); } diff --git a/cmd/devcontainer/tests/runtime_build_smoke/features.rs b/cmd/devcontainer/tests/runtime_build_smoke/features.rs index fcd7bf086..42aca7095 100644 --- a/cmd/devcontainer/tests/runtime_build_smoke/features.rs +++ b/cmd/devcontainer/tests/runtime_build_smoke/features.rs @@ -4,6 +4,8 @@ use std::fs; use std::path::Path; use std::process::Command; +use serde_json::Value; + use crate::support::runtime_harness::{write_devcontainer_config, RuntimeHarness}; #[test] @@ -100,6 +102,183 @@ fn build_materializes_workspace_oci_feature_layout() { assert!(dockerfiles.contains("jq")); } +#[test] +fn build_writes_lockfile_for_non_ghcr_workspace_oci_feature() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + let feature_root = harness.root.join("published-feature-src"); + let layout_root = workspace + .join(".devcontainer") + .join("oci-layouts") + .join("example.com") + .join("acme") + .join("features") + .join("offline-feature"); + fs::create_dir_all(feature_root.join("repo")).expect("feature repo dir"); + fs::write( + feature_root.join("devcontainer-feature.json"), + "{\n \"id\": \"offline-feature\",\n \"name\": \"Offline Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("feature manifest"); + fs::write(feature_root.join("install.sh"), "#!/bin/sh\nset -eu\n").expect("install script"); + publish_feature_layout(&feature_root, &layout_root); + write_devcontainer_config( + &workspace, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"example.com/acme/features/offline-feature:1.0.0\": {}\n }\n}\n", + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--image-name", + "example/native-build:non-ghcr-lockfile", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let lockfile: Value = serde_json::from_str( + &fs::read_to_string( + workspace + .join(".devcontainer") + .join("devcontainer-lock.json"), + ) + .expect("lockfile"), + ) + .expect("lockfile json"); + let entry = &lockfile["features"]["example.com/acme/features/offline-feature:1.0.0"]; + assert_eq!(entry["version"], "1.0.0"); + assert!( + entry["resolved"].as_str().is_some_and( + |value| value.starts_with("example.com/acme/features/offline-feature@sha256:") + ), + "{entry:?}" + ); +} + +#[test] +fn build_uses_existing_lockfile_for_broad_oci_selector() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + let feature_root = harness.root.join("published-feature-src"); + let layout_root = workspace + .join(".devcontainer") + .join("oci-layouts") + .join("ghcr.io") + .join("acme") + .join("features") + .join("pinned-feature"); + fs::create_dir_all(&feature_root).expect("feature dir"); + fs::write( + feature_root.join("devcontainer-feature.json"), + "{\n \"id\": \"pinned-feature\",\n \"name\": \"Pinned Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("feature manifest"); + fs::write(feature_root.join("install.sh"), "#!/bin/sh\nset -eu\n").expect("install script"); + publish_feature_layout(&feature_root, &layout_root); + let pinned_digest = layout_digest_for_tag(&layout_root, "1.0.0"); + fs::write( + feature_root.join("devcontainer-feature.json"), + "{\n \"id\": \"pinned-feature\",\n \"name\": \"Pinned Feature\",\n \"version\": \"1.1.0\"\n}\n", + ) + .expect("feature manifest"); + publish_feature_layout(&feature_root, &layout_root); + let latest_digest = layout_digest_for_tag(&layout_root, "1.1.0"); + assert_ne!(pinned_digest, latest_digest); + write_devcontainer_config( + &workspace, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"ghcr.io/acme/features/pinned-feature:1\": {}\n }\n}\n", + ); + fs::write( + workspace.join(".devcontainer").join("devcontainer-lock.json"), + format!( + "{{\n \"features\": {{\n \"ghcr.io/acme/features/pinned-feature:1\": {{\n \"version\": \"1.0.0\",\n \"resolved\": \"ghcr.io/acme/features/pinned-feature@{pinned_digest}\",\n \"integrity\": \"{pinned_digest}\"\n }}\n }}\n}}\n" + ), + ) + .expect("lockfile"); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--image-name", + "example/native-build:pinned-lockfile", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let lockfile: Value = serde_json::from_str( + &fs::read_to_string( + workspace + .join(".devcontainer") + .join("devcontainer-lock.json"), + ) + .expect("lockfile"), + ) + .expect("lockfile json"); + let entry = &lockfile["features"]["ghcr.io/acme/features/pinned-feature:1"]; + assert_eq!(entry["version"], "1.0.0"); + assert_eq!(entry["integrity"], pinned_digest); +} + +#[test] +fn build_writes_lockfile_for_direct_tarball_feature() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + let feature_uri = "https://github.com/codspace/tgz-features-with-dependson/releases/download/0.0.2/devcontainer-feature-B.tgz"; + write_devcontainer_config( + &workspace, + &format!( + "{{\n \"image\": \"debian:bookworm\",\n \"features\": {{\n \"{feature_uri}\": {{}}\n }}\n}}\n" + ), + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "build", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--image-name", + "example/native-build:tarball-lockfile", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let lockfile: Value = serde_json::from_str( + &fs::read_to_string( + workspace + .join(".devcontainer") + .join("devcontainer-lock.json"), + ) + .expect("lockfile"), + ) + .expect("lockfile json"); + let entry = &lockfile["features"][feature_uri]; + assert_eq!(entry["version"], "0.0.2"); + assert_eq!(entry["resolved"], feature_uri); + assert!( + entry["integrity"] + .as_str() + .is_some_and(|value| value.starts_with("sha256:")), + "{entry:?}" + ); +} + #[test] fn feature_build_includes_syntax_directive_by_default() { let harness = RuntimeHarness::new(); @@ -605,3 +784,20 @@ fn publish_feature_layout(feature_root: &Path, layout_root: &Path) { assert!(output.status.success(), "{output:?}"); } + +fn layout_digest_for_tag(layout_root: &Path, tag: &str) -> String { + let index: Value = serde_json::from_str( + &fs::read_to_string(layout_root.join("index.json")).expect("index json"), + ) + .expect("index"); + index["manifests"] + .as_array() + .expect("manifests") + .iter() + .find_map(|entry| { + (entry["annotations"]["org.opencontainers.image.ref.name"].as_str() == Some(tag)) + .then(|| entry["digest"].as_str().map(str::to_string)) + .flatten() + }) + .expect("tag digest") +} diff --git a/docs/upstream/parity-inventory.json b/docs/upstream/parity-inventory.json index c4f58b3b8..802de7dd5 100644 --- a/docs/upstream/parity-inventory.json +++ b/docs/upstream/parity-inventory.json @@ -25,7 +25,8 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/resolve.rs", - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { @@ -574,7 +575,8 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/resolve.rs", - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { @@ -1018,7 +1020,8 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/resolve.rs", - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { From 823abcfb3bfc69719df77d6289acc168b566834d Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Thu, 14 May 2026 09:09:54 +0200 Subject: [PATCH 6/8] Fix feature lockfile resolution gaps --- .../src/commands/configuration/catalog.rs | 12 +++ .../configuration/features/resolve.rs | 77 ++++++++++++++++--- .../src/commands/configuration/read.rs | 3 +- .../src/commands/configuration/tests/read.rs | 32 ++++++++ .../commands/configuration/tests/upgrade.rs | 63 +++++++++++++++ .../tests/runtime_build_smoke/features.rs | 3 +- 6 files changed, 177 insertions(+), 13 deletions(-) diff --git a/cmd/devcontainer/src/commands/configuration/catalog.rs b/cmd/devcontainer/src/commands/configuration/catalog.rs index aab8dff15..9cb2f0805 100644 --- a/cmd/devcontainer/src/commands/configuration/catalog.rs +++ b/cmd/devcontainer/src/commands/configuration/catalog.rs @@ -453,6 +453,18 @@ fn fixture_catalog() -> Vec<(String, CatalogEntry)> { depends_on: Some(vec!["ghcr.io/codspace/dependson/E".to_string()]), }, ), + ( + "https://github.com/codspace/tgz-features-with-dependson/releases/download/0.0.2/devcontainer-feature-B.tgz".to_string(), + CatalogEntry { + version: "0.0.2".to_string(), + resolved: "https://github.com/codspace/tgz-features-with-dependson/releases/download/0.0.2/devcontainer-feature-B.tgz".to_string(), + integrity: "sha256:d130123ba54335a026ab6cd51c8bcbd52d58a0aeaacd8a593512ba61c5117ea0".to_string(), + depends_on: Some(vec![ + "ghcr.io/codspace/dependson/C".to_string(), + "ghcr.io/codspace/dependson/D".to_string(), + ]), + }, + ), ] } diff --git a/cmd/devcontainer/src/commands/configuration/features/resolve.rs b/cmd/devcontainer/src/commands/configuration/features/resolve.rs index ddc75899f..aed876d12 100644 --- a/cmd/devcontainer/src/commands/configuration/features/resolve.rs +++ b/cmd/devcontainer/src/commands/configuration/features/resolve.rs @@ -1,8 +1,12 @@ //! Feature declaration parsing, dependency ordering, and source resolution helpers. use std::cmp::Ordering; -use std::collections::VecDeque; +use std::collections::{HashMap, VecDeque}; +use std::env; +use std::fs; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering as AtomicOrdering}; +use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::{json, Map, Value}; use sha2::{Digest, Sha256}; @@ -13,6 +17,7 @@ use crate::commands::collections::registry::{ normalize_collection_reference, published_feature_manifest, }; use crate::commands::common; +use crate::process_runner::{self, ProcessLogLevel, ProcessRequest}; use super::super::{catalog::exact_catalog_entry, Lockfile}; use super::control::{ensure_no_disallowed_features, feature_advisories_for_oci_features}; @@ -689,7 +694,7 @@ fn resolved_lockfile_feature( user_feature_id: feature_id.to_string(), version: manifest_version(manifest, None), resolved: uri.clone(), - integrity: synthetic_direct_tarball_integrity(uri, manifest)?, + integrity: direct_tarball_archive_integrity(uri)?, depends_on: manifest_depends_on_entries(manifest), })) } @@ -722,15 +727,69 @@ fn manifest_depends_on_entries(manifest: &Value) -> Option> { (!entries.is_empty()).then_some(entries) } -fn synthetic_direct_tarball_integrity(uri: &str, manifest: &Value) -> Result { - let payload = json!({ - "uri": uri, - "manifest": manifest, - }); - let bytes = serde_json::to_vec(&payload).map_err(|error| error.to_string())?; +fn direct_tarball_archive_integrity(uri: &str) -> Result { + let temp = TempDownloadedTarball::new(); + let result = process_runner::run_process(&ProcessRequest { + program: "curl".to_string(), + args: vec![ + "-fsSL".to_string(), + "--max-time".to_string(), + "30".to_string(), + "-o".to_string(), + temp.path.display().to_string(), + uri.to_string(), + ], + cwd: None, + env: HashMap::new(), + log_level: ProcessLogLevel::Info, + }) + .map_err(|error| error.to_string())?; + if result.status_code != 0 { + let stderr = result.stderr.trim(); + return if stderr.is_empty() { + Err(format!( + "Failed to fetch direct tarball {uri}: curl exited with status {}", + result.status_code + )) + } else { + Err(format!("Failed to fetch direct tarball {uri}: {stderr}")) + }; + } + let bytes = fs::read(&temp.path).map_err(|error| error.to_string())?; + Ok(sha256_integrity(&bytes)) +} + +fn sha256_integrity(bytes: &[u8]) -> String { let mut hasher = Sha256::new(); hasher.update(bytes); - Ok(format!("sha256:{:x}", hasher.finalize())) + format!("sha256:{:x}", hasher.finalize()) +} + +struct TempDownloadedTarball { + path: PathBuf, +} + +impl TempDownloadedTarball { + fn new() -> Self { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let nonce = COUNTER.fetch_add(1, AtomicOrdering::Relaxed); + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|duration| duration.as_nanos()) + .unwrap_or_default(); + Self { + path: env::temp_dir().join(format!( + "devcontainer-direct-tarball-{}-{now}-{nonce}.tgz", + std::process::id() + )), + } + } +} + +impl Drop for TempDownloadedTarball { + fn drop(&mut self) { + let _ = fs::remove_file(&self.path); + } } fn feature_depends_on(manifest: &Value) -> Vec { diff --git a/cmd/devcontainer/src/commands/configuration/read.rs b/cmd/devcontainer/src/commands/configuration/read.rs index 95d41eff6..319281b89 100644 --- a/cmd/devcontainer/src/commands/configuration/read.rs +++ b/cmd/devcontainer/src/commands/configuration/read.rs @@ -4,7 +4,6 @@ use serde_json::{json, Map, Value}; use super::inspect::{merged_configuration_payload, read_configuration_value, workspace_payload}; use super::load::load_optional_config; -use super::resolve_feature_support; use crate::commands::common; pub(super) fn build_read_configuration_payload(args: &[String]) -> Result { @@ -27,7 +26,7 @@ pub(super) fn build_read_configuration_payload(args: &[String]) -> Result String { hasher.update(bytes); format!("{:x}", hasher.finalize()) } + +struct SingleResponseHttpServer { + base_url: String, +} + +impl SingleResponseHttpServer { + fn new(body: &[u8]) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("http listener"); + let address = listener.local_addr().expect("http listener address"); + let body = body.to_vec(); + thread::spawn(move || { + let Ok((mut stream, _)) = listener.accept() else { + return; + }; + let mut request = [0_u8; 1024]; + let _ = stream.read(&mut request); + let headers = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = stream.write_all(headers.as_bytes()); + let _ = stream.write_all(&body); + }); + Self { + base_url: format!("http://{address}"), + } + } + + fn url(&self, path: &str) -> String { + format!("{}/{}", self.base_url, path) + } +} diff --git a/cmd/devcontainer/tests/runtime_build_smoke/features.rs b/cmd/devcontainer/tests/runtime_build_smoke/features.rs index 42aca7095..f351340e0 100644 --- a/cmd/devcontainer/tests/runtime_build_smoke/features.rs +++ b/cmd/devcontainer/tests/runtime_build_smoke/features.rs @@ -273,8 +273,7 @@ fn build_writes_lockfile_for_direct_tarball_feature() { assert_eq!(entry["resolved"], feature_uri); assert!( entry["integrity"] - .as_str() - .is_some_and(|value| value.starts_with("sha256:")), + == "sha256:d130123ba54335a026ab6cd51c8bcbd52d58a0aeaacd8a593512ba61c5117ea0", "{entry:?}" ); } From 55d444f6d61e054e1265075f265989d0c8989cbf Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Thu, 14 May 2026 09:37:58 +0200 Subject: [PATCH 7/8] Fix lockfile handling for direct tarballs --- .../src/commands/collections/features.rs | 2 +- .../commands/collections/tests/features.rs | 37 +++++++++++++ .../configuration/features/resolve.rs | 43 ++++++++++++--- .../src/commands/configuration/mod.rs | 14 +++++ .../commands/configuration/tests/upgrade.rs | 39 ++++++++++++++ cmd/devcontainer/src/runtime/context.rs | 52 +++++++++++++++++-- 6 files changed, 176 insertions(+), 11 deletions(-) diff --git a/cmd/devcontainer/src/commands/collections/features.rs b/cmd/devcontainer/src/commands/collections/features.rs index ef1600021..0f14f11c6 100644 --- a/cmd/devcontainer/src/commands/collections/features.rs +++ b/cmd/devcontainer/src/commands/collections/features.rs @@ -12,7 +12,7 @@ pub(super) fn build_features_resolve_dependencies_payload( args: &[String], ) -> Result { let (workspace_folder, config_file, configuration) = common::load_resolved_config(args)?; - let ordered = crate::commands::configuration::resolve_feature_support( + let ordered = crate::commands::configuration::resolve_feature_support_without_lockfile( args, &workspace_folder, &config_file, diff --git a/cmd/devcontainer/src/commands/collections/tests/features.rs b/cmd/devcontainer/src/commands/collections/tests/features.rs index 7fd7bdabf..a18cba55d 100644 --- a/cmd/devcontainer/src/commands/collections/tests/features.rs +++ b/cmd/devcontainer/src/commands/collections/tests/features.rs @@ -63,6 +63,43 @@ fn feature_dependency_resolution_respects_override_order() { let _ = fs::remove_dir_all(root); } +#[test] +fn feature_dependency_resolution_ignores_existing_lockfile() { + let root = unique_temp_dir(); + let config_dir = root.join(".devcontainer"); + let feature_dir = config_dir.join("local-feature"); + fs::create_dir_all(&feature_dir).expect("failed to create feature directory"); + fs::write( + feature_dir.join("devcontainer-feature.json"), + "{\n \"id\": \"local-feature\",\n \"name\": \"Local Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("failed to write feature manifest"); + fs::write(feature_dir.join("install.sh"), "#!/bin/sh\nset -eu\n") + .expect("failed to write feature install script"); + fs::write( + config_dir.join("devcontainer.json"), + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"./local-feature\": {}\n }\n}\n", + ) + .expect("failed to write config"); + fs::write( + config_dir.join("devcontainer-lock.json"), + "this is not json", + ) + .expect("failed to write corrupt lockfile"); + + let payload = build_features_resolve_dependencies_payload(&[ + "--workspace-folder".to_string(), + root.display().to_string(), + ]) + .expect("payload should not read lockfile"); + + let features = payload["resolvedFeatures"] + .as_array() + .expect("resolved features"); + assert_eq!(features, &[serde_json::json!("./local-feature")]); + let _ = fs::remove_dir_all(root); +} + #[test] fn feature_dependency_resolution_matches_upstream_local_option_round_order() { let root = copy_upstream_fixture("feature-dependencies/dependsOn/local-with-options"); diff --git a/cmd/devcontainer/src/commands/configuration/features/resolve.rs b/cmd/devcontainer/src/commands/configuration/features/resolve.rs index aed876d12..c64797b43 100644 --- a/cmd/devcontainer/src/commands/configuration/features/resolve.rs +++ b/cmd/devcontainer/src/commands/configuration/features/resolve.rs @@ -19,7 +19,7 @@ use crate::commands::collections::registry::{ use crate::commands::common; use crate::process_runner::{self, ProcessLogLevel, ProcessRequest}; -use super::super::{catalog::exact_catalog_entry, Lockfile}; +use super::super::{catalog::exact_catalog_entry, Lockfile, LockfileEntry}; use super::control::{ensure_no_disallowed_features, feature_advisories_for_oci_features}; use super::metadata::feature_metadata_entry; use super::options::{feature_object, feature_option_values_from_manifest, feature_options}; @@ -513,6 +513,7 @@ fn resolve_feature_spec( workspace_folder: &Path, lockfile: Option<&Lockfile>, ) -> Result { + let locked_entry = lockfile.and_then(|value| value.features.get(feature_id)); let (manifest, source_information, installation, source, install_order_id) = if is_local_feature_reference(feature_id) { let feature_dir = resolve_local_feature_path(config_root, feature_id); @@ -594,8 +595,7 @@ fn resolve_feature_spec( feature_id.to_string(), ) } else { - let locked_digest = lockfile - .and_then(|value| value.features.get(feature_id)) + let locked_digest = locked_entry .map(|entry| entry.integrity.as_str()) .filter(|integrity| !integrity.is_empty()); let artifact = if let Some(digest) = locked_digest { @@ -637,8 +637,13 @@ fn resolve_feature_spec( ) }; - let lockfile_feature = - resolved_lockfile_feature(feature_id, &manifest, &source, workspace_folder)?; + let lockfile_feature = resolved_lockfile_feature( + feature_id, + &manifest, + &source, + workspace_folder, + locked_entry, + )?; let options = feature_options(&manifest, value); let metadata_entry = feature_metadata_entry(&manifest); let aliases = feature_aliases(&manifest); @@ -667,6 +672,7 @@ fn resolved_lockfile_feature( manifest: &Value, source: &FeatureSource, workspace_folder: &Path, + locked_entry: Option<&LockfileEntry>, ) -> Result, String> { match source { FeatureSource::Oci { @@ -681,12 +687,16 @@ fn resolved_lockfile_feature( depends_on: manifest_depends_on_entries(manifest), })), FeatureSource::DirectTarball { uri } => { + let verified_integrity = locked_entry + .map(|entry| verify_direct_tarball_lockfile_integrity(uri, entry)) + .transpose()? + .flatten(); if let Some(entry) = exact_catalog_entry(uri, Some(workspace_folder)) { return Ok(Some(ResolvedLockfileFeature { user_feature_id: feature_id.to_string(), version: entry.version, resolved: entry.resolved, - integrity: entry.integrity, + integrity: verified_integrity.unwrap_or(entry.integrity), depends_on: entry.depends_on, })); } @@ -694,7 +704,9 @@ fn resolved_lockfile_feature( user_feature_id: feature_id.to_string(), version: manifest_version(manifest, None), resolved: uri.clone(), - integrity: direct_tarball_archive_integrity(uri)?, + integrity: verified_integrity + .map(Ok) + .unwrap_or_else(|| direct_tarball_archive_integrity(uri))?, depends_on: manifest_depends_on_entries(manifest), })) } @@ -702,6 +714,23 @@ fn resolved_lockfile_feature( } } +fn verify_direct_tarball_lockfile_integrity( + uri: &str, + locked_entry: &LockfileEntry, +) -> Result, String> { + if locked_entry.integrity.is_empty() { + return Ok(None); + } + let actual_integrity = direct_tarball_archive_integrity(uri)?; + if actual_integrity == locked_entry.integrity { + return Ok(Some(actual_integrity)); + } + Err(format!( + "Digest did not match for {uri}. Expected {}, got {actual_integrity}.", + locked_entry.integrity + )) +} + fn manifest_version(manifest: &Value, fallback: Option<&str>) -> String { manifest .get("version") diff --git a/cmd/devcontainer/src/commands/configuration/mod.rs b/cmd/devcontainer/src/commands/configuration/mod.rs index 340082dc0..0930d2b59 100644 --- a/cmd/devcontainer/src/commands/configuration/mod.rs +++ b/cmd/devcontainer/src/commands/configuration/mod.rs @@ -79,6 +79,20 @@ pub(crate) fn resolve_feature_support( features::resolve_feature_support(args, workspace_folder, config_file, configuration) } +pub(crate) fn resolve_feature_support_without_lockfile( + args: &[String], + workspace_folder: &std::path::Path, + config_file: &std::path::Path, + configuration: &Value, +) -> Result, String> { + features::resolve_feature_support_without_lockfile( + args, + workspace_folder, + config_file, + configuration, + ) +} + pub(crate) fn materialize_feature_installation( installation: &features::FeatureInstallation, destination: &std::path::Path, diff --git a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs index 0a284f5bd..fc77fe253 100644 --- a/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/tests/upgrade.rs @@ -100,6 +100,45 @@ fn upgrade_lockfile_records_direct_tarball_archive_digest() { let _ = fs::remove_dir_all(root); } +#[test] +fn ensure_native_lockfile_rejects_changed_direct_tarball_archive() { + let root = unique_temp_dir(); + fs::create_dir_all(&root).expect("failed to create root"); + let old_tarball_bytes = b"original direct tarball archive bytes"; + let changed_tarball_bytes = b"changed direct tarball archive bytes"; + let server = SingleResponseHttpServer::new(changed_tarball_bytes); + let feature_uri = server.url("devcontainer-feature-network.tgz"); + let configuration = json!({ + "image": "debian:bookworm", + "features": { + feature_uri.clone(): {}, + }, + }); + let config_file = root.join(".devcontainer.json"); + fs::write( + &config_file, + serde_json::to_string_pretty(&configuration).expect("config json"), + ) + .expect("failed to write config"); + fs::write( + root.join(".devcontainer-lock.json"), + format!( + "{{\n \"features\": {{\n \"{feature_uri}\": {{\n \"version\": \"latest\",\n \"resolved\": \"{feature_uri}\",\n \"integrity\": \"sha256:{}\"\n }}\n }}\n}}\n", + sha256_digest(old_tarball_bytes) + ), + ) + .expect("failed to write lockfile"); + + let error = ensure_native_lockfile_for_config(&[], &config_file, &configuration) + .expect_err("changed direct tarball should fail integrity verification"); + + assert!(error.contains("Digest did not match"), "{error}"); + let lockfile = fs::read_to_string(root.join(".devcontainer-lock.json")).expect("lockfile"); + assert!(lockfile.contains(&sha256_digest(old_tarball_bytes))); + assert!(!lockfile.contains(&sha256_digest(changed_tarball_bytes))); + let _ = fs::remove_dir_all(root); +} + #[test] fn feature_id_without_version_handles_tags_and_digests() { assert_eq!( diff --git a/cmd/devcontainer/src/runtime/context.rs b/cmd/devcontainer/src/runtime/context.rs index 520a96d51..add6acbb9 100644 --- a/cmd/devcontainer/src/runtime/context.rs +++ b/cmd/devcontainer/src/runtime/context.rs @@ -154,7 +154,7 @@ fn configuration_with_feature_metadata( args: &[String], resolved: &ResolvedConfig, ) -> Result { - let feature_support = configuration::resolve_feature_support( + let feature_support = configuration::resolve_feature_support_without_lockfile( args, &resolved.workspace_folder, &resolved.config_file, @@ -184,10 +184,56 @@ mod tests { use crate::test_support::unique_temp_dir; use super::{ - default_remote_workspace_folder, derived_workspace_mount, remote_workspace_folder_for_args, - workspace_mount_for_args, ResolvedConfig, + configuration_with_feature_metadata, default_remote_workspace_folder, + derived_workspace_mount, remote_workspace_folder_for_args, workspace_mount_for_args, + ResolvedConfig, }; + #[test] + fn existing_container_feature_metadata_ignores_corrupt_lockfile() { + let root = unique_temp_dir("devcontainer-runtime-context"); + let config_dir = root.join(".devcontainer"); + let feature_dir = config_dir.join("local-feature"); + fs::create_dir_all(&feature_dir).expect("failed to create feature directory"); + fs::write( + feature_dir.join("devcontainer-feature.json"), + "{\n \"id\": \"local-feature\",\n \"name\": \"Local Feature\",\n \"version\": \"1.0.0\",\n \"containerEnv\": {\n \"LOCAL_FEATURE_ENV\": \"enabled\"\n }\n}\n", + ) + .expect("failed to write feature manifest"); + fs::write(feature_dir.join("install.sh"), "#!/bin/sh\nset -eu\n") + .expect("failed to write feature install script"); + let config_file = config_dir.join("devcontainer.json"); + fs::write( + &config_file, + "{\n \"image\": \"debian:bookworm\",\n \"features\": {\n \"./local-feature\": {}\n }\n}\n", + ) + .expect("failed to write config"); + fs::write( + config_dir.join("devcontainer-lock.json"), + "this is not json", + ) + .expect("failed to write corrupt lockfile"); + let resolved = ResolvedConfig { + workspace_folder: root.clone(), + config_file, + configuration: json!({ + "image": "debian:bookworm", + "features": { + "./local-feature": {}, + }, + }), + }; + + let configuration = configuration_with_feature_metadata(&[], &resolved) + .expect("existing container metadata should not parse lockfile"); + + assert_eq!( + configuration["containerEnv"]["LOCAL_FEATURE_ENV"], + "enabled" + ); + let _ = fs::remove_dir_all(root); + } + #[test] fn remote_workspace_folder_prefers_configured_workspace_folder() { let resolved = ResolvedConfig { From 6ffa8a8a10b80b8229fc8f8287582dcd8a7f616f Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Thu, 14 May 2026 09:43:20 +0200 Subject: [PATCH 8/8] review instructions --- AGENTS.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index b912857c0..650fee4bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,3 +44,9 @@ When asked to update upstream: - Aggregated schema view: `spec/schemas/devContainer.schema.json`. - Feature schema: `spec/schemas/devContainerFeature.schema.json`. - Normative config behavior reference: `spec/docs/specs/devcontainerjson-reference.md`. + +## Reviewing +- Reviews are by the current feature branch vs main unless told otherwise +- A common problem is that features are not implemented fully. Always call out if the code is skipping functionality, stubbing things out, or only implementing particular paths +- Tests should verify actual behaviour. Call out superficial tests that use too many mocks, or boil down to just asserting that functions are called, or that a variable has a particular name in a source file, etc +- Always check that new functionality tracks upstream/ and spec/ - the solution in our code should be a superset of upstream. Extensions are fine, but missing parity should be brought up