Fix the release workflow's hook failure, and the home-directory bug behind it

[rdeago]

Checklist of related issues / discussions

No issue: this fixes a failing release workflow, and went straight to a PR.

• Closes #
• Partially closes #
• Related discussion(s): #

Proposed changes

The release workflow failed: the release/post-release hook died with
FileNotFoundException: '/home/runner/work/Buildvana/Buildvana/buildvana.jsonc'
after the configuration file moved into .buildvana/. The hook read the file by
name, relative to its working directory.

That literal was the presenting symptom. The cause underneath it is that
a configuration file inside .buildvana/ makes .buildvana/ itself a home
directory: the marker rule counted a configuration file "directly in the
directory", and .buildvana/buildvana.jsonc satisfies that clause for
.buildvana itself. Hooks are projects living in there, so every hook build
walked up, stopped at .buildvana/, and took it for the repository. Measured
before the fix:

HomeDirectory:    D:\...\Buildvana\.buildvana\
StyleCopJsonPath: (empty)

and after:

HomeDirectory:    D:\...\Buildvana\
StyleCopJsonPath: D:\...\Buildvana\stylecop.json

That second line is the SA1633 warning in the same release log: the repository's
stylecop.json was found and then discarded as "outside the repository", so hooks
compiled under StyleCop's defaults instead of ours. Every other
HomeDirectory-relative path in a hook build was wrong by one level too. No
analyzer rule was touched to fix it.

What changed

• .buildvana/ is no longer a configuration-file location. The file lives in
  the home directory; .buildvana/ keeps the hooks. The marker rule collapses to
  one sentence with no exceptions — every marker sits in the directory it marks —
  in the loader, in HomeDirectoryDiscovery, and in Sdk.props, which now probe
  two candidates instead of four. Removing the location makes the collision
  unrepresentable rather than guarding against it.
• bv resolves the configuration file once per run. BuildvanaConfigLoader
  becomes BuildvanaConfigProvider, an instance holding both the path and the
  parsed configuration, each resolved on first read, as HomeDirectoryProvider
  does for the home directory. The probe is private, so nothing can ask a second
  time and get a different answer. Previously three places found the file
  independently and one of them threw the path away.
• Hooks are told which file it is. RuntimeInfo.ConfigFile carries the path,
  and HookArgs.LoadConfig() reads that same file through the new
  BuildvanaConfig.LoadFile, so a hook after settings does not search either —
  which file comes from the args, what it says is read from disk at the moment of
  the call.
• The post-release hook stops hardcoding the file name.

Known limitation, deliberate: the hook uses BuildvanaConfig.FindFile() rather
than RuntimeInfo.ConfigFile, with a comment saying what to replace and when. The
SDK pins Buildvana.Runtime to its own version, so a hook compiles against the last
published release; using the new member now would fail the hook's compilation during
the very release that publishes it, aborting that release. It becomes a one-line
change in the release after next.

Additional changes

• The configuration model owns its defaults. dotnet.configuration → Release
  and versioning.assemblyVersionPrecision → Major used to be resolved by
  whoever read them, so a hook asking the same question of the same file got null
  where bv got a value — one file describing two different builds. Each default is
  now a constant next to its setting, with an Effective… accessor over a nullable
  receiver, so an absent section and an absent setting resolve identically and no
  consumer writes its own fallback.
• Hooks.md's contract-evolution rule was wrong and is rewritten. It applied
  binary-compatibility reasoning to a contract with no version boundary: a hook is
  compiled from source at every run against the Runtime version the SDK pins, which
  bv refuses to mismatch, and the args file is rewritten immediately before the
  run. What has to stay stable is the source surface, which is why ConfigFile
  could be added as a required member.

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

Breaking changes

This pull request introduces breaking changes:

• Yes
• No

The .buildvana/ configuration location shipped in exactly one preview
(2.1.208-preview) and never in a stable release — its own bullet was still under
## Unreleased changes — so its removal edits that bullet rather than adding a
breaking-change entry. Migration for a preview adopter is git mv.

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

[rdeago]

@claude please review this PR.

[claude]

Claude finished @rdeago's task in 7m 30s —— View job

---

Review

Read the full diff against origin/main, plus the surrounding files (BuildvanaJsonContext, IHomeDirectoryProvider/FixedHomeDirectoryProvider, Sdk.props, the hook, PublicAPI.Unshipped.txt). I could not run a build or the test suite here — dotnet build isn't permitted in this environment — so everything below is static analysis.

The diagnosis is right and the fix is the right shape: collapsing the marker rule to every marker sits in the directory it marks makes the collision unrepresentable instead of guarding against it, and it's mirrored consistently in all four places (loader, HomeDirectoryDiscovery, Sdk.props, docs). I grepped for leftovers of the old location across source, tests, docs and schemas — there are none. BuildvanaConfigProvider's two-Lazy design holds up: the default Lazy<T> mode caches exceptions as well as results, so the "result and exception alike" claim in the remarks is accurate, and _lazyConfig reading through _lazyPath really does give one probe per run.

Findings below, correctness first.

---

1. RuntimeInfo.ConfigFile's remarks send settings-reading hooks back to searching

src/Buildvana.Runtime/RuntimeInfo.cs:49-51:

A hook that reads settings has no use for this: BuildvanaConfig.Load finds the file on its own.

But HookArgs.LoadConfig() — added in the same commit — is BuildvanaConfig.LoadFile(RuntimeInfo.ConfigFile), and both its own remarks and docs/Hooks.md:98 now tell a settings-reading hook to call it precisely so it does not search. So the member that LoadConfig is built on documents itself as irrelevant to LoadConfig's only use case, and points at the searching loader instead.

This is the copy a hook author actually reads, in IntelliSense, at the moment they're deciding. Suggest the first <para> say that a hook reading settings gets there through HookArgs.LoadConfig, and keep the rest (works-on-the-file-itself) as is. Fix this →

2. None of the renamed tests would fail on the buggy code

The three tests presented as the regression guards all pass with the .buildvana/ probes restored, because none of them puts a configuration file inside .buildvana/ — which is the state that produced the bug:

• tests/Buildvana.Core.HomeDirectory.Tests/HomeDirectoryDiscoveryTests.cs:30 TryDiscover_StartingUnderBuildvanaDirectory_FindsHomeDirectory — config is at the root, so the old subdirectory clause never matches on the way up.
• tests/Buildvana.Core.HomeDirectory.Tests/HomeDirectoryDiscoveryTests.cs:51 TryDiscover_BuildvanaDirectory_IsNotMarker — writes .buildvana/hooks/release/post-release.cs; the old code was never fooled by a bare .buildvana, only by a config file in it.
• tests/Buildvana.Sdk.Tests/SdkPropsTests.cs:26 Evaluate_ProjectUnderBuildvanaDirectory_HomeIsRepositoryRoot — same shape, so _HomeDir_BySubdirConfigJsonc would evaluate empty and the old Sdk.props would answer identically.

The tests that did cover .buildvana/buildvana.json[c] were deleted rather than inverted, so the fix currently has no test that fails without it. Sdk.props is where that matters most: it is hand-maintained MSBuild that has to mirror the C# by discipline alone, and a restored probe there is silent.

Three tests would lock it in, each a near-copy of one that was deleted:

• discovery: .git/HEAD at root plus root/.buildvana/buildvana.jsonc, start at root/.buildvana/hooks/release → home is root (old code answers root/.buildvana);
• SdkPropsTests: same fixture, HomeDirectory is the repo root;
• loader: .buildvana/buildvana.jsonc present and nothing at the root → Path is null and Config is empty.

That last one also pins the migration story in the PR description: a preview adopter who copies instead of git mv-ing gets the root file, silently, with no BVSDK1005. Fix this →

3. ConfigFile = null never round-trips through JSON

ConfigFile is required string?, and System.Text.Json requires a required member to be present in the document. It works today only because BuildvanaJsonContext sets no DefaultIgnoreCondition, so "configFile": null is written — I checked. The moment anyone adds WhenWritingNull to that context, or a [JsonIgnore(Condition = WhenWritingNull)] to this member, every hook run in a repository without a configuration file fails to load its args, and the failure is a release-time one.

Load_RoundTripsWhatBvWrites (tests/Buildvana.Runtime.Tests/PostReleaseHookArgsTests.cs:26) covers only the non-null case, since SampleArgs now always sets a path. Parameterizing it over null/non-null costs a line and pins the invariant — which matters more now that the rewritten "Contract evolution" section explicitly licenses required additions. Fix this →

4. The hook's interim BuildvanaConfig.FindFile() rests on the same assumption it's avoiding

.buildvana/hooks/release/post-release.cs:27. The comment above it is exactly right about RuntimeInfo.ConfigFile — and the identical reasoning applies to FindFile() itself, which the hook did not call before this PR. It has to be public in Buildvana.Runtime 2.1.208-preview, the version global.json pins, or the hook fails to compile mid-release: the failure this whole PR exists to prevent. It very likely is (bv update's $schema rewrite is built on it, and the four-candidate rule shipped in that preview), but the repository is checked out shallow here and I couldn't reach the package to confirm — worth one look before merge, since the cost of being wrong is another aborted release.

Separately, FindFile() with no argument resolves against the current directory. The hook's working directory is the home directory, so it's correct — but BuildvanaConfig.FindFile(hookArgs.RuntimeInfo.HomeDirectory) costs nothing, depends on the args rather than on process state, and makes the future edit the one-line replacement the comment promises.

5. Nits

• src/Buildvana.Runtime/PublicAPI.Unshipped.txt:434,435,444 — the new entries land out of sort order (the two consts between JsonFileName and JsoncFileName; DotNetConfigExtensions.get_EffectiveConfiguration between BuildvanaConfig.LoadFile and BuildvanaConfig.operator !=). The rest of the file is sorted, and this is a file that gets read as a diff at every API review.
• src/Buildvana.Core.Versioning/VersioningSettings.cs:47 — the XML doc still spells the default out as <see cref="AssemblyVersionPrecision.Major"/>. Now that the default is a constant, <see cref="VersioningConfig.DefaultAssemblyVersionPrecision"/> removes the last copy that can drift, which is the point of the commit.
• docs/Hooks.md:109 — the sentence is about the C# API (the code block below uses hookArgs.RuntimeInfo.ConfigFile) but names the member runtimeInfo.configFile, in the JSON casing. The table row above says RuntimeInfo.ConfigFile.

6. One observation, no change requested

config.DotNet.EffectiveConfiguration (DotNetSettings.cs:28) and config.Versioning.EffectiveAssemblyVersionPrecision (VersioningSettings.cs:30) read like null dereferences of nullable properties — and in VersioningSettings the line directly above uses config.Versioning?.PrereleaseTag on the same receiver, so the pair looks inconsistent at the call site. Only the extension declaration reveals that the receiver is nullable by design. The alternative (?. plus ??) puts the fallback back at the call site, which is precisely what the commit removes, so I'd keep it as is — just noting that it will make a reader stop, and that "nullable receiver" is a convention this repo now has.

---

Not done: no build or test run (dotnet build isn't permitted here), so I have not verified that the solution compiles warning-free or that the tests pass; and I could not confirm the published surface of Buildvana.Runtime 2.1.208-preview for finding 4. No code changed — this is review feedback only.
• branch fix/config-file-location

[codecov]

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.63%. Comparing base (3278931) to head (ca00901).

Files with missing lines	Patch %	Lines
...e.HomeDirectory/DiscoveredHomeDirectoryProvider.cs	0.00%	1 Missing ⚠️
...DependencyInjection/ServiceCollectionExtensions.cs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   74.57%   74.63%   +0.05%     
==========================================
  Files         162      164       +2     
  Lines        4582     4596      +14     
  Branches      788      788              
==========================================
+ Hits         3417     3430      +13     
- Misses       1014     1015       +1     
  Partials      151      151              

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

Addressed, one commit per finding. Gate (bv pack + ReSharper at WARNING and above) is green: 0 errors, 0 warnings, 0 info.

1. ConfigFile remarks — done (d7c9ace)

Right, and the contradiction was load-bearing in the worst place. The first <para> now says a settings-reading hook gets there through HookArgs.LoadConfig; the rest is unchanged.

2. Regression tests — finding accepted, fixtures corrected (852fcae)

The finding is right: nothing failed if the location came back. Two of the three proposed fixtures don't demonstrate it, though.

Under the rule as it now stands a configuration file marks the directory it sits in, and .buildvana is not special — so root/.buildvana/buildvana.jsonc marks root/.buildvana, before and after the fix alike. Discovery starting at root/.buildvana/hooks/release therefore answers root/.buildvana in both, and in Sdk.props _HomeDir_ByConfigJsonc resolves to root/.buildvana, deeper than .git, so it wins. Those two tests would pass on the buggy code and fail on the fixed code.

That outcome is deliberate. A repository that keeps its configuration in .buildvana/ after this change is broken, and the fix for it is the git mv this branch opens with, not a guard. It is why the migration diagnostic and the name guard were dropped from the original plan.

What the removed clause actually did was make a directory a home directory because a .buildvana subdirectory of it held a configuration file. Hence the fixture:

fixture	old rule	current rule
.git/HEAD at root, config at root/nested/.buildvana/buildvana.jsonc, start at root/nested	root/nested	root

One test per implementation on that fixture (HomeDirectoryDiscoveryTests, SdkPropsTests), plus FindFile(dir) returning null for a file left in .buildvana/, which is the probe itself (BuildvanaConfigLoadTests). Hermetic: the .git/HEAD marker is inside the fixture, and any stray marker above the temp directory has a shorter path, so nearest-wins keeps it out.

Each was checked against the old behavior by restoring the probes in all three implementations — HomeDirectoryDiscovery, BuildvanaConfig.FindFile, and Sdk.props: one failure per test project, each of them the new test.

Not added: the fourth test, in BuildvanaConfigProviderTests. The provider has no probe of its own — it delegates to BuildvanaConfig.FindFile — so a restored probe is caught by the Buildvana.Runtime test, and a copy there would pin nothing further.

3. Null round-trip — done (b4fd520)

Load_RoundTripsWhatBvWrites now runs both ways, off an optional parameter on SampleArgs that also spares LoadConfig_WithoutConfigFile_ReturnsEmptyConfig its two-level with expression.

Both regressions were checked:

• DefaultIgnoreCondition = WhenWritingNull on the context — 3 failures, one of them the new null case. (The other two are ReleaseInfo.PreviousVersion, required and nullable for the same reason.)
• [JsonIgnore(Condition = WhenWritingNull)] on the member — 1 failure, the null case. That is the one only this test catches.

4. Hook's interim FindFile() — verified, second half done (7faa374)

FindFile is public in 2.1.208-preview: git show 2.1.208-preview:src/Buildvana.Runtime/BuildvanaConfig-Load.cs has public static string? FindFile(string? homeDirectory = null), and the tag's PublicAPI.Unshipped.txt carries the entry. dotnet build .buildvana/hooks/release/post-release.cs compiles clean against the pin. No aborted release waiting there.

The argument point is taken: it now passes hookArgs.RuntimeInfo.HomeDirectory, so the hook depends on what bv said rather than on process state, and the deferred edit is one expression.

5. Nits — all three done

• baa2528 — the four entries are sorted; the whole file now matches its sorted order line for line, so the next diff has nothing incidental in it.
• 19e00d1 — VersioningSettings cites VersioningConfig.DefaultAssemblyVersionPrecision. And DotNetSettings.cs:33, which spelled out "Release" — same drift, same commit, since that is exactly what the defaults commit set out to remove.
• ca00901 — RuntimeInfo.ConfigFile in the C# paragraph.

6. Observation

Agreed on both counts, and no change made: the alternative puts the fallback back at the call sites, which is the thing that commit removes. Noted as a convention.

---

Nothing here changed the PR description: all of it fixes code this PR introduced, which is not what the "Additional changes" section is for.

@claude please review again.

[claude]

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

---

Re-review

Read the seven new commits (d7c9ace..ca00901) against origin/main, plus the pre-fix implementations of all three probes (HomeDirectoryDiscovery, BuildvanaConfig.FindFile, Sdk.props) to check each new test actually inverts. No build or test run — dotnet isn't permitted in this environment — so this is static analysis; the green gate is your run.

All five findings are addressed, and two of them came back better than what I asked for.

---

2 — you're right and my fixtures were wrong

I traced it. Under the collapsed rule, root/.buildvana/buildvana.jsonc marks root/.buildvana in both implementations, old and new, because the file sits directly in that directory: DirectoryContainsMarker hits its first clause, and _HomeDir_ByConfigJsonc resolves deeper than _HomeDir_ByGitHead. Discovery from root/.buildvana/hooks/release therefore answers root/.buildvana either way, and the two tests I proposed would have failed on the fixed code. Only the loader one was a real discriminator.

The delivered fixture is the right one, and I verified each of the three against the pre-fix code by hand:

test	pre-fix answer	asserted
HomeDirectoryDiscoveryTests:72	_BySubdirConfig* clause matches at root/nested → root/nested/	root/
SdkPropsTests:39	_HomeDir_BySubdirConfigJsonc = repo/nested, longer than _ByGitHead → wins	repo/
BuildvanaConfigLoadTests:161	old FindFile probed .buildvana/buildvana.jsonc → returns it	null

Each fails on the old code, each for the reason the removed clause existed. SdkPropsTests is the one that matters most and it now covers the hand-maintained MSBuild.

Hermeticity holds in all three: the fixture plants .git/HEAD inside the temp tree, and nearest-wins means the loop returns at root before any ancestor of the temp directory is examined.

The rationale for not adding the fourth test in BuildvanaConfigProviderTests is sound — BuildvanaConfigProvider.FindFile is a three-line wrapper around the Runtime probe, so the Runtime test is where a reintroduced candidate gets caught, and a copy would pin the delegation rather than the rule.

And the .buildvana/-holdout repository being broken-by-design rather than guarded is your call, already made; I'm not re-opening it.

1, 3, 4, 5 — verified

• 1 (d7c9ace): the first <para> now routes a settings-reading hook through HookArgs.LoadConfig, and BuildvanaConfig-Load.cs:74 says the same thing from the other end. The contradiction is gone in both directions.
• 3 (b4fd520): Load_RoundTripsWhatBvWrites is parameterized, and loaded.RuntimeInfo == written.RuntimeInfo carries the null through the record comparison, so the assertion is real rather than incidental. Folding the withConfigFile parameter into SampleArgs and collapsing LoadConfig_WithoutConfigFile_ReturnsEmptyConfig's two-level with is a genuine improvement over what I proposed. Your two regression checks cover both ways the invariant can be lost.
• 4 (7faa374): FindFile(hookArgs.RuntimeInfo.HomeDirectory) — the hook now depends on what bv reported rather than on process state, and the comment above it names the one expression to swap. I still can't reach the tag from this checkout (fatal: invalid object name '2.1.208-preview'), but dotnet build on the file-based app against the pin is stronger evidence than the source read I asked for.
• 5: LC_ALL=C sort -c on PublicAPI.Unshipped.txt passes for the whole file (baa2528); VersioningSettings.cs:47 and DotNetSettings.cs:28 both cite their constant (19e00d1) — catching DotNetSettings was the right generalization, since it was the same drift the commit set out to remove; docs/Hooks.md:109 matches the table row above it (ca00901). Grepped for leftovers: no "Release" or Major literal remains outside the two constants, and no .buildvana/buildvana* reference survives outside the three new tests.

One observation, no change requested

docs/Hooks.md:140 rests the additive-member licence on "writer and reader are therefore the same version by construction", which holds because bv refuses a mismatched SDK pin. --skip-sdk-check is the documented way out of that refusal, and RuntimeInfo.ConfigFile is the first addition that makes the consequence reachable: under the flag, with an SDK pin older than the running bv, the hook compiles against a Runtime whose RuntimeInfo has no ConfigFile, and UnmappedMemberHandling.Disallow turns the extra member into a failed args load — mid-release, which is the failure mode this PR is about. The paragraph already handles the analogous stale-args case in its parenthetical; a clause naming the flag as the one way to break the lockstep would close the loop. Not a correctness issue in the code, and the flag exists precisely for deliberate mismatches, so entirely your call whether it's worth a sentence.

---

Nothing else. The delta is clean, the three new tests do the job the deleted ones stopped doing, and I have no further findings.

Not done: no build or test run here, so the gate result is yours, not mine; and the 2.1.208-preview surface remains confirmed by your compile rather than independently by me.
• branch fix/config-file-location