diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index ecceece..4e4e8fd 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 13 | +| Open findings | 12 | ## Summary @@ -54,7 +54,7 @@ argument parsing, and the command-tree wiring are untested. |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:18`, `src/ScadaLink.CLI/Commands/DebugCommands.cs:45`, `src/ScadaLink.CLI/CliConfig.cs:37-39` | **Description** @@ -79,7 +79,12 @@ only then override the config value. Apply the same fix to `DebugCommands.BuildS **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``). Removed the `--format` option's +`DefaultValueFactory` in `Program.cs` and added `CommandHelpers.ResolveFormat`, which uses +`ParseResult.GetResult(formatOption)` to detect an explicitly supplied flag and resolves +precedence explicitly: explicit `--format` → `CliConfig.DefaultFormat` (env var / config +file) → `"json"`. Both `CommandHelpers.ExecuteCommandAsync` and `DebugCommands.BuildStream` +now call `ResolveFormat`. Regression tests added in `FormatResolutionTests`. ### CLI-002 — Empty success body crashes table rendering with an unhandled exception diff --git a/src/ScadaLink.CLI/Commands/CommandHelpers.cs b/src/ScadaLink.CLI/Commands/CommandHelpers.cs index d424fc6..f2f5f20 100644 --- a/src/ScadaLink.CLI/Commands/CommandHelpers.cs +++ b/src/ScadaLink.CLI/Commands/CommandHelpers.cs @@ -15,8 +15,8 @@ internal static class CommandHelpers Option passwordOption, object command) { - var format = result.GetValue(formatOption) ?? "json"; var config = CliConfig.Load(); + var format = ResolveFormat(result, formatOption, config); // Resolve management URL var url = result.GetValue(urlOption); @@ -53,6 +53,27 @@ internal static class CommandHelpers return HandleResponse(response, format); } + /// + /// Resolves the output format using the documented precedence chain: + /// an explicitly supplied --format option wins, otherwise the + /// config-file / environment-variable default () + /// is used, otherwise json. The --format option must not declare a + /// DefaultValueFactory — that would mask whether the flag was supplied. + /// + internal static string ResolveFormat(ParseResult result, Option formatOption, CliConfig config) + { + // GetResult returns non-null only when the option was actually present on the + // command line, letting an explicit --format override the config default. + if (result.GetResult(formatOption) != null) + { + var explicitValue = result.GetValue(formatOption); + if (!string.IsNullOrWhiteSpace(explicitValue)) + return explicitValue; + } + + return string.IsNullOrWhiteSpace(config.DefaultFormat) ? "json" : config.DefaultFormat; + } + internal static int HandleResponse(ManagementResponse response, string format) { if (response.JsonData != null) diff --git a/src/ScadaLink.CLI/Commands/DebugCommands.cs b/src/ScadaLink.CLI/Commands/DebugCommands.cs index 84a7d39..3dc558d 100644 --- a/src/ScadaLink.CLI/Commands/DebugCommands.cs +++ b/src/ScadaLink.CLI/Commands/DebugCommands.cs @@ -42,8 +42,8 @@ public static class DebugCommands cmd.SetAction(async (ParseResult result) => { var instanceId = result.GetValue(idOption); - var format = result.GetValue(formatOption) ?? "json"; var config = CliConfig.Load(); + var format = CommandHelpers.ResolveFormat(result, formatOption, config); var url = result.GetValue(urlOption); if (string.IsNullOrWhiteSpace(url)) diff --git a/src/ScadaLink.CLI/Program.cs b/src/ScadaLink.CLI/Program.cs index 0d47849..3cb5e2a 100644 --- a/src/ScadaLink.CLI/Program.cs +++ b/src/ScadaLink.CLI/Program.cs @@ -7,8 +7,9 @@ var rootCommand = new RootCommand("ScadaLink CLI — manage the ScadaLink SCADA var urlOption = new Option("--url") { Description = "Management API URL", Recursive = true }; var usernameOption = new Option("--username") { Description = "LDAP username", Recursive = true }; var passwordOption = new Option("--password") { Description = "LDAP password", Recursive = true }; +// No DefaultValueFactory: format precedence (explicit --format -> config/env -> "json") +// is resolved by CommandHelpers.ResolveFormat, which needs to distinguish an absent flag. var formatOption = new Option("--format") { Description = "Output format (json or table)", Recursive = true }; -formatOption.DefaultValueFactory = _ => "json"; rootCommand.Add(urlOption); rootCommand.Add(usernameOption); diff --git a/tests/ScadaLink.CLI.Tests/FormatResolutionTests.cs b/tests/ScadaLink.CLI.Tests/FormatResolutionTests.cs new file mode 100644 index 0000000..fb34de4 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/FormatResolutionTests.cs @@ -0,0 +1,54 @@ +using System.CommandLine; +using ScadaLink.CLI; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +public class FormatResolutionTests +{ + private static (Option formatOption, RootCommand root) BuildHarness() + { + var formatOption = new Option("--format") { Recursive = true }; + var root = new RootCommand(); + root.Add(formatOption); + return (formatOption, root); + } + + [Fact] + public void ResolveFormat_ExplicitFlag_OverridesConfig() + { + var (formatOption, root) = BuildHarness(); + var result = root.Parse(new[] { "--format", "table" }); + var config = new CliConfig { DefaultFormat = "json" }; + + var format = CommandHelpers.ResolveFormat(result, formatOption, config); + + Assert.Equal("table", format); + } + + [Fact] + public void ResolveFormat_FlagAbsent_UsesConfigDefaultFormat() + { + // Regression for CLI-001: when --format is not supplied, the config-file / + // env-var DefaultFormat must be honoured instead of always falling back to "json". + var (formatOption, root) = BuildHarness(); + var result = root.Parse(Array.Empty()); + var config = new CliConfig { DefaultFormat = "table" }; + + var format = CommandHelpers.ResolveFormat(result, formatOption, config); + + Assert.Equal("table", format); + } + + [Fact] + public void ResolveFormat_FlagAbsent_AndNoConfig_DefaultsToJson() + { + var (formatOption, root) = BuildHarness(); + var result = root.Parse(Array.Empty()); + var config = new CliConfig { DefaultFormat = "json" }; + + var format = CommandHelpers.ResolveFormat(result, formatOption, config); + + Assert.Equal("json", format); + } +}