Wire existing bv options to buildvana.json (Config Phase 2)

[rdeago]

Closes #269. Config Phase 2 (sub-issue of #267): makes the *Settings injectables config-aware so each option resolves flag → buildvana.json → default, and replaces mainBranch with release.generateDocsFrom. The config model/loader/schema from Phase 1 (#268) are unchanged — this PR only consumes them.

What changed

• DotNetSettings / DotNetInvocationSettings (new): resolve dotnet.configuration plus the per-command sub-objects (all, restore, build, test, pack, nugetPush), each carrying args + env.
• DotNetService: injects DotNetSettings; each invocation passes its own sub-object to RunDotNetAsync, which merges dotnet.all then the per-command settings. Argument order is base → dotnet.all → per-command → forwarded command-line args (so a -- argument still wins); environment variables apply dotnet.all then per-command. bv-internal probes pass no sub-object, so they receive no user-configured args/env.
• BuildPipeline: takes its default build configuration from DotNetSettings.Configuration instead of a hardcoded "Release".
• ReleaseSettings: config-aware resolution — configuration chain (--configuration → release.configuration → dotnet.configuration), release.checkPublicApi, release.dogfood, release.changelogUpdates + release.emptyChangelog, and release.generateDocsFrom.
• ChangelogService.PrepareForRelease: substitutes release.emptyChangelog text into an empty "Unreleased changes" section; an empty section with no configured substitute fails the release (in ReleaseCommand, which owns the policy).
• mainBranch removed end-to-end: deleted GitService.MainBranch/FindMainBranch, the --main-branch global flag and its pre-parsing. Documentation generation is now gated by matching the current short branch name against the release.generateDocsFrom regexes (default ["^main$", "^master$"]); the changelog permalink in generated release notes now points at the current branch.

Breaking changes

• --main-branch removed (docs gating now via release.generateDocsFrom).
• --unstable-changelog / --require-changelog removed with no CLI replacement (changelog policy is repo-stable: release.changelogUpdates + release.emptyChangelog).

CHANGELOG updated accordingly (amended the in-cycle, still-unreleased entries that referenced the now-removed flags rather than appending contradictory bullets).

Testing

• dotnet build Buildvana.slnx — clean (0 warnings).
• Buildvana.Tool.Tests — 52/52 pass (added coverage for the configuration chain, release config layering, and generateDocsFrom matching).
• ReSharper inspectcode --severity=WARNING — 0 results.

🤖 Generated with Claude Code

[rdeago]

@claude please review this PR.

[claude]

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

---

PR Review

• Read changed files
• Analyze settings resolution chains and logic
• Check for correctness issues
• Verify test coverage
• Post review findings

---

Overall the PR is clean and well-structured. The three-tier resolution chain (flag → config → default) is applied consistently, and the removal of mainBranch/FindMainBranch in favour of release.generateDocsFrom regexes is a solid upgrade. No correctness bugs found. A few observations worth considering:

---

ContinuousIntegrationBuildArg position change (behaviour shift)

src/Buildvana.Tool/Services/DotNetService.cs

In the old code, forwardedArgs were spread into the base args array before ContinuousIntegrationBuildArg, so the CI flag came last and was authoritative — a user's -- -p:ContinuousIntegrationBuild=false was silently overridden.

In the new code the CI arg is part of args (the base), and commandLineArgs (forwarded) are appended by RunDotNetAsync after it. As the PR states, "forwarded args always win." So now bv build -- -p:ContinuousIntegrationBuild=false would actually override the server-injected value.

This is consistent with the new design, but it is a subtle behavioural change from before. Worth a conscious decision rather than just a side-effect of the refactor. If the intent is to make the CI flag immutable on cloud builds, it should remain last (or move to after commandLineArgs); if forwarded-wins is the intended policy everywhere, the current placement is correct. Either way, a comment on the property or a test asserting the ordering would make the intent explicit.

---

Resolve allocates a Dictionary when config.Env is null

src/Buildvana.Tool/Services/DotNetSettings.cs, line 62

: new(config.Args ?? [], config.Env ?? new Dictionary<string, string?>());

When config is non-null but Env is absent, a fresh Dictionary<string, string?> is allocated rather than reusing ReadOnlyDictionary<string, string?>.Empty (the same static instance already used in DotNetInvocationSettings.Empty). The BuildEnvironment null-return fast-path (all.Count == 0 && perCommand.Count == 0) is unaffected, so there is no functional issue — this is a trivial allocation nit. Swapping to ReadOnlyDictionary<string, string?>.Empty would make the empty-settings path fully allocation-free. Fix this →

---

dotnet.all is merged into nuget push invocations

src/Buildvana.Tool/Services/DotNetService.cs, line 247 / RunDotNetAsync

configuredArgs = [.. _settings.All.Args, .. invocation.Args] applies to every non-internal RunDotNetAsync call, including nuget push. If a user places MSBuild flags such as -p:Foo=Bar or -m:1 in dotnet.all.args, they will be forwarded to dotnet nuget push, which ignores unknown flags silently. This is probably harmless in practice, but it is worth documenting in the schema / buildvana.json docs so users know dotnet.all truly means all dotnet invocations.

---

MatchesDocsBranch guard pattern

src/Buildvana.Tool/Subcommands/ReleaseSettings.cs, lines 157–160

Guard.IsNotNull(branch);
if (string.IsNullOrEmpty(branch))
{
    return false;
}

Guard.IsNotNull throws on null, making the null arm of IsNullOrEmpty unreachable. The two guards are not wrong, but slightly inconsistent — either Guard.IsNotNullOrEmpty (and remove the if) or keep the graceful empty-return and drop the Guard in favour of only the if. The current shape is fine since ReleaseCommand already validates !string.IsNullOrEmpty(git.CurrentBranch) before reaching this, but cleaning it up would clarify intent.

---

Test coverage of arg/env merging

BuildEnvironment and the configuredArgs construction in RunDotNetAsync are not unit-tested directly. The logic is simple enough that the risk is low, but a couple of focused tests for the "all + per-command merge" and "null invocation skips configured args" cases would catch any future regression here cheaply.

---

Everything else looks correct

• Resolution chains (--flag → release.X → dotnet.X → default) in ReleaseSettings are correct and well-tested.
• MatchesDocsBranch correctly returns false on empty branch and handles RegexMatchTimeoutException.
• GenerateDocsFrom: [] (explicit empty list) correctly disables docs on every branch (no fallback to defaults), which is the right semantic for an explicit override.
• The emptyChangelogSubstitute flow in ReleaseCommand + PrepareForRelease is consistent: HasUnreleasedChanges() and CollectNewSectionLines() apply the same empty-section semantics; the double-check in PrepareForRelease is redundant but safe.
• Probe calls (InvocationKind.Internal) correctly receive no user-configured args.
• Changelog URL change from _git.MainBranch to _git.CurrentBranch is intentional and matches the stated design.
• DotNetInvocationSettings as a record is fine for this use; it's never compared by value.

[rdeago]

Thanks for the review @claude!

ContinuousIntegrationBuildArg position change (behaviour shift)

This is a deliberate decision, now enforced even more strictly. The ContinuousIntegrationBuild MSBuild property shall not be user-overridable.

Resolve allocates a Dictionary when config.Env is null

Fixed.

dotnet.all is merged into nuget push invocations

This is a deliberate decision and will need documenting as soon as we have actual documentation. For now, we'll count on "all" being pretty self-explanatory.

MatchesDocsBranch guard pattern

Fixed. Now Guard guards against nulls and the if condition only checks for an empty branch.

Test coverage of arg/env merging

Following a refactor that happened in this PR, the elements you mentioned are gone. Tests have been added respecting the spirit of your request though: argument ordering and environment layering are properly covered.

Other, unrelated changes have also been added. Please review again.

[claude]

Claude finished @rdeago's task in 2m 36s —— View job

---

PR Review (Round 2)

• Read changed files
• Verify ContinuousIntegrationBuildArg "enforced more strictly" change
• Verify Resolve Dictionary → ReadOnlyDictionary.Empty fix
• Verify MatchesDocsBranch Guard pattern fix
• Review test coverage for arg/env merging
• Review other changes (OutputStreaming extraction, config annotation, changelog, BuildPipeline, GitHubServerAdapter)
• Post review findings

---

All previous concerns have been addressed correctly. Here's what I found on each point, plus a look at the new additions:

---

Previous review — resolution status

ContinuousIntegrationBuildArg now in trailingArgs ✓
ContinuousIntegrationBuildArg is now passed via trailingArgs at every call site (RestoreSolutionAsync, BuildSolutionAsync, TestSolutionAsync, PackSolutionAsync). The XML doc on RunDotNetAsync makes the contract explicit: "trailing args cannot be overridden by configured or command-line arguments (e.g. the ContinuousIntegrationBuild property)". Clean.

Resolve allocation fix ✓
DotNetSettings.cs line 63 now correctly uses ReadOnlyDictionary<string, string?>.Empty when config.Env is null.

MatchesDocsBranch guard pattern ✓
Guard.IsNotNull(branch) handles the null case, if (branch.Length == 0) return false; handles the empty case. No redundancy, clear intent.

Test coverage for arg/env merging ✓
DotNetServiceMergeInvocationTests covers exactly the right paths:

• Argument ordering (base → tiers → command-line → trailing)
• Later tier overrides earlier tier for the same env key
• Null env value preserved as a removal marker
• Empty tiers: [] produces no configured args and leaves environment null (the bv-internal probe path)
• Tiers with args but no env variables also leave environment null

---

New changes

DotNetService.OutputStreaming.cs ✓
Clean extraction of the OutputStreaming struct from the (now-deleted) DotNetService.InvocationKind.cs into its own partial-class file. The three factories (Disabled, Unconditional, AtVerbosity) are clear; AtVerbosity taking an argument is correctly a method rather than a property.

DotNetInvocationConfig.cs ✓
Adding [UsedImplicitly(ImplicitUseTargetFlags.WithMembers)] is the right fix for ReSharper warning suppression on JSON-deserialized properties. XML doc added consistently.

BuildPipeline.cs ✓
DefaultConfiguration constant removed; configuration parameters are now nullable and resolve via configuration ?? _dotnetSettings.Configuration in RunAsync. The resolution is in the right place — a single point rather than scattered at call sites.

GitHubServerAdapter.cs ✓
Changelog permalink now uses _git.CurrentBranch instead of _git.MainBranch. Consistent with the stated design.

CHANGELOG.md ✓
Phase 1 bullet rewritten to reflect that buildvana.json is no longer inert. New Phase 2 entries cover the dotnet.* arg/env settings and the release.* settings, including the breaking removals of --unstable-changelog/--require-changelog and --main-branch. The env-var removal list is trimmed to match only the vars that were actually removed. All consistent with the PR.

---

Nothing significant to flag

The code is correct and consistent throughout. LGTM — ready to merge.