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

src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubRepositoryUrls.cs

@@ -0,0 +1,84 @@
+// Copyright (C) Tenacom and Contributors. Licensed under the MIT license.
+// See the LICENSE file in the project root for full license information.
+
+using System;
+using System.IO;
+using CommunityToolkit.Diagnostics;
+
+namespace Buildvana.Tool.Services.ServerAdapters.Internal.GitHub;
+
+/// <summary>
+/// Builds the URLs of a GitHub repository and of the resources it contains.
+/// </summary>
+/// <remarks>
+/// <para>This type owns both ends of the same contract: how the repository URL is built, and how the URLs of
+/// resources are derived from it. Keeping them together is the point — deriving a resource URL elsewhere,
+/// by appending to the repository URL, is what previously produced links with no separator between the
+/// repository name and the first path segment.</para>
+/// <para>The repository URL deliberately carries no trailing slash, so it can be displayed and compared as
+/// the canonical URL of the repository. Callers must therefore not treat it as a base URL for
+/// <see cref="Uri(Uri,string)"/>, which would resolve a relative URL against its parent and drop the
+/// repository name.</para>
+/// </remarks>
+internal sealed class GitHubRepositoryUrls
+{
+    private readonly string _repository;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="GitHubRepositoryUrls"/> class.
+    /// </summary>
+    /// <param name="hostName">The host name of the GitHub instance, e.g. <c>github.com</c>.</param>
+    /// <param name="repositoryOwner">The owner of the repository.</param>
+    /// <param name="repositoryName">The name of the repository.</param>
+    public GitHubRepositoryUrls(string hostName, string repositoryOwner, string repositoryName)
+    {
+        Guard.IsNotNullOrEmpty(hostName);
+        Guard.IsNotNullOrEmpty(repositoryOwner);
+        Guard.IsNotNullOrEmpty(repositoryName);
+
+        _repository = $"https://{hostName}/{repositoryOwner}/{repositoryName}";
+        Repository = new Uri(_repository);
+    }
+
+    /// <summary>
+    /// Gets the URL of the repository, without a trailing slash.
+    /// </summary>
+    public Uri Repository { get; }
+
+    /// <summary>
+    /// Builds the URL of the release identified by a tag.
+    /// </summary>
+    /// <param name="version">The version string, which is also the tag name.</param>
+    /// <returns>The URL of the release.</returns>
+    public Uri ReleaseTag(string version)
+    {
+        Guard.IsNotNullOrEmpty(version);
+        return new Uri($"{_repository}/releases/tag/{version}");
+    }
+
+    /// <summary>
+    /// Builds the URL of a file in the repository, as of a given commit or reference.
+    /// </summary>
+    /// <param name="path">The path to the file, relative to the repository root.</param>
+    /// <param name="commitish">The SHA or reference to which the file belongs.</param>
+    /// <returns>The URL of the file.</returns>
+    public Uri File(string path, string commitish)
+    {
+        Guard.IsNotNullOrEmpty(path);
+        Guard.IsNotNullOrEmpty(commitish);
+        Guard.IsTrue(!Path.IsPathFullyQualified(path), nameof(path), "A path must be relative to be converted to a file URL.");
+
+        // Normalize to forward slashes for the URL, then reject paths that escape the repo.
+        // Every ".." segment must go, not just a leading one: Uri collapses parent segments as it parses,
+        // so enough of them anywhere in the path walk out of the repository and even out of the owner
+        // (".../blob/main/docs/../../../../../../etc/passwd" parses to "https://github.com/etc/passwd").
+        var remotePath = path.Replace('\\', '/');
+        var hasParentSegment = remotePath == ".."
+            || remotePath.StartsWith("../", StringComparison.Ordinal)
+            || remotePath.EndsWith("/..", StringComparison.Ordinal)
+            || remotePath.Contains("/../", StringComparison.Ordinal);
+        Guard.IsTrue(!hasParentSegment, nameof(path), "Only a path to a file in the repository can be converted to a file URL.");
+
+        return new Uri($"{_repository}/blob/{commitish}/{remotePath}");
+    }
+}

src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs

@@ -28,6 +28,7 @@ internal sealed class GitHubServerAdapter : ServerAdapter
     private readonly GitService _git;
 
     private readonly string _token;
+    private readonly GitHubRepositoryUrls _urls;
 
     private GitHubServerAdapter(IServiceProvider services)
     {
@@ -40,7 +41,7 @@ private GitHubServerAdapter(IServiceProvider services)
         HostName = originInfo.Host;
         RepositoryOwner = originInfo.PathSegments[0];
         RepositoryName = originInfo.PathSegments[1];
-        RepositoryUrl = new Uri($"https://{HostName}/{RepositoryOwner}/{RepositoryName}");
+        _urls = new GitHubRepositoryUrls(HostName, RepositoryOwner, RepositoryName);
         var tokenEnv = services.GetRequiredService<BuildvanaConfig>().GitHub?.TokenEnv is { Length: > 0 } e
             ? e
             : "GITHUB_TOKEN";
@@ -60,7 +61,7 @@ private GitHubServerAdapter(IServiceProvider services)
     public override string RepositoryName { get; }
 
     /// <inheritdoc/>
-    public override Uri RepositoryUrl { get; }
+    public override Uri RepositoryUrl => _urls.Repository;
 
     /// <inheritdoc/>
     /// <value>Always <see langword="false"/>.</value>
@@ -111,25 +112,10 @@ public override async Task<bool> IsPrivateRepositoryAsync()
     }
 
     /// <inheritdoc/>
-    public override Uri GetReleaseUrl(string version)
-    {
-        Guard.IsNotNullOrEmpty(version);
-        return new Uri($"{RepositoryUrl}/releases/tag/{version}");
-    }
+    public override Uri GetReleaseUrl(string version) => _urls.ReleaseTag(version);
 
     /// <inheritdoc/>
-    public override Uri GetFileUrl(string path, string commitish)
-    {
-        Guard.IsNotNullOrEmpty(path);
-        Guard.IsNotNullOrEmpty(commitish);
-        Guard.IsTrue(!Path.IsPathFullyQualified(path), "A path must be relative to be converted to a file URL.");
-
-        // Normalize to forward slashes for the URL, then reject paths that escape the repo.
-        var remotePath = path.Replace('\\', '/');
-        Guard.IsTrue(remotePath != ".." && !remotePath.StartsWith("../", StringComparison.Ordinal), "Only a path to a file in the repository can be converted to a file URL.");
-
-        return new Uri($"{RepositoryUrl}/blob/{commitish}/{remotePath}");
-    }
+    public override Uri GetFileUrl(string path, string commitish) => _urls.File(path, commitish);
 
     /// <inheritdoc/>
     public override async Task<ServerRelease> CreateReleaseAsync()

src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs

@@ -60,8 +60,11 @@ private protected ServerRelease(IServiceProvider services)
     /// <para>The first call creates an empty commit, refreshes version information from the new Git height,
     /// then amends the commit with the final version-bearing message and captures its SHA into
     /// <see cref="ReleaseCommitSha"/>. Subsequent calls are no-ops.</para>
-    /// <para><see cref="UpdateRepository"/> calls this implicitly; callers only need to invoke it directly
-    /// when they want to guarantee a release commit exists without staging any file.</para>
+    /// <para><see cref="UpdateRepository"/> calls this implicitly, because it amends the release commit and
+    /// builds its own message afterwards. <see cref="AddPostReleaseCommit"/> does not: it requires a release
+    /// commit to exist already, so that the version cannot move under a message its caller has formatted.</para>
+    /// <para>Call this directly to settle the version before anything reads it - notably before building, so
+    /// that artifacts carry the same version that will be tagged and published.</para>
     /// </remarks>
     public void EnsureReleaseCommit()
     {
@@ -139,8 +142,12 @@ public void UpdateRepository(params string[] files)
     /// been published to the feed yet).</param>
     /// <param name="files">The paths of the files to stage into the new commit.</param>
     /// <remarks>
-    /// If no release commit has been created yet, this method calls <see cref="EnsureReleaseCommit"/>
-    /// first, so the post-release commit always sits on top of a tagged release commit (possibly empty).
+    /// <para>A release commit must already exist: call <see cref="EnsureReleaseCommit"/> (directly or via
+    /// <see cref="UpdateRepository"/>) first.</para>
+    /// <para>This method deliberately does not create the release commit itself. Doing so would move the
+    /// Git height, and with it the version, in the middle of a call whose <paramref name="message"/> the
+    /// caller has already formatted - typically from that very version, which the message would then
+    /// misreport. Requiring the commit up front keeps the version settled before any caller reads it.</para>
     /// </remarks>
     public void AddPostReleaseCommit(string message, params string[] files)
     {
@@ -153,7 +160,10 @@ public void AddPostReleaseCommit(string message, params string[] files)
             ThrowHelper.ThrowInvalidOperationException("Internal error: cannot add a post-release commit when updates have already been pushed.");
         }
 
-        EnsureReleaseCommit();
+        if (!_repositoryUpdated)
+        {
+            ThrowHelper.ThrowInvalidOperationException("Internal error: cannot add a post-release commit before the release commit has been created.");
+        }
 
         _git.Stage(files);
         _reporter.Info("Committing post-release changed files...");

src/Buildvana.Tool/Subcommands/ReleaseCommand.cs

@@ -176,8 +176,9 @@ public async Task<int> ExecuteAsync(CancellationToken cancellationToken)
 
             // Create the release commit now, if none of the steps above had anything to commit.
             // The Git height, hence the version, must be final before anything is built: a release commit
-            // created later (for example by AddPostReleaseCommit) would bump the height after the fact,
-            // tagging and publishing a version one patch above the one the artifacts were built with.
+            // created later would bump the height after the fact, tagging and publishing a version one
+            // patch above the one the artifacts were built with. This call is also what makes the later
+            // AddPostReleaseCommit legal, as that method requires the release commit to exist already.
             release.EnsureReleaseCommit();
 
             // At this point we know what the actual published version will be.

tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs

@@ -0,0 +1,114 @@
+// Copyright (C) Tenacom and Contributors. Licensed under the MIT license.
+// See the LICENSE file in the project root for full license information.
+
+using Buildvana.Tool.Services.ServerAdapters.Internal.GitHub;
+
+internal sealed class GitHubRepositoryUrlsTests
+{
+    private static GitHubRepositoryUrls Urls => new("github.com", "Tenacom", "Buildvana");
+
+    [Test]
+    public async Task Repository_HasNoTrailingSlash()
+    {
+        await Assert.That(Urls.Repository.ToString()).IsEqualTo("https://github.com/Tenacom/Buildvana");
+    }
+
+    [Test]
+    public async Task ReleaseTag_SeparatesRepositoryNameFromFirstSegment()
+    {
+        // Regression: this used to come out as ".../Tenacom/Buildvanareleases/tag/2.1.2-preview".
+        var url = Urls.ReleaseTag("2.1.2-preview").ToString();
+        await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/releases/tag/2.1.2-preview");
+    }
+
+    [Test]
+    public async Task File_SeparatesRepositoryNameFromFirstSegment()
+    {
+        // Regression: this used to come out as ".../Tenacom/Buildvanablob/main/CHANGELOG.md".
+        var url = Urls.File("CHANGELOG.md", "main").ToString();
+        await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/main/CHANGELOG.md");
+    }
+
+    [Test]
+    public async Task File_AcceptsCommitShaAsCommitish()
+    {
+        var url = Urls.File("CHANGELOG.md", "7f7aebf19f12da3c8e6335b16cc7d5482b7b70a6").ToString();
+        await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/7f7aebf19f12da3c8e6335b16cc7d5482b7b70a6/CHANGELOG.md");
+    }
+
+    [Test]
+    public async Task File_NormalizesBackslashesToForwardSlashes()
+    {
+        var url = Urls.File(@"docs\ConstantsSyntax.md", "main").ToString();
+        await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/main/docs/ConstantsSyntax.md");
+    }
+
+    [Test]
+    public async Task Urls_HonorNonDefaultHostName()
+    {
+        var urls = new GitHubRepositoryUrls("github.example.com", "Contoso", "Widgets");
+        await Assert.That(urls.Repository.ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets");
+        await Assert.That(urls.ReleaseTag("1.0.0").ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets/releases/tag/1.0.0");
+        await Assert.That(urls.File("README.md", "main").ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets/blob/main/README.md");
+    }
+
+    [Test]
+    [Arguments("..")]
+    [Arguments("../outside.md")]
+    [Arguments(@"..\outside.md")]
+    [Arguments("docs/..")]
+    [Arguments("docs/nested/../ConstantsSyntax.md")]
+    [Arguments("docs/../../outside.md")]
+    [Arguments("docs/../../../../../../etc/passwd")]
+    public async Task File_RejectsPathWithParentSegment(string path)
+    {
+        // A parent segment anywhere is rejected, not just a leading one: Uri collapses them as it parses,
+        // so the last case above would otherwise resolve to "https://github.com/etc/passwd".
+        await Assert.That(() => Urls.File(path, "main")).Throws<ArgumentException>();
+    }
+
+    [Test]
+    [Arguments("..gitignore.md")]
+    [Arguments("docs/..hidden/file.md")]
+    public async Task File_AcceptsNameStartingWithTwoDots(string path)
+    {
+        // Only a whole ".." segment escapes; two dots at the start of a name are just a name.
+        var url = Urls.File(path, "main").ToString();
+        await Assert.That(url).IsEqualTo($"https://github.com/Tenacom/Buildvana/blob/main/{path}");
+    }
+
+    [Test]
+    public async Task File_RejectsFullyQualifiedPath()
+    {
+        // Built from a relative path so the test means the same thing on every platform.
+        var fullyQualified = Path.GetFullPath("CHANGELOG.md");
+        await Assert.That(() => Urls.File(fullyQualified, "main")).Throws<ArgumentException>();
+    }
+
+    [Test]
+    public async Task ReleaseTag_RejectsEmptyVersion()
+    {
+        await Assert.That(() => Urls.ReleaseTag(string.Empty)).Throws<ArgumentException>();
+    }
+
+    [Test]
+    public async Task File_RejectsEmptyPath()
+    {
+        await Assert.That(() => Urls.File(string.Empty, "main")).Throws<ArgumentException>();
+    }
+
+    [Test]
+    public async Task File_RejectsEmptyCommitish()
+    {
+        await Assert.That(() => Urls.File("CHANGELOG.md", string.Empty)).Throws<ArgumentException>();
+    }
+
+    [Test]
+    [Arguments("", "Tenacom", "Buildvana")]
+    [Arguments("github.com", "", "Buildvana")]
+    [Arguments("github.com", "Tenacom", "")]
+    public async Task Constructor_RejectsEmptyArguments(string hostName, string owner, string name)
+    {
+        await Assert.That(() => new GitHubRepositoryUrls(hostName, owner, name)).Throws<ArgumentException>();
+    }
+}