From fbec587442f0a67b1b880dcb5f58f0dcf4053407 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 05:29:40 +0000 Subject: [PATCH 1/3] test: let the fake handler 404 an unexpected path [patch] A handler that returns the same scripted body for every path lets a wrong route run the whole success path and look correct, so a route assertion made afterwards on a recorded URI is not load-bearing. RespondToPath makes the route a condition of getting the scripted response at all and answers anything else the way the host would. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TU2gC1LwTwyMin2XxYxg2m --- .../Fakes/FakeHttpMessageHandler.cs | 46 ++++++++++++++++++- .../Fakes/FakeHttpMessageHandlerTests.cs | 27 +++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs index 0fe1de6..82766a6 100644 --- a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs +++ b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs @@ -66,7 +66,34 @@ internal sealed class FakeHttpMessageHandler : HttpMessageHandler /// The same handler, to allow chaining. public FakeHttpMessageHandler Respond(HttpStatusCode status, string body, params (string Name, string Value)[] headers) { - _responses.Enqueue(new QueuedResponse(status, body, headers)); + _responses.Enqueue(new QueuedResponse(status, body, headers, ExpectedPath: null)); + _totalQueued++; + + return this; + } + + /// + /// Queues the next response, to be returned only if the request that arrives asks for + /// ; any other path is answered 404. + /// + /// + /// A route assertion the code under test has to satisfy to get its scripted response at all, + /// rather than one made afterwards on a recorded URI. A test that only asserts the URI still + /// receives the success body it scripted, so everything downstream of the route — deserialization, + /// mapping, the returned model — is exercised against a request the host would have refused. That + /// is how a wrong route survived here: a test asserted a URI addressing a repository by an id + /// GitHub's name slot cannot accept, and the always-succeeding handler made the whole method look + /// correct around it. Answering an unexpected path the way the host would makes the wrong route + /// fail where it actually fails. + /// + /// The the request must carry, compared exactly. + /// The status code the response carries when the path matches. + /// The response body, sent as UTF-8 text, when the path matches. + /// Header name/value pairs to attach, routed as routes them. + /// The same handler, to allow chaining. + public FakeHttpMessageHandler RespondToPath(string expectedPath, HttpStatusCode status, string body, params (string Name, string Value)[] headers) + { + _responses.Enqueue(new QueuedResponse(status, body, headers, expectedPath)); _totalQueued++; return this; @@ -115,6 +142,21 @@ protected override async Task SendAsync(HttpRequestMessage $"{_totalQueued} queued, {_requests.Count} arrived."); } + // The scripted response is conditional on the route when the test made it so, and the + // condition is checked after the dequeue rather than before it: a request that misses stays + // one request, consuming its own queued response, so a following request still gets the + // response the test queued for it rather than this one. + if (queued.ExpectedPath is string expectedPath && !string.Equals(request.RequestUri!.AbsolutePath, expectedPath, StringComparison.Ordinal)) + { + return new HttpResponseMessage(HttpStatusCode.NotFound) + { + Content = new StringContent( + $"{{\"message\":\"Not Found\",\"expectedPath\":\"{expectedPath}\",\"requestedPath\":\"{request.RequestUri.AbsolutePath}\"}}", + Encoding.UTF8, + "application/json"), + }; + } + HttpResponseMessage response = new(queued.Status) { Content = new StringContent(queued.Body, Encoding.UTF8), @@ -163,7 +205,7 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private readonly record struct QueuedResponse(HttpStatusCode Status, string Body, (string Name, string Value)[] Headers); + private readonly record struct QueuedResponse(HttpStatusCode Status, string Body, (string Name, string Value)[] Headers, string? ExpectedPath); /// A single request this handler received, captured in full. /// The HTTP method used. diff --git a/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs b/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs index 98cb02e..505bd48 100644 --- a/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs +++ b/GitIntegration.Test/Fakes/FakeHttpMessageHandlerTests.cs @@ -87,6 +87,33 @@ public async Task NamesHowManyResponsesWereQueuedAndHowManyRequestsArrivedAsync( StringAssert.Contains(exception.Message, "2 arrived"); } + [TestMethod] + public async Task ReturnsAPathScopedResponseOnlyForThatPathAsync() + { + // The route has to be a condition of getting the scripted response, not merely something a + // test inspects afterwards: a handler that answers every path alike lets a wrong route run the + // whole success path and look correct, which is how a wrong route shipped here once. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]") + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]"); + using HttpClient client = new(handler); + + using HttpResponseMessage matched = await client.GetAsync(new Uri("https://example.invalid/repos/contoso/my-repo/pulls?state=open"), TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + using HttpResponseMessage missed = await client.GetAsync(new Uri("https://example.invalid/repos/contoso/90000001/pulls"), TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + // The query string is no part of the comparison — a route is the path. + Assert.AreEqual(HttpStatusCode.OK, matched.StatusCode); + + Assert.AreEqual(HttpStatusCode.NotFound, missed.StatusCode); + StringAssert.Contains( + await missed.Content.ReadAsStringAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false), + "/repos/contoso/90000001/pulls"); + + // A missed request still consumed its own queued response rather than the next one's, so a + // following request is answered by what the test queued for it. + Assert.AreEqual(2, handler.Requests.Count); + } + [TestMethod] public async Task AttachesResponseHeadersWhenGivenAsync() { From b5360d236f6e8105827679c8877d4c5a081a1450 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 05:29:50 +0000 Subject: [PATCH 2/3] fix: address a GitHub repository by name, and an id on GitHub's id route [patch] ToRepositoryIdentifier preferred HostRepositoryId for every provider, which is what Azure DevOps's documented {repositoryId} schema asks for but is wrong for GitHub: its {repo} slot in /repos/{owner}/{repo} takes a repository name, and the id-addressed form is the separate /repositories/{id} route. GitHubProvider fills HostRepositoryId with GitHub's numeric id, so GetPullRequestsAsync (GitRepository) and CreatePullRequest(GitRepository) issued GET /repos/{owner}/{id}/pulls and answered 404 for a repository just enumerated from the same provider - the overload IGitHostingProvider documents as the one to prefer. The preference is now per-provider, and the chosen form travels with the value on GitRepositoryAddress, so a repository carrying only an id is addressed on GitHub's id route through Octokit's long repositoryId overloads rather than by dropping an id into the name slot. Azure DevOps has one slot for both forms and keeps preferring the id. The test that should have caught this asserted the id-addressed URL against a handler that returned [] for every path; it now asserts the name-addressed one through a path-scoped response, alongside the id-route and create-route cases. Fixes #101 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TU2gC1LwTwyMin2XxYxg2m --- .../Hosting/GitHubProviderTests.cs | 78 +++++++++- .../Hosting/GitProviderTests.cs | 9 +- .../GitPullRequestCreateBuilderTests.cs | 11 +- GitIntegration/GitHubProvider.cs | 66 ++++++++- GitIntegration/GitProvider.cs | 140 +++++++++++++----- GitIntegration/Hosting/AzureDevOpsProvider.cs | 23 ++- 6 files changed, 267 insertions(+), 60 deletions(-) diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index fcb5b35..812f7bc 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -174,13 +174,15 @@ public async Task ThrowsAHostingFailureWhenAReportedNameIsNotOneThisLibraryCanRe } [TestMethod] - public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() + public async Task AddressesARepositoryByNameWhenListingPullRequestsAsync() { - // The point of carrying the id: it has to reach the request path. GitHub happens to accept a - // name in this position, but both providers answer the same question the same way under one - // interface, so both prefer the host's own identifier when one is known. + // GET /repos/{owner}/{repo}/pulls takes a repository NAME in its {repo} slot — the id-addressed + // form is the separate /repositories/{id}/pulls. This provider used to send GitHub's numeric + // HostRepositoryId here, which addresses no repository at all and answers 404 for one that was + // just enumerated from the same provider. The repository below carries both forms, so this is + // the case where the preference itself decides, and the name has to win. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; GitRepository repository = new() @@ -191,7 +193,71 @@ public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - StringAssert.Contains(handler.Requests[0].Uri.AbsoluteUri, "/repos/contoso/90000001/pulls"); + Assert.AreEqual("/repos/contoso/my-repo/pulls", handler.Requests[0].Uri.AbsolutePath); + } + + [TestMethod] + public async Task AddressesARepositoryByItsHostIdOnTheIdRouteWhenNoNameIsKnownAsync() + { + // Preferring the name does not discard the id. A repository carrying only a HostRepositoryId — + // one a caller built by hand, since GetRepositoriesAsync always reports a name — is addressed + // on GitHub's id-addressed route rather than by dropping its id into the name slot. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repositories/90000001/pulls", HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() { HostRepositoryId = "90000001".As() }; + + _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual("/repositories/90000001/pulls", handler.Requests[0].Uri.AbsolutePath); + } + + [TestMethod] + public async Task ReportsAHostRepositoryIdGitHubCannotBeAddressedByAsync() + { + // GitHostRepositoryId is unvalidated, because Azure DevOps's is a uuid and GitHub's a whole + // number, so the semantic type can enforce neither. A hand-built repository carrying a + // GitHub-impossible id is a caller's argument rather than anything a host reported — nothing is + // sent, so there is no response for a GitHostingException to describe. + using FakeHttpMessageHandler handler = new(); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() { HostRepositoryId = "not-a-number".As() }; + + ArgumentException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + StringAssert.Contains(exception.Message, "whole numbers"); + Assert.AreEqual(0, handler.Requests.Count); + } + + [TestMethod] + public async Task AddressesARepositoryByNameWhenCreatingAPullRequestAsync() + { + // CreatePullRequest(GitRepository) reaches GitHub's routes the same way GetPullRequestsAsync + // does, and carried the identical bug: Octokit's Create(owner, name, ...) builds + // POST /repos/{owner}/{repo}/pulls, so a numeric id in that slot 404s just as it does on the + // listing route. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .RespondToPath("/repos/contoso/my-repo/pulls", HttpStatusCode.Created, Fixture("github-pullrequest-created.json"), ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitRepository repository = new() + { + Name = "my-repo".As(), + HostRepositoryId = "90000001".As(), + }; + + _ = await provider.CreatePullRequest(repository) + .From("example-branch-1".As()) + .Into("main".As()) + .Titled("A title".As()) + .ExecuteAsync(TestContext.CancellationTokenSource.Token) + .ConfigureAwait(false); + + Assert.AreEqual("/repos/contoso/my-repo/pulls", handler.Requests[0].Uri.AbsolutePath); } [TestMethod] diff --git a/GitIntegration.Test/Hosting/GitProviderTests.cs b/GitIntegration.Test/Hosting/GitProviderTests.cs index 6bac4d2..ecfc603 100644 --- a/GitIntegration.Test/Hosting/GitProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitProviderTests.cs @@ -215,10 +215,15 @@ private sealed class TestProvider : GitProvider public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by these tests."); - internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => + // Arbitrary, because this provider issues no request for the choice to matter. The member is + // abstract so that each real provider has to state its own host's answer rather than inherit + // one that is wrong for it — GitHub and Azure DevOps genuinely differ. + private protected override bool PrefersHostRepositoryId => false; + + internal override Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); - internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) => + internal override Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); public HostingCredential CallResolveCredential() => ResolveCredential(); diff --git a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs index 296601d..0a9aaa2 100644 --- a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs +++ b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs @@ -192,12 +192,17 @@ private sealed class RecordingProvider : GitProvider public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by the routing test."); - internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => + // Arbitrary, because this provider issues no request for the choice to matter. The member is + // abstract so that each real provider has to state its own host's answer rather than inherit + // one that is wrong for it — GitHub and Azure DevOps genuinely differ. + private protected override bool PrefersHostRepositoryId => false; + + internal override Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by the routing test."); - internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Repository = repositoryIdentifier; + Repository = repositoryAddress.Value; Specification = specification; return Task.FromResult(CannedPullRequest); } diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index 36784ff..a7d21f9 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -45,6 +45,24 @@ public sealed class GitHubProvider : GitProvider /// private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// + /// + /// , because GitHub's repository-addressed routes are two routes rather + /// than one. GET /repos/{owner}/{repo}/pulls takes a repository name in its + /// {repo} slot; the id-addressed form is the separate GET /repositories/{id}/pulls. + /// A numeric id in the name slot addresses no repository at all and answers 404 — which + /// this provider surfaces as , indistinguishable from a + /// repository that really is gone. Azure DevOps, whose one {repositoryId} slot is + /// documented as taking the id, answers instead. + /// + /// Preferring the name does not discard the id: a repository carrying only a + /// still arrives here as one, and + /// and answer it + /// on the id-addressed route through Octokit's long repositoryId overloads. + /// + /// + private protected override bool PrefersHostRepositoryId => false; + /// /// /// Adds to the interface's remarks rather than restating them. @@ -84,9 +102,9 @@ public override async Task> GetRepositoriesAsync(Ca } /// - internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) + internal override async Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + Ensure.NotNull(repositoryAddress.Value); cancellationToken.ThrowIfCancellationRequested(); (GitHubClient client, IDisposable createdTransport) = CreateClient(); @@ -100,8 +118,13 @@ internal override async Task> GetPullRequestsCoreA // exists, so a future validating change to PullRequestRequest can never leak it. PullRequestRequest request = new() { State = ItemStateFilter.Open }; - IReadOnlyList pullRequests = await client.PullRequest - .GetAllForRepository(Owner.WeakString, repositoryIdentifier, request) + // Two routes, chosen by which form the address carries rather than by what the value looks + // like: the name overload builds /repos/{owner}/{repo}/pulls, the long overload builds + // /repositories/{id}/pulls. Sending either form down the other's route is what this + // provider used to do, and GitHub answers it with a 404 rather than a diagnosable failure. + IReadOnlyList pullRequests = await (repositoryAddress.IsHostRepositoryId + ? client.PullRequest.GetAllForRepository(ToOctokitRepositoryId(repositoryAddress), request) + : client.PullRequest.GetAllForRepository(Owner.WeakString, repositoryAddress.Value, request)) .ConfigureAwait(false); return [.. pullRequests.Select(ToGitPullRequest)]; @@ -113,9 +136,9 @@ internal override async Task> GetPullRequestsCoreA } /// - internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + Ensure.NotNull(repositoryAddress.Value); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); @@ -134,8 +157,10 @@ internal override async Task CreatePullRequestCoreAsync(string r Draft = specification.IsDraft, }; - PullRequest created = await client.PullRequest - .Create(Owner.WeakString, repositoryIdentifier, newPullRequest) + // The same two routes GetPullRequestsCoreAsync chooses between, for the same reason. + PullRequest created = await (repositoryAddress.IsHostRepositoryId + ? client.PullRequest.Create(ToOctokitRepositoryId(repositoryAddress), newPullRequest) + : client.PullRequest.Create(Owner.WeakString, repositoryAddress.Value, newPullRequest)) .ConfigureAwait(false); return ToGitPullRequest(created); @@ -146,6 +171,31 @@ internal override async Task CreatePullRequestCoreAsync(string r } } + /// + /// Reads a holding GitHub's own repository id as the + /// Octokit's id-addressed overloads take. + /// + /// + /// is an unvalidated semantic string, because the two hosts + /// disagree about what one looks like — Azure DevOps's is a uuid, GitHub's a whole number — so + /// the type cannot enforce either and this is where GitHub's constraint is checked. A value that + /// is not a whole number reaches here only from a hand-built , since + /// fills the property from Repository.Id, which is a + /// already. That is a caller's argument rather than something a host + /// reported, so it raises and not a + /// — nothing was sent, and there is no response to describe. + /// + /// The address, whose is . + /// The repository id. + /// The id is not a whole number, so GitHub cannot be addressed by it. + private static long ToOctokitRepositoryId(GitRepositoryAddress repositoryAddress) => + long.TryParse(repositoryAddress.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out long repositoryId) + ? repositoryId + : throw new ArgumentException( + $"GitHub repository ids are whole numbers, and '{repositoryAddress.Value}' is not one. " + + "Address the repository by its Name instead.", + nameof(repositoryAddress)); + /// /// Creates an Octokit client wired to this provider's transport and credential. /// diff --git a/GitIntegration/GitProvider.cs b/GitIntegration/GitProvider.cs index 9e825fe..40225ce 100644 --- a/GitIntegration/GitProvider.cs +++ b/GitIntegration/GitProvider.cs @@ -77,81 +77,114 @@ public abstract class GitProvider : IGitHostingProvider /// public Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) => - GetPullRequestsCoreAsync(Ensure.NotNull(repositoryName).WeakString, cancellationToken); + GetPullRequestsCoreAsync(GitRepositoryAddress.ByName(Ensure.NotNull(repositoryName).WeakString), cancellationToken); /// public Task> GetPullRequestsAsync(GitRepository repository, CancellationToken cancellationToken = default) => - GetPullRequestsCoreAsync(ToRepositoryIdentifier(repository), cancellationToken); + GetPullRequestsCoreAsync(ToRepositoryAddress(repository), cancellationToken); /// public IGitPullRequestCreateBuilder CreatePullRequest(GitRepositoryName repositoryName) { - string identifier = Ensure.NotNull(repositoryName).WeakString; + GitRepositoryAddress address = GitRepositoryAddress.ByName(Ensure.NotNull(repositoryName).WeakString); return new GitPullRequestCreateBuilder((specification, cancellationToken) => - CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + CreatePullRequestCoreAsync(address, specification, cancellationToken)); } /// public IGitPullRequestCreateBuilder CreatePullRequest(GitRepository repository) { - string identifier = ToRepositoryIdentifier(repository); + GitRepositoryAddress address = ToRepositoryAddress(repository); return new GitPullRequestCreateBuilder((specification, cancellationToken) => - CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + CreatePullRequestCoreAsync(address, specification, cancellationToken)); } /// - /// Chooses how a repository is addressed in a request path: by the host's own identifier when - /// one is known, and by name otherwise. + /// Gets a value indicating whether this host's repository-addressed routes want + /// in preference to . /// /// - /// is what a host documents its own API in terms of. - /// Azure DevOps types its {repositoryId} path parameter as string (uuid), and draws - /// an explicit id-or-name distinction for the sibling project parameter while withholding - /// it here — so substituting a name there is unconfirmed against the documented schema rather - /// than sanctioned by it. Preferring the id closes that gap for every repository a caller got - /// from GetRepositoriesAsync, which is the normal way to obtain one. /// - /// Falling back to rather than requiring the id, because a - /// caller may legitimately have constructed a by hand from a name - /// alone. That fallback is the same unconfirmed substitution the name-taking overloads make, and - /// it is no worse than what those overloads already do. + /// Per-provider rather than shared, because the two hosts genuinely disagree and a single answer + /// was wrong for one of them. Azure DevOps types its {repositoryId} path parameter as + /// string (uuid) and draws an explicit id-or-name distinction for the sibling + /// project parameter while withholding it here, so an id is what its documented schema + /// asks for. GitHub's {repo} slot in /repos/{owner}/{repo} takes a repository + /// name only — its id-addressed route is the separate /repositories/{id}, so + /// putting GitHub's numeric id in the name slot addresses nothing and answers 404 for a + /// repository that plainly exists. /// + /// + /// A preference rather than a requirement: this only decides which candidate wins when a + /// carries both. Which of the two a provider was handed reaches it on + /// , so a provider whose two routes differ + /// can still address either form correctly rather than guessing from the value's shape. + /// + /// + /// for the reason gives: every + /// provider this library ships lives in this assembly, and no externally-defined subclass can be + /// instantiated anyway. + /// + /// + /// to prefer the host's own id; to prefer the name. + private protected abstract bool PrefersHostRepositoryId { get; } + + /// + /// Chooses how a repository is addressed in a request path, in this host's preferred order. + /// + /// + /// Falls back to whichever form was not preferred rather than requiring the preferred one, + /// because a caller may legitimately have constructed a by hand + /// carrying only one of the two. The result says which form it carries, so a provider whose id + /// and name routes differ answers the fallback with the right route instead of sending one form + /// down the other's route — which is exactly the failure that made this per-provider. /// /// The repository to address. - /// The path segment identifying the repository. + /// The value identifying the repository, and which of the two forms it is. /// is . /// /// carries neither a /// nor a , so there is nothing to address it by. /// - private static string ToRepositoryIdentifier(GitRepository repository) + private GitRepositoryAddress ToRepositoryAddress(GitRepository repository) { Ensure.NotNull(repository); - return repository.HostRepositoryId?.WeakString - ?? repository.Name?.WeakString - ?? throw new ArgumentException( - "The repository carries neither a HostRepositoryId nor a Name, so there is no way to " + - "address it on the host.", - nameof(repository)); + GitRepositoryAddress? byHostId = repository.HostRepositoryId?.WeakString is string hostId + ? GitRepositoryAddress.ByHostRepositoryId(hostId) + : null; + + GitRepositoryAddress? byName = repository.Name?.WeakString is string name + ? GitRepositoryAddress.ByName(name) + : null; + + GitRepositoryAddress? chosen = PrefersHostRepositoryId + ? byHostId ?? byName + : byName ?? byHostId; + + return chosen ?? throw new ArgumentException( + "The repository carries neither a HostRepositoryId nor a Name, so there is no way to " + + "address it on the host.", + nameof(repository)); } /// - /// Retrieves the open pull requests for the repository a path segment identifies. + /// Retrieves the open pull requests for the repository an address identifies. /// /// /// The single implementation behind both public overloads, so the two cannot answer the same - /// question differently. Takes the finished path segment rather than a + /// question differently. Takes the resolved rather than a /// or a , because choosing between an - /// id and a name is a decision that belongs in one place — — - /// rather than repeated in each provider. + /// id and a name is a decision that belongs in one place — — + /// rather than repeated in each provider. The address carries which form was chosen, so a + /// provider that addresses ids and names through different routes picks the right one. /// - /// The path segment identifying the repository. + /// The address identifying the repository. /// A token to cancel the request. /// The repository's open pull requests, as reported by the host. - internal abstract Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken); + internal abstract Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken); /// /// Creates the pull request a finished describes. @@ -163,14 +196,14 @@ private static string ToRepositoryIdentifier(GitRepository repository) /// this library ships lives in this assembly, so nothing outside it needs to implement this /// member. /// - /// - /// The path segment identifying the repository the pull request is opened against, already chosen - /// by or taken from a caller-supplied name. + /// + /// The address identifying the repository the pull request is opened against, already chosen by + /// or taken from a caller-supplied name. /// /// The pull request's finished configuration. /// A token to cancel the request. /// The pull request as the host reports it after creation. - internal abstract Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken); + internal abstract Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken); /// /// Attempts to retrieve the credential for this provider from the credential cache. @@ -383,6 +416,41 @@ internal HostingCredential ResolveCredential() => }; } +/// +/// How one repository is addressed on a host: the value to send, and which of the two forms — +/// the host's own id, or the repository's name — that value is. +/// +/// +/// +/// The discriminator is the whole point of this type. A bare string cannot say which form it +/// carries, and a provider cannot recover that from the value's shape: GitHub's repository ids are +/// decimal digits, and a repository may legitimately be named one. Passing the two forms +/// interchangeably is exactly how a numeric id came to be sent where GitHub's /repos/{owner}/{repo} +/// route wants a name, which answers 404 rather than failing in any way a caller could read. +/// +/// +/// A provider whose id and name routes are the same path — Azure DevOps's {repositoryId} slot +/// — can ignore entirely and send . +/// +/// +/// The value identifying the repository, unescaped. +/// +/// when is the host's own repository id; +/// when it is the repository's name. +/// +internal readonly record struct GitRepositoryAddress(string Value, bool IsHostRepositoryId) +{ + /// Addresses a repository by the host's own repository id. + /// The host's repository id. + /// The address. + public static GitRepositoryAddress ByHostRepositoryId(string hostRepositoryId) => new(hostRepositoryId, IsHostRepositoryId: true); + + /// Addresses a repository by its name. + /// The repository's name. + /// The address. + public static GitRepositoryAddress ByName(string repositoryName) => new(repositoryName, IsHostRepositoryId: false); +} + /// /// The outcome of resolving a provider's credential: a bearer token, a username and password, or /// nothing — proceeding unauthenticated. diff --git a/GitIntegration/Hosting/AzureDevOpsProvider.cs b/GitIntegration/Hosting/AzureDevOpsProvider.cs index 0f61b82..f518469 100644 --- a/GitIntegration/Hosting/AzureDevOpsProvider.cs +++ b/GitIntegration/Hosting/AzureDevOpsProvider.cs @@ -73,6 +73,19 @@ public sealed class AzureDevOpsProvider : GitProvider /// private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// + /// + /// , and unchanged by the split that made this per-provider. Azure DevOps + /// has one repository-addressed path — .../repositories/{repositoryId}/... — and + /// Microsoft's reference types that parameter as string (uuid) while drawing an explicit + /// id-or-name distinction for the sibling project parameter without drawing one here, so + /// an id is what the documented schema asks for and a name there is unconfirmed rather than + /// sanctioned. Both forms go into that same slot, so this provider never needs to read + /// — unlike GitHub, whose id-addressed and + /// name-addressed routes are different routes. + /// + private protected override bool PrefersHostRepositoryId => true; + /// /// Gets the name of this Git provider. /// @@ -156,7 +169,7 @@ public override async Task> GetRepositoriesAsync(Ca /// needs none of this, because its endpoint documents no pagination at all. /// /// - /// {repositoryId} is filled with , which the caller's + /// {repositoryId} is filled with , which the caller's /// choice of overload decides. Microsoft's reference types that parameter as a repository /// id (GitRepository.id is a string (uuid)), and draws an explicit /// id-or-name distinction for the sibling project parameter without drawing one here — a @@ -170,9 +183,9 @@ public override async Task> GetRepositoriesAsync(Ca /// /// /// is . See 's remarks. - internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) + internal override async Task> GetPullRequestsCoreAsync(GitRepositoryAddress repositoryAddress, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + string repositoryIdentifier = Ensure.NotNull(repositoryAddress.Value); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet(); @@ -228,9 +241,9 @@ internal override async Task> GetPullRequestsCoreA /// rather than a specific status code. /// /// is . See 's remarks. - internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(GitRepositoryAddress repositoryAddress, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryIdentifier); + string repositoryIdentifier = Ensure.NotNull(repositoryAddress.Value); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet(); From a238c5b7f8f3b2d1d4b921b287972b226d618785 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Mon, 14 Sep 2026 05:29:50 +0000 Subject: [PATCH 3/3] docs: record the per-provider repository addressing split [patch] Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TU2gC1LwTwyMin2XxYxg2m --- CLAUDE.md | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8e9c7d4..b9339a3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -414,8 +414,8 @@ keyed lookup would work only because `HttpResponseMessage` happens to canonicali casing. A `Retry-After` carrying an HTTP date rather than seconds yields `null`, since an unset `ResetsAt` is better than an invented one. -**A repository from a hosting provider carries no `LocalPath`, and is addressed by the host's own -id.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been +**A repository from a hosting provider carries no `LocalPath`, and how it is addressed is the +provider's own decision.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been cloned. Both providers used to invent one under `Environment.CurrentDirectory`, which made the same remote repository yield a different record depending on when it was enumerated, and which forced a containment guard to exist purely to keep a name from a remote response from escaping that @@ -423,14 +423,36 @@ directory. That guard is gone with the reason for it, and `IGitClient.Clone(GitR reports a missing `LocalPath` rather than resurrecting the invented default — where a working copy goes is the caller's decision. -`GitRepository.HostRepositoryId` carries what a host documents its own API in terms of. Microsoft's +`GitRepository.HostRepositoryId` carries what a host documents its own API in terms of — and the two +hosts disagree about what that is, which is why the preference is `GitProvider`'s +`private protected abstract bool PrefersHostRepositoryId` rather than one shared answer. Microsoft's reference types Azure DevOps's `{repositoryId}` path parameter as `string (uuid)` and draws an explicit id-or-name distinction for the sibling `project` parameter while withholding it here, so -substituting a name there is unconfirmed against the documented schema rather than sanctioned by it. -`GetPullRequestsAsync(GitRepository)` and `CreatePullRequest(GitRepository)` therefore prefer the id; -the `GitRepositoryName`-taking overloads still pass a name, since that is all they are given. Both -route through one internal core taking the finished path segment, so the choice lives in exactly one -place and the overloads cannot answer the same question differently. +substituting a name there is unconfirmed against the documented schema rather than sanctioned by it: +`AzureDevOpsProvider` prefers the id. GitHub's `{repo}` slot in `/repos/{owner}/{repo}` takes a +repository **name** only — its id-addressed form is the separate `/repositories/{id}` route — so +`GitHubProvider` prefers the name. A single shared preference for the id used to be applied to both, +which put GitHub's numeric id in a slot that only accepts a name and made +`GetPullRequestsAsync(GitRepository)` and `CreatePullRequest(GitRepository)` answer `404` on GitHub +for a repository just enumerated from the same provider — the overload `IGitHostingProvider` +documents as the one to prefer. + +The preference only decides which form wins when a `GitRepository` carries both. Which form was +chosen travels with it, on the internal `GitRepositoryAddress` record struct both internal cores now +take, so a repository carrying only an id is still addressed correctly on GitHub — through Octokit's +`long repositoryId` overloads, which build the id-addressed route — rather than by dropping an id +into the name slot. A bare string could not carry that distinction and a provider cannot recover it +from the value's shape, since a repository may legitimately be *named* a number. +`AzureDevOpsProvider` has one slot for both forms and ignores the discriminator entirely. The +`GitRepositoryName`-taking overloads still pass a name, since that is all they are given, and both +overloads still route through one internal core, so the choice lives in exactly one place and the +two cannot answer the same question differently. + +`FakeHttpMessageHandler.RespondToPath` exists because of this bug: the test that should have caught +it asserted the wrong URL against a handler that returned `[]` for every path, so the whole method +ran its success path around a request GitHub would have refused. A path-scoped response answers an +unexpected path with `404`, the way the host does, which makes a wrong route fail rather than pass +quietly. Prefer it over plain `Respond` for anything whose route is part of what the test claims. **A field a host reported goes through `GitProvider.ToHostValue`, never through `As()` directly.** The hosting counterpart to `GitParseValues.ToSemantic`, and it exists for the same reason: a value