Skip to content

Commit 311d041

Browse files
committed
Let a human decide every finding, and refuse to finish until they have
Work item V2 of the M3 plan: the loop the whole app exists to reach. Findings could be produced and read, and there was no way to accept or reject one, so the founding requirement, that nothing is reported until a person says so, was not drivable at all. Four routes: confirm, dismiss with a reason, complete, and the file context a decision needs. Completing refuses while anything is undecided and says how many, on the server rather than by hiding a button, because the gatekeeper is a rule about the data and a script calling the API directly should meet the same wall the screen does. Completing also releases the worktrees, which joins cancelled and failed from V1 and finishes D-12: the confirmation screen was the last thing that needed the checkout. Dismissing requires a reason and confirming does not. Accepting the engine's case adds nothing to it; a dismissal without a reason leaves no record of whether the engine was wrong or the reviewer was in a hurry, and those two are the difference between a prompt that needs fixing and one that does not. The queue is built for someone working through twenty findings rather than admiring one: j and k to move, c to confirm, d to dismiss, enter for the surrounding code, and confirming jumps to the next undecided finding rather than the next row. Decided findings collapse to a line instead of vanishing, so what was dismissed and why stays on the page. The file-context endpoint serves only paths a finding in that review cites, and only while it awaits confirmation. The first guard's test initially passed with the guard deleted, because it asked for a file that does not exist and the 404 came from the failed read; it now asks for a fixture file that genuinely exists and that the reviewer deliberately finds nothing in, and the mutation is caught. Driven end to end against a running production server with the fake engine: complete refused with seven undecided, context returned the lines around a finding, one dismissal with a reason, six confirmations, complete accepted, and the run directory left holding bundle and logs with the checkout gone. The engine note added in V1 earned itself immediately, naming the fake in the run so there was no question which binary answered.
1 parent 1f1d54a commit 311d041

9 files changed

Lines changed: 859 additions & 49 deletions

File tree

docs/DECISIONS.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,3 +656,24 @@ verified evidence, in writing, here.
656656
confirmation; complete joins the cleanup list when the confirmation flow
657657
lands (D-12 as implemented). Cleanup is best effort: a failure to remove
658658
becomes a run note, never a mask over the outcome that matters.
659+
- 2026-07-31 DECIDED (V2, D-34): dismissing a finding requires a reason;
660+
confirming does not. Accepting the engine's case adds nothing to it, while a
661+
dismissal without a reason leaves no record of whether the engine was wrong
662+
or the reviewer was in a hurry, and those two are the difference between a
663+
prompt that needs fixing and one that does not.
664+
- 2026-07-31 DECIDED (V2, D-35): a review completes only when every finding is
665+
decided, enforced by the complete route with the undecided count in the
666+
message. The human gatekeeper is a rule about the data, so a script calling
667+
the API directly meets the same wall the screen does.
668+
- 2026-07-31 DECIDED (V2, D-36): the file-context endpoint serves only paths a
669+
finding in that review actually cites, and only while the review awaits
670+
confirmation. Without the first guard it is a way to read any file on the
671+
machine; without the second it promises a checkout that D-12 has already
672+
removed. Proving the first guard needed a test using a file that genuinely
673+
exists in the checkout: an absent path 404s from the failed read whether the
674+
guard is there or not, and the first version of that test passed with the
675+
guard deleted.
676+
- 2026-07-31 DECIDED (V2): completing a review removes its worktrees, joining
677+
cancelled and failed from V1 (D-12 now fully implemented). The confirmation
678+
screen was the last thing that needed the checkout; the bundle and the logs
679+
stay as the evidence behind the report.

docs/plans/M3-FINISH-PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ confirms the wiring.
344344
| V | Contents | Depends on | Status |
345345
| -- | ------------------------------------------------------------ | ---------- | ------ |
346346
| V1 | budget cap, engine note, no-auto-probe, worktree cleanup, 404 unlink, activity cap | - | DONE |
347-
| V2 | confirm/dismiss/complete/context routes, confirmation UI, keyboard map | V1 | |
347+
| V2 | confirm/dismiss/complete/context routes, confirmation UI, keyboard map | V1 | DONE |
348348
| V3 | project detail page, fetch-now, links CRUD, project delete | V1 | |
349349
| V4 | preflight route and panel, linked toggle with suggestion | V3 | |
350350
| V5 | report renderer, report/export routes, report UI | V2 | |
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* A human accepting a finding.
3+
*
4+
* The whole app exists to reach this moment: nothing is reported until a
5+
* person has said so. No reason is required, because accepting the engine's
6+
* case adds nothing to it; dismissing does require one.
7+
*/
8+
9+
import { confirmFinding } from "@/server/db/repositories/findings";
10+
import { handler, ok } from "@/server/api/respond";
11+
import { runtime } from "@/server/runtime";
12+
13+
export const dynamic = "force-dynamic";
14+
15+
export async function POST(
16+
_request: Request,
17+
context: { params: Promise<{ id: string }> },
18+
): Promise<Response> {
19+
return handler(async () => {
20+
const { id } = await context.params;
21+
return ok({ finding: confirmFinding(runtime().db, id) });
22+
});
23+
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* A human rejecting a finding, and saying why.
3+
*
4+
* The reason is the point. A dismissal without one leaves no record of
5+
* whether the engine was wrong or the reviewer was in a hurry, and those two
6+
* are the difference between a prompt that needs fixing and one that does not.
7+
*/
8+
9+
import { z } from "zod";
10+
import { dismissFinding } from "@/server/db/repositories/findings";
11+
import { handler, ok, readJson } from "@/server/api/respond";
12+
import { runtime } from "@/server/runtime";
13+
14+
export const dynamic = "force-dynamic";
15+
16+
const body = z.object({
17+
reason: z.string().trim().min(1, "a dismissal needs a reason"),
18+
});
19+
20+
export async function POST(
21+
request: Request,
22+
context: { params: Promise<{ id: string }> },
23+
): Promise<Response> {
24+
return handler(async () => {
25+
const { id } = await context.params;
26+
const { reason } = await readJson(request, body);
27+
return ok({ finding: dismissFinding(runtime().db, id, reason) });
28+
});
29+
}
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* Closing a review, once every finding has been decided.
3+
*
4+
* Enforced here rather than by hiding the button: the human gatekeeper is a
5+
* rule about the data, and a client that forgot to check, or a script calling
6+
* the API directly, must hit the same wall. Completing also releases the
7+
* worktrees, because the confirmation screen was the last thing that needed
8+
* them (D-12).
9+
*/
10+
11+
import { listFindingsByStatus } from "@/server/db/repositories/findings";
12+
import { requireReview, statusOf, transitionReview } from "@/server/db/repositories/reviews";
13+
import { removeReviewWorktrees } from "@/server/review/service";
14+
import { failed, handler, ok } from "@/server/api/respond";
15+
import { runtime } from "@/server/runtime";
16+
17+
export const dynamic = "force-dynamic";
18+
19+
export async function POST(
20+
_request: Request,
21+
context: { params: Promise<{ id: string }> },
22+
): Promise<Response> {
23+
return handler(async () => {
24+
const { db, dataDir, manager } = runtime();
25+
const { id } = await context.params;
26+
27+
const status = statusOf(requireReview(db, id));
28+
if (status !== "awaiting_confirmation") {
29+
return Response.json(
30+
{
31+
error: `This review is ${status.replace(/_/g, " ")}, so there is nothing to complete.`,
32+
code: "NotAwaitingConfirmation",
33+
},
34+
{ status: 409 },
35+
);
36+
}
37+
38+
const undecided = listFindingsByStatus(db, id, ["verified", "open_question"]);
39+
if (undecided.length > 0) {
40+
return Response.json(
41+
{
42+
error:
43+
`${undecided.length} finding(s) still need a decision. A report is the record ` +
44+
"of what a person accepted, so every finding is confirmed or dismissed first.",
45+
code: "UndecidedFindings",
46+
},
47+
{ status: 409 },
48+
);
49+
}
50+
51+
transitionReview(db, id, "complete", { currentStage: null });
52+
53+
try {
54+
await removeReviewWorktrees(db, id, dataDir);
55+
} catch (error) {
56+
// The review is complete either way; a stuck checkout is not a reason
57+
// to refuse the decision the human already made.
58+
return failed(error, 500);
59+
}
60+
61+
return ok({ snapshot: manager.snapshot(id) });
62+
});
63+
}
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* The lines around a finding, read from the review's own worktree.
3+
*
4+
* Two guards, both about not becoming a file server. The path must be one a
5+
* finding in this review actually cites, so a caller cannot walk the disk by
6+
* asking for something else. And the review must be awaiting confirmation,
7+
* which is the only status whose worktree is guaranteed to still exist
8+
* (D-12); afterwards the checkout is gone and the quoted code stored on each
9+
* finding is the record.
10+
*/
11+
12+
import { readFile } from "node:fs/promises";
13+
import { join } from "node:path";
14+
import { worktreeRootDir } from "@/lib/paths";
15+
import { listFindings } from "@/server/db/repositories/findings";
16+
import { requireReview, statusOf } from "@/server/db/repositories/reviews";
17+
import { handler, notFound, ok } from "@/server/api/respond";
18+
import { runtime } from "@/server/runtime";
19+
20+
export const dynamic = "force-dynamic";
21+
22+
/** Enough to see the shape of the function a finding sits in. */
23+
const RADIUS = 20;
24+
25+
export async function GET(
26+
request: Request,
27+
context: { params: Promise<{ id: string }> },
28+
): Promise<Response> {
29+
return handler(async () => {
30+
const { db, dataDir } = runtime();
31+
const { id } = await context.params;
32+
const url = new URL(request.url);
33+
const path = url.searchParams.get("path") ?? "";
34+
const line = Number.parseInt(url.searchParams.get("line") ?? "", 10);
35+
36+
const status = statusOf(requireReview(db, id));
37+
if (status !== "awaiting_confirmation") {
38+
return notFound(
39+
`File context for a review that is ${status.replace(/_/g, " ")}. ` +
40+
"The checkout only exists while the findings are being decided",
41+
);
42+
}
43+
44+
const cited = listFindings(db, id).some((finding) => finding.filePath === path);
45+
if (!cited) return notFound("A file this review did not raise a finding in");
46+
47+
let contents: string;
48+
try {
49+
contents = await readFile(join(worktreeRootDir(dataDir, id), path), "utf8");
50+
} catch {
51+
return notFound(`${path} in the review's checkout`);
52+
}
53+
54+
const all = contents.split("\n");
55+
const centre = Number.isFinite(line) ? line : 1;
56+
const start = Math.max(1, centre - RADIUS);
57+
const end = Math.min(all.length, centre + RADIUS);
58+
59+
return ok({
60+
path,
61+
start,
62+
end,
63+
total: all.length,
64+
lines: all.slice(start - 1, end).map((text, index) => ({ number: start + index, text })),
65+
});
66+
});
67+
}

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

Lines changed: 62 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
*/
1111

1212
import { use, useEffect, useState } from "react";
13+
import { ConfirmationQueue, type Finding } from "@/components/confirmation";
1314
import { PageBody, PageHeader } from "@/components/page";
1415
import {
1516
Badge,
@@ -49,16 +50,7 @@ interface Snapshot {
4950
};
5051
stages: { stage: string; status: string; attempt: number }[];
5152
notes: { kind: string; message: string; at: string }[];
52-
findings: {
53-
id: string;
54-
filePath: string;
55-
lineStart: number;
56-
severity: string;
57-
issue: string;
58-
comment: string;
59-
status: string;
60-
quotedCode: string;
61-
}[];
53+
findings: Finding[];
6254
running: boolean;
6355
}
6456

@@ -120,6 +112,54 @@ export default function ReviewPage({ params }: { params: Promise<{ id: string }>
120112
);
121113
const reported = findings.filter((finding) => finding.status !== "killed");
122114

115+
/**
116+
* The findings as a list you can only read.
117+
*
118+
* Used while a review is still running, and after it is complete. Between
119+
* those two the confirmation queue takes over, because that is the one
120+
* moment the findings are a thing to act on rather than a thing to look at.
121+
*/
122+
const renderReadOnly = () =>
123+
reported.length === 0 ? (
124+
<Empty title={snapshot.running ? "Nothing reported yet" : "No findings"}>
125+
{snapshot.running
126+
? "Findings appear once the adversarial pass has run and each one has been checked against the file."
127+
: "Every hunk was accounted for and nothing survived verification."}
128+
</Empty>
129+
) : (
130+
<ul className="grid gap-3">
131+
{reported.map((finding) => (
132+
<li key={finding.id}>
133+
<Card className="p-4">
134+
<div className="flex flex-wrap items-center gap-2">
135+
<Badge tone={severityTone(finding.severity)}>{finding.severity}</Badge>
136+
{finding.status === "confirmed" ? <Badge tone="good">confirmed</Badge> : null}
137+
{finding.status === "dismissed" ? <Badge tone="neutral">dismissed</Badge> : null}
138+
{finding.status === "open_question" ? (
139+
<Badge tone="question">open question</Badge>
140+
) : null}
141+
<Mono className="text-xs text-[var(--color-ink-muted)]">
142+
{finding.filePath}:{finding.lineStart}
143+
</Mono>
144+
</div>
145+
<p className="mt-2 font-medium">{finding.issue}</p>
146+
<p className="mt-1 text-sm text-[var(--color-ink-muted)]">{finding.comment}</p>
147+
{finding.dismissReason ? (
148+
<p className="mt-1 text-xs text-[var(--color-ink-faint)]">
149+
Dismissed: {finding.dismissReason}
150+
</p>
151+
) : null}
152+
{finding.quotedCode ? (
153+
<pre className="mt-3 overflow-x-auto rounded border border-[var(--color-border)] bg-[var(--color-surface-sunken)] p-3 text-xs">
154+
<code className="font-[family-name:var(--font-mono)]">{finding.quotedCode}</code>
155+
</pre>
156+
) : null}
157+
</Card>
158+
</li>
159+
))}
160+
</ul>
161+
);
162+
123163
return (
124164
<>
125165
<PageHeader
@@ -181,45 +221,19 @@ export default function ReviewPage({ params }: { params: Promise<{ id: string }>
181221
</ol>
182222
</Card>
183223

184-
<h2 className="mb-3 text-sm font-semibold">
185-
Findings{reported.length > 0 ? ` (${reported.length})` : ""}
186-
</h2>
187-
188-
{reported.length === 0 ? (
189-
<Empty title={snapshot.running ? "Nothing reported yet" : "No findings"}>
190-
{snapshot.running
191-
? "Findings appear once the adversarial pass has run and each one has been checked against the file."
192-
: "Every hunk was accounted for and nothing survived verification."}
193-
</Empty>
224+
{review.status === "awaiting_confirmation" && reported.length > 0 ? (
225+
<ConfirmationQueue
226+
reviewId={id}
227+
findings={findings}
228+
onChanged={() => void reload()}
229+
/>
194230
) : (
195-
<ul className="grid gap-3">
196-
{reported.map((finding) => (
197-
<li key={finding.id}>
198-
<Card className="p-4">
199-
<div className="flex flex-wrap items-center gap-2">
200-
<Badge tone={severityTone(finding.severity)}>{finding.severity}</Badge>
201-
{finding.status === "open_question" ? (
202-
<Badge tone="question">open question</Badge>
203-
) : null}
204-
<Mono className="text-xs text-[var(--color-ink-muted)]">
205-
{finding.filePath}:{finding.lineStart}
206-
</Mono>
207-
</div>
208-
<p className="mt-2 font-medium">{finding.issue}</p>
209-
<p className="mt-1 text-sm text-[var(--color-ink-muted)]">
210-
{finding.comment}
211-
</p>
212-
{finding.quotedCode ? (
213-
<pre className="mt-3 overflow-x-auto rounded border border-[var(--color-border)] bg-[var(--color-surface-sunken)] p-3 text-xs">
214-
<code className="font-[family-name:var(--font-mono)]">
215-
{finding.quotedCode}
216-
</code>
217-
</pre>
218-
) : null}
219-
</Card>
220-
</li>
221-
))}
222-
</ul>
231+
<>
232+
<h2 className="mb-3 text-sm font-semibold">
233+
Findings{reported.length > 0 ? ` (${reported.length})` : ""}
234+
</h2>
235+
{renderReadOnly()}
236+
</>
223237
)}
224238
</div>
225239

0 commit comments

Comments
 (0)