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 03f8cca393a61c59c0acdf0d3cbebd55d0cd6b00 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Mon, 25 May 2026 13:54:03 +0000 Subject: [PATCH 2/2] =?UTF-8?q?feat:=20lockfile=20graduation=20=E2=80=94?= =?UTF-8?q?=20wire=20writer=20in=20up's=20feature=20pipeline=20(PR-4b)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR-4a (#33): the user-visible flag surface, schema parity fix, and upstream-aligned error strings landed there, but the actual writer was never invoked. This wires it in. After feature resolution + download, build a Lockfile from the resolved feature set (`build_lockfile_from_features` mirrors upstream `generateLockfile`), thread it back via FeatureBuildOutput, then: - default: write `{config_dir}/devcontainer-lock.json` (sorted by key, trailing newline — byte-identical to upstream `writeLockfile`) - `--frozen-lockfile`: byte-compare against on-disk; fail with the upstream summary strings (`"Lockfile does not exist."` / `"Lockfile does not match."`) so existing CI scripts keep working - `--no-lockfile`: skip entirely - deprecated `--experimental-lockfile `: still honored for the custom-path form (hidden alias path through the 1.x line) Wired into both single-container (`container.rs`) and compose (`compose.rs`) flows via shared `handle_lockfile_post_build` helper. EROFS/EACCES on the write path downgrades to a WARN so read-only workspaces (CI mounts, read-only volumes) don't break `up`. Lockfile keys are the user-provided feature ID (e.g. `ghcr.io/devcontainers/features/node:1`), not the canonical no-tag form — matching upstream and keeping the existing pre-build structural validation aligned. Build (`crates/deacon/src/commands/build/mod.rs`) is intentionally out of scope — see issue #32 and the standing TODO at build/mod.rs:1285. That's PR-4c. Closes #32 (PR-4 phase 2 of the lockfile graduation track). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/deacon/src/commands/up/compose.rs | 13 +- crates/deacon/src/commands/up/container.rs | 8 +- .../deacon/src/commands/up/features_build.rs | 294 ++++++++++++- crates/deacon/src/commands/up/helpers.rs | 391 +++++++++++++++++- 4 files changed, 699 insertions(+), 7 deletions(-) diff --git a/crates/deacon/src/commands/up/compose.rs b/crates/deacon/src/commands/up/compose.rs index e3bd175..b8fd077 100644 --- a/crates/deacon/src/commands/up/compose.rs +++ b/crates/deacon/src/commands/up/compose.rs @@ -9,6 +9,7 @@ use super::args::{MountType, NormalizedMount, UpArgs}; use super::features_build::{ build_image_with_features, build_image_with_features_from_dockerfile, FeatureBuildOutput, }; +use super::helpers::handle_lockfile_post_build; use super::lifecycle::{execute_initialize_command, resolve_force_pty}; use super::merged_config::{ build_merged_configuration_with_options, inspect_for_merged_configuration, @@ -253,10 +254,8 @@ pub(crate) async fn execute_compose_up( // feature-extended image and rewriting the target service's `image:` via // the existing injection override. Both the `image:` shape (14a) and the // `build:` shape (14b — user-authored Dockerfile + context) are supported. - // The returned FeatureBuildOutput is currently unused — combined feature env - // is already pulled into the compose flow via the existing additional_env path. // Future work (per spec): thread resolved_features into merged_configuration. - let _feature_build = install_features_for_compose( + let feature_build = install_features_for_compose( config, &compose_manager, &mut project, @@ -265,6 +264,14 @@ pub(crate) async fn execute_compose_up( ) .await?; + // Lockfile graduation (PR-4b): mirror the single-container flow — write + // the lockfile to disk, or byte-compare it in `--frozen-lockfile` mode. + // Only runs when features were actually built (the compose path returns + // `None` when no features are declared). + if let Some(ref fb) = feature_build { + handle_lockfile_post_build(args, config_path, &fb.lockfile)?; + } + // Start the compose project // First, warn about security options that cannot be applied dynamically ComposeCommand::warn_security_options_for_compose(config); diff --git a/crates/deacon/src/commands/up/container.rs b/crates/deacon/src/commands/up/container.rs index 2f2edac..cf01610 100644 --- a/crates/deacon/src/commands/up/container.rs +++ b/crates/deacon/src/commands/up/container.rs @@ -6,7 +6,7 @@ use super::args::UpArgs; use super::features_build::build_image_with_features; -use super::helpers::apply_user_mapping; +use super::helpers::{apply_user_mapping, handle_lockfile_post_build}; use super::lifecycle::{execute_initialize_command, execute_lifecycle_commands}; use super::merged_config::{ build_merged_configuration_with_options, inspect_for_merged_configuration, @@ -216,6 +216,12 @@ pub(crate) async fn execute_container_up( feature_build.image_tag ); + // Lockfile graduation (PR-4b): write the freshly-built lockfile to disk + // (or byte-compare it in `--frozen-lockfile` mode). Runs only when + // features were actually resolved; with no features there is nothing + // to lock. + handle_lockfile_post_build(args, config_path, &feature_build.lockfile)?; + Some(feature_build.resolved_features) } else { None diff --git a/crates/deacon/src/commands/up/features_build.rs b/crates/deacon/src/commands/up/features_build.rs index 0443348..e2e7440 100644 --- a/crates/deacon/src/commands/up/features_build.rs +++ b/crates/deacon/src/commands/up/features_build.rs @@ -16,12 +16,13 @@ use deacon_core::errors::DeaconError; use deacon_core::features::{ FeatureDependencyResolver, InstallationPlan, OptionValue, ResolvedFeature, }; +use deacon_core::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::io::Write; use std::path::{Path, PathBuf}; -use tracing::{debug, info, instrument}; +use tracing::{debug, info, instrument, warn}; /// Output from building an image with features #[derive(Debug, Clone)] @@ -32,6 +33,10 @@ pub(crate) struct FeatureBuildOutput { pub combined_env: HashMap, /// Resolved features in installation order pub resolved_features: Vec, + /// Lockfile entries for the features installed in this build. + /// Keyed by the user-provided feature ID (as it appears in `devcontainer.json`). + /// Empty when the config has no features. + pub lockfile: Lockfile, } /// Internal: result of resolving + downloading + staging features for a build. @@ -40,6 +45,11 @@ struct StagedFeatures { combined_env: HashMap, temp_dir: PathBuf, features_source_dir: PathBuf, + /// Lockfile assembled from the resolved + downloaded features. + /// Keyed by the user-provided feature ID (as it appears in + /// `devcontainer.json`), matching upstream `generateLockfile` in + /// `devcontainers/cli` `src/spec-configuration/lockfile.ts`. + lockfile: Lockfile, } /// Build an extended Docker image with features installed using BuildKit. @@ -89,6 +99,9 @@ pub(crate) async fn build_image_with_features( image_tag: base_image.clone(), combined_env: HashMap::new(), resolved_features: Vec::new(), + lockfile: Lockfile { + features: HashMap::new(), + }, }); } @@ -131,6 +144,7 @@ pub(crate) async fn build_image_with_features( image_tag: extended_image_tag, combined_env: staged.combined_env, resolved_features: staged.plan.features.clone(), + lockfile: staged.lockfile, }) } @@ -314,6 +328,7 @@ pub(crate) async fn build_image_with_features_from_dockerfile( image_tag: extended_image_tag, combined_env: staged.combined_env, resolved_features: staged.plan.features.clone(), + lockfile: staged.lockfile, }) } @@ -337,6 +352,10 @@ async fn resolve_and_stage_features( // Parse and fetch features let mut feature_refs: Vec<(String, FeatureRef)> = Vec::new(); let mut feature_options_map: HashMap> = HashMap::new(); + // Canonical id (registry/namespace/name, no tag) → user-provided feature ID + // (the key as it appears in `devcontainer.json`). The lockfile MUST be + // keyed by the user-provided form to match upstream `generateLockfile`. + let mut user_id_by_canonical: HashMap = HashMap::new(); for (feature_id, feature_options) in features_obj.iter() { let (registry_url, namespace, name, tag) = @@ -350,6 +369,8 @@ async fn resolve_and_stage_features( feature_ref.registry, feature_ref.namespace, feature_ref.name ); + user_id_by_canonical.insert(canonical_id.clone(), feature_id.clone()); + let options = if let Some(opts_obj) = feature_options.as_object() { opts_obj .iter() @@ -471,14 +492,92 @@ async fn resolve_and_stage_features( } } + let lockfile = + build_lockfile_from_features(&feature_refs, &downloaded_features, &user_id_by_canonical); + Ok(StagedFeatures { plan: installation_plan, combined_env, temp_dir, features_source_dir: features_dir, + lockfile, }) } +/// Assemble the canonical lockfile from resolved + downloaded features. +/// +/// Mirrors upstream `generateLockfile` in `devcontainers/cli` +/// `src/spec-configuration/lockfile.ts`: +/// - Keys: the user-provided feature ID (as written in `devcontainer.json`). +/// - `resolved`: `{registry}/{repository}@{digest}` via +/// [`LockfileFeature::from_resolved`]. +/// - `integrity`: the manifest digest. +/// - `dependsOn`: alphabetically-sorted vec of dependency keys taken +/// verbatim from `metadata.dependsOn`, or `None` when empty. +/// +/// Features whose metadata lacks a version field fall back to the tag from +/// the user reference (e.g. `"1"`) and ultimately to `"0.0.0"` so the +/// schema's semver validation never blocks lockfile assembly. A WARN log is +/// emitted so the gap is visible in CI output. +fn build_lockfile_from_features( + feature_refs: &[(String, FeatureRef)], + downloaded_features: &HashMap, + user_id_by_canonical: &HashMap, +) -> Lockfile { + let mut entries: HashMap = HashMap::new(); + + for (canonical_id, feature_ref) in feature_refs { + let Some(downloaded) = downloaded_features.get(canonical_id) else { + // Should never happen — the caller populated downloaded_features + // from the same feature_refs vec. If it does, skip rather than + // silently emit a half-valid entry. + warn!( + feature = %canonical_id, + "Skipping lockfile entry: downloaded feature missing from map" + ); + continue; + }; + + let user_id = user_id_by_canonical + .get(canonical_id) + .cloned() + .unwrap_or_else(|| canonical_id.clone()); + + 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) + }; + + let entry = LockfileFeature::from_resolved( + &feature_ref.registry, + &feature_ref.repository(), + &downloaded.digest, + version, + depends_on, + ); + + entries.insert(user_id, entry); + } + + Lockfile { features: entries } +} + fn ensure_buildkit_or_error() -> Result<()> { use deacon_core::build::buildkit::is_buildkit_available; if !is_buildkit_available()? { @@ -521,3 +620,196 @@ pub(crate) fn copy_dir_all(src: impl AsRef, dst: impl AsRef) -> std: } Ok(()) } + +#[cfg(test)] +mod lockfile_assembly_tests { + use super::*; + use deacon_core::features::FeatureMetadata; + + 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_downloaded_with_deps(version: &str, digest: &str, deps: &[&str]) -> DownloadedFeature { + let mut depends_on = HashMap::new(); + for d in deps { + depends_on.insert(d.to_string(), serde_json::Value::Bool(true)); + } + DownloadedFeature { + path: PathBuf::from("/tmp/unused"), + metadata: FeatureMetadata { + id: "node".to_string(), + version: Some(version.to_string()), + depends_on, + ..FeatureMetadata::default() + }, + digest: digest.to_string(), + } + } + + #[test] + fn build_lockfile_keys_by_user_provided_id() { + // Mirrors upstream `generateLockfile`: the lockfile key is the + // user-provided feature ID, not the canonical (no-tag) form. + let feature_ref = FeatureRef::new( + "ghcr.io".to_string(), + "devcontainers".to_string(), + "node".to_string(), + Some("1".to_string()), + ); + let canonical = "ghcr.io/devcontainers/node".to_string(); + let user_id = "ghcr.io/devcontainers/node:1".to_string(); + + let mut downloaded_features = HashMap::new(); + downloaded_features.insert( + canonical.clone(), + make_downloaded( + Some("1.6.1"), + "sha256:1111111111111111111111111111111111111111111111111111111111111111", + ), + ); + + let mut user_id_by_canonical = HashMap::new(); + user_id_by_canonical.insert(canonical.clone(), user_id.clone()); + + let lockfile = build_lockfile_from_features( + &[(canonical, feature_ref)], + &downloaded_features, + &user_id_by_canonical, + ); + + assert_eq!(lockfile.features.len(), 1); + let entry = lockfile + .features + .get(&user_id) + .expect("lockfile must be keyed by the user-provided feature ID"); + 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 build_lockfile_falls_back_to_tag_when_version_missing() { + // Some features ship without a `version` in their metadata; rather + // than block lockfile generation we fall back to the tag the user + // requested (e.g. "1"). This is best-effort — the WARN is the + // observable signal that something is off. + let feature_ref = FeatureRef::new( + "ghcr.io".to_string(), + "x".to_string(), + "y".to_string(), + Some("3".to_string()), + ); + let canonical = "ghcr.io/x/y".to_string(); + let user_id = "ghcr.io/x/y:3".to_string(); + + let mut downloaded_features = HashMap::new(); + downloaded_features.insert( + canonical.clone(), + make_downloaded( + None, + "sha256:2222222222222222222222222222222222222222222222222222222222222222", + ), + ); + + let mut user_id_by_canonical = HashMap::new(); + user_id_by_canonical.insert(canonical.clone(), user_id.clone()); + + let lockfile = build_lockfile_from_features( + &[(canonical, feature_ref)], + &downloaded_features, + &user_id_by_canonical, + ); + + // Tag was "3", so that's the version used for the lockfile entry. + // Note: "3" is not valid semver, so a subsequent `write_lockfile` + // call would fail validation — but the assembly itself is best-effort. + let entry = lockfile.features.get(&user_id).unwrap(); + assert_eq!(entry.version, "3"); + } + + #[test] + fn build_lockfile_sorts_depends_on_alphabetically() { + // Upstream `generateLockfile` sorts `dependsOn` so byte-identical + // output is stable across runs and across implementations. + let feature_ref = FeatureRef::new( + "ghcr.io".to_string(), + "x".to_string(), + "y".to_string(), + Some("1".to_string()), + ); + let canonical = "ghcr.io/x/y".to_string(); + let user_id = "ghcr.io/x/y:1".to_string(); + + let mut downloaded_features = HashMap::new(); + downloaded_features.insert( + canonical.clone(), + make_downloaded_with_deps( + "1.0.0", + "sha256:3333333333333333333333333333333333333333333333333333333333333333", + &["zeta", "alpha", "mu"], + ), + ); + + let mut user_id_by_canonical = HashMap::new(); + user_id_by_canonical.insert(canonical.clone(), user_id.clone()); + + let lockfile = build_lockfile_from_features( + &[(canonical, feature_ref)], + &downloaded_features, + &user_id_by_canonical, + ); + + let entry = lockfile.features.get(&user_id).unwrap(); + let deps = entry.depends_on.as_ref().unwrap(); + assert_eq!(deps, &["alpha", "mu", "zeta"]); + } + + #[test] + fn build_lockfile_omits_empty_depends_on() { + let feature_ref = FeatureRef::new( + "ghcr.io".to_string(), + "x".to_string(), + "y".to_string(), + Some("1".to_string()), + ); + let canonical = "ghcr.io/x/y".to_string(); + let user_id = "ghcr.io/x/y:1".to_string(); + + let mut downloaded_features = HashMap::new(); + downloaded_features.insert( + canonical.clone(), + make_downloaded( + Some("1.0.0"), + "sha256:4444444444444444444444444444444444444444444444444444444444444444", + ), + ); + + let mut user_id_by_canonical = HashMap::new(); + user_id_by_canonical.insert(canonical.clone(), user_id.clone()); + + let lockfile = build_lockfile_from_features( + &[(canonical, feature_ref)], + &downloaded_features, + &user_id_by_canonical, + ); + + let entry = lockfile.features.get(&user_id).unwrap(); + assert!(entry.depends_on.is_none()); + } +} diff --git a/crates/deacon/src/commands/up/helpers.rs b/crates/deacon/src/commands/up/helpers.rs index e946b09..b7673fd 100644 --- a/crates/deacon/src/commands/up/helpers.rs +++ b/crates/deacon/src/commands/up/helpers.rs @@ -4,12 +4,23 @@ //! - `check_for_disallowed_features` - Check for disallowed features //! - `discover_id_labels_from_config` - Discover id-labels from configuration //! - `apply_user_mapping` - Apply user mapping configuration +//! - `handle_lockfile_post_build` - Write/compare lockfile after a feature build -use anyhow::Result; +use anyhow::{Context, Result}; use deacon_core::config::DevContainerConfig; use deacon_core::errors::DeaconError; +use deacon_core::lockfile::{get_lockfile_path, write_lockfile, Lockfile}; +use std::io; use std::path::Path; -use tracing::{debug, instrument, warn}; +use tracing::{debug, info, instrument, warn}; + +use super::args::UpArgs; + +/// Linux `EROFS` errno value — "Read-only file system". Used in the +/// best-effort lockfile write path because `io::ErrorKind::ReadOnlyFilesystem` +/// is unavailable on MSRV 1.82 (stabilized in 1.83). +#[cfg(unix)] +const EROFS: i32 = 30; /// Check if any features are disallowed and return an error if found. /// @@ -224,3 +235,379 @@ pub(crate) async fn apply_user_mapping`): +/// +/// - `--no-lockfile`: skip entirely. +/// - `--frozen-lockfile` (or `--experimental-lockfile` set): serialize the +/// freshly-built lockfile, byte-compare it to the on-disk file, and fail +/// with the upstream-aligned `"Lockfile does not match."` / +/// `"Lockfile does not exist."` strings if they differ. +/// - Default: write the freshly-built lockfile to disk. +/// +/// On read-only workspaces (EROFS/EACCES on write), emit a WARN and continue +/// so a read-only mount doesn't break `up`. Frozen mode never reaches this +/// branch — it only reads — so this fallback is write-side only. +/// +/// Mirrors upstream `writeLockfile` in `devcontainers/cli` +/// `src/spec-configuration/lockfile.ts` (`PR #1212`). +pub(crate) fn handle_lockfile_post_build( + args: &UpArgs, + config_path: &Path, + lockfile: &Lockfile, +) -> Result<()> { + if args.no_lockfile { + debug!("--no-lockfile set; skipping lockfile write/compare"); + return Ok(()); + } + + let lockfile_path = args + .experimental_lockfile + .clone() + .unwrap_or_else(|| get_lockfile_path(config_path)); + + if args.frozen_lockfile { + compare_lockfile_frozen(&lockfile_path, lockfile) + } else { + write_lockfile_best_effort(&lockfile_path, lockfile) + } +} + +/// Frozen-mode comparison: serialize the in-memory lockfile to the same +/// canonical byte form `write_lockfile` would emit, then compare byte-for-byte +/// with the on-disk file. Any deviation (missing file, mismatched bytes) +/// fails the build with the upstream-aligned summary string. +fn compare_lockfile_frozen(lockfile_path: &Path, lockfile: &Lockfile) -> Result<()> { + if !lockfile_path.exists() { + return Err( + DeaconError::Config(deacon_core::errors::ConfigError::Validation { + message: format!( + "Lockfile does not exist.\nExpected at '{}'.\n\ + Run without --frozen-lockfile to generate a lockfile, or \ + generate one with `deacon upgrade`.", + lockfile_path.display() + ), + }) + .into(), + ); + } + + let expected_bytes = canonical_lockfile_bytes(lockfile)?; + let actual_bytes = std::fs::read(lockfile_path).with_context(|| { + format!( + "Failed to read existing lockfile at '{}'", + lockfile_path.display() + ) + })?; + + if expected_bytes != actual_bytes { + return Err( + DeaconError::Config(deacon_core::errors::ConfigError::Validation { + message: format!( + "Lockfile does not match.\n\ + The on-disk lockfile at '{}' differs from the freshly-resolved feature set.\n\ + Run without --frozen-lockfile to update the lockfile, or run `deacon upgrade`.", + lockfile_path.display() + ), + }) + .into(), + ); + } + + info!( + "Lockfile up-to-date: '{}' matches the resolved feature set", + lockfile_path.display() + ); + Ok(()) +} + +/// Best-effort write: succeeds normally, but downgrades EROFS/EACCES to WARN +/// so a read-only workspace (e.g. CI mount, container with a read-only volume) +/// doesn't break `up`. All other write errors propagate. +fn write_lockfile_best_effort(lockfile_path: &Path, lockfile: &Lockfile) -> Result<()> { + match write_lockfile(lockfile_path, lockfile, true) { + Ok(()) => { + debug!("Wrote lockfile to '{}'", lockfile_path.display()); + Ok(()) + } + Err(e) => { + if is_readonly_fs_error(&e) { + warn!( + path = %lockfile_path.display(), + error = %e, + "Lockfile write skipped (read-only workspace); continuing without persisting lockfile" + ); + Ok(()) + } else { + Err(e).with_context(|| { + format!("Failed to write lockfile to '{}'", lockfile_path.display()) + }) + } + } + } +} + +/// Inspect an anyhow error chain for an `io::Error` whose kind indicates a +/// read-only / permission-denied filesystem. +/// +/// `EACCES` surfaces as `io::ErrorKind::PermissionDenied`. `EROFS` is checked +/// via `raw_os_error()` because the dedicated `ErrorKind::ReadOnlyFilesystem` +/// variant was stabilized in Rust 1.83 and our MSRV is 1.82. +fn is_readonly_fs_error(err: &anyhow::Error) -> bool { + err.chain().any(|cause| { + let Some(io_err) = cause.downcast_ref::() else { + return false; + }; + if io_err.kind() == io::ErrorKind::PermissionDenied { + return true; + } + #[cfg(unix)] + { + if io_err.raw_os_error() == Some(EROFS) { + return true; + } + } + false + }) +} + +/// Canonical lockfile bytes — exactly what `write_lockfile` would put on disk. +/// +/// We can't ask `write_lockfile` for the bytes directly (it writes through to +/// the filesystem), so we replay the same shape: `serde_json::to_value`, +/// recursively sort object keys, pretty-print with 2-space indent, append a +/// trailing newline. Any change to the on-disk format must update both this +/// helper and `deacon_core::lockfile::write_lockfile` in lockstep, otherwise +/// `--frozen-lockfile` will report spurious mismatches. +fn canonical_lockfile_bytes(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 mut json = + serde_json::to_string_pretty(&value).context("Failed to serialize lockfile to JSON")?; + json.push('\n'); + Ok(json.into_bytes()) +} + +/// Recursively sort all keys in a JSON object for deterministic output. +/// +/// Kept private to this module to mirror the private helper inside +/// `deacon_core::lockfile`; 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 lockfile_post_build_tests { + use super::*; + use deacon_core::lockfile::{read_lockfile, LockfileFeature}; + use std::collections::HashMap; + use tempfile::TempDir; + + /// Build a lockfile with a single deterministic entry for assertions. + fn one_feature_lockfile(version: &str, digest_hex: &str) -> Lockfile { + let mut features = HashMap::new(); + features.insert( + "ghcr.io/devcontainers/features/node:1".to_string(), + LockfileFeature { + version: version.to_string(), + resolved: format!("ghcr.io/devcontainers/features/node@sha256:{}", digest_hex), + integrity: format!("sha256:{}", digest_hex), + depends_on: None, + }, + ); + Lockfile { features } + } + + fn make_args(no_lockfile: bool, frozen_lockfile: bool) -> UpArgs { + UpArgs { + no_lockfile, + frozen_lockfile, + ..UpArgs::default() + } + } + + /// `canonical_lockfile_bytes` MUST match what `write_lockfile` actually + /// puts on disk — otherwise `--frozen-lockfile` would report spurious + /// mismatches because the two paths produce different bytes. + #[test] + fn canonical_bytes_match_write_lockfile_output() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("devcontainer-lock.json"); + + let lockfile = one_feature_lockfile("1.6.1", &"a".repeat(64)); + + write_lockfile(&path, &lockfile, true).expect("write_lockfile"); + let on_disk = std::fs::read(&path).unwrap(); + let in_memory = canonical_lockfile_bytes(&lockfile).expect("canonicalize"); + + assert_eq!( + in_memory, on_disk, + "canonical_lockfile_bytes diverged from write_lockfile output; \ + --frozen-lockfile would report spurious mismatches" + ); + } + + /// `--no-lockfile` short-circuits the helper entirely: no read, no write, + /// no comparison, even in `--frozen-lockfile` (the two are mutually + /// exclusive at the CLI layer, but the helper is defensive). + #[test] + fn no_lockfile_flag_skips_all_io() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + let lockfile = one_feature_lockfile("1.0.0", &"b".repeat(64)); + + let args = make_args(true, false); + handle_lockfile_post_build(&args, &config_path, &lockfile).expect("no-lockfile path"); + + let derived = get_lockfile_path(&config_path); + assert!( + !derived.exists(), + "--no-lockfile must not write the lockfile to disk" + ); + } + + /// Default mode writes the lockfile next to the config file, sorted by + /// key with a trailing newline (validated downstream by parity tests in + /// `deacon_core::lockfile`). + #[test] + fn default_mode_writes_lockfile_next_to_config() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + let lockfile = one_feature_lockfile("1.0.0", &"c".repeat(64)); + + let args = make_args(false, false); + handle_lockfile_post_build(&args, &config_path, &lockfile).expect("default-write path"); + + let derived = get_lockfile_path(&config_path); + let on_disk = read_lockfile(&derived).expect("read_lockfile").unwrap(); + assert_eq!(on_disk, lockfile); + } + + /// Frozen mode against a missing lockfile fails with the upstream string + /// `"Lockfile does not exist."` so CI scripts that match on the message + /// keep working. + #[test] + fn frozen_mode_missing_lockfile_fails_with_upstream_string() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + let lockfile = one_feature_lockfile("1.0.0", &"d".repeat(64)); + + let args = make_args(false, true); + let err = handle_lockfile_post_build(&args, &config_path, &lockfile) + .expect_err("frozen + missing must fail"); + let msg = format!("{:#}", err); + assert!( + msg.contains("Lockfile does not exist."), + "expected upstream-aligned summary, got: {msg}" + ); + } + + /// Frozen mode with a byte-identical existing lockfile succeeds. + #[test] + fn frozen_mode_matches_existing_lockfile() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + let lockfile = one_feature_lockfile("1.0.0", &"e".repeat(64)); + + // Seed the on-disk file with the canonical form. + let derived = get_lockfile_path(&config_path); + write_lockfile(&derived, &lockfile, true).unwrap(); + + let args = make_args(false, true); + handle_lockfile_post_build(&args, &config_path, &lockfile) + .expect("frozen + matching must succeed"); + } + + /// Frozen mode with a mismatched on-disk lockfile fails with the upstream + /// string `"Lockfile does not match."`. + #[test] + fn frozen_mode_mismatch_fails_with_upstream_string() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + + // On disk: version 1.0.0 + let stale = one_feature_lockfile("1.0.0", &"f".repeat(64)); + let derived = get_lockfile_path(&config_path); + write_lockfile(&derived, &stale, true).unwrap(); + + // Freshly resolved: version 2.0.0 — should NOT match. + let fresh = one_feature_lockfile("2.0.0", &"f".repeat(64)); + + let args = make_args(false, true); + let err = handle_lockfile_post_build(&args, &config_path, &fresh) + .expect_err("frozen + mismatch must fail"); + let msg = format!("{:#}", err); + assert!( + msg.contains("Lockfile does not match."), + "expected upstream-aligned summary, got: {msg}" + ); + } + + /// When the deprecated `--experimental-lockfile ` is set, the helper + /// uses that explicit path instead of deriving from the config file. + #[test] + fn experimental_lockfile_path_overrides_derivation() { + let tmp = TempDir::new().unwrap(); + let config_path = tmp.path().join(".devcontainer/devcontainer.json"); + std::fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + + let explicit = tmp.path().join("custom-lockfile.json"); + let lockfile = one_feature_lockfile("1.0.0", &"9".repeat(64)); + + let mut args = make_args(false, false); + args.experimental_lockfile = Some(explicit.clone()); + + handle_lockfile_post_build(&args, &config_path, &lockfile) + .expect("explicit path must write"); + + assert!( + explicit.exists(), + "experimental_lockfile path must be honored over derived path" + ); + assert!( + !get_lockfile_path(&config_path).exists(), + "derived path must NOT be written when explicit path is set" + ); + } + + #[test] + fn is_readonly_fs_error_detects_permission_denied() { + let inner = io::Error::from(io::ErrorKind::PermissionDenied); + let err: anyhow::Error = anyhow::anyhow!(inner).context("write failed"); + assert!(is_readonly_fs_error(&err)); + } + + #[test] + fn is_readonly_fs_error_ignores_other_io_errors() { + let inner = io::Error::from(io::ErrorKind::NotFound); + let err: anyhow::Error = anyhow::anyhow!(inner).context("read failed"); + assert!(!is_readonly_fs_error(&err)); + } +}