Split configuration into wire models and a resolved domain model

[rdeago]

Checklist of related issues / discussions

• Closes Split configuration into wire models and a resolved domain model #350

Proposed changes

BuildvanaConfig modeled the configuration file; nothing modeled the settings, so every consumer resolved defaults and precedence privately, across the five idioms catalogued in #350. This PR gives resolution a home, as designed there:

• Wire models (Buildvana.Core.Configuration): *JsonConfig records faithful to the file, all members nullable (null = not stated in the file), plus CommandLineOverrides and the file discovery/loading code moved out of Buildvana.Runtime. The provider types are renamed to match (BuildvanaJsonConfigProvider and friends).
• Domain model (Buildvana.Runtime): the same record names, now the single resolved answer. Every default is a property initializer on the record that owns it; a nullable member is a domain option whose null has exactly one documented meaning, never "unspecified". The assembly keeps no JSON-file, schema, or command-line knowledge and stays strictly BCL.
• One factory (BuildvanaConfigFactory.Create(json, commandLine)): the only place precedence is applied — scalars flag → file → default; dotnet args appended all → per-command → forwarded after --; env layered by key; release.configuration falling back to the resolved dotnet.configuration; the prerelease feed falling back to the release feed. Both producers compose through it: bv (DI, with a real CommandLineOverrides) and ComputeVersion (task host, null).
• The DotNetService seam: the factory resolves each command's final args and environment; the service contributes only its base and trailing arguments. The DotNetSettings family dissolves, and forwarded arguments become a command-line contribution to the same resolution instead of a fourth path for the build configuration: the -c/--configuration tokens bv promotes out of the forwarded stream are also consumed by it, so bv pack -- -c Debug resolves and builds Debug without dotnet restore (which rejects -c) ever seeing the flag.
• Hooks: RuntimeInfo gains a required Configuration member carrying the domain model, snapshotted into each args file as it is written; ConfigFile is re-documented as the source-file pointer for hooks that edit the file itself; HookArgs.LoadConfig() and BuildvanaConfig.Load() are gone.
• Credentials: the model stores environment-variable names; GetToken() / GetApiKey() extension methods resolve values on demand and throw BuildvanaRuntimeException, translated to BuildFailedException at bv's call sites. A test asserts no resolved secret ever appears in serialized hook args. One deviation from §7: extension methods, not extension properties — a getter that reads process state and throws on absence would betray property semantics, while the structural cannot-be-serialized guarantee is identical.
• Schema: the generator gains default emission, annotated from a domain-model instance, so the schema documents exactly what an omitted setting resolves to (required needed no generator code — System.Text.Json already emits it for C# required members, now locked by a test). nuget.feeds.* requires source + apiKeyEnv and git.identity requires name + email, so a half-written section fails at configuration load and the domain records carry non-nullable members. Blank is never a value: required strings carry minLength/pattern constraints enforced at load (BV1106/BV1107, with file and line), optional strings treat a stated blank as unstated, and a blank CLI option value is rejected like a missing one. schemas/buildvana.schema.json regenerated.
• git.identity is live: bv release resolves its committer identity as configured identity → CI bot identity → repository Git config → fail. The bot now outranks whatever a CI checkout left in the repository's config (recorded as a behavior change in the changelog); a configured identity outranks both.
• Tests: 904 passing; new coverage includes the identity chain end to end, the secrets-never-serialized guarantee, BV1104 on half-written sections, blank-value rejection at each input layer, the feed fallback, and version-cache invalidation when the configuration file changes between builds. dotnet run .claude/tools/inspect.cs --gate exits zero.

Docs (Hooks.md, DirectoryStructure.md, EnvironmentVariables.md), PublicAPI.Unshipped.txt, the changelog (including the amendments §Risks called for), and this repository's own buildvana.jsonc are updated per the acceptance criteria.

Additional changes

• EnvVarHelper.Require's message reworded to the standard adopted for EnvironmentVariables.GetRequired ("Required environment variable X is missing or empty."), so the one remaining non-configuration env read (GITHUB_OUTPUT) fails with the same words as everything else.
• The committer-identity failure message no longer ends with "before running this task" (Cake-era phrasing).
• UnknownServerAdapter.CIBotIdentity now answers null — a local build has an honest answer to that question — instead of failing the build; the class doc, already false for PushUsername/PushPassword, corrected to match.
• CHANGELOG.md: dropped the stale claim that the computed version is reported at notice: level (the task reports it at detail level).
• docs/Hooks.md: the hook-args logging level corrected to trace (the doc said detail; the code says trace).
• docs/ConfigurationFiles.md → docs/SdkConfigurationFiles.md, with a new docs/BuildvanaToolConfiguration.md TODO stub for buildvana.json; every reference re-pointed to whichever of the two it actually meant.
• buildvana.jsonc: four worked-example comments re-synced to the schema descriptions the file's intro promises they mirror.
• Buildvana.slnx: stale file entries fixed (.globalconfig, docs/Diagnostics.md, THIRD-PARTY-NOTICES without extension, NuGet.config casing) and the real documentation and configuration file sets listed.
• CommandParameters.Forwarded: the ReSharper-suppression justification named the wrong reader (the BuildPipeline constructor; it is CommandLineOverridesParser), and the member docs overstated "verbatim" forwarding now that bv consumes the configuration tokens out of the stream; both corrected.

Types of changes

This pull request introduces the following types of changes:

• Bug fix
• New feature
• Test addition / update (no changes to non-test code)
• Refactor (no changes in public API syntax or semantics)
• Performance improvement (no changes in public API syntax or semantics)
• Documentation (docs directory) update
• Dependency addition / update
• Changes to the build scripts
• Changes to CI (workflows, bot / app configurations)
• Changes to repository files (.gitattributes, .gitignore)
• Other

"Other" covers the restructuring itself: Buildvana.Runtime's public surface changes substantially (types moved out, loaders removed, extension methods and a required member added), which the "Refactor" box excludes by its no-public-API-changes parenthetical.

Breaking changes

This pull request introduces breaking changes:

• Yes
• No

Buildvana.Runtime has never shipped stable, and the identity-precedence change is recorded as a behavior change on the preview line; nothing stable breaks.

Checklist

• For all types of changes:
  • I have read the Code of Conduct and agree to be bound by it
  • I have read the Contributor Guide and agree to follow it
• For code changes only:
  • The project builds on my machine, via the provided build script, with zero warnings
  • I have added tests that prove my feature works / my fix is effective
  • I have added / modified XML documentation according to changes in code
  • I have checked that all the links I added or modified in XML documentation point to their intended destination
• For documentation changes (docs directory) only:
  • I have built and tested documentation locally
  • I have checked that all the links I added or modified point to their intended destination

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 91.40127% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.15%. Comparing base (348771a) to head (fed25d7).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Buildvana.Core.JsonSchema/JsonSchemaGenerator.cs	83.33%	3 Missing and 4 partials ⚠️
...na.Core.Configuration/BuildvanaJsonConfigSchema.cs	0.00%	3 Missing ⚠️
...dvana.Core.Configuration/BuildvanaConfigFactory.cs	97.70%	0 Missing and 2 partials ⚠️
...DependencyInjection/ServiceCollectionExtensions.cs	81.81%	2 Missing ⚠️
src/Buildvana.Tool/Services/DotNetService.cs	91.30%	2 Missing ⚠️
...uildvana.Tool/Subcommands/VersionAdvanceCommand.cs	0.00%	2 Missing ⚠️
src/Buildvana.Tool/Utilities/RuntimeAccess.cs	60.00%	2 Missing ⚠️
....Core.Configuration/BuildvanaJsonConfigProvider.cs	94.44%	0 Missing and 1 partial ⚠️
...c/Buildvana.Core.JsonSchema/JsonSchemaValidator.cs	95.65%	0 Missing and 1 partial ⚠️
src/Buildvana.Runtime/GitHubConfigExtensions.cs	0.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   74.64%   75.15%   +0.51%     
==========================================
  Files         165      170       +5     
  Lines        4586     4713     +127     
  Branches      788      802      +14     
==========================================
+ Hits         3423     3542     +119     
- Misses       1014     1019       +5     
- Partials      149      152       +3     

☔ 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]

Review of the wire/domain configuration split. The architecture does what #350 asked for — one factory owning precedence, defaults on the domain records, strictly-BCL Runtime preserved — and the migration of the deleted validation/defaulting logic was audited member-by-member: defaults all moved faithfully, the multiple-config-file guard moved with its coverage, the prerelease→release feed fallback is intact, and the deleted test scenarios are re-covered by the new factory/provider tests. Conventions all check out too (BOMs, extension-block template, no non-BCL references in Runtime, env-var test parallelism rationale).

Two correctness clusters need fixing in this PR, plus smaller items — all anchored inline:

1. Forwarded -c/--configuration (two comments + CHANGELOG). The advertised bv pack -- -c Debug fails at the Restore step: the forwarded -c is folded into Restore.Args and dotnet restore rejects it with MSB1001 (verified empirically). The breakage predates this PR, but this PR documents the syntax as working and bakes it into a test. Separately, parsing the forwarded stream breaks the documented "passed through verbatim, never parsed by bv" contract: a forwarded token that merely looks like -c (e.g. as another option's value) is captured or, in trailing position, aborts the run with a bv error. Both point at one decision: if bv owns -c/--configuration in the forwarded stream, strip them after promotion and amend the CHANGELOG contract bullet; if it doesn't, don't read them.

2. Blank-vs-absent gap in the new load-time guarantee (four comments). required checks presence and "type": "string" accepts "", so the "a half-written section fails at configuration load" guarantee holds only for absent members. Blank values regress diagnostics the old code had: "" feed source/apiKeyEnv fail cryptically at push time (the old actionable "has no source" guards were deleted with their tests), blank git.identity members are persisted into .git/config and crash with an NRE (or fail post-build for whitespace), and a blank configuration dies as a Guard ArgumentException. One rule at one layer fixes the family — generator-emitted minLength: 1 for required strings, or factory-side stated-but-blank rejection — plus a one-line IsNullOrWhiteSpace consistency fix for tokenEnv.

Smaller items: a silent failure mode in the schema generator's defaults matching (name mismatch drops a section's defaults; make the miss throw), the duplicated BuildvanaRuntimeException→BuildFailedException translation block (share a tiny Tool-side helper; the Runtime-accessor routing itself is right), a judgment call on threading config.DotNet.Configuration through five commands including two that ignore it, and line-length violations on modified lines in four places.

Also examined and deliberately not flagged, as they turned out to be intentional design: the settings classes still parsing flags whose resolved values come from the overrides parser (documented attribute-carrier mechanism), DotNetConfig.All (hooks-contract surface per the completeness principle), the factory's per-call new of defaults records (documented single-source-of-defaults idiom), and the schema generator's hand-rolled name resolution (no serializer disagreement is reachable with these models, and JsonTypeInfo wouldn't remove the reflection dependency).

[rdeago]

All 14 threads addressed and resolved — nine commits, one per finding, shared-fix threads sharing one:

• 8b6ddc9 — bv owns -c/--configuration in the forwarded stream and strips it after promotion; the changelog's -c story consolidated into one final-state bullet, the verbatim-pass-through bullet carrying its one exception (3 threads).
• 4641a4d — blank-is-not-a-value family: minLength/pattern on required strings (BV1106/BV1107 at load, with file and line), factory folds blank optional strings to unstated, reader rejects blank option values (4 threads).
• 73a4723 — schema generator: a defaults-side miss now throws; $schema opted out via [JsonSchemaNoDefault].
• bb189cf — shared RuntimeAccess.Translate for the Runtime-to-BuildFailedException translation.
• d828773 — BuildPipeline resolves its own default configuration; the five commands drop their config injection (this also rewrote every line the BuildPipeline line-length thread flagged).
• 2c97e54, 00af27d, fed25d7 — line-length wraps in DotNetService, JsonSchemaGenerator, and the schema-test samples (a fourth SuppressMessage twin in GeneratorSample.cs included), each with its whole-file sweep.

One small out-of-scope fix on the way, recorded in the PR description's Additional changes: a1c6190 corrects CommandParameters.Forwarded's stale ReSharper justification (it named the BuildPipeline constructor; the reader is CommandLineOverridesParser) and its now-overstated "verbatim" wording.

Gate is green (bv pack + inspectcode, zero findings), 904 tests passing, schema check clean. The PR description is updated to describe the branch as it now stands.

@rdeago please take another look.

[rdeago]

Re-reviewed the nine response commits against every thread. All 14 findings are correctly resolved, and the new diff introduces no new problems. Verified in particular:

• 8b6ddc9 — promotion and stripping share one CliOptionReader pass (Remaining preserves order), the read runs unconditionally so the stream is stripped even when bv's own options win, dotnet restore never sees -c, and the CHANGELOG now tells one coherent story: the pass-through bullet carries its single exception, and the consolidated -c bullet states the final behavior with both ownership costs spelled out honestly.
• 4641a4d — the non-blank constraint lives in exactly one layer per input kind: minLength: 1 + pattern: "\S" on required strings only (pinned by tests on all three sides: required string constrained, optional string and required non-string untouched), BV1106/BV1107 wired end to end (validator, provider mapping, docs table, regenerated schema), one NormalizeBlankToNull for every optional string in the factory, and the option reader rejecting blank values in both token forms. The code-point minLength counting and the pattern timeout guard are nice touches.
• 73a4723 — the schema-side skip ([JsonSchemaNoDefault], [JsonAllowedKeys] fall-through) stays ahead of the new defaults-side throw, so the $schema opt-out and dictionary keys still pass while a genuine miss now fails with an actionable message.
• bb189cf / d828773 / a1c6190 — as described in the replies; the pipeline's ??= default plus the factory's blank normalization means the surviving Guard is now genuinely unreachable from configuration input.
• Line lengths — measured every touched file: no line over 140 and no declaration over 120 anywhere in the nine commits. The only over-limit lines left in touched files (JsonSchemaValidator.cs:122, CommandParameters.cs:13–14, JsonSchemaValidatorTests.cs:162) all pre-date the branch on main, so nothing this PR added or modified is out of compliance; a wrap-only commit for those four would complete the whole-file sweep the style guide suggests, but that's optional cleanup, not a gate.

Codecov (patch and project) and CodeQL are green on fed25d7. This PR is ready to merge.