ADR-0025: GitHub Integration Split — OAuth Handshake vs Bearer-Token API

Accepted Kendo

Date: 2026-05-07

Context

Kendo's backend has two GitHub integration classes today:

• App\Services\GithubService (242 lines) implements the user-OAuth handshake (getAuthorizationUrl, exchangeCodeForToken) and a wide REST API surface (repo listing, branches, file content, code search, branch creation, authenticated-user profile). It accepts the current user's access token through a 16-line container closure in AppServiceProvider::register() that reads $user->githubConnection?->access_token.
• App\Services\GithubAppService (142 lines) implements GitHub App authentication (mint installation token from a private-key JWT) and a narrower API surface (check runs, PR comments, workflow dispatch, release-asset download).

A third caller, App\Agent\Tools\AgentToolFactory, exposes the structural smell. On line 93 it mints a GitHub App installation token and then constructs new GithubService($this->http, '', '', '', [], $token) — passing empty OAuth credentials just to instantiate the OAuth-flavored class so the agent tools can use its API methods. This works only because GitHub's REST API is bearer-token-agnostic; the same endpoints accept user OAuth tokens or App installation tokens equivalently.

The misuse indicates that GithubService conflates two responsibilities:

1. OAuth credential acquisition — needs the kendo OAuth client credentials and the redirect URI; relevant only to the OAuth handshake itself.
2. Bearer-token-parametrized API surface — needs only HttpFactory plus a token; the same surface is callable under either user OAuth tokens or App installation tokens.

These responsibilities have different dependency profiles, different test surfaces, and different sets of callers. The current GithubService class is accidentally the API-surface module, with OAuth-credential properties bolted on the front.

Decision

Three load-bearing decisions.

1. Split GithubService into GithubOAuthClient + GithubApiClient

Replace App\Services\GithubService with two final readonly classes in a new App\Services\Github\ namespace:

• GithubOAuthClient owns the OAuth handshake. Constructor depends on HttpFactory and four #[Config]-resolved OAuth credentials (github.client_id, github.client_secret, github.redirect_uri, github.scopes). Public methods: getAuthorizationUrl(string $state): string, exchangeCodeForToken(string $code): array.
• GithubApiClient owns the bearer-token-parametrized REST API surface. Constructor depends on HttpFactory only. Public methods (each accepts string $token as first argument): getAuthenticatedUser, getRepositories, getBranches, getRepository, createBranch, getFileContents, getRepoTree, searchCode.

getAuthenticatedUser belongs in GithubApiClient, not GithubOAuthClient — it is an API call (GET /user) that happens to be invoked during the OAuth callback flow, not an OAuth-flow primitive.

Per-call timeouts

Every outbound HTTP egress on both new classes commits a per-call ->timeout() rather than relying on a base-client default. This preserves today's GithubService::client() 30-second posture and matches the per-call doctrine that PR #913 (Stripe) made canonical.

• Every GithubApiClient public method calls ->timeout(30) on the request it builds.
• GithubOAuthClient::exchangeCodeForToken calls ->timeout(30) on the token-exchange POST. (getAuthorizationUrl performs no HTTP egress — it only builds a URL string — so no timeout applies.)
• 30 seconds is the per-method default across the board today, matching the current GithubService::client() posture. The per-call form leaves room for a future method-specific override (e.g. raising searchCode if GitHub's search rate-limits push p95 latency up) without requiring a class-level base-client mutation.
• The refactor PR adds GithubApiClient::class and GithubOAuthClient::class to the named whitelist in backend/tests/Arch/ExternalHttpTimeoutTest.php and removes GithubService::class in the same change.

2. GithubApiClient accepts the bearer token as the first argument on every public method

GithubApiClient does not store a token in its constructor. Every public method takes string $token as its first parameter. Token resolution is the caller's responsibility: typically an Action reads $user->githubConnection?->access_token from its injected User, or AgentToolFactory mints an installation token via GithubAppService::getInstallationToken(...).

Why:

• Auditability. Every API call's authentication context is visible at the call site. The current GithubService resolves the token in a container closure; the resulting call site shows nothing about which token is in use.
• Unification of construction shapes. Container binding (for user-OAuth callers) and direct construction (for AgentToolFactory) produce the same object, parametrized at call time. There is no constructor-vs-call divergence to mis-bind in tests.
• Aligns with GithubAppService's existing per-call pattern. GithubAppService::createCheckRun(string $installationToken, ...) already takes the token per call. The new GithubApiClient matches that ergonomic.

3. GithubAppService stays as-is

GithubAppService retains its current shape: it remains the cohesive owner of "act-as-the-app" — JWT minting, installation token retrieval, and the bot-only endpoints (createCheckRun, createPrComment, dispatchWorkflow, downloadReleaseAsset) that are always paired with installation-token authentication.

GithubAppService is not exempt from the structural rule that motivates this split — it is structurally incapable of exhibiting the same smell. Its credentials surface is GitHub App authentication (private-key JWT → installation token), not user OAuth, and its API surface is bot-only invoked exclusively under installation-token auth. There is no second auth flavor for which a token-parametrized API client would let GithubAppService shed responsibility. Doctrine compliance verified independently by Cartographer recon: final readonly, #[Config] credentials, per-call ->timeout() on every HTTP egress, 16 unit tests covering the API surface (backend/tests/Unit/Services/GithubAppServiceTest.php).

The pairing of installation-token acquisition with bot-only endpoint methods is itself cohesive. The endpoints are only ever invoked under installation-token auth; separating them from token-minting would scatter the workflow.

Options Considered

Extract a shared HTTP transport only

The duplication between GithubService and GithubAppService is ~10 lines of withToken/withHeaders/timeout/baseUrl chain — below threshold, and it doesn't fix the AgentToolFactory.php:93 misuse. Rejected.

Full three-way split — OAuth + App credentials + shared GithubApiClient

Folding createCheckRun / dispatchWorkflow / etc. into a shared API client mixes user-perspective and bot-perspective methods, producing an incohesive Module. The user-perspective vs bot-perspective distinction is real even though both go over the same REST API. Rejected.

Merge into one parametrized GithubClient with two auth strategies

The user-OAuth flow's API surface and the GitHub-App flow's API surface are nearly disjoint. Merging would force one Module to expose exchangeCodeForToken (OAuth-only) alongside dispatchWorkflow (App-only), which is a wider interface than either caller set wants. Rejected.

Token at construction (instead of per-call argument)

Matches today's GithubService ergonomics, but container-bound shape and AgentToolFactory direct construction would diverge. Container binding has the user's token; direct construction has an installation token; the same class with two ways to obtain its token is a future-error attractor. Rejected.

Hybrid (constructor + per-call override)

Backwards-compatible, but every reader has to check which path is in use to interpret a call site. Rejected.

Leave as-is, accept the duplication

The 10-line transport duplication alone is below threshold, but the AgentToolFactory.php:93 misuse is a structural artifact that papers over a real shape mismatch — it will encourage future agent-tool callers to repeat the pattern. Rejected.

Consequences

Positive:

• AppServiceProvider::register() loses its 16-line GithubServiceInterface closure; both new interfaces bind via one-line concrete bindings.
• New unit test coverage for the GitHub API surface that did not exist before (the existing GithubService had no unit test because its constructor entangled OAuth config with the API surface).
• The AgentToolFactory.php:93 misuse pattern is removed.
• Future agent-tool callers have a clean API: inject GithubApiClientInterface, pass any bearer token.

Negative:

• Four Actions and one controller migrate from GithubServiceInterface to either GithubOAuthClientInterface or GithubApiClientInterface. Each gains 1-2 lines of explicit token retrieval ($user->githubConnection?->access_token with a ?? throw new GithubNotConnectedException guard). The dependency on "current user's GitHub token" becomes visible in the Action body — which is the point — but it is more code at each call site.
• Three agent-tool classes (GetRepoTreeTool, GetFileContentTool, SearchCodeTool) gain one constructor parameter each (the captured installation token), since the Laravel AI SDK's Tool interface controls when methods are invoked and the tool must capture the token at construction.
• Test mocks across tests/Feature/IssueBranchLink/, tests/Feature/Github/, and tests/Feature/ProjectGithubRepo/ migrate from GithubServiceInterface to either GithubOAuthClientInterface or GithubApiClientInterface based on which methods the test exercises. Mechanical change, no behavior shift.

Defence of the Action-body redundancy

The ?? throw new GithubNotConnectedException guard each migrated Action gains is, strictly speaking, redundant. Routes that reach these Actions already require a GitHub connection via Tier 1 ->can() middleware (ADR-0006); a request that arrives at the Action body has already been gated. The guard cannot fire in the happy path on any current route.

The redundancy is deliberate. Three reasons:

1. The token's existence becomes a property of the Action, not of the routing layer. Anyone reading GetGithubBranchesAction::execute() sees the dependency on a connected user's token at the body of the method, not by chasing the route definition that wired the Action up. That visibility is the load-bearing ergonomic Decision 2 is selling — without it, the per-call token argument is just structural noise.
2. It survives route refactors. If a future caller wires this Action behind a route that lacks the GitHub ->can() gate (a new central-app endpoint, an MCP tool, a queue job), the guard converts a silent null token into a typed exception with a 401 response. The container-time silent-null today produces a confusing downstream API failure.
3. It is cheap. One line per Action, no runtime cost in the gated case (null-coalesce on a non-null value), and the existing exception (App\Exceptions\GithubNotConnectedException, Response::HTTP_UNAUTHORIZED) means no new error-handling infrastructure is needed.

The trade we are making: a one-line redundancy at every Action body in exchange for collapsing a 16-line container closure plus an implicit routing-layer invariant into a single explicit assertion at the place that needs the token. We judge that exchange favourable.

Enforcement

The structural split itself is one-time. Decision 2 — token as first argument, never bound at construction — is the ongoing rule, and it is the rule most likely to drift: a future contributor encountering a fresh callsite may find it cleaner to bind a token at construction "just for this one case", and ADR prose alone does not survive contributor turnover. The Enforcement Escalation Ladder exists for exactly this kind of invariant; we push it down to CI-time.

What	Mechanism	Scope
No constructor-bound bearer tokens on *Client classes	Pest arch test in backend/tests/Arch/ServicesTest.php: walks every class under App\Services\Github\ whose name matches *Client, reflects on the constructor parameters, asserts no parameter is named $token, $accessToken, or $bearerToken and typed string	App\Services\Github\*Client
GithubApiClient API-surface coverage	Pest unit test in backend/tests/Unit/Services/Github/GithubApiClientTest.php against Http::fake() — setup pattern (no constructor token, token passed per call) locks the shape from the test side	App\Services\Github\GithubApiClient
Per-call HTTP timeouts on both new clients	backend/tests/Arch/ExternalHttpTimeoutTest.php named-whitelist swap (add GithubOAuthClient::class + GithubApiClient::class, remove GithubService::class)	Both new client classes
Code review fallback	Reviewers cite this ADR when pushing back on attempts to reintroduce a unified GithubService-style class	All App\Services\Github\ additions

Promotion to a PHPStan rule (Level 2) under phpstan-warroom-rules is deferred until the pattern recurs in a non-GitHub territory; until then, kendo-local Pest enforcement is sufficient.

Resolved Questions

Why not standardize error semantics across GithubApiClient and GithubAppService?

Resolved 2026-05-03. Today, GithubApiClient's methods (inherited from GithubService) return null on failure; GithubAppService calls $response->throw(). The split preserves the current per-method semantics — this ADR deliberately does not standardize across the two services. Mixing semantics-change with structure-change in one PR muddies the diff. Revisit if callers need consistency.

Why not move GithubAppService to App\Services\Github\ too?

Resolved 2026-05-03. Currently it stays at App\Services\GithubAppService. Co-locating it would give the three GitHub modules a shared folder, but GithubAppService's ergonomics are unchanged by this ADR and renaming it would broaden the diff. Punted to a follow-up.

Why kendo-only and not cross-project?

Resolved 2026-05-07. No other territory uses GitHub today. Cross-project framing would be aspirational. The patterns this ADR establishes — split a service that conflates credential acquisition with a bearer-token-parametrized API surface; pass the bearer token as a per-call argument — generalize naturally to any future GitHub or OAuth-style integration in another territory, but until that territory exists, the rule lives kendo-local. Promotion to cross-project (with companion phpstan-warroom-rules Phase 2 rule) is pre-baked when the second territory adopts.

Why retain GithubAppService rather than fold its endpoints into GithubApiClient?

Resolved 2026-05-03. Folding would unify the API surface, but createCheckRun and dispatchWorkflow are bot-only operations that read awkwardly when listed alongside getRepositories and searchCode. The user-perspective vs bot-perspective distinction is real even though both go over the same REST API, and GithubAppService is structurally incapable of exhibiting the conflation smell that motivated the split.

Implementation

Territory	State	Notes
kendo	ADR adopted, refactor not started	Refactor PR will be scoped from docs/plans/KD-0611-github-integration-split/SEED.md (kendo). Follow-up kendo issue to be created when Jasper begins execution. Out-of-scope fortification work — caching getInstallationToken per-installation with 5-minute pre-expiry skew — tracked separately as KD-0622.
Other territories	Not in scope	No GitHub integration surface today. Pattern available for adoption when one is added.

References

• Originating PR: kendo PR #1041 (KD-0611) — ADR draft + SEED handoff note.
• Verification recon: Cartographer field report on GithubAppService doctrine compliance — independently confirmed final readonly, #[Config] credentials, per-call timeouts on all 5 HTTP egresses, 16 unit tests covering the API surface.
• Background reading: GitHub REST API authentication — confirms the bearer-token-equivalence between user OAuth tokens and App installation tokens that motivates Decision 2.
• Related ADRs: Action Class Architecture, Two-Tier Authorization, Config Attribute Injection, PHPStan Rules Package.
• Related doctrine: Doctrine #8 (explicit HTTP timeouts on external calls).
• Precedent: PR #913 (kendo Stripe per-call timeout doctrine) — the canonical example of per-call ->timeout() over base-client defaults.