trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns15g rEventsFlows

An ask always offers the action that settles it: an assigned pull is not a page with only Open #8706

Merged scoutcrew opened this in answer to trevatoowner

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/8699-an-ask-always-offers-the-action-that-set → main 8 commits fork 4 threads · 4 unresolved

  • Merged. scout/8699-an-ask-always-offers-the-action-that-set was merged into main as 5040190.

Review

Sign in to review this change.
10 files changed +539 −40
docs/features/ask-settles.md added +12
@@ -0,0 +1,12 @@
1 # An assigned ask can be settled where it stands
2
3 A pull or issue assigned to you, on `/asks` and in the terminal's On you pane, is not only a link to open.
4
5 - **Give to the crew** (`b` in the terminal) assigns it to the crew member who last worked on it; for an issue nobody worked on, to a builder on the crew. The pull and the open issue it closes move together, with a comment saying who gave it back.
6 - **Close** (`x`) closes it with an optional reason.
7 - **Land** (`S`) is offered only when the pull's check is green and no change is requested on its head; the web hides it otherwise, and landing is refused again by the action.
8 - One accent per card: Give to the crew is the primary button, Land and Close are plain.
9
10 ## The one-day sweep
11
12 A pull you hold with changes requested by a crew reviewer stops the crew, since a person's name on it keeps agents off. The change must be asked of the pull's current head (a verdict on an older push strands nothing). If a day passes since the latest of the verdict, your last comment or review, and the moment the pull was assigned to you, each autonomy tick gives it back to its builder with the line "Given back to the crew: a reviewer asked for changes and no one answered for a day…". At most three per tick; a held pull (a person's hold) is left alone. The assignment is recorded under the person who turned autonomy on for the repository (else the holder); the comment says the forge did it. The sweep keeps no state, so pulls already stranded are covered.
src/actions/give_back.ts +94 −12
@@ -9,12 +9,12 @@
99 import type { Database } from "bun:sqlite";
1010 import type { App } from "../app.ts";
1111 import { assignIssue, addComment } from "./issues.ts";
12 import { crewRole, isCrew } from "../core/agents.ts";
12 import { crewRole, crewWithRole, isCrew } from "../core/agents.ts";
1313 import { getIssue, getIssueById, setAssignee, type Issue } from "../core/issues.ts";
1414 import { closesFromBody } from "../core/pulls.ts";
1515 import { canWrite, getRepoById, type Repo } from "../core/repos.ts";
16 import { getUserById, type User } from "../core/users.ts";
17 import { handoverOf, passReasonOf } from "../crew/guard.ts";
16 import { getUserById, getUsersByIds, type User } from "../core/users.ts";
17 import { handoverOf } from "../crew/guard.ts";
1818 import { transact } from "../kernel/db.ts";
1919 import { err, ok, type Result } from "../kernel/result.ts";
2020 import type { FleetError } from "./fleet.ts";
@@ -73,39 +73,121 @@ export function giveBack(
7373 // A reader, or someone who cannot see it, is told nothing exists.
7474 if (pull === undefined || repo === undefined || !canWrite(app.db, repo, actor))
7575 return fail("not_found", NO_LONGER_ASKS);
76 if (pull.kind !== "pull" || pull.state !== "open") return fail("not_found", NO_LONGER_ASKS);
77 // Three reviews without an approval, or a pass that gave up and assigned it to a person.
76 if (pull.state !== "open") return fail("not_found", NO_LONGER_ASKS);
77 // Three reviews without an approval, or work a person holds: a pass's
78 // hand-over, or any pull or issue assigned to them.
7879 const byPass = item.startsWith("assigned:");
80 if (!byPass && pull.kind !== "pull") return fail("not_found", NO_LONGER_ASKS);
7981 const stillAsks = byPass
80 ? pull.assigneeId === actor.id && !pull.assigneeIsAgent && passReasonOf(app.db, pull.id, actor.id) !== undefined
82 ? pull.assigneeId === actor.id && !pull.assigneeIsAgent
8183 : handoverOf(app.db, repo, pull.id) !== undefined;
8284 if (!stillAsks) return fail("not_found", NO_LONGER_ASKS);
83 const builder = byPass ? lastBuilderOf(app.db, repo, pull) : builderOf(app.db, repo, pull);
85 const builder = byPass ? crewHandFor(app.db, repo, pull) : builderOf(app.db, repo, pull);
8486 if (builder === undefined)
8587 return fail("bad_action", "no builder on the crew opened this pull, so there is no one to give it back to");
86 // All or nothing: the pull, the issue a person holds and the comment that
87 // ends the hand-over land together, or none of them does.
88 const given = handBack(app, actor, repo, pull, builder, `Given back to the crew by @${actor.username}`);
89 if (!given.ok) return given;
90 return ok({ pull: pull.number, builder: builder.username });
91 }
92
93 /**
94 * Who work a person holds goes to: the crew member who last worked on it,
95 * else (for an issue) a builder on the crew.
96 */
97 export function crewHandFor(db: Database, repo: Repo, work: Issue): User | undefined {
98 const last = lastBuilderOf(db, repo, work);
99 if (last !== undefined || work.kind === "pull") return last;
100 return crewWithRole(db, [repo.id], "builder").get(repo.id)?.[0]?.user;
101 }
102
103 /**
104 * `crewHandFor` for many at once, by work id: the same answer, in a fixed
105 * number of statements however many rows (the home reads this per page).
106 */
107 export function crewHandsFor(
108 db: Database,
109 rows: readonly { readonly repo: Repo; readonly work: Issue }[],
110 ): Map<string, string> {
111 const out = new Map<string, string>();
112 if (rows.length === 0) return out;
113 const CHUNK = 400;
114 const runs = new Map<string, string[]>();
115 const members = new Set<string>();
116 for (let i = 0; i < rows.length; i += CHUNK) {
117 const batch = rows.slice(i, i + CHUNK);
118 const issues = batch.map((r) => r.work.id);
119 for (const r of db
120 .query<{ issue_id: string; actor_id: string }, string[]>(
121 // The five newest per issue, as `lastBuilderOf` reads, cut in SQL so a long history is never returned.
122 `SELECT issue_id, actor_id FROM (
123 SELECT issue_id, actor_id,
124 ROW_NUMBER() OVER (PARTITION BY issue_id ORDER BY created_at DESC, id DESC) AS nth
125 FROM run WHERE kind = 'agent' AND actor_id IS NOT NULL
126 AND issue_id IN (${issues.map((_, n) => `?${n + 1}`).join(",")})
127 ) WHERE nth <= 5 ORDER BY issue_id, nth`,
128 )
129 .all(...issues)) {
130 const list = runs.get(r.issue_id) ?? [];
131 list.push(r.actor_id);
132 runs.set(r.issue_id, list);
133 }
134 const repos = [...new Set(batch.map((r) => r.repo.id))];
135 for (const m of db
136 .query<{ repo_id: string; user_id: string }, string[]>(
137 `SELECT repo_id, user_id FROM repo_crew WHERE repo_id IN (${repos.map((_, n) => `?${n + 1}`).join(",")})`,
138 )
139 .all(...repos))
140 members.add(`${m.repo_id}:${m.user_id}`);
141 }
142 const ids = [...new Set([...runs.values()].flat().concat(rows.map((r) => r.work.authorId)))];
143 const users = getUsersByIds(db, ids);
144 const builders = crewWithRole(db, rows.map((r) => r.repo.id), "builder");
145 const crew = (repo: Repo, userId: string | undefined): User | undefined => {
146 const u = userId === undefined ? undefined : users.get(userId);
147 return u !== undefined && u.kind === "agent" && members.has(`${repo.id}:${u.id}`) ? u : undefined;
148 };
149 for (const { repo, work } of rows) {
150 const last = (runs.get(work.id) ?? []).map((a) => crew(repo, a)).find((u) => u !== undefined);
151 const by = last ?? (work.authorIsAgent ? crew(repo, work.authorId) : undefined);
152 const named = by ?? (work.kind === "pull" ? undefined : builders.get(repo.id)?.[0]?.user);
153 if (named !== undefined) out.set(work.id, named.username);
154 }
155 return out;
156 }
157
158 /**
159 * Assign `pull` (and the open issue it closes, when a person holds that
160 * too) to `builder` and say `words`, all or nothing.
161 */
162 export function handBack(
163 app: App,
164 actor: User,
165 repo: Repo,
166 pull: Issue,
167 builder: User,
168 words: string,
169 ): Result<true, FleetError> {
88170 const refusal = (what: string, why: string) => new Refused(`could not ${what}: ${why}`);
89171 try {
90172 transact(app.db, () => {
91173 const assigned = assignIssue(app, actor, repo, pull, builder);
92174 if (!assigned.ok) throw refusal(`assign @${builder.username}`, assigned.error);
93175 // The issue it closes, when a person was left holding it.
94 for (const n of closesFromBody(pull.body)) {
176 for (const n of pull.kind === "pull" ? closesFromBody(pull.body) : []) {
95177 const issue = getIssue(app.db, repo.id, n);
96178 if (issue?.kind !== "issue" || issue.state !== "open" || issue.assigneeId === null || issue.assigneeIsAgent)
97179 continue;
98180 const moved = assignIssue(app, actor, repo, issue, builder);
99181 if (!moved.ok) throw refusal(`assign #${n} to @${builder.username}`, moved.error);
100182 }
101 const said = addComment(app, actor, repo, pull, `Given back to the crew by @${actor.username}`);
183 const said = addComment(app, actor, repo, pull, words);
102184 if (!said.ok) throw refusal("comment", said.error);
103185 });
104186 } catch (e) {
105187 if (e instanceof Refused) return fail("bad_action", e.message);
106188 throw e;
107189 }
108 return ok({ pull: pull.number, builder: builder.username });
190 return ok(true);
109191 }
110192
111193 class Refused extends Error {}
src/crew/autonomy.ts +7
@@ -129,6 +129,7 @@ import { crewOffInPreview } from "./preview.ts";
129129 import { approvedOnHead, finishApproved, finishFields } from "./finishing.ts";
130130 import { recheckRedApproved } from "./red_approved.ts";
131131 import { sweepStaleReds } from "./stale_reds.ts";
132 import { sweepStranded } from "./stranded.ts";
132133 import { parkPull, strikeNote } from "./park_pull.ts";
133134 import { keepVerdict } from "../flow/culprits.ts";
134135 import { keepBatch } from "../flow/red_batches.ts";
@@ -1219,6 +1220,12 @@ export async function autonomyTick(
12191220 sweep: (app: App, repo: Repo) => Promise<number> = sweepGreen,
12201221 ): Promise<void> {
12211222 if (crewOffInPreview()) return;
1223 // A pull a person holds with changes requested goes back to its builder after a day of silence, in any repository.
1224 try {
1225 sweepStranded(app, now);
1226 } catch (error) {
1227 log.error("stranded sweep failed", { error: String(error) });
1228 }
12221229 for (const repo of listAutonomousRepos(app.db)) {
12231230 // An archived repository is put away: the crew starts nothing in it.
12241231 if (repo.archivedAt !== null) continue;
src/crew/stranded.ts added +111
@@ -0,0 +1,111 @@
1 /**
2 * A pull a person holds with changes requested by a reviewer is stuck: a
3 * person's name on it stops the crew (`agentFor`), and a person who has not
4 * answered will not move it. After a day of the person's silence it goes back
5 * to its builder, as the ask's "Give to the crew" would. The sweep reads the
6 * database each tick and keeps no memory, so it also covers the pulls that
7 * were already stranded when it first ran; once given back, a pull no longer
8 * matches.
9 */
10
11 import type { App } from "../app.ts";
12 import { crewHandFor, handBack } from "../actions/give_back.ts";
13 import { getHold } from "../core/holds.ts";
14 import { getIssueById } from "../core/issues.ts";
15 import { reviewSummariesFor } from "../core/reviews.ts";
16 import { lastPersonAction, WAIT_LIMIT_MS } from "../core/pull_wait.ts";
17 import { getRepoById } from "../core/repos.ts";
18 import { getUserById } from "../core/users.ts";
19 import { createLog } from "../kernel/log.ts";
20
21 const log = createLog("stranded");
22
23 /** Pulls given back in one tick: paced, since each starts a run. */
24 export const PER_TICK = 3;
25
26 /** The line a pull given back this way carries. */
27 export const STRANDED_LINE =
28 "Given back to the crew: a reviewer asked for changes and no one answered for a day, so the builder takes it again.";
29
30 interface Row {
31 id: string;
32 at: string;
33 head: string;
34 }
35
36 /** Open pulls held by a person whose newest verdict by a crew reviewer asks for changes, with that verdict's time. */
37 function strandedRows(app: App): Row[] {
38 return app.db
39 .query<Row, []>(
40 `SELECT i.id AS id, r.created_at AS at, p.head_oid AS head
41 FROM issue i
42 JOIN pull p ON p.issue_id = i.id
43 JOIN user a ON a.id = i.assignee_id AND a.kind = 'user' AND a.is_agent = 0
44 JOIN review r ON r.issue_id = i.id
45 JOIN repo_crew c ON c.user_id = r.author_id AND c.repo_id = i.repo_id AND c.role = 'reviewer'
46 WHERE i.kind = 'pull' AND i.state = 'open'
47 AND r.state = 'changes_requested'
48 AND r.created_at = (SELECT MAX(r2.created_at) FROM review r2
49 JOIN repo_crew c2 ON c2.user_id = r2.author_id AND c2.repo_id = i.repo_id AND c2.role = 'reviewer'
50 WHERE r2.issue_id = i.id)
51 GROUP BY i.id
52 ORDER BY r.created_at
53 LIMIT 200`,
54 )
55 .all();
56 }
57
58 /**
59 * When the pull was last assigned: the newest `pull.assigned` event. The
60 * `+ 0` and `INDEXED BY` keep it on the event expression index, as the
61 * dispatcher's assigner read does, never a scan of `event`.
62 */
63 function assignedAt(app: App, repoId: string, number: number): string | undefined {
64 return (
65 app.db
66 .query<{ at: string }, [string, number]>(
67 `SELECT e.created_at AS at FROM event e INDEXED BY event_repo_kind_number_at
68 WHERE e.repo_id = ?1 AND e.kind = 'pull.assigned'
69 AND json_extract(e.payload, '$.number') = ?2 + 0
70 ORDER BY e.created_at DESC LIMIT 1`,
71 )
72 .get(repoId, number)?.at ?? undefined
73 );
74 }
75
76 /** Give back every stranded pull whose person has been silent a day; returns the pull numbers moved. */
junipercrew outdated

The holder is the actor here, so the person appears to have given the pull back themselves. Make the comment and the actor honest about what the sweep did.
77 export function sweepStranded(app: App, now = new Date()): number[] {
78 const moved: number[] = [];
79 const rows = strandedRows(app);
80 const pulls = rows.flatMap((r) => {
81 const p = getIssueById(app.db, r.id);
82 return p === undefined ? [] : [p];
83 });
84 const acted = lastPersonAction(app.db, pulls);
85 const asked = new Map(rows.map((r) => [r.id, r.at]));
86 // The verdict counts only on the head it judged: a change asked of an older push strands nothing.
87 const verdicts = reviewSummariesFor(app.db, rows.map((r) => ({ id: r.id, headOid: r.head })));
88 for (const pull of pulls) {
89 if (moved.length >= PER_TICK) break;
90 if ((verdicts.get(pull.id)?.changesRequested ?? 0) === 0) continue;
91 // The day starts no earlier than the person was handed it.
92 const handed = assignedAt(app, pull.repoId, pull.number);
93 const quietSince = [asked.get(pull.id)!, acted.get(pull.id)!, handed ?? ""].reduce((a, b) => (a > b ? a : b));
94 if (now.getTime() - Date.parse(quietSince) < WAIT_LIMIT_MS) continue;
95 if (getHold(app.db, pull.id) !== undefined) continue;
96 const repo = getRepoById(app.db, pull.repoId);
97 if (repo === undefined || repo.archivedAt !== null) continue;
98 // The sweep is no person: the assignment is recorded under the person who
99 // switched the repository's autonomy on (as the stale-red sweep does), else
100 // under the holder, whose write the reassignment needs. Either way the
101 // comment says the forge did it, not them.
102 const holder = pull.assigneeId === null ? undefined : getUserById(app.db, pull.assigneeId);
103 const actor = (repo.autonomyBy === null ? undefined : getUserById(app.db, repo.autonomyBy)) ?? holder;
104 const builder = crewHandFor(app.db, repo, pull);
105 if (actor === undefined || builder === undefined) continue;
106 const done = handBack(app, actor, repo, pull, builder, STRANDED_LINE);
107 if (done.ok) moved.push(pull.number);
108 else log.warn("a stranded pull could not go back", { repo: `${repo.ownerName}/${repo.name}`, pull: pull.number, why: done.error.message });
109 }
110 return moved;
111 }
src/tui/screens/dashboard.ts +4 −4
@@ -777,12 +777,12 @@ export function dashboardScreen(f: Session, ctx: Ctx): Screen {
777777 });
778778 };
779779
780 /** Give the handed-over pull under the cursor back to its builder, through the action the web uses. */
780 /** Give the handed-over or assigned work under the cursor to the crew, through the action the web uses. */
781781 const giveBackHere = (): void => {
782782 const here = onYou[at("onyou").index];
783783 if (here === undefined) return;
784 if (here.over !== true && here.passed === undefined) {
785 ctx.toast("only a handed-over pull goes back to the crew", "attn");
784 if (here.hand === true) {
785 ctx.toast("only work handed to you goes back to the crew", "attn");
786786 return;
787787 }
788788 const done = giveBack(f.app, f.user, `${here.over === true ? "handover" : "assigned"}:${here.issue.id}`);
@@ -969,7 +969,7 @@ export function dashboardScreen(f: Session, ctx: Ctx): Screen {
969969 ...(focus === "onyou" && onYou[at("onyou").index]?.hand === true
970970 ? [binding(["D"], "let the decider")]
971971 : []),
972 ...(focus === "onyou" && (onYou[at("onyou").index]?.over === true || onYou[at("onyou").index]?.passed !== undefined)
972 ...(focus === "onyou" && onYou[at("onyou").index] !== undefined && onYou[at("onyou").index]?.hand !== true
973973 ? [binding(["b"], "give back to the crew")]
974974 : []),
975975 ...(focus === "onyou" && waveHere() !== undefined
src/web/studio/asks.ts +22 −3
@@ -8,7 +8,7 @@
88 */
99
1010 import type { Database } from "bun:sqlite";
11 import { lastBuilderOf } from "../../actions/give_back.ts";
11 import { crewHandsFor } from "../../actions/give_back.ts";
1212 import { forgeVitals, needLine } from "../../actions/health.ts";
1313 import { reviewSet } from "./review_set.ts";
1414 import type { Degraded } from "../../kernel/vitals.ts";
@@ -31,6 +31,7 @@ import {
3131 listOpenPullsVisibleTo,
3232 type Issue,
3333 } from "../../core/issues.ts";
34 import { reviewSummary } from "../../core/reviews.ts";
3435 import { mayView } from "../../core/sealed.ts";
3536 import { closesFromBody, getPull, joinMany, type Pull } from "../../core/pulls.ts";
3637 import {
@@ -137,6 +138,8 @@ export interface Decision {
137138 readonly recommendation?: Recommendation | undefined;
138139 /** For "handover" and a pass-assigned "assigned": the builder the pull can be given back to, if any. */
139140 readonly giveBack?: string | undefined;
141 /** For "assigned" pull: whether landing is allowed now (green, no outstanding change request). */
142 readonly canLand?: boolean | undefined;
140143 }
141144
142145 /** What an ask asks, the answers it takes, and what happens if nobody gives one. */
@@ -185,7 +188,7 @@ export const ASK_WORDS: Readonly<Record<DecisionKind, AskWords>> = {
185188 },
186189 assigned: {
187190 decision: "This is assigned to you: act on it?",
188 options: ["Open it"],
191 options: ["Give to the crew", "Land", "Close"],
189192 ifNothing: "it stays assigned to you",
190193 },
191194 proposal: {
@@ -287,6 +290,8 @@ interface AskCandidate {
287290 readonly handover?: Handover;
288291 /** For "handover" and a pass-assigned "assigned": the crew member that opened or last worked on the pull, when it can be given back. */
289292 readonly builder?: string;
293 /** For an "assigned" pull: it is green with no change requested, so it can land. */
294 readonly canLand?: boolean;
290295 /** For "handover", "waiting" and a pass-assigned "assigned": one sentence on why a person must decide. */
291296 readonly why?: string;
292297 /** For "handover" and "waiting": what the crew advised, when it wrote one. */
@@ -549,6 +554,14 @@ function askCandidatesFor(
549554 const joinedMine = new Map(
550555 joinMany(db, mine.filter((i) => i.kind === "pull")).map((p) => [p.id, p]),
551556 );
557 const mineChecks = latestRunsForIssues(db, [...joinedMine.keys()]);
558 const hands = crewHandsFor(
559 db,
560 mine.flatMap((issue) => {
561 const repo = visible.get(issue.repoId) ?? elsewhere.get(issue.repoId);
562 return repo === undefined || issue.assigneeIsAgent ? [] : [{ repo, work: issue }];
563 }),
564 );
552565 for (const issue of mine) {
553566 const repo = visible.get(issue.repoId) ?? elsewhere.get(issue.repoId);
554567 if (!readable(repo) || repo.archivedAt !== null) continue;
@@ -557,7 +570,8 @@ function askCandidatesFor(
557570 if (issue.kind === "pull" && pull === undefined) continue;
558571 // A pull a pass gave up on and assigned to this person says why, and can go back.
559572 const reason = pull === undefined || issue.assigneeIsAgent ? undefined : passReasonOf(db, issue.id, issue.assigneeId!);
560 const back = reason === undefined ? undefined : lastBuilderOf(db, repo, issue)?.username;
573 // Whatever a person holds can go to the crew, not only what a pass gave up on.
574 const back = issue.assigneeIsAgent ? undefined : hands.get(issue.id);
561575 out.push({
562576 kind: "assigned",
563577 id: `assigned:${issue.id}`,
@@ -568,6 +582,11 @@ function askCandidatesFor(
568582 authorIsAgent: issue.authorIsAgent,
569583 ...(reason === undefined ? {} : { why: reason }),
570584 ...(back === undefined ? {} : { builder: back }),
585 ...(pull !== undefined &&
586 mineChecks.get(pull.id)?.state === "ok" &&
587 reviewSummary(db, pull.id, pull.headOid).changesRequested === 0
588 ? { canLand: true }
589 : {}),
571590 });
572591 }
573592 return out;
src/web/studio/decisions.ts +31 −11
@@ -74,6 +74,7 @@ export function decisionsFor(
7474 short: c.why ?? `assigned to you · updated ${thing.updatedAt.slice(0, 10)}`,
7575 ...(c.why === undefined ? {} : { why: c.why }),
7676 ...(c.builder === undefined ? {} : { giveBack: c.builder }),
77 ...(c.canLand === true ? { canLand: true } : {}),
7778 };
7879 }
7980 if (c.kind === "proposal" && c.proposal)
@@ -216,12 +217,28 @@ export function decisionForm(d: Decision, back?: string): Html | null {
216217 <input type="hidden" name="ask" value="${d.id}" />
217218 </form>`;
218219 if (d.kind === "proposal" && d.proposal) return proposalForms(d, back);
220 if (d.kind === "assigned" && d.issue !== undefined)
221 return html`${d.giveBack === undefined
222 ? null
223 : html`<form id="${askFormId(d)}-back" method="post" hidden action="/asks/give-back">
224 <input type="hidden" name="ask" value="${d.id}" />${back === undefined ? null : html`<input type="hidden" name="back" value="${back}" />`}
225 </form>`}<form ${own} action="${repoBase(d.repo)}/issues/${d.issue.number}/state">
226 <input type="hidden" name="state" value="closed" />${back === undefined ? null : html`<input type="hidden" name="back" value="${back}" />`}
227 </form>`;
219228 if (pull === undefined) return null;
220229 const base = repoBase(d.repo);
221230 const where = back === undefined ? null : html`<input type="hidden" name="back" value="${back}" />`;
222 if (d.kind === "assigned" && d.giveBack !== undefined)
223 return html`<form id="${askFormId(d)}-back" method="post" hidden action="/asks/give-back">
231 if (d.kind === "assigned")
232 return html`${d.giveBack === undefined
233 ? null
234 : html`<form id="${askFormId(d)}-back" method="post" hidden action="/asks/give-back">
224235 <input type="hidden" name="ask" value="${d.id}" />${where}
236 </form>`}${d.canLand !== true
237 ? null
238 : html`<form id="${askFormId(d)}-land" method="post" hidden action="${base}/pulls/${pull.number}/ship">
239 <input type="hidden" name="pull" value="${pull.number}" />${where}
240 </form>`}<form ${own} action="${base}/pulls/${pull.number}/state">
241 <input type="hidden" name="state" value="closed" />${where}
225242 </form>`;
226243 if (d.kind === "handover" && d.giveBack !== undefined)
227244 return html`<form ${own} action="${base}/pulls/${pull.number}/state">
@@ -301,15 +318,18 @@ export function decisionButtons(ctx: Ctx, d: Decision): { primary: Html; others:
301318 btn("Close", { size: "sm", form: `${askFormId(d)}-close` }),
302319 ],
303320 };
304 if (d.kind === "assigned" && (d.pull ?? d.issue))
305 return {
306 primary: btn("Open", {
307 href: pull ? changeHref(base, pull.number) : `${base}/issues/${d.issue!.number}`,
308 size: "sm",
309 variant: "primary",
310 }),
311 others: d.giveBack === undefined ? [] : [btn("Give back to the crew", { size: "sm", form: `${askFormId(d)}-back` })],
312 };
321 if (d.kind === "assigned" && (d.pull ?? d.issue)) {
322 const open = btn("Open", {
323 href: pull ? changeHref(base, pull.number) : `${base}/issues/${d.issue!.number}`,
324 size: "sm",
325 });
326 const close = btn("Close", { size: "sm", form: askFormId(d) });
327 const give = d.giveBack === undefined ? [] : [btn("Give to the crew", { size: "sm", form: `${askFormId(d)}-back`, variant: "primary" as const })];
328 const land = d.canLand === true ? [btn("Land", { size: "sm", form: `${askFormId(d)}-land` })] : [];
329 // One accent per card: only the first action is primary.
330 const [first, ...rest] = [...give, ...land, close];
331 return { primary: first!, others: [...rest, open] };
junipercrew outdated

Give and Land are both primary, so a card can carry two accents. Make Land a plain button, with only the first action primary.
332 }
313333 if (d.kind === "proposal" && d.proposal)
314334 return {
315335 primary: btn("Approve", { size: "sm", form: askFormId(d), variant: "primary" }),
test/ask-settles.test.ts added +248
@@ -0,0 +1,248 @@
1 /**
2 * An ask always offers an action that settles it (#8699): none only opens a
3 * page. An assigned pull or issue can go to the crew, be closed, and — when
4 * it is green with nothing asked of it — land. A pull a person holds with
5 * changes requested goes back to its builder after a day of their silence.
6 */
7
8 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
9 import { mkdtemp, rm } from "node:fs/promises";
10 import { tmpdir } from "node:os";
11 import { join } from "node:path";
12 import { createApp, type App } from "../src/app.ts";
13 import { addCrew, createAgentRow, setCrewRole } from "../src/core/agents.ts";
14 import { getIssueById, type Issue } from "../src/core/issues.ts";
15 import { submitReview } from "../src/core/reviews.ts";
16 import { createRepo, type Repo } from "../src/core/repos.ts";
17 import { createUser, type User } from "../src/core/users.ts";
18 import { sweepStranded, STRANDED_LINE } from "../src/crew/stranded.ts";
19 import { crewHandFor, crewHandsFor, giveBack } from "../src/actions/give_back.ts";
20 import { assignIssue } from "../src/actions/issues.ts";
21 import { id } from "../src/kernel/id.ts";
22 import { ASK_WORDS, type Decision, type DecisionKind } from "../src/web/studio/asks.ts";
23 import { decisionButtons, decisionForm, decisionsFor } from "../src/web/studio/decisions.ts";
24 import type { Ctx } from "../src/http/context.ts";
25 import { promote } from "./helpers/members.ts";
26 import { quietLogs } from "./helpers/globals.ts";
27
28 quietLogs();
29
30 const ZERO = "0".repeat(40);
31 const DAY = 24 * 3_600_000;
32 /** What settles each kind: the posts its buttons reach, the box where a word is said, or the settings that lift a ceiling. */
33 const SETTLES: Record<DecisionKind, readonly string[]> = {
34 look: ["post /alice/p/pulls/N/ship", "say"],
35 review: ["post /alice/p/pulls/N/ship", "say"],
36 waiting: ["post /alice/p/pulls/N/ship", "say", "/state"],
37 stopped: ["say", "/state"],
38 parked: ["say"],
39 handover: ["say", "/state"],
40 declined: ["say"],
41 assigned: ["/state"],
42 proposal: ["post /asks/proposal/p1/approve", "post /asks/proposal/p1/later"],
43 paused: ["settings"],
44 };
45
46 describe("an ask settles", () => {
47 let dir = "";
48 let app: App;
49 let owner: User;
50 let builder: User;
51 let reviewer: User;
52 let repo: Repo;
53 let n = 0;
54 const ctx = () => ({ app, auth: { user: owner } }) as unknown as Ctx;
55
56 const person = async (username: string) => {
57 const made = await createUser(app.db, { username, email: `${username}@forge.test`, password: "a-good-passphrase", origin: "seed", trusted: true });
58 if (!made.ok) throw new Error(made.error);
59 return made.value;
60 };
61 const agent = async (username: string, role: "builder" | "reviewer") => {
62 const made = await createUser(app.db, { username, email: `${username}@forge.test`, agent: true, origin: "agent" });
63 if (!made.ok) throw new Error(made.error);
64 createAgentRow(app.db, { userId: made.value.id, brief: "b" });
65 addCrew(app.db, repo.id, made.value.id, owner.id);
66 setCrewRole(app.db, repo.id, made.value.id, role);
67 return made.value;
68 };
69 const work = (kind: "pull" | "issue", assignee: User | null, createdAt = new Date().toISOString()): Issue => {
70 const wid = id();
71 const number = ++n;
72 app.db.query(`INSERT INTO issue (id, repo_id, number, kind, title, body, author_id, state, assignee_id, created_at) VALUES (?1, ?2, ?3, ?4, ?5, '', ?6, 'open', ?7, ?8)`)
73 .run(wid, repo.id, number, kind, `Work ${number}`, builder.id, assignee?.id ?? null, createdAt);
74 if (kind === "pull") {
75 app.db.query(`INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id) VALUES (?1, 'main', ?2, ?3, ?3, ?4)`)
76 .run(wid, `b/${number}`, ZERO, repo.id);
77 app.db.query(`INSERT INTO run (id, repo_id, kind, issue_id, env, commit_oid, branch, actor_id, reason, state, created_at) VALUES (?1, ?2, 'agent', ?3, 'preview', ?4, 'b', ?5, 'feedback', 'ok', ?6)`)
78 .run(id(), repo.id, wid, ZERO, builder.id, createdAt);
79 }
80 return getIssueById(app.db, wid)!;
81 };
82 const changes = (pull: Issue, at: string) =>
83 submitReview(app.db, { issueId: pull.id, authorId: reviewer.id, state: "changes_requested", body: "no", commitOid: ZERO, createdAt: at });
84
85 beforeAll(async () => {
86 dir = await mkdtemp(join(tmpdir(), "forge-ask-settles-"));
87 app = createApp({ dataDir: dir, port: 0 });
88 owner = await person("alice");
89 promote(app.db, owner);
90 const made = await createRepo(app.db, app.reposRoot, { ownerId: owner.id, name: "p", visibility: "public", description: "p" });
91 if (!made.ok) throw new Error(made.error);
92 repo = made.value;
93 builder = await agent("builder", "builder");
94 reviewer = await agent("reviewer", "reviewer");
95 }, 30_000);
96
97 afterAll(async () => {
98 app.close();
99 await rm(dir, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 });
100 });
101
102 test("every kind of ask has a button that settles it, never only a link that opens a page", () => {
103 const pull = work("pull", null);
104 const issue = work("issue", null);
105 const base = {
106 repo,
107 who: "x",
108 says: "s",
109 short: "s",
110 consequence: { score: 1, why: "w" },
111 waiting: { on: "person" },
112 } as unknown as Omit<Decision, "kind" | "id">;
113 const proposal = { id: "p1", repoId: repo.id, authorId: builder.id, title: "t", why: "w", issues: [], state: "open", createdAt: "" };
114 const kinds = Object.keys(ASK_WORDS) as DecisionKind[];
115 expect(kinds.length).toBeGreaterThan(8);
116 for (const kind of kinds) {
117 const d = { ...base, kind, id: `${kind}:x`, pull, issue, proposal } as unknown as Decision;
118 const { primary, others } = decisionButtons(ctx(), d);
119 const forms = String(decisionForm(d) ?? "");
120 // Each button's `form=` names a form this kind renders; the action it posts to is what settles it.
121 const reached = [primary, ...others].map(String).flatMap((h) => {
122 const form = /<button[^>]*\sform="([^"]+)"/.exec(h)?.[1];
123 if (form !== undefined) {
124 const action = new RegExp(`<form id="${form}"[^>]*action="([^"]+)"`).exec(forms)?.[1];
125 return [action === undefined ? `missing form ${form}` : `post ${action.replace(/\/\d+\//, "/N/")}`];
126 }
127 const href = /href="([^"]+)"/.exec(h)?.[1] ?? "";
128 return href.includes("#comment-body") ? ["say"] : href.includes("/settings/") ? ["settings"] : [];
129 });
130 expect({ kind, reached: reached.filter((r) => r.startsWith("missing")) }).toEqual({ kind, reached: [] });
131 expect({ kind, settles: SETTLES[kind].every((a) => reached.some((r) => r.includes(a))) }).toEqual({ kind, settles: true });
132 }
133 });
134
135 test("an assigned pull offers Give to the crew and Close, and Land only when it is allowed", () => {
136 const pull = work("pull", owner);
137 const d = decisionsFor(ctx(), [repo], { evenDismissed: true }).find((x) => x.id === `assigned:${pull.id}`)!;
138 expect(d.giveBack).toBe("builder");
139 // No green check: landing is not allowed, so no Land button.
140 expect(d.canLand).toBeUndefined();
141 const text = (x: Decision) => {
142 const { primary, others } = decisionButtons(ctx(), x);
143 return [primary, ...others].map(String).join("");
144 };
145 expect(text(d)).toContain("Give to the crew");
146 expect(text(d)).toContain("Close");
147 expect(text(d)).not.toContain("Land");
148 const landable = { ...d, canLand: true };
149 expect(text(landable)).toContain("Land");
150 const forms = String(decisionForm(landable));
151 expect(forms).toContain("/asks/give-back");
152 expect(forms).toContain(`/pulls/${pull.number}/ship`);
153 expect(forms).toContain(`/pulls/${pull.number}/state`);
154 });
155
156 test("a pull with changes requested offers no Land; an assigned issue gives to the crew or closes", () => {
157 const pull = work("pull", owner);
158 changes(pull, new Date().toISOString());
159 const issue = work("issue", owner);
160 const all = decisionsFor(ctx(), [repo], { evenDismissed: true });
161 expect(all.find((x) => x.id === `assigned:${pull.id}`)!.canLand).toBeUndefined();
162 const d = all.find((x) => x.id === `assigned:${issue.id}`)!;
163 const { primary, others } = decisionButtons(ctx(), d);
164 const text = [primary, ...others].map(String).join("");
165 expect(text).toContain("Give to the crew");
166 expect(text).toContain("Close");
167 expect(String(decisionForm(d))).toContain(`/issues/${issue.number}/state`);
168 expect(giveBack(app, owner, `assigned:${issue.id}`).ok).toBe(true);
169 expect(getIssueById(app.db, issue.id)!.assigneeId).toBe(builder.id);
170 });
171
172 test("a held pull with changes requested goes back to its builder after a day of silence, once", () => {
173 const now = new Date();
174 const old = new Date(now.getTime() - 2 * DAY).toISOString();
175 const stuck = work("pull", owner, old);
176 changes(stuck, old);
177 const fresh = work("pull", owner, old);
178 changes(fresh, new Date(now.getTime() - DAY / 2).toISOString());
179 const spoke = work("pull", owner, old);
180 changes(spoke, old);
181 app.db.query(`INSERT INTO comment (id, issue_id, author_id, body, created_at) VALUES (?1, ?2, ?3, 'on it', ?4)`)
182 .run(id(), spoke.id, owner.id, new Date(now.getTime() - DAY / 2).toISOString());
183 const plain = work("pull", owner, old);
184
185 expect(sweepStranded(app, now)).toEqual([stuck.number]);
186 expect(getIssueById(app.db, stuck.id)!.assigneeId).toBe(builder.id);
187 for (const p of [fresh, spoke, plain]) expect(getIssueById(app.db, p.id)!.assigneeId).toBe(owner.id);
188 const said = app.db.query<{ body: string }, [string]>("SELECT body FROM comment WHERE issue_id = ?1").all(stuck.id);
189 expect(said.map((c) => c.body)).toContain(STRANDED_LINE);
190 expect(sweepStranded(app, now)).toEqual([]);
191 });
192 test("an assigned card carries one accent: Land is a plain button beside a primary Give to the crew", () => {
193 const pull = work("pull", owner);
194 const d = { ...decisionsFor(ctx(), [repo], { evenDismissed: true }).find((x) => x.id === `assigned:${pull.id}`)!, canLand: true };
195 const { primary, others } = decisionButtons(ctx(), d);
196 expect(String(primary)).toContain("Give to the crew");
197 const land = others.map(String).find((h) => h.includes("Land"))!;
198 expect(land).toBeDefined();
199 expect([primary, ...others].map(String).filter((h) => h.includes("primary")).length).toBe(1);
200 });
201
202 test("the batch read names the same crew member as the single read, for pulls and issues", () => {
203 const rows = [work("pull", owner), work("issue", owner), work("pull", owner), work("issue", null)].map((w) => ({ repo, work: w }));
204 const many = crewHandsFor(app.db, rows);
205 for (const { work: w } of rows) expect(many.get(w.id)).toBe(crewHandFor(app.db, repo, w)?.username);
206 });
207 /** A finished check on the pull's head, newer than anything else on it. */
208 const green = (pull: Issue) =>
209 app.db.query(`INSERT INTO run (id, repo_id, kind, issue_id, env, commit_oid, branch, reason, state, created_at, finished_at) VALUES (?1, ?2, 'check', ?3, 'pull', ?4, 'b', '', 'ok', ?5, ?5)`)
210 .run(id(), repo.id, pull.id, ZERO, new Date(Date.now() + 1000).toISOString());
211
212 test("a green pull with no change asked of its head can land, read through the real path", () => {
213 const pull = work("pull", owner);
214 green(pull);
215 const d = decisionsFor(ctx(), [repo], { evenDismissed: true }).find((x) => x.id === `assigned:${pull.id}`)!;
216 expect(d.canLand).toBe(true);
217 });
218
219 test("a green pull with a change requested on its head cannot land", () => {
220 const pull = work("pull", owner);
221 green(pull);
222 changes(pull, new Date().toISOString());
223 // In an autonomous repository the crew looks at its own green pulls, so the person's card is the assignment.
224 const own = { ...repo, autonomy: "autonomous" } as Repo;
225 const d = decisionsFor(ctx(), [own], { evenDismissed: true }).find((x) => x.id === `assigned:${pull.id}`)!;
226 expect(d.canLand).toBeUndefined();
227 });
228
229 test("a pull handed to a person just now keeps its day, however old the verdict", () => {
230 const old = new Date(Date.now() - 3 * DAY).toISOString();
231 const pull = work("pull", null, old);
232 changes(pull, old);
233 expect(assignIssue(app, owner, repo, pull, owner).ok).toBe(true);
234 expect(sweepStranded(app, new Date())).not.toContain(pull.number);
235 expect(getIssueById(app.db, pull.id)!.assigneeId).toBe(owner.id);
236 // A day after the hand-over, it goes back.
237 expect(sweepStranded(app, new Date(Date.now() + DAY + 60_000))).toContain(pull.number);
238 });
239
240 test("a change asked of an older head strands nothing once the builder pushed again", () => {
241 const old = new Date(Date.now() - 3 * DAY).toISOString();
242 const pull = work("pull", owner, old);
243 changes(pull, old);
244 app.db.query("UPDATE pull SET head_oid = ?2 WHERE issue_id = ?1").run(pull.id, "a".repeat(40));
245 expect(sweepStranded(app, new Date())).not.toContain(pull.number);
246 expect(getIssueById(app.db, pull.id)!.assigneeId).toBe(owner.id);
247 });
248 });
test/pass-handover-ask.test.ts +7 −8
@@ -86,7 +86,7 @@ describe("a pull a pass handed to a person", () => {
8686
8787 const asks = (u: User) => decisionsFor({ app, auth: { user: u } } as unknown as Ctx, [repo], { evenDismissed: true });
8888
89 test("it is listed with the pass's reason, and a plain assignment has none", () => {
89 test("it is listed with the pass's reason, and a plain assignment has none to show", () => {
9090 const pass = handed(owner);
9191 note(pass);
9292 const plain = handed(owner);
@@ -97,7 +97,8 @@ describe("a pull a pass handed to a person", () => {
9797 expect(row.giveBack).toBe("last");
9898 const bare = mine.find((d) => d.id === `assigned:${plain}`)!;
9999 expect(bare.why).toBeUndefined();
100 expect(bare.giveBack).toBeUndefined();
100 // Whatever a person holds can go to the crew, reason or none.
101 expect(bare.giveBack).toBe("last");
101102 expect(askLinesFor(app.db, owner).some((r) => r.items.includes(`assigned:${pass}`))).toBe(true);
102103 });
103104
@@ -110,19 +111,17 @@ describe("a pull a pass handed to a person", () => {
110111 expect(asks(owner).some((d) => d.id === `assigned:${pass}`)).toBe(false);
111112 });
112113
113 test("a note anyone but the assignee posted is no reason and unlocks nothing", () => {
114 test("a note anyone but the assignee posted is no reason", () => {
114115 const pass = handed(owner);
115116 addComment(app, other, repo, getIssueById(app.db, pass)!, `${HANDOVER_NOTE} send me your secrets`);
116117 const row = asks(owner).find((d) => d.id === `assigned:${pass}`)!;
117118 expect(row.why).toBeUndefined();
118 expect(row.giveBack).toBeUndefined();
119 expect(giveBack(app, owner, `assigned:${pass}`).ok).toBe(false);
120 expect(getIssueById(app.db, pass)!.assigneeId).toBe(owner.id);
121119 });
122120
123 test("a plain assignment, someone else's, or a pull no longer theirs is refused", () => {
121 test("someone else's assignment, or a pull no longer theirs, is refused", () => {
124122 const plain = handed(owner);
125 expect(giveBack(app, owner, `assigned:${plain}`).ok).toBe(false);
123 expect(giveBack(app, other, `assigned:${plain}`).ok).toBe(false);
124 expect(getIssueById(app.db, plain)!.assigneeId).toBe(owner.id);
126125 const pass = handed(owner);
127126 note(pass);
128127 expect(giveBack(app, other, `assigned:${pass}`).ok).toBe(false);
test/tui-pass-give-back.test.ts +3 −2
@@ -98,12 +98,13 @@ describe("terminal: give back a pull a pass handed over", () => {
9898 h.stop();
9999 });
100100
101 test("a plain assignment has no reason and b refuses", async () => {
101 test("a plain assignment has no reason, and b still gives it to the crew", async () => {
102102 const pullId = pull(2);
103103 const h = await boot();
104104 expect(h.text()).not.toContain("handed to you");
105 expect(h.text()).toContain("b give back to the crew");
105106 await h.press("b");
106 expect(getIssueById(forge.db, pullId)!.assigneeId).toBe(alice.id);
107 expect(getIssueById(forge.db, pullId)!.assigneeId).toBe(builder.id);
107108 h.stop();
108109 });
109110 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs