GitHub Rate Limit Fallback -> Primary - #996
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGitHub authentication now supports separate read, write, and Git transport credentials with fallback, cooldown, repository access tracking, and rate-limit pauses. Relay and PR polling use health-aware scheduling. The UI, CLI, token callers, OAuth service, and webhook authorization reflect the updated state model. ChangesGitHub integration changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
54f15a5 to
b57addc
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57addc1d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tokenStored: true, | ||
| userLogin: validated.userLogin, | ||
| authSource: tokenLookup.source, | ||
| authSource: candidate.source, |
There was a problem hiding this comment.
When a classic token has repo access but lacks the write-only workflow scope, /user succeeds and this candidate becomes active, but computeConnected still applies the old combined read/write scope requirement and returns false. prService.requireGithubSnapshotAuth then rejects every PR snapshot before the new request-level fallback can run, even though this change now defines connected as read usability and reports writes separately through writeAuthSource; evaluate read capability independently or continue to the next read candidate.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| const canTryNext = !args.token | ||
| && (response.status === 401 || response.status === 403 || response.status === 429); |
There was a problem hiding this comment.
When a higher-precedence credential authenticates successfully but cannot see the target private repository, GitHub commonly returns 404 rather than 403; this condition immediately throws instead of trying a healthy App, CLI, or PAT candidate. The mirrored headless implementation has the same exclusion, so both desktop and socket-backed CLI paths lose the promised fallback for private-repository reads; retry repository-scoped 404s through the credential chain and report not-found only after the candidates are exhausted.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| const activeWriteCandidate = writeCandidates.find((candidate) => !statusCooldown(candidate)) | ||
| ?? null; |
There was a problem hiding this comment.
When the GitHub App succeeds as the preferred read credential, the loop stops without probing the lower gh or PAT tokens, yet this selection labels the first non-cooled write token as active. If that token is expired or under-scoped, writeAuthSource still hides the connection banner and enables write-dependent UI until the user's first write fails with 401/403; validate the write candidate separately, in both desktop and headless services, before reporting it as active.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
b57addc to
b64b653
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b64b653251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const permissionDenied = errorTypes.includes("FORBIDDEN") | ||
| || /forbidden|resource not accessible|not accessible by integration|does not have access/i.test(message); |
There was a problem hiding this comment.
When a higher-priority credential cannot see a private repository, GitHub can return HTTP 200 with a GraphQL NOT_FOUND error such as “Could not resolve to a Repository”; this classifier recognizes only rate-limit and FORBIDDEN variants, so apiRequest returns the payload and prService.graphqlRequest throws before trying the App, CLI, or PAT fallback. The new REST 404 handling therefore still leaves GraphQL-backed PR status and merge reads broken in both desktop and headless paths; treat ambiguous repository NOT_FOUND responses as credential failures and exhaust the candidate chain.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| async getTokenOrThrowAsync(): Promise<string> { | ||
| const token = (await readAuthToken()).token; | ||
| const token = (await readAuthToken("write")).token; |
There was a problem hiding this comment.
When the read-only GitHub App is the only healthy credential, forcing this getter to select a write credential makes it report missing auth even though status.connected is true. Direct read callers such as projectScaffoldService.listMyGitHubRepos and prService.downloadJobLogTail call this getter before performing repository or Actions reads, so those surfaces bypass the new App-first read chain and cannot use request fallback; expose a read capability for these callers or route them through the capability-aware request path, including the mirrored headless service.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
apps/desktop/src/shared/githubOperationCredential.test.ts (1)
19-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the degraded acceptance path.
The suite covers fallback when a later candidate works. It does not cover the branch where a repository-access failure is the only outcome. That branch sets
activefrom a failed probe and callsonAcceptedProbewithvalidated: false, which decides whether the credential is reported as connected and whether probe success is recorded. Add a case withreadCandidates: [app]only, and assertactive.candidate.source === "app",activeWriteSource === null, andonAcceptedProbereceivesfalseas the third argument.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/githubOperationCredential.test.ts` around lines 19 - 56, Add a test alongside “resolves read fallback and validates the selected writer once” covering a single read candidate whose probe reports repository access failure. Use readCandidates: [app] with no writable candidate, then assert active.candidate.source is "app", activeWriteSource is null, and onAcceptedProbe is called with false as its third argument.apps/desktop/src/main/services/github/githubService.ts (1)
1269-1277: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a
304response that has no cache entry.
inFlightConditionalGetKeysprotects the key only until thefinallyblock at Lines 1265-1267 runs. A concurrent request can evict the entry between that point and the lookup at Line 1270. Control then falls through toresponse.text(), and!response.okreports the304asGitHub API request failed (HTTP 304).Keep the key protected until the cached value is read, or repeat the request without
if-none-matchwhen the entry is gone. The headless implementation inapps/ade-cli/src/headlessLinearServices.tshas the same gap in a wider form.♻️ Proposed change
- } finally { - if (sentConditionalGet) inFlightConditionalGetKeys.delete(cacheKey); - } - - if (response.status === 304) { - const cached = etagCache.get(cacheKey); - if (cached) { - recordGithubCredentialSuccess(candidate, response.headers); - repositoryFallback.recordSuccess(candidate); - releaseGitHubResponse(response); - return { data: cached.data as T, response, linkHeader: cached.linkHeader }; - } - } + } catch (error) { + if (sentConditionalGet) inFlightConditionalGetKeys.delete(cacheKey); + throw error; + } + + if (response.status === 304) { + const cached = etagCache.get(cacheKey); + if (sentConditionalGet) inFlightConditionalGetKeys.delete(cacheKey); + if (cached) { + recordGithubCredentialSuccess(candidate, response.headers); + repositoryFallback.recordSuccess(candidate); + releaseGitHubResponse(response); + return { data: cached.data as T, response, linkHeader: cached.linkHeader }; + } + releaseGitHubResponse(response); + etagCache.delete(cacheKey); + continue; + } + if (sentConditionalGet) inFlightConditionalGetKeys.delete(cacheKey);The
continueretries the same candidate list from the start of the loop only if you re-enter it; if you prefer a direct retry, extract the request into a local function and call it once without the conditional header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/github/githubService.ts` around lines 1269 - 1277, Update the conditional GitHub request flow around inFlightConditionalGetKeys and the 304 cache lookup so the key remains protected through reading etagCache. If no cached entry exists for a 304 response, retry the same request without the if-none-match header (or otherwise re-enter the candidate loop) instead of passing 304 to response.text() and the HTTP error path; apply the equivalent fix in the headless implementation.apps/desktop/src/main/services/automations/automationIngressService.test.ts (1)
1218-1224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure cooldown.
The new tests cover relay health for the configured and unconfigured paths. They do not cover the new cooldown logic: the exponential backoff at lines 1019-1027 of
automationIngressService.ts, theRetry-Afterextension, andpollNow()clearing the cooldown at line 1105. Those branches gate all background polling, so a regression there stops relay ingest silently.A test that returns
500with aRetry-Afterheader, asserts that the nextpollGithubRelayOnceissues no fetch, and then asserts thatpollNow()does issue one would cover the whole path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/automations/automationIngressService.test.ts` around lines 1218 - 1224, Add tests for the failure cooldown in the relay polling suite around pollGithubRelayOnce and pollNow: mock a 500 response with a Retry-After header, verify the initial failure sets cooldown and the next pollGithubRelayOnce performs no fetch, then call pollNow() and verify it clears the cooldown and performs exactly one fetch.apps/desktop/src/main/services/github/githubCredentialHealth.test.ts (1)
35-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this test into focused cases.
This single
itasserts shared cooldowns, resource-scoped cooldowns, background pause reserve, account filtering, health reset, andignoreNonRateLimit. It callsclearGithubCredentialHealth()four times mid-test to reset state between phases. EachclearGithubCredentialHealth()boundary marks a natural test boundary. Separateitblocks would identify which behavior regressed when the suite fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/github/githubCredentialHealth.test.ts` around lines 35 - 146, Split the oversized test around the existing clearGithubCredentialHealth() boundaries into focused it blocks covering shared cooldowns, background reserve and resource scoping, account filtering, and non-rate-limit recovery. Move each phase’s setup and assertions into its own test, retaining the relevant fake-timer setup and cleanup so each case remains isolated and failures identify the affected behavior.apps/desktop/src/main/services/prs/prService.ts (1)
4215-4215: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake mutation capability explicit in
graphqlRequest.
graphqlRequestcurrently infers"write"by matchingmutationat the start of the operation text. Addoptions.capability?: "read" | "write"and pass"write"from the mutation call sites so the credential routing does not depend on comment, fragment, or naming edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` at line 4215, Update graphqlRequest to accept an optional options.capability value of "read" or "write", and use it when determining credential routing. Change mutation call sites to pass capability: "write", while preserving read as the default for other requests and removing reliance on the query-text mutation regex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/headlessLinearServices.test.ts`:
- Around line 881-906: Update the test around the environment setup and cleanup
in the headless 404 fallback case to isolate credential sources: save and clear
ADE_GITHUB_TOKEN, GITHUB_TOKEN, and GH_TOKEN, set GH_CONFIG_DIR to an isolated
value, and restore all original values in the existing finally block alongside
ADE_HOME and globalThis.fetch. Preserve the test’s strict authorization
assertions.
In `@apps/ade-cli/src/headlessLinearServices.ts`:
- Around line 1087-1103: Protect conditional GET cache entries in the GitHub
request flow around fetchGitHub by tracking cache keys currently awaiting
responses, excluding those keys from the eviction loop, and removing them when
the request completes. When a 304 response has no cached entry, treat it as a
retryable cache miss rather than allowing it to fall through as an HTTP error,
matching the existing inFlightConditionalGetKeys behavior used by the desktop
implementation.
In `@apps/desktop/src/main/services/automations/automationIngressService.ts`:
- Around line 1110-1113: Update the stop() method to reset
relayPollCooldownUntilMs and relayPollFailureCount alongside githubRelayHealthy,
ensuring a subsequent start() performs its initial poll immediately rather than
honoring stale failure state.
- Around line 1019-1030: Clamp the server-provided retry timestamp in the
cooldown calculation around relayPollFailureCount and relayPollCooldownUntilMs
so it cannot extend the cooldown beyond GITHUB_RELAY_POLL_BACKOFF_CAP_MS from
the current time. Preserve the local exponential backoff and ensure both
scheduled polling and the github_delivery path through pollGithubRelayOnce
observe the capped cooldown.
In `@apps/desktop/src/main/services/github/githubRateLimit.ts`:
- Around line 127-176: Update the GraphQL payload guard in
classifyGitHubGraphqlCredentialFailure to return null when the response contains
usable data, while preserving classification for error-only payloads. Validate
the data field as non-null/usable before extracting errors so neither the
permissionDenied nor rateLimited branches can downgrade a successful partial
response.
In `@apps/desktop/src/main/services/prs/prPollingService.ts`:
- Around line 252-254: Update the background pause selection in the PR polling
flow to choose the project-scoped callback whenever
getGithubBackgroundPauseUntilMs is provided, even when it resolves to null; only
call githubBackgroundRequestPauseUntilMs when the callback is absent. Preserve
the existing Promise resolution and ensure unrelated credential cooldowns cannot
affect project-scoped polling.
- Around line 306-310: Update the relay safety sweep in the polling flow around
prService.refresh() to record lastRelaySafetySweepAtMs before awaiting the
refresh, ensuring failed attempts advance the interval. Also adjust the final
delay selection so accumulated consecutiveFailures backoff is preferred over the
relay safety-sweep delay, while preserving the existing minimum delay and
healthy-relay behavior.
In `@apps/desktop/src/renderer/lib/githubIntegrationStatus.ts`:
- Around line 263-270: Update the status handling around writeAuthSource so a
connected GitHub App with writeAuthSource omitted is treated the same as
writeAuthSource === "none", returning the "no-write-credential" state and its
existing write-access messaging. Add a test covering authSource: "app",
connected: true, and an omitted writeAuthSource.
In `@apps/webhook-relay/src/relay.ts`:
- Around line 1525-1530: The account-token authorization path must terminate
when the account has no matching repository binding. In
apps/webhook-relay/src/relay.ts lines 1525-1530, update the flow after
authenticateAccount and githubRepositoryAccountMatches so a present accountId
with no match returns an authorization failure before
assertGitHubRepoAuthorized. In apps/webhook-relay/test/account.test.ts lines
674-679, include a valid GitHub bearer token alongside the ADE account token and
assert a 401 response with no provider calls.
---
Nitpick comments:
In `@apps/desktop/src/main/services/automations/automationIngressService.test.ts`:
- Around line 1218-1224: Add tests for the failure cooldown in the relay polling
suite around pollGithubRelayOnce and pollNow: mock a 500 response with a
Retry-After header, verify the initial failure sets cooldown and the next
pollGithubRelayOnce performs no fetch, then call pollNow() and verify it clears
the cooldown and performs exactly one fetch.
In `@apps/desktop/src/main/services/github/githubCredentialHealth.test.ts`:
- Around line 35-146: Split the oversized test around the existing
clearGithubCredentialHealth() boundaries into focused it blocks covering shared
cooldowns, background reserve and resource scoping, account filtering, and
non-rate-limit recovery. Move each phase’s setup and assertions into its own
test, retaining the relevant fake-timer setup and cleanup so each case remains
isolated and failures identify the affected behavior.
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 1269-1277: Update the conditional GitHub request flow around
inFlightConditionalGetKeys and the 304 cache lookup so the key remains protected
through reading etagCache. If no cached entry exists for a 304 response, retry
the same request without the if-none-match header (or otherwise re-enter the
candidate loop) instead of passing 304 to response.text() and the HTTP error
path; apply the equivalent fix in the headless implementation.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Line 4215: Update graphqlRequest to accept an optional options.capability
value of "read" or "write", and use it when determining credential routing.
Change mutation call sites to pass capability: "write", while preserving read as
the default for other requests and removing reliance on the query-text mutation
regex.
In `@apps/desktop/src/shared/githubOperationCredential.test.ts`:
- Around line 19-56: Add a test alongside “resolves read fallback and validates
the selected writer once” covering a single read candidate whose probe reports
repository access failure. Use readCandidates: [app] with no writable candidate,
then assert active.candidate.source is "app", activeWriteSource is null, and
onAcceptedProbe is called with false as its third argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d0e94491-0e9e-4929-a1d4-7a24099cd853
⛔ Files ignored due to path filters (3)
docs/features/automations/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (31)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/automations/automationIngressService.test.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/main/services/github/githubCredentialHealth.test.tsapps/desktop/src/main/services/github/githubCredentialHealth.tsapps/desktop/src/main/services/github/githubRateLimit.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/lib/githubIntegrationStatus.test.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.tsapps/desktop/src/shared/githubApiPath.test.tsapps/desktop/src/shared/githubApiPath.tsapps/desktop/src/shared/githubOperationCredential.test.tsapps/desktop/src/shared/githubOperationCredential.tsapps/desktop/src/shared/types/git.tsapps/webhook-relay/src/relay.tsapps/webhook-relay/test/account.test.ts
| if ( | ||
| !payload | ||
| || typeof payload !== "object" | ||
| || !("errors" in payload) | ||
| || !Array.isArray(payload.errors) | ||
| ) return null; | ||
| const errors: unknown[] = payload.errors; | ||
| const messages = errors.flatMap((error) => { | ||
| if (!error || typeof error !== "object") return []; | ||
| const message = "message" in error ? error.message : null; | ||
| return typeof message === "string" && message.trim() ? [message.trim()] : []; | ||
| }); | ||
| const errorTypes = errors.flatMap((error) => { | ||
| if (!error || typeof error !== "object") return []; | ||
| const extensions = "extensions" in error | ||
| && error.extensions | ||
| && typeof error.extensions === "object" | ||
| ? error.extensions | ||
| : null; | ||
| return [ | ||
| "type" in error ? error.type : null, | ||
| extensions && "code" in extensions ? extensions.code : null, | ||
| extensions && "type" in extensions ? extensions.type : null, | ||
| ] | ||
| .filter((value): value is string => typeof value === "string") | ||
| .map((value) => value.toUpperCase()); | ||
| }); | ||
| const message = messages.join("; ") || "GitHub GraphQL request failed."; | ||
| const rateLimit = readGitHubRateLimitState(headers); | ||
| const rateLimited = rateLimit?.remaining === 0 | ||
| || errorTypes.some((type) => type === "RATE_LIMITED" || type === "RATE_LIMIT") | ||
| || /rate limit|too many requests|abuse detection/i.test(message); | ||
| if (rateLimited) { | ||
| return { | ||
| status: 429, | ||
| message, | ||
| ...classifyGitHubAuthFailure({ status: 429, message, headers }), | ||
| }; | ||
| } | ||
| const permissionDenied = errorTypes.includes("FORBIDDEN") | ||
| || /forbidden|resource not accessible|not accessible by integration|does not have access/i.test(message); | ||
| if (permissionDenied) { | ||
| return { | ||
| status: 403, | ||
| message, | ||
| ...classifyGitHubAuthFailure({ status: 403, message, headers }), | ||
| }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Ignore GraphQL payloads that still carry usable data.
The guard only requires an errors array. GitHub returns partial responses that contain both data and per-field errors, for example a FORBIDDEN error on one node of an otherwise successful query. In that case:
- The
permissionDeniedbranch matches onFORBIDDEN, soclassifyGitHubGraphqlCredentialFailurereturns a non-null result. githubService.ts(Lines 1341-1362) then callsrecordGithubCredentialFailure, which places a working credential on the 5-minuteFALLBACK_COOLDOWN_MS, retries the next credential, and finally throws even though the query returned data.
The same applies to the rateLimited branch when x-ratelimit-remaining is 0 on a successful query that carries a single field error.
Restrict classification to payloads without usable data.
🛠️ Proposed guard for partial responses
if (
!payload
|| typeof payload !== "object"
|| !("errors" in payload)
|| !Array.isArray(payload.errors)
) return null;
+ // GitHub returns partial GraphQL responses that carry both `data` and
+ // per-field `errors`. Those responses are usable, so they must not put the
+ // credential on cooldown or trigger a fallback retry.
+ const data = "data" in payload ? (payload as { data?: unknown }).data : null;
+ if (data != null) return null;
const errors: unknown[] = payload.errors;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !payload | |
| || typeof payload !== "object" | |
| || !("errors" in payload) | |
| || !Array.isArray(payload.errors) | |
| ) return null; | |
| const errors: unknown[] = payload.errors; | |
| const messages = errors.flatMap((error) => { | |
| if (!error || typeof error !== "object") return []; | |
| const message = "message" in error ? error.message : null; | |
| return typeof message === "string" && message.trim() ? [message.trim()] : []; | |
| }); | |
| const errorTypes = errors.flatMap((error) => { | |
| if (!error || typeof error !== "object") return []; | |
| const extensions = "extensions" in error | |
| && error.extensions | |
| && typeof error.extensions === "object" | |
| ? error.extensions | |
| : null; | |
| return [ | |
| "type" in error ? error.type : null, | |
| extensions && "code" in extensions ? extensions.code : null, | |
| extensions && "type" in extensions ? extensions.type : null, | |
| ] | |
| .filter((value): value is string => typeof value === "string") | |
| .map((value) => value.toUpperCase()); | |
| }); | |
| const message = messages.join("; ") || "GitHub GraphQL request failed."; | |
| const rateLimit = readGitHubRateLimitState(headers); | |
| const rateLimited = rateLimit?.remaining === 0 | |
| || errorTypes.some((type) => type === "RATE_LIMITED" || type === "RATE_LIMIT") | |
| || /rate limit|too many requests|abuse detection/i.test(message); | |
| if (rateLimited) { | |
| return { | |
| status: 429, | |
| message, | |
| ...classifyGitHubAuthFailure({ status: 429, message, headers }), | |
| }; | |
| } | |
| const permissionDenied = errorTypes.includes("FORBIDDEN") | |
| || /forbidden|resource not accessible|not accessible by integration|does not have access/i.test(message); | |
| if (permissionDenied) { | |
| return { | |
| status: 403, | |
| message, | |
| ...classifyGitHubAuthFailure({ status: 403, message, headers }), | |
| }; | |
| } | |
| return null; | |
| } | |
| if ( | |
| !payload | |
| || typeof payload !== "object" | |
| || !("errors" in payload) | |
| || !Array.isArray(payload.errors) | |
| ) return null; | |
| // GitHub returns partial GraphQL responses that carry both `data` and | |
| // per-field `errors`. Those responses are usable, so they must not put the | |
| // credential on cooldown or trigger a fallback retry. | |
| const data = "data" in payload ? (payload as { data?: unknown }).data : null; | |
| if (data != null) return null; | |
| const errors: unknown[] = payload.errors; | |
| const messages = errors.flatMap((error) => { | |
| if (!error || typeof error !== "object") return []; | |
| const message = "message" in error ? error.message : null; | |
| return typeof message === "string" && message.trim() ? [message.trim()] : []; | |
| }); | |
| const errorTypes = errors.flatMap((error) => { | |
| if (!error || typeof error !== "object") return []; | |
| const extensions = "extensions" in error | |
| && error.extensions | |
| && typeof error.extensions === "object" | |
| ? error.extensions | |
| : null; | |
| return [ | |
| "type" in error ? error.type : null, | |
| extensions && "code" in extensions ? extensions.code : null, | |
| extensions && "type" in extensions ? extensions.type : null, | |
| ] | |
| .filter((value): value is string => typeof value === "string") | |
| .map((value) => value.toUpperCase()); | |
| }); | |
| const message = messages.join("; ") || "GitHub GraphQL request failed."; | |
| const rateLimit = readGitHubRateLimitState(headers); | |
| const rateLimited = rateLimit?.remaining === 0 | |
| || errorTypes.some((type) => type === "RATE_LIMITED" || type === "RATE_LIMIT") | |
| || /rate limit|too many requests|abuse detection/i.test(message); | |
| if (rateLimited) { | |
| return { | |
| status: 429, | |
| message, | |
| ...classifyGitHubAuthFailure({ status: 429, message, headers }), | |
| }; | |
| } | |
| const permissionDenied = errorTypes.includes("FORBIDDEN") | |
| || /forbidden|resource not accessible|not accessible by integration|does not have access/i.test(message); | |
| if (permissionDenied) { | |
| return { | |
| status: 403, | |
| message, | |
| ...classifyGitHubAuthFailure({ status: 403, message, headers }), | |
| }; | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/services/github/githubRateLimit.ts` around lines 127 -
176, Update the GraphQL payload guard in classifyGitHubGraphqlCredentialFailure
to return null when the response contains usable data, while preserving
classification for error-only payloads. Validate the data field as
non-null/usable before extracting errors so neither the permissionDenied nor
rateLimited branches can downgrade a successful partial response.
b64b653 to
0bd05d3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bd05d3dc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async getGitTransportTokenOrThrowAsync(): Promise<string> { | ||
| const token = (await readAuthToken("read", "non-rate-limit-only")).token; | ||
| if (!token) throw new Error("GitHub auth missing. Run `gh auth login -h github.com -s repo -s workflow` or add a personal access token in Settings."); |
There was a problem hiding this comment.
When the user has authorized the ADE GitHub App and also has a healthy gh/PAT credential, this read-capability lookup selects the App before those credentials. The configured App permissions in apps/webhook-relay/README.md:289-295 omit Contents, so authenticated private-repository clones, fetches, remote-clone preflights, and cross-machine handoffs receive an unusable token instead of the Git-capable fallback. Use a transport-specific candidate set that excludes the read-only App (and mirror the correction in the headless implementation at apps/ade-cli/src/headlessLinearServices.ts:1795-1796).
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (accountId) { | ||
| if (await githubRepositoryAccountMatches(env, repo, accountId)) { | ||
| return { authorized: true, accountId }; | ||
| } | ||
| return { | ||
| authorized: false, | ||
| response: json({ ok: false, error: "unauthorized" }, { status: 401 }), | ||
| }; |
There was a problem hiding this comment.
For a signed-in headless runtime whose pre-existing installed-repository row still has account_id = null, startup goes directly to /events and sends both the ADE account token and a valid GitHub App bearer token, but this branch returns 401 before either validating the bearer or associating the row. Since headless bootstrap does not call /status first, every retry follows the same path, so relay-backed PR updates and automation events remain disabled indefinitely; authorize and bind this unassociated case, or fall back to the supplied GitHub credential.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (!run.isCurrent()) return; | ||
| if (Date.now() < relayPollCooldownUntilMs) return; |
There was a problem hiding this comment.
When an HTTP drain fails while the WebSocket remains connected, the interval is still the five-minute connected safety interval, but entering cooldown only stores a timestamp and schedules no one-shot retry. Wake-up frames arriving during the nominal 30-second backoff hit this early return and are discarded, so a transient 5xx can delay automation delivery for nearly five minutes instead of retrying when the advertised cooldown expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
apps/desktop/src/main/services/prs/prAsync.test.ts (1)
284-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the jump from one to three
listAllcalls.The assertion expects
listAllto reach 3 immediately after the 10-second boundary, which means two calls occur in the same tick. That count encodes an internal detail of the list-check path. A reader cannot tell whether 3 is intended or incidental, and a refactor that merges the two calls would fail this test for no functional reason.Add a short comment that states which two code paths call
listAllat that boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prAsync.test.ts` around lines 284 - 288, Add a concise comment immediately before the post-10-second listAll call-count assertion explaining that the count increases from one to three because both relevant list-check code paths invoke listAll in the same timer tick. Keep the existing timing and assertions unchanged.apps/desktop/src/main/services/automations/automationIngressService.ts (1)
941-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTruncate the relay error body before it becomes an error message.
responseMessagetakes the whole response text when the body is not JSON. A relay behind a proxy can return a large HTML error page. That string then flows intologger.warnat line 1076 and intolastErrorin the relay status, which the renderer displays.Cap the length before building the error.
♻️ Proposed truncation
- let responseMessage = responseText.trim(); + let responseMessage = responseText.trim().slice(0, 500);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/automations/automationIngressService.ts` around lines 941 - 960, In the non-OK response handling around the GitHub relay poll error construction, truncate responseMessage to a bounded length before interpolating it into GithubRelayPollError. Apply the cap after selecting either the parsed error/message or plain-text body, while preserving the status and retry metadata and ensuring logger.warn and relay status receive only the truncated message.apps/ade-cli/src/headlessLinearServices.test.ts (1)
1106-1181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp directories from
fs.mkdtempSyncare never removed.Line 1112 creates a temp
ADE_HOMEand thefinallyblock only restores the environment variable. Each run of this file leaves several directories in the OS temp directory, each holding an encrypted credential file. Remove the directory in thefinallyblock, or add the cleanup toisolateHeadlessGithubAuth.restoreso every test benefits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/headlessLinearServices.test.ts` around lines 1106 - 1181, Clean up the temporary ADE_HOME directory created by fs.mkdtempSync in the “drops a cached headless writer when that credential disappears” test. Track the created directory and remove it during finally, or extend isolateHeadlessGithubAuth.restore to perform this cleanup for all applicable tests while preserving environment restoration.apps/desktop/src/shared/githubConditionalRequestCache.ts (1)
43-55: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEviction uses insertion order, not recency.
storeevicts the first non-active key in insertion order.getandbegindo not move a key to the end, andentries.seton an existing key keeps its original position. A frequently reused ETag entry can therefore be evicted before a cold one, which costs extra unconditional requests against the rate limit.To make this an LRU, delete the key before re-inserting it and refresh the position on a cache hit.
♻️ Optional LRU adjustment
store(key: string, entry: GithubConditionalRequestCacheEntry): void { while (entries.size >= maxSize && !entries.has(key)) { let evictable: string | null = null; for (const candidate of entries.keys()) { if (activeConditionalRequests.has(candidate)) continue; evictable = candidate; break; } if (!evictable) break; entries.delete(evictable); } + entries.delete(key); entries.set(key, entry); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/githubConditionalRequestCache.ts` around lines 43 - 55, Update the cache methods around store, get, and begin to maintain true LRU ordering: delete an existing key before re-inserting it in store, and refresh a key’s insertion position on cache hits in get and begin. Preserve active-request eviction behavior while ensuring frequently reused entries move to the end and cold entries are evicted first.apps/desktop/src/shared/githubOperationCredential.ts (1)
216-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider recording the write probe value only for successful probes.
At Line 220 the write loop reuses
successfulProbes. The read loop at Line 201 also stores a probe value that came from a failed probe (the repository-access fallback acceptance). A later write candidate with the same token then takes theok: truepath with a probe value whoserepoAccessOkisfalse. For fine-grained and App credentialscapabilities()still returnswrite: false, so the current effect is bounded, but the map name and the reuse contract no longer match.Store the accepted-but-unvalidated probe in a separate map, or record the
validatedflag with the value so the write phase can distinguish the two cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/githubOperationCredential.ts` around lines 216 - 236, Separate repository-access fallback probes from validated entries in the shared probe cache used by the read and write loops. Update the read-phase recording and write-phase lookup around successfulProbes so a write candidate only reuses probe values from genuinely successful probes, while accepted-but-unvalidated values are tracked separately or marked with their validation state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/github/githubService.ts`:
- Around line 760-816: Memoize the inventory produced by readCredentialInventory
with a short TTL or credential-change signal, and invalidate that memoized value
wherever cachedStatusCredentialInventoryKey is reset: setToken, clearToken,
clearAppUserAuth, and pollAppUserDeviceAuth. In
apps/desktop/src/main/services/github/githubService.ts lines 760-816, update the
inventory builder and invalidation paths; in lines 2148-2155, read the memoized
inventory rather than rebuilding it on each polling tick.
- Around line 1393-1412: Update the exhaustion selection in the
credential-attempt flow around `lastAttemptError` and `firstUnavailable` to
prefer a recorded rate-limit failure over a later non-rate-limit error. Preserve
the existing `GitHubRateLimitError` construction and fallback behavior, ensuring
callers receive the rate-limit signal whenever any credential was unavailable
due to cooldown.
- Around line 2165-2203: Update the credential-reset paths in setToken,
clearToken, clearAppUserAuth, and pollAppUserDeviceAuth so
clearGithubCredentialHealth receives the affected token instead of clearing all
credential health state. Preserve the existing cache invalidation behavior, and
pass the token associated with each changed authentication source, including the
authorized device-auth result.
- Around line 1021-1033: Restrict the negative repository access recording in
the authFailure handling near classifyGitHubAuthFailure to credential failures
that indicate this specific repository is inaccessible: response status 404 or a
repository-specific access-denial message. Do not call
recordGithubCredentialRepositoryAccess(candidate, repo, false) for generic 403
cases such as scope, SAML/SSO, or IP allow-list failures, while preserving the
existing permission_denied classification for other handling.
In `@apps/desktop/src/main/services/prs/prAsync.test.ts`:
- Around line 15-18: Move the clearGithubCredentialHealth cleanup out of the
prPollingService-scoped afterEach into a file-level teardown in prAsync.test.ts,
ensuring it uses the same credential token as recordGithubCredentialSuccess() so
module-level health state is reset for every test.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 4209-4218: Update graphqlRequest to accept an optional repo
argument and prefer it when constructing the GraphQL request context, while
retaining the owner/name-derived behavior for existing callers. Update the
review-thread reply, thread resolve/unresolve, and reaction mutation callers to
pass their owning repository alongside node IDs, and add coverage for
repository-scoped write-token failure followed by a writable fallback token.
- Line 5750: Update downloadJobLogTail to use a GitHub-service request helper
instead of obtaining one token and issuing raw fetch calls directly. The helper
must preserve the existing manual redirect behavior, classify 401, 403, and 429
responses, cool down the failed credential, and retry with another read-capable
credential before returning null.
---
Nitpick comments:
In `@apps/ade-cli/src/headlessLinearServices.test.ts`:
- Around line 1106-1181: Clean up the temporary ADE_HOME directory created by
fs.mkdtempSync in the “drops a cached headless writer when that credential
disappears” test. Track the created directory and remove it during finally, or
extend isolateHeadlessGithubAuth.restore to perform this cleanup for all
applicable tests while preserving environment restoration.
In `@apps/desktop/src/main/services/automations/automationIngressService.ts`:
- Around line 941-960: In the non-OK response handling around the GitHub relay
poll error construction, truncate responseMessage to a bounded length before
interpolating it into GithubRelayPollError. Apply the cap after selecting either
the parsed error/message or plain-text body, while preserving the status and
retry metadata and ensuring logger.warn and relay status receive only the
truncated message.
In `@apps/desktop/src/main/services/prs/prAsync.test.ts`:
- Around line 284-288: Add a concise comment immediately before the
post-10-second listAll call-count assertion explaining that the count increases
from one to three because both relevant list-check code paths invoke listAll in
the same timer tick. Keep the existing timing and assertions unchanged.
In `@apps/desktop/src/shared/githubConditionalRequestCache.ts`:
- Around line 43-55: Update the cache methods around store, get, and begin to
maintain true LRU ordering: delete an existing key before re-inserting it in
store, and refresh a key’s insertion position on cache hits in get and begin.
Preserve active-request eviction behavior while ensuring frequently reused
entries move to the end and cold entries are evicted first.
In `@apps/desktop/src/shared/githubOperationCredential.ts`:
- Around line 216-236: Separate repository-access fallback probes from validated
entries in the shared probe cache used by the read and write loops. Update the
read-phase recording and write-phase lookup around successfulProbes so a write
candidate only reuses probe values from genuinely successful probes, while
accepted-but-unvalidated values are tracked separately or marked with their
validation state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd1bf6a8-c38b-4357-b5d5-ae3726465875
⛔ Files ignored due to path filters (3)
docs/features/automations/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (37)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/automations/automationIngressService.test.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/main/services/github/githubCredentialHealth.test.tsapps/desktop/src/main/services/github/githubCredentialHealth.tsapps/desktop/src/main/services/github/githubRateLimit.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/lib/githubIntegrationStatus.test.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.tsapps/desktop/src/shared/githubApiPath.test.tsapps/desktop/src/shared/githubApiPath.tsapps/desktop/src/shared/githubConditionalRequestCache.test.tsapps/desktop/src/shared/githubConditionalRequestCache.tsapps/desktop/src/shared/githubOperationCredential.test.tsapps/desktop/src/shared/githubOperationCredential.tsapps/desktop/src/shared/types/git.tsapps/webhook-relay/src/relay.tsapps/webhook-relay/test/account.test.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- apps/ade-cli/src/bootstrap.ts
- apps/webhook-relay/test/account.test.ts
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx
- apps/ade-cli/README.md
- apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts
- apps/webhook-relay/src/relay.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.test.ts
- apps/desktop/src/renderer/components/app/FeedbackReporterModal.tsx
- apps/desktop/src/shared/types/git.ts
- apps/desktop/src/shared/githubOperationCredential.test.ts
- apps/desktop/src/renderer/lib/githubIntegrationStatus.ts
- apps/desktop/src/renderer/components/settings/GitHubSection.tsx
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx
- apps/desktop/src/main/services/github/githubCredentialHealth.ts
- apps/desktop/src/main/services/prs/prService.test.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/main/services/github/githubRateLimit.ts
- apps/desktop/src/shared/githubApiPath.ts
- apps/desktop/src/main/services/prs/prPollingService.ts
- apps/desktop/src/main/main.ts
- apps/ade-cli/src/cli.ts
- apps/ade-cli/src/headlessLinearServices.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a0cc1101
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| active = { candidate, value: result.value }; | ||
| successfulProbes.set(candidate.token, result.value); | ||
| args.onAcceptedProbe(candidate, result.value, true); | ||
| break; |
There was a problem hiding this comment.
When a higher-precedence classic/OAuth credential authenticates at /user but lacks the repo scope, its probe is ok, so this assignment stops the read loop even though capabilities(candidate, result.value).read is false. computeStatus then reports connected: false and never selects a healthy App, CLI, or PAT later in the chain; check read capability before accepting the probe in the shared resolver so both desktop and headless status paths fall through.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (args.status === 403) { | ||
| return { | ||
| rateLimit, | ||
| authFailure: { | ||
| kind: "permission_denied", | ||
| message, | ||
| retryAt: null, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
When a valid credential receives an operation-level 403, such as a repository or organization policy rejecting a particular write, this unconditional classification makes apiRequest record a credential failure and place the token in a five-minute core cooldown. With one credential—or when fallback credentials receive the same policy response—unrelated PR reads, writes, and Git transport are then treated as unavailable even though authentication remains valid; only credential-specific permission failures should poison credential health. This shared classifier affects both desktop and headless request paths.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
27a0cc1 to
7703ee2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4de66e057d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); | ||
| res.end("<!doctype html><html><body style=\"font-family:Geist,-apple-system,BlinkMacSystemFont,sans-serif;padding:24px\">Linear connected. You can close this window and return to ADE.</body></html>"); | ||
| } catch (error) { | ||
| if (session.status !== "pending") return; |
There was a problem hiding this comment.
When two callback requests reach the listener concurrently, the first successful exchange marks the session completed and aborts the shared controller; the second handler then reaches this guard and returns without calling res.end() or destroying the response. Because completed sessions only invoke graceful server.close() rather than closeAllConnections(), that request remains open indefinitely and prevents the old callback server from fully closing; explicitly finish or destroy the response before returning from terminal-session paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/cto/linearOAuthService.ts (1)
505-521: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
dispose()releases the fixed callback port asynchronously without an awaitable signal.dispose()starts every server close withvoidand returns synchronously. The callback server binds the fixed portOAUTH_PORT, and the port is released only after the close callback runs on a later event-loop turn. Any caller that disposes the service and then creates a replacement can fail its firststartSessionwithEADDRINUSE.
apps/desktop/src/main/services/cto/linearOAuthService.ts#L505-L521: collect thecloseServerAndWaitandfinalizeSessionpromises and returnPromise.all(...)fromdispose(), then update theLinearOAuthServicetype and every call site to await it.apps/desktop/src/main/services/cto/linearAuth.test.ts#L722-L724: the replacement service binds port 19836 immediately after the rejection, so the assertion depends on the old listener unbinding within a few microtask turns. Await the newdispose()promise before you create the replacement service, or poll until the port is free.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/cto/linearOAuthService.ts` around lines 505 - 521, The synchronous dispose flow leaves the fixed OAuth callback port unavailable until asynchronous cleanup completes. In apps/desktop/src/main/services/cto/linearOAuthService.ts#L505-L521, update LinearOAuthService.dispose and its implementation to collect closeServerAndWait and finalizeSession promises, return Promise.all, and update every caller to await it. In apps/desktop/src/main/services/cto/linearAuth.test.ts#L722-L724, await the replacement service’s new dispose promise before creating the replacement, or poll until port 19836 is released.apps/desktop/src/main/services/automations/automationIngressService.ts (1)
791-822: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSupersession thrown before the
tryblock escapes as an unhandled rejection.Line 805 calls
run.wait(...)outside thetryblock that starts at line 839. Ifstop()runs while the account token is being retrieved,run.waitthrowsGithubRelayPollSupersededError, and no local handler catches it.pollGithubRelayOnceawaitspollGithubRelaywithout acatch, so the rejection reachesvoid pollGithubRelayOnce()call sites in the poll timer, the retry timer, and the socket handlers as an unhandled rejection. It also rejectsstart()andpollNow()for the superseded run.Catch supersession in
pollGithubRelayOnceso every entry path stays quiet.🛡️ Proposed guard
pollInFlight = (async () => { do { pollRerunRequested = false; - await pollGithubRelay(relayPollGeneration); + try { + await pollGithubRelay(relayPollGeneration); + } catch (error) { + if (!(error instanceof GithubRelayPollSupersededError)) throw error; + } } while (pollRerunRequested && !stopped && Date.now() >= relayPollCooldownUntilMs); })().finally(() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/automations/automationIngressService.ts` around lines 791 - 822, Update pollGithubRelayOnce to catch and silently handle GithubRelayPollSupersededError from pollGithubRelay, including failures from run.wait during account-token retrieval. Ensure all callers—including start(), pollNow(), timers, and socket handlers—remain non-rejecting for superseded runs while preserving propagation of unrelated errors.
🧹 Nitpick comments (4)
apps/desktop/src/main/services/cto/linearAuth.test.ts (1)
711-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the microtask-count dependency with an explicit synchronization point.
await Promise.resolve()yields exactly one microtask turn. The test relies onstartSessionOncereachingserver.listenin that turn.startSessionOncecurrently reacheslistenafter a singleawait Promise.all([]), so the ordering holds today. If anyone adds or removes anawaitbeforelisten, the interleaving changes and the test either tests a different code path or fails for an unrelated reason.Make the intent explicit. Wait until the callback port is bound, then dispose.
♻️ Suggested test change
const interruptedStart = service.startSession(); - await Promise.resolve(); + // Dispose only after the callback port is bound, so the test exercises the + // post-listen disposal path rather than a specific microtask ordering. + await waitForPortBound(19836); service.dispose();Add a helper that polls the port:
async function waitForPortBound(port: number, timeoutMs = 2_000): Promise<void> { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { const bound = await new Promise<boolean>((resolve) => { const probe = net.createConnection({ host: "127.0.0.1", port }); probe.once("connect", () => { probe.destroy(); resolve(true); }); probe.once("error", () => resolve(false)); }); if (bound) return; await waitMs(5); } throw new Error(`Timed out waiting for port ${port} to bind`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/cto/linearAuth.test.ts` around lines 711 - 715, Replace the single Promise.resolve synchronization in the interrupted startSession test with an explicit waitForPortBound helper that probes the callback port until it accepts a connection or times out. Invoke it after service.startSession() and before service.dispose(), preserving the existing rejection assertion and using the test’s configured callback port.apps/desktop/src/main/services/github/githubRawRequest.ts (1)
169-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the error-selection priority.
The
exhaustedchain picks betweenfirstRateLimitError, a rate-limitedunavailableError,lastAttemptError, and a fallbackunavailableError. This order is correct, but it is dense and this exact logic decides which message surfaces to the user when multiple credentials fail differently. Add a short comment stating the precedence so a future change to this rate-limit-fallback path does not accidentally invert it.📝 Proposed comment addition
+ // Precedence: an in-flight rate-limit failure first, then a cooldown that + // was itself caused by a rate limit, then the last live attempt failure, + // then any remaining cooldown reason. const exhausted = firstRateLimitError ?? (unavailableError?.authFailure.kind === "rate_limited" ? unavailableError : null) ?? lastAttemptError ?? unavailableError;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/github/githubRawRequest.ts` around lines 169 - 183, Add a concise comment immediately above the `exhausted` assignment documenting its precedence: `firstRateLimitError`, then a rate-limited `unavailableError`, then `lastAttemptError`, and finally the general `unavailableError`. Leave the selection logic unchanged.apps/ade-cli/src/headlessLinearServices.test.ts (1)
934-1014: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo tests still use the manual environment setup that
isolateHeadlessGithubAuthreplaces. Both sites repeat the same save, delete, and restore sequence forADE_HOME,ADE_GITHUB_TOKEN,GITHUB_TOKEN,GH_TOKEN,GH_CONFIG_DIR, andglobalThis.fetch, and neither removes the temporary directories it creates. The new helper already performs both the isolation and the cleanup.
apps/ade-cli/src/headlessLinearServices.test.ts#L934-L1014: replace the manual setup withisolateHeadlessGithubAuth("ade-headless-github-graphql-", { emptyGhConfig: true })plusstoreHeadlessAppUserToken(), and replace thefinallybody withenvironment.restore().apps/ade-cli/src/headlessLinearServices.test.ts#L1375-L1448: replace the manual setup withisolateHeadlessGithubAuth("ade-headless-github-app-only-", { emptyGhConfig: true })plusstoreHeadlessAppUserToken(), and replace thefinallybody withenvironment.restore().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/headlessLinearServices.test.ts` around lines 934 - 1014, Replace the repeated environment and fetch setup in apps/ade-cli/src/headlessLinearServices.test.ts lines 934-1014 with isolateHeadlessGithubAuth("ade-headless-github-graphql-", { emptyGhConfig: true }) and storeHeadlessAppUserToken(), then use environment.restore() in finally. Apply the same replacement at lines 1375-1448 using the "ade-headless-github-app-only-" prefix; remove the duplicated manual cleanup in both tests.apps/desktop/src/main/services/prs/prService.ts (1)
10885-10967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate thread-ownership checks and support synthetic PR ids consistently.
replyToReviewThreadandresolveReviewThreadeach repeat the same thread-ownership check inline:requireRow+repoFromRow+fetchReviewThreads+ a manualsome()check. The same logic already exists asassertThreadBelongsToPr, andpostReviewCommentandsetReviewThreadResolvedalready use it.
assertThreadBelongsToPrresolves the target throughresolvePrThreadTarget, which supports both mapped PR rows and synthetic (unmapped GitHub-tab) PR ids.requireRowonly supports mapped rows and throws for a synthetic id. As a result,replyToReviewThreadandresolveReviewThreadcannot operate on unmapped GitHub-tab PRs, whilepostReviewComment,setReviewThreadResolved, andreactToCommentcan. Since this PR already touches all four methods to add the new{ repo }argument, switchreplyToReviewThreadandresolveReviewThreadto callassertThreadBelongsToPras well. This removes the duplicated logic and makes unmapped-PR support consistent across all review-thread mutations.♻️ Proposed refactor for `replyToReviewThread`
async replyToReviewThread(args: ReplyToPrReviewThreadArgs): Promise<PrReviewThreadComment> { - const row = requireRow(args.prId); - const repo = repoFromRow(row); - const threads = await fetchReviewThreads(repo, Number(row.github_pr_number)); - if (!threads.some((t) => t.id === args.threadId)) { - throw new Error(`Thread ${args.threadId} does not belong to PR ${args.prId}`); - } + const { repo } = await assertThreadBelongsToPr(args.prId, args.threadId); const data = await graphqlRequest<{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 10885 - 10967, Replace the duplicated requireRow/repoFromRow/fetchReviewThreads ownership checks in replyToReviewThread and resolveReviewThread with the existing assertThreadBelongsToPr helper, passing the PR id and thread id as required. Use the helper’s resolved target for the subsequent GraphQL request so both mapped and synthetic PR ids remain supported, while preserving the existing reply and resolve mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/cto/linearOAuthService.ts`:
- Around line 73-81: Update finalizeSession so every terminal status closes the
OAuth server’s connections, not only the expired status. After the completed or
failed response is written, use closeIdleConnections() before
closeServerAndWait(server), or consistently use closeAllConnections() after
writing the response; preserve the existing expired-session behavior and ensure
dispose() cannot remain blocked by keep-alive connections.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 2066-2069: Update the stale comment inside markHotRefresh’s
uniquePrIds loop to describe re-arming the 15-second loop instead of the
obsolete 5-second interval, while preserving the existing quota-avoidance
rationale.
---
Outside diff comments:
In `@apps/desktop/src/main/services/automations/automationIngressService.ts`:
- Around line 791-822: Update pollGithubRelayOnce to catch and silently handle
GithubRelayPollSupersededError from pollGithubRelay, including failures from
run.wait during account-token retrieval. Ensure all callers—including start(),
pollNow(), timers, and socket handlers—remain non-rejecting for superseded runs
while preserving propagation of unrelated errors.
In `@apps/desktop/src/main/services/cto/linearOAuthService.ts`:
- Around line 505-521: The synchronous dispose flow leaves the fixed OAuth
callback port unavailable until asynchronous cleanup completes. In
apps/desktop/src/main/services/cto/linearOAuthService.ts#L505-L521, update
LinearOAuthService.dispose and its implementation to collect closeServerAndWait
and finalizeSession promises, return Promise.all, and update every caller to
await it. In apps/desktop/src/main/services/cto/linearAuth.test.ts#L722-L724,
await the replacement service’s new dispose promise before creating the
replacement, or poll until port 19836 is released.
---
Nitpick comments:
In `@apps/ade-cli/src/headlessLinearServices.test.ts`:
- Around line 934-1014: Replace the repeated environment and fetch setup in
apps/ade-cli/src/headlessLinearServices.test.ts lines 934-1014 with
isolateHeadlessGithubAuth("ade-headless-github-graphql-", { emptyGhConfig: true
}) and storeHeadlessAppUserToken(), then use environment.restore() in finally.
Apply the same replacement at lines 1375-1448 using the
"ade-headless-github-app-only-" prefix; remove the duplicated manual cleanup in
both tests.
In `@apps/desktop/src/main/services/cto/linearAuth.test.ts`:
- Around line 711-715: Replace the single Promise.resolve synchronization in the
interrupted startSession test with an explicit waitForPortBound helper that
probes the callback port until it accepts a connection or times out. Invoke it
after service.startSession() and before service.dispose(), preserving the
existing rejection assertion and using the test’s configured callback port.
In `@apps/desktop/src/main/services/github/githubRawRequest.ts`:
- Around line 169-183: Add a concise comment immediately above the `exhausted`
assignment documenting its precedence: `firstRateLimitError`, then a
rate-limited `unavailableError`, then `lastAttemptError`, and finally the
general `unavailableError`. Leave the selection logic unchanged.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 10885-10967: Replace the duplicated
requireRow/repoFromRow/fetchReviewThreads ownership checks in
replyToReviewThread and resolveReviewThread with the existing
assertThreadBelongsToPr helper, passing the PR id and thread id as required. Use
the helper’s resolved target for the subsequent GraphQL request so both mapped
and synthetic PR ids remain supported, while preserving the existing reply and
resolve mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 61c6daf8-dcd6-476a-b382-5d9d56bcd67b
⛔ Files ignored due to path filters (3)
docs/features/automations/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (41)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/automations/automationIngressService.test.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearOAuthService.tsapps/desktop/src/main/services/github/githubAppUserAuthService.tsapps/desktop/src/main/services/github/githubCredentialHealth.test.tsapps/desktop/src/main/services/github/githubCredentialHealth.tsapps/desktop/src/main/services/github/githubRateLimit.tsapps/desktop/src/main/services/github/githubRawRequest.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/lib/githubIntegrationStatus.test.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.tsapps/desktop/src/shared/githubApiPath.test.tsapps/desktop/src/shared/githubApiPath.tsapps/desktop/src/shared/githubConditionalRequestCache.test.tsapps/desktop/src/shared/githubConditionalRequestCache.tsapps/desktop/src/shared/githubOperationCredential.test.tsapps/desktop/src/shared/githubOperationCredential.tsapps/desktop/src/shared/types/git.tsapps/webhook-relay/src/relay.tsapps/webhook-relay/test/account.test.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- apps/ade-cli/src/multiProjectRpcServer.ts
- apps/ade-cli/README.md
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx
- apps/ade-cli/src/cli.ts
- apps/desktop/src/shared/githubApiPath.test.ts
- apps/desktop/src/renderer/components/app/FeedbackReporterModal.tsx
- apps/desktop/src/main/services/github/githubCredentialHealth.test.ts
- apps/desktop/src/main/services/projects/projectScaffoldService.test.ts
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx
- apps/desktop/src/main/services/prs/prAsync.test.ts
- apps/desktop/src/main/services/projects/projectScaffoldService.ts
- apps/desktop/src/shared/githubConditionalRequestCache.ts
- apps/desktop/src/main/services/prs/prPollingService.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/renderer/lib/githubIntegrationStatus.ts
- apps/webhook-relay/src/relay.ts
- apps/desktop/src/main/services/github/githubRateLimit.ts
- apps/desktop/src/renderer/components/settings/GitHubSection.tsx
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.ts
- apps/desktop/src/shared/types/git.ts
- apps/desktop/src/shared/githubOperationCredential.ts
- apps/desktop/src/main/services/github/githubService.test.ts
- apps/ade-cli/src/headlessLinearServices.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5d44693a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const requestedResource = options.resource?.trim().toLowerCase() || null; | ||
| const entries = requestedResource | ||
| ? [health.resources.get(requestedResource), health.resources.get("unknown")] | ||
| : [...health.resources.values()]; |
There was a problem hiding this comment.
When a revoked credential returns 401 from GraphQL or Search with an x-ratelimit-resource header, its invalid_token cooldown is stored under that resource, but lookups for core inspect only core and unknown. Both desktop and headless Git-transport selectors consequently choose the known-invalid higher-priority token instead of the healthy fallback, causing clones, fetches, and handoffs to fail until another core request happens to diagnose the token; keep rate-limit cooldowns resource-specific, but apply invalid-token health across every resource.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (Date.now() - lastRelaySafetySweepAtMs >= RELAY_SAFETY_SWEEP_INTERVAL_MS) { | ||
| lastRelaySafetySweepAtMs = Date.now(); | ||
| await prService.refresh(); |
There was a problem hiding this comment.
When the relay is healthy and this periodic full refresh fails transiently, advancing lastRelaySafetySweepAtMs before awaiting the request suppresses the refresh on the backoff tick. That next tick processes stale cached PRs as a success, resets consecutiveFailures, and then waits out the remainder of the 15-minute safety interval, so missed relay events are not reconciled promptly; update the timestamp only after prService.refresh() succeeds. This shared poller is wired into both desktop and headless runtime paths.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f30b29508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| finalizeSession(session, { status: "completed" }); | ||
| res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); | ||
| res.end("<!doctype html><html><body style=\"font-family:Geist,-apple-system,BlinkMacSystemFont,sans-serif;padding:24px\">Linear connected. You can close this window and return to ADE.</body></html>"); | ||
| await exchangeCode(session, code, session.abortController.signal); |
There was a problem hiding this comment.
When two callbacks for the same session overlap, both observe status === "pending" and exchange the same single-use authorization code concurrently. If the duplicate exchange fails before the valid exchange completes, its catch path marks the session failed and aborts the shared controller, preventing the successful request from completing the sign-in; reserve the session synchronously before this await or share one in-flight exchange among callbacks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/prs/prService.ts (1)
11047-11063: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an ownership check before reacting to
args.commentId.
reactToCommentonly mapsargs.prIdtorepofor credential failover, then sendsargs.commentIddirectly toaddReaction. Comment/reaction subject IDs are global GitHub node IDs, so a caller can add a reaction to any subject the credential can target, independent of the PR the caller claims. Add a check that the comment belongs toargs.prIdbefore callingaddReaction, or document why reactions are intentionally allowed outside that scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 11047 - 11063, Add an ownership validation step in reactToComment to confirm args.commentId belongs to the pull request identified by args.prId before invoking addReaction. Reuse the existing PR/comment lookup or validation mechanism, and reject or return on mismatches while preserving the current repository-based credential failover behavior.Source: Path instructions
🧹 Nitpick comments (1)
apps/desktop/src/main/services/prs/prService.ts (1)
10885-10923: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvery review-thread mutation re-fetches the full thread list to check ownership.
replyToReviewThread(line 10886),resolveReviewThread(line 10942),postReviewComment(line 10960), andsetReviewThreadResolved(line 11006) each callassertThreadBelongsToPrfirst. That helper callsfetchReviewThreads, which pages through up to 10 pages of 100 threads, each with up to 50 nested comments, before the actual mutation runs. This adds one heavy GraphQL round trip to every single reply, resolve, comment, or resolve-toggle action.Use a targeted lookup instead, for example a
node(id: $threadId) { ... on PullRequestReviewThread { pullRequest { number repository { owner { login } name } } } }query, to confirm ownership without listing every thread on the PR.Also applies to: 10941-10957, 10959-11003, 11005-11045
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 10885 - 10923, Replace the full-list ownership check used by replyToReviewThread, resolveReviewThread, postReviewComment, and setReviewThreadResolved with a targeted thread-ID lookup via node, selecting the PullRequestReviewThread pullRequest repository identity and number. Validate that the returned thread belongs to args.prId and preserve the existing rejection behavior for missing or mismatched ownership, without calling fetchReviewThreads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Line 1640: Update the Linear OAuth service resolution flow around
linearOAuthServiceTransition so the same mutex serializes the complete check,
disposal, and creation sequence for each ctx.adeDir. Ensure callers wait before
evaluating or returning linearCredentialService, then re-check the requested
directory after acquiring the mutex, preventing one project from receiving
another project's service.
---
Outside diff comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 11047-11063: Add an ownership validation step in reactToComment to
confirm args.commentId belongs to the pull request identified by args.prId
before invoking addReaction. Reuse the existing PR/comment lookup or validation
mechanism, and reject or return on mismatches while preserving the current
repository-based credential failover behavior.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 10885-10923: Replace the full-list ownership check used by
replyToReviewThread, resolveReviewThread, postReviewComment, and
setReviewThreadResolved with a targeted thread-ID lookup via node, selecting the
PullRequestReviewThread pullRequest repository identity and number. Validate
that the returned thread belongs to args.prId and preserve the existing
rejection behavior for missing or mismatched ownership, without calling
fetchReviewThreads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03375040-2560-461c-a93a-afd664fc296a
📒 Files selected for processing (14)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/desktop/src/main/services/automations/automationIngressService.test.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearOAuthService.tsapps/desktop/src/main/services/github/githubCredentialHealth.test.tsapps/desktop/src/main/services/github/githubCredentialHealth.tsapps/desktop/src/main/services/github/githubRawRequest.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.test.ts
- apps/desktop/src/main/services/prs/prAsync.test.ts
- apps/desktop/src/main/services/github/githubRawRequest.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.ts
- apps/desktop/src/main/services/prs/prService.test.ts
- apps/ade-cli/src/headlessLinearServices.test.ts
- apps/desktop/src/main/services/cto/linearOAuthService.ts
- apps/desktop/src/main/services/automations/automationIngressService.test.ts
- apps/desktop/src/main/services/automations/automationIngressService.ts
- apps/desktop/src/main/services/prs/prPollingService.ts
5f30b29 to
e1be85e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
e1be85e to
789b069
Compare
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/cto/linearOAuthService.ts (1)
86-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSettle
writeResponsewhen the write throws.
writeResponseresolves only onfinishorclose. Ifresponse.writeHeadorresponse.endthrows, for example because headers were already sent, the returned promise never settles.respondAndFinalizeSessionassigns that promise tosession.closePromise, sodispose()andstartSessionOncewould then await forever and the port would stay bound.Guard the write and resolve on a synchronous failure.
♻️ Proposed fix
response.once("finish", finish); response.once("close", finish); - response.writeHead(status, { "content-type": contentType }); - response.end(body); + try { + response.writeHead(status, { "content-type": contentType }); + response.end(body); + } catch { + finish(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/cto/linearOAuthService.ts` around lines 86 - 106, Update writeResponse to wrap response.writeHead and response.end in synchronous error handling, invoking the existing finish settlement path when either operation throws so the returned promise always resolves. Preserve the current finish/close listeners and idempotent settled guard.apps/desktop/src/main/services/ipc/runtimeBridge.test.ts (1)
1937-1941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth Linear OAuth queue tests depend on the internal await count. Each site flushes exactly one microtask with
await Promise.resolve(), then asserts that the queued operation has not created a second service yet. The number of awaits inside the IPC queue path is an implementation detail, so both assertions are timing-fragile.
apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L1937-L1941: replace the single flush with avi.waitForon an observable condition, or gate the second request on a signal the first operation controls.apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L2022-L2024: apply the same state-based wait before asserting the creation count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/runtimeBridge.test.ts` around lines 1937 - 1941, Replace the timing-dependent single microtask flush in the Linear OAuth queue test around the first site (apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L1937-L1941) with vi.waitFor on an observable state condition, or gate the second request using a signal controlled by the first operation; preserve the assertion that only one service is created. Apply the same state-based synchronization to the sibling site (apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L2022-L2024) before asserting the creation count, so both tests avoid relying on internal IPC queue await counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/main/services/cto/linearOAuthService.ts`:
- Around line 86-106: Update writeResponse to wrap response.writeHead and
response.end in synchronous error handling, invoking the existing finish
settlement path when either operation throws so the returned promise always
resolves. Preserve the current finish/close listeners and idempotent settled
guard.
In `@apps/desktop/src/main/services/ipc/runtimeBridge.test.ts`:
- Around line 1937-1941: Replace the timing-dependent single microtask flush in
the Linear OAuth queue test around the first site
(apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L1937-L1941) with
vi.waitFor on an observable state condition, or gate the second request using a
signal controlled by the first operation; preserve the assertion that only one
service is created. Apply the same state-based synchronization to the sibling
site (apps/desktop/src/main/services/ipc/runtimeBridge.test.ts#L2022-L2024)
before asserting the creation count, so both tests avoid relying on internal IPC
queue await counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 098d3582-2245-46f9-9370-6b95b28559ee
⛔ Files ignored due to path filters (3)
docs/features/automations/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (42)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/automations/automationIngressService.test.tsapps/desktop/src/main/services/automations/automationIngressService.tsapps/desktop/src/main/services/cto/linearAuth.test.tsapps/desktop/src/main/services/cto/linearOAuthService.tsapps/desktop/src/main/services/github/githubAppUserAuthService.tsapps/desktop/src/main/services/github/githubCredentialHealth.test.tsapps/desktop/src/main/services/github/githubCredentialHealth.tsapps/desktop/src/main/services/github/githubRateLimit.tsapps/desktop/src/main/services/github/githubRawRequest.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/prs/prAsync.test.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsxapps/desktop/src/renderer/components/app/IntegrationBannerHost.tsxapps/desktop/src/renderer/components/settings/GitHubSection.tsxapps/desktop/src/renderer/lib/githubIntegrationStatus.test.tsapps/desktop/src/renderer/lib/githubIntegrationStatus.tsapps/desktop/src/shared/githubApiPath.test.tsapps/desktop/src/shared/githubApiPath.tsapps/desktop/src/shared/githubConditionalRequestCache.test.tsapps/desktop/src/shared/githubConditionalRequestCache.tsapps/desktop/src/shared/githubOperationCredential.test.tsapps/desktop/src/shared/githubOperationCredential.tsapps/desktop/src/shared/types/git.tsapps/webhook-relay/src/relay.tsapps/webhook-relay/test/account.test.ts
🚧 Files skipped from review as they are similar to previous changes (39)
- apps/ade-cli/src/multiProjectRpcServer.ts
- apps/desktop/src/renderer/components/app/FeedbackReporterModal.tsx
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.test.tsx
- apps/desktop/src/shared/githubApiPath.test.ts
- apps/desktop/src/shared/githubConditionalRequestCache.test.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.test.ts
- apps/ade-cli/src/cli.ts
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/shared/githubOperationCredential.test.ts
- apps/desktop/src/main/services/projects/projectScaffoldService.test.ts
- apps/desktop/src/renderer/lib/githubIntegrationStatus.test.ts
- apps/desktop/src/shared/githubConditionalRequestCache.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/projects/projectScaffoldService.ts
- apps/ade-cli/README.md
- apps/desktop/src/main/services/prs/prAsync.test.ts
- apps/desktop/src/renderer/lib/githubIntegrationStatus.ts
- apps/desktop/src/shared/githubApiPath.ts
- apps/desktop/src/main/services/github/githubAppUserAuthService.ts
- apps/desktop/src/renderer/components/settings/GitHubSection.tsx
- apps/desktop/src/main/services/prs/prService.test.ts
- apps/desktop/src/main/services/github/githubRawRequest.ts
- apps/webhook-relay/test/account.test.ts
- apps/desktop/src/main/services/github/githubCredentialHealth.ts
- apps/ade-cli/src/headlessLinearServices.test.ts
- apps/webhook-relay/src/relay.ts
- apps/desktop/src/main/services/prs/prService.ts
- apps/desktop/src/main/services/github/githubService.test.ts
- apps/desktop/src/main/services/prs/prPollingService.ts
- apps/desktop/src/main/services/automations/automationIngressService.test.ts
- apps/desktop/src/shared/types/git.ts
- apps/desktop/src/main/services/github/githubRateLimit.ts
- apps/desktop/src/renderer/components/app/IntegrationBannerHost.tsx
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/main/services/automations/automationIngressService.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/ade-cli/src/headlessLinearServices.ts
- apps/desktop/src/shared/githubOperationCredential.ts
- apps/desktop/src/main/services/github/githubService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 789b069014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const appStatus = appUserAuth.getAuthStatus(); | ||
| const [appToken, gh] = await Promise.all([ | ||
| appStatus.tokenStored | ||
| ? appUserAuth.getValidTokenForRelay().catch(() => null) |
There was a problem hiding this comment.
When the stored App access token enters its refresh window and refreshing fails transiently (for example, a network timeout), this catch silently removes the App from the candidate inventory. With no CLI/PAT fallback, computeStatus consequently reports tokenStored: false and authFailure: null, so Settings tells an already-authorized user to reconnect rather than exposing a retryable failure; the headless mirror has the same behavior. Preserve and classify the refresh error so status remains accurate while the credential is temporarily unavailable.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb74e95fc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| authSource: candidate.source, | ||
| writeAuthSource: activeWriteSource ?? "none", |
There was a problem hiding this comment.
When read and write credentials belong to different users—for example, the App reads as Alice while an environment token writes as Bob—userLogin remains Alice even though mutations use writeAuthSource. After land() merges as Bob, resolveViewerLoginForMerge() reads this status login and recordMergeOutcome() permanently coalesces Alice into merged_by_login, corrupting the merge history attribution. Preserve the selected mutation credential's identity and use it for write attribution.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
| const connected = computeConnected({ | ||
| const connected = validatedCredentialCapabilities(candidate, value, repo).read; |
There was a problem hiding this comment.
When the App provides healthy reads but a newly saved PAT is invalid or under-scoped, this aggregate read flag is still true while writeAuthSource is none. GitHubSection.handleSaveToken() checks only status.connected, hides the PAT setup, and reports “saved and verified,” even though the new token failed validation and no writes can run; make the save confirmation require the PAT/write credential to be usable.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
Address review comments 3696023035 and 3696023038.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af19b33564
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onRejectedProbe: (entry, result, repositoryAccessFailure) => { | ||
| if (!repositoryAccessFailure) { | ||
| recordGithubCredentialFailure(entry, result.authFailure, result.rateLimit); |
There was a problem hiding this comment.
When a saved classic PAT has repo but not workflow scope and no other read credential is available, verifyStoredPat synthesizes a permission_denied result for the missing write capability and this callback records a five-minute core cooldown. Subsequent apiRequest and getStatus calls then skip the same PAT for reads even though the preceding status probe established usable read access. Fresh evidence beyond the earlier scope issue is that the new save-verification path now poisons credential health after read status succeeds; avoid recording a read cooldown for a write-only capability failure, including in the mirrored headless callback.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (args.tokenType === "fine-grained") { | ||
| // GitHub does not expose fine-grained token permissions during validation. | ||
| // A successful repository probe establishes that the user-selected token | ||
| // can target this repo; actual write requests still fail over on 403. | ||
| return { read: repositoryReadAvailable, write: repositoryReadAvailable }; |
There was a problem hiding this comment.
When a newly saved fine-grained PAT has only repository read permissions, the repository metadata probe succeeds and this return marks write: true; verifyStoredPat consequently reports a writable credential and Settings says the token was saved and verified, although the first PR mutation will receive 403. Fresh evidence for the previously reported verification problem is that the current shared evaluator explicitly equates repository-read success with write capability, affecting both desktop and headless status paths; keep write capability unverified or validate it independently instead of advertising it as active.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| const targetedPrIds = Array.from(pendingTargetedPrIds); | ||
| pendingTargetedPrIds.clear(); |
There was a problem hiding this comment.
When the relay is healthy and a webhook-triggered targeted refresh fails transiently, these IDs are cleared before prService.refresh completes and the catch path never restores them. The backoff tick then sees no target and, if the latest relay safety sweep was recent, performs no refresh and resets the failure count, leaving derived PR state stale until the next 15-minute safety sweep; remove IDs only after a successful targeted refresh or requeue them on failure. This poller is used by both desktop and headless runtime paths.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation