Delegate non-local bv runs to the repository's pinned version; add bv update

[rdeago]

Proposed changes

Why. The SDK version check was a guard, not a remedy: a global or dnx-launched bv in a pinned repository got an error and instructions, when what the user wants is for the right version to just run. And the harm was not limited to usesSdk commands: a bv of any other version deserializes buildvana.jsonc with a different Buildvana.Runtime model — silent drift or spurious validation errors on commands the check never gated.

What. Like the Angular CLI's global ng, bv now always defers to the repository:

• Delegation. When the tool manifest pins bv, a non-local bv delegates the entire invocation to the pinned version — dotnet tool restore when needed, then dotnet tool run bv with the original arguments verbatim, inherited standard streams, and forwarded exit code, printing an info line on standard error when the versions differ. The delegating side neither parses arguments nor reads configuration; judging them is the pinned version's job. --skip-delegation runs the exact binary invoked, and the BV_DELEGATED environment variable set on the child makes delegation loops impossible.
• bv update. One operation re-pins the repository's whole Buildvana surface to the running bv's version: the tool manifest entry (via dotnet tool update / install --create-manifest-if-needed, which also downloads the version), the Buildvana.Sdk pin in global.json, and the configuration file's $schema reference; the configuration is then loaded with the new model and problems surface as warnings. update is delegation-exempt ("bring this repository to me") and refuses downgrades without --force.
• sync-sdk is removed, fully subsumed by the two features above; the SDK version check stays as a backstop for repository-internal pin disagreements and now points at bv update.
• RuntimeInfo (formerly BuildvanaPaths) now carries the running bv's version and the delegating bv's version (null when not delegated) alongside the well-known paths, shared by all hook contexts.

How. The delegation decision is primarily a version comparison: a manifest-run bv always matches the pin by construction, so a mismatched bv cannot be the manifest's, and the delegated child never re-delegates. Install-layout detection (AppContext.BaseDirectory matched against the tool-store and package-cache layouts, degrading to unknown ⇒ non-local) decides only the equal-version case, so the manifest's install always runs no matter how bv was launched. Delegation needed true stdio inheritance, which CliWrap cannot do, so IProcessRunner gains RunWithInheritedStdioAsync implemented with raw System.Diagnostics.Process (parent survives Ctrl-C; the child owns shutdown). A new docs/EnvironmentVariables.md inventories the variables bv reads and sets.

Verified end to end on this repository: a dev-built bv delegates --version to the pinned 2.1.70-preview, and --skip-delegation runs it in place.

Checklist of related issues / discussions

• Fixes bv should detect a non-local launch and delegate to the repository's pinned version #325
• Partially fixes #
• Related discussion(s): #

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

Relative to the 2.1 preview line only (no stable release is affected): plain bv in a pinned repository no longer runs the invoked binary; sync-sdk is gone; BuildvanaPaths is reshaped into RuntimeInfo and the hook context's paths member becomes runtimeInfo.

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

[rdeago]

@claude please review this PR.

[claude]

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

---

Review — PR #332: delegate non-local bv runs to the pinned version

• Gather context (diff, PR body, repo rules)
• Review delegation core (DelegationService, InstallLayoutDetector, Program)
• Review bv update (SelfVersionService, ToolManifest, UpdateCommand)
• Review IProcessRunner.RunWithInheritedStdioAsync / ProcessRunner
• Review RuntimeInfo reshape + public API / docs / changelog
• Review tests
• Post review

Ric — the design holds together and the write-up is unusually good; the XML docs on DelegationService/InstallLayout explain the why well enough that I could review the decision table without guessing. RuntimeInfo reshape is consistent across PublicAPI.Unshipped.txt, the JSON context suppressions, the hook docs and the tests; no sync-sdk/BuildvanaPaths residue anywhere in the repo (I grepped); the .store-before-package ordering in InstallLayoutDetector is right and tested; and the balanced +=/-= of the cached static Ctrl-C handler in ProcessRunner is correct even under concurrency (Delegate.Remove removes one occurrence, adds and removes pair up).

Caveat: I could not run the build or tests here (dotnet build needed approval I don't have), so everything below is from reading.

---

Correctness

1. bv update picks the wrong CLI verb when the manifest has an unusable bv entry — src/Buildvana.Tool/Services/SelfVersionService.cs:224

ToolManifest.ReadBvPin returns null for three different states: no manifest, no bv entry, and entry present but version missing/unparseable. PinToolManifestAsync collapses them with var hasEntry = currentPin is not null; and therefore runs dotnet tool install bv --version X --create-manifest-if-needed when a bv entry exists with, say, "version": "2.1" or a hand-edited typo. The CLI fails that with Tool 'bv' is already installed — so bv update fails exactly in the half-broken-repository case it exists to repair, and the user gets a confusing CLI error instead of a Buildvana one.

The same conflation is benign in DelegationService (an unusable pin degrades to run-in-place, which is what you want), so the fix belongs in the reader: have ReadBvPin report the three states distinctly (e.g. return an (bool HasEntry, NuGetVersion? Version) or a small enum-bearing record), and let PinToolManifestAsync key off HasEntry rather than off the version.

Fix this →

---

Design — two things I'd like you to push back on or agree with

2. Unconditional dotnet tool restore on every delegated invocation — src/Buildvana.Tool/Infrastructure/Delegation/DelegationService.cs:107,119

The comment argues the probe would be less contractual than the layouts, and that a satisfied restore is "quick and offline". Two consequences I don't think are priced in:

• dotnet tool restore restores the whole manifest, not just bv. In a repository that also pins a tool from a private feed (this one pins ngbv), a delegated bv build now fails whenever that unrelated tool can't be restored — expired credentials, offline, feed outage. Before this PR, bv ran fine in all those cases. That's a new hard dependency of every command on the health of every other pinned tool.
• It's a dotnet process spawn on every single delegated invocation, and its output is swallowed (RunAsync here passes no onStdout/onStderr, unlike PinToolManifestAsync which does). A cold first restore that downloads packages will look like bv hanging with no output at all.

Cheaper shape: run dotnet tool run bv first and only restore-and-retry when it fails because the tool isn't restored; or, if you keep the always-restore, at least wire onStdout/onStderr to Console.Error so a slow restore is visible.

3. Is InstallLayout detection earning its keep?

The layout only decides the equal-version case. But if the manifest pins bv 2.1.70 and the running bv is 2.1.70, both are the same NuGet package at the same version — identical bits. Tool manifests have no floating versions (rollForward is runtime roll-forward, not package resolution), so I can't construct a case where equal version means different behaviour. If that's right, then pinMatches ⇒ run in place, unconditionally, and InstallLayout, InstallLayoutDetector, DelegationContext.InstallLayout and InstallLayoutDetectorTests (~120 + 64 lines of non-contractual path-shape matching, which the doc comments themselves flag as observed-not-guaranteed) all go away — along with the restore requirement in the equal-version path, which is the most common case for a globally-installed bv in a matched repository. Today that case pays a restore plus two process spawns for bits it already has.

The counter-argument would be a scenario where equal version ≠ equal bits. If you have one in mind, I'd rather it were written into the InstallLayout doc comment than left implicit; if you don't, this is a large, testable subsystem you could delete.

---

Behaviour worth documenting

4. The delegated child's working directory is the home directory, not the caller's cwd — DelegationService.cs:112

Deliberate, and I think correct — it pins manifest resolution to the bv home instead of whatever manifest dotnet finds walking up from cwd. But it silently changes cwd-relative semantics for the one class of arguments bv passes through untouched: forwarded MSBuild args. bv build -- -p:OutputPath=./out run from src/ resolves ./out under src/ today and under the repo root after delegation. (Related, pre-existing: ReleaseCommand.cs:233 Path.GetFullPath(artifactsPath) is cwd-relative too, so delegation actually changes where a subdirectory-launched bv release looks for artifacts — arguably fixing it, but by accident.)

The PR body says arguments are forwarded "verbatim", which is true of the tokens but not of the environment they're interpreted in. Worth a sentence in docs/DirectoryStructure.md and a mention in the CHANGELOG's breaking-change paragraph.

5. bv --version now answers for the pinned bv, not the invoked one

Consistent with "every judgment belongs to the pinned version", and you verified it works. But --version is the one flag whose entire purpose is "which binary am I running?", and the answer it now gives is the one question it was never asked. Angular, for what it's worth, prints both. At minimum the --skip-delegation help text (GlobalSettings.cs:39) or docs/DirectoryStructure.md should tell the user how to find out. Your call — flagging it as a design question, not a defect.

6. The delegation info line ignores --nologo — DelegationService.cs:103

A delegated run currently prints Delegating to bv X… on stderr, followed immediately by the child's own Buildvana CLI tool vX logo. Since the line is narration by your own definition, --nologo suppressing it seems consistent — and --nologo is already parsed by the time delegation runs.

---

Tests

7. Repo rule violation: process-global state without [NotInParallel] — tests/Buildvana.Core.Process.Tests/ProcessRunnerTests.cs:39,56

RunWithInheritedStdio_RemovesNullEnvironmentEntries sets and clears a process-wide environment variable. .claude/rules/testing.md requires [NotInParallel] for tests that swap process-global state. The unique variable name makes real interference unlikely, but the rule is explicit and TUnit parallelises by default.

Fix this →

8. Gaps

• No test for a manifest whose bv entry has an unparseable version (finding 1) — neither in DelegationServiceTests (should run in place) nor in SelfVersionServiceTests (should pick update, not install).
• RunWithInheritedStdio_WhenCancelled_KillsChildAndThrows asserts the wait returned fast, not that the tree actually died. A 15 s bound on a sleep 30 against a 250 ms token is also on the generous-but-flaky side under CI load.

Otherwise the test suite is dense and reads well — TryDelegate_RestoresThenRunsPinnedBvWithOriginalArguments asserting restore and delegated run share the same muxer is exactly the right assertion, and the comment-preservation test on the $schema rewrite is the acid test you say it is.

---

Nits

• NuGetVersion.Parse(ThisAssembly.AssemblyInformationalVersion) now appears in three places (Program.cs:201, Program.cs:266, ReleaseCommand.cs:228). One shared accessor would keep them from drifting.
• SchemaUrlRegex (SelfVersionService.cs:48) is character-for-character the regex in .buildvana/hooks/release/post-release.cs. Both sides of a contract, so duplication is defensible — a cross-reference comment in each would make that explicit. Also a candidate for [GeneratedRegex], given the repo's lean toward current language/BCL features.
• EnsureNoUnforcedDowngrade checks the manifest and global.json pins but not the $schema version, so --force isn't required to walk that one backwards. Probably fine (it's cosmetic metadata), but it's an asymmetry in a method whose whole job is symmetry.

Nothing here blocks; finding 1 is the only thing I'd call a bug, and findings 2–3 are worth a conversation before this line hardens into shipped behaviour.
· branch delegate-to-pinned-bv

[codecov]

Codecov Report

❌ Patch coverage is 92.78997% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.91%. Comparing base (619e800) to head (c8dd79a).

Files with missing lines	Patch %	Lines
...nfrastructure/Delegation/ToolResolverCacheProbe.cs	75.00%	3 Missing and 5 partials ⚠️
src/Buildvana.Core.Process/ProcessRunner.cs	90.90%	2 Missing and 1 partial ⚠️
src/Buildvana.Tool/Services/SelfVersionService.cs	97.22%	2 Missing and 1 partial ⚠️
src/Buildvana.Tool/Subcommands/ReleaseCommand.cs	0.00%	3 Missing ⚠️
...ool/Infrastructure/Delegation/DelegationService.cs	95.91%	1 Missing and 1 partial ⚠️
src/Buildvana.Tool/Services/DotNetService.cs	71.42%	2 Missing ⚠️
src/Buildvana.Tool/Services/ToolManifest.cs	94.44%	0 Missing and 1 partial ⚠️
src/Buildvana.Tool/Utilities/OwnVersion.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   51.54%   53.91%   +2.36%     
==========================================
  Files         138      151      +13     
  Lines        4076     4394     +318     
  Branches      718      787      +69     
==========================================
+ Hits         2101     2369     +268     
- Misses       1886     1926      +40     
- Partials       89       99      +10     

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

Point-by-point follow-up on the review; changes are in f2c4ff4.

1 — Adopted, with one correction to the scenario. "2.1" actually parses (NuGetVersion accepts partial versions); a genuinely unparseable version makes the manifest unusable to the dotnet CLI itself — restore, update, and install all fail to read it, since the CLI parses manifest versions with the same NuGetVersion.TryParse bv calls (ToolManifestEditor in dotnet/sdk). So switching verbs can't rescue that repository; what bv owes the user is a clear error instead of a forwarded CLI parse failure. ReadBvPin now reports entry-presence, raw version text, and parsed version separately; bv update fails up front naming the entry to fix or remove; the verb choice is keyed on entry presence; and delegation warns instead of silently running in place. Both unusable shapes are tested on both consumers.

2 — Adopted, in a stronger form than either suggested alternative. "Run first, restore on failure" can't work under inherited stdio: a resolver miss and the delegated bv's own non-zero exit are indistinguishable to the parent, so every failed build would restore-and-retry. Instead, the restore is now gated by a probe of the SDK's tool resolver cache — which is exactly and only what dotnet tool run consults: an entry matching the pinned version whose PathToExecutable exists (LocalToolsCommandResolver / LocalToolsResolverCache in dotnet/sdk; NuGet package presence is neither necessary nor sufficient). The cache is internal SDK surface, so the probe is trusted only in the skip direction — a miss, an unreadable file, or format drift falls back to restoring, i.e. to the prior behavior; the one residual window (the TFM dimension, unknowable without a process spawn) is documented in the class remarks. On a miss, the restore now streams its output to stderr, and a failed restore degrades to a warning followed by the run attempt: dotnet tool restore attempts every manifest tool even when one fails, so an unrelated tool's unreachable feed (this repo's ngbv, say) no longer blocks a delegated bv build, and when bv itself is the missing one, dotnet tool run fails visibly with its own actionable message. Net: the common delegated invocation costs one process spawn, and the coupling to unrelated tools' feeds is gone.

3 — Keeping the subsystem; the counter-scenario is now in the doc comment, per your fallback ask. Equal versions do not guarantee equal bits: build metadata never participates in version comparison, so a locally built bv carries the same version as the published package it descends from, and a mutable private feed can republish a version outright. Under the repository-authority rule those must yield; only PackageCache — the same files dotnet tool run would launch, identity by location rather than version-number inference — runs in place. One reframe: the subsystem doesn't enable delegation, it prevents the pointless self-delegation hop on every dotnet bv while keeping strict hand-over semantics for everything non-local. With the probe from point 2, the equal-version non-local case no longer pays a restore either.

4 — Documented in docs/DirectoryStructure.md and the changelog: the home-directory cwd is deliberate (nested-manifest shadowing; dotnet tool run has no manifest-path option), bv's own arguments are home-anchored anyway, and forwarded relative paths change interpretation.

5 — Documented rather than special-cased: in a delegating repository --version answers for the bv that actually runs, consistent with every other judgment the invocation makes; --skip-delegation --version asks the invoked binary.

6 — Declining. The line prints only on a version mismatch, which makes it a diagnostic — "a different bv than the one you invoked will run" — not decoration; it lives on stderr and cannot dirty piped stdout. --nologo is most common in CI, exactly where a silent version switch would be worst.

7 — Fixed as flagged.

8 — Unusable-pin tests added (see 1). On the cancellation test: elapsed < 15s against a 30-second sleep does prove the wait didn't run to completion, and proving actual tree death portably means waiting out the full sleep to assert a "finished" marker never appears — trading a soft bound for a hard 30-second test. Leaving as is.

Nits — all three taken: a shared OwnVersion.Value accessor replaces the three parse sites; the schema regex is [GeneratedRegex] with cross-reference comments on both copies (service and release hook); the downgrade-guard asymmetry is acknowledged in its comment — when a pin is newer the update throws before the schema reference is touched, so only a hand-desynced reference can walk backwards, and it's cosmetic.

🤖 Generated with Claude Code

[rdeago]

@claude please take another look.

[claude]

Claude finished @rdeago's task in 6m 52s —— View job

---

Re-review — PR #332 after f2c4ff4 / 21a675f / c6f3cf8

• Diff the follow-up commits against my prior review
• Verify finding 1 (unusable pin) in ToolManifest / SelfVersionService / DelegationService
• Review the new ToolResolverCacheProbe (finding 2)
• Review new/changed tests and docs
• Post review

Ric — the follow-up is solid, and two of the answers are better than what I proposed. Caveat up front, same as last time: I could not run the build or tests (dotnet build needs an approval I don't have), and this time WebFetch was denied too, so I could not re-read the dotnet/sdk sources; everything below is from reading this branch plus what I know about the SDK. One new finding, all of it cheap.

---

Round-1 findings — verified

1. Confirmed fixed, and your correction is right. NuGetVersion.TryParse("2.1") does succeed, so the scenario I named was wrong; the reachable state is a genuinely unparseable string, and since ToolManifestEditor rejects the whole manifest on it, no dotnet tool verb can repair the repository — fail-fast with a message naming the entry is the correct remedy, not a verb switch. BvManifestPin(HasEntry, VersionText, Version) reports the three states distinctly; EnsureUsableManifestEntry (SelfVersionService.cs:170) runs before the downgrade guard and before any write, so nothing is touched; delegation warns and runs in place (DelegationService.cs:97-112); both shapes are covered by [Arguments] in both consumers' tests.

2. Better than either alternative I offered. You're right that "run first, restore on failure" is unimplementable under inherited stdio — the parent cannot tell a resolver miss from the child's own non-zero exit. The probe (ToolResolverCacheProbe.cs) mirrors what dotnet tool run actually consults (<cli home>/.dotnet/toolResolverCache/1/<lowercased id>, entry matched on version with an existing PathToExecutable) and is trusted only in the skip direction: every failure mode — missing file, non-array root, non-object rows, IOException/JsonException — returns false and restores. The TFM window is documented where someone hitting it will look. The coupling I objected to is gone twice over: the common delegated invocation no longer restores at all, and when it does, a failure is a warning (DelegationService.cs:142-158) so an unrelated tool's dead feed can't block bv build. Restore output now reaches the delegation writer, which was the other half of the complaint.

3. Accepted. Build metadata not participating in comparison (locally built bv ≡ published package by version) plus republishable private feeds is a real counter-scenario, and it's now in the InstallLayout doc comment where I asked for it. Worth adding that the probe changed the economics of my objection anyway: the equal-version non-local path no longer pays a restore, so keeping the detector costs one file plus its tests, and the reframe — it exists to avoid a self-delegation hop, not to enable delegation — is the right way to describe it.

4, 5. Documented in docs/DirectoryStructure.md (both the home-directory cwd and its effect on forwarded relative paths, and --skip-delegation --version) and in the CHANGELOG's feature paragraph.

6. Your call stands. Conditional on mismatch + on stderr makes it a diagnostic, and I'd have argued the same knowing it only prints when the version actually switches.

7, 8. [NotInParallel] is on with a comment citing the rule — and you gave ToolResolverCacheProbeTests.GetDefaultCacheDirectory_HonorsDotnetCliHome the same treatment unprompted, which is the same class of test. Unusable-pin tests are present on both consumers. The cancellation-test argument is fair: a hard 30 s test to prove tree death is a bad trade.

Nits. OwnVersion.Value is now the single parse site (Program.cs:201,271, ReleaseCommand.cs:229); [GeneratedRegex] with reciprocal cross-reference comments on both copies; the downgrade asymmetry is explained rather than papered over.

---

New finding

The bv manifest entry is looked up case-sensitively; the dotnet CLI's key isn't — src/Buildvana.Tool/Services/ToolManifest.cs:51

JsonHelper loads nodes with PropertyNameCaseInsensitive = false (JsonHelper.cs:30,60), so tools[BvPackageId] is an ordinal match against the literal "bv". The dotnet CLI, on the other hand, turns every manifest tool key into a PackageId, which lowercases it — the same normalization your own probe relies on for the cache file name (ToolResolverCacheProbe.cs:60-61, whose CA1308 suppression says exactly this). So a manifest keyed "BV" is a perfectly usable manifest for the CLI and invisible to bv:

• delegation reads HasEntry: false and silently runs in place — no delegation, no warning, in a repository that does pin bv;
• bv update takes the no-entry branch and runs dotnet tool install bv --version X --create-manifest-if-needed, which fails with Tool 'bv' is already installed — the round-1 failure mode, reappearing through a different door.

Likelihood is low (manifests are normally written by dotnet tool install, which emits the lowercase id) but the blast radius is the same as finding 1, and ReadBvPin's doc comment currently claims to judge the manifest exactly as the CLI does, which this makes untrue. Matching the tools object's properties with OrdinalIgnoreCase is a couple of lines. Flagging the verification gap honestly: I could not re-read PackageId/ToolManifestEditor from dotnet/sdk in this run, so treat the CLI half as reasoned from NuGet's case-insensitive package-id rule (and from your own lowercasing comment) rather than freshly confirmed.

Fix this →

---

Smaller notes, none blocking

• BV_DELEGATED is inherited by grandchildren, for whom it isn't true. The marker means "you are the delegation target", but it survives into every descendant: inside a delegated run, a release hook that shells out to a global bv gets the marker, skips delegation, and runs whatever version it happens to be — silently, and only in the delegated case, so the behaviour differs between dotnet bv release and a delegated bv release. docs/EnvironmentVariables.md notes the inheritance but frames it as a feature for hooks that want to read it. bv already applies per-command env overrides on child dotnet invocations, so clearing the variable there would be a one-liner if you agree it's worth closing; the typed RuntimeInfo.DelegatingVersion already covers the legitimate reader.
• c6f3cf8 reads worse than what it replaced — DelegationService.cs:115-122. A switch over a bool with one when guard and no case true arm means the most interesting path — equal version, non-PackageCache layout ⇒ fall through and delegate silently — is now expressed by the absence of a case. The two ifs said it out loud. Taste, not correctness.
• GetDefaultCacheDirectory's comment overstates the empty-home fallback — ToolResolverCacheProbe.cs:93. With an empty home, Path.Combine("", ".dotnet", "toolResolverCache") yields a relative path resolved against the current directory, not "a directory that cannot exist" — and from a user's home directory that relative path is the real cache. Harmless (the probe only ever skips restores), but the comment claims a guarantee the code doesn't make.
• Partial-update window in UpdateRepositoryAsync — the manifest (a process spawn) is pinned before the two file writes, so a denied global.json write leaves the repository half-updated. It self-heals: re-running bv update reads the manifest as unchanged and retries the write. Noting it only so the ordering stays deliberate.

Nothing here blocks the merge. The case-sensitivity item is the only code change I'd suggest; the rest is documentation or taste.
· branch delegate-to-pinned-bv

[rdeago]

All five items are in 277dc09.

New finding — confirmed and fixed. Your reasoning held up against the sources you couldn't reach this run: ToolManifestEditor.GetToolManifestPackageFromOneManifestFile constructs new PackageId(toolJson.Name), and PackageId's constructor lowercases, so tool restore and tool run treat a BV key as bv. ReadBvPin now matches the entry case-insensitively, and both consumers have BV-keyed manifest tests. One footnote for the record: the CLI is not uniformly case-insensitive itself — ToolManifestEditor.Remove compares the raw JSON key ordinally against the lowercased id — but the verbs delegation depends on go through PackageId.

BV_DELEGATED inheritance — agreed and closed. Both of bv's child-spawn funnels (RunDotNetAsync, RunFileBasedAppAsync) now pass their environment through a helper that layers it over the marker's removal, so builds and hooks no longer carry it; a bv reached through them makes its own delegation decision, and RuntimeInfo.DelegatingVersion remains the way a hook learns about delegation. These funnels are the only spawn sites reachable with the marker present — DelegationService's own spawns happen only when the marker is absent, and a delegated bv never runs update. EnvironmentVariables.md now documents the removal instead of framing the inheritance as a feature.

The switch — reverted. Ric's verdict: the two ifs read better; with the early returns the method is linear enough that no simplification was owed. (The switch was ReSharper's suggestion, for what that's worth.)

The fallback comment — corrected to say what the code does: an all-empty home degrades to a relative path resolved against the current directory, and a wrong cache directory only ever costs a restore.

The update ordering — pinned with a comment stating the rationale: manifest first because it is the one step with an external actor, and the partial-update window self-heals on rerun exactly as you described.

[rdeago]

@claude please take another look.

[claude]

Claude finished @rdeago's task in 5m 29s —— View job

---

Re-review — PR #332 after 277dc09

• Diff 277dc09 against the state I reviewed at c6f3cf8
• Verify the case-insensitive bv pin lookup and its tests
• Verify BV_DELEGATED removal on child spawns (both funnels)
• Check the reverted switch, the corrected comment, the update ordering
• Fresh pass over the parts I had read least closely
• Post review

Ric — all five items from round 2 are in and correct. Same caveats as before, stated up front so you can weigh what follows: dotnet build/test needed an approval I don't have, and WebFetch was denied again, so I could not re-read dotnet/sdk sources. Everything below is from reading this branch.

---

Round-2 items — verified

Case-insensitive manifest key. FindBvEntry (ToolManifest.cs:69-80) walks the tools object with OrdinalIgnoreCase, so a BV-keyed entry now reads as a pin. Both consumers are covered — TryDelegate_WithDifferentlyCasedManifestKey_ReadsThePin asserts the delegation and the info line, and UpdateRepository_WithDifferentlyCasedManifestKey_RunsDotnetToolUpdate asserts the verb is update, which is the half that would have failed with "already installed". The doc comment now states the normalization and why. Your footnote about ToolManifestEditor.Remove comparing the raw key ordinally is worth having on the record — it means CLI case-insensitivity is per-verb, and the verbs delegation and bv update depend on are the PackageId ones.

BV_DELEGATED stops at the delegated child. DotNetService.ChildEnvironment (DotNetService.cs:351-363) seeds the removal and lets configured values layer on top; both spawn funnels use it (:288, :408). I checked for spawn sites you might have missed: RunAsync/RunWithInheritedStdioAsync appear only in DelegationService (marker provably absent — TryDelegateAsync returns early when it's present), SelfVersionService.PinToolManifestAsync (update is delegation-exempt, and it spawns dotnet tool install/update, which never runs bv), and these two. So the claim that these are the only reachable-with-marker sites holds. RuntimeInfo.DelegatingVersion is still populated from bv's own environment (ReleaseCommand.cs:230), before the removal applies to children — the hook still learns it, through the typed context. Three tests pin the layering, including the explicit-value-wins case.

The two ifs, the fallback comment, the ordering rationale. All as described; the switch revert restores the "equal version, non-PackageCache ⇒ delegate silently" path to something you can read.

---

One thing I'd verify before merge

bv update --force may hit the dotnet CLI's downgrade guard — src/Buildvana.Tool/Services/SelfVersionService.cs:255-256

With --force, PinToolManifestAsync runs dotnet tool update bv --version <older-than-pinned>. dotnet tool update carries an --allow-downgrade option, and an option like that exists to disable a guard — my recollection is that recent SDKs refuse to move a tool to a lower version without it. If that guard covers the local-manifest path on SDK 10.0.302 (what global.json pins), then --force — the flag whose entire purpose is to permit a downgrade — fails with a CLI error instead of downgrading.

What makes this worth checking rather than dismissing: nothing in the suite can see it. UpdateRepository_WithNewerPins_AndForce_Downgrades and ExecuteAsync_ForwardsForceToTheUpdate both drive a FakeProcessRunner, so they prove bv asks for the downgrade, never that the CLI performs it. And the deliberate-downgrade scenario your comment names (bisecting a regression) is exactly when a user meets it.

The check is a two-minute real run: in a repo pinned newer, bv update --force. If the guard applies, the fix is one conditional argument on the update path only (the install path has no existing entry to downgrade). I'd rather flag it as "verify" than assert it — I could not read ToolInstallLocalCommand/ToolUpdateLocalCommand this run.

Fix this →

---

Smaller notes

• The delegating bv does judge one sliver of the command line — Program.cs:52. CliArgSplitter.Split runs before TryDelegateAsync, and CliOptionReader.ReadValue throws BuildFailedException when a value-bearing global has no following value. So bv -v (a plausible typo for --version) in a pinned repository is rejected by the invoked binary, not delegated — a narrow exception to "a delegating bv neither parses the command line nor reads the configuration file", which docs/DirectoryStructure.md states without qualification. Harmless today, since any bv version would give the same message; it only bites if a future version changes what -v means. Either catch-and-delegate, or qualify the sentence.
• The null-removal contract now rides on the hot path but is only tested on the other implementation. ProcessRunnerTests.RunWithInheritedStdio_RemovesNullEnvironmentEntries covers the raw-Process path. Every build, hook, and dotnet spawn now goes through RunAsync — CliWrap — and depends on its "null value removes the variable" semantics, which nothing in the suite locks in. It's the right semantics (the IReadOnlyDictionary<string, string?> signature exists for that reason), but it's third-party behavior on which "hooks make their own delegation decision" now rests. A twin test against RunAsync would catch a CliWrap change at upgrade time — with [NotInParallel], same as its sibling.
• ReadBvPin reads one manifest; the CLI reads a chain. ToolManifest.ReadBvPin looks only at <home>/.config/dotnet-tools.json, while dotnet tool run walks up merging manifests until isRoot: true. A home manifest with "isRoot": false whose bv entry lives in an ancestor pins bv for the CLI but not for bv: no delegation, and bv update takes the install --create-manifest-if-needed branch. "The repository's own manifest is what bv pins on" is a defensible rule — it's just not the parity the case-insensitivity paragraph now advertises, so one clause in that doc comment would keep it honest. Vanishingly rare in practice.

---

Everything else I looked at this round holds up: the delegation decision table and its tests, the probe's skip-direction-only trust, InstallLayoutDetector's .store-before-package ordering and its OrdinalIgnoreCase segment matching (which also survives NuGet's lowercased package-cache folders), EnsureUsableManifestEntry running before any write, and the docs, which now say what the code does. The bv update --force question is the only thing I'd resolve before merging; the rest is your call.
· branch delegate-to-pinned-bv

[rdeago]

All four round-3 items are addressed, one commit each.

bv update --force vs. the CLI's downgrade guard — confirmed and fixed (6c6a496). We ran your two-minute experiment against the SDK pinned in global.json (10.0.302): dotnet tool update nbgv --version 3.6.146 over an installed 3.7.115 fails with exit code 1 — "The requested version 3.6.146 is lower than existing version 3.7.115 (…). Use the --allow-downgrade option to allow this update." So the guard does cover the local-manifest path, and forced downgrades died exactly as you suspected. The fix is the conditional argument you outlined: PinToolManifestAsync appends --allow-downgrade exactly when the manifest change actually is a downgrade — a state only reachable forced, since EnsureNoUnforcedDowngrade throws otherwise — so bv passes the flag precisely when it has itself authorized the downgrade, and the CLI's guard stays armed on every other path. The forced-downgrade test now expects the flag, and a new test (UpdateRepository_WithForce_AndOlderManifestPin_UpdatesWithoutAllowDowngrade) pins its absence on a forced upgrade, so the flag keys on direction, not on --force.

Delegating bv judges one sliver of the command line (dad6fd0). Right — the split runs first, and CliOptionReader.ReadValue rejects a value-bearing global with nothing after it. We qualified the sentence in docs/DirectoryStructure.md rather than catch-and-delegate: delegation consumes the split's outputs (the subcommand for the update exemption, --skip-delegation), so delegating on a parse failure would need a second, cruder pre-parser to reroute an error message that is identical in every bv version anyway.

Null-removal contract on the hot path (192f6b2). Agreed and done: a twin of RunWithInheritedStdio_RemovesNullEnvironmentEntries now runs against RunAsync, [NotInParallel] like its sibling, locking CliWrap's "null removes the variable" semantics in at upgrade time.

ReadBvPin reads one manifest, the CLI reads a chain (0a3e772). Behavior stays as is — the repository's own manifest is the pin bv manages — but the doc comment now states the deliberately narrower scope explicitly, so the case-insensitivity paragraph's CLI-parity argument can no longer be over-read.

Gates on the pushed tree: the full bv pack gate (build, tests, pack) exits 0, and ReSharper inspectcode reports zero results at WARNING severity.

@claude Could you take another look, re-reviewing the range 277dc09..0a3e772?

[claude]

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

---

Re-review — PR #332, range 277dc09..0a3e772

• Diff the four commits
• Verify --allow-downgrade (6c6a496)
• Verify the docs qualification (dad6fd0)
• Verify the CliWrap null-removal twin (192f6b2)
• Verify ReadBvPin's scope note (0a3e772)
• Post review

Ric — all four land, and the --allow-downgrade one lands better than the shape I sketched. Caveats first, since two of them bear on what I could confirm: dotnet needed an approval I don't have (no build, no test run, and no dotnet tool update --help to read the option list), and WebFetch was denied again, so dotnet/sdk sources were once more out of reach. Everything below is from reading this branch.

---

The four items

1. --allow-downgrade — correct, and keyed on the right thing (SelfVersionService.cs:260-263)

The empirical half you ran settles the question I could only pose: the guard does cover the local-manifest path, so --force was dead on arrival before this commit. The implementation is tighter than "pass it when forced":

• isDowngrade is computed from the manifest pin versus _ownVersion with VersionComparer.VersionRelease — the same basis the CLI compares on (its existing version comes from the manifest entry, and NuGetVersion's default comparison likewise ignores build metadata), so bv's answer and the CLI's cannot disagree.
• The unreachable-unforced claim in the comment holds structurally: EnsureNoUnforcedDowngrade (:212) throws for a newer manifest pin when force is false, and it runs at :158, before PinToolManifestAsync at :163.
• isDowngrade can only be true when currentPin is not null, which only happens on the HasEntry: true construction path, so it can never combine with the install --create-manifest-if-needed branch — the ternary chain's ordering is safe, not just conventional.
• Equal versions return "unchanged" at :249 before any of this, so the flag never rides along on a no-op.

UpdateRepository_WithForce_AndOlderManifestPin_UpdatesWithoutAllowDowngrade is the right test to have added: a forced upgrade is the case where a force-keyed implementation would have silently disarmed the CLI's guard, and it now fails loudly if someone rewires the condition. Pairing it with a newer global.json pin is what makes --force load-bearing in that fixture — worth the sentence it got.

One residual, low-likelihood and unverified because I couldn't read the CLI: the flag is now passed to whichever SDK the muxer resolves, and --allow-downgrade is newer than the oldest SDK Buildvana still admits (BV_MinMSBuildVersion is 17.4 ⇒ SDK 7). On an SDK old enough to have neither the guard nor the flag, a forced downgrade would fail with "unrecognized option" where it previously just worked. Requires a repo pinning an SDK that old and a forced bv downgrade, so I'd file it under "know it exists", not "fix it".

2. The command-line qualification (dad6fd0) — accurate, and I checked it exhaustively rather than taking the sentence's word for it. CliOptionReader throws in exactly one place (ReadValue, :87), --verbosity/-v is the only value-bearing global (CliArgSplitter.cs:33 — every other global is a ReadFlag), and VerbosityParser.Parse runs at Program.cs:112, after delegation, so an invalid verbosity value is still the pinned bv's to judge. The qualification is therefore complete, not just improved: "a value-bearing global with nothing after it" is the whole of what a delegating bv rejects on its own. Agreed on not catch-and-delegating — rerouting an error every version phrases identically would cost a second parser to save nothing.

3. The CliWrap twin (192f6b2) — does the job, and does it honestly: the parent variable is set first, so the assertion distinguishes "removed" from "never inherited" (exit 9 vs. exit 1) instead of passing vacuously. [NotInParallel] matches its sibling, and the comment says why the twin exists — that the contract on this path is CliWrap's, not ours — which is the part that makes it survive a future cleanup.

4. ReadBvPin's scope note (0a3e772) — states the narrower behavior plainly and stops the case-insensitivity paragraph's parity argument from being read as parity with the manifest chain. Behavior unchanged, as agreed.

---

Two follow-ons, both documentation

The unqualified sentence still stands in the changelog — CHANGELOG.md:44

dad6fd0 qualified docs/DirectoryStructure.md, but the same claim appears verbatim in the feature paragraph: "A delegating bv neither parses the command line nor reads the configuration file". The two now disagree in precision, and the changelog is the copy a user reading release notes meets first. Same one-clause fix as the doc got.

Fix this →

bv update's decision is scoped to the home manifest; the CLI it spawns is not. The new ReadBvPin paragraph says "both delegation and bv update scope themselves to it". For delegation that is airtight on both halves — the decision reads the home manifest, and dotnet tool run runs from the home directory, where the nearest manifest is that same file. For update it is true of the decision only: on the no-entry branch, dotnet tool install --create-manifest-if-needed walks up like any other CLI invocation, so in a nested layout (home has no manifest, an ancestor does) it can write the entry into that ancestor rather than creating one here. Flagging the verification gap honestly: I could not re-read ToolManifestFinder/ToolInstallLocalCommand this run, so the walk-up-then-create ordering is recollection, not fresh confirmation — and the scenario is the same rare shape you already decided to document rather than handle. A clause narrowing the claim to the decision would keep the paragraph exact.

---

Nothing in this range blocks. The --allow-downgrade fix closes the last thing I'd have wanted resolved before merge, and it closed it with a real run rather than an argument — which is the half I couldn't supply.
· branch delegate-to-pinned-bv

[rdeago]

Both follow-ons addressed:

• Changelog qualification (468029d): the delegation paragraph in CHANGELOG.md now carries the same qualification as docs/DirectoryStructure.md — the delegating bv runs the minimal split, and only a value-bearing global option with no following value is rejected before delegation, identically in every version.
• ReadBvPin scope claim (c8dd79a): the paragraph now says delegation and bv update key their decisions on the home directory's manifest alone, while the spawned dotnet CLI follows its own manifest-location rules — landing on the same manifest whenever it has a bv entry, though bv update's no-entry install can reach an ancestor's. Your recollection of the walk-up behavior matched what we verified from dotnet/sdk sources in the previous round.

Gates on the pushed tree: bv pack exit 0, ReSharper inspectcode zero results at WARNING severity, all tests passing.