fix(scm-github): handle HTTP 304 correctly in ETag guard catch blocks#1237
fix(scm-github): handle HTTP 304 correctly in ETag guard catch blocks#1237awesome-pro wants to merge 1 commit intoComposioHQ:mainfrom
Conversation
The gh CLI exits non-zero on HTTP 304 responses, causing execFileAsync to throw before the 304-detection code in the try block could run (dead code). Both catch blocks in checkPRListETag and checkCommitStatusETag were treating 304 as a failure and returning true (assume changed), causing the full GraphQL batch to run every 30s instead of being skipped when nothing changed. Add an early return false when the error message contains "HTTP 304" in both catch blocks, so a 304 is correctly treated as "not modified". Fixes ComposioHQ#1191
There was a problem hiding this comment.
Pull request overview
Fixes scm-github’s ETag guard logic so HTTP 304 (“Not Modified”) responses from gh api are treated as cache hits (skip GraphQL batch) rather than failures that force a refresh, restoring the intended polling optimization and resolving #1191.
Changes:
- In
checkPRListETagcatch block, detect"HTTP 304"in the thrown error message and returnfalse(not modified). - In
checkCommitStatusETagcatch block, detect"HTTP 304"in the thrown error message and returnfalse(not modified).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const errorMsg = err instanceof Error ? err.message : String(err); | ||
| // gh CLI exits non-zero on HTTP 304 — that means "not modified", not an error | ||
| if (errorMsg.includes("HTTP 304")) { | ||
| return false; // Not modified — skip batch, use cache (intended behavior) | ||
| } |
There was a problem hiding this comment.
The new 304 handling is in the catch path, but the existing unit tests for the guards only simulate 304 via a resolved stdout (success path). Please add/adjust a test to cover the real-world behavior where execFileAsync rejects on 304 (e.g., rejected Error message containing "gh: HTTP 304") so this regression stays protected.
| } catch (err) { | ||
| // On error, assume change to ensure we don't miss anything | ||
| const errorMsg = err instanceof Error ? err.message : String(err); | ||
| // gh CLI exits non-zero on HTTP 304 — that means "not modified", not an error | ||
| if (errorMsg.includes("HTTP 304")) { | ||
| return false; // Not modified — skip batch, use cache (intended behavior) | ||
| } |
There was a problem hiding this comment.
Same as Guard 1: add test coverage for the catch-path 304 behavior (where execFileAsync rejects on 304 and the handler returns false). Current tests cover only the resolved-stdout 304 branch, not the thrown-error scenario described in #1191.
Summary
Resolves #1191 — ETag Guard treats HTTP 304 as failure, defeats caching optimization
Root Cause
The
ghCLI exits with a non-zero exit code on HTTP 304 responses. This causedexecFileAsyncto throw before thetry-block 304-detection code could run (dead code). Both catch blocks incheckPRListETagandcheckCommitStatusETagwere treating every HTTP 304 as a failure — logging a warning and returningtrue("assume changed"). As a result, the full GraphQL batch ran every 30 seconds instead of being skipped when nothing had changed.Changes Made
packages/plugins/scm-github/src/graphql-batch.ts— Added an earlyif (errorMsg.includes("HTTP 304")) return falsecheck at the top of both catch blocks (Guard 1 incheckPRListETag, Guard 2 incheckCommitStatusETag). When the error is a 304, the function now correctly returnsfalse(not modified — skip batch, use cache). All other errors fall through to the existing warn-and-return-true path unchanged.Testing
pnpm --filter @aoagents/ao-plugin-scm-github test)graphql-batch.tsdirectly (graphql-batch.test.ts— 65 tests,graphql-batch.integration.test.ts— 8 tests) pass with no changes neededNotes
The fix matches on
"HTTP 304"as a substring in the error message, which matches the standardghCLI output format (gh: HTTP 304). The existing dead code in thetryblock (checkingstdoutfor"HTTP/1.1 304"or"HTTP/2 304") is left untouched — removing it would be out of scope for this fix.Fixes #1191