Skip to content

Commit 1f1d54a

Browse files
committed
Put a ceiling on what a stage can spend, and say which engine answered
Work item V1 of the M3 plan: spend safety first, before any more surface area. Every engine call now carries --max-budget-usd from a new stageMaxBudgetUsd setting, default 15 USD-equivalent. The flag was already plumbed through the command builder and nothing had ever set it, so a runaway stage was bounded by nothing at all. Per call rather than per review, because that is what the CLI flag means. Zero disables it, which is a choice someone makes on purpose rather than a default they fall into. Every run also opens with a note naming the engine binary, the model and the effort. That comes straight from the incident on this machine: a manual UI test spent about 0.50 USD-equivalent because TRYSQUARE_CLAUDE_PATH did not reach the server process, and the only way I noticed was that the token counts looked like a real model rather than the fake's fixed numbers. A fake-versus- real mixup should be readable from the run, not deduced afterwards. Worktrees are now removed when a run settles cancelled or failed, which nothing did before, so they accumulated. They stay while a run is paused, interrupted or awaiting confirmation, because those will be read again: the confirmation screen reads file context from the worktree. Cleanup is best effort, and a failure to remove becomes a note rather than masking the outcome that actually matters. Two smaller defects from the shipped UI slice: the project name linked to /projects/[id], which is an empty directory and a 404, so it is plain text until V3 builds that page; and the review page grew its activity list without bound, so it keeps the last two hundred lines and leaves the archive to the event log. Three mutations checked and all caught: dropping the budget pass-through, keeping worktrees on cancel, and never writing the engine note.
1 parent e59e65d commit 1f1d54a

8 files changed

Lines changed: 157 additions & 18 deletions

File tree

docs/DECISIONS.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,3 +637,22 @@ verified evidence, in writing, here.
637637
and is fixed with the documented turbopackIgnore comment. Re-check
638638
trigger: if the app ever adopts output "standalone", this acceptance is
639639
void and the warning must be resolved.
640+
- 2026-07-31 DECIDED (V1, D-30): every engine call carries --max-budget-usd
641+
from the stageMaxBudgetUsd setting, default 15 USD-equivalent. Zero
642+
disables the flag, which is a choice someone makes on purpose rather than
643+
a default they fell into. Per call rather than per review, because the CLI
644+
flag is per call; a runaway stage is bounded even when nothing else goes
645+
wrong.
646+
- 2026-07-31 DECIDED (V1, D-31): nothing spends model usage without an
647+
explicit user action: starting a review, pressing a probe button, or
648+
running the demo without --fake. There is no automatic probing at startup
649+
or anywhere else. Reason: the 2026-07-31 incident where a manual test
650+
silently used the real CLI and spent about 0.50 USD-equivalent.
651+
- 2026-07-31 DECIDED (V1, D-32): every run opens with a run note naming the
652+
engine binary it will use, the model and the effort, so a fake-versus-real
653+
mixup is readable from the run itself instead of deduced from token counts.
654+
- 2026-07-31 DECIDED (V1, D-33): worktrees are removed when a run settles
655+
cancelled or failed, and kept while it is paused, interrupted or awaiting
656+
confirmation; complete joins the cleanup list when the confirmation flow
657+
lands (D-12 as implemented). Cleanup is best effort: a failure to remove
658+
becomes a run note, never a mask over the outcome that matters.

docs/plans/M3-FINISH-PLAN.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ confirms the wiring.
343343

344344
| V | Contents | Depends on | Status |
345345
| -- | ------------------------------------------------------------ | ---------- | ------ |
346-
| V1 | budget cap, engine note, no-auto-probe, worktree cleanup, 404 unlink, activity cap | - | |
346+
| V1 | budget cap, engine note, no-auto-probe, worktree cleanup, 404 unlink, activity cap | - | DONE |
347347
| V2 | confirm/dismiss/complete/context routes, confirmation UI, keyboard map | V1 | |
348348
| V3 | project detail page, fetch-now, links CRUD, project delete | V1 | |
349349
| V4 | preflight route and panel, linked toggle with suggestion | V3 | |

src/app/projects/page.tsx

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,16 +122,7 @@ export default function ProjectsPage() {
122122
<div className="flex flex-wrap items-start justify-between gap-3">
123123
<div className="min-w-0">
124124
<div className="flex items-center gap-2">
125-
{project.cloneStatus === "ready" ? (
126-
<Link
127-
href={`/projects/${project.id}`}
128-
className="font-medium hover:underline"
129-
>
130-
{project.name}
131-
</Link>
132-
) : (
133-
<span className="font-medium">{project.name}</span>
134-
)}
125+
<span className="font-medium">{project.name}</span>
135126
{project.cloneStatus === "pending" ? (
136127
<Badge tone="accent">cloning</Badge>
137128
) : project.cloneStatus === "failed" ? (

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,13 @@ export default function ReviewPage({ params }: { params: Promise<{ id: string }>
9191
detail?: string;
9292
};
9393

94-
if (parsed.kind === "stage")
95-
setLive((lines) => [...lines, `${parsed.stage} ${parsed.phase}`]);
96-
if (parsed.kind === "engine") setLive((lines) => [...lines, ` ${parsed.detail}`]);
97-
if (parsed.kind === "note") setLive((lines) => [...lines, `note: ${parsed.note?.message}`]);
94+
// Kept as a bounded tail: a long run emits thousands of engine lines,
95+
// and the page only ever shows the most recent of them. The archive is
96+
// the event log, not component state.
97+
const push = (line: string) => setLive((lines) => [...lines, line].slice(-200));
98+
if (parsed.kind === "stage") push(`${parsed.stage} ${parsed.phase}`);
99+
if (parsed.kind === "engine") push(` ${parsed.detail}`);
100+
if (parsed.kind === "note") push(`note: ${parsed.note?.message}`);
98101

99102
// Durable facts are written before they are announced, so a read here
100103
// always finds the row already changed.

src/server/db/repositories/settings.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ export function writeSetting(db: Db, key: string, value: unknown): void {
3333
export const SETTING_KEYS = {
3434
maxConcurrentReviews: "maxConcurrentReviews",
3535
stageTimeoutMinutes: "stageTimeoutMinutes",
36+
/** USD-equivalent ceiling per engine call. Zero disables the ceiling. */
37+
stageMaxBudgetUsd: "stageMaxBudgetUsd",
3638
defaultModel: "defaultModel",
3739
defaultEngineMode: "defaultEngineMode",
3840
} as const;

src/server/review/engine-runner.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,8 @@ export interface EngineRunnerOptions {
130130
timeoutMs: number;
131131
/** How hard the model is asked to think. Unset leaves it to the CLI. */
132132
effort?: string | undefined;
133+
/** USD-equivalent ceiling per CLI call. Unset passes no flag. */
134+
maxBudgetUsd?: number | undefined;
133135
directives: readonly ImportedDirective[];
134136
rules: readonly ImportedRule[];
135137
claudePath?: string | undefined;
@@ -182,6 +184,7 @@ export function createEngineRunner(options: EngineRunnerOptions): EngineRunner {
182184
model: options.model,
183185
outputFormat: "stream-json",
184186
...(options.effort === undefined ? {} : { effort: options.effort }),
187+
...(options.maxBudgetUsd === undefined ? {} : { maxBudgetUsd: options.maxBudgetUsd }),
185188
cwd: options.worktreeRoot,
186189
timeoutMs: options.timeoutMs,
187190
logPath: join(options.logsDir, `${request.stage}.log`),
@@ -235,6 +238,7 @@ export function createEngineRunner(options: EngineRunnerOptions): EngineRunner {
235238
model: options.model,
236239
outputFormat: "stream-json",
237240
...(options.effort === undefined ? {} : { effort: options.effort }),
241+
...(options.maxBudgetUsd === undefined ? {} : { maxBudgetUsd: options.maxBudgetUsd }),
238242
cwd: options.worktreeRoot,
239243
timeoutMs: options.timeoutMs,
240244
logPath: join(options.logsDir, `${request.stage}.repair.log`),

src/server/review/service.ts

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,15 @@ import { runReviewPipeline, type PipelineResult, type StageRequest } from "./pip
8787
/** Twenty minutes. A stage that reads a large change set is not quick. */
8888
const DEFAULT_STAGE_TIMEOUT_MINUTES = 20;
8989

90+
/**
91+
* The default USD-equivalent ceiling per engine call.
92+
*
93+
* High enough that no honest stage on a large change set hits it, low enough
94+
* that a runaway one is bounded. Zero in the setting disables the flag, which
95+
* is a choice someone has to make on purpose.
96+
*/
97+
const DEFAULT_STAGE_BUDGET_USD = 15;
98+
9099
export class ReviewNotRunnableError extends Error {
91100
constructor(
92101
readonly reviewId: string,
@@ -229,6 +238,16 @@ export async function prepareAndRun(
229238

230239
transitionReview(db, reviewId, "running", { currentStage: null });
231240

241+
// Which binary answers matters as much as which model: a fake-versus-real
242+
// mixup must be readable from the run itself, not deduced from token counts.
243+
const enginePath = options.claudePath ?? process.env.TRYSQUARE_CLAUDE_PATH;
244+
appendRunNote(db, reviewId, {
245+
kind: "note",
246+
message:
247+
`Engine: ${enginePath ?? "claude on PATH"}, model ${review.model}, ` +
248+
`effort ${review.effort}.`,
249+
});
250+
232251
try {
233252
const sides = sidesOf(review, project, linkedProject, dataDir, reviewId);
234253
if (sides.length === 2 && repoSlug(project.name) === repoSlug(linkedProject?.name ?? "")) {
@@ -337,7 +356,7 @@ export async function prepareAndRun(
337356
transitionReview(db, reviewId, "awaiting_confirmation", { currentStage: null });
338357
return { kind: "completed", result };
339358
} catch (error) {
340-
return recordFailure(db, reviewId, error, options.signal);
359+
return recordFailure(db, reviewId, dataDir, error, options.signal);
341360
}
342361
}
343362

@@ -431,6 +450,12 @@ function buildRunner(
431450
z.number().int().positive(),
432451
DEFAULT_STAGE_TIMEOUT_MINUTES,
433452
);
453+
const budget = readSettingOr(
454+
db,
455+
SETTING_KEYS.stageMaxBudgetUsd,
456+
z.number().nonnegative(),
457+
DEFAULT_STAGE_BUDGET_USD,
458+
);
434459

435460
const noteStage = (stage: ReviewStage, kind: "live" | "replayed"): void => {
436461
setCurrentStage(db, reviewId, stage);
@@ -452,6 +477,7 @@ function buildRunner(
452477
effort: reviewEffortSchema.parse(review.effort),
453478
directives: snapshot.directives,
454479
rules: snapshot.rules,
480+
...(budget > 0 ? { maxBudgetUsd: budget } : {}),
455481
...((options.claudePath ?? process.env.TRYSQUARE_CLAUDE_PATH)
456482
? { claudePath: options.claudePath ?? process.env.TRYSQUARE_CLAUDE_PATH }
457483
: {}),
@@ -500,12 +526,13 @@ function ensureVerifying(db: Db, reviewId: string): void {
500526
}
501527
}
502528

503-
function recordFailure(
529+
async function recordFailure(
504530
db: Db,
505531
reviewId: string,
532+
dataDir: string,
506533
error: unknown,
507534
signal: AbortSignal | undefined,
508-
): RunOutcome {
535+
): Promise<RunOutcome> {
509536
const message = error instanceof Error ? error.message : String(error);
510537
const errorClass = error instanceof StageFailedError ? error.errorClass : undefined;
511538
const logPath = error instanceof StageFailedError ? error.detail.logPath : undefined;
@@ -517,6 +544,7 @@ function recordFailure(
517544

518545
if (errorClass === "cancelled" || signal?.aborted === true) {
519546
settle(db, reviewId, "cancelled");
547+
await discardWorktrees(db, reviewId, dataDir);
520548
return { kind: "cancelled", reason: message };
521549
}
522550

@@ -525,9 +553,27 @@ function recordFailure(
525553
message: logPath ? `${message} The stage log is at ${logPath}.` : message,
526554
});
527555
settle(db, reviewId, "failed");
556+
await discardWorktrees(db, reviewId, dataDir);
528557
return { kind: "failed", reason: message, logPath };
529558
}
530559

560+
/**
561+
* Removes the checked-out copies after a run that will not resume (D-12:
562+
* cancelled and failed here, complete when the confirmation flow lands).
563+
* Best effort on purpose: a cleanup failure becomes a note, never a mask
564+
* over the outcome that actually matters.
565+
*/
566+
async function discardWorktrees(db: Db, reviewId: string, dataDir: string): Promise<void> {
567+
try {
568+
await removeReviewWorktrees(db, reviewId, dataDir);
569+
} catch (error) {
570+
appendRunNote(db, reviewId, {
571+
kind: "note",
572+
message: `The worktrees could not be removed: ${error instanceof Error ? error.message : String(error)}`,
573+
});
574+
}
575+
}
576+
531577
/**
532578
* Records how a run ended, without letting the recording throw.
533579
*

tests/server/review/service.test.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,11 @@ import {
2626
createReview,
2727
getReview,
2828
markOrphanedReviewsInterrupted,
29+
readRunNotes,
2930
requireReview,
3031
transitionReview,
3132
} from "@/server/db/repositories/reviews";
33+
import { SETTING_KEYS, writeSetting } from "@/server/db/repositories/settings";
3234
import { models, reviews } from "@/server/db/schema";
3335
import { recordProbeSuccess, registerCandidate } from "@/server/db/repositories/models";
3436
import { listLedgerFiles } from "@/server/db/repositories/ledger";
@@ -857,3 +859,75 @@ describe("telling the reviewer what the change was for", () => {
857859
expect(hashes[1]).not.toBe(before);
858860
}, 180_000);
859861
});
862+
863+
describe("what a run is allowed to spend", () => {
864+
it("caps every engine call at the configured budget", async () => {
865+
// The ceiling exists for the call nothing else bounds: a runaway stage.
866+
// Per call rather than per review, because the CLI flag is per call.
867+
const reviewId = seedReview();
868+
writeIdealAnswers();
869+
await run(reviewId);
870+
871+
const calls = recordedArgv();
872+
expect(calls).toHaveLength(5);
873+
for (const [index, args] of calls.entries()) {
874+
expect(args[args.indexOf("--max-budget-usd") + 1], `call ${index + 1}`).toBe("15");
875+
}
876+
}, 120_000);
877+
878+
it("omits the ceiling only when someone set it to zero on purpose", async () => {
879+
writeSetting(db, SETTING_KEYS.stageMaxBudgetUsd, 0);
880+
const reviewId = seedReview();
881+
writeIdealAnswers();
882+
await run(reviewId);
883+
884+
for (const args of recordedArgv()) expect(args).not.toContain("--max-budget-usd");
885+
}, 120_000);
886+
});
887+
888+
describe("what the run says about itself", () => {
889+
it("records which engine binary answered, with the model and effort", async () => {
890+
// A fake-versus-real mixup must be readable from the run itself, not
891+
// deduced from token counts afterwards.
892+
const reviewId = seedReview();
893+
writeIdealAnswers();
894+
await run(reviewId);
895+
896+
const note = readRunNotes(requireReview(db, reviewId)).find((entry) =>
897+
entry.message.startsWith("Engine:"),
898+
);
899+
expect(note?.message).toContain("fake-claude.mjs");
900+
expect(note?.message).toContain("claude-fable-5[1m]");
901+
expect(note?.message).toContain("effort high");
902+
}, 120_000);
903+
});
904+
905+
describe("what is left on disk after a run stops", () => {
906+
it("removes the worktrees when a run is cancelled, and keeps the evidence", async () => {
907+
// D-12: cancelled will not resume, so the checkout goes; the bundle and
908+
// logs stay, because a stopped run is exactly when someone reads them.
909+
const reviewId = seedReview();
910+
writeIdealAnswers();
911+
const controller = new AbortController();
912+
await run(reviewId, {
913+
signal: controller.signal,
914+
onStageLifecycle: (event: StageLifecycleEvent) => {
915+
if (event.stage === "s2_comprehension") controller.abort();
916+
},
917+
});
918+
919+
expect(requireReview(db, reviewId).status).toBe("cancelled");
920+
expect(existsSync(worktreeRootDir(dataDir, reviewId))).toBe(false);
921+
expect(existsSync(join(bundleDir(dataDir, reviewId), "inventory.json"))).toBe(true);
922+
}, 120_000);
923+
924+
it("keeps the worktrees while a run is paused, because it will resume", async () => {
925+
const reviewId = seedReview();
926+
writeIdealAnswers();
927+
process.env.FAKE_CLAUDE_FAIL_AT = "3";
928+
await run(reviewId);
929+
930+
expect(requireReview(db, reviewId).status).toBe("paused_limit");
931+
expect(existsSync(worktreeRootDir(dataDir, reviewId))).toBe(true);
932+
}, 120_000);
933+
});

0 commit comments

Comments
 (0)