feat(server): pull requests talk to GitHub's API instead of the gh CLI#16320

🟡 Medium pullRequest/GitHubPullRequestProvider.ts:232

A rate-limited Actions workflow lookup is treated as unavailable: true, so fork PRs lose the rate-limit failure/banner and do not enter the existing retry/pause handling. listActionRequiredRuns wraps the restRead failure in GitHubPullRequestReadError, so this check only matches the unwrapped GitHubApiRateLimitError from the parallel GraphQL lookup; preserve the rate-limit error before wrapping or recognize it through its cause.

No longer relevant as of 08ca0ffebc2698bf6dba7939481bd524580ef98f

🟡 Medium pullRequest/gitHubPullRequestJson.ts:2422

getPullRequestActivity returns incomplete remarks after REVIEW_THREAD_PAGES (10) pages even when nextCommentsCursor or nextReviewsCursor is still present, so PRs with more than 1,000 comments or reviews silently lose conversation. Because the provider uses pullRequest.comments.length as commentCount and exposes only the separate thread-walk truncation, clients cannot detect this omission. Continue paging until both cursors are exhausted, or propagate truncation/count information.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Apply the timeout to the body read, not only to execute.

httpClient.execute completes when the response headers arrive. collectUint8StreamText then reads the body with no timeout. The new timeout input says "a whole pull request's patch may need longer", but the timeout does not cover the body download. A stalled body stream on an 8 MB diff or a contents read never completes.

gate.withPermit(run) holds one of the eight permits while the body is read. A few stalled bodies can therefore block every GitHub API call for the server.

Wrap the request and the body collection in the same timeout.

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Request only CLOSED for the Closed fallback.

GitHub GraphQL PullRequestState keeps CLOSED and MERGED as separate states. A CLOSED filter already excludes merged pull requests. This code requests MERGED as well, and matchesUnsortedListing then removes every merged row. In a repository with many merged pull requests, most fetched rows are discarded. The fallback then doubles up to PULL_REQUEST_FALLBACK_MAX_ROWS (1,000) requests' worth of rows. It can still return fewer closed rows than the repository holds.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Record GraphQL requests that fail.

The cost is recorded only on success, through Effect.tap. If api.graphql fails with GitHubApiResponseError or GitHubApiNotFoundError, GitHub has already answered and charged points. Callers fall back to narrower reads in these cases, so the request count and the point total come out too low. Use Effect.onExit or Effect.tapError to count the failed request too. A failed request has no body here, so record a cost of 0 and note that the cost is unknown.

🟠 High pullRequest/GitHubPullRequestCli.ts:1695

The cached KnownWorkflowRuns path drops runs awaiting approval, so approve-workflows silently approves nothing. Cached workflow runs expose action_required in conclusion (typically with status: "completed"), but this filter checks run.status; preserve and filter the cached conclusion field instead.

No longer relevant as of b9c17d762143c9c2c822ff02dabab70a808e2be2

🟠 High

https://github.com/pingdotgg/t3code/blob/4310e5887e5da1eef05c9692eb6e7e24cf13b276/apps/server/src/sourceControl/GitHubApi.ts#L349

The timeout at Effect.timeout(input.timeout ?? DEFAULT_TIMEOUT) covers only httpClient.execute, so a response that sends headers and then stalls while collectUint8StreamText reads the body waits indefinitely. This can hold one of the eight semaphore permits indefinitely and block all GitHub API work; apply the timeout to the body collector as well (or to the combined execute-and-read operation).

No longer relevant as of b9c17d762143c9c2c822ff02dabab70a808e2be2

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Keep raw GraphQL error text out of detail.

When a GraphQL request returns GitHubApiResponseError, its message joins response-provided graphqlErrors. This assignment copies that text into the provider error’s detail attribute. (raw.githubusercontent.com) Keep the raw text in cause and use a fixed, bounded detail for this error.

As per coding guidelines, “Attributes and log annotations stay bounded: no raw payloads, command arguments or output, signed URLs, credentials, query strings, or arbitrary defect text.” The exact value belongs only in cause.

Sources: Coding guidelines, Path instructions

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove the GitHub CLI attribution from API read errors.

GitHubPullRequestReadError now reports failed API-response decoding, and the login read uses the REST user endpoint. Lines 149 and 162 still say that the GitHub CLI returned the response. Name GitHub or the API instead so the error does not direct users toward the wrong component.

Also applies to: 162-162

🟠 High pullRequest/gitHubConditionalChecks.ts:120

Invalid UTF-8 responses are accepted and cached as workflow-run JSON, so replacement characters can change fields such as conclusion and cause KnownWorkflowRuns to omit an approval-required run instead of falling back to the normal read. Include response.invalidUtf8 in this rejection guard, matching the previous stdoutInvalidUtf8 behavior.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove the mapError that wraps typed API errors into a read error.

restRead already reports decode failures as GitHubPullRequestReadError with operation listWorkflowRunsRequiringApproval. The mapError on Line 1670 wraps every error again in a second GitHubPullRequestReadError:

• A decode failure is wrapped twice.
• GitHubApiRateLimitError, SourceControlRateLimitPausedError, GitHubApiAuthenticationError and GitHubCredentialUnavailableError lose their tags. The caller can then show only "unreadable response" in place of the rate-limit or sign-in message. listHeadsByBranch does not wrap its errors, so the two halves of the same approval read fail differently.

The guidelines require that "a translation boundary passes through domain errors already in the target channel and wraps only unknown or lower-level failures." The fix is to drop the wrapper.

As per coding guidelines: "Translation. … A translation boundary passes through domain errors already in the target channel and wraps only unknown or lower-level failures."

Source: Coding guidelines

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Failed REST calls are not counted.

The Effect.tap on api.rest records a request only on success. A REST call that fails, for example with a 404, is not counted. The measured restRequests total then comes out too low. This is the same root cause as the GraphQL comment above. Record the request on failure too, for example with Effect.tapError.