From 3a85e918b43dc8027147c1785e13b56c926a73b2 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 20 Aug 2026 16:46:42 -0400 Subject: [PATCH 1/2] Require variables for legacy GraphQL queries fd5f8d8f643 allowed legacy queries to replace their variables, but left the fallback variables optional. PullRequestComments supplied only a query, so retrying discarded owner, name, number, and the pagination cursor and failed with invalid-variable errors. Require variables whenever a legacy query is supplied. Pass the review-comment variables explicitly on every page, while preserving separate argument maps for queries with different inputs. Cover replacement and legacy pagination at the repository boundary. Handle missing repository data before reading review threads, so a missing response produces the intended diagnostic. --- src/github/githubRepository.ts | 2 +- src/github/pullRequestModel.ts | 18 ++++--- src/test/github/githubRepository.test.ts | 32 ++++++++++++ src/test/github/pullRequestModel.test.ts | 64 ++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 8 deletions(-) diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fb6abd413d..976c1e9edf 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -328,7 +328,7 @@ export class GitHubRepository extends Disposable { } } - query = async (query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables?: OperationVariables }): Promise> => { + query = async (query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables: OperationVariables }): Promise> => { const gql = this.authMatchesServer && this.hub && this.hub.graphql; if (!gql) { const logValue = (query.query.definitions[0] as { name: { value: string } | undefined }).name?.value; diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 12baa0c246..1c7ad1eaa8 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -1497,16 +1497,20 @@ export class PullRequestModel extends IssueModel implements IPullRe const reviewThreads: ReviewThread[] = []; try { do { + const variables = { + owner: remote.owner, + name: remote.repositoryName, + number: this.number, + after, + }; const { data } = await query({ query: schema.PullRequestComments, - variables: { - owner: remote.owner, - name: remote.repositoryName, - number: this.number, - after - }, - }, false, { query: schema.LegacyPullRequestComments }); + variables, + }, false, { query: schema.LegacyPullRequestComments, variables }); + if (!data?.repository) { + throw new Error('Review comments response did not include a repository.'); + } reviewThreads.push(...data.repository.pullRequest.reviewThreads.nodes); hasNextPage = data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage; diff --git a/src/test/github/githubRepository.test.ts b/src/test/github/githubRepository.test.ts index 946c2f7c54..1dbc91a3b9 100644 --- a/src/test/github/githubRepository.test.ts +++ b/src/test/github/githubRepository.test.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { default as assert } from 'assert'; +import { NetworkStatus } from 'apollo-boost'; import { SinonSandbox, createSandbox } from 'sinon'; import { CredentialStore } from '../../github/credentials'; import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; @@ -18,6 +19,7 @@ import { GitHubServerType } from '../../common/authentication'; import { CheckState, PullRequestCheckStatus } from '../../github/interface'; import { PullRequestBuilder as GraphQLPullRequestBuilder } from '../builders/graphql/pullRequestBuilder'; import Logger from '../../common/logger'; +import { LoggingApolloClient, LoggingOctokit } from '../../github/loggingOctokit'; describe('GitHubRepository', function () { let sinon: SinonSandbox; @@ -38,6 +40,36 @@ describe('GitHubRepository', function () { sinon.restore(); }); + describe('query', function () { + it('replaces variables for a legacy query with different arguments', async function () { + const url = 'https://github.com/some/repo'; + const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); + const repo = new GitHubRepository(1, remote, Uri.file('/workspaces/repo'), credentialStore, telemetry, true); + const graphql = sinon.createStubInstance(LoggingApolloClient); + sinon.stub(credentialStore, 'isAuthenticated').returns(true); + sinon.stub(repo, 'hub').get(() => ({ graphql, octokit: sinon.createStubInstance(LoggingOctokit) })); + const variables = { owner: 'some', name: 'repo', first: 100, after: 'cursor' }; + const response = { data: {}, loading: false, stale: false, networkStatus: NetworkStatus.ready }; + graphql.query.onFirstCall().rejects(new Error('Unsupported query')); + graphql.query.onSecondCall().resolves(response); + + try { + const result = await repo.query({ + query: repo.schema.GetSuggestedActors, + variables: { ...variables, capabilities: ['CAN_BE_ASSIGNED'] }, + }, false, { query: repo.schema.GetAssignableUsers, variables }); + + assert.strictEqual(result, response); + assert.strictEqual(graphql.query.callCount, 2); + const [fallback] = graphql.query.secondCall.args; + assert.strictEqual(fallback.query, repo.schema.GetAssignableUsers); + assert.deepStrictEqual(fallback.variables, variables); + } finally { + repo.dispose(); + } + }); + }); + describe('isGitHubDotCom', function () { it('detects when the remote is pointing to github.com', function () { const url = 'https://github.com/some/repo'; diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 5497cde4ab..14d1c86ae1 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -19,6 +19,9 @@ import { NetworkStatus } from 'apollo-client'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { GitHubServerType } from '../../common/authentication'; import { mergeQuerySchemaWithShared } from '../../github/common'; +import { GitHubRepository } from '../../github/githubRepository'; +import { LoggingApolloClient, LoggingOctokit } from '../../github/loggingOctokit'; +import Logger from '../../common/logger'; const queries = mergeQuerySchemaWithShared(require('../../github/queries.gql'), require('../../github/queriesShared.gql')) as any; const telemetry = new MockTelemetry(); @@ -96,6 +99,67 @@ describe('PullRequestModel', function () { }); describe('reviewThreadCache', function () { + function page(id: string, endCursor: string | null) { + return { + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [{ ...reviewThreadResponse, id }], + pageInfo: { hasNextPage: endCursor !== null, endCursor }, + }, + }, + }, + }, + loading: false, + stale: false, + networkStatus: NetworkStatus.ready, + }; + } + + it('passes review comment variables to every legacy page', async function () { + const repository = new GitHubRepository(1, remote, repo.rootUri, credentials, telemetry, true); + const graphql = sinon.createStubInstance(LoggingApolloClient); + sinon.stub(credentials, 'isAuthenticated').returns(true); + sinon.stub(repository, 'hub').get(() => ({ graphql, octokit: sinon.createStubInstance(LoggingOctokit) })); + sinon.stub(repository, 'ensure').resolves(repository); + graphql.query.onCall(0).rejects(new Error('Unsupported query')); + graphql.query.onCall(1).resolves(page('1', 'first')); + graphql.query.onCall(2).rejects(new Error('Unsupported query')); + graphql.query.onCall(3).resolves(page('2', null)); + + try { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repository, remote, convertRESTPullRequestToRawPullRequest(pr, repository)); + const threads = await model.getReviewThreads(); + + assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2']); + assert.strictEqual(graphql.query.callCount, 4); + for (const [call, after] of [[graphql.query.secondCall, null], [graphql.query.lastCall, 'first']] as const) { + const [fallback] = call.args; + assert.strictEqual(fallback.query, repository.schema.LegacyPullRequestComments); + assert.deepStrictEqual(fallback.variables, { + owner: remote.owner, name: remote.repositoryName, number: pr.number, after, + }); + } + } finally { + repository.dispose(); + } + }); + + it('reports missing review data without retrying', async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const query = sinon.stub(repo, 'query').resolves({ + data: null, loading: false, stale: false, networkStatus: NetworkStatus.error, + }); + const error = sinon.stub(Logger, 'error'); + + assert.deepStrictEqual(await model.getReviewThreads(), []); + assert.strictEqual(query.callCount, 1); + assert.strictEqual(error.lastCall.args[0], 'Failed to get pull request review comments: Error: Review comments response did not include a repository.'); + }); + it('should update the cache when then cache is initialized', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel( From f70dc3ef984764077eaa1eb467a5fe3b272ff7a4 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 20 Aug 2026 16:47:24 -0400 Subject: [PATCH 2/2] Retry review comments with smaller pages GitHub can return an HTML 502 response for a review-thread query that succeeds when fewer threads are requested. The fixed page size makes the entire refresh fail, including pages already fetched. Retain the normal 20-thread page from e0e76278bc6. On HTTP 502, retry the same cursor with five threads and then one, retaining the smaller size for later pages. Stop reducing at one and leave other errors alone. Both current and legacy queries accept the page size, preserving the legacy pagination added by 552316684e6. Cover cursor retention, retry exhaustion, non-502 failures, and reduced page sizes through the legacy query. --- src/github/pullRequestModel.ts | 38 ++++++++++++------ src/github/queriesShared.gql | 8 ++-- src/test/github/pullRequestModel.test.ts | 49 +++++++++++++++++++++--- 3 files changed, 75 insertions(+), 20 deletions(-) diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 1c7ad1eaa8..f0021bde13 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -67,7 +67,7 @@ import { ReviewEventEnum, } from './interface'; import { IssueChangeEvent, IssueModel } from './issueModel'; -import { compareCommits, GraphQLError, GraphQLErrorType } from './loggingOctokit'; +import { compareCommits, getErrorCode, GraphQLError, GraphQLErrorType } from './loggingOctokit'; import { convertRESTPullRequestToRawPullRequest, convertRESTReviewEvent, @@ -1493,29 +1493,45 @@ export class PullRequestModel extends IssueModel implements IPullRe const { remote, query, schema } = await this.githubRepository.ensure(); let after: string | null = null; - let hasNextPage = false; + let pageSize = 20; const reviewThreads: ReviewThread[] = []; try { - do { + while (reviewThreads.length < 1000) { const variables = { owner: remote.owner, name: remote.repositoryName, number: this.number, + first: pageSize, after, }; - const { data } = await query({ - query: schema.PullRequestComments, - variables, - }, false, { query: schema.LegacyPullRequestComments, variables }); + let data: PullRequestCommentsResponse | null; + try { + ({ data } = await query({ + query: schema.PullRequestComments, + variables, + }, false, { query: schema.LegacyPullRequestComments, variables })); + } catch (e) { + if (getErrorCode(e) !== '502' || pageSize === 1) { + throw e; + } + // Large review-thread queries can fail with HTTP 502. + // Retry the same cursor with 5, then 1 thread, and keep that size. + pageSize = Math.max(1, Math.floor(pageSize / 4)); + Logger.warn(`Retrying review comments for PR #${this.number} with ${pageSize} threads per page after HTTP 502.`, PullRequestModel.ID); + continue; + } if (!data?.repository) { throw new Error('Review comments response did not include a repository.'); } - reviewThreads.push(...data.repository.pullRequest.reviewThreads.nodes); + const page = data.repository.pullRequest.reviewThreads; + reviewThreads.push(...page.nodes); - hasNextPage = data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage; - after = data.repository.pullRequest.reviewThreads.pageInfo.endCursor; - } while (hasNextPage && reviewThreads.length < 1000); + if (!page.pageInfo.hasNextPage) { + break; + } + after = page.pageInfo.endCursor; + } Logger.debug(`Fetching review comments for PR #${this.number} - exit`, PullRequestModel.ID); return reviewThreads; diff --git a/src/github/queriesShared.gql b/src/github/queriesShared.gql index 833bd796f6..661c66da0c 100644 --- a/src/github/queriesShared.gql +++ b/src/github/queriesShared.gql @@ -566,10 +566,10 @@ query GetPendingReviewId($pullRequestId: ID!, $author: String!) { } } -query PullRequestComments($owner: String!, $name: String!, $number: Int!, $after: String) { +query PullRequestComments($owner: String!, $name: String!, $number: Int!, $first: Int!, $after: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { - reviewThreads(first: 20, after: $after) { + reviewThreads(first: $first, after: $after) { nodes { id isResolved @@ -608,10 +608,10 @@ query PullRequestComments($owner: String!, $name: String!, $number: Int!, $after } } -query LegacyPullRequestComments($owner: String!, $name: String!, $number: Int!, $after: String) { +query LegacyPullRequestComments($owner: String!, $name: String!, $number: Int!, $first: Int!, $after: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { - reviewThreads(first: 20, after: $after) { + reviewThreads(first: $first, after: $after) { nodes { id isResolved diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 14d1c86ae1..5084d1b0ee 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -125,8 +125,11 @@ describe('PullRequestModel', function () { sinon.stub(repository, 'ensure').resolves(repository); graphql.query.onCall(0).rejects(new Error('Unsupported query')); graphql.query.onCall(1).resolves(page('1', 'first')); - graphql.query.onCall(2).rejects(new Error('Unsupported query')); - graphql.query.onCall(3).resolves(page('2', null)); + const gatewayError = Object.assign(new Error('Bad Gateway'), { networkError: { statusCode: 502 } }); + graphql.query.onCall(2).rejects(gatewayError); + graphql.query.onCall(3).rejects(gatewayError); + graphql.query.onCall(4).rejects(new Error('Unsupported query')); + graphql.query.onCall(5).resolves(page('2', null)); try { const pr = new PullRequestBuilder().build(); @@ -134,12 +137,12 @@ describe('PullRequestModel', function () { const threads = await model.getReviewThreads(); assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2']); - assert.strictEqual(graphql.query.callCount, 4); - for (const [call, after] of [[graphql.query.secondCall, null], [graphql.query.lastCall, 'first']] as const) { + assert.strictEqual(graphql.query.callCount, 6); + for (const [call, after, first] of [[graphql.query.secondCall, null, 20], [graphql.query.lastCall, 'first', 5]] as const) { const [fallback] = call.args; assert.strictEqual(fallback.query, repository.schema.LegacyPullRequestComments); assert.deepStrictEqual(fallback.variables, { - owner: remote.owner, name: remote.repositoryName, number: pr.number, after, + owner: remote.owner, name: remote.repositoryName, number: pr.number, first, after, }); } } finally { @@ -147,6 +150,42 @@ describe('PullRequestModel', function () { } }); + it('retries gateway failures with smaller pages without losing the cursor', async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const gatewayError = Object.assign(new Error('Bad Gateway'), { networkError: { statusCode: 502 } }); + const query = sinon.stub(repo, 'query'); + query.onCall(0).resolves(page('1', 'first')); + query.onCall(1).rejects(gatewayError); + query.onCall(2).rejects(gatewayError); + query.onCall(3).resolves(page('2', 'second')); + query.onCall(4).resolves(page('3', null)); + + const threads = await model.getReviewThreads(); + + assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2', '3']); + assert.deepStrictEqual(query.getCalls().map(call => call.args[0].variables), [ + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 20, after: null }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 20, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 5, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 1, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 1, after: 'second' }, + ]); + }); + + for (const [statusCode, pageSizes] of [[502, [20, 5, 1]], [403, [20]]] as const) { + it(`stops retrying review comments after HTTP ${statusCode}`, async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const query = sinon.stub(repo, 'query').rejects(Object.assign(new Error('Request failed'), { + networkError: { statusCode }, + })); + + assert.deepStrictEqual(await model.getReviewThreads(), []); + assert.deepStrictEqual(query.getCalls().map(call => call.args[0].variables?.first), [...pageSizes]); + }); + } + it('reports missing review data without retrying', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo));