Ship typed configuration and hook contexts in a new Buildvana.Runtime package

[rdeago]

Follow-up to #312/#318, replacing the SDK-injected hook loader sources before they ever ship.

• New packaged library Buildvana.Runtime: the typed model of buildvana.json/buildvana.jsonc plus the run-time information bv shares with repository-owned hooks. Serialization is source-generated, so the same types work in file-based apps, where reflection-based JSON serialization is disabled. Hooks reference it with an unversioned #:package Buildvana.Runtime directive; Buildvana SDK pins the version to its own for every file-based app built in the repository.
• Typed, per-hook contexts: PostReleaseHookContext (shared verbatim between bv and hooks) structured into Paths and Release sections; context files move to .buildvana-temp/hook-contexts/<command>/<moment>.json so hooks can't clobber each other's replay context.
• Buildvana.Core.Configuration keeps schema validation and diagnostics on top of the packaged model; nuget.feeds is now a typed record (same JSON shape, better schema descriptions).
• Docs (ReleaseHooks.md), CHANGELOG, and architecture rules updated.

Note: .github/workflows/claude.yml was also updated in this branch (model/effort settings for the Claude workflow).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 63.51351% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.97%. Comparing base (d9deaa4) to head (83448fe).

Files with missing lines	Patch %	Lines
src/Buildvana.Tool/Subcommands/ReleaseCommand.cs	0.00%	14 Missing ⚠️
src/Buildvana.Runtime/BuildvanaConfig-Load.cs	83.33%	2 Missing and 2 partials ⚠️
src/Buildvana.Runtime/PostReleaseHookContext.cs	73.33%	2 Missing and 2 partials ⚠️
src/Buildvana.Runtime/BuildvanaRuntimeException.cs	66.66%	2 Missing ⚠️
...ldvana.Core.Configuration/BuildvanaConfigLoader.cs	83.33%	0 Missing and 1 partial ⚠️
...ldvana.Core.Configuration/BuildvanaConfigSchema.cs	0.00%	1 Missing ⚠️
...c/Buildvana.Core.JsonSchema/JsonSchemaGenerator.cs	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   41.64%   41.97%   +0.33%     
==========================================
  Files         114      118       +4     
  Lines        3523     3564      +41     
  Branches      625      634       +9     
==========================================
+ Hits         1467     1496      +29     
- Misses       1990     1997       +7     
- Partials       66       71       +5     

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

@claude please review this PR.

[claude]

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

---

Review of #319

• Gather context (PR description, changed files, comments)
• Fetch base branch and diff (origin/main...HEAD)
• Review new Buildvana.Runtime package
• Review typed hook context changes
• Review Buildvana.Core.Configuration changes
• Review docs/CHANGELOG/architecture rule updates
• Post final review

Solid PR overall — the split between the lean, packaged Buildvana.Runtime model and the schema-validating Buildvana.Core.Configuration loader is clean, the NuGetConfig.Feeds dictionary → typed NuGetFeedsConfig migration is done consistently everywhere it's consumed (DotNetSettings.ResolvePushTarget), and the PublicAPI.Unshipped.txt / GlobalSuppressions.cs RS0041 entries line up 1:1 with the serializable closure. Tests for the new package (BuildvanaConfigLoadTests, PostReleaseHookContextTests) and the updated HookRunnerTests look thorough.

One correctness concern:

src/Buildvana.Sdk/Modules/Hooks/Module.targets:16 — the comment above the item group says an explicitly versioned #:package directive "is left alone", but the non-CPM line:

<PackageReference Update="Buildvana.Runtime" Version="$(BuildvanaSdkVersion)" Condition="'$(ManagePackageVersionsCentrally)' != 'true'" />

uses MSBuild Update, which unconditionally overwrites the Version metadata of any existing PackageReference Include="Buildvana.Runtime" item — including one materialized from an explicitly-versioned directive (#:package Buildvana.Runtime@X) outside of CPM. That silently clobbers the user's pin with $(BuildvanaSdkVersion), contradicting the stated intent. Compare with the established idiom for this exact "don't override an explicit reference" problem in src/Buildvana.Sdk/Sdk/Sdk.targets:80 (<BV_PackageReference Remove="@(PackageReference)" />), which removes the auto-injected item instead of force-updating the real one. Worth double-checking whether the CPM branch (PackageVersion Include=...) has the same gap, or whether it's safe because VersionOverride on the reference wins regardless.

Fix this →

Minor notes (non-blocking):

• Codecov flags src/Buildvana.Tool/Subcommands/ReleaseCommand.cs at 0% patch coverage for the 14 new lines constructing PostReleaseHookContext.Paths/.Release. Given ReleaseCommand doesn't appear to have unit tests exercising the hook-context assembly today, this looks like a pre-existing gap rather than a regression, but flagging in case it's easy to close given how central this record now is to the hook contract.
• docs/ReleaseHooks.md and the hook contract docs read well; the "additive-only, optional-with-default" contract combined with UnmappedMemberHandling = Disallow only holds because bv/SDK/Buildvana.Runtime are released in lockstep from the same repo — worth keeping in mind if that assumption ever needs to change (e.g. if bv and the SDK package ever ship independently).

[rdeago]

Good catch — confirmed and fixed in 83448fe, and the CPM branch turned out to have its own variant of the problem.

Non-CPM: a versioned #:package Buildvana.Runtime@X directive materializes as <PackageReference Include="Buildvana.Runtime" Version="X" /> in the project body (verified via dotnet project convert), so the unconditional Update did clobber it. The %(Version) condition one-liner isn't legal at evaluation time (MSB4191), and the Remove idiom from Sdk.targets doesn't apply here (we're stamping metadata on the directive's item, not injecting a competing one), so the fix uses an item-function condition instead: the pin now only applies when the existing reference has no Version metadata.

CPM: a versioned directive under CPM fails upstream with NU1008 — the SDK generates Version, not VersionOverride, so the old comment's "use VersionOverride instead" advice was impossible to follow. The only override channel under CPM is the user's own PackageVersion item, which the previous unconditional PackageVersion Include would have duplicated (NU1506). The pin is now likewise guarded: a user-supplied PackageVersion wins.

All four scenarios (non-CPM ±explicit version, CPM ±user pin) verified against the edited targets file with a test harness.

On the two non-blocking notes, no changes in this PR:

• ReleaseCommand patch coverage: this is a pre-existing gap, not a regression — ReleaseCommand has no unit tests exercising hook-context assembly today, and building that harness is out of scope for this PR. It's a reasonable follow-up issue given how central PostReleaseHookContext now is to the hook contract.
• Lockstep-release assumption: deliberate and documented. bv, the SDK, and Buildvana.Runtime ship from this repo in lockstep by design; if that ever changes, the additive-only contract (and UnmappedMemberHandling.Disallow) would indeed need revisiting, but there are no plans in that direction.

@claude please review the PR again.

[claude]

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

---

Re-review of #319 (after 83448fe)

• Gather context (fetch latest branch state, prior review, fix commit)
• Review the Module.targets fix for the non-CPM clobber issue
• Review the new CPM-branch guard
• Verify against the "four scenarios" claim (non-CPM ±explicit version, CPM ±user pin)
• Check for regressions elsewhere in the diff since last review
• Post final review

The correctness issue from the last review is resolved. Since that review only one new commit landed, 83448fe, touching a single file (src/Buildvana.Sdk/Modules/Hooks/Module.targets), so this pass is scoped to that fix.

src/Buildvana.Sdk/Modules/Hooks/Module.targets:16-21

<PackageVersion Include="Buildvana.Runtime" Version="$(BuildvanaSdkVersion)"
                Condition="'$(ManagePackageVersionsCentrally)' == 'true' and '@(PackageVersion->WithMetadataValue('Identity','Buildvana.Runtime'))' == ''" />
<PackageReference Update="Buildvana.Runtime" Version="$(BuildvanaSdkVersion)"
                  Condition="'$(ManagePackageVersionsCentrally)' != 'true' and '@(PackageReference->WithMetadataValue('Identity','Buildvana.Runtime')->Metadata('Version'))' == ''" />

Traced both branches by hand:

• Non-CPM: WithMetadataValue('Identity','Buildvana.Runtime')->Metadata('Version') filters to the item materialized from the #:package directive and reads its Version metadata. An explicit #:package Buildvana.Runtime@X directive produces that metadata non-empty → condition false → Update is skipped, so the pin no longer clobbers it. An unversioned directive leaves Version empty → condition true → the pin stamps $(BuildvanaSdkVersion) as before. This directly fixes the clobber flagged previously.
• CPM: switched from unconditional Include to a guarded one — only adds the SDK's PackageVersion item when no PackageVersion with Identity == 'Buildvana.Runtime' already exists (i.e., nothing in Directory.Packages.props). This avoids the NU1506 duplicate-PackageVersion failure that the previous unconditional Include would have caused if a user pinned their own version — a real gap the original review didn't catch, and good that it was found and closed in the same pass.
• The nested-single-quote pattern ('@(Items->Function('a','b'))' == '') is unusual to read but is valid, established MSBuild syntax (same idiom appears in NuGet's own targets) — not a concern.
• The updated comment block correctly documents why VersionOverride isn't viable here (file-based app directives can't express it, and a versioned directive under CPM fails upstream with NU1008 regardless), replacing the previous comment's now-incorrect advice.

CI on 83448fe (build/test/pack, CodeQL) is green, which is consistent with this being a self-contained, syntactically valid targets change.

No new issues found. The two non-blocking notes from the previous review (missing ReleaseCommand hook-context coverage, the lockstep-release assumption) were addressed by explanation rather than code changes, which is a reasonable call for both — neither is a correctness problem, and the reasoning given (pre-existing gap; deliberate, documented architectural assumption) holds up.
· branch