diff --git a/cloudflare/GITHUB.md b/cloudflare/GITHUB.md index efda8d12..5868b700 100644 --- a/cloudflare/GITHUB.md +++ b/cloudflare/GITHUB.md @@ -12,7 +12,7 @@ and installation tokens stay inside the Worker; agents receive neither. | --------------------- | --------------------------------------------------------------------------- | -------------------------------------------------------------- | | Metadata: read | Resolve repository identity and maintainer authority | Every operation | | Contents: write | Read quest files, write job branches on service forks and iterated PR heads | Read for quest reads; write only for that publication or merge | -| Issues: write | Read native issues and publish approved comments | Read or write for that operation | +| Issues: write | Read native issues, publish approved comments, label merge jobs' PRs | Read or write for that operation | | Pull requests: write | Observe, open and update job PRs, explicitly merge an exact head | Read or write for that operation | | Checks: read | Observe checks on a commit | Check reads only | | Commit statuses: read | Observe legacy CI status contexts | Check reads only | @@ -83,9 +83,10 @@ and moves one branch. Before writing anything it checks, in order: absolute path, backslash, or control character. Each workflow publishes only its own changes: `plan` only quest documents under `quest/` (Markdown other than `AGENTS.md` and `CLAUDE.md`), `bootstrap` only `flake.nix` and - `flake.lock`, `start` and `iterate` their task's changes. No workflow changes + `flake.lock`, `start`, `iterate`, and `merge` their task's changes. No workflow changes `.github/workflows/`, because the App does not request the workflows - permission. A refused change pauses the job with a `publication` decision + permission; a merge commit may keep a pull request's own workflow changes, + but not change the workflows of the head it updates. A refused change pauses the job with a `publication` decision listing each path and reason, instead of being dropped; the maintainer's answer resumes the agent's session. - The job's requester still holds maintainer authority, and the repository's @@ -108,7 +109,7 @@ and moves one branch. Before writing anything it checks, in order: `plan`, `start`, and `bootstrap` jobs write their branch on the service fork `GITHUB_FORK_OWNER/`, whose immediate parent must be the upstream's -repository ID. An `iterate` job writes its pull request's actual head: an +repository ID. An `iterate` or `merge` job writes its pull request's actual head: an upstream branch through an upstream-scoped token, or a service fork branch. GitHub gives an installation no write access to a contributor's fork, so that case pauses with a decision reporting the blocked update; kix never opens a @@ -138,7 +139,7 @@ not replaced. Planning pull requests are always drafts, as are those of jobs asked to stop. Other jobs set `draft`, which defaults to `true` while decisions or unfinished work remain; a later delivery with `draft: false` marks the pull request ready. -kix never converts a ready pull request back. Delivery requires an enabled +kix never converts a ready pull request back. Delivery and landing require an enabled project; a paused project's running jobs can still preserve their work on their branches. Pull requests use `maintainer_can_modify: false`, because an upstream-scoped token cannot grant edits on the fork branch. @@ -162,9 +163,41 @@ merges only an exact head a maintainer explicitly chose, with the first method the repository allows: squash, then a merge commit, then rebase. GitHub's branch rules and refusals stand, and an agent's recommendation is never approval. The decision inbox offers it on a pull request item only when -GitHub reports the PR's merge state as `CLEAN` or `HAS_HOOKS` and no job holds -the PR. The form binds the head on screen. A moved head asks for another look, -GitHub's refusal is shown in its own words, and neither is retried. +GitHub reports the PR's merge state as `CLEAN` or `HAS_HOOKS` and no job or +session holds the PR. The form binds the head on screen. A moved head asks for +another look, GitHub's refusal is shown in its own words, and neither is +retried. + +### Merge jobs + +On any other open, non-draft pull request that nothing holds, the inbox's +merge button starts a `merge` job instead. Picking it covers the job's own +fixes for conflicts, failing checks, and review findings; a head moved by +anyone else, a design change, and the merge skill's design checkpoint pause it +on a decision. + +- The job holds the PR with the `quest:merge` label, under the contract + `quest merge --help` states for local sessions: the bot adds it when the job + is created, and refuses the pick while another `quest:*` label holds the PR. + A `quest:merge` whose landing already ended unmerged is spent, and removed + first. The job keeps the timeline event that added the label as its `hold`. +- The job's agent resolves conflicts by merging the base branch with Git. A + publication may name that `merge` commit, which kix accepts only when the + PR's upstream base branch already holds it; kix then publishes a merge + commit whose files are the changes applied to the merged commit's. +- Landing is a publication with `land`: kix verifies the published head and + the job's hold, then enables auto-merge bound to that head, entering the + merge queue when the base has one, or merges it at once when GitHub would + refuse auto-merge because nothing is left to wait for. It uses the method the + job names, else the first the repository allows. A refusal pauses the job on + a decision, in GitHub's words. +- The label stays while the job runs or waits, and while the PR is queued or + auto-merging. When the job ends without a landing in progress, or a + `pull_request` webhook arrives and GitHub shows the landing ended unmerged, + the App removes its own label. The PR is then an idle decision item again. + +Labels need `issues: write` on the upstream; landing needs `contents: write` +and `pull_requests: write` there, as a direct merge does. Fork pull requests do not make privileged CI safe. Workflows that run a pull request's code with secrets or write tokens, such as `pull_request_target` or @@ -178,8 +211,8 @@ Installation grants are broader than any one publication: `contents: write` and `pull_requests: write` cover every installed repository. Each token names one repository ID and only the permissions that step needs. Fork writes get `contents: write` on the fork alone, pull request delivery gets -`pull_requests: write` on the upstream alone, and only an iteration on an -upstream head gets upstream `contents: write`. That token could write any +`pull_requests: write` on the upstream alone, and only an iteration or merge +job on an upstream head gets upstream `contents: write`. That token could write any unprotected upstream branch; the service restricts it to the pull request's head and refuses the default branch, and repositories should still protect their default branch. diff --git a/cloudflare/README.md b/cloudflare/README.md index e7a8ff8c..dad318e5 100644 --- a/cloudflare/README.md +++ b/cloudflare/README.md @@ -75,8 +75,8 @@ connections and keeps no records of its own. ### Jobs -A job is one maintainer-authorized workflow (`plan`, `start`, `iterate`, or -`bootstrap`) on one target, pinned to a revision (`src/jobs/`). Maintainers +A job is one maintainer-authorized workflow (`plan`, `start`, `iterate`, +`merge`, or `bootstrap`) on one target, pinned to a revision (`src/jobs/`). Maintainers create, answer, and cancel jobs at `/projects/github///jobs`, and every request rechecks current GitHub authority. `start` takes only quests that observations show ready and unclaimed, except that retrying a failed job does @@ -90,8 +90,11 @@ target; a retry keeps both. A completed job's result may list suggested idea and takes the follow-ups and summary as context from the job's own result, never from the request, and offers them once. A job on an issue or pull request rereads it from GitHub, refusing one that closed or whose head moved since -observations read it. `iterate` jobs are untrusted and route -only to Linux runners that declare VM builds. Without a `flake.nix`, the route +observations read it. `iterate` and `merge` jobs are untrusted and route +only to Linux runners that declare VM builds. A `merge` job runs the merge +skill's loop on an open, non-draft pull request and lands it; it holds the PR +with the `quest:merge` label that local `quest merge` sessions share +(`src/jobs/landing.ts`), as described in [GITHUB.md](GITHUB.md#merge-jobs). Without a `flake.nix`, the route offers a `bootstrap` job instead. A paused project gets no new jobs and none are claimed; resuming it dispatches the jobs that waited. Disconnecting a project deletes its jobs and their output and tells their runners to stop. Duplicate diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 2b2f30a8..73ddf3e3 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -41,6 +41,11 @@ export type Pull = Issue & { merge?: "CLEAN" | "BLOCKED" | "DIRTY" | "UNSTABLE" | "UNKNOWN"; /** When it was opened, in milliseconds; defaults to the world's fixed update time. */ createdAt?: number; + labels?: string[]; + /** Timeline events that added a label, or with a null label enabled auto-merge, oldest first. */ + timeline?: { id: string; label: string | null }[]; + /** Auto-merge is enabled, so it lands once its requirements pass. */ + landing?: boolean; }; export type Commit = { parents: string[]; tree: string; message: string }; /** A tree written through the API: its base tree and the entries changed on it. */ @@ -59,6 +64,7 @@ export type Repo = { * Upstream branches and their commits. A pull request's branch that is not * listed is at a commit derived from its number; a `quest/*` branch with no * open pull request is a claim, unless it is still at a merged one's head. + * Listing `main` moves the default branch on for comparisons with it. */ branches: Record; /** Branches on the repository's service fork, `/`, and their commits. */ @@ -118,6 +124,8 @@ function entity(repo: Repo, item: Issue) { }; } +const mergeState = (pull: Pull) => pull.merge ?? (pull.draft ? "DRAFT" : "BLOCKED"); + function pullNode(repo: Repo, pull: Pull) { const oid = headOf(repo, pull); const reviews = pull.reviews ?? []; @@ -128,7 +136,8 @@ function pullNode(repo: Repo, pull: Pull) { headRefOid: oid, isDraft: !!pull.draft, reviewDecision: pull.review, - mergeStateStatus: pull.merge ?? (pull.draft ? "DRAFT" : "BLOCKED"), + mergeStateStatus: mergeState(pull), + labels: { nodes: (pull.labels ?? []).map((name) => ({ name })) }, reviews: { totalCount: reviews.length }, latestReviews: { nodes: reviews.map((r) => ({ state: r.state, author: { login: r.author }, commit: { oid } })), @@ -207,6 +216,27 @@ function pullTarget(repo: Repo, pull: Pull, after: unknown) { }; } +/** A pull request's labels, the timeline that added them, and its landing, as a merge hold reads them. */ +function holdsTarget(repo: Repo, pull: Pull) { + return { + id: nodeId(repo, pull), + state: pull.merged ? "MERGED" : pull.closed ? "CLOSED" : "OPEN", + headRefOid: headOf(repo, pull), + mergeStateStatus: mergeState(pull), + isMergeQueueEnabled: false, + autoMergeRequest: pull.landing ? { enabledAt: updatedAt } : null, + mergeQueueEntry: null, + labels: { nodes: (pull.labels ?? []).map((name) => ({ name })) }, + timelineItems: { + nodes: (pull.timeline ?? []).map((event) => + event.label === null + ? { __typename: "AutoMergeEnabledEvent", id: event.id } + : { __typename: "LabeledEvent", id: event.id, label: { name: event.label } }, + ), + }, + }; +} + const page = (nodes: T[]) => ({ nodes, pageInfo: { hasNextPage: false, endCursor: null } }); function graphql(repo: Repo, query: string, variables: Record) { @@ -221,6 +251,10 @@ function graphql(repo: Repo, query: string, variables: Record) ); return { data: { repository: objects } }; } + if (query.includes("timelineItems(")) { + const pull = repo.pulls.find((p) => p.number === variables.number); + return { data: { repository: { pullRequest: pull ? holdsTarget(repo, pull) : null } } }; + } const open = repo.pulls.filter((pull) => !pull.closed); const result: Record = {}; const errors: unknown[] = []; @@ -305,6 +339,21 @@ function connect(repo: Repo) { for (const tip of tips) repo.commits[tip] ??= { parents: [main], tree: treeOf(repo), message: "Work" }; } +/** A tree's `.github/workflows` files and their blob IDs: the default branch's files, then each written tree's changes. */ +function workflowsOf(repo: Repo, sha: string): Record { + const workflow = (path: string) => path.startsWith(".github/workflows/"); + if (sha === treeOf(repo)) + return Object.fromEntries(repo.files.filter((f) => workflow(f.path)).map((f) => [f.path, blob(f.content)])); + const written = repo.trees[sha]; + if (!written) return {}; + const files = workflowsOf(repo, written.base); + for (const entry of written.entries as { path: string; sha?: string | null; content?: string }[]) + if (workflow(entry.path)) + if (entry.sha === null) delete files[entry.path]; + else files[entry.path] = entry.sha ?? blob(entry.content ?? ""); + return files; +} + type Permission = "metadata" | "contents" | "issues" | "pull_requests" | "checks" | "statuses"; /** What an installation token may do: the repositories (`owner/name`) and permissions it was minted for. */ type Grant = { repositories: Set; permissions: Partial> }; @@ -317,6 +366,8 @@ export function fakeGitHub(world: World) { for (const repo of world.repos) connect(repo); const grants = new Map(); let minted = 0; + let events = 0; + const nextEvent = () => ++events; const json = (body: unknown, status = 200) => Response.json(body, { status }); const notFound = () => json({ message: "Not Found" }, 404); const missing = (method: string, url: string) => { @@ -444,6 +495,32 @@ export function fakeGitHub(world: World) { } return json({ errors: [{ type: "NOT_FOUND" }] }); } + const landing = query.match(/\b(enablePullRequestAutoMerge|mergePullRequest)\(/)?.[1]; + if (landing) { + for (const repo of world.repos) + for (const pull of repo.pulls) + if (nodeId(repo, pull) === variables.id) { + if ( + !scope.repositories.has(`${repo.owner}/${repo.name}`) || + !writes("contents") || + !writes("pull_requests") + ) + return json({ errors: [{ type: "FORBIDDEN" }] }); + if (pull.closed) return json({ errors: [{ message: "Pull request is closed" }] }); + if (variables.sha !== headOf(repo, pull)) + return json({ errors: [{ message: "Head sha didn't match expected head" }] }); + if (landing === "mergePullRequest") { + if (!["CLEAN", "HAS_HOOKS", "UNSTABLE"].includes(mergeState(pull))) + return json({ errors: [{ message: "Pull Request is not mergeable" }] }); + pull.closed = pull.merged = true; + } else { + pull.landing = true; + pull.timeline = [...(pull.timeline ?? []), { id: `AME_${nextEvent()}`, label: null }]; + } + return json({ data: { [landing]: { clientMutationId: null } } }); + } + return json({ errors: [{ type: "NOT_FOUND" }] }); + } const repo = world.repos.find((r) => r.owner === variables.owner && r.name === variables.name); if (!repo || !scope.repositories.has(`${repo.owner}/${repo.name}`)) return json({ errors: [{ type: "NOT_FOUND", message: "Not Found" }] }); @@ -552,6 +629,50 @@ export function fakeGitHub(world: World) { pull.merged = true; return json({ sha: commitOf(repo), merged: true, message: "Pull Request successfully merged" }); } + const labels = tail.match(/^\/issues\/(\d+)\/labels(?:\/([^/]+))?$/); + if (labels && (method === "POST" || method === "DELETE")) { + if (!writes("issues")) return forbidden(); + const pull = repo.pulls.find((p) => String(p.number) === labels[1]); + if (!pull) return notFound(); + const current = pull.labels ?? []; + if (method === "POST" && !labels[2]) { + for (const name of body.labels as string[]) { + if (!current.includes(name)) current.push(name); + pull.timeline = [...(pull.timeline ?? []), { id: `LE_${nextEvent()}`, label: name }]; + } + pull.labels = current; + return json(current.map((name) => ({ name }))); + } + const name = labels[2] && decodeURIComponent(labels[2]); + if (method !== "DELETE" || !name) return missing(method, request.url); + if (!current.includes(name)) return json({ message: "Label does not exist" }, 404); + pull.labels = current.filter((label) => label !== name); + return json(pull.labels.map((label) => ({ name: label }))); + } + const compare = tail.match(/^\/compare\/([0-9a-f]{40})\.\.\.main$/); + if (compare && method === "GET") { + const base = repo.branches.main ?? commitOf(repo); + const from = compare[1]!; + if (!repo.commits[from]) return notFound(); + const reaches = (sha: string, target: string): boolean => + sha === target || (repo.commits[sha]?.parents ?? []).some((parent) => reaches(parent, target)); + const status = + from === base + ? "identical" + : reaches(base, from) + ? "ahead" + : reaches(from, base) + ? "behind" + : "diverged"; + return json({ status }); + } + if (tail === "/contents/.github" && method === "GET") { + const tree = repo.commits[url.searchParams.get("ref") ?? ""]?.tree; + const files = Object.entries(tree ? workflowsOf(repo, tree) : {}).sort(); + if (!files.length) return notFound(); + const sha = sha1(`workflows ${JSON.stringify(files)}`); + return json([{ name: "workflows", path: ".github/workflows", type: "dir", sha }]); + } if (tail.startsWith("/contents/")) { const file = repo.files.find((f) => f.path === decodeURIComponent(tail.slice("/contents/".length))); return file ? json({ type: "file", path: file.path }) : notFound(); diff --git a/cloudflare/migrations/0019_merge_jobs.sql b/cloudflare/migrations/0019_merge_jobs.sql new file mode 100644 index 00000000..ed845cb6 --- /dev/null +++ b/cloudflare/migrations/0019_merge_jobs.sql @@ -0,0 +1,62 @@ +-- A merge job lands a pull request the maintainer picked. SQLite cannot alter +-- the workflow CHECK constraint, so the jobs table is rebuilt. Dropping it +-- cascades to its dependents, so their rows are set aside and restored. +PRAGMA defer_foreign_keys = true; +CREATE TABLE jobs_rebuilt ( + id TEXT PRIMARY KEY NOT NULL, + repository INTEGER NOT NULL REFERENCES github_projects(id) ON DELETE CASCADE, + workflow TEXT NOT NULL CHECK(workflow IN ('plan','start','iterate','merge','bootstrap')), + target TEXT NOT NULL, + revision TEXT NOT NULL, + branch TEXT NOT NULL, + actor TEXT NOT NULL REFERENCES user(id) ON DELETE CASCADE, + trusted INTEGER NOT NULL CHECK(trusted IN (0,1)), + pinRunner TEXT, + pinAccount TEXT, + state TEXT NOT NULL CHECK(state IN ('queued','running','waiting','completed','failed','cancelled')), + attempt INTEGER NOT NULL DEFAULT 0, + runner TEXT, + account TEXT, + session TEXT, + lease INTEGER, + delivered INTEGER NOT NULL DEFAULT 0 CHECK(delivered IN (0,1)), + cancel INTEGER NOT NULL DEFAULT 0 CHECK(cancel IN (0,1)), + summary TEXT, + result TEXT, + createdAt INTEGER NOT NULL, + updatedAt INTEGER NOT NULL, + head TEXT, + pending TEXT, + pendingAt INTEGER, + pull INTEGER, + continues TEXT REFERENCES jobs_rebuilt(id) ON DELETE SET NULL, + idea TEXT, + context TEXT, + -- A merge job's hold on its pull request: the ID of the timeline event that + -- added the `quest:merge` label for it, as `quest merge` names holds. Null + -- once the label is released or was never added. + hold TEXT +); +INSERT INTO jobs_rebuilt(id,repository,workflow,target,revision,branch,actor,trusted,pinRunner,pinAccount,state,attempt,runner,account,session,lease,delivered,cancel,summary,result,createdAt,updatedAt,head,pending,pendingAt,pull,continues,idea,context) + SELECT id,repository,workflow,target,revision,branch,actor,trusted,pinRunner,pinAccount,state,attempt,runner,account,session,lease,delivered,cancel,summary,result,createdAt,updatedAt,head,pending,pendingAt,pull,continues,idea,context FROM jobs; +CREATE TABLE kept_job_events AS SELECT * FROM job_events ORDER BY rowid; +CREATE TABLE kept_job_questions AS SELECT * FROM job_questions ORDER BY rowid; +CREATE TABLE kept_job_output_attempts AS SELECT * FROM job_output_attempts; +CREATE TABLE kept_job_output AS SELECT * FROM job_output; +DROP TABLE jobs; +ALTER TABLE jobs_rebuilt RENAME TO jobs; +DELETE FROM job_events; +DELETE FROM job_questions; +DELETE FROM job_output_attempts; +DELETE FROM job_output; +INSERT INTO job_events SELECT * FROM kept_job_events ORDER BY rowid; +INSERT INTO job_questions SELECT * FROM kept_job_questions ORDER BY rowid; +INSERT INTO job_output_attempts SELECT * FROM kept_job_output_attempts; +INSERT INTO job_output SELECT * FROM kept_job_output; +DROP TABLE kept_job_events; +DROP TABLE kept_job_questions; +DROP TABLE kept_job_output_attempts; +DROP TABLE kept_job_output; +CREATE UNIQUE INDEX jobs_active ON jobs(repository,target) WHERE state IN ('queued','running','waiting'); +CREATE INDEX jobs_claimable ON jobs(actor,state,createdAt); +CREATE INDEX jobs_target ON jobs(repository, target, createdAt); diff --git a/cloudflare/src/github/service.ts b/cloudflare/src/github/service.ts index 0e71dd2b..db412c34 100644 --- a/cloudflare/src/github/service.ts +++ b/cloudflare/src/github/service.ts @@ -62,6 +62,7 @@ interface PullHead { body: string | null; created_at: string; head: { ref: string; sha: string; repo: { id: number; name: string; owner: { login: string } } | null }; + base: { ref: string }; } export class GitHubError extends Error { constructor( @@ -92,7 +93,7 @@ export class NotMaintainer extends GitHubError {} export type Change = { path: string; content: Uint8Array | null; mode: "100644" | "100755" | "120000" }; /** What a job may publish: its workflow, its target, and its one branch. */ export type Contribution = { - workflow: "plan" | "start" | "iterate" | "bootstrap"; + workflow: "plan" | "start" | "iterate" | "merge" | "bootstrap"; target: string; branch: string; pull: number | null; @@ -104,8 +105,16 @@ export interface Branch { repository: GitHubRepository; /** The branch's commit, or null when it does not exist yet. */ head(): Promise; - /** Create a commit of `changes` on `parent` without moving the branch. */ - commit(parent: string, changes: Change[], message: string): Promise; + /** + * Create a commit of `changes` on `parent` without moving the branch. With + * `merge`, it is a merge commit of `parent` and `merge`, and `changes` are + * relative to `merge`'s files. + */ + commit(parent: string, changes: Change[], message: string, merge?: string): Promise; + /** Whether the branch this one merges into, upstream, already holds `commit`. */ + based(commit: string): Promise; + /** The tree ID of `commit`'s `.github/workflows`, or null when it has none. */ + workflows(commit: string): Promise; /** Move the branch from `from` (null creates it) to `to`; false when it moved first. */ move(from: string | null, to: string): Promise; /** Point the branch at `to` wherever it is, creating it if it is gone, discarding its commits. */ @@ -156,13 +165,7 @@ export function refName(name: string) { async function refusal(response: Response) { try { const { message } = (await response.json()) as { message?: unknown }; - if (typeof message === "string") - return ( - message - .replace(/[\x00-\x1f\x7f]+/g, " ") - .trim() - .slice(0, 300) || null - ); + if (typeof message === "string") return line(message) || null; } catch {} return null; } @@ -1021,18 +1024,33 @@ export class GitHubService { job: Contribution, action: (branch: Branch) => Promise, ) { - const iterate = job.workflow === "iterate"; + const pulled = job.workflow === "iterate" || job.workflow === "merge"; return this.#authorized( upstream, userId, - iterate ? { pull_requests: "read" } : {}, + pulled ? { contents: "read", pull_requests: "read" } : {}, async (token, current, installation) => { this.#public(current); if ((await this.#state(upstream, installation)) === null) throw new GitHubError("Project is not enrolled for this installation"); - if (!iterate) + // A merge commit may only bring in what the pull request's upstream base already holds. + const based = (base: string) => async (commit: string) => { + try { + const ref = base.split("/").map(encodeURIComponent).join("/"); + const { status } = await this.#api<{ status: string }>( + token, + `${repositoryPath(upstream)}/compare/${sha(commit)}...${ref}`, + ); + return status === "identical" || status === "ahead"; + } catch (error) { + if (error instanceof GitHubError && error.status === 404) return false; + throw error; + } + }; + // A job's own branch starts from the default branch's current commit, so it merges nothing in. + if (!pulled) return this.#fork(upstream, upstream.name, { contents: "write" }, (forkToken, fork) => - action(this.#branch(forkToken, fork, job.branch)), + action(this.#branch(forkToken, fork, job.branch, async () => false)), ); const pull = await this.#api( token, @@ -1041,14 +1059,15 @@ export class GitHubService { if (pull.state !== "open" || pull.head.ref !== job.branch) throw new GitHubError("The pull request was closed or its head branch changed"); const head = pull.head.repo; + const base = based(pull.base.ref); if (head?.id === upstream.id) return this.#withRepository(upstream, { contents: "write" }, async (writeToken, latest) => - action(this.#branch(writeToken, latest, job.branch)), + action(this.#branch(writeToken, latest, job.branch, base)), ); if (head && this.#forkOwner(head.owner.login)) return this.#fork(upstream, head.name, { contents: "write" }, (forkToken, fork) => { if (fork.id !== head.id) throw new GitHubError("Repository identity or availability changed"); - return action(this.#branch(forkToken, fork, job.branch)); + return action(this.#branch(forkToken, fork, job.branch, base)); }); throw new PublicationBlocked( "The pull request's head is in a contributor's fork, which the bot cannot update. Ask its author to apply the changes, or close it and start new work.", @@ -1092,7 +1111,7 @@ export class GitHubService { } } - #branch(token: string, repo: Repository, branch: string): Branch { + #branch(token: string, repo: Repository, branch: string, based: Branch["based"]): Branch { if (!refName(branch) || branch === repo.default_branch) throw new PublicationBlocked("Jobs never write a repository's default branch"); const path = `/repos/${repo.owner.login}/${repo.name}`; @@ -1113,8 +1132,25 @@ export class GitHubService { throw error; } }, - commit: async (parent, changes, message) => { - const base = await this.#api<{ tree: { sha: string } }>(token, `${path}/git/commits/${sha(parent)}`); + based, + workflows: async (commit) => { + try { + const entries = await this.#api(token, `${path}/contents/.github?ref=${sha(commit)}`); + if (!Array.isArray(entries)) return null; + const found = (entries as { name?: unknown; type?: unknown; sha?: unknown }[]).find( + (entry) => entry.name === "workflows" && entry.type === "dir", + ); + return typeof found?.sha === "string" ? sha(found.sha) : null; + } catch (error) { + if (error instanceof GitHubError && error.status === 404) return null; + throw error; + } + }, + commit: async (parent, changes, message, merge) => { + const base = await this.#api<{ tree: { sha: string } }>( + token, + `${path}/git/commits/${sha(merge ?? parent)}`, + ); const tree = []; for (const change of changes) { const mode = change.mode; @@ -1145,7 +1181,7 @@ export class GitHubService { const commit = await this.#api<{ sha: string }>(token, `${path}/git/commits`, "POST", { message, tree: sha(created.sha), - parents: [parent], + parents: merge ? [parent, sha(merge)] : [parent], }); return sha(commit.sha); }, @@ -1190,8 +1226,8 @@ export class GitHubService { job: Contribution, input: { title: string; body: string; draft: boolean }, ) { - if (job.workflow === "iterate") - throw new GitHubError("An iteration updates its pull request's head and opens no pull request"); + if (job.workflow === "iterate" || job.workflow === "merge") + throw new GitHubError("Work on a pull request updates its head and opens no pull request"); return this.#authorized(upstream, userId, { pull_requests: "write" }, async (token, current, installation) => { this.#public(current); if ((await this.#state(upstream, installation)) !== "enabled") @@ -1266,14 +1302,7 @@ export class GitHubService { */ mergePullRequest(repo: GitHubRepository, userId: string, pull: number, expectedHead: string) { return this.#authorized(repo, userId, { contents: "write", pull_requests: "write" }, async (token, current) => { - const method = current.allow_squash_merge - ? "squash" - : current.allow_merge_commit - ? "merge" - : current.allow_rebase_merge - ? "rebase" - : null; - if (!method) throw new GitHubError("The repository allows no merge method"); + const method = mergeMethod(current); let merged: { sha: string }; try { merged = await this.#api<{ sha: string }>( @@ -1291,13 +1320,232 @@ export class GitHubService { throw new GitHubError(`GitHub refused the merge: ${error.reason}`, error.status); throw error; } - // As a webhook would, mark observations stale so the inbox stops offering the merged pull request. - await this.#env.DB.prepare( - "UPDATE github_project_state SET invalidated=1,revision=revision+1 WHERE repository=?", - ) - .bind(repo.id) - .run(); + await this.#invalidate(repo); return { sha: merged.sha }; }); } + + /** As a webhook would, mark observations stale so the inbox stops offering a pull request that merged or is landing. */ + async #invalidate(repo: GitHubRepository) { + await this.#env.DB.prepare( + "UPDATE github_project_state SET invalidated=1,revision=revision+1 WHERE repository=?", + ) + .bind(repo.id) + .run(); + } + + /** A pull request's hold labels and the timeline events that add a label or start a landing, as `quest merge` reads them. */ + async #holds(token: string, repo: GitHubRepository, pull: number): Promise { + const result = await this.#api<{ + data?: { repository: { pullRequest: HoldRow | null } | null }; + errors?: unknown[]; + }>(token, "/graphql", "POST", { + query: HOLDS, + variables: { owner: repo.owner, name: repo.name, number: number(pull) }, + }); + const row = result.data?.repository?.pullRequest; + if (result.errors?.length || !row) throw new GitHubError("This pull request is unavailable"); + return { + id: row.id, + state: row.state, + head: row.headRefOid, + mergeState: row.mergeStateStatus, + queue: row.isMergeQueueEnabled === true, + landing: row.autoMergeRequest !== null || row.mergeQueueEntry !== null, + labels: row.labels.nodes.map((label) => label.name).filter((name) => name.startsWith(HOLD_PREFIX)), + events: row.timelineItems.nodes.map((event) => ({ + id: event.id, + label: event.__typename === "LabeledEvent" ? (event.label?.name ?? "") : null, + })), + }; + } + + /** Remove `quest:merge` from a pull request; one already gone is fine. */ + async #unhold(token: string, repo: GitHubRepository, pull: number) { + try { + await this.#api( + token, + `${repositoryPath(repo)}/issues/${number(pull)}/labels/${encodeURIComponent(MERGE_LABEL)}`, + "DELETE", + ); + } catch (error) { + if (!(error instanceof GitHubError) || error.status !== 404) throw error; + } + } + + /** + * Hold an open pull request for a merge job with the `quest:merge` label, + * the contract `quest merge --help` states for local sessions too. Refused + * while another session holds a `quest:*` label; a `quest:merge` whose + * landing ended unmerged is spent and removed first. When two sessions + * label it at once, the first label added keeps it. Returns the hold's + * token: the timeline event that added the label. + */ + holdPullRequest(repo: GitHubRepository, userId: string, pull: number) { + return this.#authorized(repo, userId, { issues: "write", pull_requests: "read" }, async (token) => { + let holds = await this.#holds(token, repo, pull); + if (holds.state !== "OPEN") throw new GitHubError("The pull request is no longer open"); + if (spent(holds)) { + await this.#unhold(token, repo, pull); + holds = { ...holds, labels: holds.labels.filter((name) => name !== MERGE_LABEL) }; + } + const other = holds.labels[0]; + if (other) throw new GitHubError(heldBy(other)); + await this.#api(token, `${repositoryPath(repo)}/issues/${number(pull)}/labels`, "POST", { + labels: [MERGE_LABEL], + }); + holds = await this.#holds(token, repo, pull); + const ours = added(holds, MERGE_LABEL); + if (!ours) throw new GitHubError("GitHub does not show the quest:merge label kix added"); + const rival = holds.labels.find((name) => { + if (name === MERGE_LABEL) return false; + const event = added(holds, name); + return !event || holds.events.indexOf(event) < holds.events.indexOf(ours); + }); + if (rival) { + await this.#unhold(token, repo, pull); + throw new GitHubError(heldBy(rival)); + } + return ours.id; + }); + } + + /** + * Release a merge job's `quest:merge` hold unless its pull request is + * queued or auto-merging. The App acts on its own label here, with no + * maintainer request behind it. Returns whether the hold is over: released + * now, already removed or replaced, or its pull request no longer open. + */ + releasePullRequest(repo: GitHubRepository, pull: number, hold: string) { + return this.#withRepository(repo, { issues: "write", pull_requests: "read" }, async (token) => { + const holds = await this.#holds(token, repo, pull); + if (holds.state !== "OPEN" || !holds.labels.includes(MERGE_LABEL) || added(holds, MERGE_LABEL)?.id !== hold) + return true; + if (holds.landing) return false; + await this.#unhold(token, repo, pull); + return true; + }); + } + + /** + * Land a merge job's pull request at exactly `head`, while the job still + * holds it: enable auto-merge bound to that head, which enters the merge + * queue when the base has one, or merge at once when GitHub would refuse + * auto-merge because nothing is left to wait for. `method` is the one the + * repository's instructions name, or else the first the repository allows. + */ + landPullRequest( + repo: GitHubRepository, + userId: string, + pull: number, + input: { head: string; hold: string | null; method: MergeMethod | null }, + ) { + return this.#authorized( + repo, + userId, + { contents: "write", pull_requests: "write" }, + async (token, current, installation) => { + // Landing is a contribution like opening a pull request, so a paused project lands nothing. + if ((await this.#state(repo, installation)) !== "enabled") + throw new GitHubError("Contributions are paused"); + const holds = await this.#holds(token, repo, pull); + if (holds.state !== "OPEN") throw new PublicationBlocked("The pull request is no longer open."); + if (!input.hold || !holds.labels.includes(MERGE_LABEL) || added(holds, MERGE_LABEL)?.id !== input.hold) + throw new PublicationBlocked( + "This job's quest:merge label was removed or replaced, so it no longer holds the pull request.", + ); + if (holds.head !== sha(input.head)) + throw new PublicationBlocked("The pull request's head moved outside this job."); + const mutation = + !holds.queue && IMMEDIATE.includes(holds.mergeState) + ? "mergePullRequest" + : "enablePullRequestAutoMerge"; + const result = await this.#api<{ errors?: { message?: unknown }[] }>(token, "/graphql", "POST", { + query: `mutation($id: ID!, $sha: GitObjectID!, $method: PullRequestMergeMethod!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha, mergeMethod: $method }) { clientMutationId } }`, + variables: { + id: holds.id, + sha: input.head, + method: (input.method ?? mergeMethod(current)).toUpperCase(), + }, + }); + if (result.errors?.length) { + const reason = result.errors[0]?.message; + throw new PublicationBlocked( + `GitHub refused to land the pull request${typeof reason === "string" && line(reason) ? `: ${line(reason)}` : "."}`, + ); + } + await this.#invalidate(repo); + return { merged: mutation === "mergePullRequest" }; + }, + ); + } +} + +export type MergeMethod = "merge" | "squash" | "rebase"; + +/** The first merge method the repository allows: squash, then a merge commit, then rebase. */ +function mergeMethod(current: Repository): MergeMethod { + if (current.allow_squash_merge) return "squash"; + if (current.allow_merge_commit) return "merge"; + if (current.allow_rebase_merge) return "rebase"; + throw new GitHubError("The repository allows no merge method"); +} + +/** The label prefix and merge label `quest iterate`, `ready`, and `merge` hold pull requests with. */ +const HOLD_PREFIX = "quest:"; +export const MERGE_LABEL = "quest:merge"; +/** Merge states GitHub merges at once, where it refuses to enable auto-merge. */ +const IMMEDIATE = ["CLEAN", "HAS_HOOKS", "UNSTABLE"]; +const HOLDS = `query($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { + id state headRefOid mergeStateStatus isMergeQueueEnabled autoMergeRequest { enabledAt } mergeQueueEntry { state } + labels(first: 100) { nodes { name } } + timelineItems(last: 100, itemTypes: [LABELED_EVENT, AUTO_MERGE_ENABLED_EVENT, ADDED_TO_MERGE_QUEUE_EVENT]) { nodes { __typename + ... on LabeledEvent { id label { name } } ... on AutoMergeEnabledEvent { id } ... on AddedToMergeQueueEvent { id } } } } } }`; +type HoldRow = { + id: string; + state: string; + headRefOid: string; + mergeStateStatus: string; + isMergeQueueEnabled: boolean | null; + autoMergeRequest: unknown; + mergeQueueEntry: unknown; + labels: { nodes: { name: string }[] }; + timelineItems: { nodes: { __typename: string; id: string; label?: { name: string } | null }[] }; +}; +type Holds = { + id: string; + state: string; + head: string; + mergeState: string; + queue: boolean; + /** Auto-merge is enabled or the pull request is in the merge queue. */ + landing: boolean; + /** Its `quest:*` labels. */ + labels: string[]; + /** Oldest first; `label` is null for a landing that started. */ + events: { id: string; label: string | null }[]; +}; + +/** The latest event that added `label`. */ +function added(holds: Holds, label: string) { + return [...holds.events].reverse().find((event) => event.label === label); +} + +/** A `quest:merge` label is spent once a landing started after it was added and ended unmerged. */ +function spent(holds: Holds) { + const event = added(holds, MERGE_LABEL); + if (!event || holds.landing || !holds.labels.includes(MERGE_LABEL)) return false; + return holds.events.slice(holds.events.indexOf(event) + 1).some((later) => later.label === null); +} + +function heldBy(label: string) { + return `Another session holds this pull request with ${label}; pick it again once it is released`; +} + +/** Untrusted GitHub text as one bounded line. */ +function line(text: string) { + return text + .replace(/[\x00-\x1f\x7f]+/g, " ") + .trim() + .slice(0, 300); } diff --git a/cloudflare/src/github/state.ts b/cloudflare/src/github/state.ts index 0d3cc040..d993f633 100644 --- a/cloudflare/src/github/state.ts +++ b/cloudflare/src/github/state.ts @@ -31,6 +31,8 @@ export type Pull = Entity & { reviewDecision: string | null; /** GitHub's merge readiness for the App, such as CLEAN, BLOCKED, or DIRTY; absent in older observations. */ mergeStateStatus?: string; + /** The `quest:*` label a session holds it with, such as a queued merge's; absent in older observations. */ + hold?: string | null; reviews: { totalCount: number }; latestReviews: { nodes: { state: string; author: { login: string } | null; commit: { oid: string } | null }[]; @@ -87,7 +89,7 @@ export const RETENTION = 30 * 24 * 60 * 60 * 1000; const entityFields = "databaseId:fullDatabaseId number title state updatedAt closedAt author { login } comments { totalCount }"; -const pullFields = `${entityFields} body headRefName headRefOid isDraft reviewDecision mergeStateStatus reviews { totalCount } latestReviews(first:20) { nodes { state author {login} commit {oid} } pageInfo {hasNextPage} } commits(last:1) { nodes { commit { oid statusCheckRollup { state } } } }`; +const pullFields = `${entityFields} body headRefName headRefOid isDraft reviewDecision mergeStateStatus labels(first:20) { nodes { name } } reviews { totalCount } latestReviews(first:20) { nodes { state author {login} commit {oid} } pageInfo {hasNextPage} } commits(last:1) { nodes { commit { oid statusCheckRollup { state } } } }`; // Cursor variables are declared only for unfinished connections (GraphQL rejects // unused variables). Fixed page sizes bound subrequests, independent of tree size. export function activityQuery({ history, occupancy }: Scan, stale: Stale[] = []) { @@ -164,11 +166,16 @@ function validate(kind: "issue" | "pull", item: Entity) { } } -/** What is kept of an observed entity: a PR's body only as whether it hands its draft back. */ +/** + * What is kept of an observed entity: a PR's body only as whether it hands + * its draft back, and its labels only as the `quest:*` one a session holds it with. + */ function observed(kind: "issue" | "pull", item: Entity): Entity { if (kind === "issue") return item; - const { body, ...pull } = item as Pull & { body?: unknown }; - return { ...pull, handedBack: typeof body === "string" && handedBack(body) } as Pull; + const { body, labels, ...pull } = item as Pull & { body?: unknown; labels?: { nodes: { name: unknown }[] } }; + const names = (labels?.nodes ?? []).map((label) => label.name); + const hold = names.find((name): name is string => typeof name === "string" && name.startsWith("quest:")) ?? null; + return { ...pull, handedBack: typeof body === "string" && handedBack(body), hold } as Pull; } /** diff --git a/cloudflare/src/github/webhook.ts b/cloudflare/src/github/webhook.ts index 5b0c2faa..9413acef 100644 --- a/cloudflare/src/github/webhook.ts +++ b/cloudflare/src/github/webhook.ts @@ -1,5 +1,6 @@ import { Hono } from "hono"; import { bodyLimit } from "hono/body-limit"; +import * as landing from "../jobs/landing"; import { arrived } from "../push/notify"; import { GitHubService } from "./service"; @@ -219,5 +220,8 @@ githubWebhooks.post("/webhooks/github", async (c) => { } })(), ); + // A landing that ended unmerged, such as a merge queue removal, gives a merge job's pull request back. + const landed = event === "pull_request" && positiveId(payload.pull_request?.number); + if (landed) c.executionCtx.waitUntil(landing.release(c.env, repository, `pulls/${payload.pull_request.number}`)); return c.json({ status: "accepted" }, 202); }); diff --git a/cloudflare/src/inbox/model.ts b/cloudflare/src/inbox/model.ts index bbef5de4..a0b5f7a3 100644 --- a/cloudflare/src/inbox/model.ts +++ b/cloudflare/src/inbox/model.ts @@ -141,9 +141,14 @@ export function entities(project: Enrollment, state: ProjectState | null) { return found; } -/** A PR whose observed draft its starter still holds, so nothing waits on the maintainer. */ +/** + * A PR whose observed draft its starter still holds, or that a session holds + * with a `quest:*` label, such as one whose landing is queued, so nothing + * waits on the maintainer. + */ function held(entity: Pick) { - return entity.kind === "pull" && !!entity.activity?.data && owned(entity.activity.data as Pull); + const pull = entity.kind === "pull" ? (entity.activity?.data as Pull | null | undefined) : null; + return !!pull && (owned(pull) || !!pull.hold); } /** What an item is called and links to when nothing observed names its target. */ diff --git a/cloudflare/src/inbox/page.tsx b/cloudflare/src/inbox/page.tsx index bfc455c4..5555d638 100644 --- a/cloudflare/src/inbox/page.tsx +++ b/cloudflare/src/inbox/page.tsx @@ -343,6 +343,14 @@ function blocked(item: Item, view: ProjectView | undefined) { /** The workflow a new job runs for the item, bound to the revision on screen. */ function work(item: Item, view: ProjectView) { const commit = view.state!.project.commit; + // Retrying a failed merge job picks merge again. + if (item.kind === "pull" && item.need === "retry" && item.job?.workflow === "merge") + return { + workflow: "merge", + target: item.target, + revision: (item.activity!.data as Pull).headRefOid, + label: "Merge this PR", + }; if (item.kind === "pull") return { workflow: "iterate", @@ -365,13 +373,15 @@ function work(item: Item, view: ProjectView) { } /** - * The observed pull request when GitHub reports it can merge as is: no - * conflicts, and its checks and branch rules satisfied. Anything else is - * reviewed on GitHub or merged by a later merge job. + * How picking merge lands the observed open pull request: at once, bound to + * its head, when GitHub reports it can merge as is (no conflicts, and its + * checks and branch rules satisfied), or else through a merge job that first + * fixes its conflicts, checks, and review findings. A draft is not offered. */ -function clean(item: Item) { +function landing(item: Item) { const pull = item.kind === "pull" ? (item.activity?.data as Pull | null | undefined) : null; - return pull?.state === "OPEN" && ["CLEAN", "HAS_HOOKS"].includes(pull.mergeStateStatus ?? "") ? pull : null; + if (pull?.state !== "OPEN" || pull.isDraft) return null; + return { pull, clean: ["CLEAN", "HAS_HOOKS"].includes(pull.mergeStateStatus ?? "") }; } function Refresh(props: { project: Item["project"] }) { @@ -391,17 +401,25 @@ function Actions(props: { item: Item; view: ProjectView | undefined }) { // Only an unblocked item has the observations its work is bound to. const job = starts && !reason ? work(item, view!) : null; // Only an idle item offers merge: no queued, running, or waiting job can still push to its head. - const merge = starts && !reason ? clean(item) : null; + const merge = starts && !reason ? landing(item) : null; const active = item.job && ["queued", "running", "waiting"].includes(item.job.state) && !item.job.cancel; return (
- {merge && ( -
- + {merge?.clean && ( + +
)} + {merge && !merge.clean && job?.workflow !== "merge" && ( +
+ + + + +
+ )} {reason ? ( <>

{reason}

@@ -413,7 +431,7 @@ function Actions(props: { item: Item; view: ProjectView | undefined }) { {job.target && } - @@ -638,7 +656,13 @@ function Project(props: { view: ProjectView; runners: Runners }) { ); } -const workflows = { plan: "Planning", start: "Implementing", iterate: "Iterating", bootstrap: "Adding a devShell" }; +const workflows = { + plan: "Planning", + start: "Implementing", + iterate: "Iterating", + merge: "Merging", + bootstrap: "Adding a devShell", +}; /** * One agent job: its target, state, runner and account, and the agent's latest diff --git a/cloudflare/src/jobs/hub.ts b/cloudflare/src/jobs/hub.ts index 89ac72e9..d30e9dc1 100644 --- a/cloudflare/src/jobs/hub.ts +++ b/cloudflare/src/jobs/hub.ts @@ -1,5 +1,6 @@ import { DurableObject } from "cloudflare:workers"; import * as push from "../push/notify"; +import * as landing from "./landing"; import * as output from "./output"; import { MessageError, type RunnerMessage, parse } from "./protocol"; import * as store from "./store"; @@ -120,13 +121,15 @@ export class RunnerHub extends DurableObject { const reported = (ws.deserializeAttachment() as Attachment).slots; if (reported !== null) slots.set(runner, reported); } - const { assign, cancel, wake } = await store.dispatch(this.env.DB, owner, slots, Date.now()); + const { assign, cancel, ended, wake } = await store.dispatch(this.env.DB, owner, slots, Date.now()); for (const { runner, job } of assign) { const message = { type: "assign", job, instructions: instructions(job.workflow) }; connections.get(runner)?.send(JSON.stringify(message)); } for (const { runner, job, attempt } of cancel) connections.get(runner)?.send(JSON.stringify({ type: "cancel", job, attempt })); + // An abandoned attempt that was asked to cancel reports no result, so its hold is released here. + for (const job of ended) this.ctx.waitUntil(landing.ended(this.env, job)); if (wake === null) await this.ctx.storage.deleteAlarm(); else await this.ctx.storage.setAlarm(wake); } @@ -137,7 +140,8 @@ export class RunnerHub extends DurableObject { const now = Date.now(); switch (message.type) { case "status": { - const attempts = await store.report(db, runner, message.accounts, message.attempts, now); + const { attempts, ended } = await store.report(db, runner, message.accounts, message.attempts, now); + for (const job of ended) this.ctx.waitUntil(landing.ended(this.env, job)); attachment.slots = message.slots; ws.serializeAttachment(attachment); return { attempts }; @@ -172,6 +176,8 @@ export class RunnerHub extends DurableObject { // A limit waits on an account choice; a failure waits on a retry. if (state === "waiting" || state === "failed") this.ctx.waitUntil(push.job(this.env, message.job, state === "waiting" ? "account" : "failed")); + // A merge job that ended without a landing in progress gives its pull request back. + if (state && state !== "waiting") this.ctx.waitUntil(landing.ended(this.env, message.job)); return {}; } } diff --git a/cloudflare/src/jobs/landing.ts b/cloudflare/src/jobs/landing.ts new file mode 100644 index 00000000..582b873a --- /dev/null +++ b/cloudflare/src/jobs/landing.ts @@ -0,0 +1,44 @@ +// A merge job holds its pull request with the `quest:merge` label, as local +// `quest merge` sessions do: from when the maintainer picks merge until the +// job ends without landing, or until a landing it started ends unmerged. Then +// the bot removes the label, and the pull request is an idle decision again. +import { GitHubService } from "../github/service"; +import { pullOf } from "./publish"; +import * as store from "./store"; + +/** + * Release the holds of the ended merge jobs on a pull request that is not + * queued or auto-merging. GitHub is read rather than trusted from whatever + * event prompted this, so calling it again is harmless. + */ +export async function release(env: Env, repository: number, target: string) { + const holds = await store.holding(env.DB, repository, target); + if (!holds.length) return; + const project = await env.DB.prepare("SELECT owner,name FROM github_projects WHERE id=?") + .bind(repository) + .first<{ owner: string; name: string }>(); + if (!project) return; + const github = new GitHubService(env); + for (const { id, hold } of holds) { + try { + if (await github.releasePullRequest({ ...project, id: repository }, pullOf(target), hold)) + await store.released(env.DB, id, hold); + } catch (error) { + // The next event on the pull request tries again. + console.error({ + event: "merge_hold_release_failed", + name: error instanceof Error ? error.name : "unknown", + }); + } + } +} + +/** Release a job's hold once it ended, if it is a merge job that holds one. */ +export async function ended(env: Env, job: string) { + const row = await env.DB.prepare( + "SELECT repository,target FROM jobs WHERE id=? AND workflow='merge' AND hold IS NOT NULL", + ) + .bind(job) + .first<{ repository: number; target: string }>(); + if (row) await release(env, row.repository, row.target); +} diff --git a/cloudflare/src/jobs/publish.ts b/cloudflare/src/jobs/publish.ts index 11836488..d97ede45 100644 --- a/cloudflare/src/jobs/publish.ts +++ b/cloudflare/src/jobs/publish.ts @@ -10,10 +10,11 @@ import { GitHubService, PublicationBlocked, type GitHubRepository, + type MergeMethod, } from "../github/service"; import { id, text } from "./protocol"; import * as store from "./store"; -import { JobError, type Runner, type Workflow } from "./store"; +import { JobError, onPull, type Runner, type Workflow } from "./store"; import { withDecisions } from "./trail"; /** Bounds one request's GitHub writes: inline text costs no extra request, other files one each. */ @@ -28,7 +29,20 @@ export type Publication = { session: string | null; message: string | null; changes: Change[]; + /** + * A commit of the branch's upstream base that the changes merge in: they + * become a merge commit of `head` and it, relative to its files. + */ + merge: string | null; + /** + * Submodules the merge changes relative to `merge` that the runner cannot + * send, since changes carry no submodules: publishing without them would + * revert them to the merged-in commit's version. + */ + submodules: string[]; pull: { title: string; body: string; draft: boolean } | null; + /** A merge job lands its pull request at the published head, with the merge method its instructions name. */ + land: { method: MergeMethod | null } | null; }; /** Whether `path` names a file inside a work tree, never Git's own directory. */ @@ -40,13 +54,17 @@ export function validPath(path: string) { ); } +const WORKFLOWS = "Changing GitHub Actions workflows needs the workflows permission, which the App does not request"; +const SUBMODULES = + "kix cannot publish submodule changes, so this merge would revert the submodule to the merged-in commit's version"; + /** * Why `workflow` may not publish `path`, or null when it may. A refusal * becomes a maintainer decision rather than silently dropping the change. */ -export function refusal(workflow: Workflow, path: string): string | null { - if (path === ".github/workflows" || path.startsWith(".github/workflows/")) - return "Changing GitHub Actions workflows needs the workflows permission, which the App does not request"; +export function refusal(workflow: Workflow, path: string, merging = false): string | null { + // A merge publication's changes are relative to the merged-in commit, so commit() checks its workflows instead. + if (!merging && (path === ".github/workflows" || path.startsWith(".github/workflows/"))) return WORKFLOWS; if (workflow === "bootstrap" && path !== "flake.nix" && path !== "flake.lock") return "A devShell bootstrap publishes only flake.nix and flake.lock"; if (workflow === "plan" && !(path.startsWith("quest/") && isQuest(path))) @@ -90,8 +108,26 @@ export function parsePublication(body: Record): Publication { if (bytes > LIMITS.bytes) throw new JobError("Publication too large"); if (new Set(changes.map((change) => change.path)).size !== changes.length) throw new JobError("Duplicate change path"); - const message = changes.length ? text(body.message, 64 * 1024) : null; - if (changes.length && !message) throw new JobError("Commit message required"); + if (body.merge !== undefined && !(typeof body.merge === "string" && /^[a-f0-9]{40}$/.test(body.merge))) + throw new JobError("The merged commit must be a full SHA"); + const merge = (body.merge as string | undefined) ?? null; + const submodules = body.submodules ?? []; + if ( + !Array.isArray(submodules) || + submodules.length > LIMITS.changes || + !submodules.every((path) => typeof path === "string" && validPath(path)) + ) + throw new JobError("Invalid submodule paths"); + if (submodules.length && !merge) throw new JobError("Only a merge lists the submodules it cannot publish"); + const message = changes.length || merge ? text(body.message, 64 * 1024) : null; + if ((changes.length || merge) && !message) throw new JobError("Commit message required"); + let land: Publication["land"] = null; + if (body.land !== undefined) { + const method = (body.land as Record | null)?.method; + if (method !== undefined && method !== "merge" && method !== "squash" && method !== "rebase") + throw new JobError("Land with the merge, squash, or rebase method"); + land = { method: (method as MergeMethod | undefined) ?? null }; + } let pull: Publication["pull"] = null; if (body.pull !== undefined) { const input = body.pull as Record | null; @@ -106,8 +142,19 @@ export function parsePublication(body: Record): Publication { throw new JobError("Pull request title and body required"); pull = { title, body: description, draft: input!.draft !== false }; } - if (!changes.length && !pull) throw new JobError("Nothing to publish"); - return { job, attempt, head: body.head as string | null, session, message, changes, pull }; + if (!changes.length && !merge && !pull && !land) throw new JobError("Nothing to publish"); + return { + job, + attempt, + head: body.head as string | null, + session, + message, + changes, + merge, + submodules: submodules as string[], + pull, + land, + }; } /** @@ -122,10 +169,20 @@ export class HeadMoved extends JobError { /** A pause on a decision; `asked` is false for a retry of one already asked. */ type Decision = { decision: string; cancel: boolean; asked: boolean }; -type Published = { head: string | null; pull?: { number: number; url: string; draft: boolean } }; +type Published = { + head: string | null; + pull?: { number: number; url: string; draft: boolean }; + /** A merge job's landing: merged at once, or queued or auto-merging. */ + landed?: { merged: boolean }; +}; const DECISION = "Publishing this work needs a maintainer's decision"; +/** The number of a `pulls/` target. */ +export function pullOf(target: string) { + return Number(target.match(/^pulls\/(\d{1,15})$/)?.[1] ?? Number.NaN); +} + /** Pause the attempt on a decision the maintainer must make before this can publish. */ async function decide(db: D1Database, runner: Runner, input: Publication, prompt: unknown): Promise { const { question, cancel, asked } = await store.ask( @@ -175,7 +232,13 @@ async function commit( branch: Branch, head: string | null, ): Promise { - const next = await branch.commit(head ?? job.revision, input.changes, input.message!); + if (input.merge && !(await branch.based(input.merge))) + throw new JobError("A merge commit may only merge in a commit its upstream base branch already holds"); + const parent = head ?? job.revision; + const next = await branch.commit(parent, input.changes, input.message!, input.merge ?? undefined); + // GitHub checks a merge commit's workflow changes against the head it updates, its first parent. + if (input.merge && (await branch.workflows(parent)) !== (await branch.workflows(next))) + throw new PublicationBlocked(`Merging changes the pull request's workflows. ${WORKFLOWS}.`); // Check the fence again immediately before the write that makes the commit visible. await store.claim(db, runner, job.id, job.attempt, head, next, Date.now()); // A failure here leaves the claim pending; the next publication finds out whether it landed. @@ -189,12 +252,16 @@ async function commit( export async function publish(env: Env, runner: Runner, input: Publication): Promise { const db = env.DB; const job = await store.publishing(db, runner, input.job, input.attempt); - if (input.pull && job.workflow === "iterate") - throw new JobError("An iteration updates its pull request's head and opens no pull request"); - const refused = input.changes.flatMap((change) => { - const reason = refusal(job.workflow, change.path); - return reason ? [{ path: change.path, reason }] : []; - }); + if (input.pull && onPull(job.workflow)) + throw new JobError("Work on a pull request updates its head and opens no pull request"); + if (input.land && job.workflow !== "merge") throw new JobError("Only a merge job lands its pull request"); + const refused = [ + ...input.submodules.map((path) => ({ path, reason: SUBMODULES })), + ...input.changes.flatMap((change) => { + const reason = refusal(job.workflow, change.path, !!input.merge); + return reason ? [{ path: change.path, reason }] : []; + }), + ]; if (refused.length) return decide(db, runner, input, { type: "publication", refused }); const github = new GitHubService(env); const upstream: GitHubRepository = { owner: job.owner, name: job.name, id: job.repository }; @@ -203,14 +270,30 @@ export async function publish(env: Env, runner: Runner, input: Publication): Pro // A pull request opens from the branch, so a delivery alone still checks where it is. head = await github.contribute(upstream, job.actor, job, async (branch) => { const head = await verify(db, job, input, branch); - return input.changes.length ? commit(db, runner, job, input, branch, head) : head; + return input.changes.length || input.merge ? commit(db, runner, job, input, branch, head) : head; }); } catch (error) { if (!(error instanceof PublicationBlocked)) throw error; - // Only a service fork branch is kix's to reset; an iterated pull request's head is not. - const restart = error instanceof BranchChanged && job.workflow !== "iterate"; + // Only a service fork branch is kix's to reset; a pull request's own head is not. + const restart = error instanceof BranchChanged && !onPull(job.workflow); return decide(db, runner, input, { type: "publication", blocked: error.message, ...(restart && { restart }) }); } + if (input.land) { + // The attempt may have ended, or been asked to stop, while the branch was written. + const current = await store.publishing(db, runner, input.job, input.attempt); + if (current.cancel) throw new JobError("The maintainer stopped this job, so it does not land"); + try { + const landed = await github.landPullRequest(upstream, current.actor, pullOf(current.target), { + head: head!, + hold: current.hold, + method: input.land.method, + }); + return { head, landed }; + } catch (error) { + if (!(error instanceof PublicationBlocked)) throw error; + return decide(db, runner, input, { type: "publication", blocked: error.message }); + } + } if (!input.pull) return { head }; if (!head) throw new JobError("Publish the job's changes before opening its pull request"); // The attempt may have ended, or been asked to stop, while the branch was written. @@ -249,7 +332,7 @@ export async function restart( q.attempt === job.attempt, ); const offered = open?.kind === "publication" && (open.prompt as { restart?: unknown } | null)?.restart === true; - if (job.state !== "waiting" || job.workflow === "iterate" || !offered) + if (job.state !== "waiting" || onPull(job.workflow) || !offered) throw new JobError("Only a job whose own branch changed can start over"); // The answering maintainer's authority is rechecked before anything changes. return new GitHubService(env).contribute(upstream, user, job, async (branch) => { diff --git a/cloudflare/src/jobs/routes.ts b/cloudflare/src/jobs/routes.ts index 33680ac8..b8bd7892 100644 --- a/cloudflare/src/jobs/routes.ts +++ b/cloudflare/src/jobs/routes.ts @@ -5,10 +5,11 @@ import * as push from "../push/notify"; import { GitHubError, GitHubService, refName, type Stopped } from "../github/service"; import { type ProjectState, type Pull, continued, startable } from "../github/state"; import { RUNNER_HEADER } from "./hub"; +import * as landing from "./landing"; import * as output from "./output"; import { formAnswer, parseAnswer, parsePrompt } from "./prompt"; import { data, id, text } from "./protocol"; -import { HeadMoved, LIMITS, parsePublication, publish, restart } from "./publish"; +import { HeadMoved, LIMITS, parsePublication, publish, pullOf, restart } from "./publish"; import * as store from "./store"; import { JobError } from "./store"; @@ -304,10 +305,13 @@ function resolve( const n = issue.number; return { workflow, target: `issues/${n}`, revision: commit, branch: `plan/issue-${n}`, trusted: true }; } - if (workflow === "iterate") { + if (workflow === "iterate" || workflow === "merge") { const pull = open("pull", "pulls"); - if (!pull) throw new JobError("Iterate on an open pull request"); + if (!pull) + throw new JobError(workflow === "merge" ? "Merge an open pull request" : "Iterate on an open pull request"); const data = pull.data as Pull; + if (workflow === "merge" && data.isDraft) + throw new JobError("Mark the draft ready for review before merging it"); // Until the sandbox classifies commit authors, every PR head is untrusted // and routes only to Linux runners that build its devShell in a VM. return { @@ -351,7 +355,7 @@ jobs.post(base, async (c) => { if (job.workflow === "bootstrap") { if (flake) throw new JobError("The default branch already has a flake"); } else if (!flake) { - if (job.workflow === "iterate") throw new JobError("The pull request's head has no flake devShell"); + if (store.onPull(job.workflow)) throw new JobError("The pull request's head has no flake devShell"); throw new BootstrapRequired(); } const created = await store.createJob( @@ -359,6 +363,22 @@ jobs.post(base, async (c) => { { ...job, repository: repo.id, actor: user, pinRunner, pinAccount }, Date.now(), ); + // A new merge job holds its pull request before it runs, as a local `quest merge` would. + if (created.created && job.workflow === "merge") { + const pull = pullOf(job.target); + let token: string; + let recorded: boolean; + try { + token = await github.holdPullRequest(repo, user, pull); + recorded = await store.held(c.env.DB, created.id, token); + } catch (error) { + await store.cancel(c.env.DB, created.id, user, Date.now()); + await hub(c.env, user).dispatch(user); + throw error; + } + // A stop that came first found no hold to release, so the label goes back now. + if (!recorded) await github.releasePullRequest(repo, pull, token); + } await hub(c.env, user).dispatch(user); return reply(c, form, "start", created, created.created ? 201 : 200); }); @@ -434,6 +454,8 @@ jobs.post(`${base}/:job/cancel`, async (c) => { const { user, job } = await owned(c); await store.cancel(c.env.DB, job.id, user, Date.now()); await hub(c.env, job.actor).dispatch(job.actor); + // A running attempt reports its cancellation later, which releases its hold then. + await landing.ended(c.env, job.id); return reply(c, fromForm(c) ? {} : null, "stop", null); }); diff --git a/cloudflare/src/jobs/store.ts b/cloudflare/src/jobs/store.ts index 240d718a..57291a43 100644 --- a/cloudflare/src/jobs/store.ts +++ b/cloudflare/src/jobs/store.ts @@ -9,7 +9,12 @@ /** A disconnected runner's running jobs move to another runner after this long. */ export const GRACE = 2 * 60_000; -export type Workflow = "plan" | "start" | "iterate" | "bootstrap"; +export type Workflow = "plan" | "start" | "iterate" | "merge" | "bootstrap"; + +/** Whether a workflow works on an open pull request's own head branch, rather than opening one. */ +export function onPull(workflow: Workflow) { + return workflow === "iterate" || workflow === "merge"; +} export type JobState = "queued" | "running" | "waiting" | "completed" | "failed" | "cancelled"; export type Outcome = "completed" | "failed" | "cancelled" | "limited"; export type Platform = "linux" | "darwin"; @@ -55,6 +60,8 @@ export type Job = { pull: number | null; /** The failed job this retry continues. */ continues: string | null; + /** A merge job's `quest:merge` label: the timeline event that added it, until kix releases it. */ + hold: string | null; /** A new plan's request: the maintainer's idea, and pasted context the agent reads as untrusted. */ idea: string | null; context: string | null; @@ -197,6 +204,16 @@ export async function listRunners(db: D1Database, owner: string, online: Set[]) + .filter((job) => job.workflow === "merge" && job.hold !== null && job.state === "cancelled") + .map((job) => job.id); +} + const fence = "EXISTS(SELECT 1 FROM jobs WHERE id=? AND attempt=? AND runner=? AND state='running')"; /** @@ -204,7 +221,8 @@ const fence = "EXISTS(SELECT 1 FROM jobs WHERE id=? AND attempt=? AND runner=? A * running. Listing an attempt acknowledges its assignment. Says which * reported attempts to continue, cancel, or stop because they are no longer * this runner's. An acknowledged attempt the status omits was lost, so it - * moves now; an unacknowledged one may still be on its way. + * moves now; an unacknowledged one may still be on its way. Also returns + * the merge jobs this cancelled that still hold their pull request. */ export async function report( db: D1Database, @@ -236,7 +254,7 @@ export async function report( .bind(runner.id, runner.owner, reported), db .prepare( - `UPDATE jobs SET state=CASE WHEN cancel=1 THEN 'cancelled' ELSE 'queued' END,lease=NULL,updatedAt=? WHERE runner=? AND state='running' AND delivered=1 AND NOT ${listed}`, + `UPDATE jobs SET state=CASE WHEN cancel=1 THEN 'cancelled' ELSE 'queued' END,lease=NULL,updatedAt=? WHERE runner=? AND state='running' AND delivered=1 AND NOT ${listed} RETURNING ${ending}`, ) .bind(now, runner.id, running), db @@ -249,11 +267,14 @@ export async function report( .bind(running), ]); const rows = results.at(-1)!.results as Pick[]; - return attempts.map(({ job, attempt }) => { - const row = rows.find((r) => r.id === job); - const owned = row && row.attempt === attempt && row.runner === runner.id && row.state === "running"; - return { job, attempt, action: !owned ? "stop" : row.cancel ? "cancel" : "continue" } as const; - }); + return { + attempts: attempts.map(({ job, attempt }) => { + const row = rows.find((r) => r.id === job); + const owned = row && row.attempt === attempt && row.runner === runner.id && row.state === "running"; + return { job, attempt, action: !owned ? "stop" : row.cancel ? "cancel" : "continue" } as const; + }), + ended: ended(results[3]!), + }; } /** @@ -283,14 +304,15 @@ export type Claimed = NonNullable>> & { owner * Reconcile an owner's jobs with their connected runners and free slots. * A running job has a lease exactly while its runner is disconnected; an * expired lease moves the job. Then queued work is assigned. Returns what to - * send each runner and when to run again. Every assignment its runner has + * send each runner, the merge jobs it cancelled that still hold their pull + * request, and when to run again. Every assignment its runner has * not acknowledged is sent again, since a delivery can be lost after the * claim commits. Until acknowledged, an assignment holds one of the runner's * reported free slots, which it could not count yet. */ export async function dispatch(db: D1Database, owner: string, slots: Map, now: number) { const connected = JSON.stringify([...slots.keys()]); - const [, , , , candidates, offers, cancels, pending] = await db.batch([ + const [, , cancelled, , candidates, offers, cancels, pending] = await db.batch([ db .prepare( "UPDATE jobs SET lease=NULL WHERE actor=? AND state='running' AND lease IS NOT NULL AND runner IN (SELECT value FROM json_each(?))", @@ -304,7 +326,7 @@ export async function dispatch(db: D1Database, owner: string, slots: Map'merge' OR j.hold IS NOT NULL) + ORDER BY j.createdAt,j.id`, ) .bind(owner), db @@ -386,6 +409,7 @@ export async function dispatch(db: D1Database, owner: string, slots: Map'iterate' AND f.state='failed' AND f.branch=? AND f.id=(SELECT id FROM jobs WHERE repository=p.id AND target=? ORDER BY createdAt DESC,id LIMIT 1) + LEFT JOIN jobs f ON ?=0 AND f.state='failed' AND f.branch=? AND f.id=(SELECT id FROM jobs WHERE repository=p.id AND target=? ORDER BY createdAt DESC,id LIMIT 1) WHERE p.id=? AND p.state='enabled' AND ${pinnable}`, ) .bind( @@ -662,7 +686,7 @@ export async function createJob(db: D1Database, input: NewJob, now: number) { input.target, input.revision, input.branch, - input.workflow === "iterate" ? input.revision : null, + onPull(input.workflow) ? input.revision : null, input.idea ?? null, input.context ?? null, input.actor, @@ -671,7 +695,7 @@ export async function createJob(db: D1Database, input: NewJob, now: number) { account, now, now, - input.workflow, + onPull(input.workflow) ? 1 : 0, input.branch, input.target, input.repository, @@ -1023,6 +1047,32 @@ export async function hold(db: D1Database, job: string, input: { summary: string ]); } +/** + * Record the `quest:merge` label a merge job holds its pull request with, + * while the job is still queued. Returns false once it was stopped meanwhile, + * so the caller gives the label back. + */ +export async function held(db: D1Database, job: string, token: string) { + const { meta } = await db.prepare("UPDATE jobs SET hold=? WHERE id=? AND state='queued'").bind(token, job).run(); + return meta.changes === 1; +} + +/** A merge job's hold on its pull request is over. */ +export async function released(db: D1Database, job: string, token: string) { + await db.prepare("UPDATE jobs SET hold=NULL WHERE id=? AND hold=?").bind(job, token).run(); +} + +/** The ended merge jobs on `target` that still hold its pull request, such as one whose landing is queued. */ +export async function holding(db: D1Database, repository: number, target: string) { + const { results } = await db + .prepare( + "SELECT id,hold FROM jobs WHERE repository=? AND target=? AND workflow='merge' AND hold IS NOT NULL AND state NOT IN ('queued','running','waiting')", + ) + .bind(repository, target) + .all<{ id: string; hold: string }>(); + return results; +} + /** * Stop a job. Queued, waiting, and never-acknowledged jobs stop now; a * running attempt is asked to cancel and reports its partial result. diff --git a/cloudflare/test/inbox.test.ts b/cloudflare/test/inbox.test.ts index 4787a568..5e2db90a 100644 --- a/cloudflare/test/inbox.test.ts +++ b/cloudflare/test/inbox.test.ts @@ -257,17 +257,42 @@ describe("decision inbox", () => { expect(thread(html)).toContain("GitHub reported changes"); }); - it("offers merge only on a clean pull request no job holds, to current maintainers", async () => { + it("offers merge only on a pull request no job or session holds, to current maintainers", async () => { const merge = "/projects/github/upstream/project/pulls/2/merge"; await db .prepare( "UPDATE github_project_activity SET data=json_set(data,'$.mergeStateStatus','BLOCKED') WHERE id='21'", ) .run(); + // A pull request GitHub cannot merge as is lands through a merge job, which fixes it first. let active = thread(await inbox()); expect(active).not.toContain(merge); + expect(active).toContain(''); + expect(active).toContain('