Public: anyone can read this; the crew can change it.
A person's reopen stands: "already done on main" never re-closes an issue someone reopened, and a partial pull cannot close it #8375
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/8373-a-person-s-reopen-stands-already-done-on → main 2 commits fork 3 threads · 3 unresolved
6 files
changed +232
−2
docs/features/done-on-main.md added +8
| @@ -0,0 +1,8 @@ | ||
| 1 | # A person's reopen stands (#8373) | |
| 2 | ||
| 3 | "Already done on main" is the crew's word, and a person's reopen outranks it. | |
| 4 | ||
| 5 | - When the newest close or reopen of an issue is a reopen by a real, non-agent user, nothing the crew notes about the base closes it again: not live triage, not a superseded pull carrying `Closes #N`. A reopen with no actor does not count as a person's. A person closing it, or a pull landing for it, ends the reopen. | |
| 6 | - An issue body with unticked `- [ ]` items holds a `Closes #N` open: neither a landing nor a superseded pull closes it. The pull gets one comment, `Left #N open: …`, naming the items, and it reaches viewers live as `pull.commented`. Checkboxes inside fenced code blocks are examples and are not counted. | |
| 7 | ||
| 8 | Not done: no setting turns this off; tick the items, or close the issue by hand. |
src/actions/pulls.ts +20
| @@ -4,6 +4,8 @@ | ||
| 4 | 4 | * issues — a pull is an issue with a diff. |
| 5 | 5 | */ |
| 6 | 6 | |
| 7 | import { addComment as addIssueComment } from "../core/issues.ts"; | |
| 8 | import { leftNote, unticked } from "../core/done_on_main.ts"; | |
| 7 | 9 | import { resolveOpen } from "../core/decisions.ts"; |
| 8 | 10 | import type { App } from "../app.ts"; |
| 9 | 11 | import { landVision } from "../crew/vision.ts"; |
| @@ -449,6 +451,24 @@ export async function mergePull( | ||
| 449 | 451 | const issue = getIssue(app.db, repo.id, n); |
| 450 | 452 | if (!issue || issue.kind !== "issue" || issue.state !== "open") continue; |
| 451 | 453 | if (isStructural(plan, issue.id)) continue; |
| 454 | const left = unticked(issue.body); | |
| 455 | if (left.length > 0) { | |
| 456 | const body = leftNote(issue.number, left); | |
| 457 | const noted = addIssueComment(app.db, { | |
| 458 | issueId: pull.id, | |
| 459 | authorId: writer.value.id, | |
| 460 | body, | |
| 461 | }); | |
| 462 | if (noted.ok) { | |
| 463 | app.bus.append({ | |
| 464 | kind: "pull.commented", | |
| 465 | repoId: repo.id, | |
| 466 | actorId: writer.value.id, | |
| 467 | payload: { number: pull.number, body, commentId: noted.value.id }, | |
| 468 | }); | |
| 469 | } | |
| 470 | continue; | |
| 471 | } | |
| 452 | 472 | const closed = setIssueStateCore(app.db, issue.id, "closed"); |
| 453 | 473 | if (!closed) continue; |
| 454 | 474 | app.bus.append({ |
src/core/done_on_main.ts +30
| @@ -74,3 +74,33 @@ export function alreadyDone( | ||
| 74 | 74 | } |
| 75 | 75 | return out; |
| 76 | 76 | } |
| 77 | ||
| 78 | /** | |
| 79 | * A person reopened this issue: its newest state event is a reopen by | |
| 80 | * someone who is not an agent. That stands until a person closes it, or a | |
| 81 | * pull lands for it; nothing the crew notes about the base closes it. | |
| 82 | */ | |
| 83 | export function reopenedByPerson(db: Database, repoId: string, number: number): boolean { | |
| 84 | const row = db | |
| 85 | .query<{ kind: string; agent: number | null; person: string | null }, [string, number]>( | |
| 86 | `SELECT e.kind AS kind, u.is_agent AS agent, u.id AS person FROM event e LEFT JOIN user u ON u.id = e.actor_id | |
| 87 | WHERE e.repo_id = ?1 AND e.kind IN ('issue.closed', 'issue.reopened') | |
| 88 | AND json_extract(e.payload, '$.number') = ?2 | |
| 89 | ORDER BY e.id DESC LIMIT 1`, | |
| 90 | ) | |
| 91 | .get(repoId, number); | |
| 92 | return row !== null && row.kind === "issue.reopened" && row.person !== null && row.agent !== 1; | |
| 93 | } | |
Null actor (LEFT JOIN miss) is treated as a person; require u.id IS NOT NULL. |
||
| 94 | ||
| 95 | /** The unticked `- [ ]` items of an issue body, as written; fenced code is not a list. */ | |
| 96 | export function unticked(body: string): string[] { | |
| 97 | const open = body.replace(/^[ \t]*(`{3,}|~{3,})[^\n]*\n[\s\S]*?(?:^[ \t]*\1[`~]*[ \t]*$|(?![\s\S]))/gm, ""); | |
| 98 | return [...open.matchAll(/^[ \t]*[-*+][ \t]+\[ \][ \t]+(.+)$/gm)].map((m) => m[1]!.trim()); | |
outdated
Regex reads checkboxes inside code fences; strip fenced blocks first. |
||
| 99 | } | |
| 100 | ||
| 101 | /** The one line a pull carries when an issue it names keeps work left. */ | |
| 102 | export function leftNote(number: number, left: readonly string[]): string { | |
| 103 | return `Left #${number} open: ${left.length} unticked item${left.length === 1 ? "" : "s"} remain (${left | |
| 104 | .slice(0, 3) | |
| 105 | .join("; ")}${left.length > 3 ? "; …" : ""}).`; | |
| 106 | } |
src/crew/autonomy.ts +9 −1
| @@ -35,7 +35,7 @@ import { roadmapIssues, shareWords, slotOrder, smallCap, smallIssues, smallOfIds | ||
| 35 | 35 | import { raisedAt } from "../core/thought_links.ts"; |
| 36 | 36 | import { readConflictLane, readConflictRoom } from "./conflict_room.ts"; |
| 37 | 37 | import { mergeable, resolveOid, treeOf } from "../git/merge.ts"; |
| 38 | import { CLOSED_NOTE } from "../core/done_on_main.ts"; | |
| 38 | import { CLOSED_NOTE, leftNote, reopenedByPerson, unticked } from "../core/done_on_main.ts"; | |
| 39 | 39 | import { mergePull } from "../actions/pulls.ts"; |
| 40 | 40 | import { carriesUpstream } from "../actions/upstream_follow.ts"; |
| 41 | 41 | import { |
| @@ -1555,6 +1555,14 @@ export function closeSuperseded( | ||
| 1555 | 1555 | for (const number of closesFromBody(pull.body)) { |
| 1556 | 1556 | const about = getIssue(app.db, repo.id, number); |
| 1557 | 1557 | if (!about || about.state !== "open") continue; |
| 1558 | // A superseded pull landed nothing: a person's reopen stands against it, | |
| 1559 | // and so does work still unticked. | |
| 1560 | if (reopenedByPerson(app.db, repo.id, number)) continue; | |
| 1561 | const left = unticked(about.body); | |
| 1562 | if (left.length > 0) { | |
| 1563 | addComment(app, by, repo, issue, leftNote(number, left)); | |
| 1564 | continue; | |
| 1565 | } | |
| 1558 | 1566 | if (about.assigneeId !== null) assignIssue(app, by, repo, about, null); |
| 1559 | 1567 | addComment(app, by, repo, about, `${CLOSED_NOTE}${named} (pull #${pull.number}).`); |
| 1560 | 1568 | setIssueState(app, by, repo, about, "closed", { reason: "done" }); |
src/crew/livetriage.ts +2 −1
| @@ -31,7 +31,7 @@ | ||
| 31 | 31 | import type { App } from "../app.ts"; |
| 32 | 32 | import { linkIssue } from "../actions/dependencies.ts"; |
| 33 | 33 | import { addComment, setIssueState } from "../actions/issues.ts"; |
| 34 | import { alreadyDone, CLOSED_NOTE, DONE_LIKE } from "../core/done_on_main.ts"; | |
| 34 | import { alreadyDone, CLOSED_NOTE, DONE_LIKE, reopenedByPerson } from "../core/done_on_main.ts"; | |
| 35 | 35 | import { getIssueById, getIssuesByIds, type Issue } from "../core/issues.ts"; |
| 36 | 36 | import { addDependency, removeDependency } from "../core/dependencies.ts"; |
| 37 | 37 | import { isStructural, pillarsOf, servedBy, type Pillar, type Pillars } from "../core/pillars.ts"; |
| @@ -405,6 +405,7 @@ export async function liveTriage( | ||
| 405 | 405 | if (did.closed >= MOST_CHANGES) break; |
| 406 | 406 | const issue = getIssueById(app.db, id); |
| 407 | 407 | if (!issue || issue.state !== "open") continue; |
| 408 | if (reopenedByPerson(app.db, repo.id, issue.number)) continue; | |
| 408 | 409 | const there = await onBase(dir, repo.defaultBranch, oids); |
| 409 | 410 | if (there.length !== oids.length) continue; |
| 410 | 411 | try { |
test/reopen-stands.test.ts added +163
| @@ -0,0 +1,163 @@ | ||
| 1 | /** | |
| 2 | * #8375 (issue #8373): a person's reopen stands against "already done on main", and a | |
| 3 | * landing never closes an issue that still has unticked items. | |
| 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 { addComment, openIssue, setIssueState } from "../src/actions/issues.ts"; | |
| 11 | import { mergePull, openPull } from "../src/actions/pulls.ts"; | |
| 12 | import { commitToBranch, createRepo, updateRepoSettings } from "../src/actions/repos.ts"; | |
| 13 | import { getIssue, getIssueById, listComments, type Issue } from "../src/core/issues.ts"; | |
| 14 | import { getPull } from "../src/core/pulls.ts"; | |
| 15 | import { getRepoById, updateRepoSettings as setRepo, type Repo } from "../src/core/repos.ts"; | |
| 16 | import { createUser, type User } from "../src/core/users.ts"; | |
| 17 | import { ALREADY_NOTE } from "../src/crew/already.ts"; | |
| 18 | import { closeSuperseded } from "../src/crew/autonomy.ts"; | |
| 19 | import { liveTriage } from "../src/crew/livetriage.ts"; | |
| 20 | import { reopenedByPerson, unticked } from "../src/core/done_on_main.ts"; | |
| 21 | ||
| 22 | describe("a reopen stands; unfinished work stays open", () => { | |
| 23 | let dir: string; | |
| 24 | let app: App; | |
| 25 | let alice: User; | |
| 26 | let repo: Repo; | |
| 27 | let oid: string; | |
| 28 | const fresh = () => getRepoById(app.db, repo.id)!; | |
| 29 | const issue = (title: string, body = ""): Issue => { | |
| 30 | const opened = openIssue(app, alice, fresh(), { title, body }); | |
| 31 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 32 | return opened.value; | |
| 33 | }; | |
| 34 | const state = (on: Issue) => getIssueById(app.db, on.id)!.state; | |
| 35 | const pullFor = async (branch: string, body: string) => { | |
| 36 | const pushed = await commitToBranch(app, alice, fresh(), { | |
| 37 | branch, | |
| 38 | message: branch, | |
| 39 | files: [{ path: `${branch}.txt`, content: "x\n" }], | |
| 40 | }); | |
| 41 | if (!pushed.ok) throw new Error(JSON.stringify(pushed.error)); | |
| 42 | const pull = await openPull(app, alice, fresh(), { | |
| 43 | title: branch, | |
| 44 | body, | |
| 45 | baseBranch: "main", | |
| 46 | headBranch: branch, | |
| 47 | }); | |
| 48 | if (!pull.ok) throw new Error(String(pull.error)); | |
| 49 | return getPull(app.db, repo.id, pull.value.number)!; | |
| 50 | }; | |
| 51 | ||
| 52 | beforeAll(async () => { | |
| 53 | dir = await mkdtemp(join(tmpdir(), "forge-reopen-stands-")); | |
| 54 | app = createApp({ dataDir: dir, port: 0 }); | |
| 55 | const a = await createUser(app.db, { | |
| 56 | origin: "seed", | |
| 57 | trusted: true, | |
| 58 | username: "alice", | |
| 59 | email: "[email protected]", | |
| 60 | password: "a-good-passphrase", | |
| 61 | adminIfFirst: true, | |
| 62 | }); | |
| 63 | if (!a.ok) throw new Error(a.error); | |
| 64 | alice = a.value; | |
| 65 | const made = await createRepo(app, alice, { name: "engine", visibility: "private" }); | |
| 66 | if (!made.ok) throw new Error(String(made.error)); | |
| 67 | repo = made.value; | |
| 68 | const set = await updateRepoSettings(app, alice, repo, { | |
| 69 | autonomy: "autonomous", | |
| 70 | autonomyDirection: "Keep it small.", | |
| 71 | autonomyCeilingUsd: 100, | |
| 72 | }); | |
| 73 | if (!set.ok) throw new Error(JSON.stringify(set.error)); | |
| 74 | setRepo(app.db, fresh(), { autonomyBy: alice.id }); | |
| 75 | const seeded = await commitToBranch(app, alice, fresh(), { | |
| 76 | message: "Start", | |
| 77 | files: [{ path: "README.md", content: "# engine\n" }], | |
| 78 | }); | |
| 79 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 80 | oid = seeded.value.commitOid; | |
| 81 | }, 30_000); | |
| 82 | ||
| 83 | afterAll(async () => { | |
| 84 | app.close(); | |
| 85 | await rm(dir, { recursive: true, force: true }); | |
| 86 | }); | |
| 87 | ||
| 88 | test("unticked items are read from the body", () => { | |
| 89 | expect(unticked("- [x] a\n- [ ] b\n* [ ] c\nnot - [ ] d")).toEqual(["b", "c"]); | |
| 90 | }); | |
| 91 | ||
| 92 | test("fenced code and quoted templates are not unticked items", () => { | |
| 93 | expect(unticked("- [x] a\n```md\n- [ ] in fence\n```\n- [ ] real\n~~~\n- [ ] tilde\n~~~")).toEqual(["real"]); | |
| 94 | expect(unticked("```\n- [ ] never closed")).toEqual([]); | |
| 95 | }); | |
| 96 | ||
| 97 | test("a reopen with no actor is not a person's", async () => { | |
| 98 | const orphan = issue("Reopened by nobody"); | |
| 99 | setIssueState(app, alice, fresh(), orphan, "closed"); | |
| 100 | app.bus.append({ | |
| 101 | kind: "issue.reopened", | |
| 102 | repoId: repo.id, | |
| 103 | payload: { number: orphan.number }, | |
| 104 | }); | |
| 105 | expect(reopenedByPerson(app.db, repo.id, orphan.number)).toBe(false); | |
| 106 | addComment(app, alice, fresh(), orphan, `${ALREADY_NOTE}${oid}`); | |
| 107 | const pull = await pullFor("superseded-null", `Closes #${orphan.number}`); | |
| 108 | expect(closeSuperseded(app, fresh(), pull, [oid])).toBe(true); | |
| 109 | expect(state(orphan)).toBe("closed"); | |
| 110 | }); | |
| 111 | ||
| 112 | test("live triage leaves a reopened issue open even with no earlier close note", async () => { | |
| 113 | const reopened = issue("Reopened by hand"); | |
| 114 | expect(setIssueState(app, alice, fresh(), reopened, "closed").ok).toBe(true); | |
| 115 | expect(setIssueState(app, alice, fresh(), getIssueById(app.db, reopened.id)!, "open").ok).toBe(true); | |
| 116 | addComment(app, alice, fresh(), reopened, `${ALREADY_NOTE}${oid}`); | |
| 117 | expect((await liveTriage(app, fresh())).closed).toBe(0); | |
| 118 | expect(state(reopened)).toBe("open"); | |
| 119 | }); | |
| 120 | ||
| 121 | test("a superseded pull does not re-close a reopened issue (the #8042 case)", async () => { | |
| 122 | const asked = issue("Reopened with work left"); | |
| 123 | setIssueState(app, alice, fresh(), asked, "closed"); | |
| 124 | setIssueState(app, alice, fresh(), getIssueById(app.db, asked.id)!, "open"); | |
| 125 | const pull = await pullFor("superseded-a", `Closes #${asked.number}`); | |
| 126 | expect(closeSuperseded(app, fresh(), pull, [oid])).toBe(true); | |
| 127 | expect(state(asked)).toBe("open"); | |
| 128 | expect(listComments(app.db, asked.id).some((c) => c.body.startsWith("Closed: already done"))).toBe(false); | |
| 129 | }); | |
| 130 | ||
| 131 | test("a superseded pull still closes an issue nobody reopened", async () => { | |
| 132 | const plain = issue("Plain"); | |
| 133 | const pull = await pullFor("superseded-b", `Closes #${plain.number}`); | |
| 134 | expect(closeSuperseded(app, fresh(), pull, [oid])).toBe(true); | |
| 135 | expect(state(plain)).toBe("closed"); | |
| 136 | }); | |
| 137 | ||
| 138 | test("a landing leaves an issue with unticked items open and says so on the pull", async () => { | |
| 139 | const partial = issue("Partial", "- [x] one\n- [ ] two\n- [ ] three"); | |
| 140 | const whole = issue("Whole", "- [x] one"); | |
| 141 | const pull = await pullFor("lands", `Closes #${partial.number}, closes #${whole.number}`); | |
| 142 | const merged = await mergePull(app, alice, fresh(), pull, "squash"); | |
| 143 | expect(merged.ok).toBe(true); | |
| 144 | expect(state(partial)).toBe("open"); | |
| 145 | expect(state(whole)).toBe("closed"); | |
| 146 | const lines = listComments(app.db, getIssue(app.db, repo.id, pull.number)!.id).map((c) => c.body); | |
| 147 | const events = app.db | |
| 148 | .query<{ payload: string }, [string]>("SELECT payload FROM event WHERE kind = 'pull.commented' AND repo_id = ?1") | |
| 149 | .all(repo.id) | |
| 150 | .map((e) => JSON.parse(e.payload).body); | |
| 151 | expect(events).toContain(lines[0]!); | |
| 152 | expect(lines).toEqual([`Left #${partial.number} open: 2 unticked items remain (two; three).`]); | |
| 153 | }); | |
| 154 | ||
| 155 | test("a pull that lands after a reopen closes it", async () => { | |
| 156 | const back = issue("Back again"); | |
| 157 | setIssueState(app, alice, fresh(), back, "closed"); | |
| 158 | setIssueState(app, alice, fresh(), getIssueById(app.db, back.id)!, "open"); | |
| 159 | const pull = await pullFor("lands-2", `Closes #${back.number}`); | |
| 160 | expect((await mergePull(app, alice, fresh(), pull, "squash")).ok).toBe(true); | |
| 161 | expect(state(back)).toBe("closed"); | |
| 162 | }); | |
| 163 | }); |
new above
offline
Core addComment emits no event: the note is not live and notifies nobody. Use the action-layer path or append issue.commented.