Make the hosting layer real: GitHub and Azure DevOps providers - #80
Merged
Merged
Conversation
Supersedes the v2 spec's hosting section, whose AzureDevOpsProvider was written around Microsoft.TeamFoundationServer.Client. That package was rejected by measurement, so the provider goes over raw HttpClient. Records that the layer shipping today is a stub: RefreshRemoteRepositories refreshes nothing, the repository bag is never populated, nothing consumes either, and there are no tests.
Ten tasks. Task 1 is research: the Azure DevOps endpoint details are verified against the published reference and the fixtures captured from real responses, because a client built against assumed field names works against its author's beliefs rather than the service.
Re-verified api-version=7.1 (stable) covers every field and query parameter this phase needs, and rejected the 7.2-preview.2 value the first pass reported without checking the stable surface. Also checked the Get Pull Request By Id page at both versions and the official azure-devops-node-api type definitions for _links.web.href; none show it, so the findings doc now records this as settled rather than an open gap, with the null-when-absent ruling for Task 9 to inherit.
…ng fixtures Redaction had been field-driven and missed the same real identifiers surfacing in sibling fields: a real person's identity in requested reviewers, real repository names via ssh_url, real repo/branch names beside already-redacted labels, a real GitHub App slug, and real numeric/base64/opaque GitHub node ids not reachable by plaintext substitution. Swept every GitHub fixture by value rather than by field name and closed all of it consistently with the fake identities already in use. Also fixes azure-devops-pullrequests.json, which had copied PR22's mergeId and lastMergeSourceCommit onto PR21 and PR1 and omitted lastMergeCommit entirely; corrected all three entries against the documented example so the fixture is what its provenance claims.
repo.description on the embedded head/base repo objects still carried the real project's verbatim GitHub tagline in two variants, defeating the rest of the redaction on those same objects. Replaced both with a generic placeholder everywhere the field appears. Also redacts a real default_branch value on one fork (same class as the head.ref leak fixed last round) and two real, repo-specific area labels found while re-reading every prose field for anything a by-value grep can't catch.
RegexMatch validates shape, not presence -- it short-circuits to success on an empty string, so the number's digits-only pattern let an empty value through. Author and web URI had no attribute at all. Number is required and so has no other way to express absence.
Five exception types for hosting-provider HTTP failures, deliberately rooted at Exception rather than GitException since HTTP failures carry no exit code or argument vector.
RefreshRemoteRepositories() authenticated a client and returned without fetching anything; Repositories was a bag nobody ever populated. Both are gone, replaced by IGitHostingProvider (GetRepositoriesAsync, GetPullRequestsAsync, CreatePullRequest) and the credential resolution the old code silently got wrong: CredentialWithToken was ignored without a word, and only CredentialWithUsernamePassword was ever recognised. ResolveCredential() now recognises every ktsu.CredentialCache credential subtype the library understands - a token, a username/password pair, or nothing (proceed unauthenticated) - and throws for anything else, since a caller-configured credential this library can't recognise should never be silently treated as absent. GitProvider also gains the internal transport seam (Handler, CreateHttpClient) both future providers share, mirroring IGitProcessRunner's fake/real split for the local layer. GitHubProvider keeps compiling via three NotImplementedException overrides; GitProvider.CreatePullRequest does the same pending its builder. Both are scaffolding removed by later work, not incomplete work.
Each test resetting and reconfiguring the process-wide CredentialCache singleton around its own AddOrReplace call was flaky under Microsoft Testing Platform's parallel test execution: one test's reset could land between another test's seed and its ResolveCredential read, wiping the credential before it was ever resolved. Configuring the InMemoryCredentialStore once, in a static constructor the CLR guarantees runs at most once, removes the race; each test's own PersonaGUID is all the isolation a shared cache needs.
…ssembly CreatePullRequestCoreAsync's GitPullRequestSpecification parameter is internal, so the member itself must be internal, so no code outside this assembly can override it - which means no externally-defined subclass of GitProvider, though it is public abstract, can ever be instantiated. That consequence previously lived only in two members' remarks, invisible to a reader of the class-level doc. Recorded on the class itself instead, along with why: the parameter carries no reason to become public API purely to enable subclassing nothing has asked for, and this library ships exactly two providers, both in-assembly.
Wires GitHubProvider's three abstract members to Octokit 14.0.0: repository enumeration, open-pull-request listing, and pull request creation, mapping Octokit's exceptions onto this library's GitHostingException hierarchy and applying the resolved credential to the client. CreateClient returns its transport alongside the client so it can be disposed by the caller without ever disposing a Handler this provider does not own - required to satisfy CA2000/CA2215 without a suppression.
CreateClient() previously returned a null Transport when no Handler was injected, which meant every production call built an HttpClientAdapter (and its underlying HttpClient/HttpClientHandler) through GitHubClient's default constructor and never disposed it - neither GitHubClient nor Connection implements IDisposable, so nothing downstream ever would. Both branches of CreateClient now build and return a disposable HttpClientAdapter, one call site fewer nullable-dispose no-op away from a real leak, and credential resolution now runs before either adapter is constructed so a thrown InvalidOperationException can never strand one undisposed. Also: moved request-object construction inside each method's try block so argument validation can't leak the transport; replaced two racing per-class CredentialCache static constructors with a single AssemblyInitialize; added full-field-mapping and request-body coverage for the pull request path; and documented that GetRepositoriesAsync returns GitHub's public repositories only, regardless of credential.
Implements GetPullRequestsAsync and CreatePullRequestCoreAsync on AzureDevOpsProvider against the endpoints, api-version, and field names recorded in the Azure DevOps REST findings. WebURI is read only from _links.web.href, never composed, since no official Microsoft example documents a web key on a pull request's _links. Pull request operations now throw InvalidOperationException naming Project when it is unset, since Azure DevOps has no project-less pull-request endpoint.
…rallel
GetPullRequestsAsync's remarks defended passing a repository name in
Azure DevOps's {repositoryId} slot by comparing it to GitHubProvider's
equivalent path segment. GitHub's path has no separate GUID form to
substitute across, so the parallel overstated the support: Microsoft's
live reference types repositoryId as an id, draws an explicit
id-or-name distinction for the sibling project parameter, and does not
draw one here. The substitution is still forced by IGitHostingProvider
exposing no repository id, so the behaviour is unchanged; the comment
now says plainly that this is unconfirmed against the documented
schema rather than sanctioned by it.
Also adds the missing ResponseBody assertion to the pre-existing
403-with-rate-limit-headers test, matching its companion.
No DI registration was added for GitHubProvider/AzureDevOpsProvider: neither has a
constructor dependency this container could supply, so a registered factory would only
wrap `new GitHubProvider { Owner = owner }` with no actual wiring. Documented that
decision on ServiceCollectionExtensions instead.
CLAUDE.md's Hosting layer section is rewritten to record both providers, the shared
HttpMessageHandler transport seam, the explicit open-pull-requests-only contract, Azure
DevOps's project requirement for pull request operations, GitHub's public-repositories-only
enumeration, and the absence of an integration tier for this layer. Also documents a
second KTSU0006 mechanism found this phase: the shared framework, not another package,
can be the lower resolver forcing a VersionOverride-shaped hazard, and project.assets.json
is the check that catches it.
README.md gains the hosting layer in its feature list and a full usage walkthrough:
enumeration on both providers, listing open pull requests, creating one via the builder,
and handling GitHostingException subtypes.
CLAUDE.md and README.md both claimed every hosting-layer request goes through GitProvider.CreateHttpClient(). False for GitHubProvider: it builds an Octokit HttpClientAdapter from Handler directly in its own CreateClient() and never calls CreateHttpClient() at all. Only AzureDevOpsProvider calls that method. CLAUDE.md's very next paragraph already said the two providers differ here, so the claim contradicted its own follow-up within five lines. Reworded both passages so the transport description and the per-provider difference read as one explanation instead of a claim and its correction.
Map GitHub's plain 403 to GitHostingAuthenticationException. Octokit's
AuthorizationException derives from ApiException, not ForbiddenException, so
"resource not accessible by personal access token" fell through to
GitHostingRequestException while Azure DevOps mapped the same status to an
authentication failure. Adds the ForbiddenException arm plus dedicated arms for
SecondaryRateLimitExceededException and AbuseException, which both derive from
ForbiddenException and so must precede it.
Make IsAuthenticated agree with ResolveCredential. It reported true for a
resolved CredentialWithNothing, which means "proceed unauthenticated", and for
a subtype ResolveCredential throws on. Both now report false, through one
shared helper so the two answers cannot drift apart.
Wrap the three Azure DevOps success-path deserializations. A 200 carrying HTML,
which is what a proxy interstitial or a sign-on redirect returns, threw
System.Text.Json.JsonException out of a public method whose documented failure
surface is the GitHostingException hierarchy.
Page the Azure DevOps pull request listing with $top and $skip. A repository
with more open pull requests than one page was silently truncated, where
Octokit follows every page for GitHub under the same interface.
Share one SocketsHttpHandler per provider type instead of building one per
call, with PooledConnectionLifetime set so a long-lived handler does not pin
stale DNS. Disposing a per-call handler is the other half of the socket
exhaustion antipattern. An injected handler is still never disposed, and
neither provider became IDisposable.
Also: vary the draft flag in both providers' field-mapping fixtures, so the
assertion fails when the mapping is deleted; say on IGitHostingProvider that
GitHub honours cancellationToken only at entry; drop repo-only doc paths from
shipped XML docs; correct README's IGitHostingProvider member table; fix a
comment that inverted the null-means-not-known rule; remove phase and task
references from tracked source; pin GitHub's GET /users/{login}/repos request
path; make the fake handler's request recording thread-safe; and note in
CLAUDE.md that transitive pinning gives the System.Text.Json pin repo-wide
reach.
Suite 470/470, 0 skipped, up from 455. Build 0 warnings, 0 errors.
Folds the bare-429 divergence into the finding this wave already fixed for 403. Octokit has no dedicated exception for 429: it surfaces as a plain ApiException, so it reached GitHostingRequestException while AzureDevOpsProvider maps the same status to GitHostingRateLimitException. A caller writing host-agnostic catch (GitHostingRateLimitException) was handled on Azure DevOps and missed on GitHub, which is the same defect at the same site, discovered by the 403 fix rather than by the review. The new arm matches on status rather than on type, so it sits below the three ForbiddenException-derived arms: a type test is the more specific claim, and a 429 carried by one of those subtypes should still be answered by its own arm. ResetsAt comes from the response's Retry-After header, converted the way AbuseException's own relative delay already is. The header is read case-insensitively rather than by key: header names are case-insensitive and Octokit's header dictionary compares ordinally, so a keyed lookup would work only because HttpResponseMessage happens to canonicalise this header's casing. A Retry-After carrying an HTTP date rather than seconds yields null, leaving ResetsAt unset rather than carrying an invented instant. All three facts were verified against the Octokit 14 assembly by driving a real client over a fake handler, not assumed. Also records in CLAUDE.md that a mutation check must mutate by substitution and never by deletion. Reverting a deletion means replacing the empty string, so an "exactly one occurrence" guard fails and the revert silently does nothing. That happened once during this wave and left three Translate arms deleted, caught only by the next build. Suite 472/472, 0 skipped, up from 470. Build 0 warnings, 0 errors.
The finally comment in AzureDevOpsProvider.GetRepositoriesAsync opened "This client owns its handler exactly when it constructed one". That was true before the shared-transport change and is not now: CreateHttpClient passes disposeHandler: false unconditionally and the client never constructs a handler. The line after it had been rewritten around the stale one, which kept its premise alive by implying a constructing case that no longer exists. Both lines now describe what the code does. CreateDefaultHandler carried two <remarks> elements, the second after <returns>. The XML is well-formed so the compiler stayed quiet, but doc renderers show only the first, so the reasoning for the internal accessibility choice was invisible at the point a reader would question it. Merged into one <remarks> as a fourth <para>, keeping both pieces of reasoning, with <returns> last. Confirmed against the generated ktsu.GitIntegration.xml for both target frameworks. Comments only, no behaviour. Suite 472/472, 0 skipped. Build 0 warnings, 0 errors.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
… disposal Repository names GitHub and Azure DevOps report back are combined into GitRepository.LocalPath with Environment.CurrentDirectory. Path.Combine silently drops the current directory whenever the reported name is rooted, and a name of ".." escapes upward even after stripping a root, so a malicious or compromised host response could point LocalPath outside the current directory entirely. Both providers now derive a single leaf segment first and throw GitHostingRequestException when no safe leaf can be derived (empty, ".", or ".."). Also replaces every acquire/try/finally disposal of a per-call transport (the Octokit HttpClientAdapter in GitHubProvider, the raw HttpClient in AzureDevOpsProvider) with a using declaration, and filters TryGetRetryAfterSeconds's header scan explicitly with .Where(...) instead of an if inside the loop.
GitProvider.ToLocalDirectoryLeaf derived a safe leaf segment but left the Path.Combine(Environment.CurrentDirectory, ...) step to each caller, which is why an analyzer kept flagging the call site even though the result was already contained. Renamed to ToLocalRepositoryPath and had it return the combined AbsoluteDirectoryPath directly, so no Path.Combine remains at either provider's call site and a future provider cannot call it and forget to combine, or combine against the wrong base directory.
|
This was referenced Aug 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5b of the GitIntegration v2 design. The hosting layer becomes a working subsystem: repository enumeration plus pull request listing and creation, against GitHub and Azure DevOps.
What this replaces
The hosting layer that shipped before this branch was a stub.
GitHubProvider.RefreshRemoteRepositories()set credentials and returned without refreshing anything,ConcurrentBag<GitRepository> Repositorieswas never populated by anyone, nothing in the library consumed either, and the layer had no tests. The method was alsovoidand synchronous while any real implementation must do network I/O, so as written neither Octokit nor a REST client could implement it honestly.Both members are removed. Neither ever worked, so no consumer can depend on their behaviour, and removal produces a compile error rather than a silent change. That is why this ships as
[minor]rather than a major bump.What's here
IGitHostingProvider, with credential resolution and aninternal HttpMessageHandlertransport seamGitHubProviderover Octokit,AzureDevOpsProviderover rawHttpClientGitPullRequest,GitPullRequestState, and four semantic typesGitHostingExceptionhierarchy: authentication, not found, rate limit, and requestFakeHttpMessageHandler, which scripts responses and records complete requestsBuild 0 warnings and 0 errors. Suite 472/472 passing, 0 skipped, verified under
KTSU_GIT_INTEGRATION_TESTS_REQUIRED=1 GIT_CONFIG_NOSYSTEM=1so the earlier phases' integration tier runs the way the POSIX runners see it. Zero[SuppressMessage]attributes, holding the record across six phases.Design decisions worth knowing
One
HttpMessageHandlerseam fakes both providers. Octokit receives the same handler through itsHttpClientAdapter, so a test never fakes two vendor transports. Two genuinely different implementations test whether the abstraction generalizes better than two copies of the same code would.One shared
SocketsHttpHandlerper provider type, with a short-lived client or adapter per call. A handler owns a connection pool, so building and disposing one per call tears the pool down each time and leaves sockets inTIME_WAIT.PooledConnectionLifetimeis set to two minutes, matchingIHttpClientFactory, so a process-lifetime handler does not keep using a host's original address after DNS moves it.The two providers express "not mine to dispose" differently, and that asymmetry is forced.
HttpClienttakes adisposeHandler: falseflag. Octokit'sHttpClientAdapteroffers no equivalent and disposes whatever its factory produced, so GitHub wraps in a non-owning handler instead. Wrapping on both sides was tried and rejected, because CA2000 cannot see thatHttpClienttakes ownership of a handler passed to it, and the uniform version needs a suppression this repository does not allow.GetPullRequestsAsyncreturns open pull requests only, requested explicitly of each host. Both hosts happen to default that way, but the contract is defined here rather than by restating a vendor default that could change.Azure DevOps pull request operations require
Project; enumeration does not. Azure DevOps nests repositories under projects and GitHub has no equivalent level, soProjectis optional for enumeration and required for pull requests, where it throws with a message naming what to set.GitHub returns public repositories only, and a token does not widen that.
GET /users/{login}/reposdoes not honour authentication the wayGET /user/reposwould, and switching would silently stop honouring the configuredOwner. Azure DevOps returns everything its token can see. Two implementations of one interface genuinely differ here, and both the interface and the override say so.Two hazards this branch hit
A
System.Text.Jsonreference nearly shipped aFileNotFoundExceptionto every net9.0 consumer. Adding it for analyzer KTSU0006 withPrivateAssets="all"and no version pin resolved the package's real 10.0.2 assembly on net9.0, overriding the framework's 9.0.x, whilePrivateAssetskept it out of the nupkg. CoreCLR rolls binds forward but never backward.The documented check did not catch it. "Does another package pin a lower version?" answers no here, because nothing else in the graph pins it at all. The lower resolver was the shared framework.
CLAUDE.mdnow records that second mechanism and the check that does work: readproject.assets.jsonand confirm what each target framework resolves. Both now resolve an emptylib/net9.0/_._placeholder.A test-isolation race could have written to a real keyring. Two test classes each reset the credential cache singleton in a static constructor, which runs once per type rather than once per assembly, while the suite parallelises at method level. A test touching the cache between the reset and the reconfigure would rebuild it on the platform's native secret manager. One
[AssemblyInitialize]now owns it.Deliberately out of scope
Merging or completing a pull request, comments, reviews, repository creation, and webhooks. There is also no integration tier for this layer: hitting real hosts needs credentials and network access CI cannot have, so these tests verify the client against this project's understanding of each API rather than against the APIs themselves. Fixtures captured from real responses narrow that gap without closing it, and
CLAUDE.mdsays so plainly.Follow-ups recorded, none blocking
GitRepository.LocalPathderiving from the current directory,innerExceptionoverloads on the hosting hierarchy, the{repositoryId}path segment carrying a name where Microsoft documents an id, a plainQueuein the fake beside the field that was synchronized, and an unpinned HTTP-dateRetry-Afterpath.