From 14eea01fa475c720889eddd7e97e5f3aafaeaedd Mon Sep 17 00:00:00 2001 From: mchwang Date: Fri, 23 Oct 2026 12:45:07 -0700 Subject: [PATCH 01/39] Add review-screen controls for the runner actions (#200) The review header shows the task's status, one run action the server would admit now (Start, Resume, Publish PR, Publish draft PR, Close pull requests), Stop for the item in progress, Retry, and Cancel task with an inline confirmation. Tasks waiting on a person get the design's banner; needs amendment offers Approve continuation and Review plan change. Review anyway and Continue have no server action yet (#206). The page polls GET /api/runner only while work is in progress, applies only the latest read or answer, skips reads while an action is in flight, and keeps an action ID for resends until a definite answer. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.md | 2 +- docs/designs/codeboost-plan-indexed-review.md | 2 +- test/browser/runner-controls.spec.ts | 350 ++++++++++++++++++ web/public/app.js | 270 ++++++++++++++ web/public/index.html | 11 + web/public/style.css | 24 ++ 6 files changed, 657 insertions(+), 2 deletions(-) create mode 100644 test/browser/runner-controls.spec.ts diff --git a/docs/architecture.md b/docs/architecture.md index 3ec52c13..332a6a5e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -44,7 +44,7 @@ Writing standard: plain language, ISO 24495-1:2023 - **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). `prepare-merge` refreshes the published PR's exact base/head, rebases through the durable F3/F4 path, recomputes attribution and approvals, pushes a rewritten head whose review is still clear over exactly the PR head it rewrote, and runs allowlisted `cmd:` checks in a credential-free read-only container bound to the resulting head (#22). The **Merge PR** button (the design's Approve & merge) merges that pull request (#121). Cancelling a task closes its pull requests (#111). Apart from **Merge PR** and **Cancel merge** (which stops a running merge job without cancelling the task), these are API actions; the screen has no buttons for them yet. - **In use for pre-merge automation (#22, closed):** 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). `prepare-merge` now invokes that path after current pull-request base/head reads, refreshes attribution and approvals, pushes the rewritten head behind a durable push marker (F6a, PR #153), and runs exact-head `cmd:` checks. With a runner, **Merge PR** runs one merge job that prepares, waits up to 30 minutes for GitHub's required checks, and hands the exact pair to the guarded merge, whose already-fixed check now covers every base commit after the task's original base (F6b, PR #161, `docs/implementation/merge-handoff.md`). `test` and `real-docker` run every offline T9 suite on every pull request, and the live Claude probes run nightly (F6c, PR #177, `docs/implementation/ci.md`). - - **Not built:** screens for the runner actions, review rounds, and the Queue and Learning screens. + - **Not built:** review rounds, and the Queue and Learning screens. The review screen has controls for the runner actions (#200); "Review anyway" and "Continue" for tasks that wait on a person are not built (#206). ## Terms used diff --git a/docs/designs/codeboost-plan-indexed-review.md b/docs/designs/codeboost-plan-indexed-review.md index 47d112c3..9fad48f6 100644 --- a/docs/designs/codeboost-plan-indexed-review.md +++ b/docs/designs/codeboost-plan-indexed-review.md @@ -569,7 +569,7 @@ The numbers below identify delivery milestones, not a requirement to implement t 2. **Read-only review screen.** It works on any branch whose commits are in the commit ledger, with a plan loaded into the database. It shows rows, segments, the four checks, approvals, and the per-item conversation. It does not merge (engineering review, O8). *Done; the screen has since gained the merge gate from build step 4.* 3. **Optional validation.** A future real-PR comparison may test the assumptions in "How we will know it works," but it is non-blocking under the superseding decision above. 4. **Merge gate and merging** (product workflow step 9): the merge rules, the pre-merge sequence, and merging through `gh`. Increment 1 (PR #23), merge-queue support (#38, #46, closing #24) and the pre-merge automation (lane F, F3–F6, #22) have merged. *Done:* the last items are closed: #176 (PR #195), #174 (PR #197), and the merge job's step list and Cancel button (PR #194). The rest of D16 and DT7 (named failure banners, the Queue-row step, the blocker list with links) is tracked in #199. See the scope and completion criteria below. -5. **Running agents.** Per-task clones, containers, agent adapters, permissions, one invocation per plan item, review rounds, the "already fixed" check, and opening PRs (step 6). Isolation boundary merged (lane D, PRs #31, #40, #44, #47, #50), and the runner merged (lane F; see the lane table in `docs/architecture.md`). Not built: review rounds and screen buttons for the runner actions. +5. **Running agents.** Per-task clones, containers, agent adapters, permissions, one invocation per plan item, review rounds, the "already fixed" check, and opening PRs (step 6). Isolation boundary merged (lane D, PRs #31, #40, #44, #47, #50), and the runner merged (lane F; see the lane table in `docs/architecture.md`). The review screen has controls for the runner actions (#200). Not built: review rounds, and the "Review anyway" and "Continue" actions (#206). 6. **Planning screen.** Writing plans with an agent, and approving plan changes (steps 2 and 3). *Done for T18:* planning backend (lane E, E1–E4) and the Plans screen (lane G, G1–G4, #145; PRs #148, #150, #151, #152). Not built: paste import, in-screen editing, the amendment-proposal view, check warnings, answering questions and a whole-plan approval step (#155). The suggestion cards do not yet conform to the v1 contract (#159). 7. **Queue, schedule, and recovery** (steps 4 and 5). 8. **Issue list, sorted by how critical each issue is** (step 1). *Done:* ranking backend (H1–H3), the Issues screen (H4a, #55), and the author-bound "trust this issue" action and runner guard (H4b, #108, PR #139) have merged. diff --git a/test/browser/runner-controls.spec.ts b/test/browser/runner-controls.spec.ts new file mode 100644 index 00000000..e35fbd1d --- /dev/null +++ b/test/browser/runner-controls.spec.ts @@ -0,0 +1,350 @@ +import { test, expect, type Page, type Request } from '@playwright/test'; +import { 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 { ReviewService } from '../../runner/review.ts'; +import type { RunnerDeps } from '../../runner/coordinator.ts'; +import { SafetyFindings, type ExecutionSources } from '../../runner/execution.ts'; +import { approveItem } from '../../core/approvals.ts'; +import { openReview, refreshReview } from './review-page.ts'; + +// Runner controls (#200) against a real server and runner. The runner's preparation always fails, so an admitted item +// ends `failed` without an agent (as in test/runner-start.test.ts): enough for start, resume, cancel and their guards. +type App = Awaited>; +let root: string, app: App; +const OWNER = 'd'.repeat(32); +async function approvePlan(service: ReviewService) { + const view = await service.load(); + service.store.saveReview(service.config.identity, view.expected, + view.items.map(item => approveItem(view.plan, view.segments, item.id, service.config.identity, item.count === 0)), []); +} +/** `prepare` decides when the item's preparation fails, so a test can hold an attempt in progress. */ +async function serve(options: { approve?: boolean; prepare?: () => Promise } = {}) { + root = mkdtempSync(join(tmpdir(), 'codeboost-runner-ui-')); + app = await startServer(createDemo(join(root, 'demo')), 0, undefined, undefined, 2_000, undefined, undefined, undefined, async service => { + if (options.approve !== false) await approvePlan(service); + const deps: RunnerDeps = { runnerOwner: OWNER, kinds: ['execute'], prepare: options.prepare ?? (async () => { throw new Error('no agent here'); }), + cleanupPreparation: async () => undefined, start: () => { throw new Error('no agent here'); }, validate: () => null }; + const sources: ExecutionSources = { planContext: () => service.planContext(), checkpointContext: () => service.planContext(), + issue: () => ({ text: { number: 1, title: '', body: '', comments: [] }, validate: () => undefined }), lessons: () => [], vendor: () => 'claude' }; + return { deps, sources, findings: new SafetyFindings(service.store), + recovery: { finalized: [], requeue: [], removedDirectories: [], unknownEntries: [], unmatchedStorage: [], repairedMerges: [] } }; + }); + return app; +} +test.afterEach(async () => { await app?.close(); if (root) rmSync(root, { recursive: true, force: true }); }); +const runnerPosts = (page: Page) => { + const posts: Record[] = []; + page.on('request', (request: Request) => { if (request.method() === 'POST' && new URL(request.url()).pathname === '/api/runner') posts.push(request.postDataJSON()); }); + return posts; +}; +const status = (page: Page) => page.locator('#task-status'); + +test('shows no runner controls without a runner', async ({ page }) => { + root = mkdtempSync(join(tmpdir(), 'codeboost-runner-ui-')); + app = await startServer(createDemo(join(root, 'demo')), 0); + const loaded = page.waitForResponse(response => new URL(response.url()).pathname === '/api/runner'); + await openReview(page, app.url); + expect((await (await loaded).json()).available).toBe(false); + for (const id of ['task-status', 'run-action', 'cancel-task', 'attempt-stop', 'attempt-retry', 'task-banner']) await expect(page.locator(`#${id}`)).toBeHidden(); +}); + +test('starts an approved task with the versions on screen, then offers Resume once its item has run', async ({ page }) => { + await serve(); + const posts = runnerPosts(page); + await openReview(page, app.url); + await expect(status(page)).toHaveText('○ In review'); + const start = page.getByRole('button', { name: 'Start', exact: true }); + await expect(start).toBeVisible(); + const review = await app.service.load(), before = app.service.store.getTask(app.service.config.identity).stateVersion; + await start.click(); + await expect.poll(() => posts.length).toBe(1); + expect(posts[0]).toMatchObject({ action: 'start', expectedStateVersion: before, expectedReviewVersion: review.expected.reviewVersion }); + expect(posts[0]!.actionId).toMatch(/^[0-9a-f-]{36}$/); + // The item's preparation fails, so the run ends with that item failed and the task still running: Resume is offered. + await expect(page.getByRole('button', { name: 'Resume', exact: true })).toBeVisible(); + await expect(status(page)).toHaveText('○ Running'); + await expect(page.locator('#runner-note')).toContainText(/Running P1\.|Refresh to review/); +}); + +test('offers no Start while plan items are unapproved, and Cancel task asks once before cancelling', async ({ page }) => { + await serve({ approve: false }); + const posts = runnerPosts(page); + await openReview(page, app.url); + await expect(status(page)).toHaveText('○ In review'); + await expect(page.locator('#run-action')).toBeHidden(); + const cancel = page.locator('#cancel-task'); + await cancel.click(); + await expect(cancel).toHaveText('Confirm cancel'); + await expect(page.locator('#runner-note')).toContainText('cannot be undone'); + // The first press sends nothing: the confirm window is 5 s, and this waits 1 s past the first press's handling. + await page.waitForTimeout(1_000); + expect(posts).toHaveLength(0); + await cancel.click(); + await expect.poll(() => posts.length).toBe(1); + expect(posts[0]!.action).toBe('cancel-task'); + await expect(status(page)).toHaveText('– Cancelled'); + // Cancel hid itself while focused: focus moves to Refresh. + await expect(cancel).toBeHidden(); + await expect(page.locator('#reload')).toBeFocused(); + expect(app.service.store.getTask(app.service.config.identity).status).toBe('cancelled'); +}); + +test('the cancel confirmation lapses after 5 seconds', async ({ page }) => { + await serve(); + await page.clock.install(); + const posts = runnerPosts(page); + await openReview(page, app.url); + const cancel = page.locator('#cancel-task'); + await cancel.click(); + await expect(cancel).toHaveText('Confirm cancel'); + await page.clock.runFor(5_100); + await expect(cancel).toHaveText('Cancel task'); + await expect(page.locator('#runner-note')).toHaveText(''); + // A press after the lapse asks again instead of cancelling. + await cancel.click(); + await expect(cancel).toHaveText('Confirm cancel'); + expect(posts).toHaveLength(0); +}); + +test('resends the same action ID after a 503 or a lost response, and uses a new one after a refusal', async ({ page }) => { + await serve(); + const keys: string[] = [], answers: Array<'503' | 'lost' | 'unknown' | 'pass' | '409'> = ['503', 'lost', 'unknown', 'pass', '409', 'pass']; + await page.route('**/api/runner', async route => { + if (route.request().method() !== 'POST') { await route.continue(); return; } + const body = route.request().postDataJSON(); + keys.push(body.actionId); + const answer = answers[keys.length - 1]; + if (answer === 'lost') { await route.abort('failed'); return; } + if (answer === '503') { await route.fulfill({ status: 503, contentType: 'application/json', body: JSON.stringify({ error: 'The review server is shutting down.' }) }); return; } + if (answer === '409') { await route.fulfill({ status: 409, contentType: 'application/json', body: JSON.stringify({ error: 'Stale task state. Reload before writing.' }) }); return; } + if (answer === 'unknown') { await route.fulfill({ status: 503, contentType: 'application/json', body: JSON.stringify({ error: 'Storage failed.', outcomeUnknown: true }) }); return; } + await route.continue(); + }); + await openReview(page, app.url); + const start = page.getByRole('button', { name: 'Start', exact: true }); + for (let click = 0; click < 3; click++) { + await start.click(); + await expect.poll(() => keys.length).toBe(click + 1); + await expect(page.locator('#runner-note')).toContainText('resend the same request'); + } + await start.click(); + await expect.poll(() => keys.length).toBe(4); + expect(keys.slice(1)).toEqual([keys[0], keys[0], keys[0]]); + const resume = page.getByRole('button', { name: 'Resume', exact: true }); + await expect(resume).toBeVisible(); + // A refusal is a definite answer: the next press of the same action is a new action with a new key. + await resume.click(); + await expect.poll(() => keys.length).toBe(5); + await expect(page.locator('#runner-note')).toHaveText('✕ Stale task state. Reload before writing.'); + await resume.click(); + await expect.poll(() => keys.length).toBe(6); + expect(keys[5]).not.toBe(keys[4]); + expect(keys[4]).not.toBe(keys[0]); +}); + +test('keeps focus on the run action while its request is in flight, ignores a second press, and moves focus to Refresh when it hides', async ({ page }) => { + // The item's preparation is held, so the answer shows it running and the run action has nothing to offer. + let fail!: () => void; + await serve({ prepare: () => new Promise((_, reject) => { fail = () => reject(new Error('no agent here')); }) }); + const posts = runnerPosts(page); + let release!: () => void; + const held = new Promise(resolve => { release = resolve; }); + await page.route('**/api/runner', async route => { + if (route.request().method() !== 'POST') { await route.continue(); return; } + await held; + await route.continue(); + }); + await openReview(page, app.url); + const start = page.getByRole('button', { name: 'Start', exact: true }); + await start.focus(); + await page.keyboard.press('Enter'); + await expect(start).toHaveAttribute('aria-disabled', 'true'); + await page.keyboard.press('Enter'); + await expect(start).toBeFocused(); + release(); + await expect(page.locator('#runner-note')).toHaveText('✓ Running P1.'); + expect(posts).toHaveLength(1); + // The answer shows P1 running: Start hid while focused, so focus is on Refresh, and Stop P1 is offered. + await expect(page.locator('#run-action')).toBeHidden(); + await expect(page.locator('#reload')).toBeFocused(); + await expect(page.getByRole('button', { name: 'Stop P1', exact: true })).not.toHaveAttribute('aria-disabled', 'true'); + fail(); +}); + +test('shows an item in progress with Stop, polls until it settles, and drops a poll read before the stop', async ({ page }) => { + let fail!: () => void; + await serve({ prepare: () => new Promise((_, reject) => { fail = () => reject(new Error('no agent here')); }) }); + const posts = runnerPosts(page); + await openReview(page, app.url); + await page.getByRole('button', { name: 'Start', exact: true }).click(); + const stop = page.getByRole('button', { name: 'Stop P1', exact: true }); + await expect(stop).toBeVisible(); + await expect(status(page)).toHaveText('○ Running · P1'); + // Hold the next poll, stop the item, then let the held poll answer: it was read before the stop, so it is dropped. + let releasePoll!: () => void, pollHeld!: () => void; + const pollSeen = new Promise(resolve => { pollHeld = resolve; }); + const pollGate = new Promise(resolve => { releasePoll = resolve; }); + let heldOne = false, delivered!: () => void; + const heldDelivered = new Promise(resolve => { delivered = resolve; }); + let heldView: Record | undefined; + await page.route('**/api/runner', async route => { + if (route.request().method() === 'GET' && !heldOne) { + heldOne = true; const response = await route.fetch(); heldView = await response.json(); pollHeld(); await pollGate; + await route.fulfill({ response }); delivered(); return; + } + await route.continue(); + }); + await pollSeen; + await stop.click(); + await expect.poll(() => posts.map(post => post.action)).toEqual(['start', 'cancel-attempt']); + await expect(page.locator('#runner-note')).toHaveText('Stopping the item…'); + await expect(status(page)).toContainText('Stopping…'); + // The held poll was read while P1 ran with no stop, so applying it would offer Stop P1 again. + expect(heldView!.stopRequested).toBeNull(); + expect(heldView!.attempts.find((attempt: { id: string }) => attempt.id === heldView!.task.currentAttemptId).state).toMatch(/pending|running/); + releasePoll(); + // Proof the page handled it: the response was delivered, then the page ran a task after its handlers. + await heldDelivered; + await page.evaluate(() => new Promise(resolve => setTimeout(resolve, 50))); + await expect(stop).toBeHidden(); + await expect(status(page)).toContainText('Stopping…'); + fail(); + await expect(page.locator('#runner-note')).toContainText('Refresh to review its latest changes'); + await expect(stop).toBeHidden(); +}); + +/** + * Serves `GET /api/runner` as the real server answered it, with `change` applied. Only for states this runner cannot + * reach (publishing, a scope checkpoint, a retryable non-execute attempt); every field `change` does not set is the + * real server's. Actions still go to the real server. + */ +async function viewWith(page: Page, change: (view: Record) => void) { + await page.route('**/api/runner', async route => { + if (route.request().method() !== 'GET') { await route.continue(); return; } + const response = await route.fetch(), view = await response.json(); + change(view); + await route.fulfill({ response, json: view }); + }); +} + +test('offers Publish draft PR when the server would publish, and shows the server refusal', async ({ page }) => { + await serve(); + const posts = runnerPosts(page); + await viewWith(page, view => { view.startable = false; view.publish = { ...view.publish, available: true, publishable: true, draft: true }; }); + await openReview(page, app.url); + const publish = page.getByRole('button', { name: 'Publish draft PR', exact: true }); + await publish.click(); + await expect.poll(() => posts.map(post => post.action)).toEqual(['publish']); + expect(posts[0]).not.toHaveProperty('expectedReviewVersion'); + // This test server is a demo with no publisher, so the real server refuses; the page shows its message. + await expect(page.locator('#runner-note')).toHaveText('✕ Demos never publish pull requests.'); +}); + +test('offers Retry for a retryable attempt and sends its attempt ID', async ({ page }) => { + await serve(); + const posts = runnerPosts(page); + const attemptId = '0b6e2f0a-3c4d-4e5f-8a9b-0c1d2e3f4a5b'; + await viewWith(page, view => { view.retryable = true; view.task.currentAttemptId = attemptId; }); + await openReview(page, app.url); + await page.getByRole('button', { name: 'Retry', exact: true }).click(); + await expect.poll(() => posts.length).toBe(1); + expect(posts[0]).toMatchObject({ action: 'retry', attemptId }); + await expect(page.locator('#runner-note')).toContainText('✕'); +}); + +test('shows the needs-amendment banner, approves the continuation, and opens the Plans screen', async ({ page }) => { + await serve(); + const posts = runnerPosts(page); + await viewWith(page, view => { + view.startable = false; + view.task.status = 'needs amendment'; + view.continuation = { checkpointId: 'c1', item: 'P2', completedItems: ['P1'], outOfScopePaths: ['src/config.ts'], next: 'P2', approved: false }; + }); + await openReview(page, app.url); + await expect(status(page)).toHaveText('! Needs amendment'); + await expect(page.locator('#task-banner-text')).toHaveText('P2 changed files the plan did not declare: src/config.ts. Revise the plan to declare them, then approve the continuation.'); + await page.getByRole('button', { name: 'Approve continuation', exact: true }).click(); + await expect.poll(() => posts.length).toBe(1); + expect(posts[0]).toMatchObject({ action: 'approve-continuation', expectedReviewVersion: (await app.service.load()).expected.reviewVersion }); + // The real task has no checkpoint, so the server refuses. + await expect(page.locator('#runner-note')).toContainText('✕'); + await page.getByRole('button', { name: 'Review plan change', exact: true }).click(); + await expect(page.locator('#plans-view')).toBeVisible(); +}); + +test('a continuation the revised plan cannot satisfy shows its reason and no Approve', async ({ page }) => { + await serve(); + await viewWith(page, view => { + view.startable = false; + view.task.status = 'needs amendment'; + view.continuation = { checkpointId: 'c1', item: 'P2', completedItems: ['P1'], outOfScopePaths: ['src/config.ts'], next: null, approved: false, + reason: 'The current plan does not declare src/config.ts for P2.' }; + }); + await openReview(page, app.url); + await expect(page.locator('#task-banner-text')).toHaveText('P2 changed files the plan did not declare: src/config.ts. The current plan does not declare src/config.ts for P2.'); + await expect(page.locator('#task-banner-approve')).toBeHidden(); + await expect(page.locator('#task-banner-plans')).toBeVisible(); +}); + +test('shows the needs-human and possibly-already-fixed banners with Cancel task and no run action', async ({ page }) => { + await serve(); + let state: 'needs human' | 'possibly already fixed' = 'needs human'; + await viewWith(page, view => { + view.startable = false; + view.task.status = state; + view.publish = { ...view.publish, last: { outcome: state === 'needs human' ? 'opened' : 'possibly already fixed', + message: state === 'needs human' ? 'Opened draft pull request #7.' : 'The issue may already be fixed (#401 links to it), so no pull request was opened.' } }; + }); + await openReview(page, app.url); + await expect(status(page)).toHaveText('! Needs human'); + await expect(page.locator('#task-banner-text')).toHaveText('This task needs a person. Opened draft pull request #7.'); + await expect(page.locator('#run-action')).toBeHidden(); + await expect(page.locator('#cancel-task')).toBeVisible(); + state = 'possibly already fixed'; + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(status(page)).toHaveText('! Possibly already fixed'); + await expect(page.locator('#task-banner-text')).toHaveText('The issue may already be fixed (#401 links to it), so no pull request was opened.'); + await expect(page.locator('#cancel-task')).toBeVisible(); +}); + +test('a Refresh while an action is in flight does not replace its answer with a runner read from before it', async ({ page }) => { + let fail!: () => void; + await serve({ prepare: () => new Promise((_, reject) => { fail = () => reject(new Error('no agent here')); }) }); + let release!: () => void, posted!: () => void; + const held = new Promise(resolve => { release = resolve; }), seen = new Promise(resolve => { posted = resolve; }); + const reads: number[] = []; + await page.route('**/api/runner', async route => { + if (route.request().method() !== 'POST') { reads.push(Date.now()); await route.continue(); return; } + posted(); await held; await route.continue(); + }); + await openReview(page, app.url); + await page.getByRole('button', { name: 'Start', exact: true }).click(); + await seen; + // The start has not reached the server yet. A Refresh now reloads the review; any runner read it makes sees no start. + const before = reads.length; + await refreshReview(page); + release(); + await expect(page.locator('#runner-note')).toHaveText('✓ Running P1.'); + // The action's answer shows P1 running: Stop P1 is offered and Start is not. + await expect(page.getByRole('button', { name: 'Stop P1', exact: true })).toBeVisible(); + await expect(page.locator('#run-action')).toBeHidden(); + expect(reads.length).toBeGreaterThanOrEqual(before); + fail(); +}); + +test('the review strip fits its runner controls at the 1280px minimum width', async ({ page }) => { + await serve(); + await page.setViewportSize({ width: 1280, height: 800 }); + // Every control the strip can show at once while an item runs: status, Stop, Retry, a run action and Cancel task. + await viewWith(page, view => { view.retryable = true; view.publish = { ...view.publish, available: true, publishable: true, draft: true }; }); + await openReview(page, app.url); + for (const id of ['task-status', 'attempt-retry', 'run-action', 'cancel-task']) await expect(page.locator(`#${id}`)).toBeVisible(); + // This server has no merge gateway, so the merge controls a production task also shows are unhidden here with their + // production labels (renderMergeControls), for this layout check only. + await page.evaluate(() => { for (const [id, text] of [['merge', '3 blockers'], ['merge-details', 'Review blockers']]) { const button = document.getElementById(id)!; button.hidden = false; button.textContent = text; } }); + const strip = await page.locator('#review-view .review-strip').evaluate(element => ({ scroll: element.scrollWidth, client: element.clientWidth })); + expect(strip.scroll).toBeLessThanOrEqual(strip.client); +}); diff --git a/web/public/app.js b/web/public/app.js index da0deda4..7c833b08 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -122,6 +122,7 @@ function showFailure(message, planStatusOwner) { $("merge-details").hidden = true; $("merge-steps-bar").hidden = true; if (mergeStepTimer) { clearInterval(mergeStepTimer); mergeStepTimer = null; } + renderRunner(); $("message").disabled = true; $("save-note").disabled = true; $("previous").disabled = true; @@ -163,6 +164,7 @@ async function refresh() { render(); showNewerMergeObservations(newer); setPlanStatus(planStatusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); + loadRunner(); } catch (error) { if (generation !== reviewGeneration) return; rememberDraft(); @@ -633,6 +635,7 @@ async function act(command) { data = withNewerMergeObservations(updated, newer); render(); showNewerMergeObservations(newer); + loadRunner(); return true; } catch (error) { if (generation !== reviewGeneration) return false; @@ -676,6 +679,7 @@ function render() { $("progress").textContent = `${data.approved} of ${data.items.length} approved`; renderMerge(); + renderRunner(); $("banner").textContent = data.demo ? "Demo repository · real Git changes and local SQLite storage. No tests or AI review have been run for this demo." : ""; @@ -2222,6 +2226,7 @@ $("plans-refresh").onclick = async () => { render(); showNewerMergeObservations(newer); setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); + loadRunner(); } catch (error) { if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration) return; setPlanStatus(statusOwner, "bad", `✕ Could not refresh the current plan. ${error.message}`); @@ -2247,6 +2252,271 @@ $("issues-list").onclick = (event) => { const button = event.target.closest("button.issue-trust"); if (button) changeIssueTrust(button); }; +// Runner controls (#200). The server decides every action: `GET /api/runner` says which ones it would admit now +// (`startable`, `resumable`, `retryable`, `publish`, `continuation`), and the page offers only those. +let runnerView = null, + // Every load, poll and action answer takes a generation; only the latest is applied (AGENTS.md). + runnerGeneration = 0, + runnerPollTimer = null, + runnerPollDelay = 2000, + // The action in flight, by kind; repeat activation is ignored while it is set. + runnerPending = null, + // Kept until a definite answer, so a resend after a 503, a lost response or an unknown outcome replays the same + // action instead of running it twice. Reused only for the identical request: the server hashes it under its action ID. + runnerKey = null, + // Whether the last applied view had work in progress, so the end of a run can be named once. + runnerWasActive = false, + cancelConfirmTimer = null; +const runnerPollMaximumDelay = 30000; +const runnerStatusMarks = { + queued: ["○", "Queued", "neutral"], + running: ["○", "Running", "neutral"], + "needs human": ["!", "Needs human", "warn"], + "needs amendment": ["!", "Needs amendment", "warn"], + "needs approval": ["!", "Needs approval", "warn"], + "possibly already fixed": ["!", "Possibly already fixed", "warn"], + "in review": ["○", "In review", "neutral"], + "approved but merge blocked": ["✕", "Merge blocked", "bad"], + merged: ["✓", "Merged", "good"], + cancelled: ["–", "Cancelled", "neutral"], +}; +const runnerControlIds = ["attempt-stop", "attempt-retry", "run-action", "cancel-task", "task-banner-plans", "task-banner-approve"]; +/** The attempt the task is on while it has not settled; the only one Stop can name. */ +function liveAttempt(view) { + const attempt = view.attempts.find((entry) => entry.id === view.task.currentAttemptId); + return attempt && (attempt.state === "pending" || attempt.state === "running") ? attempt : null; +} +/** Work that is still moving on its own, so the page polls until it settles. */ +function runnerBusy(view) { + return !!view?.available && (!!liveAttempt(view) || view.publish.active || view.preMerge.active + || view.stopRequested !== null || view.unresolved !== null || view.task.cancelRequested !== null); +} +/** The header's one run action, in the order a person would take them; null when the server would admit none. */ +function runAction(view) { + if (view.resumable) return { action: "resume", label: "Resume" }; + if (view.startable) return { action: "start", label: "Start" }; + if (view.publish.publishable) return { action: "publish", label: view.publish.draft ? "Publish draft PR" : "Publish PR" }; + if (view.publish.closable) return { action: "close-pull-requests", label: "Close pull requests" }; + return null; +} +/** The continuation a person can approve now: a checkpoint whose revised plan validates and is not yet approved. */ +function approvableContinuation(view) { + const continuation = view.continuation; + return continuation && !continuation.approved && !continuation.reason + && ["needs amendment", "queued", "running"].includes(view.task.status) ? continuation : null; +} +function taskBannerText(view) { + const task = view.task, attempt = view.attempts.find((entry) => entry.id === task.currentAttemptId); + if (task.status === "needs amendment") { + const continuation = view.continuation; + if (!continuation) return "This task needs a plan change before it can continue."; + const paths = continuation.outOfScopePaths.join(", "); + const head = `${continuation.item} changed ${paths ? `files the plan did not declare: ${paths}` : "files the plan did not declare"}.`; + if (continuation.reason) return `${head} ${continuation.reason}`; + return continuation.approved ? `${head} The continuation is approved; resume the task.` : `${head} Revise the plan to declare them, then approve the continuation.`; + } + if (task.status === "needs human") { + const detail = attempt?.safetyFinding ?? (attempt?.state === "failed" ? attempt.diagnostic : null) ?? view.publish.last?.message; + return `This task needs a person.${detail ? ` ${String(detail).slice(0, 300)}` : ""}`; + } + if (task.status === "possibly already fixed") + return view.publish.last?.message ?? "The issue may already be fixed, so no pull request was opened."; + if (task.status === "needs approval") return "This task needs your approval before it continues."; + return ""; +} +function setRunnerNote(text) { + $("runner-note").textContent = text; + $("runner-note").className = text ? statusClass(text) : ""; +} +function disarmCancel() { + if (cancelConfirmTimer) clearTimeout(cancelConfirmTimer); + cancelConfirmTimer = null; + $("cancel-task").textContent = "Cancel task"; + $("cancel-task").removeAttribute("data-confirm"); +} +/** + * Shows the runner controls from the last applied view. Each control's visibility is decided once, and focus on a + * control this hides moves to Refresh after every control has its final state (AGENTS.md). + */ +function renderRunner() { + const focused = document.activeElement; + const view = runnerView?.available && data ? runnerView : null; + const shown = {}; + for (const id of runnerControlIds) shown[id] = false; + if (view) { + const task = view.task, closed = task.status === "merged" || task.status === "cancelled"; + const [mark, word, tone] = runnerStatusMarks[task.status] ?? ["○", task.status, "neutral"]; + const live = liveAttempt(view); + const suffix = task.cancelRequested !== null ? " · Cancelling…" + : view.stopRequested !== null ? " · Stopping…" + : view.publish.active ? " · Publishing…" + : live ? ` · ${live.item ?? live.kind}` : ""; + $("task-status").textContent = `${mark} ${word}${suffix}`; + $("task-status").className = tone; + $("task-status").setAttribute("aria-label", `Task: ${word}${suffix.replace(" · ", ", ")}`); + shown["attempt-stop"] = !!live && view.stopRequested === null && task.cancelRequested === null; + if (live) $("attempt-stop").textContent = live.item ? `Stop ${live.item}` : "Stop attempt"; + shown["attempt-retry"] = view.retryable; + const run = runAction(view); + shown["run-action"] = !!run; + if (run && runnerPending?.action !== run.action) $("run-action").textContent = run.label; + shown["cancel-task"] = !closed && task.cancelRequested === null; + if (!shown["cancel-task"]) disarmCancel(); + const text = taskBannerText(view); + $("task-banner-text").textContent = text; + $("task-banner").hidden = !text; + shown["task-banner-plans"] = task.status === "needs amendment"; + shown["task-banner-approve"] = !!approvableContinuation(view); + } else { + $("task-banner").hidden = true; + disarmCancel(); + } + $("task-status").hidden = !view; + for (const id of runnerControlIds) { + $(id).hidden = !shown[id]; + // Marked, never disabled, while an action is in flight, so the focused control keeps focus (AGENTS.md). + if (runnerPending) $(id).setAttribute("aria-disabled", "true"); + else $(id).removeAttribute("aria-disabled"); + } + if (runnerControlIds.includes(focused?.id) && focused.hidden) $("reload").focus(); +} +function scheduleRunnerPoll() { + if (runnerPollTimer) clearTimeout(runnerPollTimer); + runnerPollTimer = null; + if (!runnerBusy(runnerView) || runnerPending) return; + const generation = runnerGeneration; + runnerPollTimer = setTimeout(() => loadRunner({ generation, poll: true }), runnerPollDelay); +} +/** Applies a view the server sent: from a load, a poll or an action's answer. */ +function applyRunnerView(view) { + const wasActive = runnerWasActive; + runnerView = view; + runnerWasActive = runnerBusy(view); + // A run that ended changed the task's code; the review shows it only after a reload, which would erase the person's + // selection and input if the page did it on its own. + if (wasActive && !runnerWasActive && view.available) { + const last = view.attempts.find((attempt) => attempt.id === view.task.currentAttemptId); + const ended = last ? `${last.item ?? last.kind} ${last.state}. ` : ""; + setRunnerNote(`${last?.state === "failed" ? "✕ " : ""}${ended}The task is now ${view.task.status}. Refresh to review its latest changes.`); + } + renderRunner(); +} +async function loadRunner({ poll = false, generation = poll ? runnerGeneration : undefined } = {}) { + // An action in flight answers with the view after it; a read now could be answered before it commits and, being + // newer, would drop that answer and leave the page on the state before the action. + if (runnerPending || (poll && generation !== runnerGeneration)) return; + generation ??= ++runnerGeneration; + runnerPollTimer = null; + try { + const view = await api("/api/runner"); + if (generation !== runnerGeneration) return; + const before = JSON.stringify(runnerView); + applyRunnerView(view); + // Unchanged work in progress backs off; any change polls quickly again. + runnerPollDelay = poll && before === JSON.stringify(view) ? Math.min(runnerPollDelay * 2, runnerPollMaximumDelay) : 2000; + } catch (error) { + if (generation !== runnerGeneration) return; + setRunnerNote(`✕ Could not read the runner state. ${error.message}`); + runnerPollDelay = Math.min(runnerPollDelay * 2, runnerPollMaximumDelay); + } + scheduleRunnerPoll(); +} +function runnerOutcomeText(action, result) { + switch (action) { + case "start": + case "resume": + return result.outcome === "started" ? `✓ Running ${result.item}.` : "✓ The task's owed work has settled; nothing new started."; + case "approve-continuation": + return `✓ Continuation approved.${result.next ? ` Resume the task to run ${result.next}.` : ""}`; + case "cancel-task": + return result.outcome === "stopping" ? "Cancelling: the running work is stopping." : "✓ Task cancelled."; + case "publish": + return result.draft ? "Publishing a draft pull request…" : "Publishing the pull request…"; + case "close-pull-requests": + return "Closing the task's pull requests…"; + case "cancel-attempt": + return "Stopping the item…"; + case "retry": + return "✓ Retrying the attempt."; + default: + return ""; + } +} +/** + * Sends one runner action with the versions the person saw: the task's from the runner view, the review's from the + * review on screen. A definite answer (applied, refused, or an upstream failure the server saved) ends its key. + */ +async function sendRunnerAction(action, attemptId = null) { + if (runnerPending || !runnerView?.available || !data) return; + const request = { action, expectedStateVersion: runnerView.stateVersion, + ...(["start", "resume", "approve-continuation"].includes(action) ? { expectedReviewVersion: data.expected.reviewVersion } : {}), + ...(attemptId ? { attemptId } : {}) }; + const same = runnerKey && JSON.stringify(runnerKey.request) === JSON.stringify(request); + if (!same) runnerKey = { request, actionId: crypto.randomUUID() }; + const key = runnerKey; + runnerPending = { action }; + const generation = ++runnerGeneration; + if (runnerPollTimer) clearTimeout(runnerPollTimer); + runnerPollTimer = null; + disarmCancel(); + setRunnerNote(""); + renderRunner(); + try { + const answer = await api("/api/runner", { ...key.request, actionId: key.actionId }); + if (runnerKey === key) runnerKey = null; + runnerPending = null; + runnerPollDelay = 2000; + if (generation === runnerGeneration) applyRunnerView(answer.runner); + setRunnerNote(runnerOutcomeText(action, answer.result)); + } catch (error) { + // No status (the response was lost), a 503 or an unknown outcome decided nothing the page can see: keep the key. + const definite = error.status !== undefined && error.status !== 503 && !error.outcomeUnknown; + if (definite && runnerKey === key) runnerKey = null; + runnerPending = null; + setRunnerNote(`✕ ${error.message}${definite ? "" : " Try again to resend the same request."}`); + // A refusal often means the view was stale; read the current one. + if (generation === runnerGeneration) { + renderRunner(); + await loadRunner(); + return; + } + } + renderRunner(); + scheduleRunnerPoll(); +} +$("run-action").onclick = () => { + if (runnerPending || !runnerView) return; + const run = runAction(runnerView); + if (run) sendRunnerAction(run.action); +}; +$("attempt-stop").onclick = () => { + if (runnerPending || !runnerView) return; + const live = liveAttempt(runnerView); + if (live) sendRunnerAction("cancel-attempt", live.id); +}; +$("attempt-retry").onclick = () => { + if (runnerPending || !runnerView?.retryable) return; + sendRunnerAction("retry", runnerView.task.currentAttemptId); +}; +$("task-banner-approve").onclick = () => { + if (runnerPending || !runnerView || !approvableContinuation(runnerView)) return; + sendRunnerAction("approve-continuation"); +}; +$("task-banner-plans").onclick = () => showView("plans"); +// Cancel closes the task and its pull requests and cannot be undone: the first press asks, a second within 5 s sends. +$("cancel-task").onclick = () => { + if (runnerPending || !runnerView) return; + if ($("cancel-task").dataset.confirm !== "true") { + $("cancel-task").dataset.confirm = "true"; + $("cancel-task").textContent = "Confirm cancel"; + const ask = "! Cancelling stops any running work and closes this task's pull requests. It cannot be undone. Press Confirm cancel to go on."; + setRunnerNote(ask); + // Clears only its own question: a later note (a poll's run end, an answer) stays. + cancelConfirmTimer = setTimeout(() => { disarmCancel(); if ($("runner-note").textContent === ask) setRunnerNote(""); }, 5000); + return; + } + sendRunnerAction("cancel-task"); +}; const initialView = new URLSearchParams(location.search).get("view"); showView(["issues", "plans"].includes(initialView) ? initialView : "review"); diff --git a/web/public/index.html b/web/public/index.html index a0530e0b..b09f1507 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -111,6 +111,11 @@

Loading plan…

Review Loading…
+ + @@ -121,6 +126,12 @@

Loading plan…

+ +