Skip to content

fix(scm-github): handle HTTP 304 correctly in ETag guard catch blocks #1237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
awesome-pro wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(scm-github): handle HTTP 304 correctly in ETag guard catch blocks #1237

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/plugins/scm-github/src/graphql-batch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,12 @@ async function checkPRListETag(
// PR list changed - cost: 1 REST point
return true;
} 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)
}
Comment on lines 374 to +378
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
// On other errors, assume change to ensure we don't miss anything
// Log but don't throw - allow GraphQL batch to proceed
// eslint-disable-next-line no-console -- Observability logging for ETag errors
console.warn(`[ETag Guard 1] PR list check failed for ${repoKey}: ${errorMsg}`);
Expand Down Expand Up @@ -429,8 +433,12 @@ async function checkCommitStatusETag(
// CI status changed - cost: 1 REST point
return true;
} 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)
}
Comment on lines 435 to +440
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
// On other errors, assume change to ensure we don't miss anything
// eslint-disable-next-line no-console -- Observability logging for ETag errors
console.warn(
`[ETag Guard 2] Commit status check failed for ${commitKey}: ${errorMsg}`,
Expand Down