From 7a4253b0e409eb118b0691369833b44306a1c9f6 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 7 Oct 2026 09:00:39 -0400 Subject: [PATCH 1/7] Ask on remote reviews --- .../reviewRemoteInstaller.containers.test.ts | 49 ++- .../scripts/e2e/journeys/remote-ask.mjs | 406 ++++++++++++++++++ .../scripts/e2e/journeys/remote-host.mjs | 20 +- .../scripts/e2e/journeys/remote-install.mjs | 2 +- .../scripts/e2e/remote/aws-hosts.mjs | 5 +- .../scripts/e2e/remote/docker-hosts.mjs | 7 +- .../e2e/remote/fake-agent/acp-agent.mjs | 83 ++++ .../scripts/e2e/remote/fake-agent/opencode | 10 + .../scripts/e2e/remote/remote.mjs | 5 +- .../review-desktop/scripts/e2e/remote/ssh.mjs | 18 + docs/remote-hosts.md | 8 + packages/review/app/src/ask-files.test.tsx | 60 +++ packages/review/app/src/ask-files.tsx | 7 +- packages/review/app/src/ask-panel.test.tsx | 52 +++ packages/review/app/src/ask-setup.tsx | 15 +- packages/review/src/ask/agents.ts | 8 +- packages/review/src/ask/threads.ts | 48 ++- .../review/src/review-api/local-data.test.ts | 121 ++++++ packages/review/src/server/desktop-server.ts | 45 +- .../review/src/server/headless-host.test.ts | 237 +++++++++- packages/review/src/server/headless-host.ts | 82 +++- .../src/server/headless-sharing.test.ts | 1 + .../review/src/server/review-gateway.test.ts | 185 ++++++++ packages/review/src/server/review-gateway.ts | 19 +- .../src/server/review-server-core.test.ts | 1 + .../review/src/server/review-server-core.ts | 29 +- 26 files changed, 1455 insertions(+), 68 deletions(-) create mode 100644 apps/review-desktop/scripts/e2e/journeys/remote-ask.mjs create mode 100644 apps/review-desktop/scripts/e2e/remote/fake-agent/acp-agent.mjs create mode 100755 apps/review-desktop/scripts/e2e/remote/fake-agent/opencode diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts index a41912dbb..1e0b0b2ca 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts @@ -5,7 +5,7 @@ import assert from "node:assert/strict"; import { execFile, execFileSync, spawn } from "node:child_process"; -import { randomBytes } from "node:crypto"; +import { randomBytes, randomUUID } from "node:crypto"; import { mkdtemp, readFile, rm } from "node:fs/promises"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; @@ -137,6 +137,53 @@ test("a host with no Node downloads the tarball, with diffr inside it; whiteboar assert.equal(await inContainer("fresh", "ls -d ~/.dev/whiteboard-remote/install.lock 2>/dev/null | wc -l"), "0"); }); +const FETCH = `const [url, token, method, path, body] = process.argv.slice(1); +const response = await fetch(url + path, { method, headers: { "x-review-token": token, "content-type": "application/json" }, body: method === "GET" ? undefined : body }); +const bytes = Buffer.from(await response.arrayBuffer()); +console.log(JSON.stringify({ status: response.status, type: response.headers.get("content-type"), size: bytes.length, text: bytes.toString() }));`; +const PIXEL = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=="; + +test("an install has the adapters but not the bundled agent binaries; instructions, images and structural diff answer", { skip, timeout: 10 * 60_000 }, async (t) => { + await installRemote(input("fresh").value); + const modules = `${installed}/node_modules`; + + assert.equal(await inContainer("fresh", `test -f ${modules}/@agentclientprotocol/codex-acp/dist/index.js && echo yes`), "yes"); + assert.equal(await inContainer("fresh", `test -f ${modules}/@agentclientprotocol/claude-agent-acp/dist/index.js && echo yes`), "yes"); + assert.equal(await inContainer("fresh", `ls -d ${modules}/@openai/codex-* ${modules}/@anthropic-ai/claude-agent-sdk-* 2>/dev/null | wc -l`), "0"); + assert.equal(await inContainer("fresh", `test -x ${modules}/@dev.fast/diffr-linux-*/diffr && echo yes`), "yes"); + assert.equal(await version("fresh"), VERSION); + + await inContainer( + "fresh", + `git init -q -b main ~/repo && cd ~/repo && printf 'export const a = 1;\\n' > a.ts && git add . && git -c user.name=t -c user.email=t@t commit -qm one && printf 'export const a = 2;\\n' > a.ts && git -c user.name=t -c user.email=t@t commit -qam two`, + ); + t.after(() => inContainer("fresh", "~/.local/bin/whiteboard server stop; rm -rf ~/repo")); + const attach = JSON.parse((await inContainer("fresh", "~/.local/bin/whiteboard remote attach --json")).split("\n").find((line) => line.startsWith("{"))!); + const api = async (method: string, path: string, body?: unknown) => + JSON.parse((await run("docker", ["exec", "-u", "dev", `wb-test-${runId}-fresh`, `${installed}/bin/node`, "--input-type=module", "-e", FETCH, attach.url, attach.token, method, `/reviews-api${path}`, JSON.stringify(body ?? null)])).stdout); + const instructions = await api("GET", "/instructions?topic=authoring"); + assert.equal(instructions.status, 200, instructions.text); + assert.ok(instructions.size > 0); + const [base, head] = (await inContainer("fresh", "cd ~/repo && git rev-parse HEAD~1 HEAD")).split("\n"); + const repositoryId = JSON.parse((await api("POST", "/repositories", { path: "/home/dev/repo" })).text).id; + const { reviewId } = JSON.parse( + (await api("POST", "/commands", { operation: { type: "create", title: "Install check", target: { kind: "commits", repositoryId, base, head } } })).text, + ); + + const id = randomUUID(); + assert.equal((await api("POST", "/resources", { id, repositoryId, kind: "image", base64: PIXEL })).status, 200); + const image = await api("GET", `/${reviewId}/resources/${id}`); + assert.deepEqual([image.status, image.type], [200, "image/png"]); + assert.ok(image.size > 0); + + const diff = await api("GET", `/${reviewId}/structural-diff`); + assert.equal(diff.status, 200); + const events = diff.text.trim().split("\n").map((line: string) => JSON.parse(line)); + assert.ok(events.some((event: { type: string }) => event.type === "file")); + assert.equal(events.at(-1).type, "complete"); + assert.equal(events.at(-1).failed, 0); +}); + test("a tarball the host cannot download fails the install with the URL and leaves nothing", { skip, timeout: 10 * 60_000 }, async () => { await reset("fresh"); const { value } = input("fresh"); diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-ask.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-ask.mjs new file mode 100644 index 000000000..5e389d20d --- /dev/null +++ b/apps/review-desktop/scripts/e2e/journeys/remote-ask.mjs @@ -0,0 +1,406 @@ +/** Ask on a review whose checkout is on another machine: a container with a fake OpenCode, reached over SSH and added in Settings as a user would. */ +import assert from "node:assert/strict"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { openHome, openSettings } from "../harness.mjs"; +import { + alias, + closeDesktop, + createRemoteReview, + masterPid, + onRemote, + prepared, + recordRequests, + remote, + remoteToken, + runDir, +} from "./remote-host.mjs"; +import { removeHost } from "./remote-install.mjs"; + +const exec = promisify(execFile); + +const title = "Remote ask"; + +const question = "what does f.ts do"; + +export const name = "remote-ask"; + +// Phase 2: the container image downloads Ubuntu packages and Node. +export const phase = 2; + +export const options = { + settings: { "review.experimental.remoteHosts.enabled": true }, + env: { DEV_FAST_REVIEW_SSH_CONFIG: `${runDir}/ssh_config` }, +}; + +export async function run(ctx) { + // A packaged build ignores DEV_FAST_REVIEW_SSH_CONFIG, so its ssh would read the user's configuration. + if (ctx.report.mode === "packaged") + throw new Error("skip: remote-ask runs in development mode only"); + + if (prepared === undefined) + try { + await exec("docker", ["info", "--format", "{{.ServerVersion}}"]); + } catch (error) { + throw new Error( + `skip: remote-ask needs Docker for its SSH server (${error.message.split("\n")[0]})`, + ); + } + + try { + await journey(ctx); + } finally { + if (prepared === undefined) + await remote("down", "--all").catch((error) => + console.error(`[remote-ask] down --all: ${error.message}`), + ); + else await closeDesktop(ctx); + } +} + +/** Pauses the container, or stops a prepared host's server and returns its resume. */ +async function freeze() { + if (prepared === undefined) { + await remote("pause", "a"); + + return undefined; + } + + const { serverPid } = JSON.parse( + await onRemote("whiteboard server status --json"), + ); + + await onRemote(`kill -STOP ${serverPid}`); + + return () => onRemote(`kill -CONT ${serverPid}`); +} + +async function journey(ctx) { + const { until } = ctx; + const timings = {}; + + const hostState = async () => + (await ctx.apiOk("/remote-hosts")).find((host) => host.alias === alias) + ?.state; + + const waitState = (state, label, timeout = 60000) => + until( + async () => (await hostState()) === state, + `${alias} ${label}`, + timeout, + ); + + const canvas = () => + ctx.page.locator(".review-canvas-root [data-review-api]"); + + // Docked in the side panel or floating, by the layout. + const panel = () => + ctx.page.locator( + '[role=complementary][aria-label="Ask"], [role=dialog][aria-label="Ask"]', + ); + + const composer = () => panel().getByRole("combobox", { name: "Question" }); + + const homeRow = () => + ctx.page + .locator("main.review-home [data-home-row]") + .filter({ hasText: title }); + + async function openReview() { + await openHome(ctx); + await homeRow().waitFor({ timeout: 60000 }); + await homeRow().getByTitle(title, { exact: true }).click(); + await canvas().getByRole("heading", { name: title }).waitFor({ + timeout: 60000, + }); + } + + /** Selects the document's sentence and opens Ask on it with ⌘L. */ + async function askAboutSentence() { + // The workbench's usage-data notice sits over the composer's Ask button. + for (const clear of await ctx.page + .locator(".notifications-toasts .codicon-notifications-clear") + .all()) + await clear.click().catch(() => {}); + + const sentence = canvas().getByText("One now returns"); + + await sentence.waitFor(); + await sentence.evaluate((element) => { + const range = document.createRange(); + + range.selectNodeContents(element); + document.getSelection().removeAllRanges(); + document.getSelection().addRange(range); + }); + await ctx.page + .getByRole("button", { name: "Ask OpenCode", exact: true }) + .waitFor({ timeout: 30000 }); + await ctx.page.keyboard.press("Meta+l"); + await composer().waitFor({ timeout: 30000 }); + } + + /** Resolves with the ms until the panel shows the answer. */ + async function send(text) { + await composer().fill(text); + + const sent = Date.now(); + + await panel().getByRole("button", { name: "Ask", exact: true }).click(); + await panel().getByText(`You asked: ${text}`).waitFor({ timeout: 20000 }); + + return Date.now() - sent; + } + + const savedConversations = () => + canvas().getByRole("button", { name: "Saved conversations" }); + + async function historyLists(label) { + await until( + async () => { + await savedConversations().first().click(); + + return ( + (await panel() + .getByRole("list") + .filter({ hasText: question }) + .count()) > 0 + ); + }, + label, + 30000, + ); + } + + // 1. A container with sshd, this checkout's package and the fake OpenCode, or a prepared host, holding a review. + if (prepared === undefined) { + await remote("up", "a", "--fake-agent"); + await remote("install", "a"); + } + + const [reviewId] = ( + await onRemote(`bash -s -- '${title}'`, createRemoteReview) + ).split("\n"); + + assert.match(reviewId, /^[0-9a-f-]{36}$/, "the remote review's id"); + + const remoteTokens = new Set([await remoteToken()]); + + ctx.check(`1. ${alias}, with a fake OpenCode, holds a review`); + + // 2. Settings, as a user adds a host. + const section = (await openSettings(ctx)).getByRole("region", { + name: "Remote hosts", + }); + + await section.getByLabel("SSH alias").fill(alias); + + const added = Date.now(); + + await section.getByRole("button", { name: "Add", exact: true }).click(); + await waitState("online", "online"); + timings.addToOnline = Date.now() - added; + ctx.check( + `2. Settings added ${alias}; online ${timings.addToOnline} ms after Add`, + ); + + // 3. The remote review, a sentence selected, ⌘L. + const requests = await recordRequests(ctx.page, /\/ask\/watch(\?|$)/); + const recorders = [{ requests, origin: new URL(ctx.discovery.url).origin }]; + + await openReview(); + await askAboutSentence(); + ctx.check("3. ⌘L on a selected sentence of the remote review opened Ask"); + + // 4. The agents are the remote's: with OpenCode gone, the panel names the host; back, it answers again. + await onRemote("mv ~/.opencode ~/.opencode.off"); + await until( + async () => { + await ctx.page.evaluate(() => window.dispatchEvent(new Event("focus"))); + + return ( + (await panel().getByText(`Not installed on ${alias}`).count()) >= 3 + ); + }, + `the panel to read Not installed on ${alias}`, + 30000, + ); + + for (const agent of ["Claude Code", "Codex", "OpenCode"]) + assert.match( + await panel() + .getByRole("listitem") + .filter({ hasText: agent }) + .innerText(), + new RegExp(`Not installed on ${alias}`), + ); + + await onRemote("mv ~/.opencode.off ~/.opencode"); + await until( + async () => { + await ctx.page.evaluate(() => window.dispatchEvent(new Event("focus"))); + + return (await composer().count()) > 0; + }, + "the composer to come back with OpenCode", + 30000, + ); + await panel().getByText("OpenCode", { exact: true }).waitFor(); + + for (const name of ["OpenCode", "Choose an agent"]) + assert.equal( + await panel().getByRole("button", { name, exact: true }).count(), + 0, + ); + ctx.check( + `4. without the remote's OpenCode, Claude Code, Codex and OpenCode read Not installed on ${alias}; with it back, the header names OpenCode alone, without a menu`, + ); + + // 5. The question goes to the remote's agent; its answer streams back through the gateway. + timings.firstAnswer = await send(question); + ctx.check( + `5. OpenCode on ${alias} answered "You asked: …" ${timings.firstAnswer} ms after Ask`, + ); + + // 6. A review on another machine has no Source window: the answer's f.ts is text that says so, and a click asks nothing. + const reference = panel().getByTitle( + "Source windows are not available for a review on another machine.", + ); + + await reference.waitFor(); + assert.equal(await reference.innerText(), "f.ts"); + assert.equal( + await reference.evaluate( + (element) => + element.closest("a, button, [role=link], [role=button]") !== null, + ), + false, + "the f.ts reference is a link or a button", + ); + assert.equal(await panel().getByRole("link", { name: "f.ts" }).count(), 0); + + // Step 11 audits the whole run for the requests a click could send. + await reference.click(); + ctx.check( + "6. the answer's f.ts is text titled Source windows are not available…, not a link or button", + ); + + // 7. Ask spoke only to the local server, and no remote token reached the page. + const asks = audit(recorders, remoteTokens); + + assert.ok( + asks.some((r) => r.received?.toString().includes("You asked:")), + "no watch stream carried the answer", + ); + ctx.check( + `7. ${asks.length} Ask requests, all to the local server, none with the remote token; the answer came on its watch stream`, + ); + + // 8. The ssh master dies and comes back: the thread is still saved. + const master = await masterPid(); + + assert.ok(master, "no ssh master for the host"); + process.kill(master, "SIGKILL"); + await until( + async () => + (await hostState()) === "online" && (await masterPid()) !== master, + `${alias} online on a new master`, + 60000, + ); + await panel().getByRole("button", { name: "Close Ask" }).click(); + await historyLists("the thread in Ask history after the reattach"); + ctx.check("8. after a new ssh master, Ask history lists the thread"); + + // 9. A Desktop restart: the thread is still listed. + await ctx.quitAndRelaunchDesktop(); + recorders.push({ + requests: await recordRequests(ctx.page, /^$/), + origin: new URL(ctx.discovery.url).origin, + }); + await waitState("online", "online after the relaunch"); + remoteTokens.add(await remoteToken()); + await openReview(); + await historyLists("the thread in Ask history after the relaunch"); + await panel().getByRole("button", { name: "Close Ask" }).click(); + ctx.check("9. after a Desktop restart, Ask history lists the thread"); + + // 10. The host goes away with a conversation open: the panel says so, and the host reads offline. + await askAboutSentence(); + await send("and then"); + const thaw = await freeze(); + + const paused = Date.now(); + + await until( + async () => + (await panel() + .getByRole("alert") + .filter({ hasText: "lost its connection" }) + .count()) > 0, + "the panel's lost-connection alert", + 20000, + ); + timings.streamEnded = Date.now() - paused; + await waitState("offline", "offline after the pause", 20000); + timings.offline = Date.now() - paused; + assert.ok( + timings.streamEnded <= 15000, + `alert after ${timings.streamEnded} ms`, + ); + assert.ok(timings.offline <= 15000, `offline after ${timings.offline} ms`); + await thaw?.(); + ctx.check( + `10. ${prepared === undefined ? "docker pause" : "server SIGSTOP"}: the panel said it lost OpenCode after ${timings.streamEnded} ms; ${alias} offline after ${timings.offline} ms`, + ); + + // 11. Removing the host takes the review out of Home. + await removeHost(ctx, alias); + await openHome(ctx); + await until( + async () => (await homeRow().count()) === 0, + "the remote review to leave Home", + ); + const allAsks = audit(recorders, remoteTokens); + + assert.deepEqual( + recorders.flatMap(({ requests: recorded }) => + [...recorded.values()].flatMap((r) => + /\/(navigator|files)(\?|$)/.test(r.url ?? "") ? [r.url] : [], + ), + ), + [], + "navigator or files requests", + ); + ctx.check( + `11. removing the host took the review out of Home; across both launches ${allAsks.length} Ask requests, all to the local server, none with a remote token, and no navigator or files request`, + ); +} + +/** Asserts each launch's Ask requests stayed local and none carried a remote token. */ +function audit(recorders, remoteTokens) { + const asks = []; + + for (const { requests, origin } of recorders) + for (const r of requests.values()) { + if (!r.url) continue; + assert.ok( + ![r.url, ...Object.values(r.headers ?? {})].some((value) => + [...remoteTokens].some((token) => String(value).includes(token)), + ), + "a request carried a remote token", + ); + + if (!/\/ask(\/|\?|$)/.test(new URL(r.url).pathname)) continue; + assert.equal( + new URL(r.url).origin, + origin, + "an Ask request past the local server", + ); + asks.push(r); + } + + assert.ok(asks.length > 0, "no Ask request was recorded"); + + return asks; +} diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs index dae1f47ce..21077540a 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs @@ -12,11 +12,11 @@ const remoteScript = path.join(import.meta.dirname, "../remote/remote.mjs"); const runId = process.env.WB_TEST_RUN ?? `e2e${Date.now().toString(36)}`; -const runDir = `/tmp/wbt.${runId}`; +export const runDir = `/tmp/wbt.${runId}`; -const prepared = process.env.REVIEW_E2E_REMOTE_HOST; +export const prepared = process.env.REVIEW_E2E_REMOTE_HOST; -const alias = `wb-test-${prepared ?? "a"}`; +export const alias = `wb-test-${prepared ?? "a"}`; const title = "Remote order"; @@ -32,7 +32,7 @@ export const options = { env: { DEV_FAST_REVIEW_SSH_CONFIG: `${runDir}/ssh_config` }, }; -const createRemoteReview = String.raw` +export const createRemoteReview = String.raw` set -e field() { node -pe "JSON.parse(require('fs').readFileSync(0, 'utf8')).$1"; } rm -rf ~/wbrepo @@ -61,7 +61,7 @@ export async function listedAs(ctx, title) { return review && `${review.host}: ${review.repositoryGroup?.label}`; } -async function remote(...args) { +export async function remote(...args) { return ( await exec(process.execPath, [remoteScript, ...args], { env: { ...process.env, WB_TEST_RUN: runId }, @@ -70,7 +70,7 @@ async function remote(...args) { ).stdout.trim(); } -function onRemote(command, input = "") { +export function onRemote(command, input = "") { return new Promise((resolve, reject) => { const child = spawn( "ssh", @@ -96,7 +96,7 @@ function onRemote(command, input = "") { const remoteApi = (tool, input) => onRemote(`whiteboard api ${tool} -`, JSON.stringify(input)); -async function remoteToken() { +export async function remoteToken() { const out = (await onRemote("whiteboard remote attach --json")).split("\n"); let token; @@ -115,7 +115,7 @@ async function remoteToken() { return token; } -async function recordRequests(page, streamed) { +export async function recordRequests(page, streamed) { const cdp = await page.context().newCDPSession(page); const requests = new Map(); const entry = (id) => requests.get(id) ?? requests.set(id, { id }).get(id); @@ -191,7 +191,7 @@ export async function run(ctx) { } } -async function closeDesktop(ctx) { +export async function closeDesktop(ctx) { const session = await ctx.browser.newBrowserCDPSession().catch(() => null); await Promise.race([ @@ -672,7 +672,7 @@ async function desktopSshProcesses() { const desktopSsh = async () => (await desktopSshProcesses()).map(([pid]) => pid); -async function masterPid() { +export async function masterPid() { return (await desktopSshProcesses()).find( ([, args]) => args.includes(" -M -N ") && args.endsWith(`-- ${alias}`), )?.[0]; diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs index d9c9000bc..52d339074 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs +++ b/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs @@ -323,7 +323,7 @@ async function addHost(ctx, host) { return section; } -async function removeHost(ctx, host, { uninstall = false } = {}) { +export async function removeHost(ctx, host, { uninstall = false } = {}) { const section = await hostsSection(ctx); await section.getByRole("button", { name: `Remove ${host}` }).click(); diff --git a/apps/review-desktop/scripts/e2e/remote/aws-hosts.mjs b/apps/review-desktop/scripts/e2e/remote/aws-hosts.mjs index 573a9e6ce..2d546a83c 100644 --- a/apps/review-desktop/scripts/e2e/remote/aws-hosts.mjs +++ b/apps/review-desktop/scripts/e2e/remote/aws-hosts.mjs @@ -1,6 +1,6 @@ /** Hosts on AWS: `aws-up`, and removing a run's instances, key pair and security groups. */ import { addOnce, aws, run, sleep } from "./exec.mjs"; -import { waitForSsh } from "./ssh.mjs"; +import { installFakeAgent, waitForSsh } from "./ssh.mjs"; const maxInstances = 2; @@ -221,6 +221,9 @@ export async function awsUp(runState, name, options) { 60, "test -f /run/systemd/shutdown/scheduled", ); + + if (options["fake-agent"]) await installFakeAgent(runState, host); + console.log(alias); } diff --git a/apps/review-desktop/scripts/e2e/remote/docker-hosts.mjs b/apps/review-desktop/scripts/e2e/remote/docker-hosts.mjs index d4737ddcf..d74934e18 100644 --- a/apps/review-desktop/scripts/e2e/remote/docker-hosts.mjs +++ b/apps/review-desktop/scripts/e2e/remote/docker-hosts.mjs @@ -8,7 +8,7 @@ import { remoteDevInstall } from "../../remote-dev-install.mjs"; import { addOnce, docker, exec, run, sleep } from "./exec.mjs"; import { hostOf } from "./run-state.mjs"; -import { waitForSsh } from "./ssh.mjs"; +import { installFakeAgent, waitForSsh } from "./ssh.mjs"; const imageDir = path.join(import.meta.dirname, "image"); @@ -122,6 +122,9 @@ export async function up(runState, name, options) { if (jump && port) throw new Error("--port and --jump cannot be combined"); + if (options["fake-agent"] && auth !== "key") + throw new Error("--fake-agent needs --auth key"); + const toolchain = options.toolchain ?? "none"; if (toolchain !== "none" && !TOOLCHAIN_IMAGES[toolchain]) @@ -286,6 +289,8 @@ export async function up(runState, name, options) { : [], ); + if (options["fake-agent"]) await installFakeAgent(runState, host); + console.log(alias); } diff --git a/apps/review-desktop/scripts/e2e/remote/fake-agent/acp-agent.mjs b/apps/review-desktop/scripts/e2e/remote/fake-agent/acp-agent.mjs new file mode 100644 index 000000000..3665aadb5 --- /dev/null +++ b/apps/review-desktop/scripts/e2e/remote/fake-agent/acp-agent.mjs @@ -0,0 +1,83 @@ +import { createInterface } from "node:readline"; + +const send = (message) => process.stdout.write(`${JSON.stringify(message)}\n`); + +const reply = (id, result) => send({ jsonrpc: "2.0", id, result }); + +const notify = (method, params) => send({ jsonrpc: "2.0", method, params }); + +const cwds = new Map(); + +let toolCalls = 0; + +createInterface({ input: process.stdin }).on("line", (line) => { + if (!line.trim()) return; + + const { id, method, params } = JSON.parse(line); + + switch (method) { + case "initialize": + return reply(id, { + protocolVersion: 1, + agentCapabilities: { + loadSession: false, + promptCapabilities: { image: false }, + }, + authMethods: [], + }); + case "session/new": { + const sessionId = `wb-test-${cwds.size + 1}`; + + cwds.set(sessionId, params.cwd); + + return reply(id, { + sessionId, + modes: { + currentModeId: "build", + availableModes: [ + { id: "build", name: "Build" }, + { id: "plan", name: "Plan" }, + ], + }, + }); + } + + case "session/set_mode": + return reply(id, {}); + case "session/prompt": { + const { sessionId, prompt } = params; + const question = prompt.findLast((block) => block.type === "text")?.text; + + notify("session/update", { + sessionId, + update: { + sessionUpdate: "agent_message_chunk", + content: { type: "text", text: `You asked: ${question}. See \`f.ts\`.` }, + }, + }); + notify("session/update", { + sessionId, + update: { + sessionUpdate: "tool_call", + toolCallId: `t${++toolCalls}`, + title: "Read f.ts", + kind: "read", + status: "completed", + locations: [{ path: `${cwds.get(sessionId)}/f.ts`, line: 1 }], + }, + }); + + return reply(id, { stopReason: "end_turn" }); + } + + case "session/cancel": + return; + default: + if (id !== undefined) + send({ + jsonrpc: "2.0", + id, + error: { code: -32601, message: `unknown method ${method}` }, + }); + } +}); diff --git a/apps/review-desktop/scripts/e2e/remote/fake-agent/opencode b/apps/review-desktop/scripts/e2e/remote/fake-agent/opencode new file mode 100755 index 000000000..9ea3b4c7a --- /dev/null +++ b/apps/review-desktop/scripts/e2e/remote/fake-agent/opencode @@ -0,0 +1,10 @@ +#!/bin/sh +# wb-test: stands in for OpenCode. `opencode acp` speaks ACP; anything else prints a version. +case "$1" in + acp) + node=$(command -v node) + [ -n "$node" ] || for candidate in /opt/whiteboard/whiteboard-remote/bin/node "$HOME"/.dev/whiteboard-remote/versions/*/bin/node; do [ -x "$candidate" ] && node=$candidate; done + [ -x "$node" ] || { echo "opencode (wb-test): no node on PATH or in a Whiteboard install" >&2; exit 127; } + exec "$node" "${0%/*}/acp-agent.mjs" ;; + *) echo "opencode 0.0.0-wb-test" ;; +esac diff --git a/apps/review-desktop/scripts/e2e/remote/remote.mjs b/apps/review-desktop/scripts/e2e/remote/remote.mjs index 583c3bba5..100cdc02a 100644 --- a/apps/review-desktop/scripts/e2e/remote/remote.mjs +++ b/apps/review-desktop/scripts/e2e/remote/remote.mjs @@ -15,13 +15,13 @@ const usage = `usage: remote.mjs up [--platform linux/amd64|linux/arm64] [--image ] [--node 24|20|none] [--auth key|password] [--banner] [--shell bash|fish] [--jump ] [--sealed] [--delay-ms ] [--no-forwarding] [--port ] - [--toolchain rust|swift|dotnet] + [--toolchain rust|swift|dotnet] [--fake-agent] install [--version ] [--commit ] pack --out [--version ] [--arch x64|arm64] ssh -- forward pause | resume | logs - aws-up [--arch x64|arm64] [--sealed] + aws-up [--arch x64|arm64] [--sealed] [--fake-agent] down | down --all | down --every-run verify-clean @@ -49,6 +49,7 @@ async function main(argv) { "no-forwarding": { type: "boolean" }, port: { type: "string" }, toolchain: { type: "string" }, + "fake-agent": { type: "boolean" }, version: { type: "string" }, commit: { type: "string" }, out: { type: "string" }, diff --git a/apps/review-desktop/scripts/e2e/remote/ssh.mjs b/apps/review-desktop/scripts/e2e/remote/ssh.mjs index d28c5e956..67570fafe 100644 --- a/apps/review-desktop/scripts/e2e/remote/ssh.mjs +++ b/apps/review-desktop/scripts/e2e/remote/ssh.mjs @@ -145,3 +145,21 @@ export async function stopTunnels(runState, name) { state.forwards = state.forwards.filter((f) => name && f.host !== name); await runState.save(); } + +/** Puts the fake OpenCode in ~/.opencode/bin, where Ask finds it. */ +export async function installFakeAgent(runState, host) { + const dir = path.join(import.meta.dirname, "fake-agent"); + + await run("ssh", [...sshArgs(runState, host), "mkdir -p ~/.opencode/bin"]); + await run("scp", [ + "-F", + `${runState.dir}/ssh_config`, + path.join(dir, "opencode"), + path.join(dir, "acp-agent.mjs"), + `${host.alias}:.opencode/bin/`, + ]); + await run("ssh", [ + ...sshArgs(runState, host), + "chmod 755 ~/.opencode/bin/opencode ~/.opencode/bin/acp-agent.mjs", + ]); +} diff --git a/docs/remote-hosts.md b/docs/remote-hosts.md index 11b387ed3..41e508b39 100644 --- a/docs/remote-hosts.md +++ b/docs/remote-hosts.md @@ -117,6 +117,14 @@ Once a host is `online`, Settings lists its unconnected agents: Claude Code, Codex, OpenCode and Pi. **Connect** runs the agent's own install commands on the host. +**Ask.** Ask works on a review that lives on a host. The agent runs on the +host, in the review's checkout, with the agents and sign-ins already there +(`claude auth login`, `codex login`, `opencode auth login`, +`cursor-agent login`, Pi's `/login`). Threads are kept on the host, so every +laptop that attaches it sees the same history for its reviews. A file that an +answer names shows as text, because Source windows are not available for a +remote review. + ## Remove Whiteboard from a host In Settings, click **Remove** on the host, check **Also remove Whiteboard from diff --git a/packages/review/app/src/ask-files.test.tsx b/packages/review/app/src/ask-files.test.tsx index bf8b48495..052da414b 100644 --- a/packages/review/app/src/ask-files.test.tsx +++ b/packages/review/app/src/ask-files.test.tsx @@ -122,3 +122,63 @@ it("links the files an answer names to the Source window, once the checkout has await act(async () => root.unmount()); }); + +it("shows the files an answer names as text where Source windows are unavailable", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const post = vi.fn(async () => ({ ok: true })); + const session = testReviewSession({}, { post }); + const fetch = vi.spyOn(session, "fetch"); + + session.review!.available = { + sourceWindows: false, + languageFeatures: false, + }; + + const entries: AgentEntry[] = [ + { kind: "agent", id: "answer", text: "See `src/f.ts:12`." }, + ]; + + const thread: AskThreadState = { + id: "thread", + agent: "codex", + agentName: "Codex", + status: "idle", + readOnly: true, + bypass: false, + head: "7fd03b8e2", + cwd: "/checkouts/whiteboard", + selection: { title: "remote review" }, + entries, + }; + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + await act(async () => + root.render( + + + null} + /> + + , + ), + ); + + const code = [...container.querySelectorAll("code")].find( + (element) => element.textContent === "src/f.ts:12", + )!; + + expect(code).toBeDefined(); + expect(container.querySelector('[role="link"], a, button')).toBeNull(); + + await act(async () => code.click()); + expect(fetch).not.toHaveBeenCalled(); + expect(post).not.toHaveBeenCalled(); + + await act(async () => root.unmount()); +}); diff --git a/packages/review/app/src/ask-files.tsx b/packages/review/app/src/ask-files.tsx index d5c5dac74..6794c626a 100644 --- a/packages/review/app/src/ask-files.tsx +++ b/packages/review/app/src/ask-files.tsx @@ -141,16 +141,19 @@ function AskFileLink({ children: ReactNode; }): ReactNode { const files = useContext(AskFilesContext); + const canOpen = useReviewSession().review?.available?.sourceWindows !== false; const { path, line } = fileRef; - useEffect(() => files?.request(path), [files, path]); + useEffect(() => { + if (canOpen) files?.request(path); + }, [canOpen, files, path]); const file = files?.lookup(path); const tooltip = useTooltip( `Open ${file}${line ? `:${line}` : ""}`, ); - if (!files || !file) return fallback; + if (!canOpen || !files || !file) return fallback; const open = () => files.open(file, line); // A link, not a button: a button is one box, so a long path would jump to diff --git a/packages/review/app/src/ask-panel.test.tsx b/packages/review/app/src/ask-panel.test.tsx index 0a2b21914..5abe5854d 100644 --- a/packages/review/app/src/ask-panel.test.tsx +++ b/packages/review/app/src/ask-panel.test.tsx @@ -12,6 +12,7 @@ import { import { AskHistoryProvider, useAskHistory } from "./ask-history"; import { AskHistoryList } from "./ask-history-list"; import { AskPanelContent, AskReadOnlyThread } from "./ask-panel"; +import { AskSignIn } from "./ask-setup"; import { TestCanvasQuery } from "./canvas-query-test-utils"; import { ReviewSessionProvider } from "./host/review-session"; import { ReviewPanelProvider, useReviewPanel } from "./review-panel"; @@ -1337,6 +1338,57 @@ it("moves from setup to asking once an agent is installed", async () => { } }); +it("names the host a remote review's agents run on (wb-test-a)", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const remote = testReviewSession(); + remote.review = { ...remote.review!, host: "wb-test-a" }; + const laptop = testReviewSession(); + + for (const session of [remote, laptop]) + vi.spyOn(session, "fetch").mockImplementation(async (endpoint) => + endpoint === "/ask/agents" + ? Response.json({ + agents: [{ id: "claude", name: "Claude Code", available: false }], + }) + : Response.json({ ok: true }, { status: 404 }), + ); + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + const render = async (session: typeof remote) => + act(async () => + root.render( + + + {}} + /> + , + ), + ); + + try { + await render(remote); + expect(container.textContent).toContain("Not installed on wb-test-a"); + expect( + container.querySelector('[aria-label="Sign in"]')?.textContent, + ).toContain("wb-test-a"); + + await render(laptop); + expect(container.textContent).toContain("Not installed"); + expect(container.textContent).not.toContain("wb-test-a"); + } finally { + await act(async () => root.unmount()); + container.remove(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + } +}); + it("closes an agent that starts after its panel closed", async () => { vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); const session = testReviewSession(); diff --git a/packages/review/app/src/ask-setup.tsx b/packages/review/app/src/ask-setup.tsx index 04dfee72e..a3e2a170f 100644 --- a/packages/review/app/src/ask-setup.tsx +++ b/packages/review/app/src/ask-setup.tsx @@ -24,6 +24,7 @@ export function AskSetup({ selection: AgentSelection; }): ReactElement { const session = useReviewSession(); + const host = session.review?.host; const notify = useNotify(); const copy = async () => { @@ -47,7 +48,11 @@ export function AskSetup({
    @@ -61,7 +66,9 @@ export function AskSetup({ {candidate.name} - Not installed + + {host ? `Not installed on ${host}` : "Not installed"} + ))} @@ -93,6 +100,8 @@ export function AskSignIn({ command: string; onRetry: () => void; }): ReactElement { + const host = useReviewSession().review?.host; + return (

    {agentName} is signed out. Sign in again in a terminal, then try again; - the question is still here. + the question is still here.{host ? ` Run it on ${host}.` : null}

    $ {command} diff --git a/packages/review/src/ask/agents.ts b/packages/review/src/ask/agents.ts index acc83aaea..661333704 100644 --- a/packages/review/src/ask/agents.ts +++ b/packages/review/src/ask/agents.ts @@ -206,6 +206,8 @@ export type AskAgentLauncher = ( bypass?: boolean; /** The MCP servers its sessions are given. */ mcpServers?: McpServer[]; + /** Where it is found, and the environment it runs in. */ + env?: NodeJS.ProcessEnv; }, ) => Promise; @@ -286,15 +288,15 @@ const STDERR_LIMIT = 8_000; export const launchAskAgent: AskAgentLauncher = async ( agent, cwd, - { bypass = false, mcpServers = [] } = {}, + { bypass = false, mcpServers = [], env: base = process.env } = {}, ) => { const spec = askAgents[agent]; - const executable = await findAgent(spec); + const executable = await findAgent(spec, base); if (!executable) throw new Error(`${spec.name} is not installed.`); const env: NodeJS.ProcessEnv = { - ...process.env, + ...base, ...((bypass && spec.bypass?.env) || spec.env), }; diff --git a/packages/review/src/ask/threads.ts b/packages/review/src/ask/threads.ts index 2bd13f02c..b529f6166 100644 --- a/packages/review/src/ask/threads.ts +++ b/packages/review/src/ask/threads.ts @@ -1,5 +1,5 @@ import { PROTOCOL_VERSION, client, methods } from "@agentclientprotocol/sdk"; -import { errorMessage } from "@dev.fast/trace-core"; +import { errorMessage, shellQuote } from "@dev.fast/trace-core"; import { type AskAgentLauncher, askAgentTakesMcp, @@ -146,6 +146,52 @@ export interface AskTools { takesMcp?: (agent: AskAgentId) => Promise; } +/** Whiteboard's CLI as the tools, with `selector` in its env so it reaches + * this server and no other. */ +export function cliAskTools( + cliPath: () => string | undefined, + selector: { name: string; value: string }, +): AskTools { + const env = () => [ + selector, + ...(process.versions.electron + ? [{ name: "ELECTRON_RUN_AS_NODE", value: "1" }] + : []), + ...(process.env.DEV_REVIEW_HOME + ? [{ name: "DEV_REVIEW_HOME", value: process.env.DEV_REVIEW_HOME }] + : []), + ]; + + return { + mcpServers: () => { + const cli = cliPath(); + + return cli + ? [ + { + name: "whiteboard", + command: process.execPath, + args: [cli, "mcp"], + env: env(), + }, + ] + : []; + }, + cli: () => { + const cli = cliPath(); + + return ( + cli && + [ + ...env().map(({ name, value }) => `${name}=${shellQuote(value)}`), + shellQuote(process.execPath), + shellQuote(cli), + ].join(" ") + ); + }, + }; +} + /** How an agent reaches Whiteboard's tools, which its first prompt says. */ export type AskToolsReach = | { kind: "mcp" } diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts index 0d0d868a8..bc658e1e4 100644 --- a/packages/review/src/review-api/local-data.test.ts +++ b/packages/review/src/review-api/local-data.test.ts @@ -18,6 +18,11 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { + type ClientConnection, + agent, + methods, +} from "@agentclientprotocol/sdk"; import { setLocalVcsCommandObserver } from "@dev.fast/local-vcs"; import { type JsonValue, @@ -25,6 +30,8 @@ import { REVIEW_CLIENT_REMOTE, } from "@dev.fast/review-protocol"; import { type SubscribeCallback, subscribe } from "@parcel/watcher"; +import { type AskAgentLauncher, detectAskAgents } from "@review/ask/agents.js"; +import { AskThreads } from "@review/ask/threads.js"; import { rangeAnchor } from "@review/lens-selection"; import { createGlobalReviewServer } from "@review/server/desktop-server.js"; import { Hono } from "hono"; @@ -1013,6 +1020,120 @@ it("gives a remote caller a commit review's checkout as remoteRootPath, with the expect(await read({ [REVIEW_CLIENT_HEADER]: "local" })).toEqual(ownMachine); }); +it("gives a remote caller Ask answers with checkout-relative mentions", async () => { + const { reviewId } = await local.store.execute( + command({ + type: "create", + title: "Ask", + target: { kind: "commits", ...pins }, + }), + ); + + const { rootPath } = await local.data.agentCheckout( + local.store.read(reviewId), + ); + + const fake = agent({ name: "fake" }) + .onRequest(methods.agent.initialize, () => ({ + protocolVersion: 1, + agentCapabilities: { loadSession: true }, + authMethods: [], + })) + .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) + .onRequest(methods.agent.session.load, () => ({})) + .onRequest(methods.agent.session.prompt, () => ({ + stopReason: "end_turn" as const, + })); + + const launch: AskAgentLauncher = async () => { + let connection: ClientConnection | undefined; + + return { + connect: (client) => (connection = client.connect(fake)), + diagnostics: () => "", + stop: () => connection?.close(), + }; + }; + + const threads = new AskThreads(launch); + + const app = createReviewApi( + local.store, + local.data, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + { threads, agents: () => detectAskAgents({}) }, + ); + + const remote = { [REVIEW_CLIENT_HEADER]: REVIEW_CLIENT_REMOTE }; + + const request = async (route: string, body?: JsonValue) => { + const response = await app.request(`/${reviewId}/${route}`, { + headers: { ...remote, "content-type": "application/json" }, + ...(body !== undefined && { + method: "POST", + body: JSON.stringify(body), + }), + }); + + expect(response.status).toBe(200); + + return response; + }; + + const read = async (route: string, body?: JsonValue) => + (await request(route, body)).json(); + + const snapshot = async (threadId: string) => { + const reader = (await request("ask/watch", { threads: [threadId] })) + .body!.pipeThrough(new TextDecoderStream()) + .getReader(); + + const { value = "" } = await reader.read(); + await reader.cancel(); + + return JSON.parse(value.split("\n")[0]!).update.snapshot; + }; + + const agents = await read("ask/agents"); + expect(agents.agents.length).toBeGreaterThan(0); + + const { threadId } = await read("ask", { + agent: "claude", + question: { text: "Why?" }, + selection: { target: { kind: "text", quote: "value" }, title: "value" }, + }); + + const asked = await snapshot(threadId); + expect(asked.cwd).toBe(rootPath); + + await vi.waitFor(async () => + expect((await read("ask/threads")).threads).toHaveLength(1), + ); + + for (const route of [ + "ask/mentions?query=example", + `ask/mentions?query=example&thread=${threadId}`, + ]) { + const mentions = await read(route); + expect(mentions.paths).toContain(source.file); + expect(mentions.paths.every((file: string) => !file.startsWith("/"))).toBe( + true, + ); + } + + threads.close(threadId); + await read(`ask/${threadId}/open`, {}); + const reopened = await snapshot(threadId); + expect(reopened.cwd).toBe(rootPath); + threads.close(threadId); +}); + it("opens a stable native workspace on the Review's pinned checkout at the selected version", async () => { const { reviewId } = await local.store.execute( command({ diff --git a/packages/review/src/server/desktop-server.ts b/packages/review/src/server/desktop-server.ts index 18a3ef25a..ba584c35c 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -20,8 +20,8 @@ import { parseReviewCliInstallApplyRequest, reviewDiffrSummarizerInputSchema, } from "@dev.fast/review-protocol"; -import { shellQuote, writePrivateJsonAtomic } from "@dev.fast/trace-core"; -import type { AskTools } from "@review/ask/threads.js"; +import { writePrivateJsonAtomic } from "@dev.fast/trace-core"; +import { cliAskTools } from "@review/ask/threads.js"; import { applyCliInstall, declineCliInstall, @@ -149,40 +149,13 @@ export function createGlobalReviewServer( const relay = input.relay ?? new GlobalReviewDesktopVerbRelay({ maxClients: 1 }); - // This Desktop's own CLI, pinned to this instance so another running - // Whiteboard never answers it. - const askCliEnv = () => [ - { name: REVIEW_INSTANCE_ENV, value: identity.key }, - ...(process.versions.electron - ? [{ name: "ELECTRON_RUN_AS_NODE", value: "1" }] - : []), - ...(process.env.DEV_REVIEW_HOME - ? [{ name: "DEV_REVIEW_HOME", value: process.env.DEV_REVIEW_HOME }] - : []), - ]; - - // Ask sessions get its MCP server, `whiteboard mcp`; an agent whose model - // would not get it uses `whiteboard api` from its shell instead. - const askTools: AskTools = { - mcpServers: () => - discovery.cliPath - ? [ - { - name: "whiteboard", - command: process.execPath, - args: [discovery.cliPath, "mcp"], - env: askCliEnv(), - }, - ] - : [], - cli: () => - discovery.cliPath && - [ - ...askCliEnv().map(({ name, value }) => `${name}=${shellQuote(value)}`), - shellQuote(process.execPath), - shellQuote(discovery.cliPath), - ].join(" "), - }; + // Ask sessions get this Desktop's own CLI, pinned to this instance so + // another running Whiteboard never answers it: its MCP server, or + // `whiteboard api` from the shell for an agent whose model would not get it. + const askTools = cliAskTools(() => discovery.cliPath, { + name: REVIEW_INSTANCE_ENV, + value: identity.key, + }); const reviewStore = input.reviewStore; diff --git a/packages/review/src/server/headless-host.test.ts b/packages/review/src/server/headless-host.test.ts index be0a3ca42..4cccb8776 100644 --- a/packages/review/src/server/headless-host.test.ts +++ b/packages/review/src/server/headless-host.test.ts @@ -17,6 +17,11 @@ import path from "node:path"; import { DatabaseSync } from "node:sqlite"; import { PassThrough } from "node:stream"; +import { + type ClientConnection, + agent, + methods, +} from "@agentclientprotocol/sdk"; import * as diffr from "@dev.fast/diffr"; import { type JsonValue, @@ -26,6 +31,7 @@ import { STRUCTURAL_DIFF_WIRE_VERSION, parseReviewDesktopVerbFrame, } from "@dev.fast/review-protocol"; +import type { AskAgentLauncher } from "@review/ask/agents.js"; import { runReviewCli } from "@review/cli-runner.js"; import { connectReviewApi, @@ -51,7 +57,7 @@ import { reviewServerIsHealthy, } from "@review/server-discovery.js"; import sharp from "sharp"; -import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, expect, it, onTestFinished, vi } from "vitest"; import { z } from "zod"; import { createGlobalReviewServer } from "./desktop-server.js"; @@ -65,6 +71,7 @@ beforeEach(async () => { root = await mkdtemp(path.join(tmpdir(), "review-headless-")); vi.stubEnv("DEV_REVIEW_HOME", root); vi.stubEnv("DEV_FAST_REVIEW_TELEMETRY_DISABLED", "1"); + vi.stubEnv("SHELL", ""); }); afterEach(async () => { @@ -76,6 +83,7 @@ afterEach(async () => { async function start( stateDir = path.join(root, "server"), softwareMapEnabled = false, + launchAskAgent?: AskAgentLauncher, ) { const controller = new AbortController(); const ready = Promise.withResolvers(); @@ -85,6 +93,7 @@ async function start( softwareMapEnabled, signal: controller.signal, onReady: ready.resolve, + launchAskAgent, }); const stop = async () => { @@ -1305,3 +1314,229 @@ it("leaves a review's checkouts to a live lease owner and takes over a dead owne ); expect(worktrees()).toBe(2); }); + +async function askCall( + server: Awaited>, + route: string, + body?: JsonValue, +) { + return fetch(`${server.discovery.url}/reviews-api/${route}`, { + headers: { + "x-review-token": server.discovery.token, + "content-type": "application/json", + }, + ...(body !== undefined && { method: "POST", body: JSON.stringify(body) }), + }); +} + +async function executable(file: string, body: string) { + await mkdir(path.dirname(file), { recursive: true }); + await writeFile(file, `#!/bin/sh\n${body}\n`, { mode: 0o755 }); +} + +async function askAgentsAndEnv() { + const launched = Promise.withResolvers(); + const started = Date.now(); + + const server = await start( + undefined, + false, + async (_agent, _cwd, options) => { + launched.resolve(options?.env); + throw new Error("No agent here."); + }, + ); + + const startMs = Date.now() - started; + const { worktree } = await reviewsOfBothKinds(server.client); + + const { agents } = await ( + await askCall(server, `${worktree}/ask/agents`) + ).json(); + + await askCall(server, `${worktree}/ask`, { + agent: "claude", + question: { text: "Why?" }, + selection: { target: { kind: "text", quote: "value" }, title: "value" }, + }); + + return { + claude: agents.find((entry: { id: string }) => entry.id === "claude"), + env: await launched.promise, + startMs, + }; +} + +it("finds and launches Ask agents on the login shell's PATH, with its own node last", async () => { + const login = path.join(root, "login-bin"); + await executable(path.join(login, "claude"), "exit 0"); + await executable( + path.join(root, "shell"), + `echo Welcome\nprintf '\\nWHITEBOARD-PATH=%s\\n' '${login}'\necho Bye`, + ); + vi.stubEnv("SHELL", path.join(root, "shell")); + vi.stubEnv("PATH", "/usr/bin:/bin"); + + const { claude, env } = await askAgentsAndEnv(); + + expect(claude).toMatchObject({ available: true }); + expect(env?.PATH).toBe( + [login, "/usr/bin:/bin", path.dirname(process.execPath)].join( + path.delimiter, + ), + ); +}); + +it("starts with its own PATH when the login shell hangs", async () => { + const bin = path.join(root, "bin"); + await executable(path.join(bin, "claude"), "exit 0"); + await executable(path.join(root, "shell"), "sleep 60"); + vi.stubEnv("SHELL", path.join(root, "shell")); + vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); + + const { claude, env, startMs } = await askAgentsAndEnv(); + + expect(startMs).toBeLessThan(8_000); + expect(claude).toMatchObject({ available: true }); + expect(env?.PATH).toBe( + [bin, "/usr/bin:/bin", path.dirname(process.execPath)].join(path.delimiter), + ); +}, 20_000); + +it("opens an Ask thread in the review's checkout and closes it on stop", async () => { + const launched = Promise.withResolvers(); + let stopped = 0; + + const fake = agent({ name: "fake" }) + .onRequest(methods.agent.initialize, () => ({ + protocolVersion: 1, + agentCapabilities: {}, + authMethods: [], + })) + .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) + .onRequest(methods.agent.session.prompt, () => ({ + stopReason: "end_turn" as const, + })); + + const server = await start(undefined, false, async () => { + let connection: ClientConnection | undefined; + launched.resolve(); + + return { + connect: (client) => (connection = client.connect(fake)), + diagnostics: () => "", + stop: () => { + stopped++; + connection?.close(); + }, + }; + }); + + const { root: checkout, worktree } = await reviewsOfBothKinds(server.client); + + const opened = await askCall(server, `${worktree}/ask`, { + agent: "claude", + question: { text: "Why?" }, + selection: { target: { kind: "text", quote: "value" }, title: "value" }, + }); + + expect(opened.status).toBe(200); + + const { threadId } = await opened.json(); + + const reader = ( + await askCall(server, `${worktree}/ask/watch`, { threads: [threadId] }) + ) + .body!.pipeThrough(new TextDecoderStream()) + .getReader(); + + const { value = "" } = await reader.read(); + expect(JSON.parse(value.split("\n")[0]!).update.snapshot.cwd).toBe(checkout); + + await launched.promise; + expect(stopped).toBe(0); + await server.stop(); + expect(stopped).toBe(1); + + while (!(await reader.read()).done); +}); + +it("closes an Ask thread ten minutes after its last watcher leaves and reopens it from history", async () => { + let launches = 0, + stopped = 0; + + const fake = agent({ name: "fake" }) + .onRequest(methods.agent.initialize, () => ({ + protocolVersion: 1, + agentCapabilities: { loadSession: true }, + authMethods: [], + })) + .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) + .onRequest(methods.agent.session.load, () => ({})) + .onRequest(methods.agent.session.prompt, () => ({ + stopReason: "end_turn" as const, + })); + + const server = await start(undefined, false, async () => { + let connection: ClientConnection | undefined; + launches++; + + return { + connect: (client) => (connection = client.connect(fake)), + diagnostics: () => "", + stop: () => { + stopped++; + connection?.close(); + }, + }; + }); + + const { worktree } = await reviewsOfBothKinds(server.client); + + vi.useFakeTimers({ + shouldAdvanceTime: true, + toFake: ["setInterval", "clearInterval", "Date"], + }); + onTestFinished(() => { + vi.useRealTimers(); + }); + + const { threadId } = await ( + await askCall(server, `${worktree}/ask`, { + agent: "claude", + question: { text: "Why?" }, + selection: { target: { kind: "text", quote: "value" }, title: "value" }, + }) + ).json(); + + const reader = ( + await askCall(server, `${worktree}/ask/watch`, { threads: [threadId] }) + ) + .body!.pipeThrough(new TextDecoderStream()) + .getReader(); + + let seen = ""; + + while (!seen.includes('"status":"idle"')) + seen += (await reader.read()).value ?? ""; + + await vi.advanceTimersByTimeAsync(60 * 60_000); + expect(stopped).toBe(0); + + await reader.cancel(); + await vi.waitFor(async () => { + await vi.advanceTimersByTimeAsync(11 * 60_000); + expect(stopped).toBe(1); + }); + + const { threads } = await ( + await askCall(server, `${worktree}/ask/threads`) + ).json(); + + expect(threads).toMatchObject([{ id: threadId }]); + + expect( + (await askCall(server, `${worktree}/ask/${threadId}/open`, {})).status, + ).toBe(200); + await vi.waitFor(() => expect(launches).toBe(2)); +}); diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 7d589aa1e..5d79b34a6 100644 --- a/packages/review/src/server/headless-host.ts +++ b/packages/review/src/server/headless-host.ts @@ -1,12 +1,18 @@ +import { spawn } from "node:child_process"; import { randomBytes, randomUUID } from "node:crypto"; import { once } from "node:events"; +import { existsSync } from "node:fs"; import { mkdir, realpath, rm } from "node:fs/promises"; import { createServer } from "node:http"; +import path from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { isObjectValue } from "@dev.fast/json"; import { withFileLock, writePrivateJsonAtomic } from "@dev.fast/trace-core"; +import type { AskAgentLauncher } from "@review/ask/agents.js"; +import { cliAskTools } from "@review/ask/threads.js"; import { desktopUsesHome } from "@review/desktop-discovery.js"; +import { findReviewPackageRoot } from "@review/package-paths.js"; import { openReviewProfile } from "@review/review-api/profile.js"; import { DESKTOP_TAKEOVER_SIGNAL, @@ -40,6 +46,7 @@ interface HeadlessServerInput { /** The CLI's instance, already on the `headless` surface. */ telemetry?: Pick; onReady(discovery: ReviewServerDiscovery): void; + launchAskAgent?: AskAgentLauncher; } export class HeadlessServerBusyError extends Error {} @@ -114,6 +121,61 @@ export async function takeOverServerHome( } } +const LOGIN_PATH_TIMEOUT_MS = 4_000; + +/** Started over ssh, this server has sshd's PATH; agents are on the login + * shell's. Its own node comes last. */ +async function agentEnv(): Promise { + const login = process.env.SHELL && (await loginPath(process.env.SHELL)); + + return { + ...process.env, + PATH: [login, process.env.PATH, path.dirname(process.execPath)] + .filter(Boolean) + .join(path.delimiter), + }; +} + +/** Only its marked line is read, so start-up output is ignored. */ +function loginPath(shell: string) { + return new Promise((resolve) => { + const child = spawn( + shell, + ["-lic", 'printf "\\nWHITEBOARD-PATH=%s\\n" "$PATH"'], + { detached: true, stdio: ["ignore", "pipe", "ignore"] }, + ); + + let output = ""; + + child.stdout.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => (output += chunk)); + + const timer = setTimeout(() => { + try { + process.kill(-child.pid!, "SIGKILL"); + } catch {} + + resolve(undefined); + }, LOGIN_PATH_TIMEOUT_MS); + + child.once("error", () => { + clearTimeout(timer); + resolve(undefined); + }); + child.once("close", () => { + clearTimeout(timer); + + const found = output + .replaceAll("\r", "") + .split("\n") + .findLast((line) => line.startsWith("WHITEBOARD-PATH=")) + ?.slice("WHITEBOARD-PATH=".length); + + resolve(found?.startsWith("/") ? found : undefined); + }); + }); +} + async function serve(input: HeadlessServerInput) { if (input.signal.aborted) return; @@ -139,7 +201,15 @@ async function serve(input: HeadlessServerInput) { const relay = new GlobalReviewDesktopVerbRelay(); - const { app, api } = createWhiteboardCore({ + const cliPath = path.join( + findReviewPackageRoot(import.meta.url), + "dist", + "cli.js", + ); + + const env = await agentEnv(); + + const { app, api, close } = createWhiteboardCore({ profile: local, relay, token: discovery.token, @@ -148,6 +218,14 @@ async function serve(input: HeadlessServerInput) { // The scratchpad is the laptop's alone, even with a Desktop attached. scratchpad: () => false, status: () => ({ key: "headless", home: input.stateDir }), + ask: { + tools: cliAskTools(() => (existsSync(cliPath) ? cliPath : undefined), { + name: "DEV_REVIEW_SERVER_DIR", + value: input.stateDir, + }), + launch: input.launchAskAgent, + env, + }, }); app.route("/reviews-api", api); @@ -177,6 +255,8 @@ async function serve(input: HeadlessServerInput) { input.signal.addEventListener("abort", () => resolve(), { once: true }); }); } finally { + close(); + // Watch streams may live forever. Drain ordinary requests, then bound shutdown. const forceClose = setTimeout(() => server.closeAllConnections(), 5_000); forceClose.unref(); diff --git a/packages/review/src/server/headless-sharing.test.ts b/packages/review/src/server/headless-sharing.test.ts index 010ab7816..525f6b4c1 100644 --- a/packages/review/src/server/headless-sharing.test.ts +++ b/packages/review/src/server/headless-sharing.test.ts @@ -37,6 +37,7 @@ it("commits and uploads through a real headless server and CLI without Desktop, vi.stubEnv("DEV_REVIEW_SHARE_ORIGIN", "https://sharing.test"); vi.stubEnv("DEV_REVIEW_HOME", stateDir); vi.stubEnv("DEV_FAST_REVIEW_TELEMETRY_DISABLED", "1"); + vi.stubEnv("SHELL", ""); const realFetch = globalThis.fetch; const requests: { url: string; headers: Headers; body?: string }[] = []; const blobs = new Map(); diff --git a/packages/review/src/server/review-gateway.test.ts b/packages/review/src/server/review-gateway.test.ts index 41b2f02af..a6d22d573 100644 --- a/packages/review/src/server/review-gateway.test.ts +++ b/packages/review/src/server/review-gateway.test.ts @@ -649,3 +649,188 @@ it("answers 503 with an offline host's reason for a review on its last list, aft error: "wb-a is offline: it refused the connection.", }); }); + +const ASK_ROUTES: [string, string][] = [ + ["GET", "ask/mentions?query=f"], + ["POST", "ask/t1/prompt"], + ["DELETE", "ask/t1"], +]; + +it("forwards Ask routes of a remote review and marks the caller remote", async () => { + const reviewId = randomUUID(); + const seen: string[] = []; + + const fake = await startFake({ + version, + reviewIds: [reviewId], + handle(request, response) { + if (!request.url?.startsWith(`/reviews-api/${reviewId}/ask`)) + return false; + seen.push( + `${request.method} ${request.url.split(`/${reviewId}/`)[1]} ${request.headers["x-review-client"]}`, + ); + response.setHeader("content-type", "application/json"); + response.end('{"ok":true}'); + + return true; + }, + }); + + const { request, gateway } = await startGateway([ + { alias: "wb-a", endpoint: fake.endpoint }, + ]); + + await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); + + for (const [method, route] of ASK_ROUTES) { + const response = await request(`/${reviewId}/${route}`, { + method, + body: method === "GET" ? undefined : "{}", + }); + + expect(response.status, `${method} ${route}`).toBe(200); + } + + expect(seen).toEqual( + ASK_ROUTES.map(([method, route]) => `${method} ${route} remote`), + ); +}); + +it("keeps an Ask watch stream open across a 12 s silence and ends it when the host goes", async () => { + const reviewId = randomUUID(); + let hang = false; + let remote: import("node:http").ServerResponse | undefined; + + const fake = await startFake({ + version, + reviewIds: [reviewId], + handle(request, response) { + if (request.url === "/health") return hang; + + if ( + request.method !== "POST" || + request.url !== `/reviews-api/${reviewId}/ask/watch` + ) + return false; + remote = response; + response.setHeader("content-type", "text/event-stream"); + response.write('data: {"n":1}\n\n'); + + return true; + }, + }); + + const { request, gateway } = await startGateway([ + { alias: "wb-a", endpoint: fake.endpoint }, + ]); + + await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); + + const response = await request(`/${reviewId}/ask/watch`, { + method: "POST", + body: JSON.stringify({ threads: ["t1"] }), + }); + + expect(response.status).toBe(200); + + const reader = response + .body!.pipeThrough(new TextDecoderStream()) + .getReader(); + + expect((await reader.read()).value).toContain('{"n":1}'); + await new Promise((resolve) => setTimeout(resolve, 12_000)); + remote!.write('data: {"n":2}\n\n'); + expect((await reader.read()).value).toContain('{"n":2}'); + + hang = true; + await expect + .poll(() => gateway.hosts()[0]?.state, { timeout: 15_000 }) + .toBe("offline"); + await expect(reader.read()).rejects.toThrow("terminated"); +}, 40_000); + +it("passes a 2 MB Ask files body to the remote intact", async () => { + const reviewId = randomUUID(); + let received = 0; + + const fake = await startFake({ + version, + reviewIds: [reviewId], + handle(request, response) { + if (!request.url?.startsWith(`/reviews-api/${reviewId}/ask/t1/files`)) + return false; + request.on("data", (chunk: Buffer) => (received += chunk.length)); + request.on("end", () => response.end('{"ok":true}')); + + return true; + }, + }); + + const { request, gateway } = await startGateway([ + { alias: "wb-a", endpoint: fake.endpoint }, + ]); + + await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); + + const body = JSON.stringify({ paths: ["x".repeat(2 * 1024 * 1024)] }); + + const response = await request(`/${reviewId}/ask/t1/files`, { + method: "POST", + body, + }); + + expect(response.status).toBe(200); + expect(received).toBe(Buffer.byteLength(body)); +}); + +it.each([ + ["GET", "ask/agents/codex/offer"], + ["POST", "ask/t1/permissions"], + ["POST", "ask/t1/choice"], +])( + "waits past the 10 s limit for a slow %s %s and keeps the host online when it never answers", + async (method, route) => { + const slow = randomUUID(); + const hung = randomUUID(); + + const fake = await startFake({ + version, + reviewIds: [slow, hung], + handle(request, response) { + if (request.url?.startsWith(`/reviews-api/${hung}/${route}`)) + return true; + + if (!request.url?.startsWith(`/reviews-api/${slow}/${route}`)) + return false; + + setTimeout(() => { + response.setHeader("content-type", "application/json"); + response.end('{"ok":true}'); + }, 11_000); + + return true; + }, + }); + + const { request, gateway } = await startLocalGateway( + root, + [{ alias: "wb-a", endpoint: fake.endpoint }], + { slowRouteMs: 14_000 }, + ); + + await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); + + const init = method === "POST" ? { method, body: "{}" } : {}; + + const [answered, unanswered] = await Promise.all([ + request(`/${slow}/${route}`, init), + request(`/${hung}/${route}`, init), + ]); + + expect(answered.status).toBe(200); + expect(unanswered.status).toBe(504); + expect(unanswered.headers.get(REVIEW_HOST_HEADER)).toBe("wb-a"); + expect(gateway.hosts()[0]?.state).toBe("online"); + }, + 30_000, +); diff --git a/packages/review/src/server/review-gateway.ts b/packages/review/src/server/review-gateway.ts index 0791cde59..5b1c779dd 100644 --- a/packages/review/src/server/review-gateway.ts +++ b/packages/review/src/server/review-gateway.ts @@ -51,7 +51,7 @@ const LOCAL_ROUTES = new Set([ interface RemoteRoute { method: string; path: string; - /** May wait on preparing a checkout. */ + /** May wait on preparing a checkout or launching an agent. */ slow?: true; } @@ -69,6 +69,23 @@ const REMOTE_ROUTES: readonly RemoteRoute[] = [ { method: "GET", path: "maps/:name" }, { method: "POST", path: "copy-context" }, { method: "GET", path: "language-context", slow: true }, + { method: "GET", path: "ask/agents" }, + { method: "GET", path: "ask/agents/:agent/offer", slow: true }, + { method: "GET", path: "ask/mentions", slow: true }, + { method: "GET", path: "ask/threads" }, + { method: "POST", path: "ask/watch" }, + { method: "GET", path: "ask/:thread" }, + { method: "POST", path: "ask", slow: true }, + { + method: "POST", + path: "ask/:thread/:action{open|choice|permissions}", + slow: true, + }, + { + method: "POST", + path: "ask/:thread/:action{prompt|permission|files|retry|cancel|close}", + }, + { method: "DELETE", path: "ask/:thread" }, ]; const SLOW_ROUTE_TIMEOUT_MS = 120_000; diff --git a/packages/review/src/server/review-server-core.test.ts b/packages/review/src/server/review-server-core.test.ts index b020e9343..197d72550 100644 --- a/packages/review/src/server/review-server-core.test.ts +++ b/packages/review/src/server/review-server-core.test.ts @@ -26,6 +26,7 @@ beforeEach(async () => { root = await mkdtemp(path.join(tmpdir(), "review-server-core-")); vi.stubEnv("DEV_REVIEW_HOME", root); vi.stubEnv("DEV_FAST_REVIEW_TELEMETRY_DISABLED", "1"); + vi.stubEnv("SHELL", ""); }); afterEach(async () => { diff --git a/packages/review/src/server/review-server-core.ts b/packages/review/src/server/review-server-core.ts index 0df82a7b7..6368837df 100644 --- a/packages/review/src/server/review-server-core.ts +++ b/packages/review/src/server/review-server-core.ts @@ -4,7 +4,11 @@ import type { ReviewServerHealthWithToken, } from "@dev.fast/review-protocol"; import { traceMachineEnabled } from "@dev.fast/trace-core"; -import { detectAskAgents, launchAskAgent } from "@review/ask/agents.js"; +import { + type AskAgentLauncher, + detectAskAgents, + launchAskAgent, +} from "@review/ask/agents.js"; import { AskThreads, type AskTools } from "@review/ask/threads.js"; import { readBuildCommit, @@ -128,7 +132,13 @@ export interface WhiteboardCoreInput { scratchpad: () => boolean; status: () => JsonObject; hooks?: ReviewApiHooks; - ask?: { tools: AskTools }; + ask?: { + tools: AskTools; + /** Tests only: an in-process agent. */ + launch?: AskAgentLauncher; + /** Where agents are found, and the environment they run in. */ + env?: NodeJS.ProcessEnv; + }; } export function createWhiteboardCore(input: WhiteboardCoreInput) { @@ -143,8 +153,16 @@ export function createWhiteboardCore(input: WhiteboardCoreInput) { const callbacks = relayReviewCallbacks(input.relay, input.softwareMapEnabled); + const { ask } = input; + + const launch = ask?.launch ?? launchAskAgent; + const askThreads = - input.ask && new AskThreads(launchAskAgent, input.ask.tools); + ask && + new AskThreads( + (agent, cwd, options) => launch(agent, cwd, { ...options, env: ask.env }), + ask.tools, + ); const closeDeletedThreads = () => askThreads?.closeDeletedReviews((id) => @@ -171,7 +189,10 @@ export function createWhiteboardCore(input: WhiteboardCoreInput) { () => traceMachineEnabled(), input.status, input.hooks, - askThreads && { threads: askThreads, agents: () => detectAskAgents() }, + askThreads && { + threads: askThreads, + agents: () => detectAskAgents(ask?.env), + }, ); // A shared store mounts the publisher with the rest of sharing. From 5de3fd146442f294d6dde46eb20c2c8e924f62c0 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 12:32:38 -0400 Subject: [PATCH 2/7] Start the remote server without waiting on the login shell Ask's agent environment is resolved on first use, so a slow login shell no longer delays startup. --- packages/review/src/server/headless-host.ts | 4 +--- packages/review/src/server/review-server-core.ts | 7 ++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 5d79b34a6..d28b3a187 100644 --- a/packages/review/src/server/headless-host.ts +++ b/packages/review/src/server/headless-host.ts @@ -207,8 +207,6 @@ async function serve(input: HeadlessServerInput) { "cli.js", ); - const env = await agentEnv(); - const { app, api, close } = createWhiteboardCore({ profile: local, relay, @@ -224,7 +222,7 @@ async function serve(input: HeadlessServerInput) { value: input.stateDir, }), launch: input.launchAskAgent, - env, + env: agentEnv(), }, }); diff --git a/packages/review/src/server/review-server-core.ts b/packages/review/src/server/review-server-core.ts index 6368837df..f910bfdaf 100644 --- a/packages/review/src/server/review-server-core.ts +++ b/packages/review/src/server/review-server-core.ts @@ -137,7 +137,7 @@ export interface WhiteboardCoreInput { /** Tests only: an in-process agent. */ launch?: AskAgentLauncher; /** Where agents are found, and the environment they run in. */ - env?: NodeJS.ProcessEnv; + env?: Promise; }; } @@ -160,7 +160,8 @@ export function createWhiteboardCore(input: WhiteboardCoreInput) { const askThreads = ask && new AskThreads( - (agent, cwd, options) => launch(agent, cwd, { ...options, env: ask.env }), + async (agent, cwd, options) => + launch(agent, cwd, { ...options, env: await ask.env }), ask.tools, ); @@ -191,7 +192,7 @@ export function createWhiteboardCore(input: WhiteboardCoreInput) { input.hooks, askThreads && { threads: askThreads, - agents: () => detectAskAgents(ask?.env), + agents: async () => detectAskAgents(await ask?.env), }, ); From 7a95e25e1a99578f10ebb736e6b4886c12dad980 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 12:32:38 -0400 Subject: [PATCH 3/7] Share one fake Ask agent across server tests --- packages/review/src/ask/ask-test-utils.ts | 40 +++++++++ .../review/src/review-api/local-data.test.ts | 32 +------ .../review/src/server/headless-host.test.ts | 83 +++---------------- 3 files changed, 55 insertions(+), 100 deletions(-) create mode 100644 packages/review/src/ask/ask-test-utils.ts diff --git a/packages/review/src/ask/ask-test-utils.ts b/packages/review/src/ask/ask-test-utils.ts new file mode 100644 index 000000000..b3c9a6156 --- /dev/null +++ b/packages/review/src/ask/ask-test-utils.ts @@ -0,0 +1,40 @@ +import { + type ClientConnection, + agent, + methods, +} from "@agentclientprotocol/sdk"; + +import type { AskAgentLauncher } from "./agents.js"; + +/** An ACP agent that ends every turn at once; counts launches and stops. */ +export function idleAskAgent() { + const counts = { launches: 0, stopped: 0 }; + + const fake = agent({ name: "fake" }) + .onRequest(methods.agent.initialize, () => ({ + protocolVersion: 1, + agentCapabilities: { loadSession: true }, + authMethods: [], + })) + .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) + .onRequest(methods.agent.session.load, () => ({})) + .onRequest(methods.agent.session.prompt, () => ({ + stopReason: "end_turn" as const, + })); + + const launch: AskAgentLauncher = async () => { + let connection: ClientConnection | undefined; + counts.launches++; + + return { + connect: (client) => (connection = client.connect(fake)), + diagnostics: () => "", + stop: () => { + counts.stopped++; + connection?.close(); + }, + }; + }; + + return { launch, counts }; +} diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts index bc658e1e4..dffde7f5d 100644 --- a/packages/review/src/review-api/local-data.test.ts +++ b/packages/review/src/review-api/local-data.test.ts @@ -18,11 +18,6 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { - type ClientConnection, - agent, - methods, -} from "@agentclientprotocol/sdk"; import { setLocalVcsCommandObserver } from "@dev.fast/local-vcs"; import { type JsonValue, @@ -30,7 +25,8 @@ import { REVIEW_CLIENT_REMOTE, } from "@dev.fast/review-protocol"; import { type SubscribeCallback, subscribe } from "@parcel/watcher"; -import { type AskAgentLauncher, detectAskAgents } from "@review/ask/agents.js"; +import { detectAskAgents } from "@review/ask/agents.js"; +import { idleAskAgent } from "@review/ask/ask-test-utils.js"; import { AskThreads } from "@review/ask/threads.js"; import { rangeAnchor } from "@review/lens-selection"; import { createGlobalReviewServer } from "@review/server/desktop-server.js"; @@ -1033,29 +1029,7 @@ it("gives a remote caller Ask answers with checkout-relative mentions", async () local.store.read(reviewId), ); - const fake = agent({ name: "fake" }) - .onRequest(methods.agent.initialize, () => ({ - protocolVersion: 1, - agentCapabilities: { loadSession: true }, - authMethods: [], - })) - .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) - .onRequest(methods.agent.session.load, () => ({})) - .onRequest(methods.agent.session.prompt, () => ({ - stopReason: "end_turn" as const, - })); - - const launch: AskAgentLauncher = async () => { - let connection: ClientConnection | undefined; - - return { - connect: (client) => (connection = client.connect(fake)), - diagnostics: () => "", - stop: () => connection?.close(), - }; - }; - - const threads = new AskThreads(launch); + const threads = new AskThreads(idleAskAgent().launch); const app = createReviewApi( local.store, diff --git a/packages/review/src/server/headless-host.test.ts b/packages/review/src/server/headless-host.test.ts index 4cccb8776..c719d043a 100644 --- a/packages/review/src/server/headless-host.test.ts +++ b/packages/review/src/server/headless-host.test.ts @@ -17,11 +17,6 @@ import path from "node:path"; import { DatabaseSync } from "node:sqlite"; import { PassThrough } from "node:stream"; -import { - type ClientConnection, - agent, - methods, -} from "@agentclientprotocol/sdk"; import * as diffr from "@dev.fast/diffr"; import { type JsonValue, @@ -32,6 +27,7 @@ import { parseReviewDesktopVerbFrame, } from "@dev.fast/review-protocol"; import type { AskAgentLauncher } from "@review/ask/agents.js"; +import { idleAskAgent } from "@review/ask/ask-test-utils.js"; import { runReviewCli } from "@review/cli-runner.js"; import { connectReviewApi, @@ -1336,7 +1332,6 @@ async function executable(file: string, body: string) { async function askAgentsAndEnv() { const launched = Promise.withResolvers(); - const started = Date.now(); const server = await start( undefined, @@ -1347,7 +1342,6 @@ async function askAgentsAndEnv() { }, ); - const startMs = Date.now() - started; const { worktree } = await reviewsOfBothKinds(server.client); const { agents } = await ( @@ -1363,7 +1357,6 @@ async function askAgentsAndEnv() { return { claude: agents.find((entry: { id: string }) => entry.id === "claude"), env: await launched.promise, - startMs, }; } @@ -1394,9 +1387,8 @@ it("starts with its own PATH when the login shell hangs", async () => { vi.stubEnv("SHELL", path.join(root, "shell")); vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); - const { claude, env, startMs } = await askAgentsAndEnv(); + const { claude, env } = await askAgentsAndEnv(); - expect(startMs).toBeLessThan(8_000); expect(claude).toMatchObject({ available: true }); expect(env?.PATH).toBe( [bin, "/usr/bin:/bin", path.dirname(process.execPath)].join(path.delimiter), @@ -1404,33 +1396,8 @@ it("starts with its own PATH when the login shell hangs", async () => { }, 20_000); it("opens an Ask thread in the review's checkout and closes it on stop", async () => { - const launched = Promise.withResolvers(); - let stopped = 0; - - const fake = agent({ name: "fake" }) - .onRequest(methods.agent.initialize, () => ({ - protocolVersion: 1, - agentCapabilities: {}, - authMethods: [], - })) - .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) - .onRequest(methods.agent.session.prompt, () => ({ - stopReason: "end_turn" as const, - })); - - const server = await start(undefined, false, async () => { - let connection: ClientConnection | undefined; - launched.resolve(); - - return { - connect: (client) => (connection = client.connect(fake)), - diagnostics: () => "", - stop: () => { - stopped++; - connection?.close(); - }, - }; - }); + const { launch, counts } = idleAskAgent(); + const server = await start(undefined, false, launch); const { root: checkout, worktree } = await reviewsOfBothKinds(server.client); @@ -1453,43 +1420,17 @@ it("opens an Ask thread in the review's checkout and closes it on stop", async ( const { value = "" } = await reader.read(); expect(JSON.parse(value.split("\n")[0]!).update.snapshot.cwd).toBe(checkout); - await launched.promise; - expect(stopped).toBe(0); + await vi.waitFor(() => expect(counts.launches).toBe(1)); + expect(counts.stopped).toBe(0); await server.stop(); - expect(stopped).toBe(1); + expect(counts.stopped).toBe(1); while (!(await reader.read()).done); }); it("closes an Ask thread ten minutes after its last watcher leaves and reopens it from history", async () => { - let launches = 0, - stopped = 0; - - const fake = agent({ name: "fake" }) - .onRequest(methods.agent.initialize, () => ({ - protocolVersion: 1, - agentCapabilities: { loadSession: true }, - authMethods: [], - })) - .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) - .onRequest(methods.agent.session.load, () => ({})) - .onRequest(methods.agent.session.prompt, () => ({ - stopReason: "end_turn" as const, - })); - - const server = await start(undefined, false, async () => { - let connection: ClientConnection | undefined; - launches++; - - return { - connect: (client) => (connection = client.connect(fake)), - diagnostics: () => "", - stop: () => { - stopped++; - connection?.close(); - }, - }; - }); + const { launch, counts } = idleAskAgent(); + const server = await start(undefined, false, launch); const { worktree } = await reviewsOfBothKinds(server.client); @@ -1521,12 +1462,12 @@ it("closes an Ask thread ten minutes after its last watcher leaves and reopens i seen += (await reader.read()).value ?? ""; await vi.advanceTimersByTimeAsync(60 * 60_000); - expect(stopped).toBe(0); + expect(counts.stopped).toBe(0); await reader.cancel(); await vi.waitFor(async () => { await vi.advanceTimersByTimeAsync(11 * 60_000); - expect(stopped).toBe(1); + expect(counts.stopped).toBe(1); }); const { threads } = await ( @@ -1538,5 +1479,5 @@ it("closes an Ask thread ten minutes after its last watcher leaves and reopens i expect( (await askCall(server, `${worktree}/ask/${threadId}/open`, {})).status, ).toBe(200); - await vi.waitFor(() => expect(launches).toBe(2)); + await vi.waitFor(() => expect(counts.launches).toBe(2)); }); From 5819baa2aecd971d087f371bf209255222577736 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 15:15:52 -0400 Subject: [PATCH 4/7] Drop Ask tests that other suites already cover The gateway's slow-route and stream-silence handling, thread idle close and reopen, and the install's API checks are tested where they live; what stays covers Ask's own routes and wiring. --- .../reviewRemoteInstaller.containers.test.ts | 40 +------ packages/review/src/server/desktop-server.ts | 3 - .../review/src/server/headless-host.test.ts | 56 +--------- .../review/src/server/review-gateway.test.ts | 105 ------------------ 4 files changed, 3 insertions(+), 201 deletions(-) diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts index 1e0b0b2ca..265f4d47c 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts @@ -5,7 +5,7 @@ import assert from "node:assert/strict"; import { execFile, execFileSync, spawn } from "node:child_process"; -import { randomBytes, randomUUID } from "node:crypto"; +import { randomBytes } from "node:crypto"; import { mkdtemp, readFile, rm } from "node:fs/promises"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; @@ -137,13 +137,7 @@ test("a host with no Node downloads the tarball, with diffr inside it; whiteboar assert.equal(await inContainer("fresh", "ls -d ~/.dev/whiteboard-remote/install.lock 2>/dev/null | wc -l"), "0"); }); -const FETCH = `const [url, token, method, path, body] = process.argv.slice(1); -const response = await fetch(url + path, { method, headers: { "x-review-token": token, "content-type": "application/json" }, body: method === "GET" ? undefined : body }); -const bytes = Buffer.from(await response.arrayBuffer()); -console.log(JSON.stringify({ status: response.status, type: response.headers.get("content-type"), size: bytes.length, text: bytes.toString() }));`; -const PIXEL = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=="; - -test("an install has the adapters but not the bundled agent binaries; instructions, images and structural diff answer", { skip, timeout: 10 * 60_000 }, async (t) => { +test("an install has the Ask adapters but not the bundled agent binaries", { skip, timeout: 10 * 60_000 }, async () => { await installRemote(input("fresh").value); const modules = `${installed}/node_modules`; @@ -152,36 +146,6 @@ test("an install has the adapters but not the bundled agent binaries; instructio assert.equal(await inContainer("fresh", `ls -d ${modules}/@openai/codex-* ${modules}/@anthropic-ai/claude-agent-sdk-* 2>/dev/null | wc -l`), "0"); assert.equal(await inContainer("fresh", `test -x ${modules}/@dev.fast/diffr-linux-*/diffr && echo yes`), "yes"); assert.equal(await version("fresh"), VERSION); - - await inContainer( - "fresh", - `git init -q -b main ~/repo && cd ~/repo && printf 'export const a = 1;\\n' > a.ts && git add . && git -c user.name=t -c user.email=t@t commit -qm one && printf 'export const a = 2;\\n' > a.ts && git -c user.name=t -c user.email=t@t commit -qam two`, - ); - t.after(() => inContainer("fresh", "~/.local/bin/whiteboard server stop; rm -rf ~/repo")); - const attach = JSON.parse((await inContainer("fresh", "~/.local/bin/whiteboard remote attach --json")).split("\n").find((line) => line.startsWith("{"))!); - const api = async (method: string, path: string, body?: unknown) => - JSON.parse((await run("docker", ["exec", "-u", "dev", `wb-test-${runId}-fresh`, `${installed}/bin/node`, "--input-type=module", "-e", FETCH, attach.url, attach.token, method, `/reviews-api${path}`, JSON.stringify(body ?? null)])).stdout); - const instructions = await api("GET", "/instructions?topic=authoring"); - assert.equal(instructions.status, 200, instructions.text); - assert.ok(instructions.size > 0); - const [base, head] = (await inContainer("fresh", "cd ~/repo && git rev-parse HEAD~1 HEAD")).split("\n"); - const repositoryId = JSON.parse((await api("POST", "/repositories", { path: "/home/dev/repo" })).text).id; - const { reviewId } = JSON.parse( - (await api("POST", "/commands", { operation: { type: "create", title: "Install check", target: { kind: "commits", repositoryId, base, head } } })).text, - ); - - const id = randomUUID(); - assert.equal((await api("POST", "/resources", { id, repositoryId, kind: "image", base64: PIXEL })).status, 200); - const image = await api("GET", `/${reviewId}/resources/${id}`); - assert.deepEqual([image.status, image.type], [200, "image/png"]); - assert.ok(image.size > 0); - - const diff = await api("GET", `/${reviewId}/structural-diff`); - assert.equal(diff.status, 200); - const events = diff.text.trim().split("\n").map((line: string) => JSON.parse(line)); - assert.ok(events.some((event: { type: string }) => event.type === "file")); - assert.equal(events.at(-1).type, "complete"); - assert.equal(events.at(-1).failed, 0); }); test("a tarball the host cannot download fails the install with the URL and leaves nothing", { skip, timeout: 10 * 60_000 }, async () => { diff --git a/packages/review/src/server/desktop-server.ts b/packages/review/src/server/desktop-server.ts index ba584c35c..d3de0471d 100644 --- a/packages/review/src/server/desktop-server.ts +++ b/packages/review/src/server/desktop-server.ts @@ -149,9 +149,6 @@ export function createGlobalReviewServer( const relay = input.relay ?? new GlobalReviewDesktopVerbRelay({ maxClients: 1 }); - // Ask sessions get this Desktop's own CLI, pinned to this instance so - // another running Whiteboard never answers it: its MCP server, or - // `whiteboard api` from the shell for an agent whose model would not get it. const askTools = cliAskTools(() => discovery.cliPath, { name: REVIEW_INSTANCE_ENV, value: identity.key, diff --git a/packages/review/src/server/headless-host.test.ts b/packages/review/src/server/headless-host.test.ts index c719d043a..c92950ccf 100644 --- a/packages/review/src/server/headless-host.test.ts +++ b/packages/review/src/server/headless-host.test.ts @@ -53,7 +53,7 @@ import { reviewServerIsHealthy, } from "@review/server-discovery.js"; import sharp from "sharp"; -import { afterEach, beforeEach, expect, it, onTestFinished, vi } from "vitest"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { z } from "zod"; import { createGlobalReviewServer } from "./desktop-server.js"; @@ -1427,57 +1427,3 @@ it("opens an Ask thread in the review's checkout and closes it on stop", async ( while (!(await reader.read()).done); }); - -it("closes an Ask thread ten minutes after its last watcher leaves and reopens it from history", async () => { - const { launch, counts } = idleAskAgent(); - const server = await start(undefined, false, launch); - - const { worktree } = await reviewsOfBothKinds(server.client); - - vi.useFakeTimers({ - shouldAdvanceTime: true, - toFake: ["setInterval", "clearInterval", "Date"], - }); - onTestFinished(() => { - vi.useRealTimers(); - }); - - const { threadId } = await ( - await askCall(server, `${worktree}/ask`, { - agent: "claude", - question: { text: "Why?" }, - selection: { target: { kind: "text", quote: "value" }, title: "value" }, - }) - ).json(); - - const reader = ( - await askCall(server, `${worktree}/ask/watch`, { threads: [threadId] }) - ) - .body!.pipeThrough(new TextDecoderStream()) - .getReader(); - - let seen = ""; - - while (!seen.includes('"status":"idle"')) - seen += (await reader.read()).value ?? ""; - - await vi.advanceTimersByTimeAsync(60 * 60_000); - expect(counts.stopped).toBe(0); - - await reader.cancel(); - await vi.waitFor(async () => { - await vi.advanceTimersByTimeAsync(11 * 60_000); - expect(counts.stopped).toBe(1); - }); - - const { threads } = await ( - await askCall(server, `${worktree}/ask/threads`) - ).json(); - - expect(threads).toMatchObject([{ id: threadId }]); - - expect( - (await askCall(server, `${worktree}/ask/${threadId}/open`, {})).status, - ).toBe(200); - await vi.waitFor(() => expect(counts.launches).toBe(2)); -}); diff --git a/packages/review/src/server/review-gateway.test.ts b/packages/review/src/server/review-gateway.test.ts index a6d22d573..35c65371d 100644 --- a/packages/review/src/server/review-gateway.test.ts +++ b/packages/review/src/server/review-gateway.test.ts @@ -696,59 +696,6 @@ it("forwards Ask routes of a remote review and marks the caller remote", async ( ); }); -it("keeps an Ask watch stream open across a 12 s silence and ends it when the host goes", async () => { - const reviewId = randomUUID(); - let hang = false; - let remote: import("node:http").ServerResponse | undefined; - - const fake = await startFake({ - version, - reviewIds: [reviewId], - handle(request, response) { - if (request.url === "/health") return hang; - - if ( - request.method !== "POST" || - request.url !== `/reviews-api/${reviewId}/ask/watch` - ) - return false; - remote = response; - response.setHeader("content-type", "text/event-stream"); - response.write('data: {"n":1}\n\n'); - - return true; - }, - }); - - const { request, gateway } = await startGateway([ - { alias: "wb-a", endpoint: fake.endpoint }, - ]); - - await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); - - const response = await request(`/${reviewId}/ask/watch`, { - method: "POST", - body: JSON.stringify({ threads: ["t1"] }), - }); - - expect(response.status).toBe(200); - - const reader = response - .body!.pipeThrough(new TextDecoderStream()) - .getReader(); - - expect((await reader.read()).value).toContain('{"n":1}'); - await new Promise((resolve) => setTimeout(resolve, 12_000)); - remote!.write('data: {"n":2}\n\n'); - expect((await reader.read()).value).toContain('{"n":2}'); - - hang = true; - await expect - .poll(() => gateway.hosts()[0]?.state, { timeout: 15_000 }) - .toBe("offline"); - await expect(reader.read()).rejects.toThrow("terminated"); -}, 40_000); - it("passes a 2 MB Ask files body to the remote intact", async () => { const reviewId = randomUUID(); let received = 0; @@ -782,55 +729,3 @@ it("passes a 2 MB Ask files body to the remote intact", async () => { expect(response.status).toBe(200); expect(received).toBe(Buffer.byteLength(body)); }); - -it.each([ - ["GET", "ask/agents/codex/offer"], - ["POST", "ask/t1/permissions"], - ["POST", "ask/t1/choice"], -])( - "waits past the 10 s limit for a slow %s %s and keeps the host online when it never answers", - async (method, route) => { - const slow = randomUUID(); - const hung = randomUUID(); - - const fake = await startFake({ - version, - reviewIds: [slow, hung], - handle(request, response) { - if (request.url?.startsWith(`/reviews-api/${hung}/${route}`)) - return true; - - if (!request.url?.startsWith(`/reviews-api/${slow}/${route}`)) - return false; - - setTimeout(() => { - response.setHeader("content-type", "application/json"); - response.end('{"ok":true}'); - }, 11_000); - - return true; - }, - }); - - const { request, gateway } = await startLocalGateway( - root, - [{ alias: "wb-a", endpoint: fake.endpoint }], - { slowRouteMs: 14_000 }, - ); - - await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); - - const init = method === "POST" ? { method, body: "{}" } : {}; - - const [answered, unanswered] = await Promise.all([ - request(`/${slow}/${route}`, init), - request(`/${hung}/${route}`, init), - ]); - - expect(answered.status).toBe(200); - expect(unanswered.status).toBe(504); - expect(unanswered.headers.get(REVIEW_HOST_HEADER)).toBe("wb-a"); - expect(gateway.hosts()[0]?.state).toBe("online"); - }, - 30_000, -); From 7867b88a18e614e40b65b1df8855208aec3e0eab Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 15:24:19 -0400 Subject: [PATCH 5/7] Keep only the Ask tests on what this change adds The remote-ask journey drives the gateway's Ask routes end to end; route-table, copy and package-listing checks and tests of unchanged Ask code are gone. --- .../reviewRemoteInstaller.containers.test.ts | 11 --- packages/review/app/src/ask-panel.test.tsx | 52 ---------- packages/review/src/ask/ask-test-utils.ts | 40 -------- .../review/src/review-api/local-data.test.ts | 95 ------------------- .../review/src/server/headless-host.test.ts | 56 +++++++---- .../review/src/server/review-gateway.test.ts | 80 ---------------- 6 files changed, 35 insertions(+), 299 deletions(-) delete mode 100644 packages/review/src/ask/ask-test-utils.ts diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts index 265f4d47c..a41912dbb 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.containers.test.ts @@ -137,17 +137,6 @@ test("a host with no Node downloads the tarball, with diffr inside it; whiteboar assert.equal(await inContainer("fresh", "ls -d ~/.dev/whiteboard-remote/install.lock 2>/dev/null | wc -l"), "0"); }); -test("an install has the Ask adapters but not the bundled agent binaries", { skip, timeout: 10 * 60_000 }, async () => { - await installRemote(input("fresh").value); - const modules = `${installed}/node_modules`; - - assert.equal(await inContainer("fresh", `test -f ${modules}/@agentclientprotocol/codex-acp/dist/index.js && echo yes`), "yes"); - assert.equal(await inContainer("fresh", `test -f ${modules}/@agentclientprotocol/claude-agent-acp/dist/index.js && echo yes`), "yes"); - assert.equal(await inContainer("fresh", `ls -d ${modules}/@openai/codex-* ${modules}/@anthropic-ai/claude-agent-sdk-* 2>/dev/null | wc -l`), "0"); - assert.equal(await inContainer("fresh", `test -x ${modules}/@dev.fast/diffr-linux-*/diffr && echo yes`), "yes"); - assert.equal(await version("fresh"), VERSION); -}); - test("a tarball the host cannot download fails the install with the URL and leaves nothing", { skip, timeout: 10 * 60_000 }, async () => { await reset("fresh"); const { value } = input("fresh"); diff --git a/packages/review/app/src/ask-panel.test.tsx b/packages/review/app/src/ask-panel.test.tsx index 5abe5854d..0a2b21914 100644 --- a/packages/review/app/src/ask-panel.test.tsx +++ b/packages/review/app/src/ask-panel.test.tsx @@ -12,7 +12,6 @@ import { import { AskHistoryProvider, useAskHistory } from "./ask-history"; import { AskHistoryList } from "./ask-history-list"; import { AskPanelContent, AskReadOnlyThread } from "./ask-panel"; -import { AskSignIn } from "./ask-setup"; import { TestCanvasQuery } from "./canvas-query-test-utils"; import { ReviewSessionProvider } from "./host/review-session"; import { ReviewPanelProvider, useReviewPanel } from "./review-panel"; @@ -1338,57 +1337,6 @@ it("moves from setup to asking once an agent is installed", async () => { } }); -it("names the host a remote review's agents run on (wb-test-a)", async () => { - vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); - const remote = testReviewSession(); - remote.review = { ...remote.review!, host: "wb-test-a" }; - const laptop = testReviewSession(); - - for (const session of [remote, laptop]) - vi.spyOn(session, "fetch").mockImplementation(async (endpoint) => - endpoint === "/ask/agents" - ? Response.json({ - agents: [{ id: "claude", name: "Claude Code", available: false }], - }) - : Response.json({ ok: true }, { status: 404 }), - ); - - const container = document.createElement("div"); - document.body.append(container); - const root = createRoot(container); - - const render = async (session: typeof remote) => - act(async () => - root.render( - - - {}} - /> - , - ), - ); - - try { - await render(remote); - expect(container.textContent).toContain("Not installed on wb-test-a"); - expect( - container.querySelector('[aria-label="Sign in"]')?.textContent, - ).toContain("wb-test-a"); - - await render(laptop); - expect(container.textContent).toContain("Not installed"); - expect(container.textContent).not.toContain("wb-test-a"); - } finally { - await act(async () => root.unmount()); - container.remove(); - vi.restoreAllMocks(); - vi.unstubAllGlobals(); - } -}); - it("closes an agent that starts after its panel closed", async () => { vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); const session = testReviewSession(); diff --git a/packages/review/src/ask/ask-test-utils.ts b/packages/review/src/ask/ask-test-utils.ts deleted file mode 100644 index b3c9a6156..000000000 --- a/packages/review/src/ask/ask-test-utils.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { - type ClientConnection, - agent, - methods, -} from "@agentclientprotocol/sdk"; - -import type { AskAgentLauncher } from "./agents.js"; - -/** An ACP agent that ends every turn at once; counts launches and stops. */ -export function idleAskAgent() { - const counts = { launches: 0, stopped: 0 }; - - const fake = agent({ name: "fake" }) - .onRequest(methods.agent.initialize, () => ({ - protocolVersion: 1, - agentCapabilities: { loadSession: true }, - authMethods: [], - })) - .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) - .onRequest(methods.agent.session.load, () => ({})) - .onRequest(methods.agent.session.prompt, () => ({ - stopReason: "end_turn" as const, - })); - - const launch: AskAgentLauncher = async () => { - let connection: ClientConnection | undefined; - counts.launches++; - - return { - connect: (client) => (connection = client.connect(fake)), - diagnostics: () => "", - stop: () => { - counts.stopped++; - connection?.close(); - }, - }; - }; - - return { launch, counts }; -} diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts index dffde7f5d..0d0d868a8 100644 --- a/packages/review/src/review-api/local-data.test.ts +++ b/packages/review/src/review-api/local-data.test.ts @@ -25,9 +25,6 @@ import { REVIEW_CLIENT_REMOTE, } from "@dev.fast/review-protocol"; import { type SubscribeCallback, subscribe } from "@parcel/watcher"; -import { detectAskAgents } from "@review/ask/agents.js"; -import { idleAskAgent } from "@review/ask/ask-test-utils.js"; -import { AskThreads } from "@review/ask/threads.js"; import { rangeAnchor } from "@review/lens-selection"; import { createGlobalReviewServer } from "@review/server/desktop-server.js"; import { Hono } from "hono"; @@ -1016,98 +1013,6 @@ it("gives a remote caller a commit review's checkout as remoteRootPath, with the expect(await read({ [REVIEW_CLIENT_HEADER]: "local" })).toEqual(ownMachine); }); -it("gives a remote caller Ask answers with checkout-relative mentions", async () => { - const { reviewId } = await local.store.execute( - command({ - type: "create", - title: "Ask", - target: { kind: "commits", ...pins }, - }), - ); - - const { rootPath } = await local.data.agentCheckout( - local.store.read(reviewId), - ); - - const threads = new AskThreads(idleAskAgent().launch); - - const app = createReviewApi( - local.store, - local.data, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - { threads, agents: () => detectAskAgents({}) }, - ); - - const remote = { [REVIEW_CLIENT_HEADER]: REVIEW_CLIENT_REMOTE }; - - const request = async (route: string, body?: JsonValue) => { - const response = await app.request(`/${reviewId}/${route}`, { - headers: { ...remote, "content-type": "application/json" }, - ...(body !== undefined && { - method: "POST", - body: JSON.stringify(body), - }), - }); - - expect(response.status).toBe(200); - - return response; - }; - - const read = async (route: string, body?: JsonValue) => - (await request(route, body)).json(); - - const snapshot = async (threadId: string) => { - const reader = (await request("ask/watch", { threads: [threadId] })) - .body!.pipeThrough(new TextDecoderStream()) - .getReader(); - - const { value = "" } = await reader.read(); - await reader.cancel(); - - return JSON.parse(value.split("\n")[0]!).update.snapshot; - }; - - const agents = await read("ask/agents"); - expect(agents.agents.length).toBeGreaterThan(0); - - const { threadId } = await read("ask", { - agent: "claude", - question: { text: "Why?" }, - selection: { target: { kind: "text", quote: "value" }, title: "value" }, - }); - - const asked = await snapshot(threadId); - expect(asked.cwd).toBe(rootPath); - - await vi.waitFor(async () => - expect((await read("ask/threads")).threads).toHaveLength(1), - ); - - for (const route of [ - "ask/mentions?query=example", - `ask/mentions?query=example&thread=${threadId}`, - ]) { - const mentions = await read(route); - expect(mentions.paths).toContain(source.file); - expect(mentions.paths.every((file: string) => !file.startsWith("/"))).toBe( - true, - ); - } - - threads.close(threadId); - await read(`ask/${threadId}/open`, {}); - const reopened = await snapshot(threadId); - expect(reopened.cwd).toBe(rootPath); - threads.close(threadId); -}); - it("opens a stable native workspace on the Review's pinned checkout at the selected version", async () => { const { reviewId } = await local.store.execute( command({ diff --git a/packages/review/src/server/headless-host.test.ts b/packages/review/src/server/headless-host.test.ts index c92950ccf..120492751 100644 --- a/packages/review/src/server/headless-host.test.ts +++ b/packages/review/src/server/headless-host.test.ts @@ -17,6 +17,11 @@ import path from "node:path"; import { DatabaseSync } from "node:sqlite"; import { PassThrough } from "node:stream"; +import { + type ClientConnection, + agent, + methods, +} from "@agentclientprotocol/sdk"; import * as diffr from "@dev.fast/diffr"; import { type JsonValue, @@ -27,7 +32,6 @@ import { parseReviewDesktopVerbFrame, } from "@dev.fast/review-protocol"; import type { AskAgentLauncher } from "@review/ask/agents.js"; -import { idleAskAgent } from "@review/ask/ask-test-utils.js"; import { runReviewCli } from "@review/cli-runner.js"; import { connectReviewApi, @@ -1380,24 +1384,34 @@ it("finds and launches Ask agents on the login shell's PATH, with its own node l ); }); -it("starts with its own PATH when the login shell hangs", async () => { - const bin = path.join(root, "bin"); - await executable(path.join(bin, "claude"), "exit 0"); - await executable(path.join(root, "shell"), "sleep 60"); - vi.stubEnv("SHELL", path.join(root, "shell")); - vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); - - const { claude, env } = await askAgentsAndEnv(); - - expect(claude).toMatchObject({ available: true }); - expect(env?.PATH).toBe( - [bin, "/usr/bin:/bin", path.dirname(process.execPath)].join(path.delimiter), - ); -}, 20_000); - it("opens an Ask thread in the review's checkout and closes it on stop", async () => { - const { launch, counts } = idleAskAgent(); - const server = await start(undefined, false, launch); + let launches = 0, + stopped = 0; + + const fake = agent({ name: "fake" }) + .onRequest(methods.agent.initialize, () => ({ + protocolVersion: 1, + agentCapabilities: {}, + authMethods: [], + })) + .onRequest(methods.agent.session.new, () => ({ sessionId: "session" })) + .onRequest(methods.agent.session.prompt, () => ({ + stopReason: "end_turn" as const, + })); + + const server = await start(undefined, false, async () => { + let connection: ClientConnection | undefined; + launches++; + + return { + connect: (client) => (connection = client.connect(fake)), + diagnostics: () => "", + stop: () => { + stopped++; + connection?.close(); + }, + }; + }); const { root: checkout, worktree } = await reviewsOfBothKinds(server.client); @@ -1420,10 +1434,10 @@ it("opens an Ask thread in the review's checkout and closes it on stop", async ( const { value = "" } = await reader.read(); expect(JSON.parse(value.split("\n")[0]!).update.snapshot.cwd).toBe(checkout); - await vi.waitFor(() => expect(counts.launches).toBe(1)); - expect(counts.stopped).toBe(0); + await vi.waitFor(() => expect(launches).toBe(1)); + expect(stopped).toBe(0); await server.stop(); - expect(counts.stopped).toBe(1); + expect(stopped).toBe(1); while (!(await reader.read()).done); }); diff --git a/packages/review/src/server/review-gateway.test.ts b/packages/review/src/server/review-gateway.test.ts index 35c65371d..41b2f02af 100644 --- a/packages/review/src/server/review-gateway.test.ts +++ b/packages/review/src/server/review-gateway.test.ts @@ -649,83 +649,3 @@ it("answers 503 with an offline host's reason for a review on its last list, aft error: "wb-a is offline: it refused the connection.", }); }); - -const ASK_ROUTES: [string, string][] = [ - ["GET", "ask/mentions?query=f"], - ["POST", "ask/t1/prompt"], - ["DELETE", "ask/t1"], -]; - -it("forwards Ask routes of a remote review and marks the caller remote", async () => { - const reviewId = randomUUID(); - const seen: string[] = []; - - const fake = await startFake({ - version, - reviewIds: [reviewId], - handle(request, response) { - if (!request.url?.startsWith(`/reviews-api/${reviewId}/ask`)) - return false; - seen.push( - `${request.method} ${request.url.split(`/${reviewId}/`)[1]} ${request.headers["x-review-client"]}`, - ); - response.setHeader("content-type", "application/json"); - response.end('{"ok":true}'); - - return true; - }, - }); - - const { request, gateway } = await startGateway([ - { alias: "wb-a", endpoint: fake.endpoint }, - ]); - - await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); - - for (const [method, route] of ASK_ROUTES) { - const response = await request(`/${reviewId}/${route}`, { - method, - body: method === "GET" ? undefined : "{}", - }); - - expect(response.status, `${method} ${route}`).toBe(200); - } - - expect(seen).toEqual( - ASK_ROUTES.map(([method, route]) => `${method} ${route} remote`), - ); -}); - -it("passes a 2 MB Ask files body to the remote intact", async () => { - const reviewId = randomUUID(); - let received = 0; - - const fake = await startFake({ - version, - reviewIds: [reviewId], - handle(request, response) { - if (!request.url?.startsWith(`/reviews-api/${reviewId}/ask/t1/files`)) - return false; - request.on("data", (chunk: Buffer) => (received += chunk.length)); - request.on("end", () => response.end('{"ok":true}')); - - return true; - }, - }); - - const { request, gateway } = await startGateway([ - { alias: "wb-a", endpoint: fake.endpoint }, - ]); - - await expect.poll(() => gateway.hosts()[0]?.state).toBe("online"); - - const body = JSON.stringify({ paths: ["x".repeat(2 * 1024 * 1024)] }); - - const response = await request(`/${reviewId}/ask/t1/files`, { - method: "POST", - body, - }); - - expect(response.status).toBe(200); - expect(received).toBe(Buffer.byteLength(body)); -}); From f365a6ad7a37d7936ab936ccfbdae08159d54412 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 15:30:32 -0400 Subject: [PATCH 6/7] Trim Ask comments --- packages/review/src/ask/agents.ts | 1 - packages/review/src/server/headless-host.ts | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/review/src/ask/agents.ts b/packages/review/src/ask/agents.ts index 661333704..109fc1d3e 100644 --- a/packages/review/src/ask/agents.ts +++ b/packages/review/src/ask/agents.ts @@ -206,7 +206,6 @@ export type AskAgentLauncher = ( bypass?: boolean; /** The MCP servers its sessions are given. */ mcpServers?: McpServer[]; - /** Where it is found, and the environment it runs in. */ env?: NodeJS.ProcessEnv; }, ) => Promise; diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index d28b3a187..a33c5c0a4 100644 --- a/packages/review/src/server/headless-host.ts +++ b/packages/review/src/server/headless-host.ts @@ -124,7 +124,7 @@ export async function takeOverServerHome( const LOGIN_PATH_TIMEOUT_MS = 4_000; /** Started over ssh, this server has sshd's PATH; agents are on the login - * shell's. Its own node comes last. */ + * shell's. */ async function agentEnv(): Promise { const login = process.env.SHELL && (await loginPath(process.env.SHELL)); @@ -136,7 +136,6 @@ async function agentEnv(): Promise { }; } -/** Only its marked line is read, so start-up output is ignored. */ function loginPath(shell: string) { return new Promise((resolve) => { const child = spawn( From aa952657656c2102144b8f469da6da5b1d1b1eb5 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Wed, 14 Oct 2026 15:35:58 -0400 Subject: [PATCH 7/7] Say a laptop connects to a host in the Ask docs --- docs/remote-hosts.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/remote-hosts.md b/docs/remote-hosts.md index 41e508b39..833715da9 100644 --- a/docs/remote-hosts.md +++ b/docs/remote-hosts.md @@ -121,8 +121,8 @@ the host. host, in the review's checkout, with the agents and sign-ins already there (`claude auth login`, `codex login`, `opencode auth login`, `cursor-agent login`, Pi's `/login`). Threads are kept on the host, so every -laptop that attaches it sees the same history for its reviews. A file that an -answer names shows as text, because Source windows are not available for a +laptop that connects to it sees the same history for its reviews. A file that +an answer names shows as text, because Source windows are not available for a remote review. ## Remove Whiteboard from a host