feat: GitHub Enterprise Server (GHES) support#362
feat: GitHub Enterprise Server (GHES) support#362raghavpillai wants to merge 3 commits intoColeMurray:mainfrom
Conversation
Greptile SummaryThis PR adds comprehensive GitHub Enterprise Server (GHES) support by replacing all hardcoded
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /** Resolve the GitHub API base URL from a hostname (supports GHES). */ | ||
| function resolveApiBase(hostname?: string): string { | ||
| const host = (hostname || "github.com").toLowerCase().replace(/\/+$/, ""); | ||
| return host === "github.com" ? "https://api.github.com" : `https://${host}/api/v3`; | ||
| } |
There was a problem hiding this comment.
Unused resolveApiBase function (dead code)
This resolveApiBase function is defined here but never called within github-auth.ts. An identical copy exists in handlers.ts (line 15) which is the one actually used. This dead code will cause lint warnings and adds confusion about which version is the "canonical" one. Consider removing it from this file, or better yet, exporting it from one location and importing it in both.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/github-bot/src/github-auth.ts
Line: 9-13
Comment:
**Unused `resolveApiBase` function (dead code)**
This `resolveApiBase` function is defined here but never called within `github-auth.ts`. An identical copy exists in `handlers.ts` (line 15) which is the one actually used. This dead code will cause lint warnings and adds confusion about which version is the "canonical" one. Consider removing it from this file, or better yet, exporting it from one location and importing it in both.
How can I resolve this? If you propose a fix, please make it concise.
packages/web/src/lib/auth.ts
Outdated
| const emails: { email: string; primary: boolean }[] = await emailsRes.json(); | ||
| profile.email = (emails.find((e) => e.primary) ?? emails[0]).email; |
There was a problem hiding this comment.
Potential crash if emails array is empty
If the /user/emails endpoint returns an empty array, emails[0] is undefined and accessing .email on it will throw a TypeError at runtime. While GitHub typically returns at least one email, GHES instances may behave differently.
| const emails: { email: string; primary: boolean }[] = await emailsRes.json(); | |
| profile.email = (emails.find((e) => e.primary) ?? emails[0]).email; | |
| const emails: { email: string; primary: boolean }[] = await emailsRes.json(); | |
| const chosen = emails.find((e) => e.primary) ?? emails[0]; | |
| if (chosen) profile.email = chosen.email; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/web/src/lib/auth.ts
Line: 87-88
Comment:
**Potential crash if `emails` array is empty**
If the `/user/emails` endpoint returns an empty array, `emails[0]` is `undefined` and accessing `.email` on it will throw a `TypeError` at runtime. While GitHub typically returns at least one email, GHES instances may behave differently.
```suggestion
const emails: { email: string; primary: boolean }[] = await emailsRes.json();
const chosen = emails.find((e) => e.primary) ?? emails[0];
if (chosen) profile.email = chosen.email;
```
How can I resolve this? If you propose a fix, please make it concise.
|
please separate out the daytona changes |
- Add github-urls utility to centralize GitHub.com vs GHES URL resolution - Parameterize API/web/git/noreply URLs across control-plane, bots, modal-infra, web, and linear-bot - Thread apiBase/urls/GITHUB_HOSTNAME/GHES_TUNNEL_URL through configs and env - Update providers and handlers to use resolved URLs instead of hardcoded github.com - Add GHES tunnel proxy support for Workers via GHES_TUNNEL_URL - Adjust tests to include apiBase and update Terraform to expose new env vars - Misc: deprecate hardcoded GITHUB_API_BASE constant and adapt avatar/git URLs for GHES
raghavpillai
force-pushed
the
feat/ghes-support
branch
from
a495421 to
02456e2
Compare
Done |
ColeMurray
left a comment
ColeMurray
left a comment
There was a problem hiding this comment.
[Automated Review] PR #362 — feat: GitHub Enterprise Server (GHES) support
Files changed: 27 | +335 / -63
Summary
Replaces all hardcoded github.com / api.github.com URLs across the stack with a hostname-configurable abstraction backed by GITHUB_HOSTNAME. A new resolveGitHubUrls() in the control plane derives API base, web base, git host, and noreply domain from a single hostname. Per-package equivalents handle github-bot and Python modal-infra. Terraform wires the new github_hostname and ghes_tunnel_url variables to all workers. The web auth layer splits browser-side (user-reachable GHES) from server-side (tunnel-proxied) endpoints. Well-structured change that methodically touches all necessary layers.
Note: The diff includes unrelated changes merged from main (command palette #367, next.js bump #366). This review focuses only on the GHES commits.
Positive Feedback
- Clean central abstraction —
resolveGitHubUrls()returns a well-typedGitHubUrlsinterface that makes it impossible to miss a dimension (API, web, git host, noreply). Single-hostname-in, all-URLs-out is elegant. - Smart browser/server split in web auth — Separation of browser-side URLs (user hits GHES for OAuth redirect) from server-side URLs (tunnel-proxied for token exchange) correctly handles private VPC topology.
- Thorough Terraform wiring — All worker types plus the Cloudflare web worker get the new variables. The
web_app_custom_urloverride is a nice touch.
See inline comments for specific feedback.
| : baseUrls; | ||
| return createSourceControlProvider({ | ||
| provider, | ||
| github: { |
There was a problem hiding this comment.
[Automated Review] Suggestion: Consolidate tunnel URL rewriting logic.
This GHES_TUNNEL_URL → /api/v3 rewriting pattern is duplicated here and in getGitHubAppConfig() (github-app.ts). Both independently resolve the same tunnel-aware API base.
This is fragile — if the tunnel URL format changes, both sites need updating. Since getGitHubAppConfig() already bakes apiBase into the config (including tunnel logic), this second derivation here is redundant for the appConfig path.
Consider either:
- Having
resolveGitHubUrls()accept an optionaltunnelUrlparameter so there's a single derivation site, or - Extracting a small helper like
resolveGitHubApiBase(hostname?, tunnelUrl?)that both sites call.
| const urls = resolveGitHubUrls(env.GITHUB_HOSTNAME); | ||
| // When GHES is behind a private VPC, use the tunnel proxy for API calls | ||
| const apiBase = env.GHES_TUNNEL_URL | ||
| ? `${env.GHES_TUNNEL_URL.replace(/\/+$/, "")}/api/v3` |
There was a problem hiding this comment.
[Automated Review] Suggestion: Guard tunnel rewrite with isEnterprise.
If GHES_TUNNEL_URL is set but GITHUB_HOSTNAME is absent (or "github.com"), this code still appends /api/v3 to the tunnel URL. That would be incorrect for github.com, where the API lives at api.github.com, not github.com/api/v3.
This is likely a misconfiguration scenario, but a defensive guard would prevent subtle breakage:
const apiBase = urls.isEnterprise && env.GHES_TUNNEL_URL
? `${env.GHES_TUNNEL_URL.replace(/\/+$/, "")}/api/v3`
: urls.apiBase;Also see comment on shared.ts — this same rewriting logic is duplicated there.
| noreplyDomain: isEnterprise ? `users.noreply.${host}` : "users.noreply.github.com", | ||
| isEnterprise, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Automated Review] Suggestion: Add unit tests for resolveGitHubUrls().
This function is the foundation of the entire GHES feature, but it has no dedicated unit tests. Given the branching logic (enterprise vs. github.com, noreply domain format, API base derivation), a small test file would be valuable. Suggested test cases:
undefined/""/"github.com"→ standard github.com URLs"github.example.com"→ GHES URLs with/api/v3,isEnterprise: true"GITHUB.EXAMPLE.COM/"→ case normalization + trailing slash stripping- Verify all returned fields (
apiBase,webBase,gitHost,noreplyDomain,isEnterprise)
|
|
||
| /** Resolve the GitHub API base URL from a hostname (supports GHES). */ | ||
| export function resolveApiBase(hostname?: string): string { | ||
| const host = (hostname || "github.com").toLowerCase().replace(/\/+$/, ""); |
There was a problem hiding this comment.
[Automated Review] Nit: Use resolveApiBase() as fallback in private functions.
getInstallationToken() (line 79) and checkSenderPermission() (line 119) use apiBase ?? "https://api.github.com" as their fallback instead of calling the resolveApiBase() function defined right here. If a future caller omits apiBase, the fallback silently targets github.com regardless of GITHUB_HOSTNAME.
Suggested:
const base = apiBase ?? resolveApiBase();Note: resolveApiBase() doesn't read the env in the github-bot (unlike the Python version), so this would still default to github.com — but it centralizes the logic.
| if (!login) return undefined; | ||
| if (provider === "github") return `https://github.com/${login}.png`; | ||
| if (provider === "github") { | ||
| const host = (githubHostname || "github.com").toLowerCase().replace(/\/+$/, ""); |
There was a problem hiding this comment.
[Automated Review] Nit: Reuse resolveGitHubUrls() instead of inline normalization.
The hostname normalization logic here ((githubHostname || "github.com").toLowerCase().replace(...)) duplicates what resolveGitHubUrls() already does. Using:
const { hostname } = resolveGitHubUrls(githubHostname);
return `https://${hostname}/${login}.png`;would be more consistent with the rest of this PR's approach.
[Automated Review] Architectural AssessmentThe inline comments addressed localized code quality. This follow-up evaluates whether the PR's overall design is the right approach. 1.
|
| Package | What it does instead |
|---|---|
control-plane |
Uses resolveGitHubUrls() (the canonical one) |
github-bot |
Rolls its own resolveApiBase() — same logic, different interface |
linear-bot |
Inlines (env.GITHUB_HOSTNAME || "github.com").toLowerCase().replace(...) |
web |
Inlines the same hostname normalization + ternary for API base |
modal-infra (Python) |
Has its own resolve_api_base() (Python can't share TS — expected) |
The repo already has @open-inspect/shared specifically for cross-package types and utilities — every TS package depends on it. resolveGitHubUrls() belongs there. Instead, the PR creates 4 independent TypeScript implementations of "hostname → GitHub URLs."
2. Hostname normalization is copy-pasted 7 times
The exact pattern (hostname || "github.com").toLowerCase().replace(/\/+$/, "") (or the Python equivalent) appears in 7 locations. Four of those are TypeScript packages sharing a common dependency graph designed for exactly this kind of reuse:
control-plane/src/github-urls.tscontrol-plane/src/session/participant-service.tsgithub-bot/src/github-auth.tslinear-bot/src/webhook-handler.tsmodal-infra/src/auth/github_app.pymodal-infra/src/sandbox/bridge.pymodal-infra/src/scheduler/image_builder.py
3. Tunnel URL rewriting logic is triplicated
The GHES_TUNNEL_URL → tunnelUrl + "/api/v3" rewrite appears in 3 places:
github-app.ts:644-645routes/shared.ts:74-75web/src/lib/auth.ts:41-42
If the path convention ever changes, three files across two packages need updating. This should be a single function call — resolveGitHubUrls() could accept an optional tunnelUrl parameter to centralize this.
4. No hostname input validation
None of the 7 normalization sites validate that the input is actually a hostname. If someone configures GITHUB_HOSTNAME=https://github.example.com (with protocol — a plausible mistake), the result is https://https://github.example.com/api/v3. The function should strip a protocol prefix or throw a clear error.
5. GITHUB_API_BASE constant is dead code
The PR marks it @deprecated but no consumer references it in the PR branch. Dead exports should be removed, not annotated.
6. Missing tests
resolveGitHubUrls() is the single most important function in this PR — every API call, OAuth flow, git push, and avatar URL depends on it — and it has zero tests. The only test changes add apiBase: "https://api.github.com" to existing assertions. No test verifies the GHES path actually works.
7. getAvatarUrl() ignores the PR's own abstraction
participant-service.ts:67 inlines hostname normalization instead of calling resolveGitHubUrls(), which is defined in the same package. If the author's own code doesn't use the abstraction, the abstraction's coverage is incomplete.
Recommended changes
- Move
resolveGitHubUrls()to@open-inspect/shared, with an optionaltunnelUrlparameter to handle tunnel rewriting in one place - Delete
resolveApiBase()from github-bot — import from shared - Replace inline hostname normalization in linear-bot, web, and participant-service with the shared function
- Add hostname validation (strip or reject protocol prefixes)
- Add unit tests for the core function (GHES vs github.com, edge cases, tunnel URL)
- Remove the dead
GITHUB_API_BASEexport
This would consolidate 4 independent TS implementations into 1 shared function (Python duplication is expected and acceptable).
Verdict
The feature objective is sound, the browser/server URL split for web auth is well-thought-out, and the Terraform wiring is thorough. But the core abstraction lives in the wrong package, and the resulting duplication across 4 TS packages undermines the value of having @open-inspect/shared. Request changes before merge.
2 participants
Summary
Closes #360.
Replaces all hardcoded
github.com/api.github.comURLs with a hostname derived fromGITHUB_HOSTNAME. Set it to your GHES hostname and everything else adjusts — API base, OAuth endpoints, git remote URLs, noreply emails, avatar URLs.This PR is standalone and based on
main— no dependency on the Daytona provider PR.Changes
packages/control-planegithub-urls.ts—resolveGitHubUrls(hostname?)returns{ apiBase, webBase, gitHost, noreplyDomain, isEnterprise }from a single hostnamegithub-app.ts:apiBasethreaded through installation token generation and repo listinggithub.ts: OAuth token exchange, user info, email fetch, noreply generation all use resolved URLsgithub-provider.ts: PR creation, branch listing, repo access, git push spec, label/reviewer endpointsparticipant-service.ts: avatar URL uses configured hostnameshared.ts(createRouteSourceControlProvider): resolves URLs and appliesGHES_TUNNEL_URLoverride for private VPC setupstypes.ts:GITHUB_HOSTNAMEandGHES_TUNNEL_URLadded toEnvpackages/github-botgithub-auth.ts:resolveApiBase()exported for installation tokens and permission checkshandlers.ts: importsresolveApiBasefromgithub-auth, reaction API URLs use resolved base,apiBaseflows through caller gatingtypes.ts:GITHUB_HOSTNAMEadded toEnvpackages/linear-bottypes.ts:GITHUB_HOSTNAMEadded toEnvwebhook-handler.ts: usesenv.GITHUB_HOSTNAMEfor resolved URLspackages/modal-infragithub_app.py:resolve_api_base()for token generationbridge.py: noreply email fallback usesGITHUB_HOSTNAMEmanager.py:VCS_HOSTreadsGITHUB_HOSTNAMEinstead of hardcodinggithub.comimage_builder.py:git ls-remoteURLs use resolved hostpackages/webauth.ts: NextAuth GitHub provider splits browser-side URLs (user hits GHES directly for OAuth) from server-side URLs (Worker goes throughGHES_TUNNEL_URLfor token exchange and API calls)Terraform
github_hostname,ghes_tunnel_url,web_app_custom_urlvariableslocals.tf:web_app_urlrespectsweb_app_custom_urloverridePrivate VPC pattern
GHES is usually not publicly accessible.
GHES_TUNNEL_URLpoints to a reverse proxy (Cloudflare Tunnel, bore, etc.) that the control plane uses for server-side GitHub API calls. The user's browser still goes directly to GHES for OAuth authorize (since the user is on the private network).