Add GHES support to the review agent #611
Conversation
WalkthroughThe changes add support for GitHub Enterprise Server instances by implementing base URL aware GitHub App initialization in the webhook handler. The webhook now resolves the GitHub API base URL from request headers, normalizes it, and uses per-base-URL caching to initialize and reuse App instances. Additionally, throttling configuration is refined and request header handling is updated for lowercase normalization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Webhook
participant Cache
participant Resolver
participant AppInit
participant EventHandler
Client->>Webhook: POST webhook event<br/>with headers
Webhook->>Resolver: resolveGithubApiBaseUrl(headers)
Resolver-->>Webhook: baseUrl
Webhook->>Webhook: normalizeGithubApiBaseUrl(baseUrl)
Note over Webhook,Cache: Per-base-URL App lookup/init
Webhook->>Cache: Check githubAppCache[normalizedUrl]
alt Cache Hit
Cache-->>Webhook: Cached App instance
else Cache Miss
Webhook->>AppInit: Initialize App for baseUrl
AppInit-->>Webhook: New App instance
Webhook->>Cache: Store in githubAppCache
end
Note over Webhook,EventHandler: Event processing with<br/>prerequisites gating
alt Installation data present
Webhook->>EventHandler: Process event<br/>(pull_request, issue_comment)
EventHandler-->>Webhook: Event handled
else Missing prerequisites
Webhook-->>Webhook: Log, respond 200 OK
end
Webhook-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
brianphillips
force-pushed
the
ghes-review-agent
branch
from
ddaa795 to
c84d1e3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/web/src/app/api/(server)/webhook/route.ts (2)
53-59: Base URL normalization is solid
normalizeGithubApiBaseUrldefaulting to the public API URL and trimming trailing slashes is straightforward and avoids subtle cache misses betweenhttps://host/api/v3vshttps://host/api/v3/. Looks good.
101-104: Header normalization and event extraction look goodLowercasing all header keys up front and then reading
x-github-eventby its canonical lowercase name simplifies lookups and avoids case-sensitivity bugs across environments. This is a clean improvement and keeps the rest of the handler unchanged behavior-wise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)packages/web/src/app/api/(server)/webhook/route.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
CHANGELOG.md (1)
14-14: Changelog entry looks goodEntry correctly documents the GHES review agent fix and follows the existing style in this section.
| githubAppBaseOptions = { | ||
| appId: env.GITHUB_REVIEW_AGENT_APP_ID, | ||
| privateKey: privateKey, | ||
| privateKey, | ||
| webhooks: { | ||
| secret: env.GITHUB_REVIEW_AGENT_APP_WEBHOOK_SECRET, | ||
| }, | ||
| Octokit: throttledOctokit, | ||
| throttle: { | ||
| onRateLimit: (retryAfter: number, options: Required<EndpointDefaults>, octokit: Octokit, retryCount: number) => { | ||
| enabled: true, | ||
| onRateLimit: (retryAfter, _options, _octokit, retryCount) => { | ||
| if (retryCount > 3) { | ||
| logger.warn(`Rate limit exceeded: ${retryAfter} seconds`); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }, | ||
| } | ||
| }); | ||
| onSecondaryRateLimit: (_retryAfter, options) => { | ||
| // no retries on secondary rate limits | ||
| logger.warn(`SecondaryRateLimit detected for ${options.method} ${options.url}`); | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Throttle configuration is defined but not actually applied to Octokit
githubAppBaseOptions includes a throttle config, but getGithubAppForBaseUrl only passes baseUrl into Octokit.plugin(throttling).defaults(...). App’s constructor doesn’t accept a throttle option; throttle must be passed to the underlying Octokit constructor (e.g., via defaults({ throttle: ... })) for @octokit/plugin-throttling to use it.
Right now, you’re likely getting Octokit’s default throttling behavior instead of the custom retryCount > 3 logic and the secondary-rate-limit “no retry” behavior.
Consider wiring the throttle options into the Octokit defaults, e.g.:
-const getGithubAppForBaseUrl = (baseUrl: string) => {
- if (!githubAppBaseOptions) {
- return undefined;
- }
-
- const normalizedBaseUrl = normalizeGithubApiBaseUrl(baseUrl);
- const cachedApp = githubAppCache.get(normalizedBaseUrl);
- if (cachedApp) {
- return cachedApp;
- }
-
- const OctokitWithBaseUrl = Octokit.plugin(throttling).defaults({ baseUrl: normalizedBaseUrl });
- const app = new App({
- ...githubAppBaseOptions,
- Octokit: OctokitWithBaseUrl,
- });
+const getGithubAppForBaseUrl = (baseUrl: string) => {
+ if (!githubAppBaseOptions) {
+ return undefined;
+ }
+
+ const normalizedBaseUrl = normalizeGithubApiBaseUrl(baseUrl);
+ const cachedApp = githubAppCache.get(normalizedBaseUrl);
+ if (cachedApp) {
+ return cachedApp;
+ }
+
+ const { throttle, ...appOptions } = githubAppBaseOptions;
+ const OctokitWithBaseUrl = Octokit
+ .plugin(throttling)
+ .defaults({
+ baseUrl: normalizedBaseUrl,
+ ...(throttle ? { throttle } : {}),
+ });
+
+ const app = new App({
+ ...appOptions,
+ Octokit: OctokitWithBaseUrl,
+ });This keeps your per-base-URL behavior while ensuring the throttle callbacks are honored.
Also applies to: 70-89
I'll verify the review comment by inspecting the actual code and confirming the behavior of the Octokit throttling plugin.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the webhook route file to inspect the getGithubAppForBaseUrl function
cat packages/web/src/app/api/\(server\)/webhook/route.ts | head -100
Length of output: 3944
---
🌐 Web query:
```web
@octokit/plugin-throttling how to pass throttle option Octokit constructor
💡 Result:
Use the throttle option when constructing the Octokit instance (after applying the plugin). Example:
JavaScript (Node)
-
import and apply plugin:
const { Octokit } = require("@octokit/rest");
const { throttling } = require("@octokit/plugin-throttling");
const MyOctokit = Octokit.plugin(throttling); -
pass throttle in the constructor:
const octokit = new MyOctokit({
auth: "token " + process.env.TOKEN,
throttle: {
onRateLimit: (retryAfter, options, octokit) => {
octokit.log.warn(Rate limit for ${options.method} ${options.url});
if (options.request.retryCount === 0) return true; // retry once
},
onSecondaryRateLimit: (retryAfter, options, octokit) => {
octokit.log.warn(Secondary rate limit for ${options.method} ${options.url});
},
// or disable: enabled: false
},
});
Notes: the option key is throttle (not “throttling”), and onRateLimit / onSecondaryRateLimit (or onAbuseLimit on older versions) should return true to retry. [1][2]
Sources:
[1] Octokit docs — Throttling examples and constructor options.
[2] octokit/octokit.js README (throttle option description).
Throttle configuration is defined but not passed to the Octokit instance
At line 81, Octokit.plugin(throttling).defaults() only receives baseUrl, not the throttle config. The throttle callbacks defined in lines 31-47 are passed to the App constructor, which doesn't accept a throttle option—they're ignored.
Per the @octokit/plugin-throttling API, the throttle option must be passed directly to the Octokit instance via defaults() or the constructor. Without this, the custom retryCount > 3 logic and secondary rate-limit "no retry" behavior won't execute.
Update getGithubAppForBaseUrl to wire throttle into Octokit.defaults():
const OctokitWithBaseUrl = Octokit.plugin(throttling).defaults({ baseUrl: normalizedBaseUrl });
const app = new App({
...githubAppBaseOptions,
Octokit: OctokitWithBaseUrl,
});to:
const { throttle, ...appOptions } = githubAppBaseOptions;
const OctokitWithBaseUrl = Octokit.plugin(throttling).defaults({
baseUrl: normalizedBaseUrl,
...(throttle ? { throttle } : {}),
});
const app = new App({
...appOptions,
Octokit: OctokitWithBaseUrl,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/web/src/app/api/(server)/webhook/route.ts around lines 26 to 47, the
throttle config is defined but passed to the App constructor (which ignores it)
instead of being provided to the Octokit instance; update getGithubAppForBaseUrl
so Octokit.plugin(throttling).defaults(...) receives both baseUrl and the
throttle object (or pass throttle into the Octokit constructor) and remove
throttle from the App constructor args so your onRateLimit/onSecondaryRateLimit
callbacks are honored by @octokit/plugin-throttling.
2 participants
Summary
x-github-enterprise-hostheader, normalize the URL, and reuse per-host App instances via a small cachex-github-eventand log which base URL is in useTesting
Summary by CodeRabbit