From 864cb5d8530538ce2cb1cc4aee34b7d4621cde4d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 3 Mar 2026 10:07:23 -0500 Subject: [PATCH] Fix 3 feature spec bugs: shorthand version, option env names, install user vars - Normalize string option values ("1.18") to {"version": "1.18"} so shorthand syntax correctly sets the VERSION env var - Add option_id_to_env_name() to convert option IDs like "node-version" to NODE_VERSION per the dev container spec - Emit _REMOTE_USER, _CONTAINER_USER, _REMOTE_USER_HOME, and _CONTAINER_USER_HOME in feature install snippets, threading remoteUser from devcontainer.json through resolve_features/generate_layer Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/arc-devcontainer/src/features.rs | 158 ++++++++++++++++-- crates/arc-devcontainer/src/lib.rs | 2 +- crates/arc-devcontainer/tests/e2e.rs | 80 +++++++++ .../.devcontainer/devcontainer.json | 7 + .../go-feature/devcontainer-feature.json | 16 ++ .../.devcontainer/go-feature/install.sh | 2 + 6 files changed, 247 insertions(+), 18 deletions(-) create mode 100644 crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/devcontainer.json create mode 100644 crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/devcontainer-feature.json create mode 100644 crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/install.sh diff --git a/crates/arc-devcontainer/src/features.rs b/crates/arc-devcontainer/src/features.rs index 39f505eab..ce0300b7f 100644 --- a/crates/arc-devcontainer/src/features.rs +++ b/crates/arc-devcontainer/src/features.rs @@ -415,29 +415,66 @@ fn topo_sort( sorted } +/// Convert an option ID to an environment variable name per the dev container spec. +/// Replaces non-alphanumeric, non-underscore chars with `_`, strips leading digits/underscores, +/// and uppercases the result. +fn option_id_to_env_name(id: &str) -> String { + let replaced: String = id + .chars() + .map(|c| if c.is_alphanumeric() || c == '_' { c } else { '_' }) + .collect(); + let trimmed = replaced.trim_start_matches(|c: char| c == '_' || c.is_ascii_digit()); + if trimmed.is_empty() { + "_".to_string() + } else { + trimmed.to_uppercase() + } +} + /// Generate a Dockerfile snippet for a single feature layer. fn generate_layer( feature_id: &str, dir_name: &str, options: &serde_json::Value, metadata: &FeatureMetadata, + remote_user: Option<&str>, ) -> String { let mut env_lines = Vec::new(); - // Collect all option names from metadata to set defaults - let user_options: HashMap = match options.as_object() { - Some(obj) => obj - .iter() - .map(|(k, v)| { - let val = match v { - serde_json::Value::String(s) => s.clone(), - other => other.to_string(), - }; - (k.clone(), val) - }) - .collect(), - None => HashMap::new(), + // Emit built-in user env vars expected by community features + let ru = remote_user.unwrap_or("root"); + let ru_home = if ru == "root" { + "/root".to_string() + } else { + format!("/home/{ru}") }; + env_lines.push(format!(" export _REMOTE_USER=\"{ru}\" && \\")); + env_lines.push(" export _CONTAINER_USER=\"root\" && \\".to_string()); + env_lines.push(format!(" export _REMOTE_USER_HOME=\"{ru_home}\" && \\")); + env_lines.push(" export _CONTAINER_USER_HOME=\"/root\" && \\".to_string()); + + // Normalize shorthand version syntax: "1.18" → {"version": "1.18"} + let options_obj = match options { + serde_json::Value::String(s) => { + let mut map = serde_json::Map::new(); + map.insert("version".to_string(), serde_json::Value::String(s.clone())); + map + } + serde_json::Value::Object(obj) => obj.clone(), + _ => serde_json::Map::new(), + }; + + // Collect all option names from metadata to set defaults + let user_options: HashMap = options_obj + .iter() + .map(|(k, v)| { + let val = match v { + serde_json::Value::String(s) => s.clone(), + other => other.to_string(), + }; + (k.clone(), val) + }) + .collect(); // Merge metadata defaults with user-provided options let mut merged_options: Vec<(String, String)> = Vec::new(); @@ -467,7 +504,7 @@ fn generate_layer( merged_options.sort_by(|a, b| a.0.cmp(&b.0)); for (name, value) in &merged_options { - let env_name = name.to_uppercase(); + let env_name = option_id_to_env_name(name); env_lines.push(format!(" export {env_name}=\"{value}\" && \\")); } @@ -492,6 +529,7 @@ fn generate_layer( pub async fn resolve_features( features: &HashMap, devcontainer_dir: &Path, + remote_user: Option<&str>, ) -> crate::Result { if features.is_empty() { return Ok(ResolvedFeatures::default()); @@ -592,7 +630,7 @@ pub async fn resolve_features( resolved.post_start_commands.push(cmd.clone()); } - let dockerfile_snippet = generate_layer(id, &dir_name, &options, &metadata); + let dockerfile_snippet = generate_layer(id, &dir_name, &options, &metadata, remote_user); resolved.layers.push(FeatureLayer { id: id.clone(), dir_name, @@ -869,6 +907,7 @@ mod tests { "node", &options, &metadata, + None, ); assert!(snippet.contains("# Feature: ghcr.io/devcontainers/features/node:1")); @@ -908,6 +947,7 @@ mod tests { "node", &options, &metadata, + None, ); // Default value "lts" should be used @@ -935,12 +975,15 @@ mod tests { "common-utils", &options, &metadata, + None, ); assert!(snippet.contains("# Feature: ghcr.io/devcontainers/features/common-utils:1")); assert!(snippet.contains("COPY common-utils/ /tmp/devcontainer-features/common-utils/")); assert!(snippet.contains("chmod +x install.sh")); - assert!(!snippet.contains("export ")); + // Should have built-in user env vars but no feature-specific options + assert!(snippet.contains("_REMOTE_USER")); + assert!(!snippet.contains("export VERSION")); } #[test] @@ -1048,7 +1091,7 @@ mod tests { "ghcr.io/devcontainers/features/node:1".to_string(), serde_json::json!({"version": "20"}), ); - let resolved = resolve_features(&features, tmp.path()).await.unwrap(); + let resolved = resolve_features(&features, tmp.path(), None).await.unwrap(); assert_eq!(resolved.layers.len(), 1); assert_eq!(resolved.layers[0].dir_name, "node"); assert!(resolved.layers[0].dockerfile_snippet.contains("export VERSION=\"20\"")); @@ -1124,4 +1167,85 @@ mod tests { assert!(matches!(&resolved.post_create_commands[0], LifecycleCommand::Array(arr) if arr.len() == 2)); assert_eq!(resolved.post_start_commands.len(), 1); } + + #[test] + fn option_id_to_env_name_hyphenated() { + assert_eq!(option_id_to_env_name("node-version"), "NODE_VERSION"); + } + + #[test] + fn option_id_to_env_name_leading_digit() { + assert_eq!(option_id_to_env_name("2fast"), "FAST"); + } + + #[test] + fn option_id_to_env_name_simple() { + assert_eq!(option_id_to_env_name("simple"), "SIMPLE"); + } + + #[test] + fn generate_layer_shorthand_version() { + let options = serde_json::json!("20"); + let mut meta_options = HashMap::new(); + meta_options.insert( + "version".to_string(), + FeatureOption { + option_type: Some("string".to_string()), + default: Some(serde_json::Value::String("lts".to_string())), + description: Some("Node.js version".to_string()), + }, + ); + let metadata = FeatureMetadata { + id: Some("node".to_string()), + name: None, + version: None, + options: meta_options, + installs_after: Vec::new(), + depends_on: HashMap::new(), + container_env: HashMap::new(), + on_create_command: None, + post_create_command: None, + post_start_command: None, + }; + + let snippet = generate_layer( + "ghcr.io/devcontainers/features/node:1", + "node", + &options, + &metadata, + None, + ); + + assert!(snippet.contains("export VERSION=\"20\"")); + } + + #[test] + fn generate_layer_install_env_vars() { + let options = serde_json::json!({}); + let metadata = FeatureMetadata { + id: Some("node".to_string()), + name: None, + version: None, + options: HashMap::new(), + installs_after: Vec::new(), + depends_on: HashMap::new(), + container_env: HashMap::new(), + on_create_command: None, + post_create_command: None, + post_start_command: None, + }; + + let snippet = generate_layer( + "ghcr.io/devcontainers/features/node:1", + "node", + &options, + &metadata, + Some("vscode"), + ); + + assert!(snippet.contains("_REMOTE_USER=\"vscode\"")); + assert!(snippet.contains("_CONTAINER_USER=\"root\"")); + assert!(snippet.contains("_REMOTE_USER_HOME=\"/home/vscode\"")); + assert!(snippet.contains("_CONTAINER_USER_HOME=\"/root\"")); + } } diff --git a/crates/arc-devcontainer/src/lib.rs b/crates/arc-devcontainer/src/lib.rs index a02d5757e..7fec99a82 100644 --- a/crates/arc-devcontainer/src/lib.rs +++ b/crates/arc-devcontainer/src/lib.rs @@ -249,7 +249,7 @@ impl DevcontainerResolver { // Features let resolved_features = if !devcontainer.features.is_empty() { - features::resolve_features(&devcontainer.features, base_dir).await? + features::resolve_features(&devcontainer.features, base_dir, devcontainer.remote_user.as_deref()).await? } else { features::ResolvedFeatures::default() }; diff --git a/crates/arc-devcontainer/tests/e2e.rs b/crates/arc-devcontainer/tests/e2e.rs index 753c5465b..8b919cd02 100644 --- a/crates/arc-devcontainer/tests/e2e.rs +++ b/crates/arc-devcontainer/tests/e2e.rs @@ -429,6 +429,86 @@ async fn feature_lifecycle_hooks_appended() { assert!(post_start.contains(&"echo node-started")); } +/// Fix 1: Shorthand version syntax "1.21" is normalized to {"version": "1.21"}. +#[tokio::test] +async fn feature_shorthand_version_syntax() { + let config = DevcontainerResolver::resolve(&fixture_path("feature-options")) + .await + .unwrap(); + + // "1.21" string should become version=1.21 env var + assert!( + config.dockerfile.contains("export VERSION=\"1.21\""), + "shorthand string \"1.21\" should set VERSION env var, got:\n{}", + config.dockerfile, + ); +} + +/// Fix 2: Hyphenated option IDs are converted to valid env var names (node-version → NODE_VERSION). +#[tokio::test] +async fn feature_option_id_hyphen_to_underscore() { + let config = DevcontainerResolver::resolve(&fixture_path("feature-options")) + .await + .unwrap(); + + // node-version default "none" should export as NODE_VERSION (not NODE-VERSION) + assert!( + config.dockerfile.contains("export NODE_VERSION=\"none\""), + "hyphenated option 'node-version' should become NODE_VERSION env var, got:\n{}", + config.dockerfile, + ); + assert!( + !config.dockerfile.contains("NODE-VERSION"), + "NODE-VERSION (with hyphen) should not appear in Dockerfile", + ); +} + +/// Fix 3: _REMOTE_USER and related env vars are emitted in feature install snippets. +#[tokio::test] +async fn feature_install_user_env_vars() { + let config = DevcontainerResolver::resolve(&fixture_path("feature-options")) + .await + .unwrap(); + + // remoteUser is "developer", so _REMOTE_USER should be "developer" + assert!( + config.dockerfile.contains("_REMOTE_USER=\"developer\""), + "_REMOTE_USER should be set to remoteUser value, got:\n{}", + config.dockerfile, + ); + assert!( + config.dockerfile.contains("_CONTAINER_USER=\"root\""), + "_CONTAINER_USER should always be root", + ); + assert!( + config.dockerfile.contains("_REMOTE_USER_HOME=\"/home/developer\""), + "_REMOTE_USER_HOME should be /home/developer", + ); + assert!( + config.dockerfile.contains("_CONTAINER_USER_HOME=\"/root\""), + "_CONTAINER_USER_HOME should always be /root", + ); +} + +/// Fix 3: _REMOTE_USER defaults to root when remoteUser is not set. +#[tokio::test] +async fn feature_install_user_env_vars_default_root() { + let config = DevcontainerResolver::resolve(&fixture_path("local-features")) + .await + .unwrap(); + + // local-features has remoteUser: "vscode" + assert!( + config.dockerfile.contains("_REMOTE_USER=\"vscode\""), + "_REMOTE_USER should be set to vscode, got:\n{}", + config.dockerfile, + ); + assert!( + config.dockerfile.contains("_REMOTE_USER_HOME=\"/home/vscode\""), + "_REMOTE_USER_HOME should be /home/vscode", + ); +} + /// Gap 2+3: Feature ordering affects both containerEnv and lifecycle hook collection. /// python-feature installsAfter node-feature, so node's env/hooks come first. #[tokio::test] diff --git a/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/devcontainer.json b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/devcontainer.json new file mode 100644 index 000000000..6f67c9c6e --- /dev/null +++ b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/devcontainer.json @@ -0,0 +1,7 @@ +{ + "image": "mcr.microsoft.com/devcontainers/base:ubuntu", + "features": { + "./go-feature": "1.21" + }, + "remoteUser": "developer" +} diff --git a/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/devcontainer-feature.json b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/devcontainer-feature.json new file mode 100644 index 000000000..7caf37a37 --- /dev/null +++ b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/devcontainer-feature.json @@ -0,0 +1,16 @@ +{ + "id": "go-feature", + "version": "1.0.0", + "options": { + "version": { + "type": "string", + "default": "latest", + "description": "Go version" + }, + "node-version": { + "type": "string", + "default": "none", + "description": "Optional Node.js version (hyphenated option ID)" + } + } +} diff --git a/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/install.sh b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/install.sh new file mode 100644 index 000000000..5cd5409ed --- /dev/null +++ b/crates/arc-devcontainer/tests/fixtures/feature-options/.devcontainer/go-feature/install.sh @@ -0,0 +1,2 @@ +#!/bin/sh +echo "Installing go ${VERSION} with node ${NODE_VERSION}"