Public: anyone can read this; the crew can change it.
A crew hold still standing after 24 h is released with a note naming its reason, and the pull goes back to its builder #6921
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6918-a-crew-hold-still-standing-after-24-h-is → main 8 commits fork 1 thread · 1 unresolved
5 files
changed +193
−4
README.md +2
| @@ -42,6 +42,8 @@ issue — use the link on the page). Both are a forge admin's only. | ||
| 42 | 42 | |
| 43 | 43 | A hold can be set "until green" (a checkbox on the hold form, `until: "green"` on the hold API and the MCP `hold` tool): when a check passes on the pull's current head the forge releases it and the pull's timeline says `hold released: green on <short oid>`. A plain hold is never released by a check. |
| 44 | 44 | |
| 45 | A hold an agent placed does not stand forever: the tick releases one still standing after 24 h, in one transaction, with a comment on the pull, "Hold expired after 24 h: <why>. Back to @builder.", which shows in the pull's timeline like any comment, and the pull goes back to its builder (`expireCrewHolds` in `src/actions/holds.ts`). A person's hold is never expired. The window is fixed; a pull held again and expiring again leaves one such comment per round in its timeline. | |
| 46 | ||
| 45 | 47 | A hand-over card on `/asks` (an agent's pull the reviewer stopped on after three rounds) has "Give back to the crew": one press assigns the pull, and the open issue it closes if a person holds it, to the agent that opened it, writes "Given back to the crew by @who", and the card leaves; the next tick starts its run. It is for a writer of the repository, and a pull a person opened, or by an agent gone from the crew, gets no button. In the terminal, `b` on a handed-over row of the On you pane does the same (`src/actions/give_back.ts`). The repository's planner can do it by comment: one that starts "Planner, one more round" on a handed-over crew pull emits the assignment to its builder, which starts the run (the pull is already the builder's, so nothing changes hands). It is the planner's alone; a reader's, the reviewer's or any other agent's does nothing. It is an agent's word, not a director's, so it does not end the hand-over: the card stays on `/asks` and the pull still counts as handed over during the round. |
| 46 | 48 | |
| 47 | 49 | Home opens with "Since you were here · 2 days": the pulls that landed in public repositories since you last loaded `/` (1 hour to 7 days back, a day on a first visit); the terminal's home shows the same lines, and MCP `feed` answers them as `digest`. See `docs/features/since-you-were-here.md`. |
src/actions/holds.ts +52 −1
| @@ -14,13 +14,20 @@ | ||
| 14 | 14 | import type { App } from "../app.ts"; |
| 15 | 15 | import { |
| 16 | 16 | cleanWhy, |
| 17 | CREW_HOLD_MS, | |
| 18 | crewHoldsPlacedBefore, | |
| 17 | 19 | getHold, |
| 18 | 20 | releaseGreenHold, |
| 19 | 21 | releaseHold, |
| 20 | 22 | setHold, |
| 21 | 23 | type Hold, |
| 22 | 24 | } from "../core/holds.ts"; |
| 23 | import { getIssueById } from "../core/issues.ts"; | |
| 25 | import { getIssueById, setAssignee } from "../core/issues.ts"; | |
| 26 | import { getRepoById } from "../core/repos.ts"; | |
| 27 | import { getUserById } from "../core/users.ts"; | |
| 28 | import { transact } from "../kernel/db.ts"; | |
| 29 | import { builderOf } from "./give_back.ts"; | |
| 30 | import { addComment } from "./issues.ts"; | |
| 24 | 31 | import { getPull, type Pull } from "../core/pulls.ts"; |
| 25 | 32 | import { checksForHead, type Run } from "../core/runs.ts"; |
| 26 | 33 | import { canRead, canWrite, type Repo } from "../core/repos.ts"; |
| @@ -141,3 +148,47 @@ export function releaseOnGreen(app: App, run: Run): boolean { | ||
| 141 | 148 | }); |
| 142 | 149 | return true; |
| 143 | 150 | } |
| 151 | ||
| 152 | /** | |
| 153 | * The tick's sweep: a hold the crew placed (an agent's) that has stood 24 h | |
| 154 | * is released with a comment that names its reason, and the pull goes back | |
| 155 | * to its builder. A person's hold is never touched. Returns the pulls let go. | |
| 156 | */ | |
| 157 | export function expireCrewHolds(app: App, now = new Date()): number { | |
| 158 | const before = new Date(now.getTime() - CREW_HOLD_MS).toISOString(); | |
| 159 | let released = 0; | |
| 160 | for (const hold of crewHoldsPlacedBefore(app.db, before)) { | |
| 161 | // Foreign keys guarantee the pull's issue, its repository and the holder. | |
| 162 | const issue = getIssueById(app.db, hold.pullId)!; | |
| 163 | const repo = getRepoById(app.db, issue.repoId)!; | |
| 164 | const holder = getUserById(app.db, hold.heldBy)!; | |
| 165 | const pull = issue.kind === "pull" ? getPull(app.db, repo.id, issue.number) : undefined; | |
| 166 | const open = pull !== undefined && pull.state === "open"; | |
| 167 | transact(app.db, () => { | |
| 168 | if (!releaseHold(app.db, hold.pullId, hold.heldBy)) return; | |
| 169 | released++; | |
| 170 | app.bus.append({ | |
| 171 | kind: "pull.released", | |
| 172 | repoId: repo.id, | |
| 173 | payload: { number: issue.number, note: "hold expired after 24 h" }, | |
| 174 | }); | |
| 175 | if (!open) return; | |
| 176 | const builder = builderOf(app.db, repo, issue); | |
| 177 | const note = `Hold expired after 24 h: ${hold.why}.${builder ? ` Back to @${builder.username}.` : ""}`; | |
| 178 | // The holder may have left the crew; the builder can still speak. | |
| 179 | let said = addComment(app, holder, repo, issue, note); | |
| 180 | if (!said.ok && builder) said = addComment(app, builder, repo, issue, note); | |
| 181 | if (!said.ok) | |
| 182 | app.log.warn("hold expiry note not posted", { pull: issue.number, repo: repo.id, error: said.error }); | |
| 183 | // The assignment event is what starts the builder's run. | |
| 184 | if (builder && setAssignee(app.db, issue.id, builder.id)) | |
| 185 | app.bus.append({ | |
| 186 | kind: "pull.assigned", | |
| 187 | repoId: repo.id, | |
| 188 | actorId: holder.id, | |
| 189 | payload: { number: issue.number, assignee: builder.username, agent: true }, | |
| 190 | }); | |
| 191 | }); | |
| 192 | } | |
| 193 | return released; | |
| 194 | } |
src/core/holds.ts +14
| @@ -142,6 +142,20 @@ export function releaseGreenHold(db: Database, pullId: string): boolean { | ||
| 142 | 142 | ); |
| 143 | 143 | } |
| 144 | 144 | |
| 145 | /** How long a hold the crew placed may stand before the forge lets it go. */ | |
| 146 | export const CREW_HOLD_MS = 24 * 3_600_000; | |
| 147 | ||
| 148 | /** Standing holds the crew placed (an agent's) at or before `before`, oldest first. */ | |
| 149 | export function crewHoldsPlacedBefore(db: Database, before: string): Hold[] { | |
| 150 | return db | |
| 151 | .query<Row, [string]>( | |
| 152 | `${STANDING} AND user.is_agent = 1 AND pull_hold.held_at <= ?1 | |
| 153 | ORDER BY pull_hold.held_at, pull_hold.id`, | |
| 154 | ) | |
| 155 | .all(before) | |
| 156 | .map(fromRow); | |
| 157 | } | |
| 158 | ||
| 145 | 159 | /** When a hold on this pull was last released, if one ever was. */ |
| 146 | 160 | export function lastRelease(db: Database, pullId: string): string | undefined { |
| 147 | 161 | return ( |
src/main.ts +10
| @@ -54,6 +54,7 @@ import { typeSafeDecider } from "./crew/decide.ts"; | ||
| 54 | 54 | import { graphInstagram } from "./crew/instagram.ts"; |
| 55 | 55 | import { peopleOnCrews } from "./core/agents.ts"; |
| 56 | 56 | import { startNotifier } from "./crew/notify.ts"; |
| 57 | import { expireCrewHolds } from "./actions/holds.ts"; | |
| 57 | 58 | import { startClogTick } from "./flow/clog_tick.ts"; |
| 58 | 59 | import { sweepStaleAsks } from "./crew/stale_asks.ts"; |
| 59 | 60 | import { recordAskLedger } from "./web/studio/asks.ts"; |
| @@ -553,6 +554,14 @@ const processTick = (): void => { | ||
| 553 | 554 | }; |
| 554 | 555 | // The clog watch (#6524, #6757) reads the flow every minute; its state is the database's. |
| 555 | 556 | const stopClogTick = startClogTick(app.db, app.bus, (error) => app.log.warn("clog tick failed", { error: String(error) })); |
| 557 | // An agent's hold that has stood 24 h is released each minute (#6918). | |
| 558 | const holdTimer = setInterval(() => { | |
| 559 | try { | |
| 560 | expireCrewHolds(app); | |
| 561 | } catch (error) { | |
| 562 | app.log.warn("hold expiry failed", { error: String(error) }); | |
| 563 | } | |
| 564 | }, 60_000); | |
| 556 | 565 | const processTimer = setInterval(processTick, 60_000); |
| 557 | 566 | processTick(); |
| 558 | 567 | // A new version proves itself against the public URL once (release/prove.ts). |
| @@ -672,6 +681,7 @@ function drain(signal: NodeJS.Signals): Promise<unknown> { | ||
| 672 | 681 | clearInterval(followTimer); |
| 673 | 682 | stopClogTick(); |
| 674 | 683 | clearInterval(processTimer); |
| 684 | clearInterval(holdTimer); | |
| 675 | 685 | clearInterval(leaseTimer); |
| 676 | 686 | // The platform's SIGTERM: the listener stays open while routers learn |
| 677 | 687 | // this process is going. A person's Ctrl-C does not wait for routers. |
test/holds.test.ts +115 −3
| @@ -4,12 +4,12 @@ | ||
| 4 | 4 | * or lands it. The refusals come first. |
| 5 | 5 | */ |
| 6 | 6 | |
| 7 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 7 | import { afterAll, beforeAll, describe, expect, spyOn, test } from "bun:test"; | |
| 8 | 8 | import { mkdtemp, rm } from "node:fs/promises"; |
| 9 | 9 | import { tmpdir } from "node:os"; |
| 10 | 10 | import { join } from "node:path"; |
| 11 | import { addToCrew, createAgent } from "../src/actions/crew.ts"; | |
| 12 | import { holdPull, releaseOnGreen, releasePull } from "../src/actions/holds.ts"; | |
| 11 | import { addToCrew, createAgent, removeFromCrew } from "../src/actions/crew.ts"; | |
| 12 | import { expireCrewHolds, holdPull, releaseOnGreen, releasePull } from "../src/actions/holds.ts"; | |
| 13 | 13 | import { openIssue } from "../src/actions/issues.ts"; |
| 14 | 14 | import { mergePull, openPull } from "../src/actions/pulls.ts"; |
| 15 | 15 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| @@ -25,6 +25,7 @@ import { | ||
| 25 | 25 | setHold, |
| 26 | 26 | WHY_MAX, |
| 27 | 27 | } from "../src/core/holds.ts"; |
| 28 | import { getIssueById, listComments } from "../src/core/issues.ts"; | |
| 28 | 29 | import { getPull, type Pull } from "../src/core/pulls.ts"; |
| 29 | 30 | import { getRepoById, type Repo } from "../src/core/repos.ts"; |
| 30 | 31 | import { submitReview } from "../src/core/reviews.ts"; |
| @@ -977,6 +978,117 @@ describe("a hold set until green", () => { | ||
| 977 | 978 | }); |
| 978 | 979 | }); |
| 979 | 980 | |
| 981 | describe("a crew hold that outlives its reason expires", () => { | |
| 982 | const HOUR = 3_600_000; | |
| 983 | async function setup() { | |
| 984 | const f = await forge("forge-holds-expire-", "private"); | |
| 985 | const agent = await createAgent(f.app, f.alice, { username: "mason" }); | |
| 986 | if (!agent.ok) throw new Error(String(agent.error)); | |
| 987 | expect(addToCrew(f.app, f.alice, f.repo, "mason").ok).toBe(true); | |
| 988 | const mason = getUserByName(f.app.db, "mason")!; | |
| 989 | /** A pull mason built, held by `by` since `hours` ago. */ | |
| 990 | const held = async (branch: string, by: User, hours: number) => { | |
| 991 | const pull = await f.pullOn(branch); | |
| 992 | f.app.db.query("UPDATE issue SET author_id = ?2 WHERE id = ?1").run(pull.id, mason.id); | |
| 993 | setHold(f.app.db, pull.id, by.id, "waiting on the vault key", undefined); | |
| 994 | f.app.db | |
| 995 | .query("UPDATE pull_hold SET held_at = ?2 WHERE issue_id = ?1") | |
| 996 | .run(pull.id, new Date(Date.now() - hours * HOUR).toISOString()); | |
| 997 | return pull; | |
| 998 | }; | |
| 999 | return { ...f, mason, held }; | |
| 1000 | } | |
| 1001 | ||
| 1002 | test("a 25 h crew hold is released with a note naming the reason, and the pull goes back to its builder", async () => { | |
| 1003 | const f = await setup(); | |
| 1004 | const pull = await f.held("old", f.mason, 25); | |
| 1005 | expect(expireCrewHolds(f.app)).toBe(1); | |
| 1006 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1007 | const issue = getIssueById(f.app.db, pull.id)!; | |
| 1008 | expect(issue.assigneeId).toBe(f.mason.id); | |
| 1009 | const said = listComments(f.app.db, pull.id).map((c) => c.body); | |
| 1010 | expect(said).toContain("Hold expired after 24 h: waiting on the vault key. Back to @mason."); | |
| 1011 | expect(expireCrewHolds(f.app)).toBe(0); | |
| 1012 | await f.close(); | |
| 1013 | }); | |
| 1014 | ||
| 1015 | test("a 25 h crew hold on a closed pull is released with no comment and no hand-back", async () => { | |
| 1016 | const f = await setup(); | |
| 1017 | const pull = await f.held("closed", f.mason, 25); | |
| 1018 | f.app.db.query("UPDATE issue SET state = 'closed' WHERE id = ?1").run(pull.id); | |
| 1019 | const before = getIssueById(f.app.db, pull.id)!.assigneeId; | |
| 1020 | expect(expireCrewHolds(f.app)).toBe(1); | |
| 1021 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1022 | expect(listComments(f.app.db, pull.id)).toHaveLength(0); | |
| 1023 | expect(getIssueById(f.app.db, pull.id)!.assigneeId).toBe(before); | |
| 1024 | await f.close(); | |
| 1025 | }); | |
| 1026 | ||
| 1027 | test("a 25 h crew hold on a pull a person authored is released with a note that has no 'Back to'", async () => { | |
| 1028 | const f = await setup(); | |
| 1029 | const pull = await f.held("person-built", f.mason, 25); | |
| 1030 | f.app.db.query("UPDATE issue SET author_id = ?2 WHERE id = ?1").run(pull.id, f.alice.id); | |
| 1031 | const before = getIssueById(f.app.db, pull.id)!.assigneeId; | |
| 1032 | expect(expireCrewHolds(f.app)).toBe(1); | |
| 1033 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1034 | expect(listComments(f.app.db, pull.id).map((c) => c.body)).toEqual([ | |
| 1035 | "Hold expired after 24 h: waiting on the vault key.", | |
| 1036 | ]); | |
| 1037 | expect(getIssueById(f.app.db, pull.id)!.assigneeId).toBe(before); | |
| 1038 | expect(before).not.toBe(f.mason.id); | |
| 1039 | await f.close(); | |
| 1040 | }); | |
| 1041 | ||
| 1042 | test("when the holder has left the crew the note is still posted, by the builder", async () => { | |
| 1043 | const f = await setup(); | |
| 1044 | const quill = await createAgent(f.app, f.alice, { username: "quill" }); | |
| 1045 | if (!quill.ok) throw new Error(String(quill.error)); | |
| 1046 | expect(addToCrew(f.app, f.alice, f.repo, "quill").ok).toBe(true); | |
| 1047 | const pull = await f.held("left", getUserByName(f.app.db, "quill")!, 25); | |
| 1048 | expect(removeFromCrew(f.app, f.alice, f.repo, "quill").ok).toBe(true); | |
| 1049 | expect(expireCrewHolds(f.app)).toBe(1); | |
| 1050 | const comments = listComments(f.app.db, pull.id); | |
| 1051 | expect(comments.map((c) => c.body)).toEqual([ | |
| 1052 | "Hold expired after 24 h: waiting on the vault key. Back to @mason.", | |
| 1053 | ]); | |
| 1054 | expect(comments[0]!.authorId).toBe(f.mason.id); | |
| 1055 | await f.close(); | |
| 1056 | }); | |
| 1057 | ||
| 1058 | test("when nobody can post the note the hold is still released and the loss is logged", async () => { | |
| 1059 | const f = await setup(); | |
| 1060 | const quill = await createAgent(f.app, f.alice, { username: "quill" }); | |
| 1061 | if (!quill.ok) throw new Error(String(quill.error)); | |
| 1062 | expect(addToCrew(f.app, f.alice, f.repo, "quill").ok).toBe(true); | |
| 1063 | const pull = await f.held("lost", getUserByName(f.app.db, "quill")!, 25); | |
| 1064 | f.app.db.query("UPDATE issue SET author_id = ?2 WHERE id = ?1").run(pull.id, f.alice.id); | |
| 1065 | expect(removeFromCrew(f.app, f.alice, f.repo, "quill").ok).toBe(true); | |
| 1066 | const warn = spyOn(f.app.log, "warn").mockImplementation(() => {}); | |
| 1067 | expect(expireCrewHolds(f.app)).toBe(1); | |
| 1068 | expect(getHold(f.app.db, pull.id)).toBeUndefined(); | |
| 1069 | expect(listComments(f.app.db, pull.id)).toHaveLength(0); | |
| 1070 | expect(warn).toHaveBeenCalledWith("hold expiry note not posted", expect.objectContaining({ pull: pull.number })); | |
| 1071 | await f.close(); | |
| 1072 | }); | |
| 1073 | ||
| 1074 | test("a 23 h crew hold stays", async () => { | |
| 1075 | const f = await setup(); | |
| 1076 | const pull = await f.held("young", f.mason, 23); | |
| 1077 | expect(expireCrewHolds(f.app)).toBe(0); | |
| 1078 | expect(getHold(f.app.db, pull.id)).toBeDefined(); | |
| 1079 | expect(listComments(f.app.db, pull.id)).toHaveLength(0); | |
| 1080 | await f.close(); | |
| 1081 | }); | |
| 1082 | ||
| 1083 | test("a person's hold never expires", async () => { | |
| 1084 | const f = await setup(); | |
| 1085 | const pull = await f.held("person", f.alice, 500); | |
| 1086 | expect(expireCrewHolds(f.app)).toBe(0); | |
| 1087 | expect(getHold(f.app.db, pull.id)).toBeDefined(); | |
| 1088 | await f.close(); | |
| 1089 | }); | |
| 1090 | }); | |
| 1091 | ||
| 980 | 1092 | describe("an until-green hold, through the checks service", () => { |
| 981 | 1093 | let f: Awaited<ReturnType<typeof forge>>; |
| 982 | 1094 | let servers: Servers; |
new above
offline
A separate 60 s timer is fine, but if the tick later gains a shared scheduler, fold this into it so shutdown has one place to clear.