From 1ad5aaf6192e33433613b74308e8a91f35cfdc8b Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 14:11:32 -0700 Subject: [PATCH 01/18] Add G1 planning screen --- docs/implementation/planning-screen.md | 21 ++ test/browser/plans.spec.ts | 413 +++++++++++++++++++++++++ web/public/app.js | 249 +++++++++++++-- web/public/index.html | 32 +- web/public/style.css | 117 +++++++ 5 files changed, 813 insertions(+), 19 deletions(-) create mode 100644 docs/implementation/planning-screen.md create mode 100644 test/browser/plans.spec.ts diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md new file mode 100644 index 00000000..eaddd838 --- /dev/null +++ b/docs/implementation/planning-screen.md @@ -0,0 +1,21 @@ +# Planning screen + +**Lane and step:** lane G, step G1. Tracking issue: #145. + +G1 adds a Plans screen over the planning contracts already owned by lanes E and F. It does not add another plan writer: the browser imports through `POST /api/plan/import`, then reloads `GET /api/review` so the current Store revision remains the only displayed authority. + +## Display contract + +The screen renders the complete current plan: issue, summary, revision, open questions, ordered items, dependencies, files (including both sides of a rename) and acceptance checks. Navigation uses `?view=plans`, announces the active destination with `aria-current`, and preserves the existing Review composer DOM while the Plans screen is open. + +## Import contract + +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request, even when a refresh has since observed that request's commit. A definite refusal clears it. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. + +The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. + +## Acceptance + +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, stale merge-queue poll suppression after full Plans refresh and import reloads, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. + +G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts new file mode 100644 index 00000000..a44ce334 --- /dev/null +++ b/test/browser/plans.spec.ts @@ -0,0 +1,413 @@ +import { test, expect } from '@playwright/test'; +import { chmodSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createDemo } from '../../scripts/demo.ts'; +import { startServer } from '../../web/server.ts'; +import type { MergeGateway, MergeQueueGateway } from '../../github/merge.ts'; +import { fixtureGit } from '../fixtures/git.ts'; + +let root: string, app: Awaited>; +test.beforeEach(async () => { + root = mkdtempSync(join(tmpdir(), 'codeboost-plans-browser-')); + app = await startServer(createDemo(join(root, 'demo')), 0); +}); +test.afterEach(async () => { + await app.close(); + rmSync(root, { recursive: true, force: true }); +}); + +const nextPlan = { + schema_version: 1, + issue: 3, + revision: 2, + summary: 'Make retry limits visible to operators', + items: [ + { + id: 'P1', + title: 'Expose the retry ceiling', + intent: 'Keep the bounded delay and make its ceiling explicit.', + files: [{ path: 'retry.ts', kind: 'edit', renamed_from: null, change: 'Name the retry ceiling.' }], + acceptance: [{ type: 'check', text: 'The delay never exceeds five seconds.' }], + depends_on: [], + }, + { + id: 'P2', + title: 'Document the named limit', + intent: 'Explain the operator-visible retry ceiling.', + files: [{ path: 'RETRYING.md', kind: 'rename', renamed_from: 'README.md', change: 'Rename and update the retry guide.' }], + acceptance: [{ type: 'check', text: 'The documentation names the five-second limit.' }], + depends_on: ['P1'], + }, + ], + questions: ['Should operators be able to lower the ceiling?'], +} as const; + +const yaml = `schema_version: 1 +issue: 3 +revision: 2 +summary: Make retry limits visible to operators +items: + - id: P1 + title: Expose the retry ceiling + intent: Keep the bounded delay and make its ceiling explicit. + files: + - path: retry.ts + kind: edit + renamed_from: null + change: Name the retry ceiling. + acceptance: + - type: check + text: The delay never exceeds five seconds. + depends_on: [] + - id: P2 + title: Document the named limit + intent: Explain the operator-visible retry ceiling. + files: + - path: RETRYING.md + kind: rename + renamed_from: README.md + change: Rename and update the retry guide. + acceptance: + - type: check + text: The documentation names the five-second limit. + depends_on: [P1] +questions: + - Should operators be able to lower the ceiling? +`; + +for (const fixture of [ + { format: 'JSON', name: 'plan.json', mimeType: 'application/json', source: JSON.stringify(nextPlan) }, + { format: 'YAML', name: 'plan.yaml', mimeType: 'application/yaml', source: yaml }, +]) { + test(`displays the current plan and imports the next ${fixture.format} revision without losing a review draft`, async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + await page.goto(app.url); + await page.getByLabel('Question about this item').fill('Keep this unsent question.'); + + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page).toHaveURL(/\?view=plans$/); + await expect(page.getByRole('link', { name: 'Plans', exact: true })).toHaveAttribute('aria-current', 'page'); + await expect(page.getByRole('heading', { name: 'Make retries predictable and document the behavior' })).toBeVisible(); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + await expect(page.getByRole('article', { name: 'Bound exponential retries' })).toContainText('retry.ts'); + await expect(page.getByRole('article', { name: 'Document retry behavior' })).toContainText('Depends on P1'); + + await page.getByLabel('Plan file').setInputFiles({ name: fixture.name, mimeType: fixture.mimeType, buffer: Buffer.from(fixture.source) }); + await expect(page.locator('#plan-file-details')).toContainText(`${fixture.name} · ${fixture.format}`); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.getByRole('heading', { name: 'Make retry limits visible to operators' })).toBeVisible(); + await expect(page.getByRole('heading', { name: 'Open questions' })).toBeVisible(); + await expect(page.getByText('Should operators be able to lower the ceiling?')).toBeVisible(); + const renamed = page.getByRole('article', { name: 'Document the named limit' }); + await expect(renamed).toContainText('Depends on P1'); + await expect(renamed.getByText('README.md')).toBeVisible(); + await expect(renamed.getByText('RETRYING.md')).toBeVisible(); + + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.getByLabel('Question about this item')).toHaveValue('Keep this unsent question.'); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.reload(); + await expect(page).toHaveURL(/\?view=plans$/); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + if (fixture.format === 'JSON') await page.screenshot({ path: 'test-results/plans-desktop.png', fullPage: true }); + expect(errors).toEqual([]); + }); +} + +test('shows a rejected import without replacing the current plan', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const wrongIssue = JSON.stringify({ ...nextPlan, issue: 413 }); + await page.getByLabel('Plan file').setInputFiles({ name: 'wrong-issue.json', mimeType: 'application/json', buffer: Buffer.from(wrongIssue) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toContainText('selected issue'); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); +}); + +test('keeps focus and a newly selected file when an older import response returns', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const file = page.getByLabel('Plan file'); + await file.setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + let requests = 0; + await page.route('**/api/plan/import', async route => { + requests++; + arrived(); + await held; + await route.continue(); + }); + const submit = page.locator('#plan-import'); + await submit.focus(); + await submit.evaluate((button: HTMLButtonElement) => { button.click(); button.click(); }); + await captured; + await expect(submit).toBeFocused(); + await expect(submit).toHaveAttribute('aria-disabled', 'true'); + await file.setInputFiles({ name: 'review-later.yaml', mimeType: 'application/yaml', buffer: Buffer.from(yaml) }); + release(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await expect(page.locator('#plan-file-details')).toContainText('review-later.yaml · YAML'); + expect(requests).toBe(1); +}); + +test('reuses the exact import action after an ambiguous response', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + const sent: unknown[] = []; + await page.route('**/api/plan/import', async route => { + sent.push(route.request().postDataJSON()); + if (sent.length === 1) await route.abort('failed'); + else await route.continue(); + }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toBeVisible(); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + expect(sent).toHaveLength(2); + expect(sent[1]).toEqual(sent[0]); +}); + +test('replays an ambiguous committed import after refresh without creating another revision', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + const sent: unknown[] = []; + await page.route('**/api/plan/import', async route => { + sent.push(route.request().postDataJSON()); + if (sent.length === 1) { + await route.fetch(); + await route.abort('failed'); + return; + } + await route.continue(); + }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toBeVisible(); + await page.locator('#plans-refresh').click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + expect(sent).toHaveLength(2); + expect(sent[1]).toEqual(sent[0]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + +test('reports a committed import separately when reloading the plan fails', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await page.route('**/api/review', route => route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Reload unavailable.' }) })); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 was imported' })).toContainText('could not reload'); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + +test('does not let an older Review refresh overwrite a committed import', async ({ page }) => { + await page.goto(app.url); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + let reads = 0; + await page.route('**/api/review', async route => { + reads++; + if (reads !== 1) { await route.continue(); return; } + const old = await route.fetch(); + arrived(); + await held; + await route.fulfill({ response: old }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await captured; + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: 'Approve P1', exact: true }).click(); + await expect(page.getByText('1 of 2 approved')).toBeVisible(); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + release(); + await expect(page.locator('#reload')).toHaveText('Refresh'); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + +test('does not let an older Plans refresh re-enable merge after it commits', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + const gateway: MergeGateway = { + inspect: async () => { + const snapshot = app.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: false, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + }; + app = await startServer(config, 0, undefined, gateway); + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Merge PR', exact: true })).toBeEnabled(); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { + const old = await route.fetch(); + arrived(); + await held; + await route.fulfill({ response: old }); + }); + await page.locator('#plans-refresh').click(); + await captured; + await page.getByRole('link', { name: 'Review', exact: true }).click(); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await expect(page.locator('#banner')).toContainText('Merge submitted.'); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.locator('#plans-refresh')).toHaveText('Refresh plan'); + await expect(page.locator('#plans-status')).toBeEmpty(); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + release(); + await expect(page.locator('#plans-refresh')).toHaveText('Refresh plan'); + await expect(page.locator('#merge')).toBeDisabled(); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.locator('#plans-status')).toBeEmpty(); + expect(app.service.store.getMergeAttempt(config.identity)?.state).toBe('merged'); +}); + +test('keeps merge-queue polling current across Plans refresh and import outcomes', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-07T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + + let release!: () => void, releaseNew!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const heldNew = new Promise(resolve => { releaseNew = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + let polls = 0; + await page.route('**/api/merge', async route => { + polls++; + if (polls === 1) { + arrived(); + await held; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + state: 'removed', reviewedHead: view.snapshot.head, url: null, reason: 'Old poll result.', phase: null, + position: null, occurredAt: '2026-10-07T08:05:00Z', retryable: true, + } }) }); + return; + } + await heldNew; + await route.continue(); + }); + await page.goto(app.url); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await captured; + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r1 is current.' })).toBeVisible(); + const staleResponse = page.waitForResponse(response => response.url().endsWith('/api/merge')); + release(); + try { + await staleResponse; + await page.waitForTimeout(100); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.locator('#banner')).not.toContainText('Old poll result.'); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + } finally { + releaseNew(); + } + const pollsBeforeRefusal = polls; + const resumedPollResponse = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const wrongIssue = JSON.stringify({ ...nextPlan, issue: 413 }); + await page.getByLabel('Plan file').setInputFiles({ name: 'wrong-issue.json', mimeType: 'application/json', buffer: Buffer.from(wrongIssue) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toBeVisible(); + await expect.poll(() => polls, { timeout: 10000 }).toBeGreaterThan(pollsBeforeRefusal); + await resumedPollResponse; + + await page.unroute('**/api/merge'); + const authoritative = await (await fetch(new URL('/api/review', app.url), { headers: { 'x-codeboost-token': app.token } })).json(); + authoritative.plan = nextPlan; + authoritative.merge = { ...authoritative.merge, ready: false, action: null, queue: null, + blockers: [{ code: 'plan-changed', message: 'The plan changed.' }] }; + let releaseImportResponse!: () => void, importRequestArrived!: () => void; + const heldImportResponse = new Promise(resolve => { releaseImportResponse = resolve; }); + const capturedImportRequest = new Promise(resolve => { importRequestArrived = resolve; }); + await page.route('**/api/plan/import', async route => { + importRequestArrived(); + await heldImportResponse; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ result: { revision: 2 } }) }); + }); + let releaseReload!: () => void, reloadArrived!: () => void; + const heldReload = new Promise(resolve => { releaseReload = resolve; }); + const capturedReload = new Promise(resolve => { reloadArrived = resolve; }); + await page.route('**/api/review', async route => { + reloadArrived(); + await heldReload; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(authoritative) }); + }); + let releaseImportPoll!: () => void, importPollArrived!: () => void; + const heldImportPoll = new Promise(resolve => { releaseImportPoll = resolve; }); + const capturedImportPoll = new Promise(resolve => { importPollArrived = resolve; }); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await capturedImportRequest; + await page.route('**/api/merge', async route => { + importPollArrived(); + await heldImportPoll; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + state: 'removed', reviewedHead: view.snapshot.head, url: null, reason: 'Old import poll result.', phase: null, + position: null, occurredAt: '2026-10-07T08:10:00Z', retryable: true, + } }) }); + }); + releaseImportResponse(); + await capturedReload; + await capturedImportPoll; + releaseReload(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + const staleImportResponse = page.waitForResponse(response => response.url().endsWith('/api/merge')); + releaseImportPoll(); + await staleImportResponse; + await page.waitForTimeout(100); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.locator('#banner')).not.toContainText('Old import poll result.'); +}); diff --git a/web/public/app.js b/web/public/app.js index 0c499655..5f0cf5ea 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -19,7 +19,9 @@ let data, mode = "question", // The reviewer's explicit view choice, held only for the item and server-named stale state (staleKey) it was made on; otherwise a stale item opens the comparison. sinceChoice = null, - busy = false; + busy = false, + busyKind = null, + busyGeneration = 0; let reviewGeneration = 0; let view = "review"; let mergeGeneration = 0, @@ -41,6 +43,17 @@ const statusClass = (text) => : text.startsWith("!") ? "warn" : "neutral"; +function beginBusy(kind) { + busy = true; + busyKind = kind; + return ++busyGeneration; +} +function endBusy(owner) { + if (owner !== busyGeneration) return; + busy = false; + busyKind = null; + renderAttachment(); +} async function api(path, body) { const response = await fetch(path, { method: body ? "POST" : "GET", @@ -91,9 +104,10 @@ function showFailure(message) { '

Review could not load

Refresh after resolving the error above.

'; } async function refresh() { - if (busy) return; - busy = true; - reviewGeneration++; + if (busy || planImportPending) return; + supersedePlanRefresh(); + const busyOwner = beginBusy("read"); + const generation = ++reviewGeneration; mergeGeneration++; mergePollState = null; mergePollDelay = 2000; @@ -102,19 +116,20 @@ async function refresh() { try { rememberDraft(); const updated = await api("/api/review"); + if (generation !== reviewGeneration) return; rememberDraft(); data = updated; snippetSelection = null; selected ??= data.items[0]?.id || "Unplanned"; render(); } catch (error) { + if (generation !== reviewGeneration) return; rememberDraft(); showFailure( `Could not read this branch’s history. ${error.message} Use Refresh to retry.`, ); } finally { - busy = false; - renderAttachment(); + endBusy(busyOwner); } } function renderMerge() { @@ -186,9 +201,10 @@ async function pollMergeQueue(generation) { } } async function act(command) { - if (busy || !data) return false; - busy = true; - reviewGeneration++; + if (busy || planImportPending || !data) return false; + supersedePlanRefresh(); + const busyOwner = beginBusy("write"); + const generation = ++reviewGeneration; mergeGeneration++; mergePollState = null; mergePollDelay = 2000; @@ -197,17 +213,18 @@ async function act(command) { rememberDraft(); // One action ID per user action: the server replays it exactly and records feedback with it. const updated = await api("/api/action", { ...command, token: data.token, actionId: crypto.randomUUID() }); + if (generation !== reviewGeneration) return true; rememberDraft(); data = updated; render(); return true; } catch (error) { + if (generation !== reviewGeneration) return false; rememberDraft(); showFailure(`${error.message} Refresh to review the latest state.`); return false; } finally { - busy = false; - renderAttachment(); + endBusy(busyOwner); } } const showSince = (item) => @@ -233,6 +250,7 @@ function render() { return render(); } $("repository").textContent = data.repository; + renderPlans(); $("issue").textContent = `#${data.plan.issue} ${data.plan.summary} · r${data.plan.revision}`; $("progress").textContent = @@ -468,8 +486,10 @@ $("merge-details").onclick = () => { ); }; $("merge").onclick = async () => { - if (busy || !data?.merge?.ready || !window.confirm(data.merge.action === "retry" ? "Retry merging this exact reviewed head?" : "Merge this reviewed pull request?")) return; - busy = true; + if (busy || planImportPending || !data?.merge?.ready || !window.confirm(data.merge.action === "retry" ? "Retry merging this exact reviewed head?" : "Merge this reviewed pull request?")) return; + supersedePlanRefresh(); + const busyOwner = beginBusy("write"); + reviewGeneration++; mergeGeneration++; mergePollState = null; mergePollDelay = 2000; @@ -494,7 +514,7 @@ $("merge").onclick = async () => { render(); $("banner").textContent = `Merge blocked. ${error.message}`; } finally { - busy = false; + endBusy(busyOwner); } }; $("review-link").onclick = (event) => { @@ -810,17 +830,166 @@ let issuesGeneration = 0, issuesLoading = false, issuesRefreshError = null, issueErrorOrder = 0; +let planImportPending = false, + planImportRetry = null, + planRefreshPending = false, + planOperationGeneration = 0, + planStatusGeneration = 0; +function claimPlanStatus(className, text) { + const owner = ++planStatusGeneration; + $("plans-status").className = className; + $("plans-status").textContent = text; + return owner; +} +function setPlanStatus(owner, className, text) { + if (owner !== planStatusGeneration) return; + $("plans-status").className = className; + $("plans-status").textContent = text; +} +function supersedePlanRefresh() { + if (!planRefreshPending) return; + planOperationGeneration++; + planRefreshPending = false; + $("plans-refresh").removeAttribute("aria-disabled"); + $("plans-refresh").textContent = "Refresh plan"; + claimPlanStatus("neutral", ""); +} +function renderPlans() { + if (!data?.plan) { + $("plans-summary").textContent = "Plan unavailable"; + $("plans-revision").textContent = ""; + $("plans-items").innerHTML = ""; + $("plans-questions").innerHTML = ""; + return; + } + const plan = data.plan; + $("plans-identity").textContent = `#${plan.issue}`; + $("plans-summary").textContent = plan.summary; + $("plans-revision").textContent = `r${plan.revision}`; + $("plans-questions").innerHTML = plan.questions.length + ? `

Open questions

    ${plan.questions.map((question) => `
  • ${esc(question)}
    • `).join("")}
` + : ""; + $("plans-items").innerHTML = plan.items.map((item) => ` +
+
${esc(item.id)}

${esc(item.title)}

+

${esc(item.intent)}

+ ${item.depends_on.length ? `

Depends on ${item.depends_on.map((dependency) => `${esc(dependency)}`).join(" ")}

` : ""} +
+

Files

    ${item.files.map((file) => `
  • ${file.kind === "rename" ? `${esc(file.renamed_from)}` : ""}${esc(file.path)}${esc(file.kind)} · ${esc(file.change)}
    • `).join("")}
+

Acceptance

    ${item.acceptance.map((check) => `
  • ${esc(check.type)}${esc(check.text)}
    • `).join("")}
+
+
`).join(""); +} +function planFileFormat(name) { + return /\.ya?ml$/i.test(name) ? "yaml" : /\.json$/i.test(name) ? "json" : null; +} +function renderPlanFile() { + const file = $("plan-file").files[0]; + const format = file && planFileFormat(file.name); + $("plan-file-details").textContent = file + ? `${file.name} · ${format ? format.toUpperCase() : "Choose a .json, .yaml, or .yml file"}` + : "No file selected"; +} +async function importPlan(event) { + event.preventDefault(); + if (planImportPending || !data?.plan) return; + if (busy && busyKind !== "read") { + claimPlanStatus("warn", "! Wait for the current review action before importing a plan."); + return; + } + const file = $("plan-file").files[0], format = file && planFileFormat(file.name); + if (!file || !format) { + claimPlanStatus("bad", "✕ Choose a JSON or YAML plan file."); + return; + } + supersedePlanRefresh(); + planOperationGeneration++; + const sharedGeneration = ++reviewGeneration; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; + const busyOwner = beginBusy("write"); + planImportPending = true; + $("plan-import").setAttribute("aria-disabled", "true"); + $("plan-import").textContent = "Importing…"; + const statusOwner = claimPlanStatus("neutral", "Reading the plan file…"); + let source; + try { + source = await file.text(); + } catch (error) { + planImportPending = false; + endBusy(busyOwner); + $("plan-import").removeAttribute("aria-disabled"); + $("plan-import").textContent = "Import next revision"; + setPlanStatus(statusOwner, "bad", `✕ Could not read the plan file. ${error.message}`); + renderMerge(); + return; + } + const expectedRevision = data.plan.revision; + const retained = planImportRetry; + const request = retained?.source === source && retained.format === format + ? retained : { source, format, expectedRevision, actionId: crypto.randomUUID() }; + planImportRetry = request; + setPlanStatus(statusOwner, "neutral", "Importing the next revision…"); + try { + const response = await api("/api/plan/import", request); + const revision = Number.isSafeInteger(response.result?.revision) ? response.result.revision : null; + planImportRetry = null; + if (data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, + blockers: [{ code: "plan-changed", message: "The plan changed. Refresh before merging." }] } }; + renderMerge(); + setPlanStatus(statusOwner, revision === null ? "warn" : "good", revision === null + ? "! The import returned success without a revision. Reloading the current plan…" + : `✓ Imported revision r${revision}. Reloading…`); + try { + rememberDraft(); + const updated = await api("/api/review"); + if (sharedGeneration !== reviewGeneration) return; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; + rememberDraft(); + data = updated; + render(); + if ($("plan-file").files[0] === file) $("plan-file").value = ""; + renderPlanFile(); + setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); + } catch (error) { + setPlanStatus(statusOwner, "warn", `! ${revision === null ? "The import returned success" : `Revision r${revision} was imported`}, but the current plan could not reload. ${error.message}`); + renderMerge(); + } + } catch (error) { + const ambiguous = !Number.isSafeInteger(error?.status) || error.status === 503 || error.outcomeUnknown === true; + if (!ambiguous) planImportRetry = null; + if (ambiguous && data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, + blockers: [{ code: "plan-import-unknown", message: "The plan import outcome is unknown. Refresh before merging." }] } }; + setPlanStatus(statusOwner, "bad", `✕ Could not import the plan. ${error.message}`); + renderMerge(); + } finally { + planImportPending = false; + endBusy(busyOwner); + $("plan-import").removeAttribute("aria-disabled"); + $("plan-import").textContent = "Import next revision"; + } +} function showView(next) { view = next; $("review-view").hidden = next !== "review"; $("issues-view").hidden = next !== "issues"; - for (const [id, name] of [["review-link", "review"], ["issues-link", "issues"]]) { + $("plans-view").hidden = next !== "plans"; + for (const [id, name] of [["review-link", "review"], ["issues-link", "issues"], ["plans-link", "plans"]]) { if (name === next) $(id).setAttribute("aria-current", "page"); else $(id).removeAttribute("aria-current"); } - document.title = `${next === "issues" ? "Issues" : "Review"} · codeboost`; - history.replaceState(null, "", next === "issues" ? "/?view=issues" : "/"); + const title = next === "issues" ? "Issues" : next === "plans" ? "Plans" : "Review"; + document.title = `${title} · codeboost`; + history.replaceState(null, "", next === "review" ? "/" : `/?view=${next}`); if (next === "issues" && !issuesRequested) loadIssues(); + if (next === "plans") renderPlans(); } const issueTime = (value) => esc(new Date(value).toLocaleString()); function issueStatus() { @@ -991,11 +1160,55 @@ $("issues-link").onclick = (event) => { event.preventDefault(); showView("issues"); }; +$("plans-link").onclick = (event) => { + event.preventDefault(); + showView("plans"); +}; +$("plans-refresh").onclick = async () => { + if (planRefreshPending || planImportPending) return; + if (busy) return; + const generation = ++planOperationGeneration; + const sharedGeneration = ++reviewGeneration; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; + planRefreshPending = true; + $("plans-refresh").setAttribute("aria-disabled", "true"); + $("plans-refresh").textContent = "Refreshing…"; + const statusOwner = claimPlanStatus("neutral", "Refreshing the current plan…"); + try { + rememberDraft(); + const updated = await api("/api/review"); + if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; + rememberDraft(); + data = updated; + render(); + setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); + } catch (error) { + if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; + setPlanStatus(statusOwner, "bad", `✕ Could not refresh the current plan. ${error.message}`); + renderMerge(); + } finally { + const current = generation === planOperationGeneration && sharedGeneration === reviewGeneration; + if (!current) + setPlanStatus(statusOwner, "neutral", ""); + if (generation === planOperationGeneration) { + planRefreshPending = false; + $("plans-refresh").removeAttribute("aria-disabled"); + $("plans-refresh").textContent = "Refresh plan"; + } + } +}; +$("plan-file").onchange = renderPlanFile; +$("plan-import-form").onsubmit = importPlan; $("issues-refresh").onclick = () => loadIssues(); $("issues-list").onclick = (event) => { const button = event.target.closest("button.issue-trust"); if (button) changeIssueTrust(button); }; -showView(new URLSearchParams(location.search).get("view") === "issues" ? "issues" : "review"); +const initialView = new URLSearchParams(location.search).get("view"); +showView(["issues", "plans"].includes(initialView) ? initialView : "review"); await refresh(); diff --git a/web/public/index.html b/web/public/index.html index 2e883cff..6deb37cd 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -41,7 +41,7 @@

A little more room to review

> @@ -61,6 +61,36 @@

A little more room to review

+
Review Loading…
diff --git a/web/public/style.css b/web/public/style.css index dc2363cf..f1ce00da 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -60,6 +60,20 @@ textarea, select { font: inherit; } +input[type="file"] { + width: 100%; + color: var(--muted); +} +input[type="file"]::file-selector-button { + min-height: 28px; + margin-right: 8px; + border: 1px solid var(--line); + border-radius: 4px; + background: var(--raised); + color: var(--text); + padding: 4px 10px; + cursor: pointer; +} button, select { min-height: 28px; @@ -714,6 +728,109 @@ body.resizing-conversation { cursor: col-resize; user-select: none; } margin-left: auto; overflow-wrap: anywhere; } +#plans-status { + margin-left: auto; + overflow-wrap: anywhere; +} +.plans-workspace { + display: grid; + grid-template-columns: minmax(0, 1fr) 360px; + min-height: 0; + flex: 1; + overflow: hidden; +} +.plan-document { + overflow: auto; + padding: 24px; +} +.plan-document-heading { + display: flex; + align-items: flex-start; + gap: 16px; + padding-bottom: 16px; + border-bottom: 1px solid var(--line); +} +.plan-document-heading > div { + flex: 1; + min-width: 0; +} +.revision-badge { + border: 1px solid var(--line); + color: var(--muted); + padding: 4px 8px; +} +.plan-questions { + border-bottom: 1px solid var(--line); + padding: 12px 0; +} +.plan-card { + padding: 20px 0; + border-bottom: 1px solid var(--line); +} +.plan-card > header { + display: flex; + align-items: baseline; + gap: 12px; +} +.plan-card h2, +.plan-card h3 { + margin: 0; + font-weight: 500; +} +.plan-card h3 { + color: var(--muted); + font-size: 12px; + text-transform: uppercase; +} +.plan-dependencies code { + margin-left: 8px; + color: var(--text); +} +.plan-card-grid { + display: grid; + grid-template-columns: 1fr 1fr; + gap: 24px; + margin-top: 16px; +} +.plan-detail-list { + list-style: none; + padding: 0; + margin: 8px 0 0; +} +.plan-detail-list li { + display: grid; + grid-template-columns: minmax(88px, max-content) 1fr; + gap: 12px; + padding: 6px 0; + border-top: 1px solid var(--line); +} +.plan-detail-list span { + overflow-wrap: anywhere; +} +.plan-file-path { + display: flex; + gap: 8px; +} +.plan-import { + overflow: auto; + border-left: 1px solid var(--line); + background: var(--surface); +} +#plan-import-form { + padding: 16px; +} +#plan-import-form label { + display: block; + margin: 20px 0 8px; + font-weight: 500; +} +#plan-file-details { + overflow-wrap: anywhere; +} +#plan-import { + width: 100%; + margin-top: 12px; +} .issues-pane { flex: 1; overflow: auto; From dd3ddb8194fe49f1791aa43badcdea26af18b3fb Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 15:57:25 -0700 Subject: [PATCH 02/18] Document G1 planning screen status --- docs/architecture.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 73763adc..be98522e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -40,11 +40,11 @@ Writing standard: plain language, ISO 24495-1:2023 - **The central idea.** A runner-owned **commit ledger** records which plan item each commit belongs to. The linking engine uses the ledger, not commit messages, to put every changed line on a plan item's row. Anything the ledger cannot explain goes to the Unplanned or Ambiguous row. - **The central safety rule.** Everything that comes from outside is untrusted: issue text, agent output, the browser, and the repository's own files. Only the runner decides. Agents never see your credentials, your other files, or GitHub. - **Build state:** - - **In use:** plan and linking library, SQLite store, review screen, Ask (questions to a Claude agent), guarded merge with merge-queue support, ranked Issues screen, agent isolation boundary, and the single-runner lock. + - **In use:** plan and linking library, SQLite store, review screen, plan display/import screen, Ask (questions to a Claude agent), guarded merge with merge-queue support, ranked Issues screen, agent isolation boundary, and the single-runner lock. - **In use for a non-demo review with a `github` block:** planning. `/api/plan/suggestions` and `/api/plan/drafts` ask Claude, through lane D, for suggestion cards or a whole next plan revision (#117, #124). Nothing becomes a revision until you apply it. - **In use with an opt-in `runner` block in `review.json`:** the whole loop after planning. `start` and `resume` on `/api/runner` run a task's plan item by item with a Claude agent (#91). They require current plan-item approvals, bind both the task state and review version, recheck approvals before each later item, and refuse when a completed prefix no longer ends at the task's current head (#107). After an out-of-scope pause, an approved amended plan can reconcile the completed prefix and resume only the unfinished suffix (#88, PR #134). When a run ends, codeboost publishes the task's pull request, a draft if the task needs a person (#103). Approve & merge merges that pull request (#121). Cancelling a task closes its pull requests (#111). These are API actions; the screen has no buttons for them yet. - **Partly built for pre-merge automation:** the trusted local rebase engine records one-to-one commit mappings, verifies the resulting checkout byte for byte, owns its subprocesses durably, and recovers interrupted work (#22, PR #136). Its production conflict path gives a sandboxed agent only the bounded conflict files and imports only audited results for both foreign and plan-owned commits (PRs #138, #140 and #144). No production request invokes the rebaser yet. - - **Not built:** screens for planning and the runner actions, production rebase admission, post-rebase attribution and approval refresh, head-bound `cmd:` checks, remote push/check refresh, review rounds, and the Planning, Queue and Learning screens. + - **Not built:** plan authoring and suggestion controls, screens for the runner actions, production rebase admission, post-rebase attribution and approval refresh, head-bound `cmd:` checks, remote push/check refresh, review rounds, and the Queue and Learning screens. ## Terms used @@ -164,7 +164,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out |---|---|---|---| | Command line | `web/cli.ts` | Parses `--demo` or `--config`, takes the single-runner lock, starts the server, releases the lock on Ctrl+C. | In use | | HTTP server | `web/server.ts` | Binds `127.0.0.1` only. Checks the token, Host and Origin. Bounded JSON bodies. Routes `/api/review`, `/api/action`, `/api/questions`, `/api/merge`, `/api/issues`, `/api/settings`, plus `/api/runner` and `/api/plan/*` when a runner or planning provider is supplied. Ordered shutdown. | In use; `/api/plan/suggestions` and `/api/plan/drafts` run Claude through lane D for a review with a `github` block (#117, #124) | -| Browser screen | `web/public/*` | Review screen and Issues screen. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | In use | +| Browser screen | `web/public/*` | Review, Issues and Plans screens. Plans displays the complete current revision and imports a complete next JSON or YAML revision through the planning API. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | In use | | Review service | `runner/review.ts` | Builds the review view: reads history, links it to the plan, computes approval states and merge blockers. Applies review commands through the store. | In use | | Ask | `runner/questions.ts`, `runner/question-*.ts` | Answers a question about one plan item with a read-only Claude agent (Codex is refused in every phase, #93). Runs lane D's setup in a worker thread so the server stays responsive. Ask and planning share one read-only container runner (`runReadOnlyAgent`, #117) but keep separate workers, owners and leftovers. Labels its Docker objects with a per-database Ask owner and recovers only its own leftovers, so reviews sharing one Docker daemon do not block each other (#95). | In use | | Merge coordinator | `runner/merge.ts` | Runs the full merge gate twice, re-reads the local generation, then merges the exact reviewed head. Tracks merge-queue attempts. For a runner task it inspects and merges the task's own published pull request; a configured `github.pullRequest` that differs is refused (#121). | In use | @@ -180,7 +180,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | In use | | Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | In use | | Issue ranking | `core/issue-ranking.ts`, `github/issues.ts`, `web/issues.ts` | Deterministic, explainable issue score. Repository-scoped trust of issue authors. | In use | -| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); no screen yet | +| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); Plans displays and imports revisions, while authoring and suggestion controls remain future work | | Execution prompt and audit | `core/execution-prompt.ts`, `core/run-audit.ts`, `prompts/execute.md` | Builds the execute and fix prompt with untrusted text only in escaped data blocks. Audits the change manifest after a run: safety violations first (Git metadata, links that leave the repository, agent commits), then scope against the declared files. | In use with the opt-in `runner` block | | Pull request text | `core/pull-request-body.ts` | Title and description with the plan. Plan and agent text go in fenced blocks, and issue references and @-mentions are neutralized, so the text cannot close other issues or notify people. | In use with the opt-in `runner` block | | Git adapter | `git/history.ts`, `git/clone.ts` | Bounded, hardened history reads. Standalone task clones with no links back to your repository. Every Git command runs with an allowlisted environment. | In use | @@ -413,7 +413,7 @@ These are the decisions that shape the structure. The full list, with the reason | F | Runner | Merged: F1 (lifecycle store, coordinator, shutdown wiring, startup recovery and the lock, planning API, feedback events, and stops that wait for their commit: #53, #74, #57, #59, #60, #96); F2a–F2b (execute prompts, audit, per-item execution, real workspace, durable safety findings: #67, #68, #92, #94, #128); F2d (already-fixed check, PR opening, branch pusher, production publishing, closing PRs on cancel: #84, #101, #110, #115, #116, #119, #123); production wiring and `start`/`resume` (#91, PRs #102, #105); planning through lane D (#117, #124, PRs #118, #120, #125); merging the task's own PR (#121, PR #126); start/resume approval, review-version, recovery and completed-prefix guards (#107, PR #130); continuation after a scope pause (#88, PR #134); the F3 durable trusted local-rebase foundation (#22, PR #136); the F4 foreign-conflict engine plus production sandbox adapter (#22, PRs #138 and #140); and sandboxed owned-commit conflict resolution (#22, PR #144). Next in #22: F5 owns current base/head refresh, rebase admission, post-rebase attribution and approval refresh, and head-bound `cmd:` execution. F6 owns rewritten-head push, fresh required-check refresh, the already-fixed rerun, and exact-pair guarded-merge handoff. The integrated flow pushes a rewritten head before it recomputes review state and reruns every plan item's head-bound `cmd:` checks; fresh required GitHub checks follow. | | Hardening | Allowlisted subprocess environments | Done: every Git command (#82, #83) and every `gh` runner (#84, #86). | | Tests | Test suite reliability | Done. The full suite runs ordinary tests first and Docker-backed files serially; CI and the real-Docker gate passed on PR #132 (#129). | -| G | Planning screen | Not started. Unblocked: E4 is done and the planning API runs Claude in production. | +| G | Planning screen | G1 built (#145, PR #148): display the complete current plan and import the next JSON or YAML revision. G2 authoring/suggestion cards, G3 revision-bound Apply, and G4 real-provider integration acceptance remain. | | H | Issue ranking and Issues screen | Done. H4b (#108, PR #139) records author-bound trust, guards runner and publishing boundaries, controls which comments reach prompts, records prompt comment evidence, and provides trust/untrust controls on the Issues screen. | | I, J | Queue and run windows; learning from feedback | Not started (after F) | @@ -424,7 +424,7 @@ These are the decisions that shape the structure. The full list, with the reason - The container's guarantees are those of Docker. The agent can always reach its own vendor account. - `cmd:` acceptance checks do not run yet, so any plan item with a `cmd:` check blocks merging. - One user, one machine, one runner per database. The lock refuses a second runner, and it refuses network filesystems, where OS locks are unreliable. -- The runner is opt-in: it needs a `runner` block and a `github` block in `review.json`, and it is always off in the demo. It runs through the API only; the screen has no start, resume, publish or planning controls yet. Publishing also needs `github.baseBranch`. +- The runner is opt-in: it needs a `runner` block and a `github` block in `review.json`, and it is always off in the demo. It runs through the API only; the screen has no start, resume, publish, plan-authoring, suggestion or Apply controls yet. Publishing also needs `github.baseBranch`. - codeboost is Claude-only. Codex is refused in every phase (#93, PR #106), because it can read files only through its shell, which every phase turns off. `agent-isolation.md` says when to revisit this. ## Test this document with a reader From b12ef6fa6e416013684327bcca7e0d5206c996bf Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 16:17:56 -0700 Subject: [PATCH 03/18] Preserve plan import and polling generations --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 137 +++++++++++++++++++++++++ web/public/app.js | 23 +++-- 3 files changed, 155 insertions(+), 9 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index eaddd838..c12bf3f4 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request, even when a refresh has since observed that request's commit. A definite refusal clears it. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review and Plans responses also invalidate merge polling again immediately before applying, so a poll started while either request was pending cannot overwrite the newer state. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, stale merge-queue poll suppression after full Plans refresh and import reloads, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, stale merge-queue poll suppression after full Plans refresh and import reloads (including polls started while refresh is pending), queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index a44ce334..53897f5f 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -200,6 +200,40 @@ test('replays an ambiguous committed import after refresh without creating anoth expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('preserves an unresolved import retry while another file is definitely refused', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const file = page.getByLabel('Plan file'); + const source = JSON.stringify(nextPlan); + await file.setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(source) }); + const sent: unknown[] = []; + await page.route('**/api/plan/import', async route => { + sent.push(route.request().postDataJSON()); + if (sent.length === 1) { + await route.fetch(); + await route.abort('failed'); + return; + } + await route.continue(); + }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toBeVisible(); + + const wrongIssue = JSON.stringify({ ...nextPlan, issue: 413 }); + await file.setInputFiles({ name: 'wrong-issue.json', mimeType: 'application/json', buffer: Buffer.from(wrongIssue) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toContainText('Stale plan revision.'); + + await page.locator('#plans-refresh').click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await file.setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(source) }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + expect(sent).toHaveLength(3); + expect(sent[2]).toEqual(sent[0]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + test('reports a committed import separately when reloading the plan fails', async ({ page }) => { await page.goto(app.url); await page.getByRole('link', { name: 'Plans', exact: true }).click(); @@ -292,6 +326,109 @@ test('does not let an older Plans refresh re-enable merge after it commits', asy expect(app.service.store.getMergeAttempt(config.identity)?.state).toBe('merged'); }); +test('does not let merge polls started during Review or Plans refresh overwrite full responses', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-07T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + + await page.goto(app.url); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + let releaseReviewRefresh!: () => void, reviewRefreshArrived!: () => void; + const heldReviewRefresh = new Promise(resolve => { releaseReviewRefresh = resolve; }); + const capturedReviewRefresh = new Promise(resolve => { reviewRefreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const current = await route.fetch(); + const refreshed = await current.json(); + refreshed.repository = 'retry-service-refreshed'; + reviewRefreshArrived(); + await heldReviewRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await capturedReviewRefresh; + let releaseReviewPoll!: () => void, reviewPollArrived!: () => void; + const heldReviewPoll = new Promise(resolve => { releaseReviewPoll = resolve; }); + const capturedReviewPoll = new Promise(resolve => { reviewPollArrived = resolve; }); + await page.route('**/api/merge', async route => { + reviewPollArrived(); + await heldReviewPoll; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + state: 'removed', reviewedHead: view.snapshot.head, url: null, reason: 'Old Review refresh poll result.', phase: null, + position: null, occurredAt: '2026-10-07T08:04:00Z', retryable: true, + } }) }); + }); + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await capturedReviewPoll; + releaseReviewRefresh(); + await expect(page.locator('#repository')).toHaveText('retry-service-refreshed'); + const staleReviewPollResponse = page.waitForResponse(response => response.url().endsWith('/api/merge')); + releaseReviewPoll(); + await staleReviewPollResponse; + await page.waitForTimeout(100); + await expect(page.locator('#banner')).not.toContainText('Old Review refresh poll result.'); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + await page.unroute('**/api/review'); + await page.unroute('**/api/merge'); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const current = await route.fetch(); + refreshArrived(); + await heldRefresh; + await route.fulfill({ response: current }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + let releasePoll!: () => void, pollArrived!: () => void; + const heldPoll = new Promise(resolve => { releasePoll = resolve; }); + const capturedPoll = new Promise(resolve => { pollArrived = resolve; }); + await page.route('**/api/merge', async route => { + pollArrived(); + await heldPoll; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + state: 'removed', reviewedHead: view.snapshot.head, url: null, reason: 'Old refresh poll result.', phase: null, + position: null, occurredAt: '2026-10-07T08:05:00Z', retryable: true, + } }) }); + }); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await capturedPoll; + releaseRefresh(); + await expect(page.locator('#plans-status')).toContainText('Revision r1 is current.'); + const stalePollResponse = page.waitForResponse(response => response.url().endsWith('/api/merge')); + releasePoll(); + await stalePollResponse; + await page.waitForTimeout(100); + await expect(page.locator('#banner')).not.toContainText('Old refresh poll result.'); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); +}); + test('keeps merge-queue polling current across Plans refresh and import outcomes', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 5f0cf5ea..364a7d00 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -117,6 +117,11 @@ async function refresh() { rememberDraft(); const updated = await api("/api/review"); if (generation !== reviewGeneration) return; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; rememberDraft(); data = updated; snippetSelection = null; @@ -831,7 +836,7 @@ let issuesGeneration = 0, issuesRefreshError = null, issueErrorOrder = 0; let planImportPending = false, - planImportRetry = null, + planImportRetries = new Map(), planRefreshPending = false, planOperationGeneration = 0, planStatusGeneration = 0; @@ -928,15 +933,14 @@ async function importPlan(event) { return; } const expectedRevision = data.plan.revision; - const retained = planImportRetry; - const request = retained?.source === source && retained.format === format - ? retained : { source, format, expectedRevision, actionId: crypto.randomUUID() }; - planImportRetry = request; + const retryKey = JSON.stringify([format, source]); + const request = planImportRetries.get(retryKey) ?? { source, format, expectedRevision, actionId: crypto.randomUUID() }; + planImportRetries.set(retryKey, request); setPlanStatus(statusOwner, "neutral", "Importing the next revision…"); try { const response = await api("/api/plan/import", request); const revision = Number.isSafeInteger(response.result?.revision) ? response.result.revision : null; - planImportRetry = null; + planImportRetries.delete(retryKey); if (data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, blockers: [{ code: "plan-changed", message: "The plan changed. Refresh before merging." }] } }; renderMerge(); @@ -964,7 +968,7 @@ async function importPlan(event) { } } catch (error) { const ambiguous = !Number.isSafeInteger(error?.status) || error.status === 503 || error.outcomeUnknown === true; - if (!ambiguous) planImportRetry = null; + if (!ambiguous) planImportRetries.delete(retryKey); if (ambiguous && data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, blockers: [{ code: "plan-import-unknown", message: "The plan import outcome is unknown. Refresh before merging." }] } }; setPlanStatus(statusOwner, "bad", `✕ Could not import the plan. ${error.message}`); @@ -1182,6 +1186,11 @@ $("plans-refresh").onclick = async () => { rememberDraft(); const updated = await api("/api/review"); if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; rememberDraft(); data = updated; render(); From 04f6efe0582e21dee0dbfa5c8aa1c851a6d879cb Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 16:55:38 -0700 Subject: [PATCH 04/18] Fix stale plan refresh responses --- test/browser/plans.spec.ts | 69 ++++++++++++++++++++++++++++++++++++++ web/public/app.js | 5 ++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 53897f5f..afe1c74d 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -276,6 +276,75 @@ test('does not let an older Review refresh overwrite a committed import', async expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('clears stale Plans data when a Review refresh fails after navigation', async ({ page }) => { + await page.goto(app.url); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { + arrived(); + await held; + await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Review read failed.' }) }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await captured; + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.getByRole('heading', { name: 'Make retries predictable and document the behavior' })).toBeVisible(); + release(); + await expect(page.locator('#reload')).toHaveText('Refresh'); + await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable'); + await expect(page.locator('#plans-status')).toContainText('Could not read this branch’s history. Review read failed. Use Refresh to retry.'); +}); + +test('does not let a question poll started during Plans refresh replace its completed answer', async ({ page }) => { + const config = app.service.config; + await app.close(); + app = await startServer(config, 0, (_prompt, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + await page.goto(app.url); + await page.getByLabel('Question about this item').fill('Will this answer finish?'); + await page.getByRole('button', { name: 'Ask agent', exact: true }).click(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toBeVisible(); + const note = app.service.store.getReviewNotes(config.identity).find(value => value.text === 'Will this answer finish?')!; + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + const completed = refreshed.notes.find((value: { id: string }) => value.id === note.id); + completed.answer = { ...completed.answer, status: 'complete', text: 'The fresh completed answer.' }; + completed.answerActive = false; + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + let releasePoll!: () => void, pollArrived!: () => void; + const heldPoll = new Promise(resolve => { releasePoll = resolve; }); + const capturedPoll = new Promise(resolve => { pollArrived = resolve; }); + await page.route('**/api/questions', async route => { + const response = await route.fetch(); + pollArrived(); + await heldPoll; + await route.fulfill({ response }); + }); + await capturedPoll; + releaseRefresh(); + await expect(page.locator('#plans-status')).toContainText('Revision r1 is current.'); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.getByText('The fresh completed answer.', { exact: true })).toBeVisible(); + const stalePollResponse = page.waitForResponse(response => response.url().endsWith('/api/questions')); + releasePoll(); + await stalePollResponse; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByText('The fresh completed answer.', { exact: true })).toBeVisible(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); +}); + test('does not let an older Plans refresh re-enable merge after it commits', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 364a7d00..5b2b4718 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -78,6 +78,8 @@ function showFailure(message) { mergePollDelay = 2000; data = null; snippetSelection = null; + renderPlans(); + claimPlanStatus(view === "plans" ? "bad" : "neutral", view === "plans" ? `✕ ${message}` : ""); $("selection-actions").hidden = true; $("attachment").hidden = true; $("banner").textContent = message; @@ -1172,7 +1174,7 @@ $("plans-refresh").onclick = async () => { if (planRefreshPending || planImportPending) return; if (busy) return; const generation = ++planOperationGeneration; - const sharedGeneration = ++reviewGeneration; + let sharedGeneration = ++reviewGeneration; mergeGeneration++; if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; @@ -1186,6 +1188,7 @@ $("plans-refresh").onclick = async () => { rememberDraft(); const updated = await api("/api/review"); if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; + sharedGeneration = ++reviewGeneration; mergeGeneration++; if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; From 6b7fd1518476198c4126f6885d0aa40487194f3e Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 17:14:37 -0700 Subject: [PATCH 05/18] Preserve completed answers across plan refresh --- test/browser/plans.spec.ts | 42 ++++++++++++++++++++++++++++++++++++++ web/public/app.js | 14 ++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index afe1c74d..b798cfaf 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -345,6 +345,48 @@ test('does not let a question poll started during Plans refresh replace its comp await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); }); +test('does not let an older Plans refresh replace an answer completed while it was pending', async ({ page }) => { + const config = app.service.config; + await app.close(); + app = await startServer(config, 0, (_prompt, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + await page.goto(app.url); + await page.getByLabel('Question about this item').fill('Did this finish during refresh?'); + await page.getByRole('button', { name: 'Ask agent', exact: true }).click(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toBeVisible(); + const note = app.service.store.getReviewNotes(config.identity).find(value => value.text === 'Did this finish during refresh?')!; + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + refreshArrived(); + await heldRefresh; + await route.fulfill({ response }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + const completedAnswer = { ...note.answer, status: 'complete', text: 'The poll completed this answer.' }; + const completedPoll = page.waitForResponse(response => response.url().endsWith('/api/questions')); + let polls = 0; + await page.route('**/api/questions', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ notes: polls++ === 0 ? [{ id: note.id, answer: completedAnswer, answerActive: false }] : [] }), + })); + await completedPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#notes')).toContainText('The poll completed this answer.'); + + releaseRefresh(); + await expect(page.locator('#plans-status')).toContainText('Revision r1 is current.'); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.getByText('The poll completed this answer.', { exact: true })).toBeVisible(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); +}); + test('does not let an older Plans refresh re-enable merge after it commits', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 5b2b4718..a2f4da15 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -773,6 +773,18 @@ function renderNotes({ follow = false } = {}) { document.querySelectorAll("[data-retry-question]").forEach(button=>button.onclick=()=>act({action:"retry-question",id:button.dataset.retryQuestion})); } let pollingQuestions=false; +function preserveCompletedQuestionAnswers(updated) { + if (!data) return updated; + const current = new Map(data.notes.map(note => [note.id, note])); + return { ...updated, notes: updated.notes.map(note => { + const newer = current.get(note.id); + return newer?.answer?.status === "complete" && + note.answer?.status === "pending" && + newer.answer.attempt === note.answer.attempt + ? { ...note, answer: newer.answer, answerActive: newer.answerActive } + : note; + }) }; +} setInterval(async()=>{ if(pollingQuestions || busy || !data || !data.notes.some(n=>n.answer?.status==="pending" || n.answerActive)) return; pollingQuestions=true; @@ -1195,7 +1207,7 @@ $("plans-refresh").onclick = async () => { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - data = updated; + data = preserveCompletedQuestionAnswers(updated); render(); setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); } catch (error) { From 01ad35835693b66683c0eb71d1a4e1fab1ae1f95 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 22:54:29 -0700 Subject: [PATCH 06/18] Preserve terminal poll results during plan refresh --- test/browser/plans.spec.ts | 117 +++++++++++++++++++++++++++++++++++++ web/public/app.js | 54 ++++++++++++----- 2 files changed, 156 insertions(+), 15 deletions(-) diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index b798cfaf..a3fc1657 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -387,6 +387,49 @@ test('does not let an older Plans refresh replace an answer completed while it w await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); }); +test('does not let an older Plans refresh replace an answer failure from the same attempt', async ({ page }) => { + const config = app.service.config; + await app.close(); + app = await startServer(config, 0, (_prompt, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + await page.goto(app.url); + await page.getByLabel('Question about this item').fill('Will this failure stay visible?'); + await page.getByRole('button', { name: 'Ask agent', exact: true }).click(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toBeVisible(); + const note = app.service.store.getReviewNotes(config.identity).find(value => value.text === 'Will this failure stay visible?')!; + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + refreshArrived(); + await heldRefresh; + await route.fulfill({ response }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + const failedAnswer = { ...note.answer, status: 'failed', error: 'The agent failed during refresh.' }; + const failedPoll = page.waitForResponse(response => response.url().endsWith('/api/questions')); + let polls = 0; + await page.route('**/api/questions', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ notes: polls++ === 0 ? [{ id: note.id, answer: failedAnswer, answerActive: false }] : [] }), + })); + await failedPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#notes')).toContainText('The agent failed during refresh.'); + + releaseRefresh(); + await expect(page.locator('#plans-status')).toContainText('Revision r1 is current.'); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.getByText('! The agent failed during refresh.', { exact: true })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Retry answer', exact: true })).toBeVisible(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); +}); + test('does not let an older Plans refresh re-enable merge after it commits', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; @@ -540,6 +583,80 @@ test('does not let merge polls started during Review or Plans refresh overwrite await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); }); +test('does not let an older Plans refresh overwrite a newer terminal merge observation', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-07T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + + await page.goto(app.url); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + let queued!: Record; + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + queued = refreshed.merge.queue; + refreshed.repository = 'poll-first-refreshed'; + refreshed.merge.blockers.push({ code: 'check', message: 'Fresh required check is pending.' }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + expect(queued.actionId).toEqual(expect.any(String)); + + let polls = 0; + await page.route('**/api/merge', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ queue: polls++ === 0 + ? { ...queued, state: 'merged', phase: null, position: null, occurredAt: '2026-10-07T08:06:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } + : queued }), + })); + const mergedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await mergedPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + + releaseRefresh(); + await expect(page.locator('#repository')).toHaveText('poll-first-refreshed'); + await page.waitForTimeout(100); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('merged'); + await expect(page.locator('#dialog-body')).toContainText('GitHub confirmed that the reviewed head was merged.'); + await expect(page.locator('#dialog-body')).toContainText('Fresh required check is pending.'); +}); + test('keeps merge-queue polling current across Plans refresh and import outcomes', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index a2f4da15..fb84a0b0 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -185,19 +185,9 @@ async function pollMergeQueue(generation) { try { const update = await api("/api/merge"); if (generation !== mergeGeneration || !data?.merge?.available) return; - data = { ...data, merge: { ...data.merge, queue: update.queue } }; const queue = update.queue; - if (queue?.state === "merged") { - data.merge = { ...data.merge, ready: false, action: null, blockers: [{ code: "queue-merged", message: "GitHub confirmed that the reviewed head was merged." }] }; - $("banner").textContent = "GitHub confirmed the reviewed head was merged."; - } else if (queue?.state === "removed" || queue?.state === "failed") { - data.merge = { ...data.merge, ready: false, action: null, blockers: [{ code: "queue-refresh", message: `${queue.reason} Refresh to verify retry readiness.` }] }; - $("banner").textContent = `${queue.state === "removed" ? "Removed from merge queue" : "Merge queue failed"}. ${queue.reason} Refresh to verify retry readiness.`; - } else if (queue?.observationError) { - $("banner").textContent = `${queue.state === "submitting" ? "Merge submission status is unknown" : "Merge remains queued"}. ${queue.observationError}`; - } else if (queue?.state === "queued") { - $("banner").textContent = `Merge queued${queue.position === null ? "" : ` at position ${queue.position}`}. Waiting for GitHub.`; - } + data = withMergeQueue(data, queue); + showMergeQueueStatus(queue); if (queue?.state === mergePollState) backOffMergePoll(); renderMerge(); } catch (error) { @@ -207,6 +197,28 @@ async function pollMergeQueue(generation) { scheduleMergePoll(); } } +const mergeLifecycleBlockers = new Set(["merge-submitted", "queue-active", "queue-head", "queue-merged", "queue-refresh", "stale-merge"]); +function withMergeQueue(view, queue) { + const blockers = view.merge.blockers.filter(blocker => !mergeLifecycleBlockers.has(blocker.code)); + if (queue?.state === "merged") + blockers.unshift({ code: "queue-merged", message: "GitHub confirmed that the reviewed head was merged." }); + else if (queue?.state === "removed" || queue?.state === "failed") + blockers.unshift({ code: "queue-refresh", message: `${queue.reason} Refresh to verify retry readiness.` }); + else + blockers.unshift(...view.merge.blockers.filter(blocker => blocker.code === "queue-active")); + return { ...view, merge: { ...view.merge, ready: false, action: null, queue, blockers } }; +} +function showMergeQueueStatus(queue) { + if (queue?.state === "merged") { + $("banner").textContent = "GitHub confirmed the reviewed head was merged."; + } else if (queue?.state === "removed" || queue?.state === "failed") { + $("banner").textContent = `${queue.state === "removed" ? "Removed from merge queue" : "Merge queue failed"}. ${queue.reason} Refresh to verify retry readiness.`; + } else if (queue?.observationError) { + $("banner").textContent = `${queue.state === "submitting" ? "Merge submission status is unknown" : "Merge remains queued"}. ${queue.observationError}`; + } else if (queue?.state === "queued") { + $("banner").textContent = `Merge queued${queue.position === null ? "" : ` at position ${queue.position}`}. Waiting for GitHub.`; + } +} async function act(command) { if (busy || planImportPending || !data) return false; supersedePlanRefresh(); @@ -773,18 +785,28 @@ function renderNotes({ follow = false } = {}) { document.querySelectorAll("[data-retry-question]").forEach(button=>button.onclick=()=>act({action:"retry-question",id:button.dataset.retryQuestion})); } let pollingQuestions=false; -function preserveCompletedQuestionAnswers(updated) { +function preserveSettledQuestionAnswers(updated) { if (!data) return updated; const current = new Map(data.notes.map(note => [note.id, note])); return { ...updated, notes: updated.notes.map(note => { const newer = current.get(note.id); - return newer?.answer?.status === "complete" && + return ["complete", "failed"].includes(newer?.answer?.status) && note.answer?.status === "pending" && newer.answer.attempt === note.answer.attempt ? { ...note, answer: newer.answer, answerActive: newer.answerActive } : note; }) }; } +function newerMergeQueue(updated) { + const newer = data?.merge?.queue, older = updated.merge?.queue; + return newer?.actionId && + newer.actionId === older?.actionId && + newer.reviewedHead === older.reviewedHead && + ["merged", "removed", "failed"].includes(newer.state) && + ["submitting", "queued"].includes(older.state) + ? newer + : null; +} setInterval(async()=>{ if(pollingQuestions || busy || !data || !data.notes.some(n=>n.answer?.status==="pending" || n.answerActive)) return; pollingQuestions=true; @@ -1207,8 +1229,10 @@ $("plans-refresh").onclick = async () => { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - data = preserveCompletedQuestionAnswers(updated); + const newerQueue = newerMergeQueue(updated); + data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); render(); + if (newerQueue) showMergeQueueStatus(newerQueue); setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); } catch (error) { if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; From b9cac2ad862c9f905fd5a7e5301ae6cc1d930a84 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 23:19:11 -0700 Subject: [PATCH 07/18] Preserve settled state across review refreshes --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 127 +++++++++++++++++++++++++ web/public/app.js | 13 ++- 3 files changed, 138 insertions(+), 6 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index c12bf3f4..f071d9a8 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review and Plans responses also invalidate merge polling again immediately before applying, so a poll started while either request was pending cannot overwrite the newer state. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review and Plans responses invalidate merge polls that return afterward and reconcile a terminal queue observation that applied while the response was pending only when its action ID and reviewed head match. Plans refresh does the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, stale merge-queue poll suppression after full Plans refresh and import reloads (including polls started while refresh is pending), queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, cancellation settlement, stale merge-queue poll suppression after import reloads, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index a3fc1657..5767d6b5 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -269,7 +269,10 @@ test('does not let an older Review refresh overwrite a committed import', async await page.getByRole('button', { name: 'Approve P1', exact: true }).click(); await expect(page.getByText('1 of 2 approved')).toBeVisible(); await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); release(); + await (await staleReviewResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.locator('#reload')).toHaveText('Refresh'); await expect(page.locator('#plans-revision')).toHaveText('r2'); await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); @@ -430,6 +433,54 @@ test('does not let an older Plans refresh replace an answer failure from the sam await expect(page.getByText('Agent · Answering…', { exact: true })).toHaveCount(0); }); +test('does not let an older Plans refresh restore cancellation activity after failure settles', async ({ page }) => { + const config = app.service.config; + await app.close(); + app = await startServer(config, 0, (_prompt, signal) => new Promise((_, reject) => signal.addEventListener('abort', () => reject(signal.reason), { once: true }))); + await page.goto(app.url); + await page.getByLabel('Question about this item').fill('Did cancellation settle during refresh?'); + await page.getByRole('button', { name: 'Ask agent', exact: true }).click(); + await expect(page.getByText('Agent · Answering…', { exact: true })).toBeVisible(); + const note = app.service.store.getReviewNotes(config.identity).find(value => value.text === 'Did cancellation settle during refresh?')!; + const failedAnswer = { ...note.answer, status: 'failed', error: 'The agent timed out.' }; + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + const failed = refreshed.notes.find((value: { id: string }) => value.id === note.id); + failed.answer = failedAnswer; + failed.answerActive = true; + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + let polls = 0; + const settledPoll = page.waitForResponse(response => response.url().endsWith('/api/questions')); + await page.route('**/api/questions', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ notes: polls++ === 0 ? [{ id: note.id, answer: failedAnswer, answerActive: false }] : [] }), + })); + await settledPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Retry answer', exact: true })).toBeVisible(); + + const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await staleReviewResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Retry answer', exact: true })).toBeVisible(); + await expect(page.getByText('Agent · Finishing cancellation…', { exact: true })).toHaveCount(0); +}); + test('does not let an older Plans refresh re-enable merge after it commits', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; @@ -472,7 +523,10 @@ test('does not let an older Plans refresh re-enable merge after it commits', asy await expect(page.locator('#plans-refresh')).toHaveText('Refresh plan'); await expect(page.locator('#plans-status')).toBeEmpty(); await page.getByRole('link', { name: 'Review', exact: true }).click(); + const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); release(); + await (await staleReviewResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.locator('#plans-refresh')).toHaveText('Refresh plan'); await expect(page.locator('#merge')).toBeDisabled(); await page.getByRole('link', { name: 'Plans', exact: true }).click(); @@ -657,6 +711,79 @@ test('does not let an older Plans refresh overwrite a newer terminal merge obser await expect(page.locator('#dialog-body')).toContainText('Fresh required check is pending.'); }); +test('does not let an older Review refresh overwrite a newer terminal merge observation', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-07T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + + await page.goto(app.url); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + let queued!: Record; + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + queued = refreshed.merge.queue; + refreshed.repository = 'review-poll-first-refreshed'; + refreshed.merge.blockers.push({ code: 'check', message: 'Fresh Review blocker is pending.' }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await capturedRefresh; + expect(queued.actionId).toEqual(expect.any(String)); + + let polls = 0; + await page.route('**/api/merge', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ queue: polls++ === 0 + ? { ...queued, state: 'merged', phase: null, position: null, occurredAt: '2026-10-07T08:07:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } + : queued }), + })); + const mergedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await mergedPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + + const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await staleReviewResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#repository')).toHaveText('review-poll-first-refreshed'); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('merged'); + await expect(page.locator('#dialog-body')).toContainText('Fresh Review blocker is pending.'); +}); + test('keeps merge-queue polling current across Plans refresh and import outcomes', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index fb84a0b0..3387ae33 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -125,10 +125,12 @@ async function refresh() { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - data = updated; + const newerQueue = newerMergeQueue(updated); + data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); snippetSelection = null; selected ??= data.items[0]?.id || "Unplanned"; render(); + if (newerQueue) showMergeQueueStatus(newerQueue); } catch (error) { if (generation !== reviewGeneration) return; rememberDraft(); @@ -790,9 +792,12 @@ function preserveSettledQuestionAnswers(updated) { const current = new Map(data.notes.map(note => [note.id, note])); return { ...updated, notes: updated.notes.map(note => { const newer = current.get(note.id); - return ["complete", "failed"].includes(newer?.answer?.status) && - note.answer?.status === "pending" && - newer.answer.attempt === note.answer.attempt + const sameAttempt = newer?.answer?.attempt !== undefined && newer.answer.attempt === note.answer?.attempt; + const settledFromPending = note.answer?.status === "pending"; + const cancellationSettled = note.answerActive && newer?.answerActive === false && + newer.answer?.status === note.answer?.status; + return sameAttempt && ["complete", "failed"].includes(newer.answer.status) && + (settledFromPending || cancellationSettled) ? { ...note, answer: newer.answer, answerActive: newer.answerActive } : note; }) }; From 4ae620bedd966949ee12f1dc27572fc5a13e8fea Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 23:38:57 -0700 Subject: [PATCH 08/18] Preserve replayed import poll results --- docs/implementation/planning-screen.md | 2 +- test/browser/plans.spec.ts | 108 +++++++++++++++++++++++-- web/public/app.js | 4 +- 3 files changed, 104 insertions(+), 10 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index f071d9a8..37437841 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,7 +10,7 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review and Plans responses invalidate merge polls that return afterward and reconcile a terminal queue observation that applied while the response was pending only when its action ID and reviewed head match. Plans refresh does the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review, Plans and post-import reload responses invalidate merge polls that return afterward and reconcile a terminal queue observation that applied while the response was pending only when its action ID and reviewed head match. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 5767d6b5..d7b8773e 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -200,6 +200,102 @@ test('replays an ambiguous committed import after refresh without creating anoth expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('does not let a replayed import reload overwrite a newer terminal merge observation', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-07T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + const sent: unknown[] = []; + await page.route('**/api/plan/import', async route => { + sent.push(route.request().postDataJSON()); + if (sent.length === 1) { + await route.fetch(); + await route.abort('failed'); + return; + } + await route.continue(); + }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Could not import the plan.' })).toBeVisible(); + await page.locator('#plans-refresh').click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + + let view = app.service.load(); + for (const segment of view.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + view = app.service.act({ action: 'accept', key: segment.key, token: view.token }); + for (const item of view.items) + view = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: view.token }); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Merge PR', exact: true })).toBeEnabled(); + page.on('dialog', dialog => dialog.accept()); + await page.getByRole('button', { name: 'Merge PR', exact: true }).click(); + await expect.poll(() => app.service.store.getMergeAttempt(config.identity)?.state).toBe('queued'); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + + let releaseReload!: () => void, reloadArrived!: () => void; + const heldReload = new Promise(resolve => { releaseReload = resolve; }); + const capturedReload = new Promise(resolve => { reloadArrived = resolve; }); + let queued!: Record; + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + queued = refreshed.merge.queue; + refreshed.repository = 'import-replay-refreshed'; + refreshed.merge.blockers.push({ code: 'check', message: 'Fresh replay blocker is pending.' }); + reloadArrived(); + await heldReload; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await capturedReload; + expect(sent).toHaveLength(2); + expect(sent[1]).toEqual(sent[0]); + expect(queued.actionId).toEqual(expect.any(String)); + + await page.route('**/api/merge', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ queue: { ...queued, state: 'merged', phase: null, position: null, + occurredAt: '2026-10-07T08:08:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } }), + })); + const mergedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P1 Expose the retry ceiling/ }).click(); + await mergedPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + + const staleReloadResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseReload(); + await (await staleReloadResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#repository')).toHaveText('import-replay-refreshed'); + await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('merged'); + await expect(page.locator('#dialog-body')).toContainText('Fresh replay blocker is pending.'); +}); + test('preserves an unresolved import retry while another file is definitely refused', async ({ page }) => { await page.goto(app.url); await page.getByRole('link', { name: 'Plans', exact: true }).click(); @@ -686,13 +782,11 @@ test('does not let an older Plans refresh overwrite a newer terminal merge obser await capturedRefresh; expect(queued.actionId).toEqual(expect.any(String)); - let polls = 0; await page.route('**/api/merge', route => route.fulfill({ status: 200, contentType: 'application/json', - body: JSON.stringify({ queue: polls++ === 0 - ? { ...queued, state: 'merged', phase: null, position: null, occurredAt: '2026-10-07T08:06:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } - : queued }), + body: JSON.stringify({ queue: { ...queued, state: 'merged', phase: null, position: null, + occurredAt: '2026-10-07T08:06:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } }), })); const mergedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); await page.getByRole('link', { name: 'Review', exact: true }).click(); @@ -759,13 +853,11 @@ test('does not let an older Review refresh overwrite a newer terminal merge obse await capturedRefresh; expect(queued.actionId).toEqual(expect.any(String)); - let polls = 0; await page.route('**/api/merge', route => route.fulfill({ status: 200, contentType: 'application/json', - body: JSON.stringify({ queue: polls++ === 0 - ? { ...queued, state: 'merged', phase: null, position: null, occurredAt: '2026-10-07T08:07:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } - : queued }), + body: JSON.stringify({ queue: { ...queued, state: 'merged', phase: null, position: null, + occurredAt: '2026-10-07T08:07:00Z', url: 'https://github.com/example/repo/pull/7', reason: null } }), })); const mergedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); diff --git a/web/public/app.js b/web/public/app.js index 3387ae33..e5e8743b 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -998,8 +998,10 @@ async function importPlan(event) { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - data = updated; + const newerQueue = newerMergeQueue(updated); + data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); render(); + if (newerQueue) showMergeQueueStatus(newerQueue); if ($("plan-file").files[0] === file) $("plan-file").value = ""; renderPlanFile(); setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); From cf3d037d7dd72e8dc642e340a9531aede4378466 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 7 Oct 2026 23:47:05 -0700 Subject: [PATCH 09/18] Harden terminal poll race regressions --- test/browser/plans.spec.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index d7b8773e..df8d1c9e 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -284,6 +284,7 @@ test('does not let a replayed import reload overwrite a newer terminal merge obs await mergedPoll; await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.unroute('**/api/merge'); const staleReloadResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); releaseReload(); @@ -794,6 +795,7 @@ test('does not let an older Plans refresh overwrite a newer terminal merge obser await mergedPoll; await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.unroute('**/api/merge'); releaseRefresh(); await expect(page.locator('#repository')).toHaveText('poll-first-refreshed'); @@ -864,6 +866,7 @@ test('does not let an older Review refresh overwrite a newer terminal merge obse await mergedPoll; await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.getByRole('button', { name: 'Merged', exact: true })).toBeDisabled(); + await page.unroute('**/api/merge'); const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); releaseRefresh(); From ff5098d09a208bbeca3d6ab7aa6a35042033ba91 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 00:06:12 -0700 Subject: [PATCH 10/18] Reconcile plan status after review refresh --- docs/implementation/planning-screen.md | 2 +- test/browser/plans.spec.ts | 46 ++++++++++++++++++++++++++ web/public/app.js | 2 ++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 37437841..0a3c3271 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -6,7 +6,7 @@ G1 adds a Plans screen over the planning contracts already owned by lanes E and ## Display contract -The screen renders the complete current plan: issue, summary, revision, open questions, ordered items, dependencies, files (including both sides of a rename) and acceptance checks. Navigation uses `?view=plans`, announces the active destination with `aria-current`, and preserves the existing Review composer DOM while the Plans screen is open. +The screen renders the complete current plan: issue, summary, revision, open questions, ordered items, dependencies, files (including both sides of a rename) and acceptance checks. Navigation uses `?view=plans`, announces the active destination with `aria-current`, and preserves the existing Review composer DOM while the Plans screen is open. An authoritative Review refresh reconciles a settled Plans revision or load-failure status with the response it applies, but only while no newer plan operation owns that status. ## Import contract diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index df8d1c9e..cf0b6141 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '@playwright/test'; +import { randomUUID } from 'node:crypto'; import { chmodSync, mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -396,6 +397,51 @@ test('clears stale Plans data when a Review refresh fails after navigation', asy await expect(page.locator('#plans-status')).toContainText('Could not read this branch’s history. Review read failed. Use Refresh to retry.'); }); +test('updates a settled Plans revision status when Review refresh observes a newer plan', async ({ page }) => { + await page.goto(app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r1 is current.' })).toBeVisible(); + + const response = await fetch(new URL('/api/plan/import', app.url), { + method: 'POST', + headers: { 'content-type': 'application/json', 'x-codeboost-token': app.token }, + body: JSON.stringify({ source: JSON.stringify(nextPlan), format: 'json', expectedRevision: 1, actionId: randomUUID() }), + }); + expect(response.status).toBe(200); + + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.getByRole('status').filter({ hasText: 'Revision r2 is current.' })).toBeVisible(); + await expect(page.locator('#plans-status')).not.toContainText('Revision r1 is current.'); +}); + +test('clears a Plans load-failure status when Review refresh recovers', async ({ page }) => { + await page.goto(app.url); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { + arrived(); + await held; + await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Review read failed.' }) }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await captured; + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + release(); + await expect(page.locator('#plans-status')).toContainText('Could not read this branch’s history. Review read failed.'); + + await page.unroute('**/api/review'); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.getByRole('status').filter({ hasText: 'Revision r1 is current.' })).toBeVisible(); + await expect(page.locator('#plans-status')).not.toContainText('Review read failed.'); +}); + test('does not let a question poll started during Plans refresh replace its completed answer', async ({ page }) => { const config = app.service.config; await app.close(); diff --git a/web/public/app.js b/web/public/app.js index e5e8743b..41ea662b 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -108,6 +108,7 @@ function showFailure(message) { async function refresh() { if (busy || planImportPending) return; supersedePlanRefresh(); + const planStatusOwner = planStatusGeneration; const busyOwner = beginBusy("read"); const generation = ++reviewGeneration; mergeGeneration++; @@ -131,6 +132,7 @@ async function refresh() { selected ??= data.items[0]?.id || "Unplanned"; render(); if (newerQueue) showMergeQueueStatus(newerQueue); + setPlanStatus(planStatusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); } catch (error) { if (generation !== reviewGeneration) return; rememberDraft(); From 740380e8e37411fb67824af6b3ae37b5243f667f Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 00:32:30 -0700 Subject: [PATCH 11/18] Preserve newer plan feedback on refresh failure --- docs/implementation/planning-screen.md | 2 +- test/browser/plans.spec.ts | 26 ++++++++++++++++++++++++++ web/public/app.js | 8 ++++++-- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 0a3c3271..e2537ffa 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -6,7 +6,7 @@ G1 adds a Plans screen over the planning contracts already owned by lanes E and ## Display contract -The screen renders the complete current plan: issue, summary, revision, open questions, ordered items, dependencies, files (including both sides of a rename) and acceptance checks. Navigation uses `?view=plans`, announces the active destination with `aria-current`, and preserves the existing Review composer DOM while the Plans screen is open. An authoritative Review refresh reconciles a settled Plans revision or load-failure status with the response it applies, but only while no newer plan operation owns that status. +The screen renders the complete current plan: issue, summary, revision, open questions, ordered items, dependencies, files (including both sides of a rename) and acceptance checks. Navigation uses `?view=plans`, announces the active destination with `aria-current`, and preserves the existing Review composer DOM while the Plans screen is open. An authoritative Review refresh reconciles a settled Plans revision or load-failure status with the response it applies, but only while no newer plan operation owns that status. A failed Review refresh still clears unavailable plan data, but likewise replaces the Plans status only while it retains ownership. ## Import contract diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index cf0b6141..5c93b74a 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -442,6 +442,32 @@ test('clears a Plans load-failure status when Review refresh recovers', async ({ await expect(page.locator('#plans-status')).not.toContainText('Review read failed.'); }); +test('does not let an older failed Review refresh replace newer Plans validation feedback', async ({ page }) => { + await page.goto(app.url); + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { + arrived(); + await held; + await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Older Review failure.' }) }); + }); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await captured; + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.txt', mimeType: 'text/plain', buffer: Buffer.from('not a plan') }); + await page.getByRole('button', { name: 'Import next revision' }).click(); + await expect(page.locator('#plans-status')).toContainText('Choose a JSON or YAML plan file.'); + + const staleReviewResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + release(); + await (await staleReviewResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable'); + await expect(page.locator('#plans-status')).toContainText('Choose a JSON or YAML plan file.'); + await expect(page.locator('#plans-status')).not.toContainText('Older Review failure.'); +}); + test('does not let a question poll started during Plans refresh replace its completed answer', async ({ page }) => { const config = app.service.config; await app.close(); diff --git a/web/public/app.js b/web/public/app.js index 41ea662b..52bf92d6 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -70,7 +70,7 @@ async function api(path, body) { function rememberDraft() { if (selected) drafts.set(`${selected}:${mode}`, $("message").value); } -function showFailure(message) { +function showFailure(message, planStatusOwner) { mergeGeneration++; if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; @@ -79,7 +79,10 @@ function showFailure(message) { data = null; snippetSelection = null; renderPlans(); - claimPlanStatus(view === "plans" ? "bad" : "neutral", view === "plans" ? `✕ ${message}` : ""); + const planStatusClass = view === "plans" ? "bad" : "neutral"; + const planStatusMessage = view === "plans" ? `✕ ${message}` : ""; + if (planStatusOwner === undefined) claimPlanStatus(planStatusClass, planStatusMessage); + else setPlanStatus(planStatusOwner, planStatusClass, planStatusMessage); $("selection-actions").hidden = true; $("attachment").hidden = true; $("banner").textContent = message; @@ -138,6 +141,7 @@ async function refresh() { rememberDraft(); showFailure( `Could not read this branch’s history. ${error.message} Use Refresh to retry.`, + planStatusOwner, ); } finally { endBusy(busyOwner); From b32894fcd1d2f3926a6fd401fa44fa7fdfb691ba Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 01:10:59 -0700 Subject: [PATCH 12/18] Keep merge queue progress across async responses --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 150 +++++++++++++++++++++++++ web/public/app.js | 10 +- 3 files changed, 160 insertions(+), 4 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index e2537ffa..4a6bd814 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Full Review, Plans and post-import reload responses invalidate merge polls that return afterward and reconcile a terminal queue observation that applied while the response was pending only when its action ID and reviewed head match. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Full responses also preserve a same-action, same-head queue observation that advanced from submitting to queued or reached a terminal state while the response was pending. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, cancellation settlement, stale merge-queue poll suppression after import reloads, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, submitting-to-queued progress, cancellation settlement, stale merge-queue poll suppression after import reloads and review actions, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 5c93b74a..f1948f1a 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -703,6 +703,156 @@ test('does not let an older Plans refresh re-enable merge after it commits', asy expect(app.service.store.getMergeAttempt(config.identity)?.state).toBe('merged'); }); +test('does not let a merge poll started during a review action replace restored retry readiness', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-08T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + + let actionPending = false; + let releasePoll!: () => void, pollArrived!: () => void; + const heldPoll = new Promise(resolve => { releasePoll = resolve; }); + const capturedPoll = new Promise(resolve => { pollArrived = resolve; }); + const queued = { kind: 'queue', state: 'queued', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: 'https://github.com/example/repo/pull/7', reason: null, phase: 'QUEUED', position: 1, + occurredAt: '2026-10-08T08:00:00Z', retryable: false }; + await page.route('**/api/merge', async route => { + if (!actionPending) { + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: queued }) }); + return; + } + pollArrived(); + await heldPoll; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: queued }) }); + }); + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + app.service.store.finishMergeAttempt(config.identity, attempt.id, { state: 'removed', reason: 'Required check failed.' }); + const retryReview = await page.request.get(new URL('/api/review', app.url).toString(), { + headers: { 'x-codeboost-token': app.token }, + }); + const actionResponse = await retryReview.json(); + expect(actionResponse.merge).toMatchObject({ ready: true, action: 'retry' }); + + let releaseAction!: () => void, actionArrived!: () => void; + const heldAction = new Promise(resolve => { releaseAction = resolve; }); + const capturedAction = new Promise(resolve => { actionArrived = resolve; }); + await page.route('**/api/action', async route => { + actionPending = true; + actionArrived(); + await heldAction; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(actionResponse) }); + }); + + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await page.getByLabel('Change to request').fill('Keep retry readiness after this response.'); + await page.getByRole('button', { name: 'Save change request', exact: true }).click(); + await capturedAction; + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await capturedPoll; + const actionFinished = page.waitForResponse(response => response.url().endsWith('/api/action')); + releaseAction(); + await (await actionFinished).finished(); + await expect(page.getByRole('button', { name: 'Retry merge', exact: true })).toBeEnabled(); + + const stalePollFinished = page.waitForResponse(response => response.url().endsWith('/api/merge')); + releasePoll(); + await (await stalePollFinished).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Retry merge', exact: true })).toBeEnabled(); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); +}); + +test('does not let an older Plans refresh replace a confirmed queued merge with submitting', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_2', phase: 'QUEUED', position: 2, + enqueuedAt: '2026-10-08T08:05:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Submitting…', exact: true })).toBeDisabled(); + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const stale = await response.json(); + expect(stale.merge.queue).toMatchObject({ actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'submitting' }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(stale) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + let polls = 0, releaseLaterPolls!: () => void; + const heldLaterPolls = new Promise(resolve => { releaseLaterPolls = resolve; }); + await page.route('**/api/merge', async route => { + polls++; + if (polls > 1) await heldLaterPolls; + await route.continue(); + }); + try { + const queuedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await (await queuedPoll).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + + const staleRefresh = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await staleRefresh).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued' }); + } finally { + releaseLaterPolls(); + } +}); + test('does not let merge polls started during Review or Plans refresh overwrite full responses', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 52bf92d6..70c30f2c 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -241,6 +241,11 @@ async function act(command) { // One action ID per user action: the server replays it exactly and records feedback with it. const updated = await api("/api/action", { ...command, token: data.token, actionId: crypto.randomUUID() }); if (generation !== reviewGeneration) return true; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; rememberDraft(); data = updated; render(); @@ -810,11 +815,12 @@ function preserveSettledQuestionAnswers(updated) { } function newerMergeQueue(updated) { const newer = data?.merge?.queue, older = updated.merge?.queue; + const progressed = newer?.state === "queued" && older?.state === "submitting" || + ["merged", "removed", "failed"].includes(newer?.state) && ["submitting", "queued"].includes(older?.state); return newer?.actionId && newer.actionId === older?.actionId && newer.reviewedHead === older.reviewedHead && - ["merged", "removed", "failed"].includes(newer.state) && - ["submitting", "queued"].includes(older.state) + progressed ? newer : null; } From 28b671607c28669326e83a212cd2351c8271c2d2 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 01:28:42 -0700 Subject: [PATCH 13/18] Preserve queue progress over action responses --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 71 ++++++++++++++++++++++++++ web/public/app.js | 4 +- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 4a6bd814..7e5d7dc8 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Full responses also preserve a same-action, same-head queue observation that advanced from submitting to queued or reached a terminal state while the response was pending. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Those responses also preserve a same-action, same-head queue observation that advanced from submitting to queued or reached a terminal state while the response was pending. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, submitting-to-queued progress, cancellation settlement, stale merge-queue poll suppression after import reloads and review actions, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index f1948f1a..b123369f 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -784,6 +784,77 @@ test('does not let a merge poll started during a review action replace restored expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); }); +test('does not let an older review action response replace a confirmed queued merge with submitting', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_3', phase: 'QUEUED', position: 3, + enqueuedAt: '2026-10-08T08:10:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + const staleReview = await page.request.get(new URL('/api/review', app.url).toString(), { + headers: { 'x-codeboost-token': app.token }, + }); + const actionResponse = await staleReview.json(); + expect(actionResponse.merge.queue).toMatchObject({ actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'submitting' }); + + let releaseAction!: () => void, actionArrived!: () => void; + const heldAction = new Promise(resolve => { releaseAction = resolve; }); + const capturedAction = new Promise(resolve => { actionArrived = resolve; }); + await page.route('**/api/action', async route => { + actionArrived(); + await heldAction; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(actionResponse) }); + }); + let polls = 0, releaseLaterPolls!: () => void; + const heldLaterPolls = new Promise(resolve => { releaseLaterPolls = resolve; }); + await page.route('**/api/merge', async route => { + polls++; + if (polls > 1) await heldLaterPolls; + await route.continue(); + }); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Submitting…', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await page.getByLabel('Change to request').fill('Preserve queue progress after this response.'); + await page.getByRole('button', { name: 'Save change request', exact: true }).click(); + await capturedAction; + try { + const queuedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await (await queuedPoll).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + + const staleAction = page.waitForResponse(response => response.url().endsWith('/api/action')); + releaseAction(); + await (await staleAction).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued' }); + } finally { + releaseLaterPolls(); + } +}); + test('does not let an older Plans refresh replace a confirmed queued merge with submitting', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 70c30f2c..e32ba22e 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -247,8 +247,10 @@ async function act(command) { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - data = updated; + const newerQueue = newerMergeQueue(updated); + data = newerQueue ? withMergeQueue(updated, newerQueue) : updated; render(); + if (newerQueue) showMergeQueueStatus(newerQueue); return true; } catch (error) { if (generation !== reviewGeneration) return false; From a48861df8a76a89199aaf8ed3bf1ccab81423189 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 01:57:13 -0700 Subject: [PATCH 14/18] Track accepted merge poll ownership --- docs/implementation/planning-screen.md | 2 +- test/browser/plans.spec.ts | 40 ++++++++++++++++++++------ web/public/app.js | 28 +++++++++++------- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 7e5d7dc8..4739d28a 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,7 +10,7 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Those responses also preserve a same-action, same-head queue observation that advanced from submitting to queued or reached a terminal state while the response was pending. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index b123369f..e3259736 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -784,7 +784,7 @@ test('does not let a merge poll started during a review action replace restored expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); }); -test('does not let an older review action response replace a confirmed queued merge with submitting', async ({ page }) => { +test('does not let an older review action response replace a newer queued observation', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; await app.close(); @@ -799,7 +799,7 @@ test('does not let an older review action response replace a confirmed queued me }, queueWatermark: async () => null, merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), - inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_3', phase: 'QUEUED', position: 3, + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_3', phase: 'QUEUED', position: 2, enqueuedAt: '2026-10-08T08:10:00Z', queueHead: head }), }; app = appRef = await startServer(config, 0, undefined, gateway); @@ -809,16 +809,19 @@ test('does not let an older review action response replace a confirmed queued me for (const item of review.items) review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); - const staleReview = await page.request.get(new URL('/api/review', app.url).toString(), { - headers: { 'x-codeboost-token': app.token }, - }); - const actionResponse = await staleReview.json(); - expect(actionResponse.merge.queue).toMatchObject({ actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'submitting' }); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + app.service.store.observeQueuedMerge(config.identity, attempt.id, { entryId: 'MQE_3', phase: 'QUEUED', position: 3 }); let releaseAction!: () => void, actionArrived!: () => void; const heldAction = new Promise(resolve => { releaseAction = resolve; }); const capturedAction = new Promise(resolve => { actionArrived = resolve; }); + let actionPending = false; await page.route('**/api/action', async route => { + const response = await route.fetch(); + const actionResponse = await response.json(); + expect(actionResponse.merge.queue).toMatchObject({ actionId: attempt.actionId, reviewedHead: review.snapshot.head, + state: 'queued', phase: 'QUEUED', position: 3 }); + actionPending = true; actionArrived(); await heldAction; await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(actionResponse) }); @@ -826,30 +829,49 @@ test('does not let an older review action response replace a confirmed queued me let polls = 0, releaseLaterPolls!: () => void; const heldLaterPolls = new Promise(resolve => { releaseLaterPolls = resolve; }); await page.route('**/api/merge', async route => { + if (!actionPending) { + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + kind: 'queue', state: 'queued', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: 'https://github.com/example/repo/pull/7', reason: null, phase: 'QUEUED', position: 3, + occurredAt: '2026-10-08T08:05:00Z', retryable: false, + } }) }); + return; + } polls++; if (polls > 1) await heldLaterPolls; await route.continue(); }); await page.goto(app.url); - await expect(page.getByRole('button', { name: 'Submitting…', exact: true })).toBeDisabled(); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); await page.getByRole('button', { name: 'Request change', exact: true }).click(); await page.getByLabel('Change to request').fill('Preserve queue progress after this response.'); await page.getByRole('button', { name: 'Save change request', exact: true }).click(); await capturedAction; try { - const queuedPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + const queuedPoll = page.waitForResponse(async response => response.url().endsWith('/api/merge') && + (await response.json()).queue?.position === 2); await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); await (await queuedPoll).finished(); await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('position 2'); + await page.getByRole('button', { name: 'Close', exact: true }).click(); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued', position: 2 }); const staleAction = page.waitForResponse(response => response.url().endsWith('/api/action')); releaseAction(); await (await staleAction).finished(); await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await expect(page.getByText('Preserve queue progress after this response.', { exact: true })).toBeVisible(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('position 2'); + await page.getByRole('button', { name: 'Close', exact: true }).click(); expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued' }); + expect(app.service.store.getReviewNotes(config.identity).some(note => note.text === 'Preserve queue progress after this response.')).toBe(true); } finally { releaseLaterPolls(); } diff --git a/web/public/app.js b/web/public/app.js index e32ba22e..8c21d45d 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -25,6 +25,7 @@ let data, let reviewGeneration = 0; let view = "review"; let mergeGeneration = 0, + mergeObservationGeneration = 0, mergePollTimer = null, mergePollState = null, mergePollDelay = 2000; @@ -114,7 +115,10 @@ async function refresh() { const planStatusOwner = planStatusGeneration; const busyOwner = beginBusy("read"); const generation = ++reviewGeneration; + const mergeObservationOwner = mergeObservationGeneration; mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; mergePollState = null; mergePollDelay = 2000; renderAttachment(); @@ -129,7 +133,7 @@ async function refresh() { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - const newerQueue = newerMergeQueue(updated); + const newerQueue = newerMergeQueue(updated, mergeObservationOwner); data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); snippetSelection = null; selected ??= data.items[0]?.id || "Unplanned"; @@ -194,6 +198,7 @@ async function pollMergeQueue(generation) { const update = await api("/api/merge"); if (generation !== mergeGeneration || !data?.merge?.available) return; const queue = update.queue; + mergeObservationGeneration++; data = withMergeQueue(data, queue); showMergeQueueStatus(queue); if (queue?.state === mergePollState) backOffMergePoll(); @@ -232,7 +237,10 @@ async function act(command) { supersedePlanRefresh(); const busyOwner = beginBusy("write"); const generation = ++reviewGeneration; + const mergeObservationOwner = mergeObservationGeneration; mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; mergePollState = null; mergePollDelay = 2000; renderAttachment(); @@ -247,7 +255,7 @@ async function act(command) { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - const newerQueue = newerMergeQueue(updated); + const newerQueue = newerMergeQueue(updated, mergeObservationOwner); data = newerQueue ? withMergeQueue(updated, newerQueue) : updated; render(); if (newerQueue) showMergeQueueStatus(newerQueue); @@ -815,14 +823,12 @@ function preserveSettledQuestionAnswers(updated) { : note; }) }; } -function newerMergeQueue(updated) { +function newerMergeQueue(updated, observationOwner) { const newer = data?.merge?.queue, older = updated.merge?.queue; - const progressed = newer?.state === "queued" && older?.state === "submitting" || - ["merged", "removed", "failed"].includes(newer?.state) && ["submitting", "queued"].includes(older?.state); - return newer?.actionId && + return observationOwner !== mergeObservationGeneration && + newer?.actionId && newer.actionId === older?.actionId && - newer.reviewedHead === older.reviewedHead && - progressed + newer.reviewedHead === older.reviewedHead ? newer : null; } @@ -1004,6 +1010,7 @@ async function importPlan(event) { : `✓ Imported revision r${revision}. Reloading…`); try { rememberDraft(); + const mergeObservationOwner = mergeObservationGeneration; const updated = await api("/api/review"); if (sharedGeneration !== reviewGeneration) return; mergeGeneration++; @@ -1012,7 +1019,7 @@ async function importPlan(event) { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - const newerQueue = newerMergeQueue(updated); + const newerQueue = newerMergeQueue(updated, mergeObservationOwner); data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); render(); if (newerQueue) showMergeQueueStatus(newerQueue); @@ -1230,6 +1237,7 @@ $("plans-refresh").onclick = async () => { if (busy) return; const generation = ++planOperationGeneration; let sharedGeneration = ++reviewGeneration; + const mergeObservationOwner = mergeObservationGeneration; mergeGeneration++; if (mergePollTimer) clearTimeout(mergePollTimer); mergePollTimer = null; @@ -1250,7 +1258,7 @@ $("plans-refresh").onclick = async () => { mergePollState = null; mergePollDelay = 2000; rememberDraft(); - const newerQueue = newerMergeQueue(updated); + const newerQueue = newerMergeQueue(updated, mergeObservationOwner); data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); render(); if (newerQueue) showMergeQueueStatus(newerQueue); From ff74fde4b3184cec710f55439095e05dae4621f8 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 02:15:15 -0700 Subject: [PATCH 15/18] Test merge queue identity boundaries --- test/browser/plans.spec.ts | 92 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index e3259736..b0b58a3f 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -877,6 +877,98 @@ test('does not let an older review action response replace a newer queued observ } }); +for (const mismatch of [ + { label: 'action ID', change: (queue: Record) => ({ ...queue, actionId: randomUUID() }) }, + { label: 'reviewed head', change: (queue: Record) => ({ ...queue, reviewedHead: 'f'.repeat(40) }) }, +]) { + test(`does not preserve a newer queue observation with a different ${mismatch.label}`, async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_identity', phase: 'QUEUED', position: 3, + enqueuedAt: '2026-10-08T08:15:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + app.service.store.observeQueuedMerge(config.identity, attempt.id, { entryId: 'MQE_identity', phase: 'QUEUED', position: 3 }); + const observed = mismatch.change({ + kind: 'queue', state: 'queued', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: 'https://github.com/example/repo/pull/7', reason: null, phase: 'QUEUED', position: 2, + occurredAt: '2026-10-08T08:16:00Z', retryable: false, + }); + + let releaseAction!: () => void, actionArrived!: () => void; + const heldAction = new Promise(resolve => { releaseAction = resolve; }); + const capturedAction = new Promise(resolve => { actionArrived = resolve; }); + await page.route('**/api/action', async route => { + const response = await route.fetch(); + const actionResponse = await response.json(); + expect(actionResponse.merge.queue).toMatchObject({ actionId: attempt.actionId, reviewedHead: review.snapshot.head, + state: 'queued', position: 3 }); + actionArrived(); + await heldAction; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(actionResponse) }); + }); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + const note = `Keep the action response for a different ${mismatch.label}.`; + await page.getByLabel('Change to request').fill(note); + await page.getByRole('button', { name: 'Save change request', exact: true }).click(); + await capturedAction; + + let polls = 0, releaseLaterPolls!: () => void; + const heldLaterPolls = new Promise(resolve => { releaseLaterPolls = resolve; }); + await page.route('**/api/merge', async route => { + polls++; + if (polls > 1) await heldLaterPolls; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: observed }) }); + }); + try { + const mismatchedPoll = page.waitForResponse(async response => response.url().endsWith('/api/merge') && + (await response.json()).queue?.position === 2); + await page.getByRole('button', { name: /P2 Document retry behavior/ }).click(); + await (await mismatchedPoll).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('position 2'); + await page.getByRole('button', { name: 'Close', exact: true }).click(); + + const staleAction = page.waitForResponse(response => response.url().endsWith('/api/action')); + releaseAction(); + await (await staleAction).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await expect(page.getByText(note, { exact: true })).toBeVisible(); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('position 3'); + await page.getByRole('button', { name: 'Close', exact: true }).click(); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, reviewedHead: review.snapshot.head, position: 3 }); + expect(app.service.store.getReviewNotes(config.identity).some(saved => saved.text === note)).toBe(true); + } finally { + releaseLaterPolls(); + } + }); +} + test('does not let an older Plans refresh replace a confirmed queued merge with submitting', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; From 993599a2a1ad9cc578f5db26c7b4a1fbd4b83c80 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 02:51:57 -0700 Subject: [PATCH 16/18] Preserve verified merge retry readiness --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 76 ++++++++++++++++++++++++++ web/public/app.js | 4 +- 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 4739d28a..31d33980 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. When the poll and full response report the same terminal state, the full response remains authoritative so its server-verified retry readiness is not discarded. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, full-response retry readiness after a matching terminal poll, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index b0b58a3f..8aba15e7 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -784,6 +784,82 @@ test('does not let a merge poll started during a review action replace restored expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); }); +test('lets a full refresh verify retry readiness after a matching terminal poll', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-08T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + app.service.store.finishMergeAttempt(config.identity, attempt.id, { state: 'removed', reason: 'Required check failed.' }); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + expect(refreshed.merge).toMatchObject({ ready: true, action: 'retry', queue: { + actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'removed', + } }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + const terminalQueue = { + kind: 'queue', state: 'removed', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: 'https://github.com/example/repo/pull/7', reason: 'Required check failed.', phase: null, position: null, + occurredAt: '2026-10-08T08:05:00Z', retryable: true, + }; + await page.route('**/api/merge', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ queue: terminalQueue }), + })); + const terminalPoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await terminalPoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#banner')).toContainText('Refresh to verify retry readiness.'); + await expect(page.getByRole('button', { name: /blocker/, exact: true })).toBeDisabled(); + await page.unroute('**/api/merge'); + + const refreshResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await refreshResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Retry merge', exact: true })).toBeEnabled(); + await expect(page.locator('#banner')).not.toContainText('Refresh to verify retry readiness.'); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); +}); + test('does not let an older review action response replace a newer queued observation', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index 8c21d45d..b2ec5a45 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -825,10 +825,12 @@ function preserveSettledQuestionAnswers(updated) { } function newerMergeQueue(updated, observationOwner) { const newer = data?.merge?.queue, older = updated.merge?.queue; + const sameTerminalState = newer?.state === older?.state && ["merged", "removed", "failed"].includes(newer?.state); return observationOwner !== mergeObservationGeneration && newer?.actionId && newer.actionId === older?.actionId && - newer.reviewedHead === older.reviewedHead + newer.reviewedHead === older.reviewedHead && + !sameTerminalState ? newer : null; } From 7883fc3ab2514cf3f871188747b80d84a24628bb Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 03:25:08 -0700 Subject: [PATCH 17/18] Prefer terminal merge refresh state --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 73 ++++++++++++++++++++++++++ web/public/app.js | 4 +- 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 31d33980..89f3131b 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. When the poll and full response report the same terminal state, the full response remains authoritative so its server-verified retry readiness is not discarded. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. A terminal full response remains authoritative over either active or matching-terminal poll state so its verified terminal result, blockers and retry readiness are not discarded. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, full-response retry readiness after a matching terminal poll, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, terminal full-response authority over matching terminal and active polls, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 8aba15e7..94d509eb 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -860,6 +860,79 @@ test('lets a full refresh verify retry readiness after a matching terminal poll' expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); }); +test('lets a terminal full response replace an active poll for the same merge attempt', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 1, + enqueuedAt: '2026-10-08T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + app.service.store.finishMergeAttempt(config.identity, attempt.id, { state: 'removed', reason: 'Required check failed.' }); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + expect(refreshed.merge).toMatchObject({ ready: true, action: 'retry', queue: { + actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'removed', + } }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + await page.route('**/api/merge', route => route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ queue: { + kind: 'queue', state: 'queued', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: 'https://github.com/example/repo/pull/7', reason: null, phase: 'QUEUED', position: 2, + occurredAt: '2026-10-08T08:05:00Z', retryable: false, + } }), + })); + const activePoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await activePoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#banner')).toContainText('Merge queued at position 2.'); + + const refreshResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await refreshResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Retry merge', exact: true })).toBeEnabled(); + await expect(page.locator('#banner')).not.toContainText('Merge queued at position 2.'); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); +}); + test('does not let an older review action response replace a newer queued observation', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index b2ec5a45..b57e6dff 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -825,12 +825,12 @@ function preserveSettledQuestionAnswers(updated) { } function newerMergeQueue(updated, observationOwner) { const newer = data?.merge?.queue, older = updated.merge?.queue; - const sameTerminalState = newer?.state === older?.state && ["merged", "removed", "failed"].includes(newer?.state); + const fullResponseIsTerminal = ["merged", "removed", "failed"].includes(older?.state); return observationOwner !== mergeObservationGeneration && newer?.actionId && newer.actionId === older?.actionId && newer.reviewedHead === older.reviewedHead && - !sameTerminalState + !fullResponseIsTerminal ? newer : null; } From 8eb624f77bcd383b96aac446d7996543dbc446c4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 03:59:06 -0700 Subject: [PATCH 18/18] Keep confirmed merge queue progress --- docs/implementation/planning-screen.md | 4 +- test/browser/plans.spec.ts | 86 ++++++++++++++++++++++++++ web/public/app.js | 4 +- 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 89f3131b..a88f066f 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -10,12 +10,12 @@ The screen renders the complete current plan: issue, summary, revision, open que ## Import contract -The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. A terminal full response remains authoritative over either active or matching-terminal poll state so its verified terminal result, blockers and retry readiness are not discarded. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. +The person selects a `.json`, `.yaml` or `.yml` file. Its extension chooses the existing import format; the browser sends the file contents, current revision and a UUID action ID to F1's import endpoint. A retry after an ambiguous transport outcome retains the exact action ID and request by format and file contents, even when another file is definitely refused or a refresh has since observed the first request's commit. A definite refusal clears only that file's retry. A successful import is reported as committed before the browser reloads the current plan, so a reload failure is not described as an import failure. The focused submit control stays enabled with `aria-disabled` and an in-flight guard. If the person selects another file while the request runs, the older response does not clear that newer selection. Every full review response uses one shared generation: an import or merge advances it, so a Review or Plans refresh captured before that write cannot replace the committed state in the UI. Review actions and full Review, Plans and post-import reload responses invalidate merge polls that return afterward. Each captures the accepted-poll generation before its request and preserves a same-action, same-head queue observation accepted while it was pending, including submitting-to-queued, terminal, position, phase and observation-error updates. A terminal full response remains authoritative over either active or matching-terminal poll state so its verified terminal result, blockers and retry readiness are not discarded. A queued full response likewise wins over a stale submitting observation. Plans and post-import reloads do the same for completed or failed answers with the same note and answer-attempt IDs, including cancellation settlement, while keeping fresh blockers and stale-context metadata from the full response. Superseding work releases the stale operation's UI ownership immediately, while a failed plan operation resumes any current merge-queue polling it interrupted. Writes are kept single-flight across the Review and Plans screens. The Store validates schema, issue identity, revision, paths, dependencies and commands. G1 does not duplicate those rules in JavaScript. A rejected import leaves the rendered and durable revision unchanged. ## Acceptance -`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, terminal full-response authority over matching terminal and active polls, submitting-to-queued progress, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. +`test/browser/plans.spec.ts` covers JSON and YAML imports, full plan and rename rendering, navigation/reload, unsent Review draft preservation, duplicate activation and focus, a newer file selection surviving an older response, exact ambiguous retry before and after a refresh observes a lost-response commit, retry preservation across another file's refusal, committed-import/reload-failure reporting, post-import Review writes while a superseded read remains unsettled, immediate stale Plans UI release after an irreversible merge, both orderings of Review/Plans refreshes against terminal merge and question observations, terminal full-response authority over matching terminal and active polls, submitting-to-queued progress in both response orders, cancellation settlement, both response orderings between review actions and merge polls, queue-poll resumption after a rejected import, and an issue-mismatch refusal that leaves the Store at the original revision. G2 adds authoring and suggestion cards. G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 94d509eb..e9d9f76f 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -933,6 +933,92 @@ test('lets a terminal full response replace an active poll for the same merge at expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'removed' }); }); +test('lets a queued full response replace a stale submitting poll for the same merge attempt', async ({ page }) => { + test.slow(); + const config = { ...app.service.config, demo: false }; + await app.close(); + chmodSync(join(config.repository, 'run.sh'), 0o644); + fixtureGit(config.repository, 'commit', '-am', 'Restore declared scope'); + let appRef: typeof app; + const gateway: MergeGateway & MergeQueueGateway = { + inspect: async () => { + const snapshot = appRef.service.load().snapshot; + return { base: snapshot.base, head: snapshot.head, pullRequestState: 'OPEN', mergeable: 'MERGEABLE', rulesKnown: true, + atomicBaseGuard: true, mergeQueue: true, requiredChecks: [], alreadyFixed: 'clear', pullRequest: 7, draft: false }; + }, + queueWatermark: async () => null, + merge: async () => ({ url: 'https://github.com/example/repo/pull/7' }), + inspectQueue: async head => ({ state: 'queued', reviewedHead: head, entryId: 'MQE_1', phase: 'QUEUED', position: 3, + enqueuedAt: '2026-10-08T08:00:00Z', queueHead: head }), + }; + app = appRef = await startServer(config, 0, undefined, gateway); + let review = app.service.load(); + for (const segment of review.segments.filter(value => value.row === 'Unplanned' || value.row === 'Ambiguous')) + review = app.service.act({ action: 'accept', key: segment.key, token: review.token }); + for (const item of review.items) + review = app.service.act({ action: 'approve', item: item.id, confirmNoChange: item.count === 0, token: review.token }); + const attempt = app.service.store.beginMergeAttempt(config.identity, review.expected as never, review.snapshot.head, null, 'queue', randomUUID()); + + await page.goto(app.url); + await expect(page.getByRole('button', { name: 'Submitting…', exact: true })).toBeDisabled(); + app.service.store.queueMergeAttempt(config.identity, attempt.id, 'https://github.com/example/repo/pull/7'); + app.service.store.observeQueuedMerge(config.identity, attempt.id, { entryId: 'MQE_1', phase: 'QUEUED', position: 3 }); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const capturedRefresh = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + const response = await route.fetch(); + const refreshed = await response.json(); + expect(refreshed.merge.queue).toMatchObject({ + actionId: attempt.actionId, reviewedHead: review.snapshot.head, state: 'queued', phase: 'QUEUED', position: 3, + }); + refreshArrived(); + await heldRefresh; + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify(refreshed) }); + }); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await page.locator('#plans-refresh').click(); + await capturedRefresh; + + let polls = 0, releaseLaterPolls!: () => void; + const heldLaterPolls = new Promise(resolve => { releaseLaterPolls = resolve; }); + await page.route('**/api/merge', async route => { + if (polls++ > 0) { + await heldLaterPolls; + await route.continue(); + return; + } + await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ queue: { + kind: 'queue', state: 'submitting', actionId: attempt.actionId, reviewedHead: review.snapshot.head, + url: null, reason: null, phase: null, position: null, occurredAt: '2026-10-08T08:05:00Z', retryable: false, + observationError: 'The older queue inspection failed.', + } }) }); + }); + const stalePoll = page.waitForResponse(response => response.url().endsWith('/api/merge')); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: /P1 Bound exponential retries/ }).click(); + await stalePoll; + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.locator('#banner')).toContainText('The older queue inspection failed.'); + + try { + const refreshResponse = page.waitForResponse(response => response.url().endsWith('/api/review')); + releaseRefresh(); + await (await refreshResponse).finished(); + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => resolve())))); + await expect(page.getByRole('button', { name: 'Merge queued', exact: true })).toBeDisabled(); + await expect(page.locator('#banner')).not.toContainText('The older queue inspection failed.'); + await page.getByRole('button', { name: 'Merge status', exact: true }).click(); + await expect(page.locator('#dialog-body')).toContainText('queued'); + await expect(page.locator('#dialog-body')).toContainText('position 3'); + await expect(page.locator('#dialog-body')).not.toContainText('The older queue inspection failed.'); + expect(app.service.store.getMergeAttempt(config.identity)).toMatchObject({ id: attempt.id, state: 'queued', position: 3 }); + } finally { + releaseLaterPolls(); + } +}); + test('does not let an older review action response replace a newer queued observation', async ({ page }) => { test.slow(); const config = { ...app.service.config, demo: false }; diff --git a/web/public/app.js b/web/public/app.js index b57e6dff..60f6c423 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -826,11 +826,13 @@ function preserveSettledQuestionAnswers(updated) { function newerMergeQueue(updated, observationOwner) { const newer = data?.merge?.queue, older = updated.merge?.queue; const fullResponseIsTerminal = ["merged", "removed", "failed"].includes(older?.state); + const pollRegressesQueued = older?.state === "queued" && newer?.state === "submitting"; return observationOwner !== mergeObservationGeneration && newer?.actionId && newer.actionId === older?.actionId && newer.reviewedHead === older.reviewedHead && - !fullResponseIsTerminal + !fullResponseIsTerminal && + !pollRegressesQueued ? newer : null; }