Give ReleaseCommand an end-to-end test harness

[rdeago]

Checklist of related issues / discussions

• Closes Give ReleaseCommand an end-to-end test harness #335

Proposed changes

ReleaseCommand.ExecuteAsync now runs end to end under test, over a real Git repository in a temporary directory, with only the boundaries faked: child processes (FakeProcessRunner), the hook's file-based app (FakeFileBasedAppRunner), and the server (a recording ServerAdapter). The service graph, the versioning, the changelog, the commits and the push are the real ones. Line coverage of ReleaseCommand goes from 0/154; 36 tests were added.

Composition. Program.BuildServiceProvider was private inside an [ExcludeFromCodeCoverage] class, so the graph every command runs on was unreachable from a test. Its registrations moved verbatim to ServiceCollectionExtensions.AddBvServices(); Program keeps the five singletons a host decides for itself. The harness calls the same method and registers its fakes after it, where the last registration of a service type wins — so the composition under test is the real one, not a copy that drifts.

Sequencing is asserted from effects, not from calls. Every recorded step carries the repository's state at the moment it happened, so "the release commit existed before the artifacts were packed" and "the branch was pushed before the packages were" are read off the repository. The fake server adapter carries no logic and its knobs are grouped by whether they will belong to the CI platform or to the Git host, so the planned split can cut it in two without rewriting what it answers.

Buildvana.Core.Testing. TempGitRepo gained a pushable bare remote, tagging, committer identity, and history reporting. Two defects surfaced while building it, both fixed here: the default log sort is by timestamp, so commits made within the same second came back in arbitrary order (now topological); and pointing libgit2's configuration search paths per instance mutated a global underneath repositories other threads held, which crashed the test host with a corrupted heap (now a type initializer, once per process). The isolation also makes every TempGitRepo-based test independent of the machine's global Git configuration, which previously decided whether a committer identity existed at all — laptop and CI runner took different code paths.

Additional changes

The harness found three things the issue did not foresee. Each is its own commit.

• bv release refused an impossible release only after building it. The preliminary checks — cloud build, branch, public-release branch, committer identity — and the version-spec computation ran after the verification pass, so a release that could never succeed cost a full clean, build and test cycle first. They now run before it. (ReleaseCommand)
• A version-spec change published a version one patch below its own artifacts. --bump minor on the 2.3 line tagged 2.4.0-preview while the commit it tagged built as 2.4.1-preview: UpdateRepository staged the version file into the release commit after the version had been computed from that commit, and the Git height is computed from committed content, so the new version line looked as if it had no committed history — height 0, which is not a height at all. The packages were built, attached and pushed as 2.4.1-preview under a 2.4.0-preview tag, and produced-package discovery matched none of them, silently skipping the dogfood rewrites. NameReleaseCommit now holds the rule in one place: the commit is made under a provisional message and named afterwards, and every method that changes its tree ends by calling it. (ServerRelease, CHANGELOG)
• Nothing prevented reaching height 0 by another route — a repository whose VERSION was never committed does it without any bump. The final consistency check now rejects it, where the version becomes final and before anything is packed or tagged. Building such a state stays legitimate; only publishing is refused. (VersionService, CHANGELOG)

Also: two pre-existing over-long lines in Program.cs, wrapped in their own commit per the whole-file line-length rule.

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

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

[codecov]

Codecov Report

❌ Patch coverage is 88.38710% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.13%. Comparing base (fc9be99) to head (5daaae0).

Files with missing lines	Patch %	Lines
...DependencyInjection/ServiceCollectionExtensions.cs	71.42%	12 Missing ⚠️
src/Buildvana.Core.Testing/TempGitRepo.cs	93.25%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #341       +/-   ##
===========================================
+ Coverage   56.48%   73.13%   +16.65%     
===========================================
  Files         160      161        +1     
  Lines        4458     4590      +132     
  Branches      782      793       +11     
===========================================
+ Hits         2518     3357      +839     
+ Misses       1840     1072      -768     
- Partials      100      161       +61     

☔ 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 8m 39s —— View job

---

Review of #341 — ReleaseCommand end-to-end harness

• Read the repo rules and the PR diff
• Review the production-code changes (ServerRelease, VersionService, ReleaseCommand, composition move)
• Review Buildvana.Core.Testing changes (TempGitRepo, TempGitCommit)
• Review the test harness and the tests
• Post the review

I read the diff statically (git diff origin/main...HEAD) plus the surrounding production code — GitService, GitHeightCalculator, VersioningService, PublicApiFilesService — to check the reasoning behind the three out-of-scope fixes. I did not run the build or the test suite in this job; CI and Codecov results already stand.

Verdict: no correctness defects found. The three bug fixes are sound, and I verified each one's premise rather than taking the description at face value:

• Height 0. GitHeightCalculator.ComputeHeight gives a qualifying commit 1 + max(parent heights), and IsInLine returns false when the commit has no VERSION blob, so 0 really is unreachable for a line with committed history. VersioningService reads Spec from the working tree but Height from commits, so height 0 means exactly what the message says. VersionFile.Load already fails when the file is absent, so the new check can't misfire on a repository that simply has no VERSION. The check is at VersionService.cs:112-114, inside isFinalCheck, and ReleaseCommand.cs:195 puts it after EnsureReleaseCommit and before both TagExists and the artifact pass — the only place it can be both final and preventive.
• Staging before the commit. GitService.Commit commits the index, and Commands.Stage stages only the explicit paths, so moving Stage above EnsureReleaseCommit changes what the commit contains without widening it. The pre-existing "whatever is in the index gets swept in" behaviour is unchanged, since the old empty commit committed the index too.
• Checks before the build. I checked the claim in the new comment at ReleaseCommand.cs:58-61. GetApiChangeKind reads PublicAPI.Unshipped.txt from source, and IsPublicRelease / CurrentBranch / IsCloudBuild are all repository or environment reads. Nothing in the moved block depends on build output, and the only write in it (git.CommitterIdentity → .git/config) is outside what Clean touches. The claim holds.

The harness design is the strongest part: recording repository state at each step (ReleaseEvent) so ordering is read off effects rather than call counts is what makes Release_PushesRepository_BeforePushingPackages an actual assertion instead of a mock-interaction check.

Findings below are quality, not correctness.

---

1. UpdateRepository writes three commit objects where one suffices, and logs two operations for one — src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs:111

When UpdateRepository is the call that creates the release commit — the common case, e.g. the version-file bump — the work happens twice:

_git.Stage(files);
EnsureReleaseCommit();          // commits the staged files, then NameReleaseCommit() amends with the final message
_reporter.Info("Amending release commit...");
_git.Commit(ProvisionalMessage, amend: true, allowEmpty: true);   // amends straight back to the provisional message
NameReleaseCommit();            // recomputes the height and amends to the same final message again

Net effect: one create plus three amends, and two _version.Update() calls (each a full walk of the version line) where one create plus one amend and one height computation are enough. It is correct — the index is unchanged across the redundant amends, so the tree and message land where they should — but the log reads Creating release commit... immediately followed by Amending release commit... for a single logical operation, which will mislead whoever next debugs a release from CI output.

The shape that says what is meant:

_git.Stage(files);
if (!_repositoryUpdated)
{
    EnsureReleaseCommit();
    return;
}

_reporter.Info("Amending release commit...");
_git.Commit(ProvisionalMessage, amend: true, allowEmpty: true);
NameReleaseCommit();

Fix this →

2. EnsureReleaseCommit's XML docs no longer describe what it does — src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs:59

The summary still reads "creating an empty one if necessary" and the remarks "The first call creates an empty commit, refreshes version information…". Since UpdateRepository now stages first, the first call routinely commits files. The contract to document is the one NameReleaseCommit's own remarks state well: the commit is made from whatever is staged, under ProvisionalMessage, and named afterwards. Worth folding into the same commit as finding 1 — they are the same change.

3. TempGitRepo mixes cached and fresh repository handles — src/Buildvana.Core.Testing/TempGitRepo.cs:67,72

GetCommits and TagNames deliberately open a handle of their own, with the rationale spelled out in the doc comment: commits made by code under test, through its own handle, must be visible immediately. HeadSha (line 72) and CurrentBranchName (line 67) still read the cached _repository. Nothing breaks today — every test reads HeadSha before RunAsync, and ReleaseCommandFailureTests compares it against a fresh-handle GetCommits(1)[0].Sha — but the asymmetry is a trap for the next test that reads HeadSha after the command has run, and the failure would look like a rollback bug rather than a stale handle.

Fix this →

4. The fake pack stamps the version the command believes, not the one the repository yields — tests/Buildvana.Tool.Tests/ReleaseHarness.cs:250

var version = _services.GetRequiredService<VersionService>().CurrentStr;

That is the very object under test. A real dotnet pack computes the version independently, from the repository — which is exactly the divergence the 2.4.0 / 2.4.1 bug consisted of. ComputeVersion() (line 159) already does the independent computation, and one test uses it; using it in WriteArtifacts instead would make "the artifacts carry the version a build of this commit produces" an invariant of all 36 tests rather than an assertion of one, and would have made the bug show up as missing packages in almost any bump scenario.

Two callers, one line — and it costs nothing, since the harness always configures prereleaseTag: "preview", which is what ComputeVersion hardcodes.

5. No test covers a bump together with dogfooding — tests/Buildvana.Tool.Tests/ReleaseCommandTests.cs:239-268

Every bump case runs with Dogfood = false, so the reported symptom of the bug this PR fixes — produced-package discovery matching nothing, self-reference rewrites silently skipped — is not asserted anywhere. Release_PublishesTheVersionItsArtifactsAreBuiltWith covers the cause (the tag names the built version); it does not cover the consequence.

I see why Dogfood = false is there: with it on, the post-release commit is itself in the version line, so ComputeVersion() at assert time would read one higher. A test that asserts on the rewritten files rather than on ComputeVersion() sidesteps that: --bump minor, dogfooding on, then assert global.json and friends say 2.4.1-preview and are in the post-release commit.

Fix this →

---

Smaller notes

• src/Buildvana.Core.Testing/TempGitRepo.cs:34 — the config-isolation directory is a fixed path under Path.GetTempPath(). The comment's guarantee ("only ever created, never written to") holds for us, not for anything else that can write to shared temp: on a multi-user box a planted /tmp/bv-test-gitconfig/.gitconfig would be read as global config by every test, which is the one thing the type initializer exists to prevent. Directory.CreateTempSubdirectory would guarantee emptiness at the cost of one empty directory per process — the current one is never removed either, so it is close to a wash.
• src/Buildvana.Core.Testing/TempGitRepo.cs:236 — Dispose deletes RootPath first; if that throws (a stray Windows lock on a Git object file is the classic), the bare-remote directories leak. A try/finally, or deleting remotes first, makes cleanup total.
• src/Buildvana.Core.Testing/TempGitRepo.cs:122 — _remotePaths.Add(name, path) runs before Remotes.Add, so a duplicate remote name leaves the dictionary holding a path for a remote that was never created. Reordering the two lines costs nothing.
• tests/Buildvana.Tool.Tests/ReleaseHarnessOptions.cs — no knob for the adapter's push credentials, so RecordingServerAdapter.PushUser/PushSecret never go null and the "No push credentials provided by the server adapter" warning branch (ReleaseCommand.cs:78-81) is unexercised. One init property away, if you want it.
• tests/Buildvana.Tool.Tests/ReleaseHarness.cs:78-83 — the env var is set and Repo is created inside the constructor; if PopulateRepository or BuildServiceProvider throws, using var harness never binds and the temp repository plus the env var survive the test. Test-only, but a wrapped try/catch that disposes and rethrows keeps a failing-constructor scenario from polluting the next test.
• AddBvServices doc — "the last registration of a service type is the one resolved" is true for GetRequiredService<T>, but both registrations remain in the collection, so an IEnumerable<T> injection would see the real one alongside the fake. Nothing does that today; worth a half-sentence in the remarks so the next person fakes a boundary with the same confidence.

What I liked

• NameReleaseCommit holding the ordering rule in one place, with the two-ended constraint (height needs a final tree; message needs the height) written down. That is the kind of comment that stops the bug coming back.
• The grouping of RecordingServerAdapter's knobs by the future CI-platform / Git-host split — cheap now, and it makes the split a cut rather than a rewrite.
• TagAnotherBranch's comment explaining why the tag has to be off-branch for the tag check to be the thing under test. Without it, the test looks needlessly elaborate.
• Fixing the two TempGitRepo defects (topological sort, per-instance config search paths) in the same PR rather than filing them, per the out-of-scope rule.
  · branch release-command-harness

[rdeago]

Nine commits, tip df23a47. Gate is green: bv pack clean, inspectcode 0 errors / 0 warnings / 0 info, 357 tests passing.

Findings

1 + 2 — three commit objects where one suffices, and EnsureReleaseCommit's stale docs — b453071

Done, as a single commit: they are one change. I had flagged the double amend myself when opening the PR and argued uniformity over a branch; I'm dropping that argument. Creating release commit... immediately followed by Amending release commit... for one logical operation is a real cost, paid by whoever next reads a release from CI output, and the branch is one if. Checked both call sites before taking it: ReleaseCommand.cs:109 is the creating one, ReleaseCommand.cs:209 — the post-pack changelog title update — is the amending one, and the early return is correct for both. The docs now state the contract as it stands: the commit is made from whatever is staged, then named.

3 — mixed cached and fresh handles — 0c407df

Done, and one step further than proposed. The rule is a property of the type rather than of two of its members, so it now lives in the type's remarks: members that read state open a handle of their own, and the handle the class keeps is for members that change something. HeadSha and CurrentBranchName follow it.

4 — the fake pack stamps the version the command believes — 0ca47cf

Done; the best catch of the review. WriteArtifacts calls ComputeVersion() now. The suite passes unchanged, which is exactly the point: what was an assertion in one test is an invariant of every test that packs anything.

5 — no bump-with-dogfooding case — c69ba2f

Done, asserting on the rewritten files rather than on a recomputed version, as you suggested, so that the post-release commit moving the height stays out of the way. The case fails on the pre-fix code twice over: the release commit carries the wrong version, and the self-references keep the old one because discovery matched nothing.

Smaller notes

a — fixed path under the temp directory — 32144e9

Done, and I disagree with "close to a wash" in both directions. The argument for changing it is stronger than stated: Directory.CreateDirectory adopts an existing directory whoever it belongs to, and the temp directory is shared between users on Linux, so the mechanism that exists to keep the machine's configuration out was one planted .gitconfig away from being the thing that let it in. But the leak is not the price either. Nothing may delete that directory while a repository can still be opened — restoring the search paths mid-process is the race the type initializer exists to avoid — which makes process exit the first moment it can go and the last at which any code of ours runs. That is where it goes now; verified that no bv-test-gitconfig-* survives a test run.

b — Dispose leaks the remotes when the working tree won't delete — 9da55e7. Done, try/finally.

c — the remote is recorded before it exists — 3a61b80. Done, reordered.

d — no knob for push credentials — 98f3469. Done, and worth having for a reason past coverage: it is the one input the command deliberately treats as non-fatal when missing, and nothing would have objected if that quietly became a hard failure.

f — AddBvServices overstates last-registration-wins — df23a47. Done.

e — the harness constructor leaks if it throws — not done. The scenario it guards against is "the harness itself is broken", which surfaces as a failing test on the spot, and the environment variable it would leak is read by nothing outside these same tests. A catch-dispose-rethrow block buys little against that, and every reader of the file pays for it. Say so if you think the failure mode is worse than I read it.

No changelog entry for any of the above: nothing here changes behaviour beyond log wording, and the two release bugs are already recorded.

@claude please review again.

[claude]

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

---

Re-review of #341 — nine follow-up commits (b453071..df23a47)

• Read the nine new commits
• Verify findings 1–5 as addressed
• Verify smaller notes a–d, f
• Assess the reasoning on note e (not done)
• Post the review

Verdict: all nine check out, no new defects. I read the diff against e0ea6b2 plus the surrounding code, and re-derived each claim rather than trusting the commit messages. I did not run the build or the suite in this job; your gate result and CI stand.

---

Findings 1–5

1 + 2 (b453071) — Correct, and I checked the two call sites myself rather than taking them from your reply. ReleaseCommand.cs:109 (version-spec change) is the only one that can be the first, and ReleaseCommand.cs:140/182/209 are all downstream of it or of EnsureReleaseCommit at line 191, so the amend path is never reached with no commit to amend. The rollback registration still lives inside EnsureReleaseCommit, so the early return does not skip it, and _repositoryUpdated is set nowhere else — it is exactly the predicate "a release commit exists". The docs now say what the code does; EnsureReleaseCommit's "commits whatever is staged — nothing at all, when no caller has staged anything" is the contract in one line.

I take the point about uniformity too. The if reads as the rule rather than as a special case, which is what the old shape was missing.

3 (0c407df) — Done, and lifting it to a type-level invariant is better than the two-property fix I proposed: the rule now explains itself, so a member added later is written to it instead of copied from whichever neighbour the author happened to look at. All six read-only members (CurrentBranchName, HeadSha, TagNames, GetCommits, GetRemoteTipSha, and GetRemoteTipSha's branch lookup) go through a fresh handle. One edge below.

4 (0ca47cf) — Verified that ComputeVersion() is genuinely independent: it builds its own VersioningService over Repo.RootPath with a fresh GitHeightCalculator, sharing no state with the graph under test. The comment above it is the part that will keep it that way.

5 (c69ba2f) — The test is stronger than what I sketched. Asserting ChangedFiles on the post-release commit means the bug's silent form — no post-release commit at all, because discovery matched nothing — fails on commits[0] being the release commit, before any content assertion runs. And since a failed release rolls the commits back in ServerRelease.DisposeAsync, the assertions carry the exit code implicitly, which is why _ = await is fine here and consistent with its neighbours.

Smaller notes

a (32144e9) — You are right and I was wrong to call it a wash. Directory.CreateDirectory adopting an existing directory is the whole point, and I filed it as a hygiene note when it was a security one: the mechanism whose job is to keep the machine's configuration out would have been the thing letting it in. Directory.CreateTempSubdirectory gives 0700 ownership on Linux, and the ProcessExit reasoning is sound — process exit really is the first moment the directory can go and the last at which our code runs. The IOException or UnauthorizedAccessException filter covers the realistic failures (DirectoryNotFoundException derives from IOException), and the leak that survives is the one nobody can do anything about anyway: a process that dies without running handlers at all.

b (9da55e7), c (3a61b80), d (98f3469), f (df23a47) — all verified. On d, your reason is better than mine: coverage was the symptom, and "nothing would have objected if the non-fatal case quietly became fatal" is what the test actually guards.

One correction on c, since you took my advice and it deserves the honest version: the old order was not as bad as I made it sound, and the new one is not strictly better. Before, a Remotes.Add failure left a dictionary entry for a remote that did not exist — but the path in it was a real directory, so Dispose cleaned it up. Now the same failure leaves the bare repository created at TempGitRepo.cs:154 in nobody's list, so it survives the test run. Both paths are unreachable (nothing adds a duplicate remote) and neither is worth another commit; I am recording it so the reorder is not read later as having closed something it did not.

e — the harness constructor — Agreed, don't do it. The scenario is "the harness is broken", which is loud and immediate; the env var is namespaced to these tests and is unconditionally re-set by the next one; and the temp directory is in the one place on the machine designed to be swept. A try/catch there would be paid by every reader of the constructor to make a broken-test-run's litter slightly tidier. Your read of the failure mode matches mine.

---

Two residual observations, neither asking for a commit

• The type-level rule at src/Buildvana.Core.Testing/TempGitRepo.cs:21-24 has a soft edge. "Members that change something" also read through the cached handle in order to change: CreateTag (line 181) applies the tag to _repository.Head.Tip, and Checkout, CheckoutDetached, Merge, and AddBareRemote resolve HEAD or a branch the same way. Nothing today calls any of them after the code under test has committed, so nothing is broken — but if the staleness that motivated the fresh handles is real, it reaches a CreateTag issued after a run, and the symptom would be a tag on the wrong commit. Half a sentence in the remarks ("…and members that change state read what they need through it, so they must not be called after the code under test has moved HEAD") would close it without any code change.
• WithPushCredentials is one knob for two properties (ReleaseHarness.cs:220-221), so the mixed case — username without secret, or the reverse — stays unexercised, and ReleaseCommand.cs:73 short-circuits before its second operand in the both-null case. Today that is a partial branch and nothing more. It gets interesting after the platform/host split the RecordingServerAdapter comment anticipates, because that is precisely when "a host with a username convention on a platform that provides no secret" becomes a state the code can be in. The fake already has the two properties separately; only the option collapses them.

Nothing here blocks. The PR reads as finished from where I sit.
· branch release-command-harness

[rdeago]

Two commits on top, tip 5daaae0. Gate is green: bv pack clean, inspectcode 0/0/0, 359 tests — 357 plus the two argument sets added below.

The soft edge in the TempGitRepo rule — 88e2f57

Taken, and by deletion rather than documentation. You are right that "members that change state read what they need through the kept handle" is the missing clause, but writing it down leaves a rule with an exception in it, and the exception is exactly where the stale read lands: a CreateTag after a run puts the tag on the wrong commit, which reads as a bug in the code under test. So the handle is gone. Every member opens one and disposes it, the field and its disposal are no longer there, and there is nothing left for the next member to be written against incorrectly. A handle per call costs nothing in a test helper.

WithPushCredentials collapsing two properties — 5daaae0

Taken, and the case is stronger than "it gets interesting after the split". It is interesting now: GitLabServerAdapter returns PushUsername => "oauth2" and PushPassword => null, so the mixed shape is what a GitLab release runs with today. The split will not invent that state; it will inherit it.

The option is now the two values themselves, and the warning test runs all three incomplete shapes. What that pins is the && at ReleaseCommand.cs:73: were it an ||, a username alone would count as a credential and libgit2 would be handed a null password. Under the old single knob, that flip changed nothing observable.

The correction on note c — recorded, no commit.

Agreed, and thanks for going back to it — the reorder traded one unreachable leak for another rather than closing anything. Making it total means a catch around Remotes.Add that deletes the directory it just created, i.e. dead code guarding a duplicate remote name that nothing adds. Not worth it; the honest version belongs in the thread rather than in the file.

No new review requested: nothing here touches production code, and you'd already called the PR finished.