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..841598e 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, @@ -531,6 +543,7 @@ pub async fn execute_build(args: BuildArgs) -> Result<()> { let mut config = load_result.config; let workspace_folder = load_result.workspace_folder; + let config_path = load_result.config_path; debug!("Loaded configuration: {:?}", config.name); @@ -625,22 +638,37 @@ pub async fn execute_build(args: BuildArgs) -> Result<()> { let config_hash = calculate_config_hash(&build_config, &workspace_folder)?; debug!("Configuration hash: {}", config_hash); - // Fail fast if features are specified (not yet supported) - // This check applies to all build modes (Dockerfile, image-reference, compose) - if !config.features.is_null() + // PR-4c: feature installation during build. + // + // Dockerfile-based builds support features via a post-build layering pass + // (we run the user's `docker build`, then layer a feature-install stage + // on top of the resulting image using `build_image_with_features` — + // the same helper the `up` command uses, which also produces a lockfile). + // + // Compose and image-reference builds still fail fast: they need different + // integration patterns (compose-build is workspace-of-services, image-ref + // wraps a pre-built upstream image with a synthetic Dockerfile) and the + // work to support each is tracked separately. + let features_present = !config.features.is_null() && config .features .as_object() - .is_some_and(|obj| !obj.is_empty()) - { + .is_some_and(|obj| !obj.is_empty()); + if features_present && (config.uses_compose() || config.image.is_some()) { return Err(anyhow!( - "Feature installation during build is not yet implemented. \ - Remove features from devcontainer.json or use 'deacon up' which will apply features after build." + "Feature installation during build is not yet supported for compose or \ + image-reference configurations. Use a Dockerfile-based build, or run \ + 'deacon up' which supports all configurations." )); } - // Check cache if not forced (skip cache if pushing or exporting) - if !args.force && !args.push && args.output.is_none() { + // Check cache if not forced (skip cache if pushing or exporting). + // When features are present we deliberately skip the cache check: the + // current `config_hash` does not include feature digests, so a cached + // hit would point at a base image without the feature layers. + // Re-running keeps correctness; a future refinement can fold the + // feature digests into the hash for proper caching. + if !args.force && !args.push && args.output.is_none() && !features_present { if let Some(cached_result) = check_build_cache(&config_hash, &workspace_folder).await? { info!("Using cached build result"); output_result( @@ -704,14 +732,31 @@ pub async fn execute_build(args: BuildArgs) -> Result<()> { tracker.record_duration("build", build_duration); } } + + // PR-4c: post-build feature install. When features are present we run a + // second BuildKit pass that FROMs the just-built image and layers the + // feature install stages on top, then write the lockfile next to the + // config file. This matches `up`'s feature flow (`build_image_with_features`) + // which also returns a `Lockfile` ready for `write_lockfile`. + let (image_id, feature_lockfile) = if features_present { + apply_features_and_lockfile(&config, &result.image_id, &workspace_folder, &config_path) + .await? + } else { + (result.image_id, None) + }; + let final_result = BuildResult { - image_id: result.image_id, + image_id, tags: result.tags, build_duration: build_duration.as_secs_f64(), metadata: result.metadata, config_hash: config_hash.clone(), }; + if let Some(path) = feature_lockfile { + debug!("Wrote feature lockfile to '{}'", path.display()); + } + // Cache the result cache_build_result(&final_result, &workspace_folder).await?; @@ -1310,6 +1355,111 @@ async fn execute_image_reference_build( result } +/// PR-4c: layer features on top of the just-built image and write the +/// lockfile next to the config file. +/// +/// Synthesizes a config that points at the just-built `image_id` and reuses +/// `up`'s `build_image_with_features` helper to: +/// 1. Resolve + download every feature declared in `config.features`, +/// 2. Generate an extension Dockerfile (`FROM AS dev_containers_target_stage` +/// + one BuildKit RUN-mount per feature), +/// 3. Build the extended image via `docker buildx build`, +/// 4. Hand back a `FeatureBuildOutput` whose `lockfile` field is keyed by +/// the user-provided feature ID (matching upstream `generateLockfile`). +/// +/// The returned `Lockfile` is then written to disk via +/// `deacon_core::lockfile::write_lockfile(force_init = true)` — same path +/// + format as `up`'s post-build lockfile write (PR-4b). On read-only +/// workspaces (`EROFS`/`EACCES`) the write is downgraded to a WARN so a +/// read-only CI mount doesn't fail the build. +/// +/// Returns `(new_image_id, Some(lockfile_path))` on success. +#[instrument(skip(config))] +async fn apply_features_and_lockfile( + config: &DevContainerConfig, + built_image_id: &str, + workspace_folder: &Path, + config_path: &Path, +) -> Result<(String, Option)> { + use crate::commands::up::features_build::build_image_with_features; + use deacon_core::container::ContainerIdentity; + use deacon_core::lockfile::{get_lockfile_path, write_lockfile}; + + info!( + built_image = %built_image_id, + "Layering features on top of build output" + ); + + // Synthesize a single-container config that points at the just-built + // image. `build_image_with_features` reads `config.image`, + // `config.features`, and `config.override_feature_install_order`; other + // fields are ignored, so cloning + retargeting `image` is sufficient. + let mut synth_config = config.clone(); + synth_config.image = Some(built_image_id.to_string()); + + // Namespace the produced tag by workspace+config so it does not collide + // with `up`'s feature-extended images on the same host. + let mut identity = ContainerIdentity::new(workspace_folder, &synth_config); + identity.workspace_hash = format!("{}-build", identity.workspace_hash); + + let feature_build = build_image_with_features(&synth_config, &identity, workspace_folder, None) + .await + .context("Failed to build feature-extended image from build output")?; + + info!( + feature_image = %feature_build.image_tag, + "Successfully built feature-extended image" + ); + + // Write the lockfile next to the config file (spec §6 naming rule). + let lockfile_path = get_lockfile_path(config_path); + let written = match write_lockfile(&lockfile_path, &feature_build.lockfile, true) { + Ok(()) => Some(lockfile_path), + Err(e) if is_readonly_filesystem_error(&e) => { + warn!( + path = %lockfile_path.display(), + error = %e, + "Lockfile write skipped (read-only workspace); continuing without persisting lockfile" + ); + None + } + Err(e) => { + return Err(e).with_context(|| { + format!("Failed to write lockfile to '{}'", lockfile_path.display()) + }); + } + }; + + Ok((feature_build.image_tag, written)) +} + +/// Mirror of `up::helpers::is_readonly_fs_error` for build's write path. We +/// don't share the helper today because `up::helpers` is private to `up`; +/// a future cleanup can lift both helpers into a shared module. +fn is_readonly_filesystem_error(err: &anyhow::Error) -> bool { + use std::io; + // Linux EROFS (28 on most arches, 30 on Linux) — checked via raw errno + // because `io::ErrorKind::ReadOnlyFilesystem` requires Rust 1.83 and our + // MSRV is 1.82. + #[cfg(unix)] + const EROFS: i32 = 30; + 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 + }) +} + /// Execute Docker build #[instrument(skip(build_config, args, workspace_folder, labels))] async fn execute_docker_build( @@ -2880,4 +3030,32 @@ mod tests { let value = secret.read_value().await.unwrap(); assert_eq!(value, "my_secret_token"); } + + // ========================================================================= + // PR-4c: features-during-build (helpers tested in isolation) + // ========================================================================= + + #[test] + fn is_readonly_filesystem_error_detects_permission_denied() { + // EACCES surfaces as PermissionDenied — the most common cause of + // a "can't write the lockfile" path on container CI mounts. + let inner = std::io::Error::from(std::io::ErrorKind::PermissionDenied); + let err: anyhow::Error = anyhow::anyhow!(inner).context("write failed"); + assert!(super::is_readonly_filesystem_error(&err)); + } + + #[test] + fn is_readonly_filesystem_error_ignores_unrelated_io_errors() { + // Other IO errors (NotFound, BrokenPipe, etc.) must propagate — + // downgrading them all would hide real bugs. + let inner = std::io::Error::from(std::io::ErrorKind::NotFound); + let err: anyhow::Error = anyhow::anyhow!(inner).context("read failed"); + assert!(!super::is_readonly_filesystem_error(&err)); + } + + #[test] + fn is_readonly_filesystem_error_ignores_non_io_errors() { + let err = anyhow::anyhow!("not an io error"); + assert!(!super::is_readonly_filesystem_error(&err)); + } } 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/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)); + } +} diff --git a/crates/deacon/src/commands/up/mod.rs b/crates/deacon/src/commands/up/mod.rs index 88e99a7..5a4ddcd 100644 --- a/crates/deacon/src/commands/up/mod.rs +++ b/crates/deacon/src/commands/up/mod.rs @@ -22,7 +22,7 @@ mod args; mod compose; mod container; mod dotfiles; -mod features_build; +pub(crate) mod features_build; mod helpers; mod image_build; mod lifecycle; @@ -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!(