Skip to content

Commit 2bc903a

Browse files
committed
Finish a review's life: resume it, notice it merged, delete it, tune it
Work item V6 of the M3 plan. A review could be started and decided, and after that the app had nothing to say about it: no way to resume one that paused, no sign that its branch had since merged, no way to delete it, and no way to change any setting that governs how it runs. Merged detection reads the refs the clone already has rather than fetching. It is a convenience, not something the app owes the network on every page load, so a merge is noticed at the next fetch, which is what someone does anyway when they open a branch list. Nothing is deleted automatically: a merged review is the record of how something got merged, and deciding that history is disposable is not the app's call. Writing its test changed the design. The first version checked only finished reviews and missed the clearest case there is: a draft of a branch that has since merged is stale before it ever ran, and is exactly what someone would want to delete. Everything except a review in flight is checked now. Settings are editable, and only the catalogued keys are accepted, each validated by its own schema and refused by name otherwise. A settings table that accepts whatever it is handed is where typos live silently. Model probing is a button and never a background task, which is the policy V1 set after the incident on this machine: a probe is a real call, and an app that probed on a timer would spend someone's usage while they were not looking. The sign-in card runs the CLI's own status check locally and says plainly when a login bills per token rather than drawing on a subscription. Driving it live found a shipped bug. readAuthStatus parsed the CLI's output without a guard, so any binary printing something unexpected threw out of a status check whose whole job is to answer a question calmly; pointing TRYSQUARE_CLAUDE_PATH at the fake returned a JSON syntax error instead of "not signed in". Guarded, tested, and the mutation is caught. Three other mutations checked and caught: recording a merge without checking ancestry, writing uncatalogued settings keys, and the unguarded parse.
1 parent 0ed4d13 commit 2bc903a

15 files changed

Lines changed: 731 additions & 21 deletions

File tree

docs/DECISIONS.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,3 +742,22 @@ verified evidence, in writing, here.
742742
the sentence explaining the problem. A report cannot make prose out of a
743743
comment full of code, so the instruction belongs where the comment is
744744
written rather than where it is rendered.
745+
- 2026-07-31 DECIDED (V6, D-41): merged detection reads the refs the clone
746+
already has rather than fetching, and runs when a review is opened or listed.
747+
It is a convenience, not a fact the app owes the network on every page load,
748+
so a branch that merged is noticed at the next fetch, which is what someone
749+
does anyway when they open a branch list.
750+
- 2026-07-31 DECIDED (V6): every review except one that is mid-run is checked
751+
for a merge. The first version checked only finished reviews and missed the
752+
clearest case there is: a draft of a branch that has since merged is stale
753+
before it ever ran, and is exactly what someone would want to delete. A
754+
review in flight is the only one where the answer is noise.
755+
- 2026-07-31 DECIDED (V6, D-43): the settings API accepts only catalogued
756+
keys, each validated by its own schema, and refuses anything else by name. A
757+
settings table that accepts whatever it is handed is where typos live
758+
silently, and a reader cannot tell which keys are real.
759+
- 2026-07-31 FIXED (V6): `readAuthStatus` parsed the CLI's output without a
760+
guard, so any binary that printed something unexpected threw out of a status
761+
check whose whole job is to answer a question calmly. Found by pointing
762+
TRYSQUARE_CLAUDE_PATH at the fake and watching the route return a JSON
763+
syntax error instead of "not signed in".

docs/plans/M3-FINISH-PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ confirms the wiring.
348348
| V3 | project detail page, fetch-now, links CRUD, project delete | V1 | DONE |
349349
| V4 | preflight route and panel, linked toggle with suggestion | V3 | DONE |
350350
| V5 | report renderer, report/export routes, report UI | V2 | DONE |
351-
| V6 | resume/queued/merged/delete UI, settings editor, probe buttons | V2 | |
351+
| V6 | resume/queued/merged/delete UI, settings editor, probe buttons | V2 | DONE |
352352
| V7 | rulesets detail, enable toggle, snapshot filter, export | V1 | |
353353
| V8 | e2e journey, theme screenshots, design audit, CI --e2e | V2-V7 | |
354354
| V9 | PROJECT-STATE, indexes, FG-2 checklist and evidence, G1 row | V8 | |

src/app/api/auth/route.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* Whether the CLI is logged in, and whether runs draw on a subscription.
3+
*
4+
* Worth surfacing because the difference is money: an API-key login bills per
5+
* token, and someone who set one up for something else would otherwise not
6+
* find out until a bill arrived. Runs `claude auth status` locally and spends
7+
* nothing.
8+
*/
9+
10+
import { readAuthStatus } from "@/server/engine/probe";
11+
import { handler, ok } from "@/server/api/respond";
12+
13+
export const dynamic = "force-dynamic";
14+
15+
export function GET(): Promise<Response> {
16+
return handler(async () => {
17+
const claudePath = process.env.TRYSQUARE_CLAUDE_PATH;
18+
return ok({
19+
auth: await readAuthStatus(claudePath === undefined ? {} : { claudePath }),
20+
});
21+
});
22+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/**
2+
* Asking the CLI whether one model is actually usable.
3+
*
4+
* Only ever from an explicit click (D-31). A probe is a real call that spends
5+
* a fraction of a cent, and an app that probed on a timer would spend the
6+
* user's usage while they were not looking. The prompt and toolset are minimal
7+
* so the cost is as small as a real call can be.
8+
*/
9+
10+
import {
11+
recordProbeFailure,
12+
recordProbeSuccess,
13+
requireModel,
14+
} from "@/server/db/repositories/models";
15+
import { probeModel } from "@/server/engine/probe";
16+
import { handler, ok } from "@/server/api/respond";
17+
import { runtime } from "@/server/runtime";
18+
19+
export const dynamic = "force-dynamic";
20+
21+
export async function POST(
22+
_request: Request,
23+
context: { params: Promise<{ id: string }> },
24+
): Promise<Response> {
25+
return handler(async () => {
26+
const { db } = runtime();
27+
const { id } = await context.params;
28+
const model = requireModel(db, id);
29+
30+
const claudePath = process.env.TRYSQUARE_CLAUDE_PATH;
31+
const outcome = await probeModel(model.id, claudePath === undefined ? {} : { claudePath });
32+
33+
if (outcome.status === "available") {
34+
recordProbeSuccess(db, model.id, {
35+
resolvedId: outcome.resolvedId,
36+
contextWindow: outcome.contextWindow,
37+
});
38+
} else {
39+
// Indeterminate is recorded as a failure with its own message rather
40+
// than left as unknown: "we asked and could not tell" is more useful
41+
// than "we never asked".
42+
recordProbeFailure(db, model.id, outcome.error);
43+
}
44+
45+
return ok({ outcome, model: requireModel(db, model.id) });
46+
});
47+
}

src/app/api/reviews/[id]/route.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
/** Everything a review page needs in one read. */
22

33
import { listFindings } from "@/server/db/repositories/findings";
4-
import { handler, ok } from "@/server/api/respond";
4+
import { requireReview } from "@/server/db/repositories/reviews";
5+
import { deleteReviewEntirely } from "@/server/review/service";
6+
import { detectMerged } from "@/server/review/merged";
7+
import { failed, handler, ok } from "@/server/api/respond";
58
import { runtime } from "@/server/runtime";
69

710
export const dynamic = "force-dynamic";
@@ -13,6 +16,31 @@ export async function GET(
1316
return handler(async () => {
1417
const { db, manager } = runtime();
1518
const { id } = await context.params;
19+
await detectMerged(db, [requireReview(db, id)]);
1620
return ok({ ...manager.snapshot(id), findings: listFindings(db, id) });
1721
});
1822
}
23+
24+
/**
25+
* Removes a review and everything it produced except its exports.
26+
*
27+
* Refused while it is running, because a review with a live subprocess needs
28+
* cancelling first: deleting the row would orphan the process rather than stop
29+
* it.
30+
*/
31+
export async function DELETE(
32+
_request: Request,
33+
context: { params: Promise<{ id: string }> },
34+
): Promise<Response> {
35+
return handler(async () => {
36+
const { db, dataDir } = runtime();
37+
const { id } = await context.params;
38+
39+
try {
40+
await deleteReviewEntirely(db, id, dataDir);
41+
} catch (error) {
42+
return failed(error, 409);
43+
}
44+
return ok({ removed: id });
45+
});
46+
}

src/app/api/reviews/route.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
import { createReview, listActiveReviews } from "@/server/db/repositories/reviews";
1919
import { fetchAll, mergeBase, resolveCommit } from "@/server/gitops/repo";
2020
import { created, handler, ok, readJson } from "@/server/api/respond";
21+
import { detectMerged } from "@/server/review/merged";
2122
import { runtime } from "@/server/runtime";
2223
import { listProjects } from "@/server/db/repositories/projects";
2324
import { listReviewsForProject } from "@/server/db/repositories/reviews";
@@ -44,6 +45,15 @@ const body = z.object({
4445
export function GET(): Promise<Response> {
4546
return handler(async () => {
4647
const { db } = runtime();
48+
49+
// Checked when the list is opened rather than on a timer: there is no
50+
// moment a background poll would be right for, and a stale badge is worse
51+
// than a late one (D-19).
52+
await detectMerged(
53+
db,
54+
listProjects(db).flatMap((project) => listReviewsForProject(db, project.id)),
55+
);
56+
4757
const reviews = listProjects(db).flatMap((project) =>
4858
listReviewsForProject(db, project.id).map((review) => ({
4959
...review,

src/app/api/settings/route.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* The handful of settings this app has.
3+
*
4+
* Only catalogued keys, each with its own schema. An unknown key is refused
5+
* rather than written, because a settings table that accepts anything becomes
6+
* a place where typos live silently and a reader cannot tell which keys are
7+
* real.
8+
*/
9+
10+
import { z } from "zod";
11+
import { SETTING_KEYS, readSettingOr, writeSetting } from "@/server/db/repositories/settings";
12+
import { handler, ok, readJson } from "@/server/api/respond";
13+
import { runtime } from "@/server/runtime";
14+
15+
export const dynamic = "force-dynamic";
16+
17+
/** Every setting, its schema, and what it means when nobody has set it. */
18+
const CATALOGUE = {
19+
[SETTING_KEYS.maxConcurrentReviews]: { schema: z.number().int().positive(), fallback: 1 },
20+
[SETTING_KEYS.stageTimeoutMinutes]: { schema: z.number().int().positive(), fallback: 20 },
21+
[SETTING_KEYS.stageMaxBudgetUsd]: { schema: z.number().nonnegative(), fallback: 15 },
22+
} as const;
23+
24+
type Key = keyof typeof CATALOGUE;
25+
26+
export function GET(): Promise<Response> {
27+
return handler(async () => {
28+
const { db } = runtime();
29+
const settings = Object.fromEntries(
30+
(Object.keys(CATALOGUE) as Key[]).map((key) => [
31+
key,
32+
readSettingOr(db, key, CATALOGUE[key].schema, CATALOGUE[key].fallback),
33+
]),
34+
);
35+
return ok({ settings });
36+
});
37+
}
38+
39+
export function PUT(request: Request): Promise<Response> {
40+
return handler(async () => {
41+
const { db } = runtime();
42+
const body = await readJson(request, z.record(z.string(), z.unknown()));
43+
44+
for (const [key, value] of Object.entries(body)) {
45+
const entry = CATALOGUE[key as Key];
46+
if (!entry) {
47+
throw Response.json(
48+
{ error: `There is no setting called "${key}".`, code: "UnknownSetting" },
49+
{ status: 400 },
50+
);
51+
}
52+
const parsed = entry.schema.safeParse(value);
53+
if (!parsed.success) {
54+
throw Response.json(
55+
{
56+
error: `${key}: ${parsed.error.issues[0]?.message ?? "is not valid"}`,
57+
code: "Invalid",
58+
},
59+
{ status: 400 },
60+
);
61+
}
62+
writeSetting(db, key, parsed.data);
63+
}
64+
65+
return (await GET()) as Response;
66+
});
67+
}

src/app/reviews/[id]/page.tsx

Lines changed: 73 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ interface Snapshot {
4343
effort: string;
4444
intent: string | null;
4545
pausedReason: string | null;
46+
mergedDetectedAt: string | null;
4647
usageInputTokens: number;
4748
usageOutputTokens: number;
4849
usageCacheReadTokens: number;
@@ -52,6 +53,7 @@ interface Snapshot {
5253
notes: { kind: string; message: string; at: string }[];
5354
findings: Finding[];
5455
running: boolean;
56+
queued: boolean;
5557
}
5658

5759
export default function ReviewPage({ params }: { params: Promise<{ id: string }> }) {
@@ -60,6 +62,8 @@ export default function ReviewPage({ params }: { params: Promise<{ id: string }>
6062
const [live, setLive] = useState<string[]>([]);
6163
const [report, setReport] = useState<string | null>(null);
6264
const [exported, setExported] = useState("");
65+
const [confirmingDelete, setConfirmingDelete] = useState(false);
66+
const [actionError, setActionError] = useState("");
6367

6468
async function reload() {
6569
const response = await fetch(`/api/reviews/${id}`);
@@ -174,27 +178,85 @@ export default function ReviewPage({ params }: { params: Promise<{ id: string }>
174178
}
175179
subtitle={
176180
<span className="flex flex-wrap items-center gap-2">
177-
<Badge tone={statusTone(review.status)}>{review.status.replace(/_/g, " ")}</Badge>
181+
<Badge tone={statusTone(review.status)}>
182+
{snapshot.queued ? "queued" : review.status.replace(/_/g, " ")}
183+
</Badge>
184+
{review.mergedDetectedAt ? <Badge tone="good">merged</Badge> : null}
178185
<Sha value={review.fromCommit} />
179186
<span>{review.model}</span>
180187
<span>effort {review.effort}</span>
181188
</span>
182189
}
183190
actions={
184-
snapshot.running ? (
185-
<Button
186-
onClick={async () => {
187-
await fetch(`/api/reviews/${id}/cancel`, { method: "POST" });
188-
await reload();
189-
}}
190-
>
191-
Cancel
192-
</Button>
193-
) : null
191+
<>
192+
{review.status === "paused_limit" || review.status === "interrupted" ? (
193+
<Button
194+
variant="primary"
195+
onClick={async () => {
196+
// No ruleset needed: a resumed review already carries the
197+
// one it was frozen with.
198+
await fetch(`/api/reviews/${id}/start`, { method: "POST" });
199+
await reload();
200+
}}
201+
>
202+
Resume
203+
</Button>
204+
) : null}
205+
206+
{snapshot.running || snapshot.queued ? (
207+
<Button
208+
onClick={async () => {
209+
await fetch(`/api/reviews/${id}/cancel`, { method: "POST" });
210+
await reload();
211+
}}
212+
>
213+
Cancel
214+
</Button>
215+
) : null}
216+
217+
{!snapshot.running && !snapshot.queued ? (
218+
confirmingDelete ? (
219+
<>
220+
<Button
221+
variant="primary"
222+
onClick={async () => {
223+
const response = await fetch(`/api/reviews/${id}`, { method: "DELETE" });
224+
if (response.ok) window.location.href = "/reviews";
225+
else
226+
setActionError(
227+
((await response.json()) as { error?: string }).error ??
228+
"The review could not be deleted.",
229+
);
230+
}}
231+
>
232+
Yes, delete
233+
</Button>
234+
<Button variant="quiet" onClick={() => setConfirmingDelete(false)}>
235+
Keep
236+
</Button>
237+
</>
238+
) : (
239+
<Button onClick={() => setConfirmingDelete(true)}>Delete</Button>
240+
)
241+
) : null}
242+
</>
194243
}
195244
/>
196245
<PageBody>
246+
{actionError ? <Problem>{actionError}</Problem> : null}
197247
{review.pausedReason ? <Problem>{review.pausedReason}</Problem> : null}
248+
{review.mergedDetectedAt ? (
249+
<p className="mb-4 text-sm text-[var(--color-ink-muted)]">
250+
This branch has since merged into {review.intoBranch}. The review is kept until you
251+
delete it.
252+
</p>
253+
) : null}
254+
{snapshot.queued ? (
255+
<p className="mb-4 text-sm text-[var(--color-ink-muted)]">
256+
Waiting for the running review to finish. One runs at a time, because two would race for
257+
the same usage limit.
258+
</p>
259+
) : null}
198260

199261
<div className="grid gap-6 lg:grid-cols-[1fr_20rem]">
200262
<div className="min-w-0">

src/app/reviews/page.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ interface Row {
1414
intoBranch: string;
1515
status: string;
1616
createdAt: string;
17+
mergedDetectedAt: string | null;
1718
}
1819

1920
export default function ReviewsPage() {
@@ -48,6 +49,7 @@ export default function ReviewsPage() {
4849
<Badge tone={statusTone(review.status)}>
4950
{review.status.replace(/_/g, " ")}
5051
</Badge>
52+
{review.mergedDetectedAt ? <Badge tone="good">merged</Badge> : null}
5153
<span className="text-sm font-medium">{review.projectName}</span>
5254
<Mono className="truncate text-xs text-[var(--color-ink-muted)]">
5355
{review.fromBranch} into {review.intoBranch}

0 commit comments

Comments
 (0)