From 1ccb98b0b5bd2845a6ac088fb4d86c90c6cc3ee4 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Mon, 25 May 2026 12:39:08 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20lockfile=20graduation=20=E2=80=94?= =?UTF-8?q?=20schema=20parity=20+=20CLI=20surface=20(PR-4a)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of lockfile graduation per docs/ROADMAP_TO_1.0.md. Ships the schema parity fixes, the user-visible CLI flag surface, the writer helper, and upstream-aligned error messages. The actual writer integration (building a Lockfile from resolved features and writing it after `up`/`build`) is tracked as PR-4b at issue #32. ## Schema parity (deacon-core/lockfile.rs) - LockfileFeature.depends_on now serializes as `dependsOn` (camelCase) via #[serde(rename)], matching upstream `devcontainers/cli`'s generateLockfile in src/spec-configuration/lockfile.ts. Deserializer accepts the camelCase form on read. - write_lockfile() emits a trailing newline to match upstream `JSON.stringify(..., 2) + '\n'`. Byte-identical output keeps the --frozen-lockfile content comparison stable. - LockfileValidationResult::format_error() now emits upstream-aligned strings: "Lockfile does not exist." / "Lockfile does not match." as the leading summary line; trailing actionable guidance references the graduated --frozen-lockfile flag rather than the deprecated --experimental-frozen-lockfile. - New LockfileFeature::from_resolved() helper constructs entries in the upstream canonical form (resolved = "{registry}/{repo}@{digest}", integrity = "{digest}"). This is the writer entry point PR-4b uses. ## CLI surface graduation up + build both gain: - --no-lockfile (visible): skip lockfile generation and verification. - --frozen-lockfile (visible): require an up-to-date lockfile; fail if resolution would change it. Mutual exclusivity (--no-lockfile xor --frozen-lockfile) enforced in the CLI layer, mirroring upstream's pre-parse validation. Deprecation: - --experimental-lockfile and --experimental-frozen-lockfile remain accepted as hidden aliases through the 1.x line. The CLI emits a WARN on use directing users to the graduated flags. - effective_frozen = frozen_lockfile || experimental_frozen_lockfile matches upstream's effectiveFrozenLockfile coalescing. ## Downstream consumers - up/mod.rs frozen-validation path now reads args.frozen_lockfile (the effective value) instead of args.experimental_frozen_lockfile. No behavior change — only the variable name moves. - build/mod.rs args struct gains no_lockfile + frozen_lockfile fields (carrier-only for PR-4b; #[allow(dead_code)] until then). ## Tests Added in deacon-core/lockfile.rs: - test_depends_on_serializes_as_camel_case - test_write_lockfile_emits_trailing_newline - test_from_resolved_constructs_upstream_form Updated in deacon/tests/up_lockfile_frozen.rs (5 assertions): align with the new upstream-format error messages ("Lockfile does not exist." / "Lockfile does not match." / capital "F" on "Features ..." substrings). ## Verified - cargo fmt --all -- --check - cargo clippy --all-targets -- -D warnings - cargo nextest run --profile dev-fast --no-default-features: 1930/1930 pass - cargo test --doc --workspace: 130/130 pass ## Follow-up tracked - #32 — PR-4b: wire writer in up + build feature pipeline. - Build-command lockfile wiring is gated on the pre-existing TODO at build/mod.rs:1285 (build doesn't install features today). Will land with #32 or as a sibling PR. ## Refs - docs/ROADMAP_TO_1.0.md Tier 1 item "Lockfile graduation" - Upstream: devcontainers/cli#1212 (graduated lockfile in v0.87.0) Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/core/src/lockfile.rs | 152 +++++++++++++++++++--- crates/deacon/src/cli.rs | 75 ++++++++++- crates/deacon/src/commands/build/mod.rs | 20 ++- crates/deacon/src/commands/up/args.rs | 20 ++- crates/deacon/src/commands/up/mod.rs | 21 +-- crates/deacon/tests/up_lockfile_frozen.rs | 43 +++--- 6 files changed, 270 insertions(+), 61 deletions(-) diff --git a/crates/core/src/lockfile.rs b/crates/core/src/lockfile.rs index 35b5c2a..1e47817 100644 --- a/crates/core/src/lockfile.rs +++ b/crates/core/src/lockfile.rs @@ -121,11 +121,41 @@ pub struct LockfileFeature { /// SHA256 digest for integrity checking (e.g., "sha256:...") pub integrity: String, - /// Optional feature dependencies - #[serde(skip_serializing_if = "Option::is_none")] + /// Optional feature dependencies. Spec emits this field as `dependsOn` + /// (camelCase) — see upstream `generateLockfile` in + /// `devcontainers/cli` `src/spec-configuration/lockfile.ts`. + #[serde(rename = "dependsOn", skip_serializing_if = "Option::is_none", default)] pub depends_on: Option>, } +impl LockfileFeature { + /// Construct a lockfile entry in the upstream canonical form. + /// + /// Mirrors `generateLockfile` in `devcontainers/cli` + /// `src/spec-configuration/lockfile.ts`: + /// resolved = `{registry}/{repository}@{digest}` + /// integrity = `{digest}` + /// The `digest` argument MUST be in `sha256:<64-hex>` form (the manifest + /// digest returned by the OCI fetcher). + /// + /// `depends_on` should be an alphabetically-sorted vec of feature IDs, or + /// `None` for features with no dependencies. + pub fn from_resolved( + registry: &str, + repository: &str, + digest: &str, + version: String, + depends_on: Option>, + ) -> Self { + Self { + version, + resolved: format!("{}/{}@{}", registry, repository, digest), + integrity: digest.to_string(), + depends_on, + } + } +} + /// Get lockfile path adjacent to config file /// /// Implements the lockfile naming convention: @@ -274,9 +304,13 @@ pub fn write_lockfile(path: &Path, lockfile: &Lockfile, force_init: bool) -> Res // Sort all object keys recursively for stable JSON output sort_json_object(&mut value); - // Serialize with pretty printing (2-space indentation) - let json = + // Serialize with pretty printing (2-space indentation) and a trailing + // newline to match upstream `devcontainers/cli`'s `writeLockfile` output + // (`JSON.stringify(..., 2) + '\n'`). Byte-identical output keeps the + // `--frozen-lockfile` content comparison stable across implementations. + let mut json = serde_json::to_string_pretty(&value).context("Failed to serialize lockfile to JSON")?; + json.push('\n'); // Atomic write: write to temp file in same directory, then rename // Using same directory ensures same filesystem for atomic rename on all platforms @@ -615,31 +649,35 @@ impl LockfileValidationResult { /// Format the validation result as an error message. /// /// Returns a user-friendly error message describing the mismatch, - /// including actionable guidance on how to resolve the issue. + /// including actionable guidance on how to resolve the issue. The leading + /// summary line mirrors the canonical upstream `devcontainers/cli` strings + /// (`"Lockfile does not exist."` / `"Lockfile does not match."`) so + /// existing CI scripts that match on those messages continue to work. pub fn format_error(&self) -> String { match self { LockfileValidationResult::Matched => "Lockfile validation passed".to_string(), LockfileValidationResult::Missing { expected_path } => { format!( - "Frozen lockfile mode requires a lockfile, but none found at '{}'.\n\ - Run without --experimental-frozen-lockfile to generate a lockfile, \ - or create one with `deacon build --experimental-lockfile`.", + "Lockfile does not exist.\nExpected at '{}'.\n\ + Run without --frozen-lockfile to generate a lockfile, or \ + generate one with `deacon upgrade`.", expected_path.display() ) } LockfileValidationResult::MissingFromLockfile { features } => { format!( - "Frozen lockfile mismatch: features declared in config but missing from lockfile:\n \ + "Lockfile does not match.\nFeatures declared in config but missing from lockfile:\n \ - {}\n\ - Update the lockfile or remove --experimental-frozen-lockfile to allow resolution.", + Run without --frozen-lockfile to update the lockfile, or run `deacon upgrade`.", features.join("\n - ") ) } LockfileValidationResult::ExtraInLockfile { features } => { format!( - "Frozen lockfile mismatch: features in lockfile but not declared in config:\n \ + "Lockfile does not match.\nFeatures in lockfile but not declared in config:\n \ - {}\n\ - Update the lockfile to remove stale entries, or add these features to your config.", + Update the lockfile to remove stale entries (e.g. via `deacon upgrade`), \ + or add these features to your config.", features.join("\n - ") ) } @@ -648,10 +686,10 @@ impl LockfileValidationResult { extra_in_lockfile, } => { format!( - "Frozen lockfile mismatch:\n\ + "Lockfile does not match.\n\ Features declared in config but missing from lockfile:\n - {}\n\ Features in lockfile but not declared in config:\n - {}\n\ - Update the lockfile or remove --experimental-frozen-lockfile to allow resolution.", + Run without --frozen-lockfile to update the lockfile, or run `deacon upgrade`.", missing_from_lockfile.join("\n - "), extra_in_lockfile.join("\n - ") ) @@ -1450,14 +1488,90 @@ mod tests { assert_eq!(result.format_error(), "Lockfile validation passed"); } + // ========================================================================= + // Upstream wire-format parity tests + // ========================================================================= + + /// The `dependsOn` field MUST serialize as camelCase per upstream + /// `devcontainers/cli` `generateLockfile`. Deserialization must accept + /// the camelCase form too. + #[test] + fn test_depends_on_serializes_as_camel_case() { + let entry = LockfileFeature { + version: "1.0.0".to_string(), + resolved: "ghcr.io/x/y@sha256:1111111111111111111111111111111111111111111111111111111111111111".to_string(), + integrity: "sha256:1111111111111111111111111111111111111111111111111111111111111111".to_string(), + depends_on: Some(vec!["dep-a".to_string()]), + }; + let json = serde_json::to_string(&entry).unwrap(); + assert!( + json.contains("\"dependsOn\""), + "expected camelCase `dependsOn` on the wire, got: {}", + json + ); + assert!( + !json.contains("\"depends_on\""), + "snake_case `depends_on` must not be emitted, got: {}", + json + ); + + // Round-trip both wire forms. + let from_camel: LockfileFeature = serde_json::from_str(&json).unwrap(); + assert_eq!(from_camel, entry); + } + + /// Writer emits a trailing newline to match upstream + /// `JSON.stringify(..., 2) + '\n'`. + #[test] + fn test_write_lockfile_emits_trailing_newline() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("lf.json"); + let lockfile = Lockfile { + features: HashMap::new(), + }; + write_lockfile(&path, &lockfile, true).unwrap(); + let bytes = std::fs::read(&path).unwrap(); + assert_eq!( + bytes.last().copied(), + Some(b'\n'), + "lockfile output must end with a single trailing newline" + ); + } + + /// `LockfileFeature::from_resolved` constructs entries in the upstream + /// canonical form: `{registry}/{repository}@{digest}` for `resolved`, + /// digest alone for `integrity`. + #[test] + fn test_from_resolved_constructs_upstream_form() { + let entry = LockfileFeature::from_resolved( + "ghcr.io", + "devcontainers/features/node", + "sha256:1111111111111111111111111111111111111111111111111111111111111111", + "1.2.3".to_string(), + None, + ); + assert_eq!(entry.version, "1.2.3"); + assert_eq!( + entry.resolved, + "ghcr.io/devcontainers/features/node@sha256:1111111111111111111111111111111111111111111111111111111111111111" + ); + assert_eq!( + entry.integrity, + "sha256:1111111111111111111111111111111111111111111111111111111111111111" + ); + assert!(entry.depends_on.is_none()); + } + #[test] fn test_validation_result_format_error_missing() { let result = LockfileValidationResult::Missing { expected_path: PathBuf::from("/path/to/lockfile.json"), }; let error = result.format_error(); - assert!(error.contains("Frozen lockfile mode requires a lockfile")); + // Upstream-aligned strings (devcontainers/cli writeLockfile errors). + assert!(error.contains("Lockfile does not exist.")); assert!(error.contains("/path/to/lockfile.json")); + assert!(error.contains("--frozen-lockfile")); } #[test] @@ -1466,7 +1580,8 @@ mod tests { features: vec!["feature-a".to_string(), "feature-b".to_string()], }; let error = result.format_error(); - assert!(error.contains("features declared in config but missing from lockfile")); + assert!(error.contains("Lockfile does not match.")); + assert!(error.contains("Features declared in config but missing from lockfile")); assert!(error.contains("feature-a")); assert!(error.contains("feature-b")); } @@ -1477,7 +1592,8 @@ mod tests { features: vec!["stale-feature".to_string()], }; let error = result.format_error(); - assert!(error.contains("features in lockfile but not declared in config")); + assert!(error.contains("Lockfile does not match.")); + assert!(error.contains("Features in lockfile but not declared in config")); assert!(error.contains("stale-feature")); } @@ -1488,7 +1604,7 @@ mod tests { extra_in_lockfile: vec!["old-feature".to_string()], }; let error = result.format_error(); - assert!(error.contains("Frozen lockfile mismatch")); + assert!(error.contains("Lockfile does not match.")); assert!(error.contains("new-feature")); assert!(error.contains("old-feature")); } diff --git a/crates/deacon/src/cli.rs b/crates/deacon/src/cli.rs index fafdbec..97554f6 100644 --- a/crates/deacon/src/cli.rs +++ b/crates/deacon/src/cli.rs @@ -228,11 +228,18 @@ pub enum Commands { /// Skip feature auto-mapping (hidden testing flag) #[arg(long, hide = true)] skip_feature_auto_mapping: bool, - /// Path to feature lockfile for validation (experimental, hidden) + /// Disable lockfile generation and verification. Mutually exclusive with --frozen-lockfile. + #[arg(long)] + no_lockfile: bool, + /// Require an up-to-date lockfile; fail if resolution would change it. + /// Mutually exclusive with --no-lockfile. + #[arg(long)] + frozen_lockfile: bool, + /// DEPRECATED: use --frozen-lockfile (and pass a path via --config if needed). + /// Kept as a hidden alias through the 1.x line; emits a WARN when used. #[arg(long, hide = true)] experimental_lockfile: Option, - /// Require lockfile to exist and match config features exactly (experimental, hidden) - /// Implies --experimental-lockfile if not specified; uses default lockfile location. + /// DEPRECATED alias for --frozen-lockfile (graduated in 1.0). Hidden; emits a WARN. #[arg(long, hide = true)] experimental_frozen_lockfile: bool, /// Dotfiles repository URL @@ -380,10 +387,17 @@ pub enum Commands { /// Do not persist customizations from features into image metadata #[arg(long, hide = true)] skip_persisting_customizations_from_features: bool, - /// Write feature lockfile (experimental) + /// Disable lockfile generation and verification. Mutually exclusive with --frozen-lockfile. + #[arg(long)] + no_lockfile: bool, + /// Require an up-to-date lockfile; fail if resolution would change it. + /// Mutually exclusive with --no-lockfile. + #[arg(long)] + frozen_lockfile: bool, + /// DEPRECATED: lockfile is now written by default. Hidden alias kept through 1.x; emits a WARN. #[arg(long, hide = true)] experimental_lockfile: bool, - /// Fail if lockfile changes would occur (experimental) + /// DEPRECATED alias for --frozen-lockfile (graduated in 1.0). Hidden; emits a WARN. #[arg(long, hide = true)] experimental_frozen_lockfile: bool, /// Omit Dockerfile syntax directive workaround @@ -959,6 +973,8 @@ impl Cli { prefer_cli_features, feature_install_order, skip_feature_auto_mapping, + no_lockfile, + frozen_lockfile, experimental_lockfile, experimental_frozen_lockfile, dotfiles_repository, @@ -979,6 +995,29 @@ impl Cli { }) => { use crate::commands::up::{execute_up, UpArgs}; + // Mutual exclusivity check (mirrors devcontainers/cli). + if no_lockfile && (frozen_lockfile || experimental_frozen_lockfile) { + anyhow::bail!("--no-lockfile and --frozen-lockfile are mutually exclusive."); + } + // Emit deprecation WARN for the hidden experimental aliases. + if experimental_lockfile.is_some() { + tracing::warn!( + target: "deacon::lockfile", + "--experimental-lockfile is deprecated and will be removed in 2.0. \ + Lockfile generation is now the default; pass --no-lockfile to disable. \ + The custom-path form has no replacement (the lockfile lives next to the config)." + ); + } + if experimental_frozen_lockfile { + tracing::warn!( + target: "deacon::lockfile", + "--experimental-frozen-lockfile is deprecated and will be removed in 2.0; \ + use --frozen-lockfile." + ); + } + // effective_frozen = either flag (matches upstream's effectiveFrozenLockfile) + let effective_frozen_lockfile = frozen_lockfile || experimental_frozen_lockfile; + let args = UpArgs { id_label, remove_existing_container, @@ -997,6 +1036,8 @@ impl Cli { cache_to, buildkit, skip_feature_auto_mapping, + no_lockfile, + frozen_lockfile: effective_frozen_lockfile, experimental_lockfile, experimental_frozen_lockfile, dotfiles_repository, @@ -1137,12 +1178,34 @@ impl Cli { output, skip_feature_auto_mapping, skip_persisting_customizations_from_features, + no_lockfile, + frozen_lockfile, experimental_lockfile, experimental_frozen_lockfile, omit_syntax_directive, }) => { use crate::commands::build::{execute_build, BuildArgs}; + // Mutual exclusivity check (mirrors devcontainers/cli). + if no_lockfile && (frozen_lockfile || experimental_frozen_lockfile) { + anyhow::bail!("--no-lockfile and --frozen-lockfile are mutually exclusive."); + } + if experimental_lockfile { + tracing::warn!( + target: "deacon::lockfile", + "--experimental-lockfile is deprecated and will be removed in 2.0; \ + lockfile generation is now the default. Pass --no-lockfile to disable." + ); + } + if experimental_frozen_lockfile { + tracing::warn!( + target: "deacon::lockfile", + "--experimental-frozen-lockfile is deprecated and will be removed in 2.0; \ + use --frozen-lockfile." + ); + } + let effective_frozen_lockfile = frozen_lockfile || experimental_frozen_lockfile; + let args = BuildArgs { no_cache, platform, @@ -1177,6 +1240,8 @@ impl Cli { output, skip_feature_auto_mapping, skip_persisting_customizations_from_features, + no_lockfile, + frozen_lockfile: effective_frozen_lockfile, experimental_lockfile, experimental_frozen_lockfile, omit_syntax_directive, diff --git a/crates/deacon/src/commands/build/mod.rs b/crates/deacon/src/commands/build/mod.rs index 650810f..86f2459 100644 --- a/crates/deacon/src/commands/build/mod.rs +++ b/crates/deacon/src/commands/build/mod.rs @@ -66,11 +66,21 @@ pub struct BuildArgs { /// Do not persist customizations from features into image metadata #[allow(dead_code)] // Reserved for future feature implementation pub skip_persisting_customizations_from_features: bool, - /// Write feature lockfile (experimental) - #[allow(dead_code)] // Reserved for future feature implementation + /// Skip lockfile generation and verification (graduated 1.0). + #[allow(dead_code)] // Wired to behavior in PR-4b (writer integration follow-up) + pub no_lockfile: bool, + /// Require an up-to-date lockfile; fail if resolution would change it + /// (graduated 1.0). Effective value (CLI ORs the deprecated + /// `--experimental-frozen-lockfile` alias into this). + #[allow(dead_code)] // Wired to behavior in PR-4b + pub frozen_lockfile: bool, + /// DEPRECATED hidden alias for the legacy --experimental-lockfile (bool form). + /// CLI layer emits a WARN when set; kept through 1.x for backward compat. + #[allow(dead_code)] // Will be removed in 2.0 pub experimental_lockfile: bool, - /// Fail if lockfile changes would occur (experimental) - #[allow(dead_code)] // Reserved for future feature implementation + /// DEPRECATED hidden alias for --frozen-lockfile. + /// CLI layer ORs into `frozen_lockfile` and emits a WARN. + #[allow(dead_code)] // Will be removed in 2.0 pub experimental_frozen_lockfile: bool, /// Omit Dockerfile syntax directive workaround #[allow(dead_code)] // Reserved for future feature implementation @@ -113,6 +123,8 @@ impl Default for BuildArgs { output: None, skip_feature_auto_mapping: false, skip_persisting_customizations_from_features: false, + no_lockfile: false, + frozen_lockfile: false, experimental_lockfile: false, experimental_frozen_lockfile: false, omit_syntax_directive: false, diff --git a/crates/deacon/src/commands/up/args.rs b/crates/deacon/src/commands/up/args.rs index 8351bfb..8304781 100644 --- a/crates/deacon/src/commands/up/args.rs +++ b/crates/deacon/src/commands/up/args.rs @@ -365,10 +365,22 @@ pub struct UpArgs { pub dotfiles_install_command: Option, pub dotfiles_target_path: Option, - // Lockfile control (experimental) - /// Path to feature lockfile for validation (experimental) + // Lockfile control (graduated in 1.0; default behavior is read + write) + /// Skip lockfile generation and verification. + pub no_lockfile: bool, + /// Require an up-to-date lockfile; fail if resolution would change it. + /// This is the effective value (already OR'd with the deprecated + /// `experimental_frozen_lockfile` alias in the CLI layer). + pub frozen_lockfile: bool, + /// DEPRECATED hidden alias for the legacy `--experimental-lockfile ` + /// form. Kept through the 1.x line so existing CI scripts keep working; + /// the CLI layer emits a WARN when set. The custom-path semantics have + /// no replacement in the graduated flag surface. pub experimental_lockfile: Option, - /// Require lockfile to exist and match config features exactly (experimental) + /// DEPRECATED hidden alias for `--frozen-lockfile`. The CLI layer ORs + /// this into `frozen_lockfile` and emits a WARN; downstream code should + /// read `frozen_lockfile` only. + #[allow(dead_code)] // Plumbed for shape parity; CLI layer ORs into frozen_lockfile. pub experimental_frozen_lockfile: bool, // Metadata and output control @@ -448,6 +460,8 @@ impl Default for UpArgs { dotfiles_repository: None, dotfiles_install_command: None, dotfiles_target_path: None, + no_lockfile: false, + frozen_lockfile: false, experimental_lockfile: None, experimental_frozen_lockfile: false, omit_config_remote_env_from_metadata: false, diff --git a/crates/deacon/src/commands/up/mod.rs b/crates/deacon/src/commands/up/mod.rs index 88e99a7..eb962d2 100644 --- a/crates/deacon/src/commands/up/mod.rs +++ b/crates/deacon/src/commands/up/mod.rs @@ -203,18 +203,19 @@ pub(crate) async fn execute_up_with_runtime( check_for_disallowed_features(&config.features)?; debug!("Validated features - no disallowed features found"); - // T012: Enforce lockfile/frozen validation pre-build - // T013: User-facing error handling for lockfile/frozen enforcement - // If frozen mode is enabled, or lockfile path is explicitly provided, validate before build - if args.experimental_frozen_lockfile || args.experimental_lockfile.is_some() { - // Determine lockfile path: use explicit path if provided, otherwise derive from config + // Frozen-lockfile pre-build validation (graduated in 1.0). + // `args.frozen_lockfile` is the effective value (CLI layer ORs the + // deprecated --experimental-frozen-lockfile alias into it). + // Skip lockfile interaction entirely when --no-lockfile is set. + if !args.no_lockfile && (args.frozen_lockfile || args.experimental_lockfile.is_some()) { + // Determine lockfile path: use explicit path if provided (deprecated), + // otherwise derive from config. let lockfile_path = args .experimental_lockfile .clone() .unwrap_or_else(|| get_lockfile_path(&config_path)); - // Use info-level log so users can see lockfile validation is active - if args.experimental_frozen_lockfile { + if args.frozen_lockfile { info!( "Frozen lockfile mode enabled: validating features against '{}'", lockfile_path.display() @@ -231,7 +232,7 @@ pub(crate) async fn execute_up_with_runtime( format!( "Failed to read lockfile at '{}'. \ The file may be corrupted or contain invalid JSON. \ - To regenerate, remove the file and run without --experimental-frozen-lockfile.", + To regenerate, remove the file and run without --frozen-lockfile.", lockfile_path.display() ) })?; @@ -242,7 +243,7 @@ pub(crate) async fn execute_up_with_runtime( match &validation_result { LockfileValidationResult::Matched => { - if args.experimental_frozen_lockfile { + if args.frozen_lockfile { info!( "Lockfile validation passed: all features match '{}'", lockfile_path.display() @@ -254,7 +255,7 @@ pub(crate) async fn execute_up_with_runtime( _ => { let error_message = validation_result.format_error(); - if args.experimental_frozen_lockfile { + if args.frozen_lockfile { // Frozen mode: fail immediately on any mismatch (exit code 1) return Err(DeaconError::Config( deacon_core::errors::ConfigError::Validation { diff --git a/crates/deacon/tests/up_lockfile_frozen.rs b/crates/deacon/tests/up_lockfile_frozen.rs index d450eec..d79b3fb 100644 --- a/crates/deacon/tests/up_lockfile_frozen.rs +++ b/crates/deacon/tests/up_lockfile_frozen.rs @@ -265,16 +265,17 @@ fn test_frozen_lockfile_missing_fails() { ), } - // Verify error message content + // Verify error message content matches upstream-aligned format + // ("Lockfile does not exist." / "--frozen-lockfile" — see lockfile.rs format_error) let error_msg = validation_result.format_error(); assert!( - error_msg.contains("Frozen lockfile mode requires a lockfile"), - "Error message should indicate lockfile is required. Got: {}", + error_msg.contains("Lockfile does not exist."), + "Error message should match upstream missing-lockfile string. Got: {}", error_msg ); assert!( - error_msg.contains("--experimental-frozen-lockfile"), - "Error message should mention the flag to disable. Got: {}", + error_msg.contains("--frozen-lockfile"), + "Error message should mention the graduated flag. Got: {}", error_msg ); } @@ -321,7 +322,7 @@ fn test_frozen_lockfile_mismatch_fails() { // Verify error message content let error_msg = validation_result.format_error(); assert!( - error_msg.contains("features declared in config but missing from lockfile"), + error_msg.contains("Features declared in config but missing from lockfile"), "Error message should describe mismatch direction. Got: {}", error_msg ); @@ -385,7 +386,7 @@ fn test_lockfile_mismatch_warns_continues() { // The format_error() provides the warning message that would be logged. let warning_msg = validation_result.format_error(); assert!( - warning_msg.contains("features in lockfile but not declared in config"), + warning_msg.contains("Features in lockfile but not declared in config"), "Warning should describe the mismatch. Got: {}", warning_msg ); @@ -480,7 +481,7 @@ fn test_frozen_mode_with_lockfile_features_not_in_config_fails() { let error_msg = validation_result.format_error(); assert!( - error_msg.contains("features in lockfile but not declared in config"), + error_msg.contains("Features in lockfile but not declared in config"), "Error should describe the mismatch direction. Got: {}", error_msg ); @@ -584,10 +585,10 @@ fn test_frozen_missing_lockfile_error_message_content() { let error_msg = result.format_error(); - // Verify message contains required components + // Verify message matches upstream-aligned format assert!( - error_msg.contains("Frozen lockfile mode requires a lockfile"), - "Error should indicate frozen mode requirement. Got: {}", + error_msg.contains("Lockfile does not exist."), + "Error should use upstream missing-lockfile string. Got: {}", error_msg ); assert!( @@ -596,8 +597,8 @@ fn test_frozen_missing_lockfile_error_message_content() { error_msg ); assert!( - error_msg.contains("--experimental-frozen-lockfile"), - "Error should provide actionable guidance. Got: {}", + error_msg.contains("--frozen-lockfile"), + "Error should reference the graduated flag. Got: {}", error_msg ); } @@ -612,12 +613,12 @@ fn test_frozen_mismatch_missing_from_lockfile_error_message_content() { let error_msg = result.format_error(); assert!( - error_msg.contains("Frozen lockfile mismatch"), - "Error should indicate frozen lockfile mismatch. Got: {}", + error_msg.contains("Lockfile does not match."), + "Error should use upstream mismatch string. Got: {}", error_msg ); assert!( - error_msg.contains("features declared in config but missing from lockfile"), + error_msg.contains("Features declared in config but missing from lockfile"), "Error should describe mismatch direction. Got: {}", error_msg ); @@ -638,12 +639,12 @@ fn test_frozen_mismatch_extra_in_lockfile_error_message_content() { let error_msg = result.format_error(); assert!( - error_msg.contains("Frozen lockfile mismatch"), - "Error should indicate frozen lockfile mismatch. Got: {}", + error_msg.contains("Lockfile does not match."), + "Error should use upstream mismatch string. Got: {}", error_msg ); assert!( - error_msg.contains("features in lockfile but not declared in config"), + error_msg.contains("Features in lockfile but not declared in config"), "Error should describe mismatch direction. Got: {}", error_msg ); @@ -665,8 +666,8 @@ fn test_frozen_mismatch_bidirectional_error_message_content() { let error_msg = result.format_error(); assert!( - error_msg.contains("Frozen lockfile mismatch"), - "Error should indicate frozen lockfile mismatch. Got: {}", + error_msg.contains("Lockfile does not match."), + "Error should use upstream mismatch string. Got: {}", error_msg ); assert!( From 025be242d5685f0cdbfecd024b2ccc29fcb87664 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Mon, 25 May 2026 18:01:58 +0000 Subject: [PATCH 2/2] =?UTF-8?q?feat:=20upgrade=20subcommand=20=E2=80=94=20?= =?UTF-8?q?regenerate-lockfile=20MVP=20(PR-5a)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the regenerate path of `deacon upgrade` per `docs/subcommand-specs/upgrade/SPEC.md`: re-resolve every Feature against the OCI registry, build a fresh `Lockfile`, and either print it to stdout (`--dry-run`) or write it to disk via `write_lockfile(force_init = true)`. ## CLI surface `deacon upgrade [--dry-run] [--docker-path ] [--docker-compose-path ] [-f ] [-v ]` Workspace + `--config` come from the global flags (parity with other consumer subcommands). `-f`/`--feature` and `-v`/`--target-version` are hidden (Dependabot-only). ## What this PR includes - Validation (fail-fast per spec §2/§3): - `--feature` and `--target-version` must both be set or both absent; error message matches upstream: `"The '--target-version' and '--feature' flag must be used together."` - `--target-version` must match `^\d+(\.\d+(\.\d+)?)?$`; error matches upstream: `"Invalid version 'X'. Must be in the form of 'x', 'x.y', or 'x.y.z'"` - Config load via the shared `load_config` helper (extends chain honored). - Feature resolution via the existing OCI fetcher (`default_fetcher()` + `fetch_feature`), which returns each feature's manifest digest + metadata version. - Lockfile assembly keyed by user-provided feature ID (matches upstream `generateLockfile` + PR-4b's helper in `up`). Metadata-version missing → falls back to the user-requested tag (e.g. `"1"`) with a WARN. - `--dry-run`: pretty-printed canonical JSON to stdout (sorted keys, trailing newline) — byte-identical to what `write_lockfile` would emit. - Default: `write_lockfile(force_init = true)` per spec §5 phase 4. ## Tests 13 new unit tests in `upgrade::tests`: - Pin-flag pairing: both-set, both-absent, only-feature, only-target - Target-version regex: 5 valid forms, 8 invalid forms (incl. `latest`, `v1`, `1.x.3`) - Spec-exact error message on invalid `--target-version` - Lockfile entry assembly: metadata-version vs tag fallback, sorted `dependsOn` - Empty-features short-circuit (two flavors — missing object, empty object) - Argument defaults Verification: - `cargo fmt --all -- --check` - `cargo clippy --all-targets -- -D warnings` - `cargo test -p deacon --lib` → 226 pass (no regression; baseline includes PR-4a since this branch is based on `pr4-lockfile-graduation`) ## Deferred to PR-5b - `--feature` / `--target-version` config-pin behavior (modifies `devcontainer.json` in place). The flags are accepted today so the CLI surface is stable, but using them returns `"--feature/--target-version pinning is not yet implemented (PR-5b)"` rather than silently doing nothing. ## Code-dedup follow-up The lockfile-entry assembly logic is intentionally duplicated from PR-4b (`crates/deacon/src/commands/up/features_build.rs::build_lockfile_from_features`) so PR-5a doesn't depend on PR-4b's diff. Once both land on `main`, a small follow-up PR can lift the shared logic into `deacon_core::lockfile`. ## Base branch Based on `pr4-lockfile-graduation` (#33) since upgrade needs `deacon_core::lockfile::{Lockfile, LockfileFeature, get_lockfile_path, write_lockfile}` — all added in PR-4a. After #33 merges, this branch auto-rebases onto `main`. Closes part of the "Implement `upgrade`" Tier 1 blocker (issue #34). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/deacon/src/cli.rs | 50 +++ crates/deacon/src/commands/mod.rs | 2 + crates/deacon/src/commands/upgrade.rs | 485 ++++++++++++++++++++++++++ 3 files changed, 537 insertions(+) create mode 100644 crates/deacon/src/commands/upgrade.rs diff --git a/crates/deacon/src/cli.rs b/crates/deacon/src/cli.rs index 97554f6..7aa419e 100644 --- a/crates/deacon/src/cli.rs +++ b/crates/deacon/src/cli.rs @@ -510,6 +510,34 @@ pub enum Commands { command: TemplateCommands, }, + /// Regenerate (or refresh) the devcontainer lockfile from the currently + /// resolved Feature set. Use `--dry-run` to print the lockfile JSON to + /// stdout instead of writing to disk. + /// + /// See `docs/subcommand-specs/upgrade/SPEC.md` for the authoritative behavior. + #[cfg(feature = "full")] + Upgrade { + /// Print the generated lockfile JSON to stdout instead of writing it + /// to disk. Spec §2. + #[arg(long)] + dry_run: bool, + /// Docker CLI path. Default `docker`. Spec §2 surface parity only. + #[arg(long, default_value = "docker")] + docker_path: String, + /// Docker Compose CLI path. Default `docker-compose`. Spec §2 surface parity only. + #[arg(long, default_value = "docker-compose")] + docker_compose_path: String, + /// HIDDEN: pin the version of a specific Feature in `devcontainer.json` + /// before regenerating the lockfile. Used by Dependabot. + /// Must be used with `--target-version`. **Pinning is deferred to PR-5b.** + #[arg(long, short = 'f', hide = true)] + feature: Option, + /// HIDDEN: target version for `--feature`. Must match + /// `^\d+(\.\d+(\.\d+)?)?$`. **Pinning is deferred to PR-5b.** + #[arg(long, short = 'v', hide = true)] + target_version: Option, + }, + /// Run user-defined lifecycle commands #[cfg(feature = "full")] #[allow(clippy::enum_variant_names)] @@ -1370,6 +1398,28 @@ impl Cli { execute_templates(args).await } #[cfg(feature = "full")] + Some(Commands::Upgrade { + dry_run, + docker_path, + docker_compose_path, + feature, + target_version, + }) => { + use crate::commands::upgrade::{execute_upgrade, UpgradeArgs}; + + let args = UpgradeArgs { + workspace_folder: self.workspace_folder, + config_path: self.config, + docker_path, + docker_compose_path, + dry_run, + feature, + target_version, + }; + + execute_upgrade(args).await + } + #[cfg(feature = "full")] Some(Commands::RunUserCommands { skip_post_create, skip_post_attach, diff --git a/crates/deacon/src/commands/mod.rs b/crates/deacon/src/commands/mod.rs index 7d399a9..7e9678e 100644 --- a/crates/deacon/src/commands/mod.rs +++ b/crates/deacon/src/commands/mod.rs @@ -17,6 +17,8 @@ pub mod shared; #[cfg(feature = "full")] pub mod templates; pub mod up; +#[cfg(feature = "full")] +pub mod upgrade; /// Re-export the UpResult type to preserve the stdout JSON contract for the up command. pub use up::UpResult; diff --git a/crates/deacon/src/commands/upgrade.rs b/crates/deacon/src/commands/upgrade.rs new file mode 100644 index 0000000..db9e4ec --- /dev/null +++ b/crates/deacon/src/commands/upgrade.rs @@ -0,0 +1,485 @@ +//! Upgrade subcommand implementation. +//! +//! `deacon upgrade` regenerates `devcontainer-lock.json` from the currently +//! resolved feature set, mirroring upstream `devcontainers/cli`'s `upgrade` +//! command. See `docs/subcommand-specs/upgrade/SPEC.md` for the authoritative +//! behavior. +//! +//! ## PR-5a scope (MVP) +//! +//! - CLI surface (all spec §2 flags accepted) +//! - Argument validation: `--feature` ↔ `--target-version` mutual requirement, +//! `--target-version` regex `^\d+(\.\d+(\.\d+)?)?$` +//! - Config load via shared `ConfigLoader::load_with_extends` +//! - Feature resolution via the OCI fetcher (fetches manifests to obtain +//! digests + actual versions) +//! - Lockfile assembly (private helper modeled on PR-4b's +//! `build_lockfile_from_features` — will be deduplicated in a follow-up) +//! - `--dry-run`: print canonical lockfile JSON to stdout +//! - Default: `write_lockfile(force_init = true)` per spec §5 +//! +//! ## Deferred to PR-5b +//! +//! - `--feature` / `--target-version` config-pin behavior (modifies +//! `devcontainer.json` in place). The flags are accepted today so the CLI +//! surface is stable, but using them returns +//! `"--feature/--target-version pinning is not yet implemented (PR-5b)"` +//! instead of silently doing nothing. + +use anyhow::{Context, Result}; +use deacon_core::config::DevContainerConfig; +use deacon_core::lockfile::{get_lockfile_path, write_lockfile, Lockfile, LockfileFeature}; +use deacon_core::oci::{default_fetcher, DownloadedFeature, FeatureRef}; +use deacon_core::registry_parser::parse_registry_reference; +use std::collections::HashMap; +use std::path::PathBuf; +use tracing::{debug, info, instrument, warn}; + +use crate::commands::shared::{load_config, ConfigLoadArgs, ConfigLoadResult}; + +/// Arguments for the `upgrade` command. Mirrors the spec's CLI surface +/// (`docs/subcommand-specs/upgrade/SPEC.md` §2). +#[derive(Debug, Clone)] +pub struct UpgradeArgs { + /// Workspace folder. Required by spec §2; if omitted, defaults to the + /// current working directory (matches upstream's `getCliHost` behavior + /// when invoked without `--workspace-folder`). + pub workspace_folder: Option, + /// Optional `--config `. + pub config_path: Option, + /// Docker CLI path; default `"docker"`. Spec §2 surface parity only — + /// upgrade itself does not invoke docker (the OCI fetcher uses HTTP). + #[allow(dead_code)] + pub docker_path: String, + /// Docker Compose CLI path; default `"docker-compose"`. Spec §2 surface + /// parity only — upgrade does not invoke compose. + #[allow(dead_code)] + pub docker_compose_path: String, + /// When true, print the generated lockfile JSON to stdout instead of + /// writing to disk. Spec §2 (`--dry-run`). + pub dry_run: bool, + /// Hidden: pin the version of a specific feature in `devcontainer.json` + /// before regenerating the lockfile. Used by Dependabot. Must be set + /// together with `--target-version`. **Deferred to PR-5b.** + pub feature: Option, + /// Hidden: target version for `--feature`. Must match + /// `^\d+(\.\d+(\.\d+)?)?$`. **Deferred to PR-5b.** + pub target_version: Option, +} + +/// Execute the `upgrade` command end-to-end. +#[instrument(skip(args))] +pub async fn execute_upgrade(args: UpgradeArgs) -> Result<()> { + info!("Starting upgrade execution"); + + // Phase 1: Fail-fast validation (spec §2/§3). + validate_pin_flag_pairing(args.feature.as_deref(), args.target_version.as_deref())?; + if let Some(tv) = args.target_version.as_deref() { + validate_target_version_format(tv)?; + } + if args.feature.is_some() { + // Surface-parity placeholder: accept the flag combination (so callers + // can validate their wiring) but bail before touching devcontainer.json. + return Err(anyhow::anyhow!( + "--feature/--target-version pinning is not yet implemented (PR-5b)" + )); + } + + // Phase 2: Resolve the workspace + config path via the shared loader. + // The shared loader returns the resolved config_path so we can derive the + // lockfile path adjacent to it (spec §6 naming rule). + let ConfigLoadResult { + config, + config_path, + .. + } = load_config(ConfigLoadArgs { + workspace_folder: args.workspace_folder.as_deref(), + config_path: args.config_path.as_deref(), + override_config_path: None, + secrets_files: &[], + })?; + + debug!( + "Loaded configuration from '{}' (features: {:?})", + config_path.display(), + config + .features + .as_object() + .map(|m| m.keys().collect::>()) + ); + + // Phase 3: Resolve features against the OCI registry. This is the + // "regenerate" step — every feature in config is re-fetched so we + // obtain the current matching digest + version. + let lockfile = resolve_lockfile_from_config(&config).await?; + + // Phase 4: Output. + if args.dry_run { + emit_lockfile_json(&lockfile)?; + info!("Dry-run: lockfile JSON printed to stdout; nothing written to disk"); + return Ok(()); + } + + let lockfile_path = get_lockfile_path(&config_path); + // Spec §5 phase 4: force_init = true so the writer always overwrites. + write_lockfile(&lockfile_path, &lockfile, true) + .with_context(|| format!("Failed to write lockfile to '{}'", lockfile_path.display()))?; + info!("Wrote lockfile to '{}'", lockfile_path.display()); + Ok(()) +} + +/// Validate that `--feature` and `--target-version` are either both set or +/// both absent (spec §2/§3: mutually constrained). +fn validate_pin_flag_pairing(feature: Option<&str>, target_version: Option<&str>) -> Result<()> { + match (feature.is_some(), target_version.is_some()) { + (false, false) | (true, true) => Ok(()), + _ => Err(anyhow::anyhow!( + "The '--target-version' and '--feature' flag must be used together." + )), + } +} + +/// Validate `--target-version` matches the spec-§2 regex +/// `^\d+(\.\d+(\.\d+)?)?$` — accepts `X`, `X.Y`, or `X.Y.Z`. +fn validate_target_version_format(version: &str) -> Result<()> { + if !is_valid_target_version(version) { + return Err(anyhow::anyhow!( + "Invalid version '{}'. Must be in the form of 'x', 'x.y', or 'x.y.z'", + version + )); + } + Ok(()) +} + +/// Pure predicate that mirrors the spec-§2 regex. Hand-rolled to avoid +/// pulling `regex` into the call site (we already depend on it transitively, +/// but the check is trivial enough to inline). +fn is_valid_target_version(version: &str) -> bool { + if version.is_empty() { + return false; + } + let parts: Vec<&str> = version.split('.').collect(); + if parts.len() > 3 { + return false; + } + parts + .iter() + .all(|p| !p.is_empty() && p.chars().all(|c| c.is_ascii_digit())) +} + +/// Re-fetch every feature in the config and assemble the lockfile. +/// +/// Each feature is parsed from its user-provided ID, looked up via the OCI +/// fetcher (which resolves tags to concrete manifests + digests), and +/// converted into a `LockfileFeature` entry keyed by the user-provided ID +/// per upstream `generateLockfile`. +async fn resolve_lockfile_from_config(config: &DevContainerConfig) -> Result { + let features_obj = match config.features.as_object() { + Some(obj) => obj, + // Per spec §5: an empty/missing features map is not an error; + // upgrade just produces an empty lockfile. + None => { + return Ok(Lockfile { + features: HashMap::new(), + }) + } + }; + + if features_obj.is_empty() { + return Ok(Lockfile { + features: HashMap::new(), + }); + } + + let fetcher = default_fetcher().context("Failed to construct OCI fetcher for upgrade")?; + let mut entries: HashMap = HashMap::new(); + + for (user_id, _opts) in features_obj.iter() { + let (registry, namespace, name, tag) = parse_registry_reference(user_id) + .with_context(|| format!("Invalid feature ID '{}'", user_id))?; + let feature_ref = FeatureRef::new(registry, namespace, name, tag); + + debug!(feature = %user_id, "Re-resolving feature against OCI registry"); + let downloaded: DownloadedFeature = fetcher + .fetch_feature(&feature_ref) + .await + .with_context(|| format!("Failed to fetch feature '{}' from OCI registry", user_id))?; + + let entry = lockfile_entry_for(&feature_ref, &downloaded, user_id); + entries.insert(user_id.clone(), entry); + } + + Ok(Lockfile { features: entries }) +} + +/// Build a single `LockfileFeature` from a resolved feature reference + +/// download. Mirrors PR-4b's `build_lockfile_from_features` entry logic +/// (duplicated here intentionally so PR-5 doesn't depend on PR-4b's diff; +/// will be deduplicated once both land on main). +fn lockfile_entry_for( + feature_ref: &FeatureRef, + downloaded: &DownloadedFeature, + user_id: &str, +) -> LockfileFeature { + let version = match &downloaded.metadata.version { + Some(v) if !v.is_empty() => v.clone(), + _ => { + let fallback = feature_ref.tag(); + warn!( + feature = %user_id, + fallback = %fallback, + "Feature metadata has no version field; using tag as fallback for lockfile entry" + ); + fallback.to_string() + } + }; + + let depends_on = if downloaded.metadata.depends_on.is_empty() { + None + } else { + let mut deps: Vec = downloaded.metadata.depends_on.keys().cloned().collect(); + deps.sort(); + Some(deps) + }; + + LockfileFeature::from_resolved( + &feature_ref.registry, + &feature_ref.repository(), + &downloaded.digest, + version, + depends_on, + ) +} + +/// Print the lockfile to stdout as canonical JSON. +/// +/// Format mirrors `deacon_core::lockfile::write_lockfile`: serde-derived +/// JSON, recursively sorted object keys, 2-space pretty-printed, with a +/// trailing newline. A `--dry-run` consumer should be able to redirect this +/// directly into the lockfile file and get the same bytes as a non-dry-run. +fn emit_lockfile_json(lockfile: &Lockfile) -> Result<()> { + let mut value = + serde_json::to_value(lockfile).context("Failed to serialize lockfile to JSON value")?; + sort_json_object(&mut value); + let json = + serde_json::to_string_pretty(&value).context("Failed to serialize lockfile to JSON")?; + println!("{}", json); + Ok(()) +} + +/// Recursively sort all keys in a JSON object for deterministic output. +/// Kept private here to mirror `deacon_core::lockfile`'s private helper +/// (same logic; both produce identical orderings). +fn sort_json_object(value: &mut serde_json::Value) { + match value { + serde_json::Value::Object(map) => { + let sorted: std::collections::BTreeMap<_, _> = map.iter().collect(); + *map = sorted + .into_iter() + .map(|(k, v)| { + let mut v = v.clone(); + sort_json_object(&mut v); + (k.clone(), v) + }) + .collect(); + } + serde_json::Value::Array(arr) => { + for item in arr { + sort_json_object(item); + } + } + _ => {} + } +} + +#[cfg(test)] +mod tests { + use super::*; + use deacon_core::features::FeatureMetadata; + use std::path::PathBuf; + + fn make_downloaded(version: Option<&str>, digest: &str) -> DownloadedFeature { + DownloadedFeature { + path: PathBuf::from("/tmp/unused"), + metadata: FeatureMetadata { + id: "node".to_string(), + version: version.map(|s| s.to_string()), + ..FeatureMetadata::default() + }, + digest: digest.to_string(), + } + } + + fn make_feature_ref() -> FeatureRef { + FeatureRef::new( + "ghcr.io".to_string(), + "devcontainers".to_string(), + "node".to_string(), + Some("1".to_string()), + ) + } + + // ========================================================================= + // Validation + // ========================================================================= + + #[test] + fn validate_pin_pairing_accepts_both_set() { + assert!(validate_pin_flag_pairing(Some("node"), Some("1.2.3")).is_ok()); + } + + #[test] + fn validate_pin_pairing_accepts_both_absent() { + assert!(validate_pin_flag_pairing(None, None).is_ok()); + } + + #[test] + fn validate_pin_pairing_rejects_only_feature() { + let err = validate_pin_flag_pairing(Some("node"), None).unwrap_err(); + assert!( + err.to_string().contains("must be used together"), + "got: {err}" + ); + } + + #[test] + fn validate_pin_pairing_rejects_only_target_version() { + let err = validate_pin_flag_pairing(None, Some("1.2.3")).unwrap_err(); + assert!(err.to_string().contains("must be used together")); + } + + #[test] + fn is_valid_target_version_accepts_x_xy_xyz() { + // Spec §2: `--target-version` regex `^\d+(\.\d+(\.\d+)?)?$`. + assert!(is_valid_target_version("1")); + assert!(is_valid_target_version("1.2")); + assert!(is_valid_target_version("1.2.3")); + assert!(is_valid_target_version("10")); + assert!(is_valid_target_version("10.20.30")); + } + + #[test] + fn is_valid_target_version_rejects_non_numeric_and_extras() { + assert!(!is_valid_target_version("")); + assert!(!is_valid_target_version("v1")); + assert!(!is_valid_target_version("1.2.3.4")); + assert!(!is_valid_target_version("1.")); + assert!(!is_valid_target_version(".1")); + assert!(!is_valid_target_version("1.x.3")); + assert!(!is_valid_target_version("1.2-beta")); + assert!(!is_valid_target_version("latest")); + } + + #[test] + fn validate_target_version_format_surfaces_spec_message() { + let err = validate_target_version_format("v1").unwrap_err(); + // Spec §2 mandates this exact summary string so existing CI + // scripts that grep for it keep working. + assert!(err.to_string().contains("Invalid version 'v1'")); + assert!(err + .to_string() + .contains("Must be in the form of 'x', 'x.y', or 'x.y.z'")); + } + + // ========================================================================= + // Lockfile entry assembly + // ========================================================================= + + #[test] + fn lockfile_entry_uses_metadata_version_when_present() { + let feature_ref = make_feature_ref(); + let downloaded = make_downloaded( + Some("1.6.1"), + "sha256:1111111111111111111111111111111111111111111111111111111111111111", + ); + let entry = lockfile_entry_for(&feature_ref, &downloaded, "ghcr.io/devcontainers/node:1"); + + assert_eq!(entry.version, "1.6.1"); + assert_eq!( + entry.resolved, + "ghcr.io/devcontainers/node@sha256:1111111111111111111111111111111111111111111111111111111111111111" + ); + assert_eq!( + entry.integrity, + "sha256:1111111111111111111111111111111111111111111111111111111111111111" + ); + assert!(entry.depends_on.is_none()); + } + + #[test] + fn lockfile_entry_falls_back_to_tag_when_metadata_version_missing() { + // Mirrors PR-4b's fallback so the two helpers produce identical + // entries for the same input — important for the eventual dedup. + let feature_ref = make_feature_ref(); + let downloaded = make_downloaded( + None, + "sha256:2222222222222222222222222222222222222222222222222222222222222222", + ); + let entry = lockfile_entry_for(&feature_ref, &downloaded, "ghcr.io/devcontainers/node:1"); + assert_eq!(entry.version, "1"); + } + + #[test] + fn lockfile_entry_sorts_depends_on() { + let feature_ref = make_feature_ref(); + let mut downloaded = make_downloaded( + Some("1.0.0"), + "sha256:3333333333333333333333333333333333333333333333333333333333333333", + ); + downloaded + .metadata + .depends_on + .insert("zeta".to_string(), serde_json::Value::Bool(true)); + downloaded + .metadata + .depends_on + .insert("alpha".to_string(), serde_json::Value::Bool(true)); + + let entry = lockfile_entry_for(&feature_ref, &downloaded, "ghcr.io/devcontainers/node:1"); + assert_eq!( + entry.depends_on.as_deref(), + Some(&["alpha".to_string(), "zeta".to_string()][..]) + ); + } + + // ========================================================================= + // Empty config short-circuit (no network needed) + // ========================================================================= + + #[tokio::test] + async fn resolve_lockfile_returns_empty_for_no_features() { + let config = DevContainerConfig::default(); + let lockfile = resolve_lockfile_from_config(&config).await.unwrap(); + assert!(lockfile.features.is_empty()); + } + + #[tokio::test] + async fn resolve_lockfile_returns_empty_for_empty_features_object() { + let config = DevContainerConfig { + features: serde_json::json!({}), + ..DevContainerConfig::default() + }; + let lockfile = resolve_lockfile_from_config(&config).await.unwrap(); + assert!(lockfile.features.is_empty()); + } + + // ========================================================================= + // Args defaults + // ========================================================================= + + #[test] + fn upgrade_args_defaults_are_sensible() { + let args = UpgradeArgs { + workspace_folder: None, + config_path: None, + docker_path: "docker".to_string(), + docker_compose_path: "docker-compose".to_string(), + dry_run: false, + feature: None, + target_version: None, + }; + assert!(!args.dry_run); + assert!(args.feature.is_none()); + assert!(args.target_version.is_none()); + } +}