diff --git a/CHANGELOG.md b/CHANGELOG.md index 67e19097..5e932132 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,7 @@ See the Nerdbank.GitVersioning removal entry under _Changes to existing features - Buildvana SDK now correctly checks the `IsTestingPlatformApplication` (required by MTP) instead of `IsTestProject` (required by VSTest) to determine whether a project is a test project and set `BV_IsTestProject` accordingly. - `bv release` no longer tags and publishes a version one patch above the one its artifacts were built with. The "Prepare release" commit bumps the Git height, hence the version, but it was only created when an earlier step had a file to commit; a release with nothing to commit before the build (typically a prerelease with no version-spec change and `release.changelogUpdates` set to `stable` or `none`) therefore built and pushed its packages at the pre-commit version, then created the commit, and tagged and released the version above. The release commit is now always created before the build. - URLs that `bv release` builds from the repository URL are no longer missing the separator before their first path segment: release links (`.../Buildvanareleases/tag/1.1.10`) and file links (`.../Buildvanablob/main/CHANGELOG.md`) now come out as `.../Buildvana/releases/tag/1.1.10` and `.../Buildvana/blob/main/CHANGELOG.md`. This affected the version section titles written into the changelog and the "human-curated changelog" link at the top of every generated release description; the titles already written for 1.0.220, 1.1.4, and 1.1.10 have been corrected in place. +- `bv clean` no longer silently ignores unknown options: `bv clean --bogus` now fails with `Unknown option '--bogus' for command 'clean'`. Every `bv` command now rejects options it does not recognize, and does so before anything else runs: previously, commands that parse their own options (e.g. `bv release`) reported an unknown option only after the SDK version check, so a mismatched SDK pin could mask the typo. ### Known problems introduced by this release diff --git a/src/Buildvana.Tool/CommandLine/BvOptionAttribute.cs b/src/Buildvana.Tool/CommandLine/BvOptionAttribute.cs index 597ccb79..1b651540 100644 --- a/src/Buildvana.Tool/CommandLine/BvOptionAttribute.cs +++ b/src/Buildvana.Tool/CommandLine/BvOptionAttribute.cs @@ -12,9 +12,11 @@ namespace Buildvana.Tool.CommandLine; /// "-c|--configuration <NAME>" or "--no-color". /// /// -/// This attribute carries help metadata only. It does not drive parsing: the option reader -/// () is fed explicit names by each *Settings type. The help renderer -/// reflects these attributes to print the OPTIONS grid. +/// This attribute does not drive parsing — the option reader () is fed +/// explicit names by each *Settings type — but it does drive validation: the argument validator reflects +/// it to determine which options a command accepts, and whether they consume a value token, so the declared +/// names must match the names the settings type reads. The help renderer reflects these attributes to print +/// the OPTIONS grid. /// [AttributeUsage(AttributeTargets.Property)] internal sealed class BvOptionAttribute : Attribute diff --git a/src/Buildvana.Tool/Infrastructure/Execution/CommandArgumentValidator.cs b/src/Buildvana.Tool/Infrastructure/Execution/CommandArgumentValidator.cs index 27edee1b..55d8ca86 100644 --- a/src/Buildvana.Tool/Infrastructure/Execution/CommandArgumentValidator.cs +++ b/src/Buildvana.Tool/Infrastructure/Execution/CommandArgumentValidator.cs @@ -14,7 +14,10 @@ namespace Buildvana.Tool.Infrastructure.Execution; /// Enforces the argument contract for a dispatched command: /// forwarding commands take no tokens before -- (everything to forward goes after it); /// non-forwarding commands have nowhere to forward, so they reject anything after --, and accept only as -/// many positionals as their settings type declares via . +/// many positionals as their settings type declares via and the options it +/// declares via (a command with no settings type declares none, so it takes no +/// options at all). The settings type's Parse can therefore assume every option token it receives is one +/// the command declares. /// internal static class CommandArgumentValidator { @@ -46,12 +49,21 @@ public static void Validate(CommandRegistration command, ParsedCommandLine parse throw new BuildFailedException($"Command '{command.Name}' does not forward arguments; remove the '--' separator and everything after it."); } + // Excess positionals are checked before unknown options so that in the typical shape of a botched + // command line (`bv clean junk --bogus`) the offending tokens are reported in command-line order. var arguments = DeclaredArguments(command); if (positionals.Count > arguments.Count) { throw new BuildFailedException($"Unexpected argument '{positionals[arguments.Count]}' for command '{command.Name}'."); } + var reader = new CliOptionReader(parsed.OptionTokens); + ConsumeDeclaredOptions(reader, command); + if (reader.Remaining.Count > 0) + { + throw new BuildFailedException($"Unknown option '{reader.Remaining[0]}' for command '{command.Name}'."); + } + for (var i = positionals.Count; i < arguments.Count; i++) { if (arguments[i].Required) @@ -74,4 +86,35 @@ private static IReadOnlyList DeclaredArguments(CommandRegis .Select(static p => p.GetCustomAttribute()) .OfType()]; } + + private static void ConsumeDeclaredOptions(CliOptionReader reader, CommandRegistration command) + { + foreach (var option in DeclaredOptions(command)) + { + foreach (var name in option.LongNames.Concat(option.ShortNames)) + { + if (option.ValueName is null) + { + _ = reader.ReadFlag(name); + } + else + { + _ = reader.ReadValue(name); + } + } + } + } + + private static IEnumerable DeclaredOptions(CommandRegistration command) + { + if (command.SettingsType is null) + { + return []; + } + + return command.SettingsType + .GetProperties(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) + .Select(static p => p.GetCustomAttribute()) + .OfType(); + } } diff --git a/src/Buildvana.Tool/Subcommands/ReleaseSettings.cs b/src/Buildvana.Tool/Subcommands/ReleaseSettings.cs index 561d0d59..5b6f0919 100644 --- a/src/Buildvana.Tool/Subcommands/ReleaseSettings.cs +++ b/src/Buildvana.Tool/Subcommands/ReleaseSettings.cs @@ -70,34 +70,28 @@ private ReleaseSettings(ReleaseConfig? config, DotNetSettings dotNetSettings) public bool? Dogfood { get; init; } /// - /// Parses the command's option tokens into a , rejecting any option the command - /// does not recognize, and binds the configuration sources consulted by the Resolve* methods. + /// Parses the command's option tokens into a and binds the configuration + /// sources consulted by the Resolve* methods. Unknown options have already been rejected by + /// CommandArgumentValidator, so every option token is one the command declares. /// /// The option tokens for the release command (from CommandParameters.Options). /// The Buildvana configuration whose release section layers between the flags and the defaults. /// The resolved dotnet settings, providing the fallback build configuration. /// The parsed settings. - /// An option value is invalid, or an unrecognized option was given. + /// An option value is invalid. public static ReleaseSettings Parse(IReadOnlyList options, BuildvanaConfig config, DotNetSettings dotNetSettings) { Guard.IsNotNull(options); Guard.IsNotNull(config); Guard.IsNotNull(dotNetSettings); var reader = new CliOptionReader(options); - var settings = new ReleaseSettings(config.Release, dotNetSettings) + return new ReleaseSettings(config.Release, dotNetSettings) { Configuration = reader.ReadValue("--configuration", "-c"), Bump = reader.ReadValue("--bump"), CheckPublicApi = ParseBool(reader.ReadValue("--check-public-api"), "--check-public-api"), Dogfood = ParseBool(reader.ReadValue("--dogfood"), "--dogfood"), }; - - if (reader.Remaining.Count > 0) - { - throw new BuildFailedException($"Unknown option '{reader.Remaining[0]}' for command 'release'."); - } - - return settings; } /// diff --git a/src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs b/src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs index e07c8d95..9e3ebc5a 100644 --- a/src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs +++ b/src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs @@ -57,39 +57,28 @@ private VersionAdvanceSettings(ReleaseConfig? releaseConfig) public bool Force { get; init; } /// - /// Parses the command's positional and option tokens into a , rejecting - /// anything the command does not recognize, and binds the configuration sources consulted by the - /// Resolve* methods. + /// Parses the command's positional and option tokens into a and binds + /// the configuration sources consulted by the Resolve* methods. Excess positionals and unknown options + /// have already been rejected by CommandArgumentValidator, so at most one positional is present and + /// every option token is one the command declares. /// /// The positional tokens for the version advance command (from CommandParameters.Positionals). /// The option tokens for the version advance command (from CommandParameters.Options). /// The Buildvana configuration whose release section layers between the flags and the defaults. /// The parsed settings. - /// An option value is invalid, or an unrecognized argument or option was given. + /// An option value is invalid. public static VersionAdvanceSettings Parse(IReadOnlyList positionals, IReadOnlyList options, BuildvanaConfig config) { Guard.IsNotNull(positionals); Guard.IsNotNull(options); Guard.IsNotNull(config); - if (positionals.Count > 1) - { - throw new BuildFailedException($"Unexpected argument '{positionals[1]}' for command 'version advance'."); - } - var reader = new CliOptionReader(options); - var settings = new VersionAdvanceSettings(config.Release) + return new VersionAdvanceSettings(config.Release) { Change = positionals.Count > 0 ? positionals[0] : null, CheckPublicApi = ParseBool(reader.ReadValue("--check-public-api"), "--check-public-api"), Force = reader.ReadFlag("--force"), }; - - if (reader.Remaining.Count > 0) - { - throw new BuildFailedException($"Unknown option '{reader.Remaining[0]}' for command 'version advance'."); - } - - return settings; } /// diff --git a/tests/Buildvana.Tool.Tests/CommandArgumentValidatorTests.cs b/tests/Buildvana.Tool.Tests/CommandArgumentValidatorTests.cs index bbd2de53..68be4fd0 100644 --- a/tests/Buildvana.Tool.Tests/CommandArgumentValidatorTests.cs +++ b/tests/Buildvana.Tool.Tests/CommandArgumentValidatorTests.cs @@ -49,6 +49,83 @@ public async Task NonForwardingCommand_AllowsItsOwnOptionTokens() await Assert.That(parsed.OptionTokens.Count).IsEqualTo(2); } + [Test] + public async Task SettingsLessCommand_RejectsUnknownOption() + { + var command = CommandRegistry.Find("clean")!; + var parsed = CliArgSplitter.Split(["clean", "--bogus"]); + var exception = await Assert.That(() => CommandArgumentValidator.Validate(command, parsed, parsed.Positionals)) + .Throws(); + await Assert.That(exception!.Message).IsEqualTo("Unknown option '--bogus' for command 'clean'."); + } + + [Test] + public async Task SettingsLessCommand_RejectsUnknownOption_ViaAlias() + { + var command = CommandRegistry.Find("version")!; + var parsed = CliArgSplitter.Split(["version", "--bogus"]); + var exception = await Assert.That(() => CommandArgumentValidator.Validate(command, parsed, parsed.Positionals)) + .Throws(); + await Assert.That(exception!.Message).IsEqualTo("Unknown option '--bogus' for command 'version show'."); + } + + [Test] + public async Task SettingsLessCommand_ReportsOffendingTokensInCommandLineOrder() + { + var command = CommandRegistry.Find("clean")!; + var parsed = CliArgSplitter.Split(["clean", "junk", "--bogus"]); + var exception = await Assert.That(() => CommandArgumentValidator.Validate(command, parsed, parsed.Positionals)) + .Throws(); + await Assert.That(exception!.Message).IsEqualTo("Unexpected argument 'junk' for command 'clean'."); + } + + [Test] + public async Task SettingsCarryingCommand_RejectsUnknownOption() + { + var command = CommandRegistry.Find("release")!; + var parsed = CliArgSplitter.Split(["release", "--bogus"]); + var exception = await Assert.That(() => CommandArgumentValidator.Validate(command, parsed, parsed.Positionals)) + .Throws(); + await Assert.That(exception!.Message).IsEqualTo("Unknown option '--bogus' for command 'release'."); + } + + [Test] + public async Task SettingsCarryingCommand_ConsumesOptionValue() + { + var command = CommandRegistry.Find("release")!; + var parsed = CliArgSplitter.Split(["release", "--bump", "minor"]); + CommandArgumentValidator.Validate(command, parsed, parsed.Positionals); + await Assert.That(parsed.OptionTokens.Count).IsEqualTo(2); + } + + [Test] + public async Task SettingsCarryingCommand_AllowsInlineOptionValue() + { + var command = CommandRegistry.Find("release")!; + var parsed = CliArgSplitter.Split(["release", "--bump=minor"]); + CommandArgumentValidator.Validate(command, parsed, parsed.Positionals); + await Assert.That(parsed.OptionTokens.Count).IsEqualTo(1); + } + + [Test] + public async Task SettingsCarryingCommand_AllowsFlagOption() + { + var command = CommandRegistry.Find("version advance")!; + var parsed = CliArgSplitter.Split(["version", "advance", "--force"]); + CommandArgumentValidator.Validate(command, parsed, []); + await Assert.That(parsed.OptionTokens.Count).IsEqualTo(1); + } + + [Test] + public async Task SettingsCarryingCommand_RejectsValueOptionWithoutValue() + { + var command = CommandRegistry.Find("release")!; + var parsed = CliArgSplitter.Split(["release", "--bump"]); + var exception = await Assert.That(() => CommandArgumentValidator.Validate(command, parsed, parsed.Positionals)) + .Throws(); + await Assert.That(exception!.Message).IsEqualTo("Option '--bump' requires a value."); + } + [Test] public async Task NonForwardingCommand_RejectsPositionals_WhenNoArgumentsDeclared() { diff --git a/tests/Buildvana.Tool.Tests/ReleaseSettingsTests.cs b/tests/Buildvana.Tool.Tests/ReleaseSettingsTests.cs index 6e1b6991..c2039adb 100644 --- a/tests/Buildvana.Tool.Tests/ReleaseSettingsTests.cs +++ b/tests/Buildvana.Tool.Tests/ReleaseSettingsTests.cs @@ -146,12 +146,6 @@ public async Task Parse_Throws_OnInvalidBool() await Assert.That(() => Parse(["--dogfood", "maybe"])).Throws(); } - [Test] - public async Task Parse_Throws_OnUnknownOption() - { - await Assert.That(() => Parse(["--bogus"])).Throws(); - } - private static ReleaseSettings Parse(string[] options, BuildvanaConfig? config = null) { config ??= new BuildvanaConfig(); diff --git a/tests/Buildvana.Tool.Tests/VersionAdvanceSettingsTests.cs b/tests/Buildvana.Tool.Tests/VersionAdvanceSettingsTests.cs index 0b31faf7..3cf19f2f 100644 --- a/tests/Buildvana.Tool.Tests/VersionAdvanceSettingsTests.cs +++ b/tests/Buildvana.Tool.Tests/VersionAdvanceSettingsTests.cs @@ -64,18 +64,6 @@ public async Task Parse_Throws_OnInvalidBool() await Assert.That(() => Parse([], ["--check-public-api", "maybe"])).Throws(); } - [Test] - public async Task Parse_Throws_OnUnknownOption() - { - await Assert.That(() => Parse([], ["--bogus"])).Throws(); - } - - [Test] - public async Task Parse_Throws_OnExcessPositionals() - { - await Assert.That(() => Parse(["minor", "extra"], [])).Throws(); - } - private static VersionAdvanceSettings Parse(string[] positionals, string[] options, BuildvanaConfig? config = null) => VersionAdvanceSettings.Parse(positionals, options, config ?? new BuildvanaConfig()); }