Stop reporting the version from a constructor

[rdeago]

Description

bv release printed the same version line up to twenty-two times per run — four in a row at the head of each pipeline stage, plus two from bv itself.

The cause is that VersioningService reported the computed version from its constructor. That made the message count a function of how many instances got built, which in the SDK means one per MSBuild node, since the ComputeVersion cache is per process. Four lines on a 4-vCPU CI runner; twenty-nine on a 32-core desktop. Nothing about the repository determines it.

The class had a second problem behind the first: it was named a service but behaved as a snapshot. It computed everything in its constructor, VersionService.Update() discarded the instance and built another, and the SDK task copied its ten properties into an identically shaped record.

So it is now the pair its own project already establishes with GitHeightCalculator → GitHeightResult:

• VersionInfo — the immutable snapshot, deriving the five version strings from the spec, the height, and the settings. No I/O, and an internal constructor: VersionCalculator is the only thing that builds one, so the one place that has to get the parameters consistent is the only place that can pass them.
• VersionCalculator — the lookups (version file, prerelease tag validation, height walk), returning a VersionInfo. Stateless, so calculating again after a commit is the explicit way to get a second answer.

IReporter is out of Buildvana.Core.Versioning altogether. CachedVersion collapses to (Fingerprint, VersionInfo) and stays inside the caching partial. Reporting now happens in exactly two deliberate places:

• the SDK task, at detail level (MessageImportance.Low), invisible below -v detailed, where per-node repetition is a diagnostic rather than noise — and where ComputeVersionTests reads it as evidence of a cache hit;
• bv release, once, just after the tag check — the last thing that can refuse this particular version — where naming it records a decision rather than a guess.

bv build/pack say nothing about the version, as before.

One consequence worth stating plainly: a release that fails before that point no longer names a version at the default verbosity. What it used to name was a guess — VersionService is resolved at the top of the command, before the version spec change and before the release commit, so the old line reported a height at least one below the version that would be published, and named a different version line entirely on a --bump run. -v detailed still carries the SDK task's line for anyone who wants the provisional number.

Additional changes

None.

Checklist

• Build is clean: 0 errors, 0 warnings
• Sanity check green (bv pack + ReSharper at WARNING and above, both silent)
• Tests pass: Core.Versioning 102, Sdk.Tasks 41, Tool 399 — including Release_RecordsTheVersionItPublishes, which asserts the line appears exactly once
• No changelog entry — the versioning feature is still under Unreleased changes and never documented the line

[rdeago]

@claude please review this PR.

[claude]

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

---

Review of #349 — Stop reporting the version from a constructor

• Read the diff (7891618...HEAD, 11 files)
• Review the VersionInfo / VersionCalculator split
• Review the SDK task caching and reporting change
• Review bv release reporting and the new tests
• Check for leftover references and log consumers

Static review only — I did not build here (self-hosting needs the preview feed); I'm relying on the gate you already ran.

---

The split is right

VersioningService really was a snapshot wearing a service's name, and the pair now matches GitHeightCalculator → GitHeightResult exactly. Things I specifically checked and found preserved:

• Error precedence. VersionCalculator.Calculate still validates the prerelease tag before the height walk (VersionCalculator.cs:61-63), so a repo that is both tagless and un-walkable reports the same message it reported before.
• The MSBuild empty-string convention moved to the boundary where it belongs: CommitId ?? string.Empty is now in ComputeVersion.cs:63, and VersionInfo.CommitId stays honestly nullable.
• Detail → MessageImportance.Low is confirmed at TaskLoggingHelperReporter.cs:94-97, so "invisible below -v detailed" is accurate.
• Nothing else depended on the old line: no remaining reference to VersioningService anywhere in the tree, no log scraping in .github/, no mention in docs/.
• The comment at ComputeVersion-caching.cs:39-44 is the part that will stop someone "fixing" that line back into a Notice in six months. Worth the eight lines.
• In the new test, computing the oracle after the run is the right choice, and it is sound here: with Dogfood = false and no hook there are no post-release updates, so no post-release commit moves the height off the release commit.

---

Findings

1. The regression test does not test for the regression — tests/Buildvana.Tool.Tests/ReleaseCommandReportingTests.cs:28

The bug was not "the version is never stated", it was "the version is stated twenty-two times". Contains passes identically for one occurrence and for twenty-two, so the one new test on the tool side does not pin the thing the PR is named after. A count assertion does:

await Assert.That(harness.Notices).Contains($"Releasing version {harness.ComputeVersion()}.");
await Assert.That(harness.Notices.Count(x => x.StartsWith("Releasing version", StringComparison.Ordinal))).IsEqualTo(1);

The ComputeVersionTests side already counts (IsEqualTo(1) / IsEqualTo(0)), which is exactly why that side would catch a relapse and this one would not.
Fix this →

2. VersionInfo's constructor no longer enforces the invariant its own output depends on — src/Buildvana.Core.Versioning/VersionInfo.cs:36

spec.Prerelease and prereleaseTag used to arrive from the same computation, so they could not disagree. Now they are two independent parameters of a public constructor, and disagreement produces a snapshot that lies:

• new VersionInfo(new VersionSpec(2, 3, true), 1, sha, isPublicRelease: false, prereleaseTag: null, …) → IsPrerelease is true, SemVer is 2.3.1, and ComputeInformationalVersion therefore picks the . separator (VersionInfo.cs:126) for a version with no prerelease part: 2.3.1.gdeadbeef01, which is not a semantic version.
• The mirror case, a tag on a stable spec, gives 2.3.1-preview-gdeadbeef01.

Not a live bug — VersionCalculator.Calculate is the only producer, and it gets the pairing right — so this is about whether a type that is now public and reachable from three assemblies should be constructible in a state it cannot represent. Two cheap lines close it:

var tagMatchesSpec = spec.Prerelease == !string.IsNullOrEmpty(prereleaseTag);
Guard.IsTrue(tagMatchesSpec, nameof(prereleaseTag));

Your call: it is defensive code on a path nothing takes today.
Fix this →

3. A release that fails before the artifact pass no longer names its version — src/Buildvana.Tool/Subcommands/ReleaseCommand.cs:204

The notice sits after pipeline.RunThroughAsync(BuildStep.Test, …). Before this PR, the constructor line named the version at the very start of the run, so a release that died in the verification pass still told you which version it was trying to cut; now that information is only at -v detailed, from the SDK task. The consistency-check failures all name the version themselves, and a test failure is rarely version-specific, so this may well be the trade you want — but it is a real change to what a failed CI release log contains, and it is not called out in the PR description.

4. Nit — the line precedes the last thing that can still refuse the release — ReleaseCommand.cs:204 vs :209

git.TagExists throws immediately after, so Releasing version 2.3.1. can be followed by Tag '2.3.1' already exists. Moving the notice five lines down, below the tag check, makes the comment's own claim ("the version that gets published") literally true.

5. Nit — .Version at the call site — ComputeVersion.cs:55

GetOrComputeVersion(…).Version reaches through a caching detail. Having GetOrComputeVersion return VersionInfo would keep CachedVersion entirely inside ComputeVersion-caching.cs, where it belongs.

---

Nothing here blocks: findings 3–5 are judgment calls, 2 is latent, and only 1 is something I'd want in before merge.
· branch fix/redundant-version-notices

[codecov]

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.65%. Comparing base (7891618) to head (91d7b48).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
...uildvana.Sdk.Tasks/Tasks/ComputeVersion-caching.cs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   74.63%   74.65%   +0.02%     
==========================================
  Files         164      165       +1     
  Lines        4596     4589       -7     
  Branches      788      788              
==========================================
- Hits         3430     3426       -4     
+ Misses       1015     1014       -1     
+ Partials      151      149       -2     

☔ 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. Seven commits; the gate (bv pack + ReSharper at WARNING and above) is green on the final state.

1 — the regression test now tests the regression. 7db8c84, taken as written. The bug was twenty-two lines and Contains cannot tell one from twenty-two.

await Assert.That(harness.Notices.Count(x => x.StartsWith("Releasing version", StringComparison.Ordinal))).IsEqualTo(1);

2 — fixed, though not with the guard. 3d0c6c6, b1cc161.

The invalid output is real, but a guard throws on a state that a smaller change makes unrepresentable. ComputeInformationalVersion chose the separator from spec.Prerelease, when the question it actually asks is whether the string it appends to already carries a prerelease part — and that is decided two lines above, by prereleaseTag. Reading the answer off the tag makes both of your disagreement cases produce valid output instead of an exception, and leaves behind no defensive branch that no test can honestly reach.

@rdeago's point closes it from the other side: the constructor is now internal. VersionCalculator.Calculate is the only thing that builds a VersionInfo, and it lives in the same assembly, so the one place that has to get the pairing right is now the only place that can pass it at all.

One correction to the premise, since it is what made narrowing free: Buildvana.Core.Versioning is a Core-tier project and is not packaged, and it tracks no public API file. "Public and reachable from three assemblies" means our own three.

3 — no code change; now stated in the description. The observation is right, but what was lost was a guess rather than information. VersionService is resolved at ReleaseCommand.cs:50, before the version spec change (:94) and before EnsureReleaseCommit (:195), so the old line reported a height at least one below what would be published, and named a different version line entirely on a --bump run. Restoring an early line means announcing the version twice, which is the bug this PR is about. Disclosure it does deserve, so the description now says that a release failing before the tag check names no version at default verbosity, and points at -v detailed.

4 — taken. 2632cfe. Announcing Releasing version 2.3.1. and then refusing to release it was silly. Below the tag check the comment's claim holds: what can still fail from there is a build, not a verdict on the version.

5 — taken. 6e8e468. GetOrComputeVersion returns VersionInfo, and CachedVersion no longer leaves the file that caches.

Two more, neither of them yours:

• c4701d9 — Codecov's single partial branch was the ?? string.Empty at the MSBuild boundary: nothing in the tree ran the calculation against a repository with no commits. Now something does, which also pins the empty-string convention you flagged as correctly placed.
• f9b1ce5 — three lines in ComputeVersionTests ran past 140 characters, two of them lines this branch had changed. The repeated Count(...Contains(...)) that made them long is a named helper now, and the configuration document moved into a local, since as an argument a multi-line raw literal trips SA1118.

@claude, another look when you have a moment.

[claude]

Claude finished @rdeago's task in 2m 45s —— View job

---

Re-review of #349 — seven follow-up commits

• Read the seven follow-up commits (f83dd8a..HEAD)
• Verify finding 1 — the count assertion
• Verify finding 2 — separator derived from the tag, internal constructor
• Verify findings 4 and 5 — notice placement, CachedVersion scope
• Check the two self-initiated commits (c4701d9, f9b1ce5)
• Look for regressions the follow-ups may have introduced

All five findings are addressed. Nothing new blocks; one micro-nit at the end, take it or leave it.

---

Verification, finding by finding

1 — 7db8c84. ReleaseCommandReportingTests.cs:29 now counts. The pair is the right shape: Contains pins the wording and the version, Count(...).IsEqualTo(1) pins the thing the PR is named after. A relapse to per-instance reporting fails this test on both sides now.

2 — 3d0c6c6 + b1cc161, and your fix is better than mine. I proposed a guard; you removed the possibility of disagreement instead. SemVer is built from prereleaseTag is null (VersionInfo.cs:50) and the separator now from prereleaseTag is not null (:53) — the same predicate on the same value, one line apart, so the two cannot drift no matter what the spec says. A guard would have thrown on a state that this makes unrepresentable, and left a branch no honest test could reach. The comment at :119-122 states the rule in terms of the string being appended to, which is the durable version of the reasoning.

Both separator branches are covered, as it happens: Calculate_PrereleaseNonPublic_AppendsCommitIdToInformationalVersion (.) and Calculate_StableNonPublic_AppendsCommitIdAsPrerelease (-).

The internal constructor closes it from the other side, and the empty-tag case with it: GetPrereleaseTag (VersionCalculator.cs:70-72) rejects an empty tag before it can reach the constructor, and Calculate is now the only caller in the only assembly that has one. Confirmed no other new VersionInfo(...) in the tree, and Buildvana.Core.Versioning grants no InternalsVisibleTo — VersionCalculatorTests goes through Calculate() for every case, so the narrowing cost the tests nothing.

Your correction on the premise is right, and it does change the calculus: no PublicAPI.*.txt in src/Buildvana.Core.Versioning/, and the project is unpackaged, so "public" was three of our own assemblies rather than a contract with anyone. I overstated the exposure; narrowing was cheap precisely because there wasn't any.

3 — description, no code. Agreed, and the point about --bump settles it: a line printed at ReleaseCommand.cs:50 names the version of a repository that the run is about to change. Restoring it means either announcing twice or announcing a number that is wrong on exactly the runs where it matters most. The PR body now states the consequence and points at -v detailed, which is what the observation was actually worth.

4 — 2632cfe. ReleaseCommand.cs:204 throws, :210 announces. The comment's claim now holds literally: past the tag check nothing left can refuse this version, only fail to build it.

5 — 6e8e468. GetOrComputeVersion returns VersionInfo at all three exits (ComputeVersion-caching.cs:26,31,36), ComputeVersion.cs:55 reads it directly, and CachedVersion is now referenced only inside the caching partial and its own file. That is the whole point of the split: the caller doesn't know the cache exists.

The two you added

• c4701d9 — good catch on what the partial branch actually was. Verified it is a real path rather than a coverage gesture: GitHeightCalculator.Calculate returns new GitHeightResult(0, string.Empty, null) for an unborn HEAD (GitHeightCalculator.cs:83-86), so CommitId ?? string.Empty at ComputeVersion.cs:63 is genuinely exercised, and the assertions on SemVer/InformationalVersion also pin that a null commit ID takes the early return in ComputeInformationalVersion rather than the separator logic. The test would fail on a null, not silently pass.
• f9b1ce5 — CountVersionMessages reads better than the inlined Count(...Contains(...)) it replaces, and hoisting the config into a local for SA1118 is the right trade. No line over 140 in either file.

One nit

VersionInfo.cs:107-108 — the summary of InformationalVersion says the commit ID is "appended to the prerelease part", which is true for a prerelease and not for a stable version, where the ID becomes the prerelease part. 3d0c6c6 is exactly about that distinction, so the doc is now the one place stating it loosely:

/// Gets the informational version: <see cref="SemVer"/>, plus a <c>g</c>-prefixed short commit ID
/// appended to the prerelease part — becoming it, when the version has none — when the build is not
/// a public release.

Pure prose; nothing depends on it.

---

Ready to merge from my side.
· branch fix/redundant-version-notices