Cover GitHub URL composition with tests, and make release-commit ordering a precondition

[rdeago]

Follow-up to the two fixes pushed straight to main after the 2.1.2-preview release went out tagged as 2.1.3-preview. Those stopped the bleeding; this closes the two holes that let it happen.

Give GitHub URL composition an owner, and tests

The missing separator fixed in 6bba582 (.../Buildvanareleases/tag/1.1.10) was possible because nothing owned the contract: the repository URL was built in the adapter's constructor without a trailing slash, and two unrelated methods appended path segments to it. The two ends could drift, and did, for three releases.

GitHubRepositoryUrls now owns both ends — it takes the host, owner and name, builds the repository URL, and derives the release and file URLs from it. GitHubServerAdapter holds one and delegates. It needs no service provider, no Git origin and no token, which is precisely why the methods it replaces had no tests; there are now 22 cases covering both regressions, the enterprise-host case, normalization and the guards.

Two further bugs fell out of writing those tests:

• Guard.IsTrue takes the parameter name as its second argument, not a message, so both guards were reporting Parameter "A path must be relative to be converted to a file URL." must be true, was false. They now use the three-argument overload.
• The escape check only rejected a leading .., but Uri collapses parent segments as it parses, so enough of them anywhere in the path walked out of the repository and even out of the owner: docs/../../../../../../etc/passwd resolved to https://github.com/etc/passwd. Any .. segment is now rejected, while a name merely starting with two dots is still accepted.

The second one is only reachable from Buildvana's own code, which passes a literal CHANGELOG.md, so it was never exploitable — but it is exactly the class of mistake the guard exists to catch.

Require the release commit before a post-release commit

AddPostReleaseCommit called EnsureReleaseCommit on its caller's behalf, so it could move the Git height — and with it the version — in the middle of a call whose message the caller had already formatted from that same version. That is how the self-reference commit came to announce 2.1.2-preview while the release it sat on was tagged 2.1.3-preview.

_version.Update() is called in exactly one place in the codebase, inside EnsureReleaseCommit, so the version can only move at that single moment. Requiring the release commit up front removes the hazard outright rather than working around it with a deferred message factory: once the commit exists, nothing a caller invokes can change the version underneath it.

UpdateRepository keeps creating the commit implicitly, and the asymmetry is deliberate: it amends the release commit, so it owns it, and builds its message internally once the version has settled. A post-release commit merely sits on top of that commit, so it depends on it.

No functional change today — ReleaseCommand already settles the release commit before packing, which is what made that call legal in the first place.

Notes

No changelog entry: every type involved is internal, and the user-visible URLs were already covered by the entry for 6bba582.

Verified with dotnet bv pack (314 tests, 0 warnings) and inspectcode --severity=WARNING (0 results).

[rdeago]

@claude please review this PR.

[claude]

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

---

Review

Tasks

• Gather context (fetch diff, read changed files)
• Understand the request (code review)
• Review GitHubRepositoryUrls.cs and its tests
• Review GitHubServerAdapter.cs changes
• Review ServerRelease.cs (release-commit ordering precondition)
• Review ReleaseCommand.cs changes
• Post final review feedback

This is a tightly-scoped, well-documented fix. I didn't find any correctness issues. Details below.

GitHubRepositoryUrls.cs (new)

• The class does exactly what the PR description claims: it's the single owner of the repository URL and everything derived from it, so the two ends can no longer drift the way they did across 6bba582's three broken releases.
• I traced the escape check by hand against all 22 test cases in GitHubRepositoryUrlsTests.cs, including the four-condition boolean at GitHubRepositoryUrls.cs:76-79. It correctly rejects a ".." segment anywhere (leading, trailing, or internal, pre- or post- backslash normalization) while still accepting names that merely start with two dots ("..gitignore.md"). I couldn't find a bypass.
• Guard.IsTrue(..., nameof(path), "...") at lines 69 and 80 correctly uses the 3-argument overload, fixing the mislabeled-message bug described in the PR body.
• Minor/optional nit, not a blocker: the 4-clause boolean (== "..", StartsWith("../"), EndsWith("/.."), Contains("/../")) is correct but could be expressed more directly as remotePath.Split('/').Contains(".."). Current form is already well-commented, so I wouldn't hold up the PR for this.

GitHubServerAdapter.cs

• Clean delegation to _urls; RepositoryUrl becomes a computed property (=> _urls.Repository) instead of an init-time field, with no behavior change. GetReleaseUrl/GetFileUrl are now one-liners. Confirmed the only other RepositoryUrl in the repo (Common.props' MSBuild property) is unrelated — no naming collision risk.

ServerRelease.cs / ReleaseCommand.cs

• Verified the "no functional change today" claim directly: AddPostReleaseCommit has exactly one call site in the whole codebase (ReleaseCommand.cs:231), and it's already unconditionally preceded by release.EnsureReleaseCommit() at ReleaseCommand.cs:182. So the new precondition guard (ServerRelease.cs:163-166) can't currently throw in practice — it just closes off the hazard for future callers, exactly as described.
• The guard's error message style ("Internal error: cannot add a post-release commit before...") matches the existing ThrowInvalidOperationException messages elsewhere in the file.
• Doc comments on EnsureReleaseCommit and AddPostReleaseCommit were updated consistently to reflect the new contract.
• Optional observation: there's no unit test exercising the new guard directly (e.g., asserting AddPostReleaseCommit throws before EnsureReleaseCommit/UpdateRepository), but ServerRelease has no existing test harness at all (it needs a real/fake GitService), so this is consistent with existing coverage rather than a regression. Not blocking.

Not independently re-verified

I did static review only (read the diff and traced logic/tests by hand); I didn't run dotnet bv pack or inspectcode myself in this environment. The PR body states both were run clean (314 tests, 0 warnings / 0 inspectcode results), and my trace of the URL logic against the added tests is consistent with that.

No blocking issues found. Nice, focused follow-up — the URL ownership fix in particular closes a real class of bug (not just the two symptoms already patched).