From cff594fa3ecbb0c748a915b53063c8e1a290aa28 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Tue, 14 Apr 2026 22:01:24 +0200 Subject: [PATCH 1/4] Support remaining outdated and upgrade CLI flags --- TODO_ARGS.md | 10 - build/generate-parity-inventory.js | 20 ++ cmd/devcontainer/src/cli.rs | 20 +- cmd/devcontainer/src/cli_metadata.json | 10 +- cmd/devcontainer/src/commands/common.rs | 3 +- cmd/devcontainer/src/commands/common/args.rs | 89 ++++++- .../src/commands/configuration/upgrade.rs | 46 +++- cmd/devcontainer/tests/cli_smoke/lockfile.rs | 218 ++++++++++++++---- docs/upstream/parity-inventory.json | 86 +++++-- docs/upstream/parity-inventory.md | 14 +- 10 files changed, 404 insertions(+), 112 deletions(-) diff --git a/TODO_ARGS.md b/TODO_ARGS.md index 0aacb2840..4af657d0d 100644 --- a/TODO_ARGS.md +++ b/TODO_ARGS.md @@ -5,13 +5,3 @@ Unsupported CLI args for the current pinned upstream command surface. - Upstream commit: `39685cf1aa58b5b11e90085bd32562fad61f4103` - Source: `upstream/src/spec-node/devContainersSpecCLI.ts` -## `outdated` - -- `--log-level`: Log level for the --terminal-log-file. When set to trace, the log level for --log-file will also be set to trace. [choices: "info", "debug", "trace"] [default: "info"] -- `--terminal-columns`: Number of columns to render the output for. This is required for some of the subprocesses to correctly render their output. [number] -- `--terminal-rows`: Number of rows to render the output for. This is required for some of the subprocesses to correctly render their output. [number] - -## `upgrade` - -- `--log-level`: Log level. [choices: "error", "info", "debug", "trace"] [default: "info"] - diff --git a/build/generate-parity-inventory.js b/build/generate-parity-inventory.js index b51b9d6c5..3373fe0eb 100644 --- a/build/generate-parity-inventory.js +++ b/build/generate-parity-inventory.js @@ -294,6 +294,26 @@ const OPTION_EVIDENCE_OVERRIDES = { 'cmd/devcontainer/src/runtime/lifecycle/selection.rs', ], }, + 'outdated': { + 'log-level': [ + 'cmd/devcontainer/src/commands/common/args.rs', + 'cmd/devcontainer/src/commands/configuration/upgrade.rs', + ], + 'terminal-columns': [ + 'cmd/devcontainer/src/commands/common/args.rs', + 'cmd/devcontainer/src/commands/configuration/upgrade.rs', + ], + 'terminal-rows': [ + 'cmd/devcontainer/src/commands/common/args.rs', + 'cmd/devcontainer/src/commands/configuration/upgrade.rs', + ], + }, + 'upgrade': { + 'log-level': [ + 'cmd/devcontainer/src/commands/common/args.rs', + 'cmd/devcontainer/src/commands/configuration/upgrade.rs', + ], + }, }; function readFile(relativePath) { diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index 686cc65d9..3e8e59d15 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -250,28 +250,22 @@ mod tests { } #[test] - fn metadata_tracks_unsupported_flags() { + fn metadata_tracks_no_remaining_visible_unsupported_flags() { let command = command_help("outdated").expect("outdated metadata"); - assert!(command - .unsupported_options - .contains(&"log-level".to_string())); + assert!(command.unsupported_options.is_empty()); - let up = command_help("up").expect("up metadata"); - assert!(!up - .unsupported_options - .contains(&"omit-syntax-directive".to_string())); + let upgrade = command_help("upgrade").expect("upgrade metadata"); + assert!(upgrade.unsupported_options.is_empty()); } #[test] - fn detects_unsupported_command_options() { + fn supported_command_options_are_not_reported_as_unsupported() { let error = unsupported_argument_error( "outdated", &["--log-level".to_string(), "trace".to_string()], - ) - .expect("unsupported error"); + ); - assert!(error.contains("--log-level")); - assert!(error.contains("devcontainer outdated")); + assert!(error.is_none()); } #[test] diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index b27bb68a6..a31b02178 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -2256,11 +2256,7 @@ } ], "positionals": [], - "unsupportedOptions": [ - "log-level", - "terminal-columns", - "terminal-rows" - ], + "unsupportedOptions": [], "unsupportedPositionals": [] }, { @@ -2417,9 +2413,7 @@ } ], "positionals": [], - "unsupportedOptions": [ - "log-level" - ], + "unsupportedOptions": [], "unsupportedPositionals": [] }, { diff --git a/cmd/devcontainer/src/commands/common.rs b/cmd/devcontainer/src/commands/common.rs index 5c59dbbda..be0075d29 100644 --- a/cmd/devcontainer/src/commands/common.rs +++ b/cmd/devcontainer/src/commands/common.rs @@ -8,7 +8,8 @@ mod manifest; pub(crate) use args::{ has_flag, parse_bool_option, parse_json_string_array_option, parse_option_value, parse_option_values, remote_env_overrides, runtime_options, runtime_process_request, - secrets_env, + secrets_env, validate_choice_option, validate_number_option, validate_option_values, + validate_paired_options, }; pub(crate) use config_resolution::{ load_resolved_config, resolve_override_config_path, resolve_read_configuration_path, diff --git a/cmd/devcontainer/src/commands/common/args.rs b/cmd/devcontainer/src/commands/common/args.rs index 638576d60..194faff61 100644 --- a/cmd/devcontainer/src/commands/common/args.rs +++ b/cmd/devcontainer/src/commands/common/args.rs @@ -127,6 +127,53 @@ pub(crate) fn validate_option_values(args: &[String], options: &[&str]) -> Resul Ok(()) } +pub(crate) fn validate_choice_option( + args: &[String], + option: &str, + choices: &[&str], +) -> Result<(), String> { + validate_option_values(args, &[option])?; + + for value in parse_option_values(args, option) { + if !choices.contains(&value.as_str()) { + return Err(format!( + "Invalid value for option {option}: {value}. Expected one of: {}", + choices.join(", ") + )); + } + } + + Ok(()) +} + +pub(crate) fn validate_number_option(args: &[String], option: &str) -> Result<(), String> { + validate_option_values(args, &[option])?; + + for value in parse_option_values(args, option) { + if value.parse::().is_err() { + return Err(format!( + "Invalid value for option {option}: {value}. Expected a number." + )); + } + } + + Ok(()) +} + +pub(crate) fn validate_paired_options( + args: &[String], + left: &str, + right: &str, +) -> Result<(), String> { + let has_left = has_flag(args, left); + let has_right = has_flag(args, right); + if has_left != has_right { + return Err(format!("Options {left} and {right} must be used together.")); + } + + Ok(()) +} + pub(crate) fn parse_option_values(args: &[String], option: &str) -> Vec { let mut values = Vec::new(); let mut index = 0; @@ -211,7 +258,9 @@ mod tests { use crate::process_runner::ProcessLogLevel; - use super::runtime_options; + use super::{ + runtime_options, validate_choice_option, validate_number_option, validate_paired_options, + }; #[test] fn runtime_options_collect_shared_runtime_flags() { @@ -305,4 +354,42 @@ mod tests { assert!(!options.skip_feature_auto_mapping); assert!(!options.stop_for_personalization); } + + #[test] + fn choice_options_reject_unknown_values() { + let error = validate_choice_option( + &["--log-level".to_string(), "warning".to_string()], + "--log-level", + &["info", "debug", "trace"], + ) + .expect_err("invalid choice"); + + assert!(error.contains("--log-level")); + assert!(error.contains("warning")); + } + + #[test] + fn number_options_require_numeric_values() { + let error = validate_number_option( + &["--terminal-columns".to_string(), "wide".to_string()], + "--terminal-columns", + ) + .expect_err("invalid number"); + + assert!(error.contains("--terminal-columns")); + assert!(error.contains("wide")); + } + + #[test] + fn paired_options_require_both_flags() { + let error = validate_paired_options( + &["--terminal-columns".to_string(), "120".to_string()], + "--terminal-columns", + "--terminal-rows", + ) + .expect_err("missing paired option"); + + assert!(error.contains("--terminal-columns")); + assert!(error.contains("--terminal-rows")); + } } diff --git a/cmd/devcontainer/src/commands/configuration/upgrade.rs b/cmd/devcontainer/src/commands/configuration/upgrade.rs index ec3df1f1c..a858c5cca 100644 --- a/cmd/devcontainer/src/commands/configuration/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/upgrade.rs @@ -15,7 +15,7 @@ use super::{FeatureReference, Lockfile, LockfileEntry}; use crate::commands::common; pub(super) fn run_outdated(args: &[String]) -> ExitCode { - match build_outdated_payload(args) { + match validate_outdated_options(args).and_then(|()| build_outdated_payload(args)) { Ok(payload) => { let output_format = common::parse_option_value(args, "--output-format") .unwrap_or_else(|| "json".to_string()); @@ -34,7 +34,7 @@ pub(super) fn run_outdated(args: &[String]) -> ExitCode { } pub(super) fn run_upgrade(args: &[String]) -> ExitCode { - match run_upgrade_lockfile(args) { + match validate_upgrade_command_options(args).and_then(|()| run_upgrade_lockfile(args)) { Ok(lockfile) => { if common::has_flag(args, "--dry-run") { println!( @@ -127,8 +127,6 @@ pub(super) fn build_outdated_payload(args: &[String]) -> Result { } pub(super) fn run_upgrade_lockfile(args: &[String]) -> Result { - validate_upgrade_options(args)?; - let mut loaded = load_config(args)?; if let (Some(feature), Some(target_version)) = ( common::parse_option_value(args, "--feature"), @@ -160,6 +158,46 @@ pub(super) fn run_upgrade_lockfile(args: &[String]) -> Result Ok(generated) } +fn validate_outdated_options(args: &[String]) -> Result<(), String> { + common::validate_option_values( + args, + &[ + "--user-data-folder", + "--workspace-folder", + "--config", + "--output-format", + "--log-level", + "--log-format", + "--terminal-columns", + "--terminal-rows", + ], + )?; + common::validate_choice_option(args, "--output-format", &["text", "json"])?; + common::validate_choice_option(args, "--log-format", &["text", "json"])?; + common::validate_choice_option(args, "--log-level", &["info", "debug", "trace"])?; + common::validate_paired_options(args, "--terminal-columns", "--terminal-rows")?; + common::validate_number_option(args, "--terminal-columns")?; + common::validate_number_option(args, "--terminal-rows")?; + Ok(()) +} + +fn validate_upgrade_command_options(args: &[String]) -> Result<(), String> { + common::validate_option_values( + args, + &[ + "--workspace-folder", + "--docker-path", + "--docker-compose-path", + "--config", + "--log-level", + "--feature", + "--target-version", + ], + )?; + common::validate_choice_option(args, "--log-level", &["error", "info", "debug", "trace"])?; + validate_upgrade_options(args) +} + fn validate_upgrade_options(args: &[String]) -> Result<(), String> { let feature = common::parse_option_value(args, "--feature"); let target_version = common::parse_option_value(args, "--target-version"); diff --git a/cmd/devcontainer/tests/cli_smoke/lockfile.rs b/cmd/devcontainer/tests/cli_smoke/lockfile.rs index 37fbe7c78..ec38ba9d5 100644 --- a/cmd/devcontainer/tests/cli_smoke/lockfile.rs +++ b/cmd/devcontainer/tests/cli_smoke/lockfile.rs @@ -10,18 +10,22 @@ use crate::support::test_support::{ copy_recursive, devcontainer_command, repo_root, unique_temp_dir, }; -#[test] -fn outdated_supports_upstream_json_output_fixture() { - let root = repo_root(); - let fixture = root +fn copied_lockfile_fixture(name: &str) -> std::path::PathBuf { + let fixture = repo_root() .join("upstream") .join("src") .join("test") .join("container-features") .join("configs") - .join("lockfile-outdated-command"); + .join(name); let workspace = unique_temp_dir("devcontainer-cli-smoke"); copy_recursive(&fixture, &workspace); + workspace +} + +#[test] +fn outdated_supports_upstream_json_output_fixture() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); let output = devcontainer_command(None) .args([ @@ -62,16 +66,7 @@ fn outdated_supports_upstream_json_output_fixture() { #[test] fn outdated_supports_text_output_fixture() { - let root = repo_root(); - let fixture = root - .join("upstream") - .join("src") - .join("test") - .join("container-features") - .join("configs") - .join("lockfile-outdated-command"); - let workspace = unique_temp_dir("devcontainer-cli-smoke"); - copy_recursive(&fixture, &workspace); + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); let output = devcontainer_command(None) .args([ @@ -97,18 +92,117 @@ fn outdated_supports_text_output_fixture() { let _ = fs::remove_dir_all(workspace); } +#[test] +fn outdated_accepts_log_level_and_terminal_dimensions() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); + + let output = devcontainer_command(None) + .args([ + "outdated", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--output-format", + "json", + "--log-level", + "trace", + "--terminal-columns", + "120", + "--terminal-rows", + "40", + ]) + .output() + .expect("outdated should run"); + + assert!(output.status.success(), "{output:?}"); + let payload: Value = serde_json::from_slice(&output.stdout).expect("json payload"); + assert_eq!( + payload["features"]["ghcr.io/devcontainers/features/git:1.0"]["latest"], + "1.2.0" + ); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn outdated_rejects_unpaired_terminal_dimensions() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); + + let output = devcontainer_command(None) + .args([ + "outdated", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--output-format", + "json", + "--terminal-columns", + "120", + ]) + .output() + .expect("outdated should run"); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains("--terminal-columns"), "{stderr}"); + assert!(stderr.contains("--terminal-rows"), "{stderr}"); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn outdated_rejects_non_numeric_terminal_dimensions() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); + + let output = devcontainer_command(None) + .args([ + "outdated", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--output-format", + "json", + "--terminal-columns", + "wide", + "--terminal-rows", + "40", + ]) + .output() + .expect("outdated should run"); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains("--terminal-columns"), "{stderr}"); + assert!(stderr.contains("number"), "{stderr}"); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn outdated_rejects_invalid_log_level() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); + + let output = devcontainer_command(None) + .args([ + "outdated", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--output-format", + "json", + "--log-level", + "warning", + ]) + .output() + .expect("outdated should run"); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains("--log-level"), "{stderr}"); + assert!(stderr.contains("warning"), "{stderr}"); + + let _ = fs::remove_dir_all(workspace); +} + #[test] fn upgrade_matches_upstream_lockfile_fixture() { - let root = repo_root(); - let fixture = root - .join("upstream") - .join("src") - .join("test") - .join("container-features") - .join("configs") - .join("lockfile-upgrade-command"); - let workspace = unique_temp_dir("devcontainer-cli-smoke"); - copy_recursive(&fixture, &workspace); + let workspace = copied_lockfile_fixture("lockfile-upgrade-command"); fs::copy( workspace.join("outdated.devcontainer-lock.json"), workspace.join(".devcontainer-lock.json"), @@ -134,18 +228,61 @@ fn upgrade_matches_upstream_lockfile_fixture() { let _ = fs::remove_dir_all(workspace); } +#[test] +fn upgrade_accepts_log_level_in_dry_run_mode() { + let workspace = copied_lockfile_fixture("lockfile-upgrade-command"); + + let output = devcontainer_command(None) + .args([ + "upgrade", + "--dry-run", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--log-level", + "trace", + ]) + .output() + .expect("upgrade should run"); + + assert!(output.status.success(), "{output:?}"); + let payload: Value = serde_json::from_slice(&output.stdout).expect("dry-run lockfile"); + let expected: Value = serde_json::from_str( + &fs::read_to_string(workspace.join("upgraded.devcontainer-lock.json")) + .expect("expected lockfile"), + ) + .expect("expected lockfile json"); + assert_eq!(payload, expected); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn upgrade_rejects_invalid_log_level() { + let workspace = copied_lockfile_fixture("lockfile-upgrade-command"); + + let output = devcontainer_command(None) + .args([ + "upgrade", + "--dry-run", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--log-level", + "warning", + ]) + .output() + .expect("upgrade should run"); + + assert!(!output.status.success(), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains("--log-level"), "{stderr}"); + assert!(stderr.contains("warning"), "{stderr}"); + + let _ = fs::remove_dir_all(workspace); +} + #[test] fn upgrade_with_feature_updates_config_and_dry_run_lockfile() { - let root = repo_root(); - let fixture = root - .join("upstream") - .join("src") - .join("test") - .join("container-features") - .join("configs") - .join("lockfile-upgrade-feature"); - let workspace = unique_temp_dir("devcontainer-cli-smoke"); - copy_recursive(&fixture, &workspace); + let workspace = copied_lockfile_fixture("lockfile-upgrade-feature"); fs::copy( workspace.join("input.devcontainer.json"), workspace.join(".devcontainer.json"), @@ -182,16 +319,7 @@ fn upgrade_with_feature_updates_config_and_dry_run_lockfile() { #[test] fn upgrade_supports_upstream_dependson_lockfile_fixture() { - let root = repo_root(); - let fixture = root - .join("upstream") - .join("src") - .join("test") - .join("container-features") - .join("configs") - .join("lockfile-dependson"); - let workspace = unique_temp_dir("devcontainer-cli-smoke"); - copy_recursive(&fixture, &workspace); + let workspace = copied_lockfile_fixture("lockfile-dependson"); let output = devcontainer_command(None) .args([ diff --git a/docs/upstream/parity-inventory.json b/docs/upstream/parity-inventory.json index 1d5090ba2..570ed7da2 100644 --- a/docs/upstream/parity-inventory.json +++ b/docs/upstream/parity-inventory.json @@ -5,8 +5,8 @@ "commandPathsTotal": 20, "commandPathsDeclared": 20, "optionsTotal": 200, - "optionsReferenced": 196, - "optionsMissing": 4 + "optionsReferenced": 200, + "optionsMissing": 0 }, "commands": [ { @@ -66,6 +66,7 @@ "evidence": [ "cmd/devcontainer/src/commands/configuration/load.rs", "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/context.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -103,6 +104,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -112,6 +114,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/container/uid_update.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" @@ -200,13 +203,15 @@ "name": "log-format", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/cli.rs" + "cmd/devcontainer/src/cli.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "log-level", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -318,6 +323,7 @@ "name": "terminal-columns", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -325,6 +331,7 @@ "name": "terminal-rows", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -341,6 +348,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/control.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -584,6 +592,7 @@ "evidence": [ "cmd/devcontainer/src/commands/configuration/load.rs", "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/context.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -593,6 +602,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -602,6 +612,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/container/uid_update.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" @@ -642,13 +653,15 @@ "name": "log-format", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/cli.rs" + "cmd/devcontainer/src/cli.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "log-level", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -713,6 +726,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/control.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -981,6 +995,7 @@ "evidence": [ "cmd/devcontainer/src/commands/configuration/load.rs", "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/context.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -1001,6 +1016,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" ] @@ -1010,6 +1026,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/container/uid_update.rs", "cmd/devcontainer/src/runtime/engine.rs", "cmd/devcontainer/src/runtime/exec.rs" @@ -1045,13 +1062,15 @@ "name": "log-format", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/cli.rs" + "cmd/devcontainer/src/cli.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "log-level", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -1093,6 +1112,7 @@ "name": "terminal-columns", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -1100,6 +1120,7 @@ "name": "terminal-rows", "sourceReferenced": true, "evidence": [ + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -1108,6 +1129,7 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/features/control.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs", "cmd/devcontainer/src/runtime/exec.rs" ] }, @@ -1136,8 +1158,8 @@ "declared": true, "optionSummary": { "total": 8, - "referenced": 5, - "missing": 3 + "referenced": 8, + "missing": 0 }, "options": [ { @@ -1145,20 +1167,25 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/load.rs", - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "log-format", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/cli.rs" + "cmd/devcontainer/src/cli.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "log-level", - "sourceReferenced": false, - "evidence": [] + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/common/args.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] }, { "name": "output-format", @@ -1169,19 +1196,26 @@ }, { "name": "terminal-columns", - "sourceReferenced": false, - "evidence": [] + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/common/args.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] }, { "name": "terminal-rows", - "sourceReferenced": false, - "evidence": [] + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/common/args.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] }, { "name": "user-data-folder", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/commands/configuration/features/control.rs" + "cmd/devcontainer/src/commands/configuration/features/control.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { @@ -1205,8 +1239,8 @@ "declared": true, "optionSummary": { "total": 8, - "referenced": 7, - "missing": 1 + "referenced": 8, + "missing": 0 }, "options": [ { @@ -1214,21 +1248,24 @@ "sourceReferenced": true, "evidence": [ "cmd/devcontainer/src/commands/configuration/load.rs", - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "docker-compose-path", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { "name": "docker-path", "sourceReferenced": true, "evidence": [ - "cmd/devcontainer/src/commands/configuration/read.rs" + "cmd/devcontainer/src/commands/configuration/read.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" ] }, { @@ -1247,8 +1284,11 @@ }, { "name": "log-level", - "sourceReferenced": false, - "evidence": [] + "sourceReferenced": true, + "evidence": [ + "cmd/devcontainer/src/commands/common/args.rs", + "cmd/devcontainer/src/commands/configuration/upgrade.rs" + ] }, { "name": "target-version", diff --git a/docs/upstream/parity-inventory.md b/docs/upstream/parity-inventory.md index 894250818..cd88b3049 100644 --- a/docs/upstream/parity-inventory.md +++ b/docs/upstream/parity-inventory.md @@ -5,7 +5,7 @@ Generated from the pinned upstream CLI command matrix and static source evidence - Upstream commit: `39685cf1aa58b5b11e90085bd32562fad61f4103` - Source: `upstream/src/spec-node/devContainersSpecCLI.ts` - Declared upstream command paths present natively: `20/20` -- Upstream options with a native source reference in mapped files: `196/200` +- Upstream options with a native source reference in mapped files: `200/200` This report is a static inventory, not a semantic parity proof. A referenced option can still be only partially implemented, and command-level known gaps are called out explicitly below. @@ -18,8 +18,8 @@ This report is a static inventory, not a semantic parity proof. A referenced opt | `build` | yes | 22/22 | 0 | 2 | | `run-user-commands` | yes | 27/27 | 0 | 1 | | `read-configuration` | yes | 18/18 | 0 | 2 | -| `outdated` | yes | 5/8 | 3 | 1 | -| `upgrade` | yes | 7/8 | 1 | 1 | +| `outdated` | yes | 8/8 | 0 | 1 | +| `upgrade` | yes | 8/8 | 0 | 1 | | `features` | yes | 0/0 | 0 | 1 | | `features test` | yes | 13/13 | 0 | 1 | | `features package` | yes | 0/0 | 0 | 1 | @@ -78,16 +78,16 @@ This report is a static inventory, not a semantic parity proof. A referenced opt - Description: Show current and available versions - Declared natively: yes -- Option source references: 5/8 -- Missing option references: `log-level`, `terminal-columns`, `terminal-rows` +- Option source references: 8/8 +- Missing option references: none - Known gaps: Backed by fixture/manual catalog data rather than real upstream registry resolution. ## `upgrade` - Description: Upgrade lockfile - Declared natively: yes -- Option source references: 7/8 -- Missing option references: `log-level` +- Option source references: 8/8 +- Missing option references: none - Known gaps: Backed by fixture/manual catalog data rather than real upstream registry resolution. ## `features` From a28b852a79eabce579f56752a6452ee23ada8fa8 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Tue, 14 Apr 2026 22:01:47 +0200 Subject: [PATCH 2/4] Audit native help output against committed CLI metadata --- Makefile | 6 +- cmd/devcontainer/tests/cli_smoke/help.rs | 140 ++++++++++++++++++++--- docs/upstream/compatibility-dashboard.md | 2 +- 3 files changed, 127 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index 5ab358eb4..82a25e869 100644 --- a/Makefile +++ b/Makefile @@ -13,12 +13,13 @@ check-parity-inventory \ check-cli-metadata \ check-todo-args \ + check-compatibility-dashboard \ upstream-compatibility RUST_MANIFEST := cmd/devcontainer/Cargo.toml RELEASE_BINARY := ./cmd/devcontainer/target/release/devcontainer -tests: rust-fmt rust-clippy rust-check rust-tests build-release standalone-artifact-smoke native-only-startup-contract command-matrix-drift-check schema-drift-check parity-harness no-node-runtime check-parity-inventory check-cli-metadata check-todo-args upstream-compatibility +tests: rust-fmt rust-clippy rust-check rust-tests build-release standalone-artifact-smoke native-only-startup-contract command-matrix-drift-check schema-drift-check parity-harness no-node-runtime check-parity-inventory check-cli-metadata check-todo-args check-compatibility-dashboard upstream-compatibility rust-fmt: cargo fmt --manifest-path $(RUST_MANIFEST) --all -- --check @@ -62,5 +63,8 @@ check-cli-metadata: check-todo-args: node build/generate-todo-args.js --check +check-compatibility-dashboard: + node build/generate-compatibility-dashboard.js --check + upstream-compatibility: node build/check-upstream-compatibility.js diff --git a/cmd/devcontainer/tests/cli_smoke/help.rs b/cmd/devcontainer/tests/cli_smoke/help.rs index 7d3da6c26..3cdc82c61 100644 --- a/cmd/devcontainer/tests/cli_smoke/help.rs +++ b/cmd/devcontainer/tests/cli_smoke/help.rs @@ -1,7 +1,42 @@ //! CLI smoke tests for help text and unsupported command surfaces. +use serde::Deserialize; + use crate::support::test_support::devcontainer_command; +const UNSUPPORTED_MARKER: &str = " [not yet implemented in native Rust CLI]"; +const CLI_METADATA_JSON: &str = include_str!("../../src/cli_metadata.json"); + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct CliMetadata { + root: HelpPage, + commands: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct HelpPage { + lines: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct HelpLine { + text: String, + option_names: Vec, + positional_names: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct CommandHelp { + path: String, + lines: Vec, + unsupported_options: Vec, + unsupported_positionals: Vec, +} + #[test] fn top_level_help_matches_public_cli_surface() { let output = devcontainer_command(None) @@ -125,20 +160,65 @@ fn only_top_level_long_version_flag_is_supported() { } #[test] -fn unsupported_visible_command_option_fails_with_native_message() { +fn committed_help_metadata_matches_actual_native_help_output() { + let metadata: CliMetadata = serde_json::from_str(CLI_METADATA_JSON).expect("cli metadata"); + let output = devcontainer_command(None) - .args(["outdated", "--log-level", "trace"]) + .arg("--help") .output() - .expect("outdated command should run"); + .expect("top-level help should run"); - assert!(!output.status.success(), "{output:?}"); - let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); - assert!(stderr.contains("--log-level"), "{stderr}"); - assert!( - stderr.contains("not yet implemented in the native Rust CLI"), - "{stderr}" + assert!(output.status.success(), "{output:?}"); + let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); + assert_eq!( + stdout.lines().collect::>(), + rendered_lines(&metadata.root.lines, &[], &[]) ); - assert!(stderr.contains("devcontainer outdated"), "{stderr}"); + + for command in &metadata.commands { + let mut args = command + .path + .split_whitespace() + .map(str::to_string) + .collect::>(); + args.push("--help".to_string()); + + let output = devcontainer_command(None) + .args(&args) + .output() + .unwrap_or_else(|error| panic!("help command {:?} should run: {error}", args)); + + assert!(output.status.success(), "{output:?}"); + let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); + assert_eq!( + stdout.lines().collect::>(), + rendered_lines( + &command.lines, + &command.unsupported_options, + &command.unsupported_positionals, + ), + "help mismatch for {}", + command.path + ); + } +} + +#[test] +fn outdated_help_no_longer_marks_log_and_terminal_flags_as_unsupported() { + let output = devcontainer_command(None) + .args(["outdated", "--help"]) + .output() + .expect("outdated help should run"); + + assert!(output.status.success(), "{output:?}"); + let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); + for flag in ["--log-level", "--terminal-columns", "--terminal-rows"] { + let line = stdout + .lines() + .find(|line| line.contains(flag)) + .unwrap_or_else(|| panic!("missing help line for {flag}: {stdout}")); + assert!(!line.contains(UNSUPPORTED_MARKER), "{stdout}"); + } } #[test] @@ -154,20 +234,42 @@ fn help_omits_hidden_upstream_options() { } #[test] -fn help_marks_unsupported_options_inline() { +fn upgrade_help_no_longer_marks_log_level_as_unsupported() { let output = devcontainer_command(None) - .args(["outdated", "--help"]) + .args(["upgrade", "--help"]) .output() - .expect("outdated help should run"); + .expect("upgrade help should run"); assert!(output.status.success(), "{output:?}"); let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); - let marked_line = stdout + let line = stdout .lines() .find(|line| line.contains("--log-level")) - .expect("marked unsupported option"); - assert!( - marked_line.contains("[not yet implemented in native Rust CLI]"), - "{stdout}" - ); + .expect("log-level help line"); + assert!(!line.contains(UNSUPPORTED_MARKER), "{stdout}"); +} + +fn rendered_lines( + lines: &[HelpLine], + unsupported_options: &[String], + unsupported_positionals: &[String], +) -> Vec { + lines + .iter() + .map(|line| { + if line + .option_names + .iter() + .any(|name| unsupported_options.contains(name)) + || line + .positional_names + .iter() + .any(|name| unsupported_positionals.contains(name)) + { + format!("{}{}", line.text, UNSUPPORTED_MARKER) + } else { + line.text.clone() + } + }) + .collect() } diff --git a/docs/upstream/compatibility-dashboard.md b/docs/upstream/compatibility-dashboard.md index f34440e3b..39c40a87a 100644 --- a/docs/upstream/compatibility-dashboard.md +++ b/docs/upstream/compatibility-dashboard.md @@ -8,7 +8,7 @@ ## Current snapshot - Declared upstream command paths present natively: `20/20` -- Upstream options with a native source reference in mapped Rust sources: `186/200` +- Upstream options with a native source reference in mapped Rust sources: `200/200` - The parity inventory is a static source-evidence report. It is intended to identify obvious gaps and track drift, not to claim semantic parity by itself. ## Highest-Impact Gaps From 12d29bb9f6b739c48d08aa1fce3355e1bab0d5ab Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 15 Apr 2026 18:34:22 +0200 Subject: [PATCH 3/4] Implement outdated and upgrade runtime logging --- .../src/commands/configuration/upgrade.rs | 172 +++++++++++++++++- cmd/devcontainer/src/output.rs | 140 +++++++++++++- cmd/devcontainer/tests/cli_smoke/lockfile.rs | 80 +++++++- 3 files changed, 380 insertions(+), 12 deletions(-) diff --git a/cmd/devcontainer/src/commands/configuration/upgrade.rs b/cmd/devcontainer/src/commands/configuration/upgrade.rs index a858c5cca..82711217c 100644 --- a/cmd/devcontainer/src/commands/configuration/upgrade.rs +++ b/cmd/devcontainer/src/commands/configuration/upgrade.rs @@ -13,9 +13,13 @@ use super::catalog::{ use super::load::load_config; use super::{FeatureReference, Lockfile, LockfileEntry}; use crate::commands::common; +use crate::output::{CommandLogLevel, CommandLogger, LogFormat, TerminalDimensions}; pub(super) fn run_outdated(args: &[String]) -> ExitCode { - match validate_outdated_options(args).and_then(|()| build_outdated_payload(args)) { + let logger = outdated_logger(args); + match validate_outdated_options(args) + .and_then(|()| build_outdated_payload_with_logger(args, Some(&logger))) + { Ok(payload) => { let output_format = common::parse_option_value(args, "--output-format") .unwrap_or_else(|| "json".to_string()); @@ -27,14 +31,17 @@ pub(super) fn run_outdated(args: &[String]) -> ExitCode { ExitCode::SUCCESS } Err(error) => { - eprintln!("{error}"); + logger.error(error); ExitCode::from(1) } } } pub(super) fn run_upgrade(args: &[String]) -> ExitCode { - match validate_upgrade_command_options(args).and_then(|()| run_upgrade_lockfile(args)) { + let logger = upgrade_logger(args); + match validate_upgrade_command_options(args) + .and_then(|()| run_upgrade_lockfile_with_logger(args, Some(&logger))) + { Ok(lockfile) => { if common::has_flag(args, "--dry-run") { println!( @@ -54,7 +61,7 @@ pub(super) fn run_upgrade(args: &[String]) -> ExitCode { ExitCode::SUCCESS } Err(error) => { - eprintln!("{error}"); + logger.error(error); ExitCode::from(1) } } @@ -95,15 +102,47 @@ pub(super) fn ensure_native_lockfile( Ok(()) } +#[cfg(test)] pub(super) fn build_outdated_payload(args: &[String]) -> Result { + build_outdated_payload_with_logger(args, None) +} + +fn build_outdated_payload_with_logger( + args: &[String], + logger: Option<&CommandLogger>, +) -> Result { + if let Some(logger) = logger { + logger.debug("Loading dev container configuration"); + logger.trace_terminal_dimensions(); + } let loaded = load_config(args)?; - let lockfile = read_lockfile(lockfile_path(&loaded.config_file))?; + if let Some(logger) = logger { + logger.debug(format!( + "Loading dev container configuration from {}", + loaded.config_file.display() + )); + } + let lockfile_path = lockfile_path(&loaded.config_file); + let lockfile = read_lockfile(lockfile_path.clone())?; + if let Some(logger) = logger { + if lockfile.is_some() { + logger.debug(format!("Loaded lockfile from {}", lockfile_path.display())); + } else { + logger.debug(format!("No lockfile found at {}", lockfile_path.display())); + } + } let features = loaded .configuration .get("features") .and_then(Value::as_object) .cloned() .unwrap_or_default(); + if let Some(logger) = logger { + logger.trace(format!( + "Enumerating {} configured feature definition(s)", + features.len() + )); + } let mut payload_features = Map::new(); for feature_id in features.keys() { @@ -121,38 +160,97 @@ pub(super) fn build_outdated_payload(args: &[String]) -> Result { payload_features.insert(feature_id.clone(), feature_info); } + if let Some(logger) = logger { + logger.debug(format!( + "Generated outdated payload for {} feature(s)", + payload_features.len() + )); + } Ok(json!({ "features": payload_features, })) } +#[cfg(test)] pub(super) fn run_upgrade_lockfile(args: &[String]) -> Result { + run_upgrade_lockfile_with_logger(args, None) +} + +fn run_upgrade_lockfile_with_logger( + args: &[String], + logger: Option<&CommandLogger>, +) -> Result { + if let Some(logger) = logger { + logger.debug("Loading dev container configuration"); + } let mut loaded = load_config(args)?; + if let Some(logger) = logger { + logger.debug(format!( + "Loading dev container configuration from {}", + loaded.config_file.display() + )); + } if let (Some(feature), Some(target_version)) = ( common::parse_option_value(args, "--feature"), common::parse_option_value(args, "--target-version"), ) { + if let Some(logger) = logger { + logger.info(format!( + "Updating '{feature}' to '{target_version}' in devcontainer.json" + )); + } update_feature_version_in_config( &loaded.config_file, &loaded.raw_text, &loaded.configuration, &feature, &target_version, + logger, )?; + if let Some(logger) = logger { + logger.debug("Reloading dev container configuration after feature update"); + } loaded = load_config(args)?; + if let Some(logger) = logger { + logger.debug(format!( + "Loading dev container configuration from {}", + loaded.config_file.display() + )); + } } + let feature_count = loaded + .configuration + .get("features") + .and_then(Value::as_object) + .map_or(0, Map::len); + if let Some(logger) = logger { + logger.debug(format!( + "Generating lockfile for {feature_count} feature(s)" + )); + } let generated = generate_lockfile( &loaded.configuration, Some(loaded.workspace_folder.as_path()), )?; if !common::has_flag(args, "--dry-run") { let lockfile_path = lockfile_path(&loaded.config_file); + if let Some(logger) = logger { + logger.info(format!("Writing lockfile: '{}'", lockfile_path.display())); + } fs::write( &lockfile_path, serde_json::to_string_pretty(&generated).map_err(|error| error.to_string())?, ) .map_err(|error| error.to_string())?; + if let Some(logger) = logger { + logger.debug(format!( + "Lockfile write complete: '{}'", + lockfile_path.display() + )); + } + } else if let Some(logger) = logger { + logger.debug("Dry-run lockfile generation complete"); } Ok(generated) @@ -324,6 +422,7 @@ fn update_feature_version_in_config( configuration: &Value, target_feature: &str, target_version: &str, + logger: Option<&CommandLogger>, ) -> Result<(), String> { let target_base = feature_id_without_version(target_feature); let current_key = configuration @@ -337,12 +436,29 @@ fn update_feature_version_in_config( .cloned(); let Some(current_key) = current_key else { + if let Some(logger) = logger { + logger.trace(format!( + "No changes to config file: {}", + config_path.display() + )); + } return Ok(()); }; let updated = raw_text.replace(¤t_key, &format!("{target_base}:{target_version}")); + if let Some(logger) = logger { + logger.trace(updated.as_str()); + } if updated != raw_text { + if let Some(logger) = logger { + logger.info(format!("Updating config file: '{}'", config_path.display())); + } fs::write(config_path, updated).map_err(|error| error.to_string())?; + } else if let Some(logger) = logger { + logger.trace(format!( + "No changes to config file: {}", + config_path.display() + )); } Ok(()) } @@ -386,6 +502,52 @@ fn cell(value: Option<&Value>) -> String { value.and_then(Value::as_str).unwrap_or("-").to_string() } +fn outdated_logger(args: &[String]) -> CommandLogger { + CommandLogger::new( + parse_requested_log_format(args), + parse_outdated_log_level(args), + ) + .with_terminal_dimensions(parse_terminal_dimensions(args)) +} + +fn upgrade_logger(args: &[String]) -> CommandLogger { + CommandLogger::new(LogFormat::Text, parse_upgrade_log_level(args)) +} + +fn parse_requested_log_format(args: &[String]) -> LogFormat { + match common::parse_option_value(args, "--log-format").as_deref() { + Some("json") => LogFormat::Json, + _ => LogFormat::Text, + } +} + +fn parse_outdated_log_level(args: &[String]) -> CommandLogLevel { + match common::parse_option_value(args, "--log-level").as_deref() { + Some("trace") => CommandLogLevel::Trace, + Some("debug") => CommandLogLevel::Debug, + _ => CommandLogLevel::Info, + } +} + +fn parse_upgrade_log_level(args: &[String]) -> CommandLogLevel { + match common::parse_option_value(args, "--log-level").as_deref() { + Some("error") => CommandLogLevel::Error, + Some("trace") => CommandLogLevel::Trace, + Some("debug") => CommandLogLevel::Debug, + _ => CommandLogLevel::Info, + } +} + +fn parse_terminal_dimensions(args: &[String]) -> Option { + let columns = common::parse_option_value(args, "--terminal-columns")? + .parse::() + .ok()?; + let rows = common::parse_option_value(args, "--terminal-rows")? + .parse::() + .ok()?; + Some(TerminalDimensions { columns, rows }) +} + pub(super) fn parse_feature_reference(feature_id: &str) -> Option { if !feature_id.starts_with("ghcr.io/") && !feature_id.starts_with("https://") diff --git a/cmd/devcontainer/src/output.rs b/cmd/devcontainer/src/output.rs index 3b155fe10..5eb379e1e 100644 --- a/cmd/devcontainer/src/output.rs +++ b/cmd/devcontainer/src/output.rs @@ -6,6 +6,106 @@ pub enum LogFormat { Json, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum CommandLogLevel { + Trace, + Debug, + Info, + Error, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct TerminalDimensions { + pub columns: usize, + pub rows: usize, +} + +pub struct CommandLogger { + format: LogFormat, + level: CommandLogLevel, + terminal_dimensions: Option, +} + +impl CommandLogLevel { + fn severity(self) -> u8 { + match self { + Self::Trace => 0, + Self::Debug => 1, + Self::Info => 2, + Self::Error => 3, + } + } + + fn as_str(self) -> &'static str { + match self { + Self::Trace => "trace", + Self::Debug => "debug", + Self::Info => "info", + Self::Error => "error", + } + } +} + +impl CommandLogger { + pub fn new(format: LogFormat, level: CommandLogLevel) -> Self { + Self { + format, + level, + terminal_dimensions: None, + } + } + + pub fn with_terminal_dimensions( + mut self, + terminal_dimensions: Option, + ) -> Self { + self.terminal_dimensions = terminal_dimensions; + self + } + + pub fn error(&self, message: impl AsRef) { + self.log(CommandLogLevel::Error, message); + } + + pub fn info(&self, message: impl AsRef) { + self.log(CommandLogLevel::Info, message); + } + + pub fn debug(&self, message: impl AsRef) { + self.log(CommandLogLevel::Debug, message); + } + + pub fn trace(&self, message: impl AsRef) { + self.log(CommandLogLevel::Trace, message); + } + + pub fn trace_terminal_dimensions(&self) { + if let Some(dimensions) = self.terminal_dimensions { + self.trace(format!( + "Using terminal dimensions: columns={} rows={}", + dimensions.columns, dimensions.rows + )); + } + } + + fn log(&self, level: CommandLogLevel, message: impl AsRef) { + if let Some(rendered) = self.render(level, message.as_ref()) { + eprintln!("{rendered}"); + } + } + + fn render(&self, level: CommandLogLevel, message: &str) -> Option { + if level.severity() < self.level.severity() { + return None; + } + + Some(match self.format { + LogFormat::Text => format!("[{}] {message}", level.as_str()), + LogFormat::Json => render_log(self.format, level.as_str(), message), + }) + } +} + pub fn render_log(format: LogFormat, level: &str, message: &str) -> String { match format { LogFormat::Text => message.to_string(), @@ -19,7 +119,7 @@ pub fn render_log(format: LogFormat, level: &str, message: &str) -> String { #[cfg(test)] mod tests { - use super::{render_log, LogFormat}; + use super::{render_log, CommandLogLevel, CommandLogger, LogFormat, TerminalDimensions}; #[test] fn renders_text_logs_without_json_envelope() { @@ -33,4 +133,42 @@ mod tests { "{\"level\":\"info\",\"message\":\"quoted \\\"value\\\"\"}" ); } + + #[test] + fn command_logger_filters_entries_below_configured_level() { + let logger = CommandLogger::new(LogFormat::Text, CommandLogLevel::Info); + + assert_eq!(logger.render(CommandLogLevel::Trace, "ignored"), None); + assert_eq!( + logger.render(CommandLogLevel::Error, "emitted"), + Some("[error] emitted".to_string()) + ); + } + + #[test] + fn command_logger_renders_json_entries() { + let logger = CommandLogger::new(LogFormat::Json, CommandLogLevel::Trace); + + assert_eq!( + logger.render(CommandLogLevel::Debug, "quoted \"value\""), + Some("{\"level\":\"debug\",\"message\":\"quoted \\\"value\\\"\"}".to_string()) + ); + } + + #[test] + fn command_logger_stores_terminal_dimensions() { + let logger = CommandLogger::new(LogFormat::Text, CommandLogLevel::Trace) + .with_terminal_dimensions(Some(TerminalDimensions { + columns: 120, + rows: 40, + })); + + assert_eq!( + logger.terminal_dimensions, + Some(TerminalDimensions { + columns: 120, + rows: 40, + }) + ); + } } diff --git a/cmd/devcontainer/tests/cli_smoke/lockfile.rs b/cmd/devcontainer/tests/cli_smoke/lockfile.rs index ec38ba9d5..7a7c24440 100644 --- a/cmd/devcontainer/tests/cli_smoke/lockfile.rs +++ b/cmd/devcontainer/tests/cli_smoke/lockfile.rs @@ -2,6 +2,7 @@ use std::fs; use std::path::Path; +use std::process::Output; use serde_json::{json, Value}; use sha2::{Digest, Sha256}; @@ -23,6 +24,17 @@ fn copied_lockfile_fixture(name: &str) -> std::path::PathBuf { workspace } +fn utf8_stderr(output: &Output) -> String { + String::from_utf8(output.stderr.clone()).expect("utf8 stderr") +} + +fn stderr_json_logs(output: &Output) -> Vec { + utf8_stderr(output) + .lines() + .map(|line| serde_json::from_str(line).expect("json log line")) + .collect() +} + #[test] fn outdated_supports_upstream_json_output_fixture() { let workspace = copied_lockfile_fixture("lockfile-outdated-command"); @@ -93,7 +105,49 @@ fn outdated_supports_text_output_fixture() { } #[test] -fn outdated_accepts_log_level_and_terminal_dimensions() { +fn outdated_emits_json_logs_when_requested() { + let workspace = copied_lockfile_fixture("lockfile-outdated-command"); + + let output = devcontainer_command(None) + .args([ + "outdated", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--output-format", + "json", + "--log-format", + "json", + "--log-level", + "trace", + ]) + .output() + .expect("outdated should run"); + + assert!(output.status.success(), "{output:?}"); + let payload: Value = serde_json::from_slice(&output.stdout).expect("json payload"); + assert_eq!( + payload["features"]["ghcr.io/devcontainers/features/git:1.0"]["latest"], + "1.2.0" + ); + + let logs = stderr_json_logs(&output); + assert!(!logs.is_empty(), "{logs:?}"); + assert!( + logs.iter().any(|entry| entry["level"] == "trace"), + "{logs:?}" + ); + assert!( + logs.iter().any(|entry| entry["message"] + .as_str() + .is_some_and(|message| message.contains("Generated outdated payload"))), + "{logs:?}" + ); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn outdated_trace_logs_include_terminal_dimensions() { let workspace = copied_lockfile_fixture("lockfile-outdated-command"); let output = devcontainer_command(None) @@ -119,6 +173,10 @@ fn outdated_accepts_log_level_and_terminal_dimensions() { payload["features"]["ghcr.io/devcontainers/features/git:1.0"]["latest"], "1.2.0" ); + let stderr = utf8_stderr(&output); + assert!(stderr.contains("terminal dimensions"), "{stderr}"); + assert!(stderr.contains("120"), "{stderr}"); + assert!(stderr.contains("40"), "{stderr}"); let _ = fs::remove_dir_all(workspace); } @@ -141,7 +199,7 @@ fn outdated_rejects_unpaired_terminal_dimensions() { .expect("outdated should run"); assert!(!output.status.success(), "{output:?}"); - let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + let stderr = utf8_stderr(&output); assert!(stderr.contains("--terminal-columns"), "{stderr}"); assert!(stderr.contains("--terminal-rows"), "{stderr}"); @@ -168,7 +226,7 @@ fn outdated_rejects_non_numeric_terminal_dimensions() { .expect("outdated should run"); assert!(!output.status.success(), "{output:?}"); - let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + let stderr = utf8_stderr(&output); assert!(stderr.contains("--terminal-columns"), "{stderr}"); assert!(stderr.contains("number"), "{stderr}"); @@ -193,7 +251,7 @@ fn outdated_rejects_invalid_log_level() { .expect("outdated should run"); assert!(!output.status.success(), "{output:?}"); - let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + let stderr = utf8_stderr(&output); assert!(stderr.contains("--log-level"), "{stderr}"); assert!(stderr.contains("warning"), "{stderr}"); @@ -229,7 +287,7 @@ fn upgrade_matches_upstream_lockfile_fixture() { } #[test] -fn upgrade_accepts_log_level_in_dry_run_mode() { +fn upgrade_trace_logs_cover_dry_run_milestones() { let workspace = copied_lockfile_fixture("lockfile-upgrade-command"); let output = devcontainer_command(None) @@ -252,6 +310,16 @@ fn upgrade_accepts_log_level_in_dry_run_mode() { ) .expect("expected lockfile json"); assert_eq!(payload, expected); + let stderr = utf8_stderr(&output); + assert!( + stderr.contains("Loading dev container configuration"), + "{stderr}" + ); + assert!(stderr.contains("Generating lockfile"), "{stderr}"); + assert!( + stderr.contains("Dry-run lockfile generation complete"), + "{stderr}" + ); let _ = fs::remove_dir_all(workspace); } @@ -273,7 +341,7 @@ fn upgrade_rejects_invalid_log_level() { .expect("upgrade should run"); assert!(!output.status.success(), "{output:?}"); - let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + let stderr = utf8_stderr(&output); assert!(stderr.contains("--log-level"), "{stderr}"); assert!(stderr.contains("warning"), "{stderr}"); From 1531513ec827d44c8f71ac6b078d9d93c7ba3146 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 15 Apr 2026 18:46:51 +0200 Subject: [PATCH 4/4] Preserve upstream JSON log event shape --- cmd/devcontainer/src/cli.rs | 7 ++- cmd/devcontainer/src/output.rs | 65 +++++++++++++++----- cmd/devcontainer/tests/cli_smoke/lockfile.rs | 10 ++- 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index 3e8e59d15..4a590184f 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -4,7 +4,7 @@ use std::sync::OnceLock; use serde::Deserialize; -use crate::output::{self, LogFormat}; +use crate::output::{self, CommandLogLevel, LogFormat}; const CLI_METADATA_JSON: &str = include_str!("cli_metadata.json"); const UNSUPPORTED_MARKER: &str = " [not yet implemented in native Rust CLI]"; @@ -144,7 +144,10 @@ pub fn emit_log(log_format: &str, message: &str) { "json" => LogFormat::Json, _ => LogFormat::Text, }; - println!("{}", output::render_log(format, "info", message)); + println!( + "{}", + output::render_log(format, CommandLogLevel::Info, message) + ); } pub fn is_command_help_request(args: &[String]) -> bool { diff --git a/cmd/devcontainer/src/output.rs b/cmd/devcontainer/src/output.rs index 5eb379e1e..3fa3c16ac 100644 --- a/cmd/devcontainer/src/output.rs +++ b/cmd/devcontainer/src/output.rs @@ -1,5 +1,7 @@ //! Output rendering helpers for JSON and human-readable command results. +use std::time::{SystemTime, UNIX_EPOCH}; + #[derive(Clone, Copy)] pub enum LogFormat { Text, @@ -44,6 +46,15 @@ impl CommandLogLevel { Self::Error => "error", } } + + fn upstream_level(self) -> u8 { + match self { + Self::Trace => 1, + Self::Debug => 2, + Self::Info => 3, + Self::Error => 5, + } + } } impl CommandLogger { @@ -101,37 +112,58 @@ impl CommandLogger { Some(match self.format { LogFormat::Text => format!("[{}] {message}", level.as_str()), - LogFormat::Json => render_log(self.format, level.as_str(), message), + LogFormat::Json => render_log(self.format, level, message), }) } } -pub fn render_log(format: LogFormat, level: &str, message: &str) -> String { +pub fn render_log(format: LogFormat, level: CommandLogLevel, message: &str) -> String { match format { LogFormat::Text => message.to_string(), LogFormat::Json => serde_json::json!({ - "level": level, - "message": message, + "type": "text", + "level": level.upstream_level(), + "timestamp": log_timestamp(), + "text": message, }) .to_string(), } } +fn log_timestamp() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock should be after unix epoch") + .as_millis() as u64 +} + #[cfg(test)] mod tests { + use serde_json::Value; + use super::{render_log, CommandLogLevel, CommandLogger, LogFormat, TerminalDimensions}; #[test] fn renders_text_logs_without_json_envelope() { - assert_eq!(render_log(LogFormat::Text, "info", "hello"), "hello"); + assert_eq!( + render_log(LogFormat::Text, CommandLogLevel::Info, "hello"), + "hello" + ); } #[test] - fn renders_json_logs_with_level_and_message() { - assert_eq!( - render_log(LogFormat::Json, "info", "quoted \"value\""), - "{\"level\":\"info\",\"message\":\"quoted \\\"value\\\"\"}" - ); + fn renders_json_logs_as_upstream_text_events() { + let rendered: Value = serde_json::from_str(&render_log( + LogFormat::Json, + CommandLogLevel::Info, + "quoted \"value\"", + )) + .expect("json log"); + + assert_eq!(rendered["type"], "text"); + assert_eq!(rendered["level"], 3); + assert_eq!(rendered["text"], "quoted \"value\""); + assert!(rendered["timestamp"].as_i64().is_some(), "{rendered:?}"); } #[test] @@ -149,10 +181,15 @@ mod tests { fn command_logger_renders_json_entries() { let logger = CommandLogger::new(LogFormat::Json, CommandLogLevel::Trace); - assert_eq!( - logger.render(CommandLogLevel::Debug, "quoted \"value\""), - Some("{\"level\":\"debug\",\"message\":\"quoted \\\"value\\\"\"}".to_string()) - ); + let rendered = logger + .render(CommandLogLevel::Debug, "quoted \"value\"") + .expect("json log"); + let entry: Value = serde_json::from_str(&rendered).expect("json log"); + + assert_eq!(entry["type"], "text"); + assert_eq!(entry["level"], 2); + assert_eq!(entry["text"], "quoted \"value\""); + assert!(entry["timestamp"].as_i64().is_some(), "{entry:?}"); } #[test] diff --git a/cmd/devcontainer/tests/cli_smoke/lockfile.rs b/cmd/devcontainer/tests/cli_smoke/lockfile.rs index 7a7c24440..5e8cd25ab 100644 --- a/cmd/devcontainer/tests/cli_smoke/lockfile.rs +++ b/cmd/devcontainer/tests/cli_smoke/lockfile.rs @@ -133,11 +133,17 @@ fn outdated_emits_json_logs_when_requested() { let logs = stderr_json_logs(&output); assert!(!logs.is_empty(), "{logs:?}"); assert!( - logs.iter().any(|entry| entry["level"] == "trace"), + logs.iter() + .any(|entry| entry["type"] == "text" && entry["level"] == 1), "{logs:?}" ); assert!( - logs.iter().any(|entry| entry["message"] + logs.iter() + .all(|entry| entry["timestamp"].as_i64().is_some()), + "{logs:?}" + ); + assert!( + logs.iter().any(|entry| entry["text"] .as_str() .is_some_and(|message| message.contains("Generated outdated payload"))), "{logs:?}"