Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import type {
Check,
ChecksFastPath,
Expand Down Expand Up @@ -98,11 +98,15 @@
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
);
Expand Down
40 changes: 34 additions & 6 deletions plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import { describe, expect, it } from "bun:test";
import { WatchDeadline } from "./deadline.ts";
import {
Expand All @@ -9,6 +9,7 @@
parseFastCheck,
parsePullRequest,
parseReviewThreads,
PR_FACTS_QUERY,
resolveChecks,
resolveContext,
runJson,
Expand Down Expand Up @@ -299,7 +300,7 @@
mergeStateStatus: "CLEAN",
reviewDecision: "APPROVED",
headRefOid: "head",
baseRefOid: "base",
baseRef: { target: { oid: "base" } },
headRefName: "feature",
baseRefName: "main",
state: "OPEN",
Expand All @@ -316,11 +317,10 @@
).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", () => {
Expand All @@ -329,6 +329,32 @@
).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(
Expand Down Expand Up @@ -406,7 +432,9 @@
},
},
};
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]);
Expand Down
102 changes: 59 additions & 43 deletions plugins/pstack/skills/poteto-mode/scripts/watch-pr/github.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { 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";
Expand All @@ -21,6 +21,22 @@
}`;
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";

Expand Down Expand Up @@ -205,15 +221,8 @@
"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:"))
Expand Down Expand Up @@ -422,11 +431,9 @@
}
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;
Expand Down Expand Up @@ -485,7 +492,7 @@
),
reviewDecision: reviewDecision(object.reviewDecision),
headRefOid: optionalString(object.headRefOid, "pull request.headRefOid"),
baseRefOid: optionalString(object.baseRefOid, "pull request.baseRefOid"),
baseRefOid: baseRefTargetOid(object.baseRef),
headRefName: string(object.headRefName, "pull request.headRefName"),
baseRefName: string(object.baseRefName, "pull request.baseRefName"),
state: enumValue(
Expand All @@ -497,7 +504,13 @@
isDraft: object.isDraft,
};
return facts.state === "OPEN"
? { ...facts, ...parseLandingRevision(object, context), state: facts.state }
? {
...facts,
state: facts.state,
headRefOid: text(facts.headRefOid, "headRefOid"),
baseRefName: text(facts.baseRefName, "baseRefName"),
baseRefOid: text(facts.baseRefOid, "baseRefOid"),
}
: { ...facts, state: facts.state };
}
function graphqlArgs(
Expand Down Expand Up @@ -542,18 +555,24 @@
number: pr ?? parsePrNumber(object.number, "current PR.number"),
};
}
private async graphql(
query: string,
context: T.PrContext,
after: string | null = null
): Promise<Record<string, unknown>> {
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 record(
at(response, ["data", "repository", "pullRequest"]),
"data.repository.pullRequest"
);
}
async pullRequest(context: T.PrContext): Promise<T.PullRequestFacts> {
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",
]),
await this.graphql(PR_FACTS_QUERY, context),
context
);
}
Expand Down Expand Up @@ -650,11 +669,13 @@
context: T.PrContext,
after: string | null
): Promise<T.RollupPage> {
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)
Expand Down Expand Up @@ -691,13 +712,12 @@
const cursors = new Set<string>();
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")
Expand All @@ -711,18 +731,14 @@
cursors.add(after);
}
} while (after !== null);
return parseReviewThreads({
data: { repository: { pullRequest: { reviewThreads: { nodes } } } },
});
return parseReviewThreads(nodes);
}
async commitRollups(
context: T.PrContext
): Promise<readonly T.CommitRollup[]> {
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) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe("commit identity", () => {
mergeStateStatus: "CLEAN",
reviewDecision: "APPROVED",
headRefOid: "head",
baseRefOid: "base",
baseRef: { target: { oid: "base" } },
headRefName: "feature",
baseRefName: "main",
state: "OPEN",
Expand All @@ -56,7 +56,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);
Expand All @@ -70,7 +73,7 @@ describe("commit identity", () => {
state: "MERGED",
mergedAt: "2026-07-26T00:00:00Z",
headRefOid: null,
baseRefOid: null,
baseRef: null,
},
context
)
Expand Down Expand Up @@ -135,6 +138,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;
Expand Down Expand Up @@ -809,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)
Expand Down Expand Up @@ -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",
Expand Down
15 changes: 10 additions & 5 deletions plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 baseRefTargetOid(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<const V extends readonly string[]>(
value: unknown,
values: V,
Expand All @@ -50,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"),
Expand Down
Loading
Loading