fix(github): survive GraphQL responses that carry no envelope - #132
Open
factory-nizar wants to merge 1 commit into
Open
fix(github): survive GraphQL responses that carry no envelope#132factory-nizar wants to merge 1 commit into
factory-nizar wants to merge 1 commit into
Conversation
`@octokit/graphql` resolves `response.data.data` and only throws when the body carries an `errors` array, so an HTTP 200 whose body is not a GraphQL envelope resolves `undefined`. `fetchPRBranchData` dereferenced that value directly, turning a transient GitHub answer into `TypeError: undefined is not an object (evaluating 'prResult.repository')` that killed the `Prepare validator` step of Droid Auto Review and skipped the review pass that posts the comments. Route both fetchers in this module through one helper that checks the response shape before reading it, retries the request via `utils/retry.ts` per the repo convention for GitHub API calls, and reports the HTTP status, content type and body preview captured through a request hook, since `@octokit/graphql` otherwise discards the body that would explain the failure. A present-but-null `repository` stays a stable "not found" answer and is not retried. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Contributor
|
Droid finished @factory-nizar's task —— View job Good robustness fix for transient non-GraphQL 200 responses, with solid coverage. Only minor concern is the new retry-focused unit tests use real backoff sleeps and retry logging, which can slow and noisify CI runs. |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
Comment on lines
+64
to
+72
| let consoleErrorSpy: ReturnType<typeof spyOn>; | ||
|
|
||
| beforeEach(() => { | ||
| consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| consoleErrorSpy.mockRestore(); | ||
| }); |
Contributor
There was a problem hiding this comment.
[P3] Avoid slow/noisy retries in unit tests
These tests intentionally exercise retry paths, but retryWithBackoff uses real setTimeout sleeps and logs each attempt, which can make the suite noticeably slower and the output noisy. Consider stubbing console.log (and, if this grows, using fake timers or a test override for retry delays) so retry-focused tests stay fast and stable.
Suggested change
| let consoleErrorSpy: ReturnType<typeof spyOn>; | |
| beforeEach(() => { | |
| consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); | |
| }); | |
| afterEach(() => { | |
| consoleErrorSpy.mockRestore(); | |
| }); | |
| let consoleErrorSpy: ReturnType<typeof spyOn>; | |
| let consoleLogSpy: ReturnType<typeof spyOn>; | |
| beforeEach(() => { | |
| consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); | |
| consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); | |
| }); | |
| afterEach(() => { | |
| consoleErrorSpy.mockRestore(); | |
| consoleLogSpy.mockRestore(); | |
| }); |
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
1 participant
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.
What happened
Droid Auto Reviewrun 32616161689 on factory-mono#19051 went red in thePrepare validatorstep:Two details from that log correct the first reading of this failure:
Pass 1 candidates validated: 1 comments foundprinted 40 ms earlier, so pass 1 ran for 13 minutes and produced a finding. What the crash killed wasRun Droid Exec (validator)— the pass that posts the comments. The finding was computed and thrown away.fetchPRBranchDatahad already succeeded in the same job at03:46:11with the same query and the same repository (Checking out PR #19051 branch for diff computation...). It failed at04:00:20on the second call. Auth was fine either way (OIDC token successfully obtained/App token successfully obtainedimmediately before the crash). 1 failure in 78 runs of the workflow over 08-22..08-23.The check does not block merge, so #19051 merged 99 minutes later with the review check red and no review posted.
Root cause
@octokit/graphqlreturnsresponse.data.dataand throws only when the body carries anerrorsarray:So an HTTP 200 whose body is not a GraphQL envelope — an HTML error page, or JSON with no
datamember — resolvesundefined.prResult.repositorythen throws. TheTypeErrorwording pins it precisely: a GraphQL error payload would have thrownGraphqlResponseErrorinstead, and a nulledrepositoryfield would have readnull is not an object.prResultitself wasundefined, i.e. GitHub answered 200 with something that was not a GraphQL response.The old
catchalso discarded the cause (Failed to fetch PR branch data for PR #19051), which is why the raw response never reached the log.Reproduction
Driving the real
@octokit/graphqlwith a stubbed fetch that returns200 text/html, before the fix:Byte-for-byte the CI signature, including the interleaved source excerpt. After the fix the same input produces:
The change
Both fetchers in
src/github/data/pr-fetcher.tsnow go through onefetchGraphQLFieldhelper that:TypeError.utils/retry.ts(3 attempts, 1s/2s backoff), which is what actually keeps the check green — this repo'sAGENTS.mdalready prescribesutils/retry.tsfor GitHub API operations, and the same query demonstrably succeeded minutes earlier in the same job.request.hook, because@octokit/graphqlhands back only thedatamember and otherwise the next occurrence would be just as undiagnosable as this one.A present-but-null
repositoryorpullRequestis a stable "not found" answer, so it is reported immediately and costs one request, not three. The wrapping errors now carry their cause.Scope.
pr-fetcher.tswas the only module dereferencing a GraphQL response without optional chaining (rg '\.repository\.' src);src/mcp/github-pr-server.ts:405already readsthreadLookupQuery?.repository?.pullRequest?.... Both functions here are fixed, which covers every entry point that reads this query:generate-review-prompt,review,review-validator,security-review,fillandsecurity-scan.Validation
bun test test/github/data/pr-fetcher.test.ts— 11 pass / 0 fail (new file).bun test— 588 pass / 0 fail across 54 files.bun run typecheck— clean.bun run format:check— clean.Mutation-checked, per the repo's rule against tests that cannot fail. Each mutation was applied to the fix and the suite re-run:
pr-fetcher.tsmaxAttempts: 3->1(no retry)""Only the two happy-path tests survive reverting the fix, which is the intended shape.
Workstream · Change