From dccdefea7899f48fe53cda7ba7b32d2e9bcea8f8 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 10 Nov 2026 18:47:51 -0500 Subject: [PATCH 1/2] Console: start Fix runs from Ready for review rows (#1788) --- console/tests/inbox.test.cjs | 2 + console/tests/start-fixture.cjs | 26 ++++- console/tests/start.test.cjs | 194 +++++++++++++++++++++++++++++++- docs/guide.md | 12 +- site/architecture/index.html | 2 +- site/docs/index.html | 12 +- site/how-it-works/index.html | 2 +- site/index.html | 2 +- src/console.rs | 7 ++ src/console/assets/console.js | 3 +- src/console/assets/start.js | 58 +++++++--- src/console/inbox.html | 2 +- src/console/row.html | 2 +- src/console/start_controls.rs | 27 +++++ src/logs/status/inbox.rs | 12 +- tests/console/start.rs | 83 ++++++++++++++ 16 files changed, 409 insertions(+), 37 deletions(-) diff --git a/console/tests/inbox.test.cjs b/console/tests/inbox.test.cjs index a47197eab..6ca2108fc 100644 --- a/console/tests/inbox.test.cjs +++ b/console/tests/inbox.test.cjs @@ -197,6 +197,8 @@ test('Copy fix command works on ready Inbox rows and in Command details', async await row.waitFor(); const copy = row.getByRole('button', { name: 'Copy fix command', exact: true }); assert.equal(await copy.count(), offered ? 1 : 0, `${width}:${outcome}:${ci}:${mergeable}`); + // This Console knows no Launch directory: retain the copy fallback alone. + assert.equal(await row.getByRole('button', { name: 'Start Fix run', exact: true }).count(), 0); if (offered) { assert.equal(await copy.getAttribute('type'), 'button'); assert.equal(await copy.getAttribute('data-copy-cli'), cli); diff --git a/console/tests/start-fixture.cjs b/console/tests/start-fixture.cjs index 56053b9c9..bbc1c3f6c 100644 --- a/console/tests/start-fixture.cjs +++ b/console/tests/start-fixture.cjs @@ -1,10 +1,11 @@ const { spawn, execFileSync } = require('node:child_process'); const { once } = require('node:events'); -const { mkdtemp, mkdir, writeFile, rm } = require('node:fs/promises'); +const { mkdtemp, mkdir, readFile, writeFile, rm } = require('node:fs/promises'); const { tmpdir } = require('node:os'); const { join, resolve } = require('node:path'); const { createInterface } = require('node:readline'); const { chromium } = require('./playwright.cjs'); +const { replaceRecord } = require('./records.cjs'); async function startFixture(extraEnv = {}) { const home = await mkdtemp(join(tmpdir(), 'thirdshift-start-page-')); @@ -65,6 +66,29 @@ else: finally { lines.close(); } browser = await chromium.launch({ headless: true }); return { home, repo, browser, origin, close, + async readyPullRequest(number = 42, changes = {}, github = {}) { + const logs = join(home, '.thirdshift/logs'); + const directory = join(logs, 'acme/widgets/commands/review'); + await mkdir(directory, { recursive: true }); + const pull_request = `https://github.com/acme/widgets/pull/${number}`; + const now = new Date().toISOString(); + const path = join(directory, `${number}-fixture.command.json`); + await replaceRecord(path, JSON.stringify({ + version: 1, kind: 'Review run', repository: 'acme/widgets', hostname: 'fixture', + issue: null, process_id: 0, process_start: 'gone', harness: 'claude', model: 'old-model', effort: 'high', + started_at: now, ended_at: now, stages: [], outcome: 'ready for review', pull_request, + ...changes + })); + const cache = join(logs, 'github-inbox.json'); + const kept = JSON.parse(await readFile(cache, 'utf8').catch(error => { + if (error.code !== 'ENOENT') throw error; + return '{"version":1,"subjects":{}}'; + })); + kept.subjects[pull_request] = { state: 'Open', ci: 'red', mergeable: 'mergeable', title: 'Repair these widgets', ...github }; + await replaceRecord(cache, JSON.stringify(kept)); + await writeFile(join(logs, 'github-inbox.changed'), `${number}:${JSON.stringify(github)}`); + return path; + }, cli(args, input) { return execFileSync(binary, args, { env, input, encoding: 'utf8' }); }, scopes(value) { execFileSync(binary, ['console', 'scopes', '--scopes', value], { env }); }, async page(options) { diff --git a/console/tests/start.test.cjs b/console/tests/start.test.cjs index 81796adc3..dcf3f89d3 100644 --- a/console/tests/start.test.cjs +++ b/console/tests/start.test.cjs @@ -1,13 +1,204 @@ const { test, before, after } = require('node:test'); const assert = require('node:assert/strict'); -const { mkdir, writeFile } = require('node:fs/promises'); +const { mkdir, readFile, writeFile, rm } = require('node:fs/promises'); const { join } = require('node:path'); const { startFixture } = require('./start-fixture.cjs'); +const { hostname } = require('node:os'); let fixture; before(async () => { fixture = await startFixture(); }); after(async () => { await fixture?.close(); }); +test('Start Fix run confirms the pull request and defaults, cancels, and submits once', async t => { + await fixture.readyPullRequest(); + for (const viewport of [{ width: 1440, height: 900 }, { width: 390, height: 844 }]) { + const page = await fixture.page({ viewport }); + t.after(() => page.context().close()); + const requests = []; + let respond; + let received; + const receiving = new Promise(resolve => { received = resolve; }); + await page.route('**/api/v1/runs', async route => { + requests.push({ body: route.request().postDataJSON(), csrf: route.request().headers()['x-csrf-token'] }); + received(); + await new Promise(resolve => { respond = resolve; }); + await route.fulfill({ json: { version: 1, name: 'acme/widgets fix #42' } }); + }); + const start = page.getByRole('button', { name: 'Start Fix run', exact: true }); + await start.click({ timeout: 5000 }); + const confirm = page.getByRole('dialog', { name: 'Confirm start', exact: true }); + await confirm.waitFor(); + const request = { command: 'fix', repository: 'acme/widgets', pull_request: 'https://github.com/acme/widgets/pull/42' }; + assert.deepEqual(JSON.parse(await confirm.locator('pre').textContent()), request); + assert.match(await confirm.textContent(), /https:\/\/github.com\/acme\/widgets\/pull\/42/); + assert.ok((await confirm.textContent()).includes(`machine ${hostname()}`)); + assert.match(await confirm.textContent(), /User config and the CLI defaults/); + assert.deepEqual(requests, []); + await confirm.getByRole('button', { name: 'Cancel', exact: true }).click(); + await confirm.waitFor({ state: 'hidden' }); + await start.click(); + await page.keyboard.press('Escape'); + await confirm.waitFor({ state: 'hidden' }); + assert.deepEqual(requests, []); + await start.click(); + await confirm.getByRole('button', { name: 'Start now', exact: true }).click(); + try { + await page.locator('[data-start-result]').filter({ hasText: 'Starting…' }).waitFor(); + await receiving; + assert.equal(await start.isDisabled(), true); + assert.equal(requests.length, 1); + assert.deepEqual(requests[0].body, request); + assert.ok(requests[0].csrf); + } finally { respond?.(); } + await page.locator('[data-start-result]').filter({ hasText: 'Started Command acme/widgets fix #42.' }).waitFor(); + await start.click(); + await confirm.waitFor(); + await page.keyboard.press('Escape'); + await confirm.waitFor({ state: 'hidden' }); + assert.equal(await page.evaluate(() => document.documentElement.scrollWidth <= innerWidth), true); + await page.context().close(); + } +}); + +test('Start Fix run requires an eligible row, current run permission and a known Launch directory', async t => { + const configPath = join(fixture.home, '.thirdshift/config.toml'); + const config = await readFile(configPath, 'utf8'); + const cronPath = join(fixture.home, 'crontab'); + const cron = await readFile(cronPath, 'utf8'); + t.after(async () => { + await writeFile(configPath, config); + await writeFile(cronPath, cron); + fixture.scopes('read,security,act,run,admin'); + await fixture.readyPullRequest(); + }); + const page = await fixture.page(); + t.after(() => page.context().close()); + for (const [outcome, ci, mergeable, eligible] of [ + ['ready for review', 'red', 'mergeable', true], + ['ready for review', 'green', 'conflicting', true], + ['ready for review', 'pending', 'conflicting', true], + ['ready for review', 'green', 'mergeable', false], + ['ready for review', 'unknown', 'unknown', false], + ['failed', 'red', 'conflicting', false], + ['merged', 'red', 'conflicting', false], + ]) { + await fixture.readyPullRequest(42, { outcome }, { ci, mergeable }); + await page.goto(`${fixture.origin}/?section=${outcome === 'failed' ? 'needs-you' : outcome === 'merged' ? 'done' : 'ready-for-review'}`); + assert.equal(await page.getByRole('button', { name: 'Start Fix run', exact: true }).count(), Number(eligible), `${outcome}:${ci}:${mergeable}`); + } + await fixture.readyPullRequest(); + for (const scopes of ['read', 'read,act', 'read,run']) { + fixture.scopes(scopes); + await page.goto(fixture.origin); + assert.equal(await page.getByRole('button', { name: 'Start Fix run', exact: true }).count(), Number(scopes.includes('run'))); + assert.equal(await page.getByRole('button', { name: 'Copy fix command', exact: true }).count(), 1); + } + await writeFile(configPath, config.replace(/^launch_directories = .*\n/, '')); + await writeFile(cronPath, ''); + await page.goto(fixture.origin); + assert.equal(await page.getByRole('button', { name: 'Start Fix run', exact: true }).count(), 0); + assert.equal(await page.getByRole('button', { name: 'Copy fix command', exact: true }).count(), 1); +}); + +test('Start Fix run works after live row and permission changes and preserves uncertain-response feedback', async t => { + await fixture.readyPullRequest(42, {}, { ci: 'green' }); + fixture.scopes('read'); + t.after(async () => { fixture.scopes('read,security,act,run,admin'); await fixture.readyPullRequest(); }); + const page = await fixture.page(); + t.after(() => page.context().close()); + const start = page.locator('.command[data-command$="42-fixture.command.json"]').getByRole('button', { name: 'Start Fix run', exact: true }); + assert.equal(await start.count(), 0); + fixture.scopes('read,run'); + await fixture.readyPullRequest(42, {}, { ci: 'green', mergeable: 'conflicting' }); + await start.waitFor(); + await start.click(); + const confirm = page.getByRole('dialog', { name: 'Confirm start', exact: true }); + await confirm.waitFor(); + await page.keyboard.press('Escape'); + await confirm.waitFor({ state: 'hidden' }); + let requests = 0, release, received; + const receiving = new Promise(resolve => { received = resolve; }); + await page.route('**/api/v1/runs', async route => { + requests++; + received(); + await new Promise(resolve => { release = resolve; }); + await route.abort('failed'); + }); + await start.click(); + await confirm.getByRole('button', { name: 'Start now', exact: true }).click(); + await receiving; + try { + await fixture.readyPullRequest(42, { cause: 'Updated while start is pending' }); + await page.getByText('Updated while start is pending', { exact: true }).waitFor({ state: 'attached' }); + assert.equal(await start.isDisabled(), true); + assert.equal(requests, 1); + } finally { release(); } + const result = page.locator('[data-start-result]'); + await result.filter({ hasText: 'Check Inbox and logs before retrying.' }).waitFor(); + await fixture.readyPullRequest(42, { cause: 'Updated after uncertain response' }); + await page.getByText('Updated after uncertain response', { exact: true }).waitFor({ state: 'attached' }); + assert.match(await result.textContent(), /Check Inbox and logs before retrying/); + assert.equal(requests, 1); + const inserted = await fixture.readyPullRequest(43); + t.after(() => rm(inserted, { force: true })); + const newStart = page.locator('.command[data-command$="43-fixture.command.json"]').getByRole('button', { name: 'Start Fix run', exact: true }); + await newStart.click(); + await confirm.waitFor(); + assert.equal(JSON.parse(await confirm.locator('pre').textContent()).pull_request, 'https://github.com/acme/widgets/pull/43'); + await page.keyboard.press('Escape'); + await confirm.waitFor({ state: 'hidden' }); + fixture.scopes('read'); + await start.waitFor({ state: 'detached' }); +}); + +test('Start Fix run forwards only to the owning machine with its current run permission', async t => { + const owner = await startFixture(); + t.after(() => owner.close()); + const caller = await startFixture({ THIRDSHIFT_FLEET_POLL_MS: '100' }); + t.after(() => caller.close()); + await owner.readyPullRequest(); + const token = owner.cli(['console', 'token', 'create', 'fix-caller', '--scopes', 'read,run']).trim(); + caller.cli(['console', 'fleet', 'add', 'owner', owner.origin], `${token}\n`); + caller.scopes('read'); + for (const viewport of [{ width: 1440, height: 900 }, { width: 390, height: 844 }]) { + const page = await caller.page({ viewport }); + t.after(() => page.context().close()); + const row = page.locator('.command[data-machine="owner"]'); + const start = row.getByRole('button', { name: 'Start Fix run', exact: true }); + await start.click(); + const confirm = page.getByRole('dialog', { name: 'Confirm start', exact: true }); + await confirm.waitFor(); + assert.match(await confirm.textContent(), /machine owner/); + const requests = []; + page.on('request', request => { + if (request.method() === 'POST' && /\/runs(?:\?|$)/.test(request.url())) { + requests.push({ path: new URL(request.url()).pathname + new URL(request.url()).search, body: request.postDataJSON() }); + } + }); + const forwarding = page.waitForResponse(response => response.url().includes('/fleet/runs?machine=owner')); + await confirm.getByRole('button', { name: 'Start now', exact: true }).click(); + // The fake GitHub refuses PR admission. This still exercises real forwarding, + // destination authorization, CSRF and attribution, without starting a Harness. + const response = await forwarding; + const failure = (await response.json()).error; + assert.equal(response.status(), 422, failure); + await page.locator('[data-start-result]').filter({ hasText: failure }).waitFor(); + assert.deepEqual(requests, [{ path: '/fleet/runs?machine=owner', body: { + command: 'fix', repository: 'acme/widgets', pull_request: 'https://github.com/acme/widgets/pull/42' + } }]); + assert.match(owner.cli(['console', 'log', '--json']), /owner@example.test from the Console on fix-caller/); + assert.equal(await page.evaluate(() => document.documentElement.scrollWidth <= innerWidth), true); + await page.context().close(); + } + const page = await caller.page(); + t.after(() => page.context().close()); + const row = page.locator('.command[data-machine="owner"]'); + await row.getByRole('button', { name: 'Start Fix run', exact: true }).waitFor(); + owner.scopes('read'); + await row.getByRole('button', { name: 'Start Fix run', exact: true }).waitFor({ state: 'detached' }); + await row.getByRole('button', { name: 'Copy fix command', exact: true }).waitFor(); +}); + test('Start requires confirmation, names its Harness and shows the Command in Inbox', async t => { for (const viewport of [{ width: 1440, height: 900 }, { width: 390, height: 844 }]) { await t.test(`${viewport.width}px`, async t => { @@ -110,6 +301,7 @@ test('Start reviews every shared option before submission and remains inert with t.after(() => noScript.context().close()); await noScript.getByText('Start work', { exact: true }).click(); assert.equal(await noScript.getByRole('button', { name: 'Review start', exact: true }).isDisabled(), true); + assert.equal(await noScript.getByRole('button', { name: 'Start Fix run', exact: true }).isDisabled(), true); await noScript.goto(`${fixture.origin}/cronjobs`); assert.equal(await noScript.getByRole('button', { name: 'Run now', exact: true }).isDisabled(), true); }); diff --git a/docs/guide.md b/docs/guide.md index 17103ed1e..e3824b73d 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -146,7 +146,7 @@ Ready for review rows show the pull request's latest head CI as **CI green**, ** A conflicting pull request also shows a separate **Merge conflict** pill beside CI, even when CI is green. It stays in Ready for review. The same GitHub read refreshes mergeability and wakes open pages when it changes; `status` prints **Merge conflict** too. `status --json` includes `github_mergeable` (`mergeable`, `conflicting` or `unknown`). Only `conflicting` shows the pill; unknown and older saved answers do not claim a conflict. Failed reads retain the last answer with the same freshness warning. -Ready for review rows with a pull request and either **CI red** or **Merge conflict** show `thirdshift fix ` and a **Copy fix command** button, both on the Inbox row and in Command details (Peek). Clicking it copies the command; run it from a clone of that repository to start a [Fix run](#fix-runs). A later Command on the same pull request URL takes up the earlier waiting row in Ready for review or Needs you, just as one on the same issue does, including when neither Command has a linked issue. The row says `taken up by a later Command started at