diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index c40f35d..0285bb1 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "name": "t3-pstack", "source": "./plugins/pstack", "description": "pstack for T3 Code: rigorous agent workflows whose panels and fan-outs run across model families through T3. Original pstack by Lauren Tan (poteto); Claude Code port by Michael Denyer.", - "version": "1.0.8" + "version": "1.0.9" } ] } diff --git a/CHANGES.md b/CHANGES.md index add11af..3fe93ec 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,12 @@ This file is the release changelog, with one `## - ` entry per release, newest first. The Cursor-to-Claude rewrite rules live in [`tools/substitutions.json`](tools/substitutions.json), and the [sync boundary](CONTRIBUTING.md#the-sync-boundary) in `CONTRIBUTING.md` defines how a change to upstream's skill content is declared. +## 1.0.9 - merge pstack-claude 0.9.81 + +t3-pstack 1.0.9 merges pstack-claude 0.9.80 and 0.9.81 (upstream commit `24c96b1`). It carries no new change from Lauren Tan's pstack. The upstream entries below describe what came in: `watch-pr` and `ship-pr` compare the base branch's current commit instead of the PR's recorded `baseRefOid`, every GraphQL read in the watcher fails closed on `errors`, and `ship-pr inspect` reads a closed or merged PR whose base branch is gone. The Pi orphan cleanup fix comes in as upstream ships it; t3-pstack does not use Pi. Model defaults are unchanged. + +- The settle window's GraphQL read fails closed on `errors` too. It cannot go through the reader's new `graphql()` method, which returns only the pull request, because it also reads the top-level `viewer`, so `parseReviewActivity` in `settle.ts` checks `errors` itself and throws the same retryable failure. The `github.ts` hook is unchanged. + ## 1.0.8 - merge pstack-claude 0.9.79 t3-pstack 1.0.8 merges pstack-claude 0.9.76 through 0.9.79 (upstream commit `60ae9e2`), which carries Lauren Tan's pstack v0.15.15. The upstream entries below describe what came in. `/poteto-help` offers `/setup-pstack` when the model sheet is missing and the answer depends on it, and the substitution rules match model lists of any length. reflect passes its reviewers the transcript path with nothing beside it, and the fallback digest records events without a verdict. `haiku` joins the default Claude panel in `models.json`. Its `t3` block is unchanged, so the T3 defaults and every T3 sheet are too. Fixes cover the shipping dequeue input id, the worktree-audit and Pi restore guards, `watch-pr` rejecting an `--interval` too long for one timer, the orchestration store lock, the Pi wakeup slot, the sync slug period, and the shipping test fakes, with a `lean/` model of the polling arithmetic. @@ -87,6 +93,17 @@ t3-pstack is pstack-claude 0.9.69 with every subagent, panel, fan-out, schedule, - The checker rejects a malformed or duplicated role line, a list on a single-model role, a `default effort` line, a bad `session hook` value, and a single-family panel, counting `inherit-parent` as the parent's family. The generator runs the same grammar over the `t3` block. - The plugin is `t3-pstack@t3-pstack` (marketplace repo `chhoumann/t3-pstack`), and its model sheet is `t3-pstack-models.md`, so it never collides with pstack-claude's `pstack@pstack-claude` and `pstack-models.md`. Skill text keeps upstream's `pstack:<name>` spelling; `t3-tools.md` reads it as `t3-pstack:<name>`. - The sync denylist's `grok-` token became `(?<!/)grok-`, so a T3 target such as `grok/grok-4.7` passes while a bare Cursor slug still fails. +## 0.9.81 - inspect terminal PRs and recover orphaned Pi agents + +`ship-pr inspect` reads closed and merged PRs after their base branch is deleted, using the stored base commit when the current branch is gone. Open PRs still require the current base target, and cancellation refuses a closed or merged PR before changing anything. + +Pi retries orphan cleanup when process inspection recovers, including when inspection failed at the delayed SIGKILL check. Each signal checks process identity, termination attempts do not overlap, and the agent can resume only after the previous process exits. Both fixes are in [#247](https://github.com/michael-denyer/pstack-claude/pull/247). + +## 0.9.80 - compare the base branch's current commit + +`watch-pr` and `ship-pr` read the base branch's current commit from `baseRef { target { oid } }`. They read the pull request's `baseRefOid` before, a value GitHub records when the PR opens and does not always move when the base branch advances, so a base that moved between two reads could pass the snapshot check and the `cancel-pending` comparison. The watcher takes the head and base facts from one GraphQL query, so both come from one observation. An open PR whose base ref is unavailable is a query failure, not a landing record. @mshk found and fixed this in [#245](https://github.com/michael-denyer/pstack-claude/pull/245), closing [#244](https://github.com/michael-denyer/pstack-claude/issues/244). + +The watcher unwraps every GraphQL response in one place, so a response carrying `errors` fails all five queries the same way. The empty review decision that `gh pr view` reported is gone with the command that reported it: GraphQL returns null, and an empty string is now rejected like any other unknown value. ## 0.9.79 - dequeue a queued PR, and fixes from three models and a code review diff --git a/VERSION b/VERSION index b0f3d96..66c4c22 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.0.8 +1.0.9 diff --git a/package.json b/package.json index 8064a22..dc85375 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "t3-pstack", "private": true, - "version": "1.0.8", + "version": "1.0.9", "description": "If you want to go fast, go deep first. pstack helps you write less, but higher quality code. rigorous agent workflows you can parallelize with confidence. Pi package of the Claude Code plugin; skills are shared, tool names resolve via skills/poteto-mode/references/pi-tools.md. Original pstack by Lauren Tan (poteto).", "author": { "name": "Christian Bager Bach Houmann", diff --git a/plugins/pstack/.claude-plugin/plugin.json b/plugins/pstack/.claude-plugin/plugin.json index 4f757a3..30bda59 100644 --- a/plugins/pstack/.claude-plugin/plugin.json +++ b/plugins/pstack/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "t3-pstack", "displayName": "t3-pstack (pstack for T3 Code)", - "version": "1.0.8", + "version": "1.0.9", "description": "pstack for T3 Code. Rigorous agent workflows whose subagents, panels, schedules, and PR watches dispatch through T3's orchestration, so panels mix Claude, GPT, and Grok. Original pstack by Lauren Tan (poteto); Claude Code port by Michael Denyer.", "author": { "name": "Christian Bager Bach Houmann", diff --git a/plugins/pstack/.codex-plugin/plugin.json b/plugins/pstack/.codex-plugin/plugin.json index d24ab1a..75a8073 100644 --- a/plugins/pstack/.codex-plugin/plugin.json +++ b/plugins/pstack/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "t3-pstack", - "version": "1.0.8", + "version": "1.0.9", "description": "pstack for T3 Code on a Codex host. Subagents, panels, schedules, and PR watches dispatch through T3's orchestration (skills/poteto-mode/references/t3-tools.md); other tool names resolve via codex-tools.md. Original pstack by Lauren Tan (poteto); Claude Code port by Michael Denyer.", "author": { "name": "Christian Bager Bach Houmann", diff --git a/plugins/pstack/.github/plugin/plugin.json b/plugins/pstack/.github/plugin/plugin.json index 4624d6a..d86af34 100644 --- a/plugins/pstack/.github/plugin/plugin.json +++ b/plugins/pstack/.github/plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "t3-pstack", "displayName": "t3-pstack (no Copilot support)", - "version": "1.0.8", + "version": "1.0.9", "description": "t3-pstack runs only inside T3 Code; on GitHub Copilot install pstack@pstack-claude from michael-denyer/pstack-claude.", "author": { "name": "Christian Bager Bach Houmann", diff --git a/plugins/pstack/pi/agents.ts b/plugins/pstack/pi/agents.ts index 8e1b9f7..aaaae5b 100644 --- a/plugins/pstack/pi/agents.ts +++ b/plugins/pstack/pi/agents.ts @@ -78,8 +78,10 @@ interface Run { // A remote agent runs under the live pi process that restore left it to. type LocalAgent = { kind: "local"; record: RunningRecord; run: Run }; +// Unverified restores retry identity checks; interrupted ones await termination. type AgentState = LocalAgent | { kind: "remote"; record: RunningRecord } + | { kind: "unverified"; record: RunningRecord } | { kind: "interrupted"; record: RunningRecord } | { kind: "ended"; record: EndedRecord }; @@ -169,8 +171,8 @@ function fateOf(record: RunningRecord): Fate { // Stops an orphan, by its group so the bash command it has running goes too. // No other fate is signalled: a pid that may have been reused is not ours to hit. -function reapOrphan(record: RunningRecord, killGraceMs: number): void { - if (record.pid !== undefined) terminateGroup(record.pid, () => fateOf(record) === "orphan", killGraceMs); +function reapOrphan(record: RunningRecord, killGraceMs: number): Promise<void> { + return record.pid === undefined ? Promise.resolve() : terminateGroup(record.pid, () => fateOf(record) === "orphan", killGraceMs); } const now = () => new Date().toISOString(); @@ -312,12 +314,12 @@ export class AgentRunner { // An agent this process can act on. One running under another pi process is // not: only that process holds its stdin and can message or stop it. - private owned(to: string): Exclude<AgentState, { kind: "remote" | "interrupted" }> { + private owned(to: string): Exclude<AgentState, { kind: "remote" | "unverified" | "interrupted" }> { const state = this.find(to); if (state.kind === "remote") { throw new Error(`Agent ${state.record.agent.id} is running under another pi process (pid ${state.record.parentPid}); only that process can message or stop it.`); } - if (state.kind === "interrupted") { + if (state.kind === "unverified" || state.kind === "interrupted") { throw new Error(`Agent ${state.record.agent.id}'s previous process (pid ${state.record.pid}) has not exited; it cannot resume or be stopped here until its exit is confirmed.`); } return state; @@ -380,10 +382,19 @@ export class AgentRunner { if (fate === "kept" && state.kind === "remote") return state; if (fate !== "gone") { if (state.kind === "interrupted") return state; - const interrupted: AgentState = { kind: "interrupted", record }; - this.agents.set(record.agent.id, interrupted); - reapOrphan(record, this.settings.killGraceMs); - return interrupted; + const restored: AgentState = { + kind: fate === "orphan" ? "interrupted" : "unverified", + record, + }; + this.agents.set(record.agent.id, restored); + if (restored.kind === "interrupted") { + void reapOrphan(record, this.settings.killGraceMs).then(() => { + if (this.agents.get(record.agent.id) === restored) { + this.agents.set(record.agent.id, { kind: "unverified", record }); + } + }); + } + return restored; } const stopped: EndedRecord = { agent: record.agent, diff --git a/plugins/pstack/pi/child.ts b/plugins/pstack/pi/child.ts index 5874945..0000af9 100644 --- a/plugins/pstack/pi/child.ts +++ b/plugins/pstack/pi/child.ts @@ -247,8 +247,15 @@ export function signalGroup(pid: number, signal: NodeJS.Signals): void { // SIGTERM to the group now, SIGKILL if `ours` still holds after the grace // period. `ours` guards both signals, since a reused pid is not ours to signal. -export function terminateGroup(pid: number, ours: () => boolean, graceMs: number): void { - if (!ours()) return; +// Resolves when the signal attempt ends, not when the process exits. A restored +// orphan can retry if either identity check was unavailable during this attempt. +export function terminateGroup(pid: number, ours: () => boolean, graceMs: number): Promise<void> { + if (!ours()) return Promise.resolve(); signalGroup(pid, "SIGTERM"); - setTimeout(() => ours() && signalGroup(pid, "SIGKILL"), graceMs).unref(); + return new Promise((resolve) => { + setTimeout(() => { + if (ours()) signalGroup(pid, "SIGKILL"); + resolve(); + }, graceMs).unref(); + }); } 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 8eddbdc..bd212fd 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 @@ -100,11 +100,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 c6a06b4..71e0672 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,7 @@ describe("closed enum parsing", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "base", + baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", state: "OPEN", @@ -316,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", () => { @@ -329,6 +329,32 @@ 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( @@ -406,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 d0de2fb..6ba10da 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 { baseRefTargetOid, text } from "./landing.ts"; import { spawn } from "node:child_process"; import { DeadlineExceeded, type WatchDeadline } from "./deadline.ts"; import { readReviewActivity, type ReviewActivity } from "./settle.ts"; @@ -22,6 +22,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"; interface CommandResult { @@ -205,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:")) @@ -422,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; @@ -485,7 +492,7 @@ export function parsePullRequest( ), 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( @@ -497,7 +504,13 @@ export function parsePullRequest( 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( @@ -542,18 +555,24 @@ export class GhGitHubReader implements T.GitHubReader { 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 ); } @@ -650,11 +669,13 @@ export class GhGitHubReader implements T.GitHubReader { 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) @@ -691,13 +712,12 @@ export class GhGitHubReader implements T.GitHubReader { 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") @@ -711,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<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) => { 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 18b6960..3d0eab1 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 @@ -48,7 +48,7 @@ describe("commit identity", () => { mergeStateStatus: "CLEAN", reviewDecision: "APPROVED", headRefOid: "head", - baseRefOid: "base", + baseRef: { target: { oid: "base" } }, headRefName: "feature", baseRefName: "main", state: "OPEN", @@ -57,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); @@ -71,7 +74,7 @@ describe("commit identity", () => { state: "MERGED", mergedAt: "2026-07-26T00:00:00Z", headRefOid: null, - baseRefOid: null, + baseRef: null, }, context ) @@ -136,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; @@ -810,10 +835,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) @@ -843,7 +869,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/landing.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/landing.ts index 3fc7864..2fe6d22 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 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, @@ -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"), diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.test.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.test.ts index 66d636a..8ec637f 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.test.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.test.ts @@ -426,6 +426,24 @@ describe("review activity reader", () => { }); }); + it("fails closed on a response that carries GraphQL errors", () => { + const failure = (() => { + try { + parseReviewActivity({ + ...(graphql({}) as object), + errors: [{ message: "Something went wrong" }], + }); + } catch (error) { + return error; + } + })(); + expect(failure).toBeInstanceOf(WatcherQueryError); + expect((failure as WatcherQueryError).failure).toMatchObject({ + kind: "missing-key", + retryable: true, + }); + }); + it("dates the head only by the branch's newest push", () => { expect( parseHeadPush( diff --git a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.ts index dca5fb0..ec4c7ef 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/settle.ts @@ -132,7 +132,16 @@ export interface ParsedReviewActivity } | null; } export function parseReviewActivity(value: unknown): ParsedReviewActivity { - const data = object(object(value, "response").data, "data"); + // Fails closed on `errors` like GhGitHubReader.graphql, which this query + // cannot go through because it also reads the top-level `viewer`. + const response = object(value, "response"); + if (response.errors !== undefined) + throw new WatcherQueryError({ + kind: "missing-key", + retryable: true, + detail: `invalid GraphQL errors: ${JSON.stringify(response.errors)}`, + }); + const data = object(response.data, "data"); const pr = object( object(data.repository, "repository").pullRequest, "pullRequest" 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 565fe5f..253c7a8 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,8 @@ function fixture( state: "OPEN", headRefOid: "head", baseRefName: "main", - baseRefOid: "base", + baseRefOid: "stored-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); @@ -124,3 +125,35 @@ it("the CLI treats a missing queue field as unavailable", () => expect(result.status).toBe(1); expect(result.output.kind).toBe("unavailable"); })); + +for (const state of ["CLOSED", "MERGED"]) { + it(`the CLI inspects a ${state} PR with a deleted base and refuses cancellation`, () => + fixture((run, file, dir) => { + const inspected = run("inspect", "--repo", "owner/repo", "--pr", "1"); + expect(inspected.status).toBe(0); + const saved = join(dir, "record.json"); + writeFileSync(saved, JSON.stringify(inspected.output)); + const terminal = { + ...JSON.parse(readFileSync(file, "utf8")), + state, + baseRef: null, + autoMergeRequest: null, + mergeQueueEntry: null, + mergeCommit: state === "MERGED" ? { oid: "merged" } : null, + }; + writeFileSync(file, JSON.stringify(terminal)); + const record = { + state, + revision: { baseRefOid: "stored-base" }, + mergeCommitOid: state === "MERGED" ? "merged" : null, + }; + + const observed = run("inspect", "--repo", "owner/repo", "--pr", "1"); + expect(observed.status).toBe(0); + expect(observed.output).toMatchObject({ kind: "inspected", record }); + const cancelled = run("cancel-pending", "--record", saved); + expect(cancelled.status).toBe(1); + expect(cancelled.output).toMatchObject({ kind: "not-open", record }); + expect(JSON.parse(readFileSync(file, "utf8"))).toEqual(terminal); + })); +} 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 cd36f6a..ad5f935 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,7 @@ const raw = { state: "OPEN", headRefOid: "head", baseRefName: "main", - baseRefOid: "base", + baseRef: { target: { oid: "base" } }, autoMergeRequest: null, mergeQueueEntry: null, mergeCommit: null, @@ -190,6 +190,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 +213,7 @@ describe("shipping GitHub boundary", () => { }); for (const field of [ - "baseRefOid", + "baseRef", "autoMergeRequest", "mergeQueueEntry", "mergeCommit", @@ -243,6 +255,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, baseRefOid: "stored-base" }) + ); + expect(await inspectLanding(service, context)).toMatchObject({ + kind: "unavailable", + }); + }); + it("rejects malformed saved context before it can select a PR", () => { expect(() => parseLandingRecord({ @@ -262,8 +335,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/shipping.ts b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts index 2ab2263..7773aca 100644 --- a/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts +++ b/plugins/pstack/skills/poteto-mode/scripts/watch-pr/shipping.ts @@ -1,4 +1,5 @@ import { + baseRefTargetOid, flag, nullableText, object, @@ -118,6 +119,7 @@ const INSPECT_QUERY = `query Landing($owner:String!,$repo:String!,$pr:Int!) { repository(owner:$owner,name:$repo) { pullRequest(number:$pr) { id state headRefOid baseRefName baseRefOid + baseRef { target { oid } } autoMergeRequest { enabledAt } mergeQueueEntry { id } mergeCommit { oid } @@ -158,6 +160,8 @@ export class GhShippingService implements ShippingService { object(response.repository, "repository").pullRequest, "pullRequest" ); + const state = oneOf(fields.state, STATES, "PR state"); + const currentBaseOid = baseRefTargetOid(fields.baseRef); const autoMerge = fields.autoMergeRequest === null ? false @@ -175,9 +179,19 @@ export class GhShippingService implements ShippingService { "queue entry id" ); return { - revision: parseLandingRevision(fields, context), + revision: { + context, + headRefOid: text(fields.headRefOid, "headRefOid"), + baseRefName: text(fields.baseRefName, "baseRefName"), + baseRefOid: text( + state === "OPEN" + ? currentBaseOid + : (currentBaseOid ?? fields.baseRefOid), + "baseRef.target.oid" + ), + }, pullRequestId: text(fields.id, "pull request id"), - state: oneOf(fields.state, STATES, "PR state"), + state, pending: { autoMerge, queueEntryId }, mergeCommitOid: fields.mergeCommit === null 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 094c6cc..d04faf1 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,8 +37,8 @@ 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') { - 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 }; +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] === '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"); diff --git a/tests/pi/agents-session.test.mjs b/tests/pi/agents-session.test.mjs index efcec25..afe401c 100644 --- a/tests/pi/agents-session.test.mjs +++ b/tests/pi/agents-session.test.mjs @@ -1,6 +1,8 @@ // The agent registry across a session's life: restore from persisted entries, // shutdown, an abrupt parent exit, and the settle hold of a non-interactive run. -import { describe, expect, test } from "bun:test"; +import { describe, expect, spyOn, test } from "bun:test"; +import * as childProcess from "node:child_process"; +import * as fs from "node:fs"; import { join } from "node:path"; import { agentEntry, alive, flag, listAgents, recordWith, restore, sleep, useWorld, waitFor } from "./harness.mjs"; @@ -156,6 +158,57 @@ describe("registry", () => { await resumed.emit("session_shutdown", {}, ctx); }); + for (const failure of ["restore", "escalation"]) test(`an orphan recovers from failed inspection at ${failure}, and resumes only after exit`, async () => { + const { w, ctx, host, entries } = await otherPi({ script: { byPrompt: { + x: [{ ignoreSigterm: true }, { spawn: "running" }, { sleep: 30000 }], + again: [{ reply: "resumed" }], + } } }); + const [first] = w.invocations(); + expect(entries[0].data.pidStart).toBeString(); + const id = entries[0].data.agent.id; + const exited = exitOf(host); + host.kill("SIGKILL"); + await exited; + let resumed; + if (failure === "escalation") { + resumed = await restore(w, entries); + await w.until("sigterm-ignored"); + } + + // Fail both process-inspection APIs while leaving the real child running. + const readFile = fs.readFileSync; + const spawn = childProcess.spawnSync; + const proc = spyOn(fs, "readFileSync").mockImplementation((path, ...args) => { + if (path === `/proc/${first.pid}/stat`) throw Object.assign(new Error("inspection unavailable"), { code: "EACCES" }); + return readFile(path, ...args); + }); + const ps = spyOn(childProcess, "spawnSync").mockImplementation((command, args, ...options) => + command === "ps" && args.includes(String(first.pid)) ? { status: 1, stdout: "" } : spawn(command, args, ...options), + ); + try { + if (failure === "restore") resumed = await restore(w, entries); + else await sleep(2 * w.settings.killGraceMs); + expect((await listAgents(resumed, ctx))[0].status).toBe("running"); + await expect(resumed.call("send_message", { to: id, message: "again" }, ctx)).rejects.toThrow(/process.*has not exited/); + expect(alive(first.pid)).toBe(true); + } finally { + proc.mockRestore(); + ps.mockRestore(); + } + + expect((await listAgents(resumed, ctx))[0].status).toBe("running"); + await expect(resumed.call("stop_agent", { id }, ctx)).rejects.toThrow(/process.*has not exited/); + await expect(resumed.call("send_message", { to: id, message: "again" }, ctx)).rejects.toThrow(/process.*has not exited/); + expect(w.invocations()).toHaveLength(1); + await waitFor(() => !alive(first.pid)); + expect(w.logged("sigterm-ignored")).toHaveLength(failure === "restore" ? 1 : 2); + expect((await listAgents(resumed, ctx))[0].status).toBe("stopped"); + + await resumed.call("send_message", { to: id, message: "again" }, ctx); + await waitFor(() => resumed.messages.length === 1); + expect(flag(w.invocations()[1], "--session-id")).toBe(flag(first, "--session-id")); + }); + for (const [what, edit, env, status] of [ ["is left running, untouched", (data) => data, {}, "running"], ["is left running on a record an earlier version wrote, which has no start time", ({ pidStart, ...data }) => data, {}, "running"],