Reject unknown options on commands that take none

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
 

src/Buildvana.Tool/CommandLine/BvOptionAttribute.cs

@@ -12,9 +12,11 @@ namespace Buildvana.Tool.CommandLine;
 /// <c>"-c|--configuration &lt;NAME&gt;"</c> or <c>"--no-color"</c>.
 /// </summary>
 /// <remarks>
-/// <para>This attribute carries help metadata only. It does not drive parsing: the option reader
-/// (<see cref="CliOptionReader"/>) is fed explicit names by each <c>*Settings</c> type. The help renderer
-/// reflects these attributes to print the OPTIONS grid.</para>
+/// <para>This attribute does not drive parsing — the option reader (<see cref="CliOptionReader"/>) is fed
+/// explicit names by each <c>*Settings</c> 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.</para>
 /// </remarks>
 [AttributeUsage(AttributeTargets.Property)]
 internal sealed class BvOptionAttribute : Attribute

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 <c>--</c> (everything to forward goes after it);
 /// non-forwarding commands have nowhere to forward, so they reject anything after <c>--</c>, and accept only as
-/// many positionals as their settings type declares via <see cref="BvArgumentAttribute"/>.
+/// many positionals as their settings type declares via <see cref="BvArgumentAttribute"/> and the options it
+/// declares via <see cref="BvOptionAttribute"/> (a command with no settings type declares none, so it takes no
+/// options at all). The settings type's <c>Parse</c> can therefore assume every option token it receives is one
+/// the command declares.
 /// </summary>
 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<BvArgumentAttribute> DeclaredArguments(CommandRegis
             .Select(static p => p.GetCustomAttribute<BvArgumentAttribute>())
             .OfType<BvArgumentAttribute>()];
     }
+
+    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<BvOptionAttribute> DeclaredOptions(CommandRegistration command)
+    {
+        if (command.SettingsType is null)
+        {
+            return [];
+        }
+
+        return command.SettingsType
+            .GetProperties(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
+            .Select(static p => p.GetCustomAttribute<BvOptionAttribute>())
+            .OfType<BvOptionAttribute>();
+    }
 }

src/Buildvana.Tool/Subcommands/ReleaseSettings.cs

@@ -70,34 +70,28 @@ private ReleaseSettings(ReleaseConfig? config, DotNetSettings dotNetSettings)
     public bool? Dogfood { get; init; }
 
     /// <summary>
-    /// Parses the command's option tokens into a <see cref="ReleaseSettings"/>, rejecting any option the command
-    /// does not recognize, and binds the configuration sources consulted by the <c>Resolve*</c> methods.
+    /// Parses the command's option tokens into a <see cref="ReleaseSettings"/> and binds the configuration
+    /// sources consulted by the <c>Resolve*</c> methods. Unknown options have already been rejected by
+    /// <c>CommandArgumentValidator</c>, so every option token is one the command declares.
     /// </summary>
     /// <param name="options">The option tokens for the <c>release</c> command (from <c>CommandParameters.Options</c>).</param>
     /// <param name="config">The Buildvana configuration whose <c>release</c> section layers between the flags and the defaults.</param>
     /// <param name="dotNetSettings">The resolved <c>dotnet</c> settings, providing the fallback build configuration.</param>
     /// <returns>The parsed settings.</returns>
-    /// <exception cref="BuildFailedException">An option value is invalid, or an unrecognized option was given.</exception>
+    /// <exception cref="BuildFailedException">An option value is invalid.</exception>
     public static ReleaseSettings Parse(IReadOnlyList<string> 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;
     }
 
     /// <summary>

src/Buildvana.Tool/Subcommands/VersionAdvanceSettings.cs

@@ -57,39 +57,28 @@ private VersionAdvanceSettings(ReleaseConfig? releaseConfig)
     public bool Force { get; init; }
 
     /// <summary>
-    /// Parses the command's positional and option tokens into a <see cref="VersionAdvanceSettings"/>, rejecting
-    /// anything the command does not recognize, and binds the configuration sources consulted by the
-    /// <c>Resolve*</c> methods.
+    /// Parses the command's positional and option tokens into a <see cref="VersionAdvanceSettings"/> and binds
+    /// the configuration sources consulted by the <c>Resolve*</c> methods. Excess positionals and unknown options
+    /// have already been rejected by <c>CommandArgumentValidator</c>, so at most one positional is present and
+    /// every option token is one the command declares.
     /// </summary>
     /// <param name="positionals">The positional tokens for the <c>version advance</c> command (from <c>CommandParameters.Positionals</c>).</param>
     /// <param name="options">The option tokens for the <c>version advance</c> command (from <c>CommandParameters.Options</c>).</param>
     /// <param name="config">The Buildvana configuration whose <c>release</c> section layers between the flags and the defaults.</param>
     /// <returns>The parsed settings.</returns>
-    /// <exception cref="BuildFailedException">An option value is invalid, or an unrecognized argument or option was given.</exception>
+    /// <exception cref="BuildFailedException">An option value is invalid.</exception>
     public static VersionAdvanceSettings Parse(IReadOnlyList<string> positionals, IReadOnlyList<string> 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;
     }
 
     /// <summary>

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<BuildFailedException>();
+        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<BuildFailedException>();
+        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<BuildFailedException>();
+        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<BuildFailedException>();
+        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<BuildFailedException>();
+        await Assert.That(exception!.Message).IsEqualTo("Option '--bump' requires a value.");
+    }
+
     [Test]
     public async Task NonForwardingCommand_RejectsPositionals_WhenNoArgumentsDeclared()
     {

tests/Buildvana.Tool.Tests/ReleaseSettingsTests.cs

@@ -146,12 +146,6 @@ public async Task Parse_Throws_OnInvalidBool()
         await Assert.That(() => Parse(["--dogfood", "maybe"])).Throws<BuildFailedException>();
     }
 
-    [Test]
-    public async Task Parse_Throws_OnUnknownOption()
-    {
-        await Assert.That(() => Parse(["--bogus"])).Throws<BuildFailedException>();
-    }
-
     private static ReleaseSettings Parse(string[] options, BuildvanaConfig? config = null)
     {
         config ??= new BuildvanaConfig();

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<BuildFailedException>();
     }
 
-    [Test]
-    public async Task Parse_Throws_OnUnknownOption()
-    {
-        await Assert.That(() => Parse([], ["--bogus"])).Throws<BuildFailedException>();
-    }
-
-    [Test]
-    public async Task Parse_Throws_OnExcessPositionals()
-    {
-        await Assert.That(() => Parse(["minor", "extra"], [])).Throws<BuildFailedException>();
-    }
-
     private static VersionAdvanceSettings Parse(string[] positionals, string[] options, BuildvanaConfig? config = null)
         => VersionAdvanceSettings.Parse(positionals, options, config ?? new BuildvanaConfig());
 }