Reject unknown options on commands that take none

[rdeago]

Closes #314.

Option validation lived in two places: CommandArgumentValidator (forwarded tokens, excess positionals) and each command's *Settings.Parse (leftover option tokens). Commands with no settings type — clean, sync-sdk, version show — fell through both nets, so bv clean --bogus ran normally and typos went unnoticed.

CommandArgumentValidator is now the single authority for unknown-option rejection: it consumes every option declared via BvOptionAttribute on the command's settings type (ValueName tells it which options swallow the following token) and rejects whatever is left, as Unknown option '--bogus' for command 'clean'. A settings-less command is simply one that declares zero options — no special case. The now-redundant leftover checks in ReleaseSettings.Parse and VersionAdvanceSettings.Parse are gone, so a future *Settings type cannot reintroduce the bug by forgetting the check. Offending tokens are reported in command-line order (excess positionals before unknown options), and — since validation runs before the SDK version check — bv release --bogus now reports the typo even in a repository with a mismatched SDK pin.

Note: the issue names clean and version show; sync-sdk is settings-less too and is covered by the same fix.

Note: the first review round suggested centralizing unknown-option rejection as a follow-up issue. It is folded into this PR instead: the changes are small and self-contained, and a separate PR wasn't worth the overhead.

Verification

• New/updated tests: unknown options rejected for settings-less commands (clean --bogus, and version --bogus pinning the canonical version show name in the message) and for settings-carrying commands (release --bogus); declared options consumed correctly (space-separated and inline values, flags, missing-value error); clean junk --bogus reports junk first.
• 469 tests pass solution-wide; dotnet bv pack green; ReSharper inspectcode --swea --severity=WARNING: zero results.

🤖 Generated with Claude Code

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 4m 40s —— View job

---

Review: reject unknown options on commands that take none

Ric — I read the diff against origin/main plus the surrounding command-line plumbing (CliArgSplitter, CliOptionReader, CommandRegistry, Program, BvHelpRenderer, both *Settings.Parse). The change is correct and I found no way to make it reject something legitimate. Notes below are nits and one design follow-up, no blockers.

• Read the PR diff and the surrounding validation code
• Check the settings-less command set and global-option handling
• Verify --help and other cross-cutting paths still work
• Review tests and changelog

Correctness — what I checked and why it holds

• No documented global can be mistaken for an unknown option. The set stripped in CliArgSplitter.Split (src/Buildvana.Tool/CommandLine/CliArgSplitter.cs:33-39) — -v|--verbosity, --color, --no-color, --nologo, --skip-sdk-check, --version, -h|--help — matches the BvOption templates on GlobalSettings exactly, and those are what the help grid advertises (BvHelpRenderer.WriteGlobalOptions, pinned by SettingsHelpReflectionTests:14). So bv clean --nologo -v diag still passes. Both inline (--verbosity=diag) and space-separated forms are consumed, so neither leaves residue.
• --help is unaffected: Program.cs:79-83 short-circuits before Validate, so bv clean --bogus --help prints help rather than failing. Consistent with the rest of the tool.
• No overlap with the forwarding branch: every consumesAllArguments: true command also has SettingsType == null, but the new check sits in the else, so those keep the more helpful "Forward arguments to dotnet after --" wording. No double message, no conflict.
• Bonus coverage: options before the subcommand land in OptionTokens too (Classify at CliArgSplitter.cs:107-111), so bv --bogus clean is now caught as well, not just bv clean --bogus.
• Message wording matches ReleaseSettings.cs:97 and VersionAdvanceSettings.cs:89 verbatim, and using command.Name instead of a hardcoded literal is the better of the two idioms.
• Changelog is in the right section (## Unreleased changes → ### Bugs fixed in this release). No docs change needed: there's no per-command option reference in docs/ — --help is the reference — so the area:docs label is just the CI labeler reacting to CHANGELOG.md.

I could not run dotnet build/test here (the sandbox denied dotnet), but the Build, test, and pack run on the head commit is green.

Nits

1. Error precedence is now option-before-positional. CommandArgumentValidator.cs:51 runs ahead of the excess-positional check at line 57, so bv clean junk --bogus reports --bogus while junk — the earlier offender — goes unmentioned until the next run. Reporting left-to-right would be friendlier. Genuinely arguable the other way (a stray option is the likelier typo), so take it or leave it. Fix this →

2. The new tests assert the type, not the message. CommandArgumentValidatorTests.cs:61-66 is named _ViaAlias, but since it only asserts Throws<BuildFailedException>() it can't distinguish the alias path from anything else — and the interesting fact about that path is that bv version --bogus reports the canonical name: Unknown option '--bogus' for command 'version show'. That's the right call (CommandRegistration.Name is documented as the error-message form), but it's currently implied rather than pinned. Asserting the message in the clean test would also pin the exact wording CHANGELOG.md:103 promises users. The file's existing style is type-only, so this is a deliberate step up — SelfVersionServiceTests already does capture-and-assert-message where wording matters. Fix this →

Follow-up worth an issue (not this PR)

The fix closes the SettingsType == null hole, but its twin is still open: nothing structurally enforces the reader.Remaining.Count > 0 check inside a *Settings.Parse. Both current implementations have it; a third one that forgets reintroduces exactly the bug #314 describes, and no test would notice.

Making the validator the single authority would close it for good — reflect BvOptionAttribute off SettingsType, using ValueName is not null to know which options swallow the following token. Then:

• the SettingsType is null special case stops being a special case (it's just "zero declared options");
• the Remaining checks in ReleaseSettings.Parse / VersionAdvanceSettings.Parse become redundant, and their hardcoded 'release' / 'version advance' literals give way to command.Name;
• it fixes a timing asymmetry this PR makes visible: Parse runs at DI resolution (Program.cs:146), i.e. after EnsureSdkVersionMatch() at Program.cs:111-113. In a repo with a stale global.json pin, bv release --bogus reports the SDK mismatch rather than the typo, while bv clean --bogus now fails immediately and correctly.

All pre-existing and clearly out of scope here. Fix this →

One editorial question on the changelog

CHANGELOG.md:103 lists clean, sync-sdk, and version show — but sync-sdk and version show are both new in this same unreleased cycle (lines 44-45), so for a reader upgrading from 1.1.10 there is nothing they "no longer" do. Only clean (née prepare) is a real behavior change against the last release. Consider framing it as the fix to clean plus the general rule, e.g. "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." Your call — purely presentational.

---

Nothing here blocks merge. If you want, I can apply nits 1 and 2 on this branch.
· branch fix/314-settingsless-unknown-options

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.27%. Comparing base (4e11635) to head (6653a91).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
+ Coverage   47.12%   47.27%   +0.15%     
==========================================
  Files         126      126              
  Lines        4036     4044       +8     
  Branches      711      714       +3     
==========================================
+ Hits         1902     1912      +10     
+ Misses       2046     2045       -1     
+ Partials       88       87       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rdeago]

All four findings addressed, one commit per finding:

• Nit 1 (c28a18a): excess positionals are now checked before unknown options, so bv clean junk --bogus reports junk; a new test pins it. One accepted limitation: bv --bogus clean junk also reports junk first — true left-to-right ordering would require tracking original token positions, which isn't worth it for that shape.
• Nit 2 (bba4f19): both settings-less tests now assert the exact message; the alias test pins the canonical version show name.
• Follow-up (2babf9c): folded into this PR rather than spun off — the changes are small and self-contained. CommandArgumentValidator is now the single authority: it consumes every BvOptionAttribute-declared option (ValueName marks the value-swallowing ones) and rejects the remainder; a settings-less command is just the zero-options case. The redundant Remaining checks in ReleaseSettings.Parse and VersionAdvanceSettings.Parse are gone — plus the equally redundant excess-positional check in VersionAdvanceSettings.Parse — and the timing asymmetry is fixed: bv release --bogus reports the typo before the SDK version check. BvOptionAttribute's "help metadata only" remark was updated to match its new validation role.
• Editorial (6653a91): the changelog entry is reframed around clean plus the general rule, as suggested, and now also mentions the report-before-SDK-check timing.

Verification: 469 tests pass solution-wide, dotnet bv pack green, ReSharper inspectcode --swea --severity=WARNING zero results.

@claude please take another look. From this round on, flag only merge-blocking issues; anything else, list briefly as non-blocking notes that need no re-review.

[claude]

Claude finished @rdeago's task in 3m 40s —— View job

---

Re-review: reject unknown options on commands that take none

Read all four commits against origin/main, plus CliOptionReader, CliArgSplitter, BvHelpRenderer, CommandRegistry, Program, and both *Settings.Parse.

• Read the updated diff (c28a18a, bba4f19, 2babf9c, 6653a91)
• Audit the centralized validator against every declared option
• Check the removed Parse checks left no gap
• Verify tests and changelog

No merge-blocking issues. Ship it.

What I verified on the centralization commit (2babf9c)

The reflection-driven consumption is the only part with real risk, so I checked it end to end:

• No aliasing of the caller's tokens. CliOptionReader copies (_tokens = [..tokens], CliOptionReader.cs:33), so the validator's consumption doesn't mutate parsed.OptionTokens; Parse still sees the full list at DI resolution. The three OptionTokens.Count assertions in the new tests pin exactly this.
• Declaration matches consumption for every option that exists today. --force is the only ValueName is null template and the only ReadFlag call in Parse; -c|--configuration, --bump, --check-public-api, --dogfood all carry <...> and are all ReadValue. No flag/value mismatch.
• Property order matches Parse order in both settings types (ReleaseSettings 40/47/61/68 vs. Parse 90-93; VersionAdvanceSettings 48/55 vs. 79-80), so the validator can't throw requires a value on a line Parse would have accepted. SettingsHelpReflectionTests:17-29 already pins that order — those tests now do double duty as a guard on the accepted-option set.
• Splitting LongNames/ShortNames into separate ReadValue calls is safe. It differs from Parse's single two-name call only in which occurrence wins under -c X --configuration Y; the validator discards the value and both calls consume, so Remaining is identical either way.
• DeclaredOnly binding flags match BvHelpRenderer.EnumerateOptions (BvHelpRenderer.cs:63) — help can't advertise an option validation rejects. Both settings types are sealed with no base, so nothing is hidden by DeclaredOnly anyway.
• Timing claim holds: Program.cs:93 Validate runs before BuildServiceProvider/EnsureSdkVersionMatch (line 111-113), so bv release --bogus reports the typo first. --help still short-circuits earlier (line 79-83).
• Globals unaffected: CliArgSplitter.Split strips all seven globals plus -h|--help before Classify, so bv clean --nologo -v diag stays valid.
• Deleting the positionals.Count > 1 check in VersionAdvanceSettings.Parse is covered — [BvArgument("[CHANGE]")] is the sole declared argument, so the validator's positionals.Count > arguments.Count produces the same message.

Nits 1 and 2 are resolved as described; the alias test now pins version show, which is the fact worth pinning. Changelog reframing reads well and the added SDK-check sentence is accurate.

CI on head 6653a91: Build, test, and pack and CodeQL both green. dotnet is still denied in my sandbox, so I did not re-run the build locally.

Non-blocking notes — no re-review needed

• Declaration/read drift is now the one unguarded seam: a future [BvOption("--foo <X>")] whose Parse calls ReadFlag("--foo") (or never reads it) would have the validator and Parse disagree, and the per-settings Parse tests bypass the validator so nothing would catch it. CommandRegistry's existing argument-ordering check (CommandRegistry.cs:147-169) is the natural home for an assertion if it ever bites.
• Unknown option 'minor' for command 'release' is still the message for a stray non-option token that lands in OptionTokens (bv release --bump --bogus minor). Inherited verbatim from the deleted Parse checks, not introduced here.
  · branch fix/314-settingsless-unknown-options