From ba68be29c93c49e4ed2f74f72a1e200e1bebab6f Mon Sep 17 00:00:00 2001 From: mshk <29593+mshk@users.noreply.github.com> Date: Fri, 9 Oct 2026 20:41:09 +0900 Subject: [PATCH 1/4] test(poteto-mode): catch stale base ref OIDs --- .../scripts/watch-pr/fakes.test-helper.ts | 10 ++- .../scripts/watch-pr/github.test.ts | 33 +++++++- .../scripts/watch-pr/integrity.test.ts | 34 +++++++- .../scripts/watch-pr/shipping-cli.test.ts | 3 +- .../scripts/watch-pr/shipping.test.ts | 82 ++++++++++++++++++- .../scripts/watch-pr/transport.test.ts | 4 +- 6 files changed, 152 insertions(+), 14 deletions(-) diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/fakes.test-helper.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/fakes.test-helper.ts index 4bc68d5a..8ab0c6ee 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/fakes.test-helper.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/fakes.test-helper.ts @@ -98,11 +98,15 @@ export function fakeReader( async pullRequest(requested) { calls.push("pullRequest"); reads += 1; + const facts = { + ...defaults, + ...options.facts, + ...(reads > 1 ? options.factsOnReread : {}), + }; return parsePullRequest( { - ...defaults, - ...options.facts, - ...(reads > 1 ? options.factsOnReread : {}), + ...facts, + baseRef: { target: { oid: facts.baseRefOid } }, }, requested ); diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts index c6a06b49..ef7deeab 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts @@ -9,6 +9,7 @@ import { parseFastCheck, parsePullRequest, parseReviewThreads, + PR_FACTS_QUERY, resolveChecks, resolveContext, runJson, @@ -299,7 +300,8 @@ describe("closed enum parsing", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "base", + baseRefOid: "stale-base", + baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", state: "OPEN", @@ -329,6 +331,35 @@ describe("closed enum parsing", () => { ).toThrow(WatcherQueryError); }); + it("uses the current base target when the scalar base OID is stale", () => { + expect( + parsePullRequest( + { + ...rawPullRequest, + baseRefOid: "stale-base", + baseRef: { target: { oid: "current-base" } }, + }, + context + ).baseRefOid + ).toBe("current-base"); + }); + + it("queries the current base target with the other PR facts", () => { + expect(PR_FACTS_QUERY).toContain("baseRef { target { oid } }"); + expect(PR_FACTS_QUERY).not.toContain("baseRefOid"); + }); + + it("fails closed when the current base ref is null or missing", () => { + const missingBaseRef = Object.fromEntries( + Object.entries(rawPullRequest).filter(([key]) => key !== "baseRef") + ); + for (const value of [ + { ...rawPullRequest, baseRef: null }, + missingBaseRef, + ]) + expect(() => parsePullRequest(value, context)).toThrow(WatcherQueryError); + }); + it("rejects unknown enum values as retryable errors carrying the raw value", () => { try { parsePullRequest( diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts index cff627b1..413248f2 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts @@ -47,7 +47,8 @@ describe("commit identity", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "base", + baseRefOid: "stale-base", + baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", state: "OPEN", @@ -56,7 +57,10 @@ describe("commit identity", () => { }; it("rejects an open PR without a head or base commit where it is parsed", () => { - for (const missing of [{ headRefOid: null }, { baseRefOid: "" }]) + for (const missing of [ + { headRefOid: null }, + { baseRef: { target: { oid: "" } } }, + ]) expect(() => parsePullRequest({ ...rawPullRequest, ...missing }, context) ).toThrow(WatcherQueryError); @@ -70,7 +74,7 @@ describe("commit identity", () => { state: "MERGED", mergedAt: "2026-07-26T00:00:00Z", headRefOid: null, - baseRefOid: null, + baseRef: null, }, context ) @@ -135,6 +139,28 @@ describe("commit identity", () => { ); }); + it("detects current base movement when the PR's scalar base OID stays stale", async () => { + const base = fakeReader(); + let reads = 0; + const reader = { + ...base, + async pullRequest(requested: typeof context) { + const baseRefOid = reads++ === 0 ? "base" : "advanced"; + return parsePullRequest( + { + ...rawPullRequest, + baseRefOid: "stale-base", + baseRef: { target: { oid: baseRefOid } }, + }, + requested + ); + }, + }; + await expect(readSnapshot({ ...snapshotArgs, reader })).rejects.toThrow( + "PR changed while collecting" + ); + }); + it("cannot report ready when the destination query is unavailable", async () => { const base = fakeReader(); let reads = 0; @@ -842,7 +868,7 @@ describe("landing validators", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: null, - baseRefOid: "base", + baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", state: "OPEN", diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts index 565fe5fe..66d61adb 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts @@ -31,6 +31,7 @@ function fixture( headRefOid: "head", baseRefName: "main", baseRefOid: "base", + baseRef: { target: { oid: "base" } }, autoMergeRequest: { enabledAt: "now" }, mergeQueueEntry: { id: "queue" }, mergeCommit: null, @@ -106,7 +107,7 @@ it("the CLI refuses a changed base without cancelling anything", () => writeFileSync(saved, JSON.stringify(inspected.output)); const changed = { ...JSON.parse(readFileSync(file, "utf8")), - baseRefOid: "advanced", + baseRef: { target: { oid: "advanced" } }, }; writeFileSync(file, JSON.stringify(changed)); const result = run("cancel-pending", "--record", saved); diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts index cd36f6a0..23e77cc6 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts @@ -180,7 +180,8 @@ const raw = { state: "OPEN", headRefOid: "head", baseRefName: "main", - baseRefOid: "base", + baseRefOid: "stale-base", + baseRef: { target: { oid: "base" } }, autoMergeRequest: null, mergeQueueEntry: null, mergeCommit: null, @@ -190,6 +191,18 @@ const response = (pullRequest: unknown) => ({ }); describe("shipping GitHub boundary", () => { + it("reads the current base target instead of the PR's stored base OID", async () => { + let query = ""; + const service = new GhShippingService(async (args) => { + query = args.find((arg) => arg.startsWith("query=")) ?? ""; + return response(raw); + }); + expect(await service.inspect(context)).toMatchObject({ + revision: { baseRefOid: "base" }, + }); + expect(query).toContain("baseRef { target { oid } }"); + }); + it("parses the complete record and distinguishes queue-only pending state", async () => { const service = new GhShippingService(async () => response({ ...raw, mergeQueueEntry: { id: "queued" } }) @@ -201,7 +214,7 @@ describe("shipping GitHub boundary", () => { }); for (const field of [ - "baseRefOid", + "baseRef", "autoMergeRequest", "mergeQueueEntry", "mergeCommit", @@ -243,6 +256,67 @@ describe("shipping GitHub boundary", () => { }); }); + it("stops before dequeue when the current base moves during readback", async () => { + const reads = [ + { + ...raw, + baseRef: { target: { oid: "base" } }, + autoMergeRequest: { enabledAt: "now" }, + mergeQueueEntry: { id: "queue" }, + }, + { + ...raw, + baseRef: { target: { oid: "base" } }, + autoMergeRequest: { enabledAt: "now" }, + mergeQueueEntry: { id: "queue" }, + }, + { + ...raw, + baseRef: { target: { oid: "advanced" } }, + autoMergeRequest: null, + mergeQueueEntry: { id: "queue" }, + }, + { + ...raw, + baseRef: { target: { oid: "advanced" } }, + autoMergeRequest: null, + mergeQueueEntry: null, + }, + ]; + const mutations: string[] = []; + const service = new GhShippingService(async (args) => { + const query = args.find((arg) => arg.startsWith("query=")) ?? ""; + if (query.includes("disablePullRequestAutoMerge")) { + mutations.push("disable"); + return { data: { disablePullRequestAutoMerge: {} } }; + } + if (query.includes("dequeuePullRequest")) { + mutations.push("dequeue"); + return { data: { dequeuePullRequest: {} } }; + } + const pullRequest = reads.shift(); + if (!pullRequest) throw new Error("unexpected inspection"); + return response(pullRequest); + }); + const expected = await service.inspect(context); + + expect(await cancelPending(service, expected)).toMatchObject({ + kind: "changed", + expected: { revision: { baseRefOid: "base" } }, + observed: { revision: { baseRefOid: "advanced" } }, + }); + expect(mutations).toEqual(["disable"]); + }); + + it("treats a null base ref as unavailable for an open PR", async () => { + const service = new GhShippingService(async () => + response({ ...raw, baseRef: null }) + ); + expect(await inspectLanding(service, context)).toMatchObject({ + kind: "unavailable", + }); + }); + it("rejects malformed saved context before it can select a PR", () => { expect(() => parseLandingRecord({ @@ -262,8 +336,8 @@ describe("shipping validation detail", () => { ], [ "an empty base commit", - { ...raw, baseRefOid: "" }, - "baseRefOid must be a non-empty string", + { ...raw, baseRef: { target: { oid: "" } } }, + "baseRef.target.oid must be a non-empty string", ], [ "a malformed queue entry", diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts index a42087ba..a00a2490 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts @@ -37,7 +37,9 @@ if (scenario === 'slow') { const noCi = scenario.startsWith('no-ci'); const fail = (stderr) => { console.error(stderr); process.exit(1); }; let value; -if (args[0] === 'pr' && args[1] === 'view') { +if (args.some(a => a.includes('query PullRequestFacts'))) { + value = { data: { repository: { pullRequest: { headRefOid: 'head', baseRef: { target: { oid: 'base' } }, mergeable: 'MERGEABLE', mergeStateStatus: scenario === 'no-ci-blocked' ? 'BLOCKED' : 'CLEAN', reviewDecision: noCi ? null : 'APPROVED', headRefName: 'feature', baseRefName: 'main', state: 'OPEN', mergedAt: null, isDraft: false } } } }; +} else if (args[0] === 'pr' && args[1] === 'view') { value = { headRefOid: 'head', baseRefOid: 'base', mergeable: 'MERGEABLE', mergeStateStatus: scenario === 'no-ci-blocked' ? 'BLOCKED' : 'CLEAN', reviewDecision: noCi ? '' : 'APPROVED', headRefName: 'feature', baseRefName: 'main', state: 'OPEN', mergedAt: null, isDraft: false }; } else if (args[0] === 'pr' && args[1] === 'checks') { if (scenario === 'no-ci-unauthorized') fail('HTTP 401: Bad credentials (https://api.github.com/graphql)'); From 44e6cb853e459bfba6c9afa51419771595ecba5d Mon Sep 17 00:00:00 2001 From: mshk <29593+mshk@users.noreply.github.com> Date: Fri, 9 Oct 2026 20:41:09 +0900 Subject: [PATCH 2/4] fix(poteto-mode): read current base ref targets --- .../poteto-mode/scripts/watch-pr/github.ts | 44 +++++++++++++------ .../poteto-mode/scripts/watch-pr/landing.ts | 8 ++++ .../poteto-mode/scripts/watch-pr/shipping.ts | 7 ++- 3 files changed, 44 insertions(+), 15 deletions(-) diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts index 3e3b6f97..726a3866 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts @@ -1,4 +1,4 @@ -import { parseLandingRevision } from "./landing.ts"; +import { parseBaseRefTargetOid, parseLandingRevision } from "./landing.ts"; import { spawn } from "node:child_process"; import { DeadlineExceeded, type WatchDeadline } from "./deadline.ts"; import type * as T from "./types.ts"; @@ -21,6 +21,22 @@ export const REVIEW_THREADS_QUERY = `query ReviewThreads($owner: String!, $repo: }`; export const PR_COMMIT_STATUS_QUERY = "\nquery PrCommitStatuses($owner: String!, $repo: String!, $pr: Int!) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $pr) {\n commits(last: 50) {\n nodes {\n commit {\n oid\n statusCheckRollup {\n state\n }\n }\n }\n }\n }\n }\n}\n"; +export const PR_FACTS_QUERY = `query PullRequestFacts($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + mergeable + mergeStateStatus + reviewDecision + headRefOid + headRefName + baseRefName + baseRef { target { oid } } + state + mergedAt + isDraft + } + } +}`; export const PR_CHECK_ROLLUP_QUERY = "\nquery PrCheckRollup($owner: String!, $repo: String!, $pr: Int!, $after: String) {\n repository(owner: $owner, name: $repo) {\n pullRequest(number: $pr) {\n commits(last: 1) {\n nodes {\n commit {\n statusCheckRollup {\n contexts(first: 100, after: $after) {\n pageInfo {\n hasNextPage\n endCursor\n }\n nodes {\n __typename\n ... on CheckRun {\n name\n status\n conclusion\n detailsUrl\n }\n ... on StatusContext {\n context\n state\n targetUrl\n }\n }\n }\n }\n }\n }\n }\n }\n }\n}\n"; @@ -471,6 +487,7 @@ export function parsePullRequest( const object = record(value, "pull request"); if (typeof object.isDraft !== "boolean") missing("pull request.isDraft", object.isDraft); + const baseRefOid = parseBaseRefTargetOid(object.baseRef); const facts = { context, mergeable: enumValue( @@ -485,7 +502,7 @@ export function parsePullRequest( ), reviewDecision: reviewDecision(object.reviewDecision), headRefOid: optionalString(object.headRefOid, "pull request.headRefOid"), - baseRefOid: optionalString(object.baseRefOid, "pull request.baseRefOid"), + baseRefOid, headRefName: string(object.headRefName, "pull request.headRefName"), baseRefName: string(object.baseRefName, "pull request.baseRefName"), state: enumValue( @@ -497,7 +514,11 @@ export function parsePullRequest( isDraft: object.isDraft, }; return facts.state === "OPEN" - ? { ...facts, ...parseLandingRevision(object, context), state: facts.state } + ? { + ...facts, + ...parseLandingRevision({ ...object, baseRefOid }, context), + state: facts.state, + } : { ...facts, state: facts.state }; } function graphqlArgs( @@ -543,17 +564,14 @@ export class GhGitHubReader implements T.GitHubReader { }; } async pullRequest(context: T.PrContext): Promise { + const response = record( + await this.runJson(graphqlArgs(PR_FACTS_QUERY, context)), + "GraphQL response" + ); + if (response.errors !== undefined) + missing("GraphQL errors", response.errors); return parsePullRequest( - await this.runJson([ - "gh", - "pr", - "view", - String(context.number), - "--repo", - `${context.owner}/${context.repo}`, - "--json", - "mergeable,mergeStateStatus,reviewDecision,headRefOid,headRefName,baseRefName,baseRefOid,state,mergedAt,isDraft", - ]), + at(response, ["data", "repository", "pullRequest"]), context ); } diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts index 3fc7864b..c271ce24 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts @@ -27,6 +27,14 @@ export function text(value: unknown, label: string): string { export const nullableText = (value: unknown, label: string): string | null => value === null ? null : text(value, label); +export function parseBaseRefTargetOid(value: unknown): string | null { + if (value === null) return null; + const baseRef = object(value, "baseRef"); + if (baseRef.target === null) return null; + const target = object(baseRef.target, "baseRef.target"); + return nullableText(target.oid, "baseRef.target.oid"); +} + export function oneOf( value: unknown, values: V, diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts index 2ab22635..6768f420 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts @@ -1,5 +1,6 @@ import { flag, + parseBaseRefTargetOid, nullableText, object, oneOf, @@ -117,7 +118,8 @@ export async function cancelPending( const INSPECT_QUERY = `query Landing($owner:String!,$repo:String!,$pr:Int!) { repository(owner:$owner,name:$repo) { pullRequest(number:$pr) { - id state headRefOid baseRefName baseRefOid + id state headRefOid baseRefName + baseRef { target { oid } } autoMergeRequest { enabledAt } mergeQueueEntry { id } mergeCommit { oid } @@ -158,6 +160,7 @@ export class GhShippingService implements ShippingService { object(response.repository, "repository").pullRequest, "pullRequest" ); + const baseRefOid = parseBaseRefTargetOid(fields.baseRef); const autoMerge = fields.autoMergeRequest === null ? false @@ -175,7 +178,7 @@ export class GhShippingService implements ShippingService { "queue entry id" ); return { - revision: parseLandingRevision(fields, context), + revision: parseLandingRevision({ ...fields, baseRefOid }, context), pullRequestId: text(fields.id, "pull request id"), state: oneOf(fields.state, STATES, "PR state"), pending: { autoMerge, queueEntryId }, From 8db5cfce6e84c54d4cd5b8e30bf40c72335dd19e Mon Sep 17 00:00:00 2001 From: mshk <29593+mshk@users.noreply.github.com> Date: Fri, 9 Oct 2026 20:41:09 +0900 Subject: [PATCH 3/4] style(poteto-mode): format base ref regression --- .../skills/poteto-mode/scripts/watch-pr/github.test.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts index ef7deeab..bc01229c 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts @@ -353,10 +353,7 @@ describe("closed enum parsing", () => { const missingBaseRef = Object.fromEntries( Object.entries(rawPullRequest).filter(([key]) => key !== "baseRef") ); - for (const value of [ - { ...rawPullRequest, baseRef: null }, - missingBaseRef, - ]) + for (const value of [{ ...rawPullRequest, baseRef: null }, missingBaseRef]) expect(() => parsePullRequest(value, context)).toThrow(WatcherQueryError); }); From e5cf03dda9ab463d2f7a7d9bef2952023bf90cdc Mon Sep 17 00:00:00 2001 From: Michael Denyer <97485362+michael-denyer@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:28:43 +0100 Subject: [PATCH 4/4] refactor(watch-pr): unwrap GraphQL once and drop the gh pr view leftovers GhGitHubReader reads every GraphQL envelope through one graphql() method, which checks errors and returns data.repository.pullRequest for all four queries. Before, only the PR facts query checked errors. An open PR's landing revision narrows the parsed facts with text() instead of re-parsing a copy of the raw response under the old baseRefOid key. ship-pr inspect builds its revision from the GraphQL fields directly. parseLandingRevision parses the saved record only, so its optional context parameter goes. The empty-string review decision came from gh pr view, which the facts reader no longer runs. GraphQL returns null, and an empty string is rejected like any other unknown value. The transport fake's pr view branch and the baseRefOid decoys in shared fixtures go with it. --- .../scripts/watch-pr/github.test.ts | 12 +-- .../poteto-mode/scripts/watch-pr/github.ts | 78 +++++++++---------- .../scripts/watch-pr/integrity.test.ts | 10 +-- .../poteto-mode/scripts/watch-pr/landing.ts | 9 +-- .../scripts/watch-pr/shipping-cli.test.ts | 1 - .../scripts/watch-pr/shipping.test.ts | 1 - .../poteto-mode/scripts/watch-pr/shipping.ts | 13 +++- .../scripts/watch-pr/transport.test.ts | 2 - 8 files changed, 62 insertions(+), 64 deletions(-) diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts index bc01229c..71e06722 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts @@ -300,7 +300,6 @@ describe("closed enum parsing", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "stale-base", baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", @@ -318,11 +317,10 @@ describe("closed enum parsing", () => { ).toBe("CONFLICTING"); }); - it("reads gh's empty reviewDecision as no decision rather than a parse failure", () => { - expect( + it("rejects an empty reviewDecision like any other unknown enum value", () => { + expect(() => parsePullRequest({ ...rawPullRequest, reviewDecision: "" }, context) - .reviewDecision - ).toBeNull(); + ).toThrow(WatcherQueryError); }); it("still rejects an unknown reviewDecision", () => { @@ -434,7 +432,9 @@ it("annotates Bugbot threads with distinct review-pass counts", () => { }, }, }; - const threads = parseReviewThreads(response); + const threads = parseReviewThreads( + response.data.repository.pullRequest.reviewThreads.nodes + ); expect(threads).toHaveLength(2); expect(threads.map((thread) => thread.isBugbot)).toEqual([true, true]); expect(threads.map((thread) => thread.bugbotReviewPasses)).toEqual([3, 3]); diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts index 726a3866..243af083 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts @@ -1,4 +1,4 @@ -import { parseBaseRefTargetOid, parseLandingRevision } from "./landing.ts"; +import { baseRefTargetOid, text } from "./landing.ts"; import { spawn } from "node:child_process"; import { DeadlineExceeded, type WatchDeadline } from "./deadline.ts"; import type * as T from "./types.ts"; @@ -221,15 +221,8 @@ const REVIEW_DECISIONS = [ "CHANGES_REQUESTED", "REVIEW_REQUIRED", ] as const; -// `gh pr view` reports no review decision as "", not null. Only this field does -// it, so the normalization stays here rather than in nullableEnum, where it -// would stop a genuinely unexpected rollup state from failing closed. const reviewDecision = (value: unknown): T.ReviewDecision => - nullableEnum( - value === "" ? null : value, - REVIEW_DECISIONS, - "pull request.reviewDecision" - ); + nullableEnum(value, REVIEW_DECISIONS, "pull request.reviewDecision"); function parseRemote(value: string): T.Repository | null { let normalized = value.trim(); if (normalized.startsWith("git@github.com:")) @@ -438,11 +431,9 @@ function passKey(comment: T.ReviewComment | null): string | null { } return null; } -export function parseReviewThreads(value: unknown): readonly T.ReviewThread[] { - const nodes = list( - at(value, ["data", "repository", "pullRequest", "reviewThreads", "nodes"]), - "reviewThreads.nodes" - ); +export function parseReviewThreads( + nodes: readonly unknown[] +): readonly T.ReviewThread[] { const threads: { readonly id: string; readonly firstComment: T.ReviewComment | null; @@ -487,7 +478,6 @@ export function parsePullRequest( const object = record(value, "pull request"); if (typeof object.isDraft !== "boolean") missing("pull request.isDraft", object.isDraft); - const baseRefOid = parseBaseRefTargetOid(object.baseRef); const facts = { context, mergeable: enumValue( @@ -502,7 +492,7 @@ export function parsePullRequest( ), reviewDecision: reviewDecision(object.reviewDecision), headRefOid: optionalString(object.headRefOid, "pull request.headRefOid"), - baseRefOid, + baseRefOid: baseRefTargetOid(object.baseRef), headRefName: string(object.headRefName, "pull request.headRefName"), baseRefName: string(object.baseRefName, "pull request.baseRefName"), state: enumValue( @@ -516,8 +506,10 @@ export function parsePullRequest( return facts.state === "OPEN" ? { ...facts, - ...parseLandingRevision({ ...object, baseRefOid }, context), state: facts.state, + headRefOid: text(facts.headRefOid, "headRefOid"), + baseRefName: text(facts.baseRefName, "baseRefName"), + baseRefOid: text(facts.baseRefOid, "baseRefOid"), } : { ...facts, state: facts.state }; } @@ -563,15 +555,24 @@ export class GhGitHubReader implements T.GitHubReader { number: pr ?? parsePrNumber(object.number, "current PR.number"), }; } - async pullRequest(context: T.PrContext): Promise { - const response = record( - await this.runJson(graphqlArgs(PR_FACTS_QUERY, context)), - "GraphQL response" - ); + private async graphql( + query: string, + context: T.PrContext, + after: string | null = null + ): Promise> { + const argv = graphqlArgs(query, context); + if (after !== null) argv.push("-f", `after=${after}`); + const response = record(await this.runJson(argv), "GraphQL response"); if (response.errors !== undefined) missing("GraphQL errors", response.errors); - return parsePullRequest( + return record( at(response, ["data", "repository", "pullRequest"]), + "data.repository.pullRequest" + ); + } + async pullRequest(context: T.PrContext): Promise { + return parsePullRequest( + await this.graphql(PR_FACTS_QUERY, context), context ); } @@ -668,11 +669,13 @@ export class GhGitHubReader implements T.GitHubReader { context: T.PrContext, after: string | null ): Promise { - const argv = graphqlArgs(PR_CHECK_ROLLUP_QUERY, context); - if (after !== null) argv.push("-f", `after=${after}`); - const value = await this.runJson(argv); + const pullRequest = await this.graphql( + PR_CHECK_ROLLUP_QUERY, + context, + after + ); const commits = list( - at(value, ["data", "repository", "pullRequest", "commits", "nodes"]), + at(pullRequest, ["commits", "nodes"]), "commits.nodes" ); if (commits.length === 0) @@ -709,13 +712,12 @@ export class GhGitHubReader implements T.GitHubReader { const cursors = new Set(); let after: string | null = null; do { - const argv = graphqlArgs(REVIEW_THREADS_QUERY, context); - if (after !== null) argv.push("-f", `after=${after}`); - const value = await this.runJson(argv); - const connection = record( - at(value, ["data", "repository", "pullRequest", "reviewThreads"]), - "reviewThreads" + const pullRequest = await this.graphql( + REVIEW_THREADS_QUERY, + context, + after ); + const connection = record(pullRequest.reviewThreads, "reviewThreads"); nodes.push(...list(connection.nodes, "reviewThreads.nodes")); const page = record(connection.pageInfo, "reviewThreads.pageInfo"); if (typeof page.hasNextPage !== "boolean") @@ -729,18 +731,14 @@ export class GhGitHubReader implements T.GitHubReader { cursors.add(after); } } while (after !== null); - return parseReviewThreads({ - data: { repository: { pullRequest: { reviewThreads: { nodes } } } }, - }); + return parseReviewThreads(nodes); } async commitRollups( context: T.PrContext ): Promise { - const value = await this.runJson( - graphqlArgs(PR_COMMIT_STATUS_QUERY, context) - ); + const pullRequest = await this.graphql(PR_COMMIT_STATUS_QUERY, context); const commits = list( - at(value, ["data", "repository", "pullRequest", "commits", "nodes"]), + at(pullRequest, ["commits", "nodes"]), "commits.nodes" ); return commits.map((item, index) => { diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts index 413248f2..d3101ab1 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/integrity.test.ts @@ -47,7 +47,6 @@ describe("commit identity", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "stale-base", baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", @@ -835,10 +834,11 @@ describe("landing validators", () => { [ "parseLandingRevision", () => - parseLandingRevision( - { headRefOid: "head", baseRefName: "main" }, - context - ), + parseLandingRevision({ + context, + headRefOid: "head", + baseRefName: "main", + }), "baseRefOid must be a non-empty string", ], ] as const) diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts index c271ce24..2fe6d223 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts @@ -27,7 +27,7 @@ export function text(value: unknown, label: string): string { export const nullableText = (value: unknown, label: string): string | null => value === null ? null : text(value, label); -export function parseBaseRefTargetOid(value: unknown): string | null { +export function baseRefTargetOid(value: unknown): string | null { if (value === null) return null; const baseRef = object(value, "baseRef"); if (baseRef.target === null) return null; @@ -58,13 +58,10 @@ export function parseContext(value: unknown): PrContext { return { owner, repo, number: parsePrNumber(fields.number) }; } -export function parseLandingRevision( - value: unknown, - context?: PrContext -): LandingRevision { +export function parseLandingRevision(value: unknown): LandingRevision { const fields = object(value, "landing revision"); return { - context: context ?? parseContext(fields.context), + context: parseContext(fields.context), headRefOid: text(fields.headRefOid, "headRefOid"), baseRefName: text(fields.baseRefName, "baseRefName"), baseRefOid: text(fields.baseRefOid, "baseRefOid"), diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts index 66d61adb..75df8294 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping-cli.test.ts @@ -30,7 +30,6 @@ function fixture( state: "OPEN", headRefOid: "head", baseRefName: "main", - baseRefOid: "base", baseRef: { target: { oid: "base" } }, autoMergeRequest: { enabledAt: "now" }, mergeQueueEntry: { id: "queue" }, diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts index 23e77cc6..08ce5ea4 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.test.ts @@ -180,7 +180,6 @@ const raw = { state: "OPEN", headRefOid: "head", baseRefName: "main", - baseRefOid: "stale-base", baseRef: { target: { oid: "base" } }, autoMergeRequest: null, mergeQueueEntry: null, diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts index 6768f420..266dac5c 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts @@ -1,6 +1,6 @@ import { + baseRefTargetOid, flag, - parseBaseRefTargetOid, nullableText, object, oneOf, @@ -160,7 +160,6 @@ export class GhShippingService implements ShippingService { object(response.repository, "repository").pullRequest, "pullRequest" ); - const baseRefOid = parseBaseRefTargetOid(fields.baseRef); const autoMerge = fields.autoMergeRequest === null ? false @@ -178,7 +177,15 @@ export class GhShippingService implements ShippingService { "queue entry id" ); return { - revision: parseLandingRevision({ ...fields, baseRefOid }, context), + revision: { + context, + headRefOid: text(fields.headRefOid, "headRefOid"), + baseRefName: text(fields.baseRefName, "baseRefName"), + baseRefOid: text( + baseRefTargetOid(fields.baseRef), + "baseRef.target.oid" + ), + }, pullRequestId: text(fields.id, "pull request id"), state: oneOf(fields.state, STATES, "PR state"), pending: { autoMerge, queueEntryId }, diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts index a00a2490..8a8751a2 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/transport.test.ts @@ -39,8 +39,6 @@ const fail = (stderr) => { console.error(stderr); process.exit(1); }; let value; if (args.some(a => a.includes('query PullRequestFacts'))) { value = { data: { repository: { pullRequest: { headRefOid: 'head', baseRef: { target: { oid: 'base' } }, mergeable: 'MERGEABLE', mergeStateStatus: scenario === 'no-ci-blocked' ? 'BLOCKED' : 'CLEAN', reviewDecision: noCi ? null : 'APPROVED', headRefName: 'feature', baseRefName: 'main', state: 'OPEN', mergedAt: null, isDraft: false } } } }; -} else if (args[0] === 'pr' && args[1] === 'view') { - value = { headRefOid: 'head', baseRefOid: 'base', mergeable: 'MERGEABLE', mergeStateStatus: scenario === 'no-ci-blocked' ? 'BLOCKED' : 'CLEAN', reviewDecision: noCi ? '' : 'APPROVED', headRefName: 'feature', baseRefName: 'main', state: 'OPEN', mergedAt: null, isDraft: false }; } else if (args[0] === 'pr' && args[1] === 'checks') { if (scenario === 'no-ci-unauthorized') fail('HTTP 401: Bad credentials (https://api.github.com/graphql)'); if (noCi) fail("no checks reported on the 'feature' branch");