Skip to content

Commit d4fe387

Browse files
mchwangclaude
mchwang
and
claude
authored
Keep input made before the first review load, and explain a refused plan import (#172)
* Run review history work in an abortable worker thread ReviewService.load read Git history (synchronous Git children and an object-storage walk) and linked it on the HTTP server's event loop, for up to 30 s. During that time cancellation, shutdown, request timeouts and every other request were stuck. History reading and linking now run in one worker thread per load (runner/history-job.ts). readHistory spawns Git asynchronously and waits for each process to exit, so a cancel kills the running Git process; a worker that cannot read the cancel within 1 s is terminated. The load's deadline crosses threads as a relative budget counted from the worker's start. Store reads and writes stay on the server thread; a load whose inputs changed while the worker ran reads again, at most three reads in all. load and act are async, and a synchronous apply runs inside Store.userAction. Every load takes the request's or operation's abort signal. Merge admission, pre-merge preparation and merge replays recheck their trust and task generations after each new wait, in the same turn as the write they guard. A failure after a review command was applied now says outcomeUnknown, a load failure before it is not recorded under its action ID, and a question saved during shutdown still gets its retryable answer. Browser specs ran the server in the Playwright process and relied on the old blocking loads to sequence steps. They now wait for the page's review load or refresh, and the Playwright timeouts allow for the server's real Git work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make a remote read the last await before merge and preparation writes (#170 review) The worker-backed review load had become an await after the final GitHub read: merge admission could begin an attempt for a pull request that moved while the load ran, and pre-merge preparation could record readiness for a stale remote pair. Merge admission now inspects the pull request once more after its final review load, compares it with the previous pass, and only then checks local trust and the review's generations synchronously before the attempt. Preparation drops the review load that followed its final remote inspection; readiness uses the view loaded just before it, which the synchronous generation check after the inspection keeps current. Tests cover a pull request moving and the review changing during the final merge load, the load/inspect order in preparation, and trust or task changes during preparation's final load, each asserting the check that refuses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bound concurrent history workers and settle only after the thread exits (#170 review) Review loads no longer serialize on the server thread, so several tabs or quick refreshes could start any number of worker threads and Git processes. At most two history workers now run at once. A later job waits for a slot in arrival order; the wait counts against its budget, and a job aborted or out of time while it waits never starts. Each slot is held until its worker thread has ended. A rejected worker.terminate() no longer settles the job: it proves nothing about the thread, so the job waits for the thread's exit event. The gated Git test fixture numbers calls under a lock, so calls that start together get distinct numbers. The review deadline test moves its mocked clock only once the worker holds its budget, and its clock and environment stubs are restored in afterEach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Record the review lessons from moving review loads off the server thread (#170) Four rules for async work, from the findings on this pull request: recheck every same-turn guard after a new await and make the final remote validation the last await before an irreversible action; bound work that leaves a serializing path; never compare clocks across threads; and in tests that host the server, wait for observable state instead of relying on the server blocking the test's event loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Trim the new async rules to what existing rules do not already say (#170 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Say plainly which process the in-process test rule is about (#170 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep input made before the first review load, and explain a refused plan import Text typed into the composer before the first /api/review answer had no item to belong to, so applying the answer erased it (switching Ask/Request change erased it too). It is now kept under the unselected key and handed to the first item render() selects. "Import next revision" did nothing visible while no plan was loaded. It is now aria-disabled with no plan, and pressing it says why: still loading (written under the load's status ownership, so the load's result replaces it), or not loaded after a failure. The Plans screen shows "Loading plan…" during the first load and Refresh plan. The busy-write refusal is checked first, so an import pressed during a Retry Apply no longer hides the Apply's result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test that text typed before a failed first load reaches the first item after Refresh (#172 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Explain a refused plan import even when no file is chosen (#172 review) The file input's `required` let the browser's own check stop the form before importPlan ran, so with no plan loaded and no file chosen the page asked for a file instead of saying why it could not import. The form is now novalidate; importPlan already reports a missing file. The no-plan refusal test also checks its ✕ status colour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test an import with a plan loaded and no file; say which refusals keep ownership (#172 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Say exactly which import refusals keep status ownership (#172 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test text typed after the last mode switch; state why the file refusal claims status (#172 review) The draft tests switched modes before the load answered, which saved the text early, so they passed even without the save made when the response applies. They now type more after the last switch. The importPlan comment now says the file refusal always claims the status line, because feedback on the file just chosen is newer than any read already running (as "does not let an older failed Review refresh replace newer Plans validation feedback" already requires). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 99e58a9 commit d4fe387

5 files changed

Lines changed: 347 additions & 10 deletions

File tree

‎test/browser/first-load.spec.ts‎

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
import { test, expect, type Page } from '@playwright/test';
2+
import { mkdtempSync, rmSync } from 'node:fs';
3+
import { tmpdir } from 'node:os';
4+
import { join } from 'node:path';
5+
import { createDemo } from '../../scripts/demo.ts';
6+
import { startServer } from '../../web/server.ts';
7+
8+
// These tests act while the page's first review load is still pending, so they must not use openReview/reloadReview,
9+
// which wait for that load to finish.
10+
const LOADING = 'Linking changes to plan items…';
11+
12+
let root: string, app: Awaited<ReturnType<typeof startServer>>;
13+
test.beforeEach(async () => {
14+
root = mkdtempSync(join(tmpdir(), 'codeboost-first-load-browser-'));
15+
app = await startServer(createDemo(join(root, 'demo')), 0);
16+
});
17+
test.afterEach(async () => {
18+
await app.close();
19+
rmSync(root, { recursive: true, force: true });
20+
});
21+
22+
/** Opens the page with its first /api/review request held in the browser until the test calls `release`. */
23+
async function openWithHeldReview(page: Page, url: string, answer: 'pass' | 'fail' = 'pass') {
24+
let release!: () => void, arrived!: () => void;
25+
const held = new Promise<void>(resolve => { release = resolve; });
26+
const reached = new Promise<void>(resolve => { arrived = resolve; });
27+
await page.route('**/api/review', async route => {
28+
arrived();
29+
await held;
30+
if (answer === 'fail') await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'History unavailable.' }) });
31+
else await route.continue();
32+
}, { times: 1 });
33+
await page.goto(url, { waitUntil: 'commit' });
34+
await reached;
35+
await expect(page.locator('#banner')).toHaveText(LOADING);
36+
return release;
37+
}
38+
39+
// app.url carries the session token in its hash; the view goes in the query before it.
40+
function plansUrl() {
41+
const url = new URL(app.url);
42+
url.search = '?view=plans';
43+
return url.href;
44+
}
45+
46+
function countRequests(page: Page, path: string) {
47+
const seen: string[] = [];
48+
page.on('request', request => { if (new URL(request.url()).pathname === path) seen.push(request.method()); });
49+
return seen;
50+
}
51+
52+
const nextPlan = {
53+
schema_version: 1,
54+
issue: 3,
55+
revision: 2,
56+
summary: 'Make retry limits visible to operators',
57+
items: [{
58+
id: 'P1',
59+
title: 'Expose the retry ceiling',
60+
intent: 'Keep the bounded delay and make its ceiling explicit.',
61+
files: [{ path: 'retry.ts', kind: 'edit', renamed_from: null, change: 'Name the retry ceiling.' }],
62+
acceptance: [{ type: 'check', text: 'The delay never exceeds five seconds.' }],
63+
depends_on: [],
64+
}],
65+
questions: [],
66+
};
67+
68+
test('keeps question and change-request text typed before the first review load and gives it to the first item', async ({ page }) => {
69+
const errors: string[] = [];
70+
page.on('pageerror', error => errors.push(error.message));
71+
const actions = countRequests(page, '/api/action');
72+
const release = await openWithHeldReview(page, app.url);
73+
74+
await page.getByLabel('Question about this item').fill('Typed before the review loaded.');
75+
await page.getByRole('button', { name: 'Request change', exact: true }).click();
76+
await page.getByLabel('Change to request').fill('Change typed before the review loaded.');
77+
await page.getByRole('button', { name: 'Ask', exact: true }).click();
78+
await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded.');
79+
// Typed after the last mode switch: only the page's handling of the response can save this part.
80+
await page.getByLabel('Question about this item').press('End');
81+
await page.getByLabel('Question about this item').pressSequentially(' Still typing.');
82+
83+
release();
84+
await expect(page.locator('#banner')).not.toHaveText(LOADING, { timeout: 30_000 });
85+
await expect(page.locator('[data-select="P1"]')).toHaveAttribute('aria-current', 'true');
86+
await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded. Still typing.');
87+
await expect(page.getByLabel('Question about this item')).toBeFocused();
88+
await expect(page.getByText('Retained draft')).toHaveCount(0);
89+
await page.getByRole('button', { name: 'Request change', exact: true }).click();
90+
await expect(page.getByLabel('Change to request')).toHaveValue('Change typed before the review loaded.');
91+
await page.getByRole('button', { name: 'Ask', exact: true }).click();
92+
93+
// The text now belongs to P1: another item starts empty, and P1 still has it on return.
94+
await page.locator('[data-select="P2"]').click();
95+
await expect(page.getByLabel('Question about this item')).toHaveValue('');
96+
await page.locator('[data-select="P1"]').click();
97+
await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded. Still typing.');
98+
99+
expect(actions).toEqual([]);
100+
expect(app.service.store.getReviewNotes(app.service.config.identity)).toEqual([]);
101+
expect(errors).toEqual([]);
102+
});
103+
104+
test('keeps text typed before a failed first load and gives it to the first item when Refresh succeeds', async ({ page }) => {
105+
const errors: string[] = [];
106+
page.on('pageerror', error => errors.push(error.message));
107+
const release = await openWithHeldReview(page, app.url, 'fail');
108+
await page.getByLabel('Question about this item').fill('Typed before the failed load.');
109+
await page.getByRole('button', { name: 'Request change', exact: true }).click();
110+
await page.getByLabel('Change to request').fill('Change typed before the failed load.');
111+
await page.getByRole('button', { name: 'Ask', exact: true }).click();
112+
// Typed after the last mode switch, so no mode switch saved this part.
113+
await page.getByLabel('Question about this item').press('End');
114+
await page.getByLabel('Question about this item').pressSequentially(' Still typing.');
115+
116+
release();
117+
await expect(page.locator('#banner')).toContainText('Could not read this branch’s history.');
118+
await expect(page.getByLabel('Question about this item')).toBeDisabled();
119+
await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the failed load. Still typing.');
120+
121+
await page.getByRole('button', { name: 'Refresh', exact: true }).click();
122+
await expect(page.locator('[data-select="P1"]')).toHaveAttribute('aria-current', 'true', { timeout: 30_000 });
123+
await expect(page.getByLabel('Question about this item')).toBeEnabled();
124+
await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the failed load. Still typing.');
125+
await expect(page.getByText('Retained draft')).toHaveCount(0);
126+
await page.getByRole('button', { name: 'Request change', exact: true }).click();
127+
await expect(page.getByLabel('Change to request')).toHaveValue('Change typed before the failed load.');
128+
expect(app.service.store.getReviewNotes(app.service.config.identity)).toEqual([]);
129+
expect(errors).toEqual([]);
130+
});
131+
132+
test('refuses a plan import made before the first review load, says why, and imports once the plan appears', async ({ page }) => {
133+
const errors: string[] = [];
134+
page.on('pageerror', error => errors.push(error.message));
135+
const imports = countRequests(page, '/api/plan/import');
136+
const release = await openWithHeldReview(page, plansUrl());
137+
138+
await expect(page.locator('#plans-summary')).toHaveText('Loading plan…');
139+
await expect(page.locator('#plans-status')).toHaveText('Loading the current plan…');
140+
const importButton = page.getByRole('button', { name: 'Import next revision' });
141+
await expect(importButton).toHaveAttribute('aria-disabled', 'true');
142+
// aria-disabled keeps the control reachable: keyboard activation must explain the refusal, not do nothing. With no
143+
// file chosen yet, the browser's own file check must not answer instead.
144+
await importButton.focus();
145+
await page.keyboard.press('Enter');
146+
await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.');
147+
await expect(page.locator('#plans-status')).toHaveClass('warn');
148+
await expect(importButton).toBeFocused();
149+
await page.locator('#plans-status').evaluate(element => { element.textContent = ''; });
150+
await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) });
151+
await importButton.focus();
152+
await page.keyboard.press('Enter');
153+
await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.');
154+
await expect(importButton).toBeFocused();
155+
156+
release();
157+
await expect(page.locator('#plans-status')).toHaveText('✓ Revision r1 is current.', { timeout: 30_000 });
158+
await expect(page.locator('#plans-revision')).toHaveText('r1');
159+
await expect(importButton).not.toHaveAttribute('aria-disabled', 'true');
160+
expect(imports).toEqual([]);
161+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1);
162+
163+
// The file chosen during the load is still selected and imports normally now.
164+
await expect(page.locator('#plan-file-details')).toHaveText('plan.json · JSON');
165+
await importButton.click();
166+
await expect(page.locator('#plans-status')).toHaveText('✓ Revision r2 is current.', { timeout: 30_000 });
167+
expect(imports).toEqual(['POST']);
168+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2);
169+
expect(errors).toEqual([]);
170+
});
171+
172+
test('refuses a plan import after the first review load failed, and says why', async ({ page }) => {
173+
const errors: string[] = [];
174+
page.on('pageerror', error => errors.push(error.message));
175+
const imports = countRequests(page, '/api/plan/import');
176+
const release = await openWithHeldReview(page, plansUrl(), 'fail');
177+
release();
178+
await expect(page.locator('#plans-status')).toContainText('✕ Could not read this branch’s history. History unavailable.');
179+
await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable');
180+
await expect(page.locator('#plan-author-status')).toHaveText('Load a plan before asking the agent.');
181+
182+
const importButton = page.getByRole('button', { name: 'Import next revision' });
183+
await expect(importButton).toHaveAttribute('aria-disabled', 'true');
184+
await importButton.focus();
185+
await page.keyboard.press('Enter');
186+
await expect(page.locator('#plans-status')).toHaveText('✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import.');
187+
await expect(page.locator('#plans-status')).toHaveClass('bad');
188+
await expect(importButton).toBeFocused();
189+
await page.locator('#plans-status').evaluate(element => { element.textContent = ''; });
190+
await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) });
191+
await importButton.focus();
192+
await page.keyboard.press('Enter');
193+
await expect(page.locator('#plans-status')).toHaveText('✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import.');
194+
expect(imports).toEqual([]);
195+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1);
196+
expect(errors).toEqual([]);
197+
});
198+
199+
test('refuses a plan import during Refresh plan after a failed first load, and keeps the refresh result', async ({ page }) => {
200+
const errors: string[] = [];
201+
page.on('pageerror', error => errors.push(error.message));
202+
const imports = countRequests(page, '/api/plan/import');
203+
const release = await openWithHeldReview(page, plansUrl(), 'fail');
204+
release();
205+
await expect(page.locator('#plans-status')).toContainText('✕ Could not read this branch’s history.');
206+
207+
let releaseRefresh!: () => void, refreshArrived!: () => void;
208+
const heldRefresh = new Promise<void>(resolve => { releaseRefresh = resolve; });
209+
const refreshReached = new Promise<void>(resolve => { refreshArrived = resolve; });
210+
await page.route('**/api/review', async route => {
211+
refreshArrived();
212+
await heldRefresh;
213+
await route.continue();
214+
}, { times: 1 });
215+
await page.getByRole('button', { name: 'Refresh plan' }).click();
216+
await refreshReached;
217+
await expect(page.locator('#plans-summary')).toHaveText('Loading plan…');
218+
await expect(page.locator('#plan-author-status')).toHaveText('The plan is still loading. Ask the agent after it appears.');
219+
220+
const importButton = page.getByRole('button', { name: 'Import next revision' });
221+
await expect(importButton).toHaveAttribute('aria-disabled', 'true');
222+
await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) });
223+
await importButton.focus();
224+
await page.keyboard.press('Enter');
225+
await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.');
226+
227+
releaseRefresh();
228+
await expect(page.locator('#plans-status')).toHaveText('✓ Revision r1 is current.', { timeout: 30_000 });
229+
await expect(page.locator('#plans-revision')).toHaveText('r1');
230+
await expect(importButton).not.toHaveAttribute('aria-disabled', 'true');
231+
expect(imports).toEqual([]);
232+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1);
233+
expect(errors).toEqual([]);
234+
});

‎test/browser/plans-authoring.spec.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,55 @@ test('replays an ambiguous suggestion Apply, stales siblings, and refreshes befo
387387
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(3);
388388
});
389389

390+
test('refuses an import during a Retry Apply made after the review failed, and keeps the Apply result', async ({ page }) => {
391+
const { deps, invocations } = planning();
392+
await openPlans(page, deps);
393+
await page.getByLabel('Guidance').fill('Improve the retry item.');
394+
await page.getByRole('button', { name: 'Suggest edits' }).click();
395+
await expect.poll(() => invocations.length).toBe(1);
396+
invocations[0]!.resolve(JSON.stringify(suggestions()));
397+
await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(1);
398+
399+
// The first Apply commits but its answer is lost, so the result stays Apply-uncertain.
400+
let applies = 0, releaseRetry!: () => void, retryArrived!: () => void;
401+
const heldRetry = new Promise<void>(resolve => { releaseRetry = resolve; });
402+
const retryReached = new Promise<void>(resolve => { retryArrived = resolve; });
403+
await page.route('**/api/plan/suggestions/*/apply', async route => {
404+
applies++;
405+
if (applies === 1) { await route.fetch(); await route.abort('failed'); return; }
406+
retryArrived();
407+
await heldRetry;
408+
await route.continue();
409+
});
410+
await page.getByRole('button', { name: 'Apply this edit' }).click();
411+
await expect(page.getByRole('button', { name: 'Retry Apply', exact: true })).toBeVisible();
412+
413+
// A failed review refresh unloads the plan while Retry Apply stays available.
414+
await page.route('**/api/review', route => route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'History unavailable.' }) }), { times: 1 });
415+
await page.getByRole('link', { name: 'Review', exact: true }).click();
416+
await page.getByRole('button', { name: 'Refresh', exact: true }).click();
417+
await expect(page.locator('#banner')).toContainText('Could not read this branch’s history.');
418+
await page.getByRole('link', { name: 'Plans', exact: true }).click();
419+
await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable');
420+
421+
const imports: string[] = [];
422+
page.on('request', request => { if (new URL(request.url()).pathname === '/api/plan/import') imports.push(request.method()); });
423+
await page.getByRole('button', { name: 'Retry Apply', exact: true }).click();
424+
await retryReached;
425+
await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from('{}') });
426+
const importButton = page.getByRole('button', { name: 'Import next revision' });
427+
await importButton.focus();
428+
await page.keyboard.press('Enter');
429+
await expect(page.locator('#plans-status')).toHaveText('! Wait for the current review action before importing a plan.');
430+
431+
releaseRetry();
432+
await expect(page.locator('#plans-status')).toHaveText('✓ Revision r2 is current.');
433+
await expect(page.locator('#plans-revision')).toHaveText('r2');
434+
expect(imports).toEqual([]);
435+
expect(applies).toBe(2);
436+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2);
437+
});
438+
390439
test('retries an ambiguous suggestion start with the exact request', async ({ page }) => {
391440
const { deps, invocations } = planning();
392441
await openPlans(page, deps);

‎test/browser/plans.spec.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,21 @@ for (const fixture of [
120120
});
121121
}
122122

123+
test('explains an import pressed with no file chosen instead of leaving it to the browser', async ({ page }) => {
124+
const imports: string[] = [];
125+
page.on('request', request => { if (new URL(request.url()).pathname === '/api/plan/import') imports.push(request.method()); });
126+
await openReview(page, app.url);
127+
await page.getByRole('link', { name: 'Plans', exact: true }).click();
128+
await expect(page.locator('#plans-revision')).toHaveText('r1');
129+
const importButton = page.getByRole('button', { name: 'Import next revision' });
130+
await importButton.click();
131+
await expect(page.locator('#plans-status')).toHaveText('✕ Choose a JSON or YAML plan file.');
132+
await expect(page.locator('#plans-status')).toHaveClass('bad');
133+
await expect(importButton).toBeFocused();
134+
expect(imports).toEqual([]);
135+
expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1);
136+
});
137+
123138
test('shows a rejected import without replacing the current plan', async ({ page }) => {
124139
await openReview(page, app.url);
125140
await page.getByRole('link', { name: 'Plans', exact: true }).click();

0 commit comments

Comments
 (0)