[bv] Forward command-line arguments verbatim to dotnet

[rdeago]

Closes #260 (Phase 3 of #257). With this merged, all three phases are complete.

Summary

• bv restore, bv build, bv test, and bv pack are now marked [ConsumeAllArguments] and forward any extra arguments verbatim, in encounter order, to their underlying dotnet invocation(s). bv no longer parses or validates them — malformed/unknown args now produce a dotnet (or, for test, a Microsoft.Testing.Platform) error instead of a bv one. Previously only -p://p: properties were forwarded.
• Each forwarded dotnet invocation is composed as: leading group (-nologo, -v <verbosity>, --no-restore/--no-build where applicable, overridable Configuration default) → user args → trailing group (authoritative -p:ContinuousIntegrationBuild=…).
• -maxcpucount:1 is no longer forced; MSBuild uses its default parallelism unless the user forwards their own -m.
• --main-branch is promoted to a position-flexible global option (shown in GLOBAL OPTIONS, honored by the Git-touching commands).
• Configuration (-c/--configuration) moves from the deleted BuildSettings to ReleaseSettings. Pipeline commands no longer parse it (forwarded; bv emits Release as an overridable default). release keeps it, since it needs the value to locate artifacts.
• Help: the [MSBuild: …] indicators are gone; root help marks the forwarding commands and per-command help gains a FORWARDED ARGUMENTS section.
• Removes MSBuildProperties, MSBuildOptionKinds, AcceptsMSBuildOptionsAttribute, BuildSettings, BuildSettingsHolder; adds ConsumeAllArgumentsAttribute, ConsumeAllArgumentsCommands (a reflection map shared by Program.Main and BvHelpProvider), and a ForwardedArguments DI holder.

Notable design decisions

Flagged here so reviewers don't have to dig (a few diverge from #260's written proposal):

• Configuration is overridable, not authoritative (per design discussion, a change from [bv] Forward extra command-line arguments to the underlying dotnet invocation #260's stated acceptance criteria). bv emits the default in the leading group as an MSBuild property (-p:Configuration=Release, --property:Configuration=Release for test since dotnet test rejects -p:), so a forwarded -c Debug/-p:Configuration=Debug wins under MSBuild's last-wins. This relies on dotnet preserving arg order to MSBuild — the same observed-not-documented last-wins reliance Overhaul the bv CLI command and option surface #257 already accepts.
• restore emits no Configuration, matching today's behavior (dotnet restore has no -c). ContinuousIntegrationBuild is still emitted (trailing) for all four.
• The dotnet test MTP probe stays minimal (dotnet msbuild <proj> -nologo -getProperty:IsTestingPlatformApplication) — it deliberately does not receive the forwarded args, which may be test-app flags dotnet msbuild would reject.
• --verbosity is always forwarded, so dotnet's build output now follows bv's verbosity (default normal) instead of dotnet's own default (minimal).
• GlobalOptions is now public (was internal) — the public GitService/DotNetService take it as a constructor parameter (same reason the removed MSBuildProperties was public).

Test plan

• dotnet bv pack — 0 errors, 0 warnings; both Buildvana.Sdk and bv packages produced.
• dotnet dnx JetBrains.ReSharper.GlobalTools inspectcode --swea --severity=WARNING … Buildvana.slnx — 0 results.
• Manual (freshly-built bv): root help (--main-branch in globals, forward annotations), bv build --help (FORWARDED ARGUMENTS section), bv release --help (keeps -c, no forwarding section), --version, --nologo --version, no-args help, unknown-command error, missing--v-value error.
• ⚠️ Not verified at runtime: the bv test forwarding path (--nologo/-v/--property: → dotnet test → MTP). The repo has no test projects, so this path is implemented to spec but only compile-checked.

[rdeago]

@claude please review this PR.

[claude]

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

---

PR Review: Forward command-line arguments verbatim to dotnet

• Gather context
• Read changed files
• Analyze code
• Post review findings

---

Overall this is a well-structured PR. The design is sound and the implementation is clean. I found one correctness concern, one brittleness, and a few minor observations.

---

1. RestoreSolutionAsync — configuration parameter accepted, guarded, and documented but never used

src/Buildvana.Tool/Services/DotNetService.cs lines 79–89

public Task RestoreSolutionAsync(SolutionContext solution, string configuration, IReadOnlyList<string> forwardedArgs)
{
    Guard.IsNotNull(solution);
    Guard.IsNotNullOrEmpty(configuration);   // ← guards a value never read below
    Guard.IsNotNull(forwardedArgs);
    _logger.LogInformation("Restoring NuGet packages for solution...");
    List<string> args = ["restore", solution.SolutionPath, "--disable-parallel", "-nologo", "-v", Verbosity];
    args.AddRange(forwardedArgs);
    args.Add(ContinuousIntegrationBuildArg(dotnetTest: false));
    return RunDotNetAsync(args);
}

configuration is never inserted into args. The PR description explains this is intentional (dotnet restore has no -c), but as written the parameter is actively misleading:

• The Guard enforces a constraint on a value that has no effect, implying the value matters.
• The XML doc comment says "The MSBuild configuration to build", which is untrue for this method.
• BuildSteps.RestoreAsync dutifully receives a default configuration = DefaultConfiguration and forwards it — silently dropped.

The cleanest fix is to remove the parameter from RestoreSolutionAsync entirely (callers pass it but it's not needed) and update BuildSteps.RestoreAsync accordingly. If you want to keep API shape parity with the other overloads, at minimum drop the Guard and update the XML comment to say the parameter is accepted for API consistency but not forwarded.

---

2. ConsumeAllArgumentsCommands — name derivation is tightly coupled to Spectre registration naming

src/Buildvana.Tool/Cli/ConsumeAllArgumentsCommands.cs lines 35–43

if (type.GetCustomAttribute<ConsumeAllArgumentsAttribute>() is not null)
{
    result.Add(type.Name[..^"Command".Length]);
}

This strips "Command" from the class name (BuildCommand → "Build") and stores it case-insensitively. That maps correctly onto single-word commands registered with matching lowercase names (config.AddCommand<BuildCommand>("build")). But there's a silent assumption: class name → strip suffix → matches Spectre registration. This breaks for a hyphenated command name (e.g., MySpecialCommand would yield "MySpecial", not "my-special"). It also breaks if a command is registered under a different alias than the class name.

Not a bug today, but worth an inline comment on the limitation, or a unit test to catch the invariant if you add a hyphenated command later.

---

3. Minor: BaseSettings.MainBranch appears in help but is always null in handlers

src/Buildvana.Tool/Cli/BaseSettings.cs

[CommandOption("--main-branch <NAME>")]
[Description("Name of the repository's main branch. Defaults to 'main'.")]
public string? MainBranch { get; init; }

This property exists purely to drive help generation. The pre-parser strips --main-branch before Spectre binds it, so settings.MainBranch will always be null in any command handler. The real value lives in GlobalOptions.MainBranch. This is consistent with the existing pattern for --verbosity, and the help output is technically correct (the FORWARDED ARGUMENTS section says "other than the options above", so --main-branch is implicitly excluded from forwarding). Worth a brief comment so future contributors don't try to read settings.MainBranch and wonder why it's null.

---

4. Minor: SplitCommandArgs — help-flag detection uses ordinal Contains

src/Buildvana.Tool/Program.cs line 194

var helpRequested = nonGlobal.Contains("--help") || nonGlobal.Contains("-h");

TryNormalizeHelp already normalizes both flags to lowercase canonical forms before they land in nonGlobal, so the ordinal comparison is correct. Just worth noting this invariant is load-bearing: if TryNormalizeHelp ever returns a non-lowercase canonical, this check would silently miss it.

---

5. Nit: ReleaseCommand — pre-pipeline uses BuildSteps.* (which read ForwardedArguments from DI) but release is not a ConsumeAllArguments command

src/Buildvana.Tool/Cli/ReleaseCommand.cs lines ~31–36

await BuildSteps.CleanAsync(services).ConfigureAwait(false);
await BuildSteps.RestoreAsync(services, configuration).ConfigureAwait(false);
await BuildSteps.BuildAsync(services, configuration).ConfigureAwait(false);
await BuildSteps.TestAsync(services, configuration).ConfigureAwait(false);

Since release is not in ConsumeAllArgumentsCommands, ForwardedArguments.Args is an empty list, so BuildSteps.* silently forward nothing — which is the right behavior. The second block (below the changelog logic) calls dotnet.* directly with [] for forwarded args. The asymmetry is harmless, but the pre-pipeline block could also call dotnet.* directly with [] for clarity, matching the style of the second block.

---

Summary

The design is clean and the implementation is correct with one exception: the unused configuration parameter in RestoreSolutionAsync (issue 1) is the only thing I'd call out as needing a fix before merge — it's actively misleading. The rest are observations and a minor brittleness to be aware of. The CHANGELOG entries are thorough and accurate.

[rdeago]

Hey @claude 😜

1. RestoreSolutionAsync — configuration parameter accepted, guarded, and documented but never used

Fixed.

2. ConsumeAllArgumentsCommands — name derivation is tightly coupled to Spectre registration naming

Fixed by refactoring the association between command names and implementing classes using an ImplementsCommand attribute that also includes a "consumeAllArguments" flag.

3. Minor: BaseSettings.MainBranch appears in help but is always null in handlers

This will be fixed when we introduce a settings file, as mainBranch will be moved there.

4. Minor: SplitCommandArgs — help-flag detection uses ordinal Contains

Good catch, worth a one-line comment before var helpRequested.

5. Nit: ReleaseCommand — pre-pipeline uses BuildSteps.* (which read ForwardedArguments from DI) but release is not a ConsumeAllArguments command

Fair point. Fixed by using BuildSteps calls in both blocks.

[claude]

Claude finished @rdeago's task in 1m 43s —— View job

---

Follow-up review

• Read current state of changed files
• Verify fix for issue 1 (RestoreSolutionAsync unused configuration)
• Verify fix for issue 2 (ImplementsCommandAttribute refactor)
• Verify fix for issue 3 (BaseSettings.MainBranch documentation)
• Verify fix for issue 4 (SplitCommandArgs ordinal Contains comment)
• Verify fix for issue 5 (ReleaseCommand uses BuildSteps in both blocks)

---

All five fixes are in and look correct. Summary:

Issue 1 (a280bd1): RestoreSolutionAsync signature is now (SolutionContext solution, IReadOnlyList<string> forwardedArgs) — the dead configuration parameter and its Guard are gone. XML doc updated accordingly. BuildSteps.RestoreAsync calls it without configuration. Clean fix.

Issue 2 (dfbd6d6): The old "strip Command suffix from class name" approach is replaced by ImplementsCommandAttribute(name, consumesAllArguments). CommandRegistry discovers commands by reflection on this attribute and uses attribute.Name directly — no string-manipulation fragility. ConsumesAllArguments(commandName) does a case-insensitive name match. Fail-fast for any pipeline name with no implementing class (line 71–78) is a nice bonus. Solid.

Issue 3 (c615448): BaseSettings.MainBranch now has a <remarks> doc that explicitly says it's always null in command handlers and that the effective value comes from GlobalOptions.MainBranch. The type-level remarks were also updated to explain the pre-scan pattern. Good enough for now; the deferred settings-file fix will complete the story.

Issue 4 (d8e4db2): Comment is present at line 189: // The ordinal Contains works because TryNormalizeHelp already canonicalized --help/-h to lowercase. — exactly what was needed.

Issue 5 (b552a69): Both the pre-pipeline block (lines 39–42) and the final build block (lines 178–181) in ReleaseCommand now consistently use BuildSteps.*. Since release is not a ConsumeAllArguments command, ForwardedArguments.Args is [], so the calls are semantically equivalent to the former direct dotnet.* calls — but the style is now consistent throughout the file.

No further issues. The PR looks good to merge from my end.