diff --git a/README.md b/README.md index c35075c7..e985abd2 100644 --- a/README.md +++ b/README.md @@ -378,6 +378,7 @@ checks success passed=14 failing=0 pending=0 review stale commented kixelated 2026-10-16T06:14:04Z https://github.com/kixelated/quest/pull/128#pullrequestreview-5543745302 review current commented chatgpt-codex-connector 2026-10-16T17:57:16Z https://github.com/kixelated/quest/pull/128#pullrequestreview-5551703054 thread PRRT_kwDOUhuoys6tUUV2 current chatgpt-codex-connector https://github.com/kixelated/quest/pull/128#discussion_r4297708263 src/cli/gate.ts:115 +hold quest:merge LE_example 2026-10-16T17:57:16.000Z https://api.github.com/repos/kixelated/quest/issues/events/123456 1 hour ago ``` Each line starts with its record type; any free text, a check name or a path, @@ -395,6 +396,8 @@ comes last, and a missing value prints as `-`: | `review` | `current` or `stale` against the head, state, reviewer, time, and URL of each reviewer's latest review. | | `reacted:` | Actor, time, and the PR or review reacted to, as `quest wait` names reactions. | | `thread` | Node ID, `current` or `outdated`, last commenter, latest comment URL, and `path:line` of each unresolved thread. | +| `hold` | Label, acquisition token, acquisition time, verified GitHub event API URL, and age; `hold -` when no label holds it. Unknown acquisition facts print as `-` and `unknown age`. | +| `starter-hold` | A draft its starter owns: last activity time, PR URL, and `last active` age. Last activity is not when ownership began. | The node ID is what GraphQL's `addPullRequestReviewThreadReply` and `resolveReviewThread` take. Neutral and skipped checks pass. A required check @@ -402,7 +405,7 @@ is one branch protection or a ruleset requires for this PR. `--json` prints the same facts as one object with the fields `url`, `state`, `draft`, `head`, `mergeable`, `mergeState`, `reviewDecision`, `autoMerge`, `mergeQueue`, `landingStopped`, `checks` (`state`, `passed`, `failing`, `pending`), `reviews`, `reactions`, and -`threads`. Long lists are paged; reactions are the newest 100 per PR or review, +`threads`, `holds` (`label`, `token`, `takenAt`, `age`, `eventUrl`), and `starterHold` (`lastActive`, `age`, `url`, or null). Ownership uses the state commands' shared reader without changing labels, and event links match the opaque acquisition token against GitHub's event listing. Long lists are paged; reactions are the newest 100 per PR or review, as in `quest wait`. Any PR state exits 0, invalid arguments exit 2, and other failures exit 1. diff --git a/assets/skills/complete.md b/assets/skills/complete.md index 6d37022f..a52a36d5 100644 --- a/assets/skills/complete.md +++ b/assets/skills/complete.md @@ -6,7 +6,7 @@ Before starting, run `quest guide`, `quest skill ready`, `quest skill merge`, an The goal is to evaluate the open PRs in the repository, drafts included, and decide which ones to merge. For a draft, summarize the open decisions its body lists. -Skip each PR another session holds: one with a `quest:*` label, or a draft without a `## Decisions needed` section. +Read each PR with `quest status` and skip one another session holds, using its `hold` and `starter-hold` records instead of a separate label read. A `hold` names the label, acquisition token/time, event URL, and age. A `starter-hold` identifies a draft without a `## Decisions needed` section and reports when it was last active; that is not an acquisition time. List those with how long ago each was last active, and offer to take over only one that looks stale. The skill argument can be used to filter the PRs in scope. Each merge is performed in parallel by a sub-agent, following the guide's Reviews section for the shared design checkpoint across review rounds. diff --git a/quest/m0/helpers/README.md b/quest/m0/helpers/README.md index 7ea5e699..9ae4d51b 100644 --- a/quest/m0/helpers/README.md +++ b/quest/m0/helpers/README.md @@ -68,7 +68,6 @@ commands from Sep-Oct. Session counts in each child come from that window. - [Merge state race](/quest/m0/helpers/merge-state-race.md) - `--land` and merge jobs land a PR whose merge state GitHub is still computing, on the first try - [Wait and status accuracy](/quest/m0/helpers/wait-accuracy.md) - wake on edited bot comments, honest `checks:passed`, review rounds in `quest status` -- [Status holds](/quest/m0/helpers/status-holds.md) - `quest status` shows a PR's `quest:*` hold - [Headless agents](/quest/m0/helpers/headless-agents.md) - `/quest-spawn api` launches, resumes, and stops billed agents through one command auto mode allows - [Sibling PRs through a server-side merge](/quest/m0/helpers/queue-siblings.md) - children of one epic land back to back through a merge queue without conflicting on its link list - [kixelated/quest lands through quest land](/quest/m0/helpers/land-adopt.md) - this repository's `main` changes only through its own gate diff --git a/quest/m0/helpers/status-holds.md b/quest/m0/helpers/status-holds.md deleted file mode 100644 index ab3edb8a..00000000 --- a/quest/m0/helpers/status-holds.md +++ /dev/null @@ -1,18 +0,0 @@ -# [XS] Show PR holds in quest status - -## Goal - -`quest status ` shows whether a `quest:*` hold is on the PR: its label, -how long ago it was taken, and the hold's timeline event, so `/quest-complete` -can skip PRs another session holds without a separate label read. - -## Plan - -Decided 2026-10-22 (user) as a follow-up of -[kixelated/quest#185](https://github.com/kixelated/quest/pull/185), which -added the holds: - -- Read the hold the same way the state commands do, through the shared - GitHub client, and report a draft its starter still holds the same way. -- Update the `complete` skill to read holds from `quest status`, and the - README's status example. diff --git a/quest/m0/helpers/wait-accuracy.md b/quest/m0/helpers/wait-accuracy.md index c8dd5360..7dfdff0e 100644 --- a/quest/m0/helpers/wait-accuracy.md +++ b/quest/m0/helpers/wait-accuracy.md @@ -28,7 +28,3 @@ Decided 2026-10-24 (user) from the merge wave that landed skill's design checkpoint counts today with `gh pr view --json`. - Dropped (user): removing the `quest:merge` label after a PR merges; holds only count on open PRs, so the leftover label is cosmetic. - -## Related - -- [Status holds](/quest/m0/helpers/status-holds.md) - also extends `quest status` diff --git a/src/cli/hold.ts b/src/cli/hold.ts index bfa260e7..166e9c2f 100644 --- a/src/cli/hold.ts +++ b/src/cli/hold.ts @@ -23,10 +23,10 @@ export type Method = "merge" | "squash" | "rebase"; const IMMEDIATE = ["CLEAN", "HAS_HOOKS", "UNSTABLE"]; /** The PR, its `quest:*` labels, and the timeline events that add a label or start a landing. */ -const READ = `query($owner: String!, $repo: String!, $number: Int!) { +const READ = `query HoldOwnership($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { - id url state isDraft body baseRefName headRefOid mergeStateStatus isMergeQueueEnabled + id url state isDraft body updatedAt baseRefName headRefOid mergeStateStatus isMergeQueueEnabled autoMergeRequest { enabledAt } mergeQueueEntry { state } labels(first: 100) { nodes { name } } @@ -55,6 +55,7 @@ interface Snapshot { state: "OPEN" | "CLOSED" | "MERGED"; draft: boolean; body: string; + updatedAt: string; base: string; head: string; mergeState: string; @@ -73,6 +74,63 @@ export interface Hold { token: string; } +export interface HoldFact { + label: string; + /** Null when the acquisition is older than the available timeline. */ + token: string | null; + takenAt: string | null; + age: string | null; + /** GitHub's verified event URL; the opaque GraphQL token is not a browser anchor. */ + eventUrl: string | null; +} +export interface HoldReport { + head: string; + holds: HoldFact[]; + starter: { lastActive: string; age: string; url: string } | null; +} + +/** Read ownership facts through the state commands' reader, without changing labels. */ +export async function readHoldStatus(pr: PullRequest, deps: GitHubDependencies): Promise { + const labels = new Labels(pr, deps); + const snapshot = await labels.read(); + const holds = + snapshot.state !== "OPEN" + ? [] + : snapshot.labels.map((label): HoldFact => { + const event = latestHold(snapshot.events, label); + return { + label, + token: event?.id ?? null, + takenAt: event ? new Date(event.time).toISOString() : null, + age: event ? ago(deps.now(), event.time) : null, + eventUrl: null, + }; + }); + if (holds.some((hold) => hold.token !== null)) { + // REST publishes event URLs and the same node IDs the ownership tokens use. + for (const event of await labels.github.all(`${repositoryPath(pr)}/issues/${pr.number}/events?per_page=100`)) { + const hold = holds.find((hold) => hold.token === event.node_id); + if (hold && event.event === "labeled" && object(event.label).name === hold.label) + hold.eventUrl = string(event.url); + } + } + return { + head: snapshot.head, + holds, + starter: starterHeld(snapshot) + ? { + lastActive: snapshot.updatedAt, + age: ago(deps.now(), Date.parse(snapshot.updatedAt)), + url: snapshot.url, + } + : null, + }; +} + +function starterHeld(snapshot: Snapshot): boolean { + return snapshot.state === "OPEN" && snapshot.draft && !handedBack(snapshot.body); +} + /** Take `pr` for `state`, or swap to it from the label `token` holds. Prints nothing; returns the hold. */ export async function take( pr: PullRequest, @@ -226,7 +284,7 @@ async function acquire( if (other !== undefined) throw new Refusal(holder(snapshot, other, labels.deps.now())); check?.(snapshot); if (mine === label) return { hold: { label, token: token! }, snapshot }; - if (mine === undefined && snapshot.draft && !handedBack(snapshot.body)) { + if (mine === undefined && starterHeld(snapshot)) { throw new Refusal( `${snapshot.url} is a draft its starter still holds: its body has no ## Decisions needed section, so taking it over is the user's choice`, ); @@ -323,6 +381,7 @@ class Labels { state: string(row.state) as Snapshot["state"], draft: row.isDraft === true, body: typeof row.body === "string" ? row.body : "", + updatedAt: string(row.updatedAt), base: string(row.baseRefName), head: string(row.headRefOid), mergeState: string(row.mergeStateStatus), diff --git a/src/cli/status.ts b/src/cli/status.ts index 17008acf..d7185480 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -10,6 +10,7 @@ import { string, } from "./github"; import { EJECTED } from "./land"; +import { readHoldStatus, type HoldFact, type HoldReport } from "./hold"; /** * One query covers the PR; each connection pages by its own cursor, and a @@ -115,9 +116,12 @@ export interface Status { reactions: Reaction[]; /** Unresolved review threads, oldest first. */ threads: Thread[]; + /** Ownership facts read by the same client as the state commands. */ + holds: HoldFact[]; + starterHold: HoldReport["starter"]; } -/** Read the PR's state in one query, plus one per extra page of a long list. */ +/** Read the paged PR state, then the state commands’ ownership snapshot. */ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencies): Promise { const github = new GitHub(deps); const cursors: Record<"checks" | "reviews" | "threads", string | null | undefined> = { @@ -168,7 +172,14 @@ export async function pullRequestStatus(pr: PullRequest, deps: GitHubDependencie if (cursors.reviews !== undefined) page(current.reviews, "reviews", reviewNodes); if (cursors.threads !== undefined) page(current.reviewThreads, "threads", threadNodes); } - return summarize(first!, rollup, contexts, reviewNodes, threadNodes); + const ownership = await readHoldStatus(pr, deps); + if (ownership.head !== first!.headRefOid) + throw new Error("the PR head changed while reading its holds; rerun quest status"); + return { + ...summarize(first!, rollup, contexts, reviewNodes, threadNodes), + holds: ownership.holds, + starterHold: ownership.starter, + }; } function summarize( @@ -273,6 +284,8 @@ function summarize( reviews: [...latest.values()], reactions, threads, + holds: [], + starterHold: null, }; } @@ -344,6 +357,17 @@ export function statusLines(status: Status): string[] { (t) => `thread ${t.id} ${t.outdated ? "outdated" : "current"} ${t.lastAuthor} ${t.url} ${t.path}:${dash(t.line)}`, ), + ...(status.holds.length + ? status.holds.map( + (hold) => + `hold ${hold.label} ${dash(hold.token)} ${dash(hold.takenAt)} ${dash(hold.eventUrl)} ${hold.age ?? "unknown age"}`, + ) + : ["hold -"]), + ...(status.starterHold + ? [ + `starter-hold ${status.starterHold.lastActive} ${status.starterHold.url} last active ${status.starterHold.age}`, + ] + : []), ]; } diff --git a/test/hold.test.ts b/test/hold.test.ts index 5ead9082..016af9a2 100644 --- a/test/hold.test.ts +++ b/test/hold.test.ts @@ -134,6 +134,7 @@ class FakePull { state: this.state, isDraft: this.draft, body: this.body, + updatedAt: new Date(this.clock).toISOString(), baseRefName: "main", headRefOid: this.head, mergeStateStatus: this.mergeState, diff --git a/test/status.test.ts b/test/status.test.ts index f862d4b3..35536942 100644 --- a/test/status.test.ts +++ b/test/status.test.ts @@ -30,7 +30,13 @@ function pull( data: { repository: { pullRequest: { + id: "PR_1", url: URL_, + body: "Summary.", + updatedAt: "2026-10-07T12:00:00Z", + baseRefName: "main", + isMergeQueueEnabled: false, + labels: { nodes: [] }, state: "OPEN", isDraft: false, headRefOid: head, @@ -97,22 +103,59 @@ function thread(id: string, isResolved: boolean, line: number | null = 3, author }; } -function setup(bodies: unknown[]) { +function setup( + bodies: unknown[], + options: { + now?: number; + holdFields?: Node; + eventPages?: { rows: Node[]; next?: string }[]; + } = {}, +) { const queries: Record[] = []; + const holdQueries: Record[] = []; + const eventRequests: string[] = []; + let last: ReturnType | undefined; + const eventPages = [...(options.eventPages ?? [])]; const deps: GitHubDependencies = { token: "test-token", - now: () => 0, + now: () => options.now ?? 0, sleep: async () => {}, warn: () => {}, fetch: async (url, init) => { - expect(new URL(String(url)).pathname).toBe("/graphql"); - queries.push(JSON.parse(String(init?.body)).variables); + const path = new URL(String(url)); + if (path.pathname !== "/graphql") { + expect(path.pathname).toBe("/repos/example/project/issues/1/events"); + eventRequests.push(String(url)); + const page = eventPages.shift(); + if (!page) throw new Error("unexpected event request"); + return Response.json(page.rows, { headers: page.next ? { Link: `<${page.next}>; rel="next"` } : {} }); + } + const request = JSON.parse(String(init?.body)); + if (request.query.includes("query HoldOwnership(")) { + holdQueries.push(request.variables); + if (!last) throw new Error("ownership read before PR state"); + const row = last.data.repository.pullRequest; + const events = (row.timelineItems as { nodes: Node[] }).nodes.filter((event) => + ["LabeledEvent", "AutoMergeEnabledEvent", "AddedToMergeQueueEvent"].includes( + String(event.__typename), + ), + ); + return Response.json({ + data: { + repository: { + pullRequest: { ...row, timelineItems: { nodes: events }, ...options.holdFields }, + }, + }, + }); + } + queries.push(request.variables); const body = bodies.shift(); if (body === undefined) throw new Error("unexpected request"); - return new Response(JSON.stringify(body)); + last = body as ReturnType; + return Response.json(body); }, }; - return { deps, queries }; + return { deps, queries, holdQueries, eventRequests }; } test("reports checks by requiredness, each reviewer's latest review against the head, and open threads", async () => { @@ -148,6 +191,7 @@ test("reports checks by requiredness, each reviewer's latest review against the ]); const status = await pullRequestStatus(pr, fixture.deps); expect(fixture.queries).toHaveLength(1); + expect(fixture.holdQueries).toHaveLength(1); expect(fixture.queries[0]).toMatchObject({ number: 1, moreChecks: true, moreReviews: true, moreThreads: true }); expect(statusLines(status)).toEqual([ `url ${URL_}`, @@ -173,6 +217,7 @@ test("reports checks by requiredness, each reviewer's latest review against the `reacted:thumbs_up codex[bot] 2026-10-07T12:04:00Z ${URL_}#pullrequestreview-3`, `thread T2 current reviewer ${URL_}#discussion_rT2 src/a.ts:3`, `thread T3 outdated author ${URL_}#discussion_rT3 src/a.ts:2`, + "hold -", ]); }); @@ -314,3 +359,86 @@ test("an invalid PR argument is a usage error before authentication", () => { expect(main(["status", "https://evil.example/example/project/pull/1"], output)).toBe(2); expect(errors.join("")).toContain("GitHub pull request URL"); }); + +const holdEvent = (id: string, label: string, at = "2026-10-07T11:00:00Z") => ({ + __typename: "LabeledEvent", + id, + createdAt: at, + label: { name: label }, +}); + +test("reports the latest acquisition, age, and verified paged event URL for each hold label", async () => { + const label = "quest:iterate"; + const body = pull({ + checks: null, + reviews: page([]), + threads: page([]), + labels: { nodes: [{ name: "bug" }, { name: label }] }, + }); + const eventUrl = "https://api.github.com/repos/example/project/issues/events/23"; + const next = "https://api.github.com/repos/example/project/issues/1/events?per_page=100&page=2"; + const fixture = setup([body], { + now: Date.parse("2026-10-07T12:00:00Z"), + holdFields: { timelineItems: { nodes: [holdEvent("old", label), holdEvent("new", label)] } }, + eventPages: [ + { rows: [{ node_id: "old", event: "labeled", label: { name: label }, url: "old-url" }], next }, + { rows: [{ node_id: "new", event: "labeled", label: { name: label }, url: eventUrl }] }, + ], + }); + const status = await pullRequestStatus(pr, fixture.deps); + expect(status.holds).toEqual([ + { label, token: "new", takenAt: "2026-10-07T11:00:00.000Z", age: "1 hour ago", eventUrl }, + ]); + expect(statusLines(status)).toContain(`hold ${label} new 2026-10-07T11:00:00.000Z ${eventUrl} 1 hour ago`); + expect(fixture.eventRequests).toHaveLength(2); + expect(status.starterHold).toBeNull(); +}); + +test("retains a hold fact when its acquisition is outside the available timeline", async () => { + const fixture = setup([ + pull({ checks: null, reviews: page([]), threads: page([]), labels: { nodes: [{ name: "quest:ready" }] } }), + ]); + const status = await pullRequestStatus(pr, fixture.deps); + expect(status.holds).toEqual([{ label: "quest:ready", token: null, takenAt: null, age: null, eventUrl: null }]); + expect(statusLines(status)).toContain("hold quest:ready - - - unknown age"); + expect(fixture.eventRequests).toEqual([]); +}); + +test("never attaches a URL from a different label or event to a hold token", async () => { + const body = pull({ + checks: null, + reviews: page([]), + threads: page([]), + labels: { nodes: [{ name: "quest:merge" }] }, + }); + const fixture = setup([body], { + holdFields: { timelineItems: { nodes: [holdEvent("token", "quest:merge")] } }, + eventPages: [{ rows: [{ node_id: "token", event: "labeled", label: { name: "bug" }, url: "wrong" }] }], + }); + expect((await pullRequestStatus(pr, fixture.deps)).holds[0].eventUrl).toBeNull(); +}); + +test("reports draft starter ownership as last activity, and does not invent its acquisition time", async () => { + const draft = pull({ + checks: null, + reviews: page([]), + threads: page([]), + isDraft: true, + updatedAt: "2026-10-07T11:00:00Z", + }); + const status = await pullRequestStatus(pr, setup([draft], { now: Date.parse("2026-10-07T12:00:00Z") }).deps); + expect(status.starterHold).toEqual({ lastActive: "2026-10-07T11:00:00Z", age: "1 hour ago", url: URL_ }); + expect(statusLines(status)).toContain(`starter-hold 2026-10-07T11:00:00Z ${URL_} last active 1 hour ago`); + for (const change of [{ body: "## Decisions needed\n\nChoose one." }, { state: "CLOSED" }, { state: "MERGED" }]) { + const copy = structuredClone(draft); + Object.assign(copy.data.repository.pullRequest, change); + expect((await pullRequestStatus(pr, setup([copy]).deps)).starterHold).toBeNull(); + } +}); + +test("refuses a head moved between the paged state and ownership read", async () => { + const fixture = setup([pull({ checks: null, reviews: page([]), threads: page([]) })], { + holdFields: { headRefOid: B }, + }); + await expect(pullRequestStatus(pr, fixture.deps)).rejects.toThrow("head changed while reading its holds"); +});