Repository navigation
fix(host-service): classify expected GitHub auth and 404 outcomes as non-500s - #6389
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe host service now reports missing GitHub credentials with a typed tRPC error. Direct pull-request lookups return ChangesGitHub error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/host-service/src/app.ts (1)
101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the typed tRPC error contract.
Test that a missing token produces
PRECONDITION_FAILEDwithcause.kind === "NO_GITHUB_TOKEN". Also test that a valid token still creates an authenticatedOctokitclient.The PR objectives state that no existing tests cover the changed paths.
🤖 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 `@packages/host-service/src/app.ts` around lines 101 - 106, Add regression tests for the missing-token and valid-token branches in the application’s GitHub client setup: assert that the missing-token path throws TRPCError code PRECONDITION_FAILED with cause.kind equal to NO_GITHUB_TOKEN, and that providing a valid token creates an authenticated Octokit client. Use the existing app setup and test utilities rather than changing the error contract.
🤖 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
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts`:
- Around line 416-429: Update the Octokit catch in the pull-request fetch flow
to return null only when the error’s HTTP status is exactly 404, allowing other
failures—including messages containing “not found”—to propagate. Keep
isGithubNotFoundError unchanged for execGh results, and add a regression test
covering a non-404 error with a “not found” message.
---
Nitpick comments:
In `@packages/host-service/src/app.ts`:
- Around line 101-106: Add regression tests for the missing-token and
valid-token branches in the application’s GitHub client setup: assert that the
missing-token path throws TRPCError code PRECONDITION_FAILED with cause.kind
equal to NO_GITHUB_TOKEN, and that providing a valid token creates an
authenticated Octokit client. Use the existing app setup and test utilities
rather than changing the error contract.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b6a627b-2b8b-4de7-ae0c-98e022eba2de
📒 Files selected for processing (2)
packages/host-service/src/app.tspackages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts
Comment on lines
+416
to
+429
| const response = await octokit.pulls | ||
| .get({ | ||
| owner: repo.owner, | ||
| repo: repo.name, | ||
| pull_number: prNumber, | ||
| }) | ||
| .catch((error: unknown) => { | ||
| // A 404 means the PR doesn't exist or isn't accessible to the user — | ||
| // an expected empty result, not an internal error. | ||
| if (isGithubNotFoundError(error)) return null; | ||
| throw error; | ||
| }); | ||
| if (!response) return null; | ||
| const pr = response.data; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target procedure ---'
sed -n '380,445p' packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts
printf '%s\n' '--- helper definition and usages ---'
rg -n -C 8 'isGithubNotFoundError' packages/host-service/src
printf '%s\n' '--- relevant tests ---'
rg -n -C 6 'search-pull|pull.?request|not found|HTTP 404|status.*404' packages/host-service --glob '*.{test,spec}.{ts,tsx,js,jsx}'Repository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- helper implementation ---'
sed -n '145,210p' packages/host-service/src/trpc/router/workspace-creation/shared/github-search.ts
printf '%s\n' '--- focused helper tests ---'
sed -n '218,245p' packages/host-service/src/trpc/router/workspace-creation/shared/github-search.test.ts
printf '%s\n' '--- behavior probe ---'
node - <<'JS'
const pattern = /could not resolve to|\bnot found\b|HTTP 404/i;
const errors = [
Object.assign(new Error("Not Found"), { status: 404 }),
Object.assign(new Error("Not Found"), { status: 500 }),
Object.assign(new Error("transport: resource not found"), { status: 503 }),
Object.assign(new Error("rate limit exceeded"), { status: 429 }),
];
for (const error of errors) {
const status =
typeof error === "object" && error !== null && "status" in error
? error.status
: undefined;
console.log(JSON.stringify({
message: error.message,
status,
sharedPredicate: status === 404 || pattern.test(error.message),
statusOnlyPredicate: status === 404,
}));
}
JSRepository: superset-sh/superset
Length of output: 3369
Restrict the fallback to an actual 404 response.
At line 425, check the Octokit error status directly. A non-404 error with not found in its message currently returns null and hides the failure. Keep isGithubNotFoundError for execGh results. Add a regression test for this case.
🤖 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
`@packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts`
around lines 416 - 429, Update the Octokit catch in the pull-request fetch flow
to return null only when the error’s HTTP status is exactly 404, allowing other
failures—including messages containing “not found”—to propagate. Keep
isGithubNotFoundError unchanged for execGh results, and add a regression test
covering a non-404 error with a “not found” message.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 2 files
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Architecture diagram
sequenceDiagram
participant Client as tRPC Client
participant TRPC as tRPC Procedure
participant GH as github() Context Factory
participant Octokit as Octokit Client
participant Sentry as Sentry Middleware
Note over Client,Sentry: GitHub Auth Flow (every ctx.github() procedure)
Client->>TRPC: Invoke tRPC procedure (e.g., git.getPullRequestThreads)
TRPC->>GH: Request GitHub context
GH->>GH: Get token from credential providers
alt Token available
GH-->>TRPC: Octokit client
TRPC->>Octokit: GitHub API call
Octokit-->>TRPC: Response
else No token configured
GH-->>TRPC: TRPCError PRECONDITION_FAILED (cause: NO_GITHUB_TOKEN)
Note over GH,TRPC: Classified as precondition failure - not 500
TRPC->>Sentry: PRECONDITION_FAILED error
Sentry-->>Client: Non-500 error (no Sentry alert)
end
Note over Client,Sentry: PR Direct Lookup Flow (searchPullRequests)
Client->>TRPC: searchPullRequests(single PR number)
TRPC->>Octokit: octokit.pulls.get()
alt PR found
Octokit-->>TRPC: Pull request data
TRPC->>TRPC: Apply author/review filters
TRPC-->>Client: PullRequestResult
else PR not found (404)
Octokit-->>TRPC: HttpError 404
TRPC->>TRPC: Check via isGithubNotFoundError
TRPC-->>Client: null (empty result)
Note over TRPC,Sentry: 404 treated as expected empty result - not 500
else Other error (5xx, network, rate limit)
Octokit-->>TRPC: Other error
TRPC->>Sentry: Propagate as INTERNAL_SERVER_ERROR
Sentry-->>Client: 500 + Sentry alert
end
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two expected GitHub outcomes escape host-service tRPC procedures as
INTERNAL_SERVER_ERROR500s and file Sentry issues:github()context factory throws a plainError("No GitHub token available...")the moment any procedure requests an Octokit client (seen ongit.getPullRequestThreads). Every PR view without GitHub auth files a 500.workspaceCreation.searchPullRequests, an explicit-PR-number lookup for a PR that doesn't exist or isn't accessible rejects with an Octokit HttpError 404 that propagates unclassified.Root cause
Both are expected domain states (missing precondition / empty result) thrown as untyped errors, so the tRPC Sentry middleware — which reports anything still
INTERNAL_SERVER_ERRORat the boundary — correctly treats them as bugs. Per the repo's error contract, the missing translation belongs at the throw sites, never as a Sentry-side filter.Fix
packages/host-service/src/app.ts: thegithub()context factory now throwsTRPCError({ code: "PRECONDITION_FAILED", cause: { kind: "NO_GITHUB_TOKEN" } })with the existing message unchanged. Every procedure that callsctx.github()inherits the classification from this one shared site; no call sites touched.packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts:octokitDirectLookupRowtreats a 404 fromoctokit.pulls.getas "no matching PR" and returnsnull, which the single-target caller already handles gracefully (emptyPullRequestsPage) and which matches how the multi-targetPromise.allSettledpath already filters 404 rejections viaisGithubNotFoundError(status/name check, noinstanceof).Genuine unexpected GitHub failures (5xx, network, rate limits) still propagate and report exactly as before.
github.mergePRclassification (HOST-SERVICE-1P) is intentionally untouched — open PR #6228 ownsgithub.ts.Verification
bunx biome checkon the two changed files:cd packages/host-service && bun run typecheck:No existing tests cover the changed code paths (no co-located test files for
app.tsorsearch-pull-requests.ts), so no test run applies.Refs HOST-SERVICE-1M
Refs HOST-SERVICE-1Q
https://claude.ai/code/session_72e9200a-ff85-4667-ac6d-add2c90fb109
Summary by cubic
Classifies two expected GitHub outcomes so they no longer surface as 500s or file Sentry issues. Previously, missing GitHub credentials or a direct PR lookup 404 resulted in INTERNAL_SERVER_ERROR; now missing credentials throw PRECONDITION_FAILED and 404 direct lookups return null (empty result). Unexpected GitHub failures still propagate unchanged.
What changed
packages/host-service/src/app.ts:github()now throwsTRPCError(PRECONDITION_FAILED, cause{ kind: "NO_GITHUB_TOKEN" }) from@trpc/serverinstead of a generic error.packages/host-service/src/trpc/router/workspace-creation/procedures/search-pull-requests.ts: treat@octokit/restpulls.get404 as “no matching PR” and returnnullvia existingisGithubNotFoundError; caller already handles empty results. No call sites changed.PRECONDITION_FAILEDtRPC error; single-PR direct lookup may returnnullinstead of throwing. Sentry noise is reduced;github.mergePRhandling is unchanged.Written for commit c928b75. Summary will update on new commits.