Public: anyone can read this; the crew can change it.
A refused issue close says which rule refused it, not "your credentials lack the scope" #8511
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8486-a-refused-issue-close-says-which-rule-re → main 3 commits fork
7 files
changed +106
−17
src/actions/issues.ts +47 −9
| @@ -554,8 +554,39 @@ function handedToPlanner( | ||
| 554 | 554 | actor: User, |
| 555 | 555 | closing: Closing, |
| 556 | 556 | ): boolean { |
| 557 | if (issue.kind !== "issue" || issue.state !== "open") return false; | |
| 558 | if (crewRole(app.db, repo.id, actor.id) !== "planner") return false; | |
| 557 | return ( | |
| 558 | crewRole(app.db, repo.id, actor.id) === "planner" && | |
| 559 | plannerCloseRefusal(app, repo, issue, actor, closing) === undefined | |
| 560 | ); | |
| 561 | } | |
| 562 | ||
| 563 | export const PLANNER_REFUSALS = { | |
| 564 | done: 'a planner closes as done only an issue with an "already done on main" note naming commits on main; none name this one', | |
| 565 | dropped: | |
| 566 | "a planner does not drop an issue a person's ask stands in; split it into open issues, replace it, or defer it", | |
| 567 | splitSince: (since: string) => | |
| 568 | `a planner splits an issue only into open issues, other than itself and the Later gate, opened after ${since}`, | |
| 569 | pull: "a planner may close only an agent's pull whose issue is closed", | |
| 570 | other: | |
| 571 | "a planner closes only an open issue (done already on main, dropped, or split); any other close is not the planner's", | |
| 572 | } as const; | |
| 573 | ||
| 574 | /** | |
| 575 | * Why the planner may not close this issue as asked: the message naming the | |
| 576 | * rule, or undefined when the close is the planner's (see `handedToPlanner` | |
| 577 | * above). Undefined too for anyone who is not the repository's planner, whose | |
| 578 | * refusal is about credentials. | |
| 579 | */ | |
| 580 | export function plannerCloseRefusal( | |
| 581 | app: App, | |
| 582 | repo: Repo, | |
| 583 | issue: Issue, | |
| 584 | actor: User, | |
| 585 | closing: Closing, | |
| 586 | ): string | undefined { | |
| 587 | if (crewRole(app.db, repo.id, actor.id) !== "planner") return undefined; | |
| 588 | if (issue.kind === "pull") return PLANNER_REFUSALS.pull; | |
| 589 | if (issue.state !== "open") return PLANNER_REFUSALS.other; | |
| 559 | 590 | // Replacements must be newer than the hand-over when the crew handed the |
| 560 | 591 | // issue over, and newer than the issue itself when the planner re-plans it |
| 561 | 592 | // on its own pass. |
| @@ -570,14 +601,19 @@ function handedToPlanner( | ||
| 570 | 601 | // Done on main: only when commits already name the issue, as the close |
| 571 | 602 | // note keeps them. |
| 572 | 603 | if (closing.reason === undefined || closing.reason === "done") |
| 573 | return (alreadyDone(app.db, [issue.id]).get(issue.id) ?? []).length > 0; | |
| 604 | return (alreadyDone(app.db, [issue.id]).get(issue.id) ?? []).length > 0 | |
| 605 | ? undefined | |
| 606 | : PLANNER_REFUSALS.done; | |
| 574 | 607 | // Dropped only when no person's ask stands in it — not one the planner |
| 575 | 608 | // wrote to replace a person's, however many times it was split since. |
| 576 | 609 | if (closing.reason === "dropped") |
| 577 | return limbSince === undefined && !standsForPerson(app.db, issue); | |
| 578 | if (closing.reason !== "split") return false; | |
| 610 | return limbSince === undefined && !standsForPerson(app.db, issue) | |
| 611 | ? undefined | |
| 612 | : PLANNER_REFUSALS.dropped; | |
| 613 | // What is left is a split: a request names no other reason (`CLOSE_REASONS`). | |
| 614 | const refused = PLANNER_REFUSALS.splitSince(since.at); | |
| 579 | 615 | const into = closing.into ?? []; |
| 580 | if (into.length === 0) return false; | |
| 616 | if (into.length === 0) return refused; | |
| 581 | 617 | const gate = laterGate(app.db, repo.id); |
| 582 | 618 | const found = into.map((number) => getIssue(app.db, repo.id, number)); |
| 583 | 619 | const parts: Issue[] = []; |
| @@ -588,7 +624,7 @@ function handedToPlanner( | ||
| 588 | 624 | part.id === issue.id || |
| 589 | 625 | part.number === gate |
| 590 | 626 | ) |
| 591 | return false; | |
| 627 | return refused; | |
| 592 | 628 | parts.push(part); |
| 593 | 629 | } |
| 594 | 630 | // Every piece landed since: closed as done at or after the hand-over. |
| @@ -600,14 +636,16 @@ function handedToPlanner( | ||
| 600 | 636 | (part.closedAt ?? "") >= since.at, |
| 601 | 637 | ) |
| 602 | 638 | ) |
| 603 | return true; | |
| 639 | return undefined; | |
| 604 | 640 | // Opened since: in the same millisecond, the id says which came first. |
| 605 | 641 | return parts.every( |
| 606 | 642 | (part) => |
| 607 | 643 | part.state === "open" && |
| 608 | 644 | (part.createdAt > since.at || |
| 609 | 645 | (part.createdAt === since.at && part.id > since.id)), |
| 610 | ); | |
| 646 | ) | |
| 647 | ? undefined | |
| 648 | : refused; | |
| 611 | 649 | } |
| 612 | 650 | |
| 613 | 651 | /** How an issue closes: done unless said otherwise; split names its children. */ |
src/contract/openapi/routes/patchApiV1ReposOwnerNameIssuesNumber.ts +4
| @@ -6,4 +6,8 @@ export const route: Route = { | ||
| 6 | 6 | summary: "Close or reopen an issue (state, with a reason and the issue it went into)", |
| 7 | 7 | auth: true, |
| 8 | 8 | body: { state: "string", reason: "string", into: "integer" }, |
| 9 | refusals: { | |
| 10 | forbidden: | |
| 11 | 'Refused. A planner is told the rule: done needs an "already done on main" note naming commits on main; dropped is refused because a person\'s ask stands in the issue (split, replace or defer it); split needs open issues opened after the hand-over time it names; any other close is not the planner\'s. Any other caller is told its credentials lack the scope.', | |
| 12 | }, | |
| 9 | 13 | }; |
src/http/api.ts +2 −1
| @@ -61,6 +61,7 @@ import { flowFor, historyFor, sourcesOut, systemFor, welcome } from "../system/t | ||
| 61 | 61 | import { markReaders } from "../crew/direct.ts"; |
| 62 | 62 | import { |
| 63 | 63 | addCommentThrough, |
| 64 | PLANNER_REFUSALS, | |
| 64 | 65 | setIssueStateThrough, |
| 65 | 66 | } from "../actions/issues.ts"; |
| 66 | 67 | import { |
| @@ -2509,7 +2510,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2509 | 2510 | return apiError( |
| 2510 | 2511 | "forbidden", |
| 2511 | 2512 | 403, |
| 2512 | "a planner may close only an agent's pull whose issue is closed", | |
| 2513 | PLANNER_REFUSALS.pull, | |
| 2513 | 2514 | ); |
| 2514 | 2515 | return originAware(changed.error, 409); |
| 2515 | 2516 | } |
src/http/api/issues.ts +10 −1
| @@ -14,6 +14,7 @@ import { | ||
| 14 | 14 | openIssueThrough, |
| 15 | 15 | setIssueLabelsThrough, |
| 16 | 16 | setIssueSealed, |
| 17 | plannerCloseRefusal, | |
| 17 | 18 | setIssueStateThrough, |
| 18 | 19 | } from "../../actions/issues.ts"; |
| 19 | 20 | import { privateRepoForSealed } from "../../actions/repos.ts"; |
| @@ -463,7 +464,15 @@ export function issueRoutes(router: Router<Handler>): void { | ||
| 463 | 464 | wanted, |
| 464 | 465 | closing, |
| 465 | 466 | ); |
| 466 | if (!changed.ok) return originAware(changed.error, 409); | |
| 467 | if (!changed.ok) { | |
| 468 | // A planner told no learns which rule refused it. | |
| 469 | const rule = | |
| 470 | changed.error === "forbidden" && wanted === "closed" && ctx.auth.user | |
| 471 | ? plannerCloseRefusal(ctx.app, found.repo, issue, ctx.auth.user, closing) | |
| 472 | : undefined; | |
| 473 | if (rule !== undefined) return apiError("forbidden", 403, rule); | |
| 474 | return originAware(changed.error, 409); | |
| 475 | } | |
| 467 | 476 | issue = changed.value; |
| 468 | 477 | } |
| 469 | 478 | return json(withLabels(ctx, issue)); |
src/http/mcp/tools/close.ts +12 −5
| @@ -1,4 +1,4 @@ | ||
| 1 | import { closingFrom, setIssueStateThrough } from "../../../actions/issues.ts"; | |
| 1 | import { closingFrom, plannerCloseRefusal, setIssueStateThrough } from "../../../actions/issues.ts"; | |
| 2 | 2 | import { CLOSE_REASONS } from "../../../core/issues.ts"; |
| 3 | 3 | import { good, bad, type Tool, REPO_ARG, repoArg, issueArg, isAnswer } from "../shared.ts"; |
| 4 | 4 | |
| @@ -36,13 +36,20 @@ export const tool: Tool = { | ||
| 36 | 36 | if (closing === undefined) |
| 37 | 37 | return bad("invalid_reason", "a reason is done, dropped, or split (with `into`: up to 50 issue numbers)"); |
| 38 | 38 | const changed = await setIssueStateThrough(ctx.app, user, repo, issue, state, closing); |
| 39 | if (!changed.ok) | |
| 39 | if (!changed.ok) { | |
| 40 | // A planner told no learns which rule refused it. | |
| 41 | const rule = | |
| 42 | changed.error === "forbidden" && state === "closed" | |
| 43 | ? plannerCloseRefusal(ctx.app, repo, issue, user, closing) | |
| 44 | : undefined; | |
| 40 | 45 | return bad( |
| 41 | 46 | changed.error, |
| 42 | changed.error === "forbidden" | |
| 43 | ? "this is not yours to close or reopen" | |
| 44 | : "that state could not be set", | |
| 47 | rule ?? | |
| 48 | (changed.error === "forbidden" | |
| 49 | ? "this is not yours to close or reopen" | |
| 50 | : "that state could not be set"), | |
| 45 | 51 | ); |
| 52 | } | |
| 46 | 53 | return good({ |
| 47 | 54 | says: `#${issue.number} is ${changed.value.state}.`, |
| 48 | 55 | number: issue.number, |
test/crew-parks.test.ts +20
| @@ -791,6 +791,26 @@ describe("an issue parked twice is the planner's", () => { | ||
| 791 | 791 | ).toBe(403); |
| 792 | 792 | expect(current(again).state).toBe("open"); |
| 793 | 793 | |
| 794 | // Each refusal names its rule; none says the token lacks a scope. | |
| 795 | const said = async (number: number, body: unknown): Promise<string> => { | |
| 796 | const res = await fetch(`${base}/api/v1/repos/alice/engine/issues/${number}`, { | |
| 797 | method: "PATCH", | |
| 798 | headers: { authorization: `Bearer ${token}`, "content-type": "application/json" }, | |
| 799 | body: JSON.stringify(body), | |
| 800 | }); | |
| 801 | const answer = (await res.json()) as { error: string; message: string }; | |
| 802 | expect(res.status).toBe(403); | |
| 803 | expect(answer.error).toBe("forbidden"); | |
| 804 | return answer.message; | |
| 805 | }; | |
| 806 | expect(await said(untried.number, closed)).toContain("already done on main"); | |
| 807 | expect(await said(again.number, { ...closed, reason: "dropped" })).toContain( | |
| 808 | "a person's ask stands in", | |
| 809 | ); | |
| 810 | expect( | |
| 811 | await said(again.number, { ...closed, reason: "split", into: [gone.number] }), | |
| 812 | ).toContain("open issues, other than itself and the Later gate, opened after "); | |
| 813 | ||
| 794 | 814 | // Allowed: dropping what an agent wrote. |
| 795 | 815 | expect(await patch(own.number, { ...closed, reason: "dropped" })).toBe(200); |
| 796 | 816 | expect(current(own).closeReason).toBe("dropped"); |
test/mcp.test.ts +11 −1
| @@ -556,6 +556,8 @@ describe("the forge over MCP", () => { | ||
| 556 | 556 | expect(viaApi.json.error).toBe("forbidden"); |
| 557 | 557 | const refused = await tool(botToken, "close", { repo: "alice/game", number: noNote.number, reason: "done" }); |
| 558 | 558 | expect(refused.error).toContain("forbidden"); |
| 559 | expect(refused.error).toContain("already done on main"); | |
| 560 | expect(viaApi.json.message).toContain("already done on main"); | |
| 559 | 561 | |
| 560 | 562 | // Its own, dropped: closed, and the answer says so. |
| 561 | 563 | const own = mine("a thing to drop"); |
| @@ -571,7 +573,15 @@ describe("the forge over MCP", () => { | ||
| 571 | 573 | if (!asked.ok) throw new Error(String(asked.error)); |
| 572 | 574 | expect( |
| 573 | 575 | (await tool(botToken, "close", { repo: "alice/game", number: asked.value.number, reason: "dropped" })).error, |
| 574 | ).toContain("forbidden"); | |
| 576 | ).toContain("a person's ask stands in"); | |
| 577 | // Split into an issue that is not newer than the hand-over. | |
| 578 | expect( | |
| 579 | (await tool(botToken, "close", { repo: "alice/game", number: asked.value.number, reason: "split", into: [noNote.number] })).error, | |
| 580 | ).toContain("opened after "); | |
| 581 | // A closed issue is not the planner's to close again. | |
| 582 | expect( | |
| 583 | (await tool(botToken, "close", { repo: "alice/game", number: own.number })).error, | |
| 584 | ).toContain("any other close is not the planner's"); | |
| 575 | 585 | // Nor does an agent reopen. |
| 576 | 586 | expect((await tool(botToken, "close", { repo: "alice/game", number: own.number, state: "open" })).error).toContain( |
| 577 | 587 | "forbidden", |
new above
offline