Skip to content

fix: refresh PR Sous Chef metadata through scoped GraphQL #69229

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

Merged
pelikhan merged 1 commit into from
Oct 16, 2026
Merged

fix: refresh PR Sous Chef metadata through scoped GraphQL #69229

Show file tree
Hide file tree
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
59 changes: 40 additions & 19 deletions scripts/pr-sous-chef.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,28 +294,49 @@ function inspect(repo, pr, runs, api = gh, now = Date.now()) {
const prFields = "number,updatedAt,author,isDraft,state,labels";

export function fetchCandidate(repo, number, api = gh, now) {

@github-actions github-actions Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/codebase-design] fetchCandidate now duplicates the owner/name split, -f owner=/-f name= argument construction, and response?.errors?.length error-check that inspect (just above, ~line 269-288) already implements for the same repo-scoped GraphQL pattern.

💡 Suggested refactor

Extract a small shared helper, e.g.:

function graphqlQuery(repo, api, query, vars) {
  const [owner, name] = repo.split("/");
  const response = api(["api", "graphql", "-f", `query=${query}`, "-f", `owner=${owner}`, "-f", `name=${name}`, ...vars]);
  if (response?.errors?.length) throw new Error(JSON.stringify(response.errors));
  return response?.data?.repository?.pullRequest;
}

Both call sites could then pass only their query body and PR-number variable. This isn't blocking, but as a third repo-scoped GraphQL call site gets added later this duplication will make it easy for one copy to drift (e.g. only fetchCandidate currently guards with response?.errors optional chaining while inspect uses response.errors unguarded).

@copilot please address this.

// gh pr view's union fragments are rewritten incorrectly by the GitHub CLI proxy.
const candidate = api(["api", `repos/${repo}/pulls/${number}`]);
if (candidate?.number !== Number(number) || !candidate.updated_at || !candidate.head?.sha || !candidate.head.ref || !candidate.base?.ref) throw new Error(`PR #${number}: could not refresh PR metadata`);
// The proxy can mistake REST PR labels for the response collection and return [].
// Use an explicit repository-scoped query, without gh pr view's union fragments.
const [owner, name] = repo.split("/");
const response = api([
"api",
"graphql",
"-f",
`query=query($owner:String!,$name:String!,$number:Int!){
repository(owner:$owner,name:$name){pullRequest(number:$number){
number title url state isDraft headRefOid headRefName baseRefName
createdAt updatedAt changedFiles mergeStateStatus author{login} authorAssociation

@github-actions github-actions Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changedFiles in the metadata query makes refresh fail for otherwise-valid PRs whenever GitHub cannot compute that count, so Sous Chef can stop processing the PR before it even reaches the existing check/status logic.

💡 Why this is risky

gh api graphql can return partial data with an errors array when one field resolver fails. changedFiles is exactly the kind of expensive field that can be unavailable on large PRs, and line 317 turns any such partial error into a hard exception. We only use the count later for the zero-diff heuristic, so this changes a non-critical signal into a total refresh failure.

A safer pattern is to request changedFilesIfAvailable (or drop the field from this refresh query), carry null or undefined through metadata, and only apply the zero-diff heuristic when the value is known.

Suggested change
createdAt updatedAt changedFiles mergeStateStatus author{login} authorAssociation
createdAt updatedAt changedFilesIfAvailable mergeStateStatus author{login} authorAssociation

That keeps the refresh path resilient while preserving the optional zero-diff optimization.

labels(first:100){pageInfo{hasNextPage} nodes{name}}
}}}`,
"-f",
`owner=${owner}`,
"-f",
`name=${name}`,
"-F",
`number=${number}`,
]);
if (response?.errors?.length) throw new Error(`PR #${number}: ${JSON.stringify(response.errors)}`);
const candidate = response?.data?.repository?.pullRequest;
if (
candidate?.number !== Number(number) ||
!candidate.updatedAt ||
!candidate.headRefOid ||
!candidate.headRefName ||
!candidate.baseRefName ||
!["OPEN", "CLOSED", "MERGED"].includes(candidate.state) ||
typeof candidate.isDraft !== "boolean" ||
!Array.isArray(candidate.labels?.nodes) ||
!candidate.labels.nodes.every(label => typeof label?.name === "string") ||
candidate.labels?.pageInfo?.hasNextPage !== false

@github-actions github-actions Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Failing closed on labels.pageInfo.hasNextPage !== false means any PR with more than 100 labels now throws out of fetchCandidate, and in the full queue path that one PR aborts the entire scan.

💡 Why this is risky

This query only fetches the first 100 labels, but the new guard treats pagination as a metadata corruption case instead of an incomplete-but-recoverable response. fetchQueue does not isolate per-PR refresh failures, so a single heavily-labeled PR can stop Sous Chef from producing a queue for every other PR.

If the real goal is only to detect the broccoli opt-out label, either paginate until that label is found or treat hasNextPage as incomplete labels and continue without the opt-out shortcut instead of throwing.

Suggested change
candidate.labels?.pageInfo?.hasNextPage !== false
labels(first:100){pageInfo{hasNextPage endCursor} nodes{name}}

Then either follow the cursor or degrade gracefully when the first page is incomplete.

)
throw new Error(`PR #${number}: could not refresh PR metadata`);
const metadata = {
number: candidate.number,
title: candidate.title,
url: candidate.html_url,
state: candidate.state.toUpperCase(),
isDraft: candidate.draft,
headRefOid: candidate.head.sha,
headRefName: candidate.head.ref,
baseRefName: candidate.base.ref,
createdAt: candidate.created_at,
updatedAt: candidate.updated_at,
changedFiles: candidate.changed_files,
author: candidate.user ? { login: candidate.user.login } : null,
labels: candidate.labels ?? [],
mergeStateStatus: candidate.mergeable_state === "dirty" ? "CONFLICTING" : (candidate.mergeable_state ?? "unknown").toUpperCase(),
...candidate,
labels: candidate.labels.nodes,
mergeStateStatus: candidate.mergeStateStatus === "DIRTY" ? "CONFLICTING" : candidate.mergeStateStatus,

@github-actions github-actions Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] mergeStateStatus is validated far less strictly than the other enum-shaped fields (state is checked against a fixed allowlist, but mergeStateStatus is passed through unchecked besides the DIRTY rename).

💡 Why this matters

GitHub's MergeStateStatus enum has 7 values (CLEAN, DIRTY, UNKNOWN, BLOCKED, BEHIND, UNSTABLE, HAS_HOOKS), and downstream code (branchAction on line ~180, classify on line ~385) specifically matches against "CONFLICTING"/"BEHIND". An unexpected or malformed value (e.g. null, missing field, future enum addition) silently flows through as a falsy/unknown string instead of failing closed like the other required fields do just above.

Consider adding mergeStateStatus to the existing validation list (e.g. !["CLEAN","DIRTY","UNKNOWN","BLOCKED","BEHIND","UNSTABLE","HAS_HOOKS"].includes(candidate.mergeStateStatus)) so a proxy regression on this field fails the same way as the other guard fields rather than silently producing an unrecognized status value.

@copilot please address this.

};
if (now !== undefined && metadataSkip(metadata, now)) return metadata;
const checkRuns = api(["api", `repos/${repo}/commits/${candidate.head.sha}/check-runs?per_page=100&filter=latest`, "--paginate", "--slurp"]).flatMap(page => page.check_runs);
const statuses = api(["api", `repos/${repo}/commits/${candidate.head.sha}/status?per_page=100`, "--paginate", "--slurp"]).flatMap(page => page.statuses);
const checkRuns = api(["api", `repos/${repo}/commits/${candidate.headRefOid}/check-runs?per_page=100&filter=latest`, "--paginate", "--slurp"]).flatMap(page => page.check_runs);
const statuses = api(["api", `repos/${repo}/commits/${candidate.headRefOid}/status?per_page=100`, "--paginate", "--slurp"]).flatMap(page => page.statuses);
return {
...metadata,
statusCheckRollup: [
Expand Down
100 changes: 69 additions & 31 deletions scripts/pr-sous-chef.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,32 +317,32 @@ test("active-run lookup can be narrowed to one PR branch", () => {
assert.ok(calls.every(args => args[1].includes("&branch=copilot%2Ffix%20a%20bug")));
});

const restPr = {
number: 42,
title: pr.title,
state: "open",
draft: false,
html_url: "https://github.com/github/gh-aw/pull/42",
head: { sha: "abc", ref: "copilot/fix", repo: { owner: { login: "external-author" } } },
base: { ref: "main" },
user: { login: "Copilot" },
created_at: pr.createdAt,
updated_at: pr.updatedAt,
changed_files: 2,
mergeable_state: "dirty",
const graphPr = {
...pr,
mergeStateStatus: "DIRTY",
isDraft: false,
url: "https://github.com/github/gh-aw/pull/42",
labels: { nodes: [], pageInfo: { hasNextPage: false } },
};

test("targeted refresh uses paginated REST APIs, not proxy-incompatible gh pr view", () => {
function metadataResponse(candidate = graphPr) {
return { data: { repository: { pullRequest: candidate } } };
}

test("targeted refresh uses explicit GraphQL metadata and paginated REST checks", () => {
const calls = [];
const candidate = fetchCandidate("github/gh-aw", "42", args => {
calls.push(args);
if (args[1].includes("/pulls/")) return restPr;
if (args[1] === "graphql") return metadataResponse();
if (args[1].includes("/check-runs?")) return [{ check_runs: [{ name: "CGO", status: "completed", conclusion: "failure", html_url: "https://github.com/run/1" }] }];
return [{ statuses: [{ context: "external-ci", state: "pending", target_url: "https://github.com/run/2" }] }];
});
assert.equal(candidate.number, 42);
assert.deepEqual(calls, [
["api", "repos/github/gh-aw/pulls/42"],
assert.equal(calls[0][1], "graphql");
assert.match(calls[0][3], /repository\(owner:\$owner,name:\$name\)\{pullRequest\(number:\$number\)/);
assert.doesNotMatch(calls[0][3], /\.\.\.|statusCheckRollup/);
assert.deepEqual(calls[0].slice(4), ["-f", "owner=github", "-f", "name=gh-aw", "-F", "number=42"]);
assert.deepEqual(calls.slice(1), [
["api", "repos/github/gh-aw/commits/abc/check-runs?per_page=100&filter=latest", "--paginate", "--slurp"],
["api", "repos/github/gh-aw/commits/abc/status?per_page=100", "--paginate", "--slurp"],
]);
Expand All @@ -353,9 +353,47 @@ test("targeted refresh uses paginated REST APIs, not proxy-incompatible gh pr vi
});

test("targeted refresh fails closed when metadata is incomplete or the PR is absent", () => {
for (const result of [null, { ...restPr, number: 43 }, { ...restPr, updated_at: null }, { ...restPr, head: null }, { ...restPr, base: null }]) {
assert.throws(() => fetchCandidate("github/gh-aw", "42", () => result), /could not refresh PR metadata/);
for (const result of [
null,
{ ...graphPr, number: 43 },
{ ...graphPr, updatedAt: null },
{ ...graphPr, headRefOid: null },
{ ...graphPr, headRefName: null },
{ ...graphPr, baseRefName: null },
{ ...graphPr, state: null },
{ ...graphPr, isDraft: null },
{ ...graphPr, labels: null },
{ ...graphPr, labels: { nodes: [null], pageInfo: { hasNextPage: false } } },
{ ...graphPr, labels: { nodes: [{}], pageInfo: { hasNextPage: false } } },
{ ...graphPr, labels: { nodes: [], pageInfo: { hasNextPage: true } } },
]) {
assert.throws(() => fetchCandidate("github/gh-aw", "42", () => metadataResponse(result)), /could not refresh PR metadata/);
}
});

test("proxy-filtered and GraphQL error responses never become successful refreshes", () => {
for (const response of [[], {}, null, { data: null }, { data: { repository: null } }, { ...metadataResponse(), errors: [{ message: "access denied" }] }]) {
let calls = 0;
assert.throws(
() =>
fetchCandidate("github/gh-aw", "42", () => {
calls++;
return response;
}),
/could not refresh PR metadata|access denied/
);
assert.equal(calls, 1);
}
});

test("GraphQL metadata normalizes conflicts and preserves branch states and opted-out labels", () => {
for (const mergeStateStatus of ["DIRTY", "BEHIND", "BLOCKED", "UNKNOWN"]) {
const { api } = queueAPI({ candidate: { ...graphPr, mergeStateStatus } });
assert.equal(fetchCandidate("github/gh-aw", "42", api, now).mergeStateStatus, mergeStateStatus === "DIRTY" ? "CONFLICTING" : mergeStateStatus);
}
const { api, calls } = queueAPI({ candidate: { ...graphPr, labels: { nodes: [{ name: "broccoli" }], pageInfo: { hasNextPage: false } } } });
assert.equal(fetchQueue("github/gh-aw", "42", api, now).skipped[0].skip_reason, "opted_out");
assert.equal(calls.length, 2);
});

test("deduplication finds a nudge beyond the first hundred comments", () => {
Expand Down Expand Up @@ -657,15 +695,15 @@ const budgets = {
};
const idleMsForTest = 10 * 60 * 1000;

function queueAPI({ listed = [pr], candidate = restPr, checks = [], statuses = [], data = detail(), comments = [], runs = [], approvals = [] } = {}) {
function queueAPI({ listed = [pr], candidate = graphPr, checks = [], statuses = [], data = detail(), comments = [], runs = [], approvals = [] } = {}) {
const calls = [];
return {
calls,
api(args) {
calls.push(args);
if (args[1] === "rate_limit") return budgets;
if (args[0] === "pr") return listed;
if (args[1].includes("/pulls/")) return candidate;
if (args[1] === "graphql" && args[3].includes("headRefOid")) return metadataResponse(candidate);
if (args[1].includes("/check-runs?")) return [{ check_runs: checks }];
if (args[1].includes("/status?")) return [{ statuses }];
if (args[1].includes("/actions/runs?")) return [{ workflow_runs: args[1].includes("status=action_required") ? approvals : runs }];
Expand Down Expand Up @@ -700,21 +738,21 @@ test("cheap metadata exclusions avoid all detail and active-run reads", () => {

test("targeted refresh preserves metadata gates before fetching checks", () => {
for (const [changes, reason] of [
[{ draft: true }, "closed_or_draft"],
[{ state: "closed" }, "closed_or_draft"],
[{ labels: [{ name: "broccoli" }] }, "opted_out"],
[{ updated_at: "2026-10-03T17:55:00Z" }, "not_idle"],
[{ user: { login: "renovate[bot]" } }, "dependency_bot"],
[{ isDraft: true }, "closed_or_draft"],
[{ state: "CLOSED" }, "closed_or_draft"],
[{ labels: { nodes: [{ name: "broccoli" }], pageInfo: { hasNextPage: false } } }, "opted_out"],
[{ updatedAt: "2026-10-03T17:55:00Z" }, "not_idle"],
[{ author: { login: "renovate[bot]" } }, "dependency_bot"],
]) {
const { api, calls } = queueAPI({ candidate: { ...restPr, ...changes } });
const { api, calls } = queueAPI({ candidate: { ...graphPr, ...changes } });
const queue = fetchQueue("github/gh-aw", "42", api, now);
assert.deepEqual(queue.skipped, [{ pr_number: 42, skip_reason: reason }]);
assert.equal(calls.length, 2);
}
});

test("metadata is rechecked when the PR changed after list enumeration", () => {
const { api, calls } = queueAPI({ candidate: { ...restPr, updated_at: "2026-10-03T17:55:00Z" } });
const { api, calls } = queueAPI({ candidate: { ...graphPr, updatedAt: "2026-10-03T17:55:00Z" } });
assert.equal(fetchQueue("github/gh-aw", undefined, api, now).skipped[0].skip_reason, "not_idle");
assert.equal(calls.length, 4);
});
Expand All @@ -726,7 +764,7 @@ test("pending checks and active agent checks avoid review and run lookups", () =
]) {
const { api, calls } = queueAPI({ checks: [check] });
assert.equal(fetchQueue("github/gh-aw", undefined, api, now).skipped[0].skip_reason, reason);
assert.ok(!calls.some(args => args[1] === "graphql" || args[1].includes("/actions/runs?")));
assert.ok(!calls.some(args => (args[1] === "graphql" && args[3].includes("reviewThreads")) || args[1].includes("/actions/runs?")));
}
});

Expand All @@ -736,7 +774,7 @@ test("approval-required runs are discovered by conclusion, scoped to the PR bran
{ name: "CGO", status: "waiting" },
]) {
const { api, calls } = queueAPI({
candidate: { ...restPr, mergeable_state: "blocked" },
candidate: { ...graphPr, mergeStateStatus: "BLOCKED" },
checks: [check],
approvals: [{ ...waitingRun, status: "completed", conclusion: "action_required" }],
});
Expand All @@ -751,7 +789,7 @@ test("approval-required runs are discovered by conclusion, scoped to the PR bran

test("healthy idle PRs and human-only approvals produce empty queues", () => {
for (const checks of [[], [{ name: "Other CI", status: "completed", conclusion: "action_required" }]]) {
const { api } = queueAPI({ candidate: { ...restPr, mergeable_state: "clean" }, checks });
const { api } = queueAPI({ candidate: { ...graphPr, mergeStateStatus: "CLEAN" }, checks });
const queue = fetchQueue("github/gh-aw", undefined, api, now);
assert.deepEqual(queue.prs, []);
assert.equal(queue.skipped[0].skip_reason, checks.length ? "approval_required" : "nothing_actionable");
Expand Down
Loading