Public: anyone can read this; the crew can change it.
The planner can close a crew pull whose issue it closed: PATCH …/pulls/{number} {"state":"closed"} with its run token #3305
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3301-the-planner-can-close-a-crew-pull-whose → main 2 commits fork 3 threads · 3 unresolved
3 files
changed +182
−6
src/actions/issues.ts +43 −4
| @@ -46,7 +46,7 @@ import { | ||
| 46 | 46 | standsForPerson, |
| 47 | 47 | } from "../core/issues.ts"; |
| 48 | 48 | import { isStructural, pillarsOf } from "../core/pillars.ts"; |
| 49 | import { getPull } from "../core/pulls.ts"; | |
| 49 | import { closesFromBody, getPull } from "../core/pulls.ts"; | |
| 50 | 50 | import { canRead, canWrite, type Repo } from "../core/repos.ts"; |
| 51 | 51 | import { getUserById, type User } from "../core/users.ts"; |
| 52 | 52 | import { |
| @@ -400,12 +400,16 @@ export function setIssueState( | ||
| 400 | 400 | issue.kind === "pull" && |
| 401 | 401 | user.value.kind === "agent" && |
| 402 | 402 | user.value.id === issue.authorId; |
| 403 | const plannerClosesPull = | |
| 404 | state === "closed" && | |
| 405 | plannerMayClosePull(app, repo, issue, user.value, closing); | |
| 403 | 406 | const mayModerate = |
| 404 | 407 | ownSay || |
| 405 | 408 | withdrawsOwn || |
| 406 | 409 | canWrite(app.db, repo, user.value) || |
| 407 | 410 | (state === "closed" && |
| 408 | handedToPlanner(app, repo, issue, user.value, closing)); | |
| 411 | handedToPlanner(app, repo, issue, user.value, closing)) || | |
| 412 | plannerClosesPull; | |
| 409 | 413 | if (!mayModerate) return err("forbidden"); |
| 410 | 414 | // The plan's own issues — a pillar's umbrella, the Later umbrella, the |
| 411 | 415 | // Later gate — are a person's to close, whatever access an agent holds: |
| @@ -459,6 +463,13 @@ export function setIssueState( | ||
| 459 | 463 | .map((o) => `[\`${o.slice(0, 7)}\`](/${repo.ownerName}/${repo.name}/commit/${o})`) |
| 460 | 464 | .join(", ")}.`, |
| 461 | 465 | }); |
| 466 | // The planner's close of a pull is logged on its thread, with the why. | |
| 467 | if (written && plannerClosesPull && !canWrite(app.db, repo, user.value)) | |
| 468 | insertComment(app.db, { | |
| 469 | issueId: issue.id, | |
| 470 | authorId: user.value.id, | |
| 471 | body: PLANNER_CLOSED_PULL_NOTE, | |
| 472 | }); | |
| 462 | 473 | return written; |
| 463 | 474 | })(); |
| 464 | 475 | if (!changed) return ok(getIssueById(app.db, issue.id) ?? issue); |
| @@ -481,6 +492,34 @@ export function setIssueState( | ||
| 481 | 492 | return ok(changed); |
| 482 | 493 | } |
| 483 | 494 | |
| 495 | export const PLANNER_CLOSED_PULL_NOTE = | |
| 496 | "Closed by the planner: the issue this pull was built for is closed."; | |
| 497 | ||
| 498 | /** | |
| 499 | * The one close the planner has on a pull: an open, unmerged pull an agent | |
| 500 | * wrote, whose description names the issue(s) it was built for and every | |
| 501 | * one of them is closed. A person's pull, a merge, and a reopen stay with | |
| 502 | * the people who can write here. | |
| 503 | */ | |
| 504 | function plannerMayClosePull( | |
| 505 | app: App, | |
| 506 | repo: Repo, | |
| 507 | issue: Issue, | |
| 508 | actor: User, | |
| 509 | closing: Closing, | |
| 510 | ): boolean { | |
| 511 | if (issue.kind !== "pull" || issue.state !== "open") return false; | |
| 512 | if (actor.kind !== "agent" || crewRole(app.db, repo.id, actor.id) !== "planner") | |
| 513 | return false; | |
| 514 | if (closing.reason !== undefined || closing.into !== undefined) return false; | |
| 515 | if (!issue.authorIsAgent) return false; | |
| 516 | if (getPull(app.db, repo.id, issue.number)?.mergedAt != null) return false; | |
| 517 | const built = closesFromBody(issue.body) | |
| 518 | .map((n) => getIssue(app.db, repo.id, n)) | |
| 519 | .filter((i): i is Issue => i !== undefined && i.kind === "issue"); | |
| 520 | return built.length > 0 && built.every((i) => i.state === "closed"); | |
| 521 | } | |
| 522 | ||
| 484 | 523 | /** |
| 485 | 524 | * The one close a crew member may make on an issue it did not write: the |
| 486 | 525 | * repository's planner, on an open issue it is re-planning — one the crew |
| @@ -497,8 +536,8 @@ export function setIssueState( | ||
| 497 | 536 | * |
| 498 | 537 | * - done, only when commits on main already name the issue (`alreadyDone`). |
| 499 | 538 | * |
| 500 | * A pull, a reopen, and any other close stay with the people who can write | |
| 501 | * here. | |
| 539 | * A reopen and any other close stay with the people who can write here; the | |
| 540 | * one close of a pull is `plannerMayClosePull`'s. | |
| 502 | 541 | */ |
| 503 | 542 | function handedToPlanner( |
| 504 | 543 | app: App, |
src/http/api.ts +15 −2
| @@ -333,7 +333,7 @@ import { | ||
| 333 | 333 | import { appsRuntime, envsFor, type AppsRuntime } from "../apps/runtime.ts"; |
| 334 | 334 | import { isEnvName } from "../apps/data.ts"; |
| 335 | 335 | import { getAppByRepo } from "../core/apps.ts"; |
| 336 | import { isCrew, type Agent } from "../core/agents.ts"; | |
| 336 | import { crewRole, isCrew, type Agent } from "../core/agents.ts"; | |
| 337 | 337 | import { listApps, type AppRecord } from "../core/apps.ts"; |
| 338 | 338 | import { |
| 339 | 339 | CLOSE_REASONS, |
| @@ -3735,7 +3735,20 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3735 | 3735 | getPull(ctx.app.db, found.repo.id, found.pull.number)!, |
| 3736 | 3736 | wanted, |
| 3737 | 3737 | ); |
| 3738 | if (!changed.ok) return originAware(changed.error, 409); | |
| 3738 | if (!changed.ok) { | |
| 3739 | // A planner told no learns the one rule it works by. | |
| 3740 | if ( | |
| 3741 | changed.error === "forbidden" && | |
| 3742 | wanted === "closed" && | |
| 3743 | crewRole(ctx.app.db, found.repo.id, ctx.auth.user!.id) === "planner" | |
| 3744 | ) | |
| 3745 | return apiError( | |
| 3746 | "forbidden", | |
| 3747 | 403, | |
| 3748 | "a planner may close only an agent's pull whose issue is closed", | |
| 3749 | ); | |
| 3750 | return originAware(changed.error, 409); | |
| 3751 | } | |
| 3739 | 3752 | } |
| 3740 | 3753 | return json( |
| 3741 | 3754 | withLabels(ctx, getPull(ctx.app.db, found.repo.id, found.pull.number)!), |
test/planner-closes-pull.test.ts added +124
| @@ -0,0 +1,124 @@ | ||
| 1 | /** | |
| 2 | * The planner's one close on a pull (#3301): an agent's pull whose issue is | |
| 3 | * closed. An open issue, a person's pull, a merge and a reopen stay refused. | |
| 4 | */ | |
| 5 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 6 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 7 | import { tmpdir } from "node:os"; | |
| 8 | import { join } from "node:path"; | |
| 9 | import { createApp, type App } from "../src/app.ts"; | |
| 10 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 11 | import { listComments } from "../src/core/issues.ts"; | |
Nit: |
||
| 12 | import { openIssue, setIssueState } from "../src/actions/issues.ts"; | |
| 13 | import { createRepo } from "../src/actions/repos.ts"; | |
| 14 | import { setCrewRole } from "../src/core/agents.ts"; | |
| 15 | import { getIssueById, type Issue } from "../src/core/issues.ts"; | |
| 16 | import type { Repo } from "../src/core/repos.ts"; | |
| 17 | import { createToken } from "../src/core/tokens.ts"; | |
| 18 | import { createUser, getUserByName, type User } from "../src/core/users.ts"; | |
| 19 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 20 | ||
| 21 | describe("the planner closes a pull", () => { | |
| 22 | let dataDir: string; | |
| 23 | let app: App; | |
| 24 | let servers: Servers; | |
| 25 | let alice: User; | |
| 26 | let atlas: User; | |
| 27 | let mason: User; | |
| 28 | let repo: Repo; | |
| 29 | let token: string; | |
| 30 | ||
| 31 | const seedPull = (by: User, title: string, body: string): Issue => { | |
| 32 | const opened = openIssue(app, by, repo, { title, body }); | |
| 33 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 34 | app.db | |
| 35 | .query( | |
| 36 | `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid) | |
| 37 | VALUES (?1, 'main', ?2, 'a', 'b')`, | |
| 38 | ) | |
| 39 | .run(opened.value.id, `f${opened.value.number}`); | |
| 40 | app.db.query("UPDATE issue SET kind = 'pull' WHERE id = ?1").run(opened.value.id); | |
| 41 | return opened.value; | |
| 42 | }; | |
| 43 | const patch = (number: number, state: string, as = token) => | |
| 44 | fetch(`${servers.url}/api/v1/repos/alice/engine/pulls/${number}`, { | |
| 45 | method: "PATCH", | |
| 46 | headers: { authorization: `Bearer ${as}`, "content-type": "application/json" }, | |
| 47 | body: JSON.stringify({ state }), | |
| 48 | }); | |
| 49 | ||
| 50 | beforeAll(async () => { | |
| 51 | dataDir = await mkdtemp(join(tmpdir(), "forge-planner-closes-pull-")); | |
| 52 | app = createApp({ dataDir, port: 0 }); | |
| 53 | servers = await serve(app); | |
| 54 | const made = await createUser(app.db, { | |
| 55 | origin: "seed", | |
| 56 | trusted: true, | |
| 57 | username: "alice", | |
| 58 | email: "[email protected]", | |
| 59 | password: "a-good-passphrase", | |
| 60 | adminIfFirst: true, | |
| 61 | }); | |
| 62 | if (!made.ok) throw new Error(made.error); | |
| 63 | alice = made.value; | |
| 64 | const r = await createRepo(app, alice, { name: "engine", visibility: "public" }); | |
| 65 | if (!r.ok) throw new Error(String(r.error)); | |
| 66 | repo = r.value; | |
| 67 | for (const name of ["atlas", "mason"]) { | |
| 68 | const agent = await createAgent(app, alice, { username: name }); | |
| 69 | if (!agent.ok) throw new Error(String(agent.error)); | |
| 70 | expect(addToCrew(app, alice, repo, name).ok).toBe(true); | |
| 71 | } | |
| 72 | atlas = getUserByName(app.db, "atlas")!; | |
| 73 | mason = getUserByName(app.db, "mason")!; | |
| 74 | setCrewRole(app.db, repo.id, atlas.id, "planner"); | |
| 75 | token = createToken(app.db, atlas.id, "rw", "rw").token; | |
| 76 | }); | |
| 77 | ||
| 78 | afterAll(async () => { | |
| 79 | await servers.stop(); | |
| 80 | await rm(dataDir, { recursive: true, force: true }); | |
| 81 | }); | |
| 82 | ||
| 83 | test("closes an agent's pull whose issue is closed, and says so on the thread", async () => { | |
| 84 | const ask = openIssue(app, alice, repo, { title: "ask" }); | |
| 85 | if (!ask.ok) throw new Error("ask"); | |
| 86 | expect(setIssueState(app, alice, repo, ask.value, "closed").ok).toBe(true); | |
| 87 | const pull = seedPull(mason, "built", `Closes #${ask.value.number}`); | |
| 88 | const res = await patch(pull.number, "closed"); | |
| 89 | expect(res.status).toBe(200); | |
| 90 | expect(getIssueById(app.db, pull.id)!.state).toBe("closed"); | |
| 91 | expect(listComments(app.db, pull.id).some((c) => c.body.startsWith("Closed by the planner"))).toBe(true); | |
| 92 | // Closing only: it may not reopen. | |
| 93 | expect((await patch(pull.number, "open")).status).toBe(403); | |
| 94 | }); | |
| 95 | ||
| 96 | test("refuses an agent's pull whose issue is still open", async () => { | |
| 97 | const ask = openIssue(app, alice, repo, { title: "still wanted" }); | |
| 98 | if (!ask.ok) throw new Error("ask"); | |
| 99 | const pull = seedPull(mason, "built", `Closes #${ask.value.number}`); | |
| 100 | const res = await patch(pull.number, "closed"); | |
| 101 | expect(res.status).toBe(403); | |
| 102 | expect(((await res.json()) as { message: string }).message).toContain("issue is closed"); | |
| 103 | expect(getIssueById(app.db, pull.id)!.state).toBe("open"); | |
| 104 | }); | |
| 105 | ||
| 106 | test("refuses a person's pull", async () => { | |
Add the role boundary: a non-planner crew agent (mason with its own |
||
| 107 | const ask = openIssue(app, alice, repo, { title: "done ask" }); | |
| 108 | if (!ask.ok) throw new Error("ask"); | |
| 109 | expect(setIssueState(app, alice, repo, ask.value, "closed").ok).toBe(true); | |
| 110 | const pull = seedPull(alice, "mine", `Closes #${ask.value.number}`); | |
| 111 | expect((await patch(pull.number, "closed")).status).toBe(403); | |
| 112 | expect(getIssueById(app.db, pull.id)!.state).toBe("open"); | |
| 113 | }); | |
| 114 | ||
| 115 | test("refuses a crew agent who is not the planner", async () => { | |
| 116 | const ask = openIssue(app, alice, repo, { title: "shipped ask" }); | |
| 117 | if (!ask.ok) throw new Error("ask"); | |
| 118 | expect(setIssueState(app, alice, repo, ask.value, "closed").ok).toBe(true); | |
| 119 | const pull = seedPull(atlas, "built", `Closes #${ask.value.number}`); | |
| 120 | const builder = createToken(app.db, mason.id, "rw", "rw").token; | |
| 121 | expect((await patch(pull.number, "closed", builder)).status).toBe(403); | |
| 122 | expect(getIssueById(app.db, pull.id)!.state).toBe("open"); | |
| 123 | }); | |
| 124 | }); |
new above
offline
This lands under
handedToPlanner's JSDoc, so that comment now documents this constant andhandedToPlannerhas none. Move the note andplannerMayClosePullabove the JSDoc block, and change its last line ("A pull … stay with the people who can write here"), which this change makes untrue.