diff --git a/apps/review-desktop/UPSTREAM b/apps/review-desktop/UPSTREAM index 5a35951ab..842cc0b53 100644 --- a/apps/review-desktop/UPSTREAM +++ b/apps/review-desktop/UPSTREAM @@ -369,6 +369,10 @@ one of these entries: fatal either: it ends that connection only. Isolation is unconditional, so a remote Source window gets no upstream reload dialog; it reconnects through the fork's resolver and shows its host state in the status bar. +- `src/vs/platform/window/common/window.ts` and + `src/vs/platform/native/electron-main/nativeHostMainService.ts`: the + `openWindow` option `reviewReferencesToShow` runs Find References in the + opened window. - `extensions/package.json` and the extension `package.json`/`package-lock.json` files: `@vscode/extension-telemetry` is hoisted to the shared extensions install (github keeps its own ^1.0.0 copy; the shared copy is ^0.9.8). The @@ -614,7 +618,8 @@ one of these entries: profile, extension host status, lifetime and extensions scanner services, and the `logger`, `userDataProfiles`, `remoteextensionsenvironment`, `remoteExtensionsScanner`, `remoteFilesystem` and extension host debug - broadcast channels. `telemetry` is a no-op channel and `ITelemetryService` + broadcast channels. `telemetry` is a no-op channel, `extensions` and + `extensionGalleryManifest` list no extensions, and `ITelemetryService` is `NullTelemetryService`. Cuts requests, telemetry appenders and machine ids, the gallery and extension installs, language packs, terminals, the agent host, MCP and the sandbox helper. diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewApiEditorSerializer.test.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewApiEditorSerializer.test.ts index 0a474ab7c..8c1635fa0 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewApiEditorSerializer.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewApiEditorSerializer.test.ts @@ -26,12 +26,15 @@ test("opening historical source requests its version and opens a separate native { async getConnection() { return { serverUrl: "http://localhost", token: "test" }; } } as never, { async openWindow(...args: unknown[]) { opened.push(args); } } as never, { warn() {} } as never, + {} as never, + {} as never, ); t.after(() => tabs.dispose()); await tabs.openApiSource({ reviewId: "review-a", kind: "version", version: 7 }, "Historical Review"); assert.equal(requests[0], "http://localhost/reviews-api/review-a/navigator?version=7"); assert.equal(opened.length, 1); - assert.deepEqual(opened[0]![1], { forceNewWindow: true }); + assert.equal((opened[0]![0] as { workspaceUri: { fsPath: string } }[])[0].workspaceUri.fsPath, "/pinned/review.code-workspace"); + assert.equal((opened[0]![1] as { remoteAuthority?: string }).remoteAuthority, undefined); ok = false; await assert.rejects(tabs.openApiSource({ reviewId: "review-a", kind: "current" }, "Review")); assert.equal(opened.length, 1); @@ -60,7 +63,7 @@ test("native group restoration preserves both reviews, order and pinned source v }; const groupService = { groups, mainPart: { activeGroup: undefined as EditorGroupModel | undefined } }; const createTabs = () => - new ReviewCanvasEditorTabsService(instantiation as never, editors as never, groupService as never, {} as never, {} as never, { warn() {} } as never); + new ReviewCanvasEditorTabsService(instantiation as never, editors as never, groupService as never, {} as never, {} as never, { warn() {} } as never, {} as never, {} as never); tabs = createTabs(); const registry = Registry.as(EditorExtensions.EditorFactory); registry.start({ get: () => instantiation } as never); @@ -127,7 +130,7 @@ test("current Source tabs retain identity and main version tabs still restore", }, invokeFunction(fn: (accessor: { get(): ReviewCanvasEditorTabsService }) => unknown) { return fn({ get: () => tabs }); }, }; - tabs = new ReviewCanvasEditorTabsService(instantiation as never, { onDidCloseEditor: Event.None } as never, {} as never, {} as never, {} as never, { warn() {} } as never); + tabs = new ReviewCanvasEditorTabsService(instantiation as never, { onDidCloseEditor: Event.None } as never, {} as never, {} as never, {} as never, { warn() {} } as never, {} as never, {} as never); try { const serializer = new ReviewApiEditorSerializer(); const restored = serializer.deserialize(instantiation as never, JSON.stringify({ kind: "api-source", reviewId: "a", title: "A", selection: { reviewId: "a", kind: "current" } })); diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts index 86d019584..1f312fb33 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts @@ -71,9 +71,11 @@ import type { ReviewDocumentWidthChoice, ReviewKeymapChoice, ReviewReadyNotificationChoice, + ReviewRemoteHostActions, ReviewRuntimeConfig, ReviewSurfaceEvent, ReviewTheme, + ReviewVerbResponse, TutorialProgressV1, TutorialStepId, } from "../../../common/reviewProtocol.js"; @@ -86,6 +88,7 @@ import { } from "../../../common/reviewProtocol.js"; import { IReviewVerbsService } from "../../../contrib/verbs/reviewVerbs.js"; import { vetoStoppingAskAgents } from "../../reviewAskShutdown.js"; +import { notifyOpenFailure } from "./reviewHostFailure.js"; import { confirmReviewDeletion } from "../../reviewDeleteConfirmation.js"; import { showReviewNotification } from "../../reviewNotification.js"; import { showReviewCanvasMenu } from "../../reviewCanvasMenu.js"; @@ -459,6 +462,7 @@ export class ReviewCanvasEditorPane extends EditorPane { kind: "api", reviewId, ...remoteEntry(this.apiCatalog.reviews.find((review) => review.reviewId === reviewId)), + remoteHosts: this.remoteHostActions(), structuralDiffEnabled: this.currentStructuralDiffEnabled(), softwareMapEnabled: this.currentSoftwareMapEnabled(), documentWidth: this.currentDocumentWidth(), @@ -487,20 +491,18 @@ export class ReviewCanvasEditorPane extends EditorPane { request: requestReviewApi, post: async (request) => { if (request.name === "openSourceTree") { - await this.tabsService.openApiSource(sourceSelection, input.getName()); - return { ok: true }; + return this.openSource(() => this.tabsService.openApiSource(sourceSelection, input.getName())); } if (request.name === "reveal") { - const range = { startLine: request.args.startLine, endLine: request.args.endLine }; - await this.apiSource.open( - { view: reviewSourceAnchor(sourceView, request.args.pins), file: request.args.path, side: request.args.side ?? "head" }, - range, - ); - return { ok: true }; + const { args } = request; + return this.openSource(() => this.apiSource.open( + { view: reviewSourceAnchor(sourceView, args.pins), file: args.path, side: args.side ?? "head" }, + { startLine: args.startLine, endLine: args.endLine }, + )); } if (request.name === "openDiff") { - await this.apiSource.openDiff(sourceView, request.args.path); - return { ok: true }; + const { path } = request.args; + return this.openSource(() => this.apiSource.openDiff(sourceView, path)); } return this.verbs.dispatch(request); }, @@ -574,11 +576,11 @@ export class ReviewCanvasEditorPane extends EditorPane { this.reviewTelemetryService.capture("review_restored", { via: "home" }); return this.apiCatalog.lifecycle(uuid, "active"); }, - hostStates: () => this.desktopConnection.readRemoteHosts(), + remoteHosts: this.remoteHostActions(), openSourceTree: (uuid) => { const api = this.apiCatalog.reviews.find((review) => review.reviewId === uuid); if (api) { - void this.tabsService.openApiSource({ reviewId: api.reviewId, kind: "current" }, api.title).catch(error => this.notificationService.error(error)); + void this.tabsService.openApiSource({ reviewId: api.reviewId, kind: "current" }, api.title).catch(error => this.notifyOpenFailure(error)); return; } }, @@ -642,6 +644,32 @@ export class ReviewCanvasEditorPane extends EditorPane { } } + private notifyOpenFailure(error: unknown): void { + notifyOpenFailure(this.notificationService, (alias) => this.desktopConnection.retryRemoteHost(alias), error); + } + + /** Answers the canvas instead of rejecting, once the failure is shown. */ + private async openSource(open: () => Promise): Promise { + try { + await open(); + return { ok: true }; + } catch (error) { + this.notifyOpenFailure(error); + return { ok: false, error: error instanceof Error ? error.message : String(error) }; + } + } + + private remoteHostActions(): ReviewRemoteHostActions { + return { + states: () => this.desktopConnection.readRemoteHosts(), + retry: (alias) => this.desktopConnection.retryRemoteHost(alias), + install: (alias) => this.desktopConnection.installRemoteHost(alias), + openSettings: async () => { + await this.tabsService.openSettings(true); + }, + }; + } + async refreshComparison(): Promise { if (!(this.input instanceof ReviewCanvasEditorInput) || this.input.target.kind !== "api") return; this.readyInput = undefined; diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewHostFailure.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewHostFailure.ts new file mode 100644 index 000000000..19530ddea --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewHostFailure.ts @@ -0,0 +1,28 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import Severity from "../../../../base/common/severity.js"; +import type { INotificationService } from "../../../../platform/notification/common/notification.js"; +import { reviewHostStatus, type ReviewGatewayHostState } from "../../../common/reviewProtocol.js"; + +type HostDown = Pick; + +export class ReviewHostDown extends Error { + constructor(readonly host: HostDown) { + super(reviewHostStatus(host).sentence); + } +} + +export function notifyOpenFailure(notifications: INotificationService, retry: (alias: string) => Promise, error: unknown): void { + if (!(error instanceof ReviewHostDown)) { + notifications.error(error instanceof Error ? error : String(error)); + return; + } + const { alias } = error.host; + const choices = reviewHostStatus(error.host).action === "retry" + ? [{ label: "Retry", run: () => void retry(alias).catch(retryError => notifications.error(retryError)) }] + : []; + notifications.prompt(Severity.Warning, error.message, choices); +} diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.test.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.test.ts index 1fc09d905..fb7c920c6 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.test.ts @@ -4,7 +4,7 @@ import test from "node:test"; import type { ReviewApiSummary } from "../../../common/reviewProtocol.js"; import { remoteEntry, withRemoteEntry } from "./reviewRemoteEntry.js"; -const available = { sourceWindows: false, languageFeatures: false }; +const available = { sourceWindows: false }; const entry = (host?: string) => ({ reviewId: "r1", title: "Remote", ...(host && { host, available }) }) as ReviewApiSummary; test("a review a remote opened before its list entry arrived takes the host once the entry does", () => { @@ -15,6 +15,8 @@ test("a review a remote opened before its list entry arrived takes the host once const updated = withRemoteEntry(mounted, entry("wb-a")); assert.deepEqual(updated, { kind: "api", reviewId: "r1", host: "wb-a", available }); assert.equal(withRemoteEntry(updated!, entry("wb-a")), undefined); + + assert.deepEqual(withRemoteEntry(updated!, { ...entry("wb-a"), hostState: "offline" }), { ...updated, hostState: "offline" }); }); test("a laptop review's entry changes nothing", () => { diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.ts index af1d21611..9ca0aee1a 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.ts +++ b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewRemoteEntry.ts @@ -1,12 +1,12 @@ import type { ReviewApiSummary } from "../../../common/reviewProtocol.js"; -type RemoteFields = Pick; +type RemoteFields = Pick; export const remoteEntry = (review: ReviewApiSummary | undefined): RemoteFields => - review?.host ? { host: review.host, available: review.available } : {}; + review?.host ? { host: review.host, ...(review.hostState && { hostState: review.hostState }), available: review.available } : {}; export function withRemoteEntry(content: T, review: ReviewApiSummary | undefined): T | undefined { const entry = remoteEntry(review); - if (!entry.host || (entry.host === content.host && JSON.stringify(entry.available) === JSON.stringify(content.available))) return undefined; + if (!entry.host || (entry.host === content.host && entry.hostState === content.hostState && JSON.stringify(entry.available) === JSON.stringify(content.available))) return undefined; return { ...content, ...entry }; } diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.test.ts index 88c5e98c7..e50a3c8ca 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.test.ts @@ -381,7 +381,7 @@ test("a cancelled prompt is auth-failed, and there is no second attempt", async await until(() => last()?.problem !== undefined); assert.equal(last()?.problem?.state, "auth-failed"); - assert.equal(last()!.problem!.detail, "dev@127.0.0.1: Permission denied (publickey,password)."); + assert.equal(last()!.problem!.detail, `wb-test-a did not accept your SSH key. Check that "ssh wb-test-a" works in a terminal.`); assert.equal(clock.pending, 0); assert.equal(ssh.of("wb-test-a", "master").length, 1); }); @@ -404,7 +404,7 @@ test("the master exits and the host reconnects after the backoff", async (t) => await until(() => last()?.problem !== undefined); assert.equal(last()?.problem?.state, "unreachable"); - assert.match(last()!.problem!.detail, /closed by remote host/); + assert.equal(last()!.problem!.detail, "wb-test-a closed the SSH connection."); assert.equal(ssh.of("wb-test-a", "master").length, 1); assert.ok(clock.next()); await until(() => last()?.endpoint !== undefined); @@ -488,7 +488,7 @@ test("an authenticated master that ends is unreachable and retried, whatever its await until(() => last()?.problem !== undefined); assert.equal(last()?.problem?.state, "unreachable"); - assert.equal(last()!.problem!.detail, "The SSH connection to wb-test-a ended: Connection reset by peer"); + assert.equal(last()!.problem!.detail, "wb-test-a closed the SSH connection."); assert.equal(clock.pending, 1); }); @@ -724,18 +724,17 @@ test("the version present: no prompt and no install shown; the installed CLI att ); }); -test("the version present with another sha256 is not installed: the user is asked and the install is shown", async (t) => { +test("the version present with another sha256 is reinstalled without asking, and the install is shown", async (t) => { const port = await healthServer(t); - const { flow, prompts, runs } = await installFlow(t, "ask", { answers: [true], steps: STEPS }); + const { flow, prompts, runs } = await installFlow(t, "ask", { steps: STEPS }); const { host, reports, last } = hostFor(t, { probe: { installed: [at("0.1.5"), at("0.1.6", "b".repeat(64))] } }, port, "wb-test-a", "/tmp/wb-ssh-test", flow); host.start(); await until(() => last()?.endpoint !== undefined); - assert.equal(prompts.length, 1); + assert.equal(prompts.length, 0); assert.equal(runs.length, 1); assert.deepEqual(reports.find((report) => report.installing)?.installing, { step: "preparing" }); - assert.equal(await flow.consent.get("wb-test-a"), "allow"); }); test("the version absent with installs always: each step is reported, then the host attaches", async (t) => { diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts index 66cca15b0..b6b4075aa 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHost.ts @@ -83,6 +83,28 @@ export function classifySshFailure(stderr: string, promptCancelled: boolean): "a : "unreachable"; } +const SSH_SENTENCES: [RegExp, (alias: string) => string][] = [ + [/REMOTE HOST IDENTIFICATION HAS CHANGED/, (alias) => `${alias}'s host key changed since you last connected. Verify the new fingerprint before replacing its old key in known_hosts.`], + [/Host key verification failed/, (alias) => `${alias}'s host key could not be verified. Run "ssh ${alias}" in a terminal and verify its fingerprint before accepting it.`], + [/Permission denied/, (alias) => `${alias} did not accept your SSH key. Check that "ssh ${alias}" works in a terminal.`], + [/Could not resolve hostname/, (alias) => `${alias}'s host name could not be found. Check its address in your SSH config.`], + [/Connection refused/, (alias) => `${alias} refused the SSH connection. Check that its SSH server is running.`], + [/(Operation|Connection) timed out/, (alias) => `The SSH connection to ${alias} timed out. Check that it is on and reachable from this network.`], +]; + +function sshFailureSentence(stderr: string, alias: string): string | undefined { + return SSH_SENTENCES.find(([pattern]) => pattern.test(stderr))?.[1](alias); +} + +const SSH_DROPS: [RegExp, (alias: string) => string][] = [ + [/Timeout, server .* not responding/, (alias) => `${alias} stopped answering over SSH. It may be asleep, or off this network.`], + [/Connection reset|Broken pipe|closed by remote host/, (alias) => `${alias} closed the SSH connection.`], +]; + +function sshDropSentence(lastLines: string, alias: string): string | undefined { + return SSH_DROPS.find(([pattern]) => pattern.test(lastLines))?.[1](alias); +} + export const OPENSSH_NEEDED = "OpenSSH is needed: no `ssh` command was found on PATH."; type Problem = NonNullable; @@ -557,14 +579,17 @@ export class ReviewRemoteHost { if (error?.code === "ENOENT") return this.fail({ state: "unreachable", detail: OPENSSH_NEEDED }); const exited = error?.message || `ssh exited with code ${code ?? "none"}.`; if (this.connectedAt !== undefined) { + const last = lastLines(this.masterStderr); return this.fail({ state: "unreachable", - detail: `The SSH connection to ${this.alias} ended: ${lastLines(this.masterStderr) || exited}`, + detail: sshDropSentence(last, this.alias) ?? `The SSH connection to ${this.alias} ended: ${last || exited}`, }); } + const sentence = sshFailureSentence(this.masterStderr, this.alias); + if (sentence) this.options.log(`${this.alias}: ${exited} ${firstLines(this.masterStderr)}`); this.fail({ state: classifySshFailure(this.masterStderr, this.promptCancelled), - detail: firstLines(this.masterStderr) || exited, + detail: sentence ?? (firstLines(this.masterStderr) || exited), }); } @@ -613,7 +638,9 @@ export class ReviewRemoteHost { if (artifact instanceof ReviewRemoteNoTarball) return this.cannotInstall(probe, version, noTarballText(this.alias)); if (artifact instanceof Error) return this.cannotInstall(probe, version, installFailureText(this.alias, version, "preparing", artifact.message)); if (!present && !probe.releasesReachable) return this.cannotInstall(probe, version, `${this.alias} cannot reach ${origin}.`); - if (!present && mode === "ask" && !(await this.agreed(flow, probe, version, stale))) { + // Like VS Code's server, Desktop updates its own installs without asking; a CLI the user installed is theirs. + const upgrade = probe.installed.length > 0; + if (!present && !upgrade && mode === "ask" && !(await this.agreed(flow, probe, version, stale))) { this.declined = true; return stale() ? undefined : onPath; } diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.test.ts index 195dcc3e3..10595f9ef 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.test.ts @@ -12,7 +12,7 @@ import test from "node:test"; import type { ReviewGatewayHost } from "../../common/reviewProtocol.js"; import { attachOutput, COMMIT, detectOutput, FAKE_SERVER_ID, fakeClock, fakeSsh, flush, healthServer, until, type FakeRemote } from "./test/fakeSsh.js"; import type { ReviewRemoteInstallFlow } from "./reviewRemoteHost.js"; -import { ReviewRemoteHosts } from "./reviewRemoteHosts.js"; +import { closeRemoteHostWindows, ReviewRemoteHosts, type ReviewRemoteHostsOptions } from "./reviewRemoteHosts.js"; import { openRemoteInstallConsent } from "./reviewRemoteInstallConsent.js"; import type { SshPromptRequest } from "./reviewSshAskpass.js"; import { reviewSshInstancePrefix } from "./reviewSshCommand.js"; @@ -23,6 +23,7 @@ async function managerFor( answer?: string, before?: (directory: string) => Promise, install?: ReviewRemoteInstallFlow, + removed?: ReviewRemoteHostsOptions["removed"], ) { const port = await healthServer(t); const directory = await mkdtemp(join(tmpdir(), "wb-hosts-")); @@ -48,6 +49,7 @@ async function managerFor( clock, timeouts: { poll: 1 }, install, + removed, }); t.after(async () => { await manager.dispose(); @@ -98,6 +100,49 @@ test("removing a host from the setting closes its connection", async (t) => { assert.ok(ssh.master("wb-test-b")!.alive); }); +test("removing a machine's last alias closes the windows bound to it, and no other window", async (t) => { + const closed: string[] = []; + const windows = [`whiteboard+${FAKE_SERVER_ID}`, "whiteboard+wb-test-b-server", "ssh-remote+x", undefined].map((remoteAuthority) => ({ + remoteAuthority, + close: () => closed.push(String(remoteAuthority)), + })); + const b = { attach: { code: 0, stdout: attachOutput(41234, "remote-token", { serverId: "wb-test-b-server" }) } }; + const { manager, sentUntil } = await managerFor(t, { "wb-test-a": {}, "wb-test-a2": {}, "wb-test-b": b }, undefined, undefined, undefined, (serverId) => + closeRemoteHostWindows({ getWindows: () => windows }, serverId), + ); + + manager.update(true, ["wb-test-a", "wb-test-a2", "wb-test-b"]); + await sentUntil((hosts) => hosts.length === 3 && hosts.every((h) => h.endpoint)); + manager.update(true, ["wb-test-a", "wb-test-b"]); + assert.deepEqual(closed, []); + manager.update(true, ["wb-test-b"]); + assert.deepEqual(closed, [`whiteboard+${FAKE_SERVER_ID}`]); +}); + +test("a machine whose entry lost its id while unreachable is known through its alias until that alias leaves the setting", async (t) => { + const { manager, sentUntil } = await managerFor(t, { "wb-test-a": {} }); + manager.update(true, ["wb-test-a"]); + await sentUntil((hosts) => byAlias(hosts, "wb-test-a")?.endpoint !== undefined); + + const unreachable = [{ alias: "wb-test-a", state: "unreachable" as const }]; + assert.deepEqual(manager.hostState(FAKE_SERVER_ID, unreachable), { alias: "wb-test-a", state: "unreachable" }); + assert.deepEqual(manager.hostState(FAKE_SERVER_ID, []), { alias: "wb-test-a", state: "connecting" }); + assert.equal(manager.hostState("other-machine", unreachable), undefined); + + manager.update(true, []); + assert.equal(manager.hostState(FAKE_SERVER_ID, unreachable), undefined); +}); + +test("after a restart, a window's last alias gives its machine's state until that alias attaches", async (t) => { + const { manager } = await managerFor(t, { "wb-test-a": { master: "hang" } }); + manager.update(true, ["wb-test-a"]); + + const unreachable = [{ alias: "wb-test-a", state: "unreachable" as const }]; + assert.deepEqual(manager.hostState(FAKE_SERVER_ID, unreachable, "wb-test-a"), { alias: "wb-test-a", state: "unreachable" }); + assert.equal(manager.hostState(FAKE_SERVER_ID, unreachable), undefined); + assert.equal(manager.hostState(FAKE_SERVER_ID, unreachable, "gone"), undefined); +}); + test("an alias added again while its old host closes starts only after the old master exited", async (t) => { const { manager, ssh, sentUntil } = await managerFor(t, { "wb-test-a": { exitDelayMs: 150 } }); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts index d2fabf456..97f788f4a 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts @@ -18,6 +18,7 @@ import { type SpawnSsh, } from "./reviewRemoteHost.js"; import type { ReviewSshAskpass, SshPromptRequest } from "./reviewSshAskpass.js"; +import { reviewRemoteAuthority } from "../../services/remote/reviewRemoteAuthority.js"; import { uninstallRemote } from "./reviewRemoteUninstall.js"; import { prepareSshControlDirectory, @@ -48,6 +49,26 @@ export interface ReviewRemoteHostsOptions { readonly clock?: ReviewRemoteClock; readonly timeouts?: ReviewRemoteHostOptions["timeouts"]; readonly install?: ReviewRemoteInstallFlow; + /** No host in the setting serves this machine any more. */ + removed?(serverId: string): void; +} + +/** The gateway's state of the machine `serverId`; `configured` is the alias that served it. */ +export function remoteHostState( + serverId: string, + states: readonly ReviewGatewayHostState[], + configured?: string, +): ({ alias?: string } & Pick) | undefined { + const own = states.filter((state) => state.serverId === serverId); + const best = own.find((state) => state.state === "online") ?? own.find((state) => state.state === "connecting") ?? own[0]; + if (best) return best; + if (configured) return states.find((state) => state.alias === configured) ?? { alias: configured, state: "connecting" }; + return states.some((state) => state.serverId === undefined && state.state === "connecting") ? { state: "connecting" } : undefined; +} + +export function closeRemoteHostWindows(windows: { getWindows(): readonly { readonly remoteAuthority?: string; close(): void }[] }, serverId: string): void { + const authority = reviewRemoteAuthority(serverId); + if (authority) for (const window of windows.getWindows()) if (window.remoteAuthority === authority) window.close(); } export function freeLoopbackPort(): Promise { @@ -110,6 +131,7 @@ export class ReviewRemoteHosts { this.hosts.delete(alias); this.options.log(`${alias}: removed from the setting; closing its connection.`); this.closing.set(host, host.dispose().finally(() => this.closing.delete(host))); + this.forget(host); } this.order = wanted; this.refused.clear(); @@ -183,6 +205,7 @@ export class ReviewRemoteHosts { if (host && this.hosts.get(alias) === host) { this.hosts.delete(alias); this.removed.add(alias); + this.forget(host); this.publish(); } if (!host) return; @@ -203,6 +226,13 @@ export class ReviewRemoteHosts { return online && this.hosts.get(online.alias)?.languageEndpoint(serverId); } + /** The state of machine `serverId`, through the alias that served it, or `last` until that alias attaches. */ + hostState(serverId: string, states: readonly ReviewGatewayHostState[], last?: string): ReturnType { + const configured = [...this.hosts.values()].find((host) => host.serverId === serverId)?.state.alias + ?? (last !== undefined && this.hosts.has(last) && this.hosts.get(last)?.serverId === undefined ? last : undefined); + return remoteHostState(serverId, states, configured); + } + resume(): void { for (const host of this.hosts.values()) void host.resume(); } @@ -239,6 +269,11 @@ export class ReviewRemoteHosts { }); } + private forget(host: ReviewRemoteHost): void { + const { serverId } = host; + if (serverId && ![...this.hosts.values()].some((other) => other.serverId === serverId)) this.options.removed?.(serverId); + } + private askpass(): Promise { return (this.prepared ??= (async () => { await prepareSshControlDirectory(this.options.controlDirectory); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.test.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.test.ts index e2b6159d8..a2876dea8 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.test.ts @@ -14,7 +14,7 @@ import test from "node:test"; import type { SpawnSsh } from "./reviewRemoteHost.js"; import { probeRemote } from "./reviewRemoteProbe.js"; import { uninstallRemote } from "./reviewRemoteUninstall.js"; -import { compareVersions, installRemote, type ReviewRemoteInstallInput, type ReviewRemoteInstallProgress } from "./reviewRemoteInstaller.js"; +import { installRemote, type ReviewRemoteInstallInput, type ReviewRemoteInstallProgress } from "./reviewRemoteInstaller.js"; import { cleanupScript, lockScript, REVIEW_REMOTE_INSTALL_MARKER, REVIEW_REMOTE_WRAPPER_MARK, shellQuote } from "./reviewRemoteInstallScript.js"; import type { ReviewRemoteProbe } from "./reviewRemoteProbe.js"; import { reviewSshSession } from "./reviewSshCommand.js"; @@ -430,11 +430,6 @@ test("shellQuote survives quotes and refuses control characters", async () => { assert.throws(() => shellQuote("a\u0007b"), /control character/); }); -test("versions order as semver", () => { - const sorted = ["0.1.10", "0.1.7", "0.1.7-preview.20261003.10", "0.1.6", "0.1.7-preview.20261003.2", "0.2.0-preview.1"].sort(compareVersions); - assert.deepEqual(sorted, ["0.1.6", "0.1.7-preview.20261003.2", "0.1.7-preview.20261003.10", "0.1.7", "0.1.10", "0.2.0-preview.1"]); -}); - test("a lock taken over between the stealer's two reads is left to its new holder", async (t) => { const f = await fixture(t); const lock = join(f.remoteRoot, "install.lock"); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.ts index eb27645ca..98987249e 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteInstaller.ts @@ -6,6 +6,8 @@ import { randomBytes } from "node:crypto"; import { hostname } from "node:os"; +import * as semver from "../../../base/common/semver/semver.js"; + import type { ReviewRemoteArtifact } from "./reviewRemoteArtifacts.js"; import { lastLine, runSsh, type RunResult, type SpawnSsh, type SshChildProcess } from "./reviewRemoteHost.js"; import { @@ -169,7 +171,7 @@ export async function installRemote(input: ReviewRemoteInstallInput): Promise REVIEW_REMOTE_VERSION.test(name)); - const newest = [input.version, ...have].sort(compareVersions).at(-1); + const newest = [input.version, ...have].sort(semver.compare).at(-1); const finished = await run( "finishing", finishScript(context, { @@ -181,7 +183,7 @@ export async function installRemote(input: ReviewRemoteInstallInput): Promise !keep.has(name)).sort(compareVersions).reverse(); + const candidates = have.filter((name) => !keep.has(name)).sort(semver.rcompare); if (candidates.length) { await run("removing old versions", cleanupScript(context, { candidates, room: 2 - keep.size })).catch((error: unknown) => { if (signal.aborted) throw error; @@ -239,31 +241,6 @@ function parseVersion(text: string | undefined): string | undefined { } } -export function compareVersions(a: string, b: string): number { - const [coreA, preA] = split(a); - const [coreB, preB] = split(b); - for (let i = 0; i < 3; i++) if (coreA[i] !== coreB[i]) return coreA[i] - coreB[i]; - if (!preA.length || !preB.length) return preB.length - preA.length; - for (let i = 0; i < Math.max(preA.length, preB.length); i++) { - const x = preA[i]; - const y = preB[i]; - if (x === undefined || y === undefined) return x === undefined ? -1 : 1; - if (x === y) continue; - const nx = /^\d+$/.test(x); - const ny = /^\d+$/.test(y); - if (nx && ny) return Number(x) - Number(y); - if (nx !== ny) return nx ? -1 : 1; - return x < y ? -1 : 1; - } - return 0; -} - -function split(version: string): [number[], string[]] { - const dash = version.indexOf("-"); - const core = (dash < 0 ? version : version.slice(0, dash)).split(".").map(Number); - return [core, dash < 0 ? [] : version.slice(dash + 1).split(".")]; -} - function delay(ms: number, signal: AbortSignal): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => { diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteUninstall.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteUninstall.ts index ed35cb9db..d86f1b780 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteUninstall.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteUninstall.ts @@ -3,9 +3,9 @@ * Licensed under the MIT License. See LICENSE in the repository root for license information. *--------------------------------------------------------------------------------------------*/ +import * as semver from "../../../base/common/semver/semver.js"; import { plainText } from "./reviewRemoteAgents.js"; import { lastLine, runSsh, type SpawnSsh } from "./reviewRemoteHost.js"; -import { compareVersions } from "./reviewRemoteInstaller.js"; import { REVIEW_REMOTE_INSTALL_MARKER, REVIEW_REMOTE_INSTALL_SAY, REVIEW_REMOTE_ROOT_SCRIPT, REVIEW_REMOTE_VERSION, shellQuote } from "./reviewRemoteInstallScript.js"; import { sshExecArgs, type ReviewSshSession } from "./reviewSshCommand.js"; @@ -47,7 +47,7 @@ export async function uninstallRemote(input: { if (!root?.startsWith("/")) throw failed("DEV_REVIEW_HOME there is not an absolute, normalised path."); const newest = said("HAVE") .filter((name) => REVIEW_REMOTE_VERSION.test(name)) - .sort(compareVersions) + .sort(semver.compare) .at(-1); if (!newest) throw failed("Whiteboard Desktop installed nothing there."); diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopChannel.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopChannel.ts index a538b0703..69ea22d73 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopChannel.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopChannel.ts @@ -57,6 +57,11 @@ export class ReviewDesktopChannel implements IServerChannel { if (typeof arg !== "string") return undefined as T; return (await this.host.getRemoteLanguageEndpoint(arg)) as T; } + if (command === "getRemoteHostState") { + const [serverId, last] = Array.isArray(arg) ? arg : [arg]; + if (typeof serverId !== "string") return undefined as T; + return (await this.host.getRemoteHostState(serverId, typeof last === "string" ? last : undefined)) as T; + } if (command === "retryRemoteHost") { if (typeof arg === "string") this.host.retryRemoteHost(arg); return undefined as T; diff --git a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts index fa0ad8c39..47fe9b11d 100644 --- a/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts +++ b/apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts @@ -9,6 +9,7 @@ import { Disposable, toDisposable } from "../../base/common/lifecycle.js"; import { join } from "../../base/common/path.js"; import { IConfigurationService } from "../../platform/configuration/common/configuration.js"; import { IEnvironmentMainService } from "../../platform/environment/electron-main/environmentMainService.js"; +import { IInstantiationService } from "../../platform/instantiation/common/instantiation.js"; import { ILifecycleMainService, LifecycleMainPhase, @@ -20,6 +21,7 @@ import { IApplicationStorageMainService } from "../../platform/storage/electron- import { NullTelemetryService } from "../../platform/telemetry/common/telemetryUtils.js"; import { IUpdateService } from "../../platform/update/common/update.js"; import { UtilityProcess } from "../../platform/utilityProcess/electron-main/utilityProcess.js"; +import { IWindowsMainService } from "../../platform/windows/electron-main/windows.js"; import type { ReviewDesktopConnection } from "../common/reviewDesktopBootstrap.js"; import type { ReviewGatewayHostState, @@ -52,7 +54,7 @@ import type { ReviewRemoteInstallFlow, ReviewRemoteInstallMode, } from "./remote/reviewRemoteHost.js"; -import { ReviewRemoteHosts } from "./remote/reviewRemoteHosts.js"; +import { closeRemoteHostWindows, ReviewRemoteHosts } from "./remote/reviewRemoteHosts.js"; import { openRemoteInstallConsent, reviewRemoteInstallConsentPath, @@ -100,6 +102,8 @@ export class ReviewDesktopHost extends Disposable { @IUpdateService private readonly updateService: IUpdateService, @IApplicationStorageMainService private readonly applicationStorageMainService: IApplicationStorageMainService, + @IInstantiationService + private readonly instantiationService: IInstantiationService, ) { super(); let resolvedEnvironment: Promise | undefined; @@ -255,17 +259,23 @@ export class ReviewDesktopHost extends Disposable { async getRemoteLanguageEndpoint(serverId: string) { const manager = this.remoteHosts; - if (!manager) return undefined; + const states = manager && (await this.remoteHostStates()); + return states && manager.languageEndpoint(serverId, states).catch(() => undefined); + } + + async getRemoteHostState(serverId: string, last?: string) { + const manager = this.remoteHosts; + const states = manager && (await this.remoteHostStates()); + return states && manager.hostState(serverId, states, last); + } + + private async remoteHostStates(): Promise { try { const { url, token } = await this.whenConnected(); const response = await fetch(new URL("/remote-hosts", url), { headers: { "x-review-token": token }, }); - if (!response.ok) return undefined; - return await manager.languageEndpoint( - serverId, - (await response.json()) as ReviewGatewayHostState[], - ); + return response.ok ? ((await response.json()) as ReviewGatewayHostState[]) : undefined; } catch { return undefined; } @@ -354,6 +364,10 @@ export class ReviewDesktopHost extends Disposable { send: (hosts) => this.supervisor.setRemoteHosts(hosts), log: (message) => this.logService.info(`[Remote hosts] ${message}`), install: this.remoteInstallFlow(), + removed: (serverId) => + this.instantiationService.invokeFunction((accessor) => + closeRemoteHostWindows(accessor.get(IWindowsMainService), serverId), + ), }); this.remoteHosts = manager; const update = () => { diff --git a/apps/review-desktop/code-oss/src/vs/review/navigator.desktop.main.ts b/apps/review-desktop/code-oss/src/vs/review/navigator.desktop.main.ts index d4bbff646..2fd719366 100644 --- a/apps/review-desktop/code-oss/src/vs/review/navigator.desktop.main.ts +++ b/apps/review-desktop/code-oss/src/vs/review/navigator.desktop.main.ts @@ -47,6 +47,20 @@ import { ILanguageFeaturesService } from '../editor/common/services/languageFeat import { SymbolNavigationAnchor } from '../editor/contrib/gotoSymbol/browser/goToCommands.js'; import { CommandsRegistry, ICommandService } from '../platform/commands/common/commands.js'; import { IEditorService } from '../workbench/services/editor/common/editorService.js'; +import { NativeExtensionService } from '../workbench/services/extensions/electron-browser/nativeExtensionService.js'; +import { IExtensionService } from '../workbench/services/extensions/common/extensions.js'; +import { IRemoteAuthorityResolverService, type ResolverResult } from '../platform/remote/common/remoteAuthorityResolver.js'; +import { IMainProcessService } from '../platform/ipc/common/mainProcessService.js'; +import { REVIEW_DESKTOP_CHANNEL } from './common/reviewDesktopBootstrap.js'; +import { isReviewRemoteAuthority, reviewRemoteServerId } from './services/remote/reviewRemoteAuthority.js'; +import { reloadWhenOnline, reviewWindowAuthorityResolver, reviewWindowHosts } from './services/remote/reviewWindowAuthorityResolver.js'; +import { ReviewSourceWindowHostState } from './services/remote/reviewSourceWindowHostState.js'; +import { IHostService } from '../workbench/services/host/browser/host.js'; +import { FilesConfigurationService, IFilesConfigurationService } from '../workbench/services/filesConfiguration/common/filesConfigurationService.js'; +import type { IBaseFileStat } from '../platform/files/common/files.js'; +import { isEqual } from '../base/common/resources.js'; +import type { IMarkdownString } from '../base/common/htmlContent.js'; +import type { IWorkspaceContextService } from '../platform/workspace/common/workspace.js'; class NavigatorDefaults { constructor(@IStorageService storage: IStorageService) { @@ -59,6 +73,46 @@ class NavigatorDefaults { } registerWorkbenchContribution2('review.navigator.defaults', NavigatorDefaults, WorkbenchPhase.BlockStartup); +registerWorkbenchContribution2(ReviewSourceWindowHostState.ID, ReviewSourceWindowHostState, WorkbenchPhase.BlockRestore); + +class NavigatorExtensionService extends NativeExtensionService { + private readonly windowHosts = this._instantiationService.invokeFunction((accessor) => reviewWindowHosts(accessor.get(IMainProcessService).getChannel(REVIEW_DESKTOP_CHANNEL))); + protected override readonly _remoteAuthorityResolverService = this._instantiationService.invokeFunction((accessor) => + reviewWindowAuthorityResolver(accessor.get(IRemoteAuthorityResolverService), this.windowHosts)); + private retrying = false; + + protected override async _resolveAuthority(remoteAuthority: string): Promise { + if (!isReviewRemoteAuthority(remoteAuthority)) return super._resolveAuthority(remoteAuthority); + try { + return await this._remoteAuthorityResolverService.resolveAuthority(remoteAuthority); + } catch (error) { + if (!this.retrying) { + this.retrying = true; + const host = this._instantiationService.invokeFunction((accessor) => accessor.get(IHostService)); + this._register(reloadWhenOnline(this.windowHosts, reviewRemoteServerId(remoteAuthority), () => void host.reload())); + } + throw error; + } + } +} + +registerSingleton(IExtensionService, NavigatorExtensionService, InstantiationType.Eager); + +/** Every file in a Source window's folders is read-only, whatever a folder's settings say. */ +class NavigatorFilesConfigurationService extends FilesConfigurationService { + private readonly workspace: IWorkspaceContextService; + + constructor(...args: ConstructorParameters) { + super(...args); + this.workspace = args[2]; + } + + override isReadonly(resource: URI, stat?: IBaseFileStat): boolean | IMarkdownString { + return super.isReadonly(resource, stat) || (!!this.workspace.getWorkspaceFolder(resource) && !isEqual(resource, this.workspace.getWorkspace().configuration ?? undefined)); + } +} + +registerSingleton(IFilesConfigurationService, NavigatorFilesConfigurationService, InstantiationType.Eager); CommandsRegistry.registerCommand('review.action.showReferencesInSource', async (accessor, resource: string, lineNumber: number, column: number) => { const editorService = accessor.get(IEditorService); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewRemoteAuthority.ts b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewRemoteAuthority.ts index ca32e269b..c2223b36f 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewRemoteAuthority.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewRemoteAuthority.ts @@ -6,14 +6,26 @@ import { Schemas } from "../../../base/common/network.js"; import type { URI } from "../../../base/common/uri.js"; +const PREFIX = "whiteboard+"; + export function reviewRemoteAuthority(serverId: string): string | undefined { - return /^[0-9a-z-]+$/i.test(serverId) ? `whiteboard+${serverId.toLowerCase()}` : undefined; + return /^[0-9a-z-]+$/i.test(serverId) ? `${PREFIX}${serverId.toLowerCase()}` : undefined; +} + +/** The server id in a `reviewRemoteAuthority`. */ +export function reviewRemoteServerId(authority: string): string { + return authority.slice(PREFIX.length); } export function ownsRemoteResource(authority: string, resource: URI): boolean { return resource.scheme === Schemas.vscodeRemote && resource.authority.toLowerCase() === authority; } +/** Exactly the lower-case `whiteboard+` that `reviewRemoteAuthority` makes. */ +export function isReviewRemoteAuthority(authority: string | null | undefined): authority is string { + return !!authority && /^whiteboard\+[0-9a-z-]+$/.test(authority); +} + export function override(base: T, members: Partial): T { return new Proxy(base, { get(target, key) { diff --git a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.test.ts new file mode 100644 index 000000000..898f94f00 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.test.ts @@ -0,0 +1,159 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from "node:assert/strict"; +import test from "node:test"; +import { Emitter, Event } from "../../../base/common/event.js"; +import { CommandsRegistry } from "../../../platform/commands/common/commands.js"; +import type { IMainProcessService } from "../../../platform/ipc/common/mainProcessService.js"; +import type { INotification, INotificationActions, INotificationService } from "../../../platform/notification/common/notification.js"; +import { PersistentConnectionEventType, type PersistentConnectionEvent } from "../../../platform/remote/common/remoteAgentConnection.js"; +import type { IStorageService } from "../../../platform/storage/common/storage.js"; +import type { IWorkbenchEnvironmentService } from "../../../workbench/services/environment/common/environmentService.js"; +import type { IRemoteAgentConnection, IRemoteAgentService } from "../../../workbench/services/remote/common/remoteAgentService.js"; +import type { IStatusbarEntry, IStatusbarService } from "../../../workbench/services/statusbar/browser/statusbar.js"; +import { RETRY_HOST_COMMAND, ReviewSourceWindowHostState, showSourceWindowHostState } from "./reviewSourceWindowHostState.js"; +import type { ReviewWindowHosts } from "./reviewWindowAuthorityResolver.js"; + +const OFFLINE = "devbox offline"; + +function fakes() { + const entries: IStatusbarEntry[] = []; + const shown: INotification[] = []; + const retried: string[] = []; + let closed = 0; + const statusbar = { + addEntry: (entry: IStatusbarEntry) => { + entries.push(entry); + return { update: (next: IStatusbarEntry) => void entries.push(next), dispose: () => {} }; + }, + } as unknown as IStatusbarService; + const notifications = { + notify: (notification: INotification) => { + const shownAt = shown.push({ ...notification }) - 1; + return { + onDidClose: Event.None, + close: () => void closed++, + updateMessage: (message: string) => void (shown[shownAt] = { ...shown[shownAt], message }), + updateActions: (actions: INotificationActions) => void (shown[shownAt] = { ...shown[shownAt], actions }), + }; + }, + } as unknown as INotificationService; + let state: { state: string; detail?: string } = { state: "online" }; + let polls = 0; + const hosts = { endpoint: async () => undefined, state: async () => (polls++, { alias: "devbox", ...state }) } as unknown as ReviewWindowHosts; + const connection = new Emitter(); + return { + statusbar, + notifications, + hosts, + connection: { onDidStateChange: connection.event } as unknown as IRemoteAgentConnection, + retry: (alias: string) => void retried.push(alias), + fire: (type: PersistentConnectionEventType) => connection.fire({ type } as PersistentConnectionEvent), + set: (next: string, detail?: string) => void (state = { state: next, detail }), + polls: () => polls, + last: () => entries.at(-1), + shown, + retried, + closed: () => closed, + }; +} + +async function until(check: () => boolean) { + while (!check()) await new Promise((resolve) => setTimeout(resolve, 1)); +} +const labels = (notification: INotification | undefined) => notification?.actions?.primary?.map((action) => action.label); + +test("shows the alias while online, the offline state within one poll, and restores it", async () => { + const f = fakes(); + const watch = showSourceWindowHostState("abc-1", f.hosts, f.statusbar, f.notifications, f.connection, f.retry, undefined, 5); + try { + await until(() => f.last()?.text === "devbox"); + assert.notEqual(f.last()?.kind, "warning"); + assert.equal(f.last()?.command, undefined); + assert.equal(f.shown.length, 0); + + f.set("offline"); + await until(() => f.last()?.text === OFFLINE); + assert.equal(f.last()?.kind, "warning"); + assert.equal(f.last()?.command, RETRY_HOST_COMMAND); + assert.deepEqual(f.shown.map((n) => [n.message, labels(n)]), [["devbox offline.", ["Retry"]]]); + + f.fire(PersistentConnectionEventType.ConnectionLost); + f.set("online"); + const polls = f.polls(); + await until(() => f.polls() >= polls + 2); + assert.equal(f.last()?.text, OFFLINE, "the window's own connection is still down"); + assert.equal(f.closed(), 0); + + f.fire(PersistentConnectionEventType.ConnectionGain); + assert.equal(f.last()?.text, "devbox"); + assert.notEqual(f.last()?.kind, "warning"); + assert.equal(f.closed(), 1); + + f.set("unreachable", "ssh: connect to host devbox port 22: Connection refused"); + await until(() => f.shown.length === 2); + assert.equal(f.last()?.text, OFFLINE); + assert.deepEqual(f.shown.map((n) => n.message), ["devbox offline.", "ssh: connect to host devbox port 22: Connection refused"], "a later outage notifies again"); + } finally { + watch.dispose(); + } +}); + +test("Retry in the notice and the status bar retries the host", async () => { + const f = fakes(); + f.set("offline"); + const watch = showSourceWindowHostState("abc-1", f.hosts, f.statusbar, f.notifications, f.connection, f.retry, undefined, 5); + try { + await until(() => f.shown.length === 1); + await f.shown[0]?.actions?.primary?.[0]?.run(); + await CommandsRegistry.getCommand(RETRY_HOST_COMMAND)?.handler(undefined as never); + assert.deepEqual(f.retried, ["devbox", "devbox"]); + } finally { + watch.dispose(); + } +}); + +test("a host that refuses its sign-in is named so, with no Retry, and the open notice follows the state", async () => { + const f = fakes(); + f.set("offline"); + const watch = showSourceWindowHostState("abc-1", f.hosts, f.statusbar, f.notifications, f.connection, f.retry, undefined, 5); + try { + await until(() => f.last()?.text === OFFLINE); + f.set("auth-failed", "Permission denied (publickey)."); + await until(() => f.last()?.text === "Can't sign in to devbox"); + assert.equal(f.last()?.command, undefined); + assert.deepEqual(f.shown.map((n) => [n.message, labels(n)]), [["Permission denied (publickey).", []]]); + } finally { + watch.dispose(); + } +}); + +test("a window restored before main knows its host's alias names it by the alias it stored when it opened", async () => { + const stored = new Map(); + const storage = { get: (key: string) => stored.get(key), store: (key: string, value: string) => stored.set(key, value) } as unknown as IStorageService; + const open = (state: object) => { + const f = fakes(); + const contribution = new ReviewSourceWindowHostState( + { remoteAuthority: "whiteboard+3f454168-aaaa" } as IWorkbenchEnvironmentService, + { getChannel: () => ({ call: async (command: string) => (command === "getRemoteHostState" ? state : undefined) }) } as unknown as IMainProcessService, + f.statusbar, + f.notifications, + { getConnection: () => null } as unknown as IRemoteAgentService, + storage, + ); + return { f, contribution }; + }; + const first = open({ alias: "wb-test-a", state: "online" }); + await until(() => first.f.last()?.text === "wb-test-a"); + first.contribution.dispose(); + const { f, contribution } = open({ state: "offline" }); + try { + await until(() => f.last()?.text === "wb-test-a offline"); + assert.equal(f.shown[0]?.message, "wb-test-a offline."); + } finally { + contribution.dispose(); + } +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.ts b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.ts new file mode 100644 index 000000000..7c3ba0603 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewSourceWindowHostState.ts @@ -0,0 +1,134 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { toAction } from "../../../base/common/actions.js"; +import { Event } from "../../../base/common/event.js"; +import { Disposable, DisposableStore, toDisposable, type IDisposable } from "../../../base/common/lifecycle.js"; +import Severity from "../../../base/common/severity.js"; +import { CommandsRegistry } from "../../../platform/commands/common/commands.js"; +import { IMainProcessService } from "../../../platform/ipc/common/mainProcessService.js"; +import { INotificationService, type INotificationHandle } from "../../../platform/notification/common/notification.js"; +import { PersistentConnectionEventType } from "../../../platform/remote/common/remoteAgentConnection.js"; +import type { IWorkbenchContribution } from "../../../workbench/common/contributions.js"; +import { IWorkbenchEnvironmentService } from "../../../workbench/services/environment/common/environmentService.js"; +import { IRemoteAgentService, type IRemoteAgentConnection } from "../../../workbench/services/remote/common/remoteAgentService.js"; +import { IStatusbarService, StatusbarAlignment, type IStatusbarEntry, type IStatusbarEntryAccessor } from "../../../workbench/services/statusbar/browser/statusbar.js"; +import { IStorageService, StorageScope, StorageTarget } from "../../../platform/storage/common/storage.js"; +import { reviewHostStatus, type ReviewGatewayHostState } from "../../common/reviewProtocol.js"; +import { REVIEW_DESKTOP_CHANNEL } from "../../common/reviewDesktopBootstrap.js"; +import { isReviewRemoteAuthority, reviewRemoteServerId } from "./reviewRemoteAuthority.js"; +import { reviewWindowHosts, type ReviewWindowHosts } from "./reviewWindowAuthorityResolver.js"; + +export const HOST_STATE_POLL_MS = 2_000; + +export const RETRY_HOST_COMMAND = "review.sourceWindow.retryHost"; + +const HOST_ALIAS_KEY = "review.sourceWindow.hostAlias"; + +/** Shows a Source window's host as main sees it, with Retry while it is offline. */ +export function showSourceWindowHostState( + serverId: string, + hosts: ReviewWindowHosts, + statusbar: IStatusbarService, + notifications: INotificationService, + connection: Pick | null, + retry: (alias: string) => void, + stored?: { get(): string | undefined; set(alias: string): void }, + every = HOST_STATE_POLL_MS, +): IDisposable { + const store = new DisposableStore(); + let mainAlias: string | undefined; + let host: Pick | undefined; + let lost = false; + let notified = false; + let entry: IStatusbarEntryAccessor | undefined; + let notice: INotificationHandle | undefined; + const alias = () => mainAlias ?? stored?.get() ?? serverId.slice(0, 8); + store.add(CommandsRegistry.registerCommand(RETRY_HOST_COMMAND, () => retry(alias()))); + const retryAction = toAction({ id: RETRY_HOST_COMMAND, label: "Retry", run: () => retry(alias()) }); + const render = () => { + if (!host) return; + const state = lost && host.state === "online" ? "offline" : host.state; + const status = reviewHostStatus({ alias: alias(), state, detail: host.detail }); + const offline = state !== "online"; + const canRetry = status.action === "retry"; + const props: IStatusbarEntry = { + name: "Remote Host", + text: status.label, + ariaLabel: status.label, + tooltip: offline ? status.sentence : undefined, + kind: offline ? "warning" : undefined, + command: canRetry ? RETRY_HOST_COMMAND : undefined, + }; + if (entry) entry.update(props); + else entry = store.add(statusbar.addEntry(props, "review.sourceWindow.host", StatusbarAlignment.LEFT, Number.MAX_VALUE)); + if (!offline) { + notified = false; + notice?.close(); + notice = undefined; + return; + } + const actions = { primary: canRetry ? [retryAction] : [] }; + if (notice) { + notice.updateMessage(status.sentence); + notice.updateActions(actions); + } else if (!notified) { + notified = true; + const shown = notifications.notify({ severity: Severity.Warning, message: status.sentence, actions }); + notice = shown; + Event.once(shown.onDidClose)(() => { + if (notice === shown) notice = undefined; + }); + } + }; + if (connection) { + store.add(connection.onDidStateChange(({ type }) => { + lost = type !== PersistentConnectionEventType.ConnectionGain; + render(); + })); + } + let stopped = false; + let handle: ReturnType | undefined; + const tick = async () => { + const state = await hosts.state(serverId, mainAlias ?? stored?.get()).catch(() => undefined); + if (stopped) return; + if (state) { + if (state.alias && state.alias !== mainAlias) stored?.set(state.alias); + mainAlias = state.alias ?? mainAlias; + host = state; + render(); + } + handle = setTimeout(() => void tick(), every); + }; + void tick(); + store.add(toDisposable(() => { + stopped = true; + clearTimeout(handle); + })); + return store; +} + +export class ReviewSourceWindowHostState extends Disposable implements IWorkbenchContribution { + static readonly ID = "review.sourceWindow.hostState"; + + constructor( + @IWorkbenchEnvironmentService environment: IWorkbenchEnvironmentService, + @IMainProcessService mainProcess: IMainProcessService, + @IStatusbarService statusbar: IStatusbarService, + @INotificationService notifications: INotificationService, + @IRemoteAgentService remoteAgent: IRemoteAgentService, + @IStorageService storage: IStorageService, + ) { + super(); + const authority = environment.remoteAuthority; + if (!authority || !isReviewRemoteAuthority(authority)) return; + const channel = mainProcess.getChannel(REVIEW_DESKTOP_CHANNEL); + const retry = (alias: string) => void channel.call("retryRemoteHost", alias).catch(() => undefined); + this._register(showSourceWindowHostState(reviewRemoteServerId(authority), reviewWindowHosts(channel), statusbar, notifications, remoteAgent.getConnection(), retry, { + get: () => storage.get(HOST_ALIAS_KEY, StorageScope.WORKSPACE), + set: (alias) => storage.store(HOST_ALIAS_KEY, alias, StorageScope.WORKSPACE, StorageTarget.MACHINE), + })); + } +} diff --git a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.test.ts new file mode 100644 index 000000000..342fb84a4 --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.test.ts @@ -0,0 +1,165 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from "node:assert/strict"; +import test from "node:test"; +import { URI } from "../../../base/common/uri.js"; +import type { IChannel } from "../../../base/parts/ipc/common/ipc.js"; +import { + type IRemoteAuthorityResolverService, + RemoteAuthorityResolverError, + RemoteAuthorityResolverErrorCode, + RemoteConnectionType, + type ResolverResult, +} from "../../../platform/remote/common/remoteAuthorityResolver.js"; +import type { ReviewGatewayHostState } from "../../common/reviewProtocol.js"; +import { remoteHostState } from "../../electron-main/remote/reviewRemoteHosts.js"; +import { reloadWhenOnline, reviewWindowAuthorityResolver, reviewWindowHosts, type ReviewWindowHosts } from "./reviewWindowAuthorityResolver.js"; + +const AUTHORITY = "whiteboard+abc-1"; + +/** Main's answers, in order; the last one repeats. */ +function fakeChannel(answers: { endpoint?: { host: string; port: number; connectionToken: string }; state?: { alias: string; state: string } }[]) { + const calls: [string, unknown][] = []; + let index = 0; + const channel = { + call: async (command: string, arg: unknown) => { + calls.push([command, arg]); + const answer = answers[Math.min(index, answers.length - 1)]; + if (command === "getRemoteLanguageEndpoint") return answer.endpoint; + index++; + return answer.state; + }, + listen: () => { + throw new Error("not used"); + }, + } as unknown as IChannel; + return { channel, calls }; +} + +/** + * Main as it behaves: each phase is what the gateway lists and which alias in + * the setting served the machine; the state goes through main's own lookup. + */ +function fakeMain(phases: { endpoint?: number; states: ReviewGatewayHostState[]; configured?: string }[]) { + let index = 0; + const phase = () => phases[Math.min(index, phases.length - 1)]; + const hosts: ReviewWindowHosts = { + endpoint: async () => { + const port = phase().endpoint; + return port === undefined ? undefined : endpoint(port); + }, + state: async (serverId) => { + const { states, configured } = phase(); + index++; + return remoteHostState(serverId, states, configured); + }, + }; + return { hosts, next: (...more: typeof phases) => phases.splice(0, phases.length, ...more) }; +} + +function fakeBase() { + let provider: ((uri: URI) => Promise) | undefined; + const base = { + resolveAuthority: async (authority: string): Promise => ({ + authority: { authority, connectTo: { type: RemoteConnectionType.WebSocket, host: "base", port: 1 }, connectionToken: undefined }, + }), + _setCanonicalURIProvider: (next: (uri: URI) => Promise) => (provider = next), + getCanonicalURI: (uri: URI) => provider!(uri), + } as unknown as IRemoteAuthorityResolverService; + return base; +} + +const endpoint = (port: number) => ({ host: "127.0.0.1", port, connectionToken: "vscode-token" }); +const connectTo = (result: ResolverResult) => { + const { connectTo, connectionToken } = result.authority; + assert.equal(connectTo.type, RemoteConnectionType.WebSocket); + return { host: connectTo.host, port: connectTo.port, connectionToken }; +}; + +test("a whiteboard+ authority resolves to its host's forward, afresh each time", async () => { + const { channel, calls } = fakeChannel([{ endpoint: endpoint(41000) }]); + const resolver = reviewWindowAuthorityResolver(fakeBase(), reviewWindowHosts(channel)); + + assert.deepEqual(connectTo(await resolver.resolveAuthority(AUTHORITY)), { host: "127.0.0.1", port: 41000, connectionToken: "vscode-token" }); + assert.deepEqual(calls, [["getRemoteLanguageEndpoint", "abc-1"]]); +}); + +test("a resolved authority waits through offline and unreachable for the moved forward", async () => { + const online = { alias: "devbox", serverId: "abc-1", state: "online" as const }; + const { hosts, next } = fakeMain([{ endpoint: 41000, states: [online], configured: "devbox" }]); + const resolver = reviewWindowAuthorityResolver(fakeBase(), hosts, { wait: 20, poll: 5 }); + assert.equal(connectTo(await resolver.resolveAuthority(AUTHORITY)).port, 41000); + + // The gateway keeps the id while offline and drops it with Desktop's problem report; each phase outlasts the wait. + const offline = Array.from({ length: 6 }, () => ({ states: [{ ...online, state: "offline" as const }], configured: "devbox" })); + const unreachable = Array.from({ length: 6 }, () => ({ states: [{ alias: "devbox", state: "unreachable" as const }], configured: "devbox" })); + next(...offline, ...unreachable, { endpoint: 42000, states: [online], configured: "devbox" }); + assert.equal(connectTo(await resolver.resolveAuthority(AUTHORITY)).port, 42000); +}); + +test("a resolved authority whose machine is gone after the wait fails; one still known keeps waiting", async () => { + const answers: Parameters[0] = [{ endpoint: endpoint(41000) }]; + const { channel } = fakeChannel(answers); + const resolver = reviewWindowAuthorityResolver(fakeBase(), reviewWindowHosts(channel), { wait: 20, poll: 5 }); + await resolver.resolveAuthority(AUTHORITY); + + answers.splice(0, 1, {}); + await assert.rejects(resolver.resolveAuthority(AUTHORITY), (error: RemoteAuthorityResolverError) => error._code === RemoteAuthorityResolverErrorCode.NotAvailable); + + answers.splice(0, 1, ...Array.from({ length: 10 }, () => ({ state: { alias: "devbox", state: "offline" } })), { endpoint: endpoint(43000) }); + assert.equal(connectTo(await resolver.resolveAuthority(AUTHORITY)).port, 43000); +}); + +test("a connecting host is waited for, and resolves once its forward appears", async () => { + const connecting = { state: { alias: "devbox", state: "connecting" } }; + const { channel } = fakeChannel([connecting, connecting, { endpoint: endpoint(43000) }]); + const resolver = reviewWindowAuthorityResolver(fakeBase(), reviewWindowHosts(channel), { wait: 1_000, poll: 1 }); + + assert.equal(connectTo(await resolver.resolveAuthority(AUTHORITY)).port, 43000); +}); + +test("a host still connecting after the wait, or not connecting at all, is offline", async () => { + for (const [state, wait] of [["connecting", 20], ["offline", 60_000]] as const) { + const { channel, calls } = fakeChannel([{ state: { alias: "devbox", state } }]); + const resolver = reviewWindowAuthorityResolver(fakeBase(), reviewWindowHosts(channel), { wait, poll: 5 }); + + await assert.rejects(resolver.resolveAuthority(AUTHORITY), (error: RemoteAuthorityResolverError) => { + assert.ok(error instanceof RemoteAuthorityResolverError); + assert.equal(error._code, RemoteAuthorityResolverErrorCode.NotAvailable); + assert.equal(error._detail, "devbox is offline"); + return true; + }); + if (state === "offline") assert.equal(calls.length, 2); + } +}); + +test("canonical URIs of whiteboard+ are themselves; every other authority is the window's", async () => { + const { channel, calls } = fakeChannel([{ endpoint: endpoint(41000) }]); + const resolver = reviewWindowAuthorityResolver(fakeBase(), reviewWindowHosts(channel)); + resolver._setCanonicalURIProvider(async () => { + throw new Error("no resolver extension"); + }); + + const own = URI.parse(`vscode-remote://${AUTHORITY}/home/dev/repo/f.ts`); + assert.equal(await resolver.getCanonicalURI(own), own); + await assert.rejects(resolver.getCanonicalURI(URI.parse("vscode-remote://ssh-remote+x/f.ts")), /no resolver extension/); + + for (const other of ["ssh-remote+x", "Whiteboard+ABC", "whiteboard+a/b"]) { + assert.equal(connectTo(await resolver.resolveAuthority(other)).host, "base"); + } + assert.deepEqual(calls, []); +}); + +test("a window that could not reach its host reloads when the host's forward answers", async () => { + let polls = 0; + const hosts: ReviewWindowHosts = { endpoint: async () => (++polls >= 3 ? endpoint(45000) : undefined), state: async () => undefined }; + const reloaded = Promise.withResolvers(); + const watch = reloadWhenOnline(hosts, "abc-1", reloaded.resolve, 1); + + await reloaded.promise; + watch.dispose(); + assert.equal(polls, 3); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.ts b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.ts new file mode 100644 index 000000000..e84366b9d --- /dev/null +++ b/apps/review-desktop/code-oss/src/vs/review/services/remote/reviewWindowAuthorityResolver.ts @@ -0,0 +1,86 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) dev.fast. All rights reserved. + * Licensed under the MIT License. See LICENSE in the repository root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { timeout } from "../../../base/common/async.js"; +import { toDisposable, type IDisposable } from "../../../base/common/lifecycle.js"; +import type { IChannel } from "../../../base/parts/ipc/common/ipc.js"; +import { + type IRemoteAuthorityResolverService, + RemoteAuthorityResolverError, + RemoteAuthorityResolverErrorCode, + type ResolverResult, + WebSocketRemoteConnection, +} from "../../../platform/remote/common/remoteAuthorityResolver.js"; +import type { ReviewGatewayHostState } from "../../common/reviewProtocol.js"; +import type { ReviewRemoteLanguageEndpoint } from "../reviewDesktopConnectionService.js"; +import { isReviewRemoteAuthority, override, reviewRemoteServerId } from "./reviewRemoteAuthority.js"; + +/** How long a Source window waits for its host to finish connecting. */ +export const REMOTE_WINDOW_RESOLVE_WAIT_MS = 60_000; + +export interface ReviewWindowHosts { + endpoint(serverId: string): Promise; + state(serverId: string, last?: string): Promise<({ alias?: string } & Pick) | undefined>; +} + +/** Reloads a window that could not reach its host, once the host's forward answers. */ +export function reloadWhenOnline(hosts: ReviewWindowHosts, serverId: string, reload: () => void, every = 5_000): IDisposable { + let done = false; + const check = async () => { + const endpoint = await hosts.endpoint(serverId).catch(() => undefined); + if (!endpoint || done) return; + done = true; + clearInterval(handle); + reload(); + }; + const handle = setInterval(() => void check(), every); + return toDisposable(() => { + done = true; + clearInterval(handle); + }); +} + +export function reviewWindowHosts(channel: IChannel): ReviewWindowHosts { + return { + endpoint: (serverId) => channel.call("getRemoteLanguageEndpoint", serverId), + state: (serverId, last) => channel.call("getRemoteHostState", last === undefined ? serverId : [serverId, last]), + }; +} + +/** Resolves `whiteboard+` to the host's current forward; other authorities go to `base`. */ +export function reviewWindowAuthorityResolver( + base: IRemoteAuthorityResolverService, + hosts: ReviewWindowHosts, + { wait = REMOTE_WINDOW_RESOLVE_WAIT_MS, poll = 1_000 } = {}, +): IRemoteAuthorityResolverService { + const resolved = new Set(); + const resolve = async (authority: string): Promise => { + const serverId = reviewRemoteServerId(authority); + const again = resolved.has(authority); + const deadline = Date.now() + wait; + for (;;) { + const endpoint = await hosts.endpoint(serverId); + if (endpoint) { + resolved.add(authority); + const { host, port, connectionToken } = endpoint; + return { authority: { authority, connectTo: new WebSocketRemoteConnection(host, port), connectionToken }, options: {} }; + } + const state = await hosts.state(serverId); + const left = deadline - Date.now(); + // A moved forward passes through offline or unreachable; only a machine gone from the setting ends a reconnect. + const waiting = again ? state !== undefined || left > 0 : (state === undefined || state.state === "connecting") && left > 0; + if (!waiting) { + const offline = `${state?.alias ?? serverId.slice(0, 8)} is offline`; + throw new RemoteAuthorityResolverError(offline, RemoteAuthorityResolverErrorCode.NotAvailable, offline); + } + await timeout(left > 0 ? Math.min(poll, left) : poll); + } + }; + return override(base, { + resolveAuthority: (authority) => (isReviewRemoteAuthority(authority) ? resolve(authority) : base.resolveAuthority(authority)), + _setCanonicalURIProvider: (provider) => + base._setCanonicalURIProvider((uri) => (isReviewRemoteAuthority(uri.authority) ? Promise.resolve(uri) : provider(uri))), + }); +} diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.test.ts index 3476334a2..4f62fd4d7 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.test.ts @@ -4,10 +4,14 @@ *--------------------------------------------------------------------------------------------*/ import assert from "node:assert/strict"; -import test from "node:test"; +import test, { type TestContext } from "node:test"; +import type { VSBuffer } from "../../base/common/buffer.js"; import { Event } from "../../base/common/event.js"; +import { URI } from "../../base/common/uri.js"; import { ReviewCanvasEditorInput } from "../browser/parts/canvas/reviewCanvasEditorInput.js"; +import { ReviewHostDown } from "../browser/parts/canvas/reviewHostFailure.js"; +import { apiSourceUri } from "../common/reviewSourceView.js"; import { ReviewCanvasEditorTabsService } from "./reviewCanvasEditorTabsService.js"; async function closeWelcome(updateNeeded: boolean): Promise { @@ -34,6 +38,8 @@ async function closeWelcome(updateNeeded: boolean): Promise { connection as never, {} as never, { warn() {} } as never, + {} as never, + {} as never, ); try { const welcome = await tabs.openWelcome(true); @@ -49,3 +55,68 @@ test("closing Welcome finishes the CLI install update only when one is pending", assert.equal(await closeWelcome(true), 1); assert.equal(await closeWelcome(false), 0); }); + +const HOST = "whiteboard+c0ffee"; +const remote = (path: string) => URI.from({ scheme: "vscode-remote", authority: HOST, path }).toString(); +const view = (generation?: string) => ({ reviewId: "r1", version: 2, generation }); + +function sourceTabs(t: TestContext, answer: (url: URL) => object) { + const opened: { toOpen: { workspaceUri?: URI; fileUri?: URI; label?: string }[]; options: Record }[] = []; + const written: string[] = []; + t.mock.method(globalThis, "fetch", async (url: string) => Response.json(answer(new URL(url)))); + const tabs = new ReviewCanvasEditorTabsService( + { createInstance: (_ctor: unknown, target: never) => new ReviewCanvasEditorInput(target, {} as never) } as never, + { onDidCloseEditor: Event.None } as never, + {} as never, + { async getConnection() { return { serverUrl: "http://localhost", token: "test" }; }, async readRemoteHosts() { return []; } } as never, + { async openWindow(toOpen: never, options: never) { opened.push({ toOpen, options }); } } as never, + { warn() {} } as never, + { async writeFile(resource: URI, content: VSBuffer) { written.push(`${resource.toString()}=${content.toString()}`); } } as never, + { cacheHome: URI.file("/laptop/cache") } as never, + ); + t.after(() => tabs.dispose()); + return { tabs, opened, written }; +} + +const remoteAnswer = { + workspacePath: "/home/dev/navigator/repo.code-workspace", + filePath: "/home/dev/repo/src/a b.ts", + serverId: "C0FFEE", +}; + +test("a remote answer opens the host's workspace and file at the position, titled with the review", async (t) => { + const { tabs, opened } = sourceTabs(t, () => remoteAnswer); + tabs.inputFor({ kind: "api", reviewId: "r1", title: "Fix the parser" }); + const resource = apiSourceUri({ view: view(), side: "head", file: "src/a b.ts" }); + assert.equal(await tabs.openSourceEditor({ resource, options: { selection: { startLineNumber: 3, startColumn: 2 } } }), true); + const [{ toOpen, options }] = opened; + assert.deepEqual(toOpen.map(item => (item.workspaceUri ?? item.fileUri)!.toString()), [remote("/home/dev/navigator/repo.code-workspace"), remote("/home/dev/repo/src/a b.ts:3:2")]); + assert.equal(options.remoteAuthority, HOST); +}); + +test("a remote diff opens its empty side from a laptop empty file", async (t) => { + const { tabs, opened, written } = sourceTabs(t, url => url.searchParams.has("empty") + ? { workspacePath: remoteAnswer.workspacePath, serverId: remoteAnswer.serverId, emptySide: true } + : remoteAnswer); + await tabs.openSourceEditor({ + original: { resource: apiSourceUri({ view: view("wt1"), side: "base", file: "src/a b.ts" }, true) }, + modified: { resource: apiSourceUri({ view: view("wt1"), side: "head", file: "src/a b.ts" }) }, + }); + const empty = URI.file("/laptop/cache/source-empty/empty"); + assert.deepEqual(written, [`${empty.toString()}=`]); + const [{ toOpen, options }] = opened; + assert.deepEqual(toOpen.map(item => (item.workspaceUri ?? item.fileUri)!.toString()), [ + remote("/home/dev/navigator/repo.code-workspace"), + empty.with({ scheme: "vscode-userdata" }).toString(), + remote("/home/dev/repo/src/a b.ts"), + ]); + assert.equal(options.remoteAuthority, HOST); + assert.equal(options.diffMode, true); +}); + +test("a gateway 503 naming a host is that host's error", async (t) => { + const { tabs } = sourceTabs(t, () => remoteAnswer); + t.mock.method(globalThis, "fetch", async () => Response.json({ ok: false, error: "gateway text" }, { status: 503, headers: { "x-review-host": "devbox" } })); + await assert.rejects(tabs.openApiSource({ reviewId: "r1", kind: "current" }, "Fix the parser"), (error: unknown) => + error instanceof ReviewHostDown && error.host.alias === "devbox" && error.host.state === "offline"); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.ts index 9a4d54687..5e5865bbc 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewCanvasEditorTabsService.ts @@ -3,10 +3,15 @@ * Licensed under the MIT License. See LICENSE in the repository root for license information. *--------------------------------------------------------------------------------------------*/ +import { VSBuffer } from "../../base/common/buffer.js"; import { Event } from "../../base/common/event.js"; +import { Schemas } from "../../base/common/network.js"; import { Disposable } from "../../base/common/lifecycle.js"; +import { joinPath } from "../../base/common/resources.js"; import { URI } from "../../base/common/uri.js"; import type { ITextEditorOptions } from "../../platform/editor/common/editor.js"; +import { IEnvironmentService } from "../../platform/environment/common/environment.js"; +import { IFileService } from "../../platform/files/common/files.js"; import { createDecorator, IInstantiationService } from "../../platform/instantiation/common/instantiation.js"; import { ILogService } from "../../platform/log/common/log.js"; import type { EditorInput } from "../../workbench/common/editor/editorInput.js"; @@ -18,12 +23,23 @@ import { ReviewCanvasEditorInput, type ReviewCanvasEditorTarget, } from "../browser/parts/canvas/reviewCanvasEditorInput.js"; +import { ReviewHostDown } from "../browser/parts/canvas/reviewHostFailure.js"; -import { reviewSourceQuery, type ReviewSourceSelection } from "../common/reviewProtocol.js"; +import { reviewSourceQuery, type ReviewRemoteNavigatorAnswer, type ReviewSourceSelection } from "../common/reviewProtocol.js"; import { REVIEW_LANGUAGE_SOURCE_SCHEME } from "../common/reviewReadonlySource.js"; import { sourceLocation, sourceSelectionIdentity, REVIEW_API_SOURCE_SCHEME } from "../common/reviewSourceView.js"; +import { reviewRemoteAuthority } from "./remote/reviewRemoteAuthority.js"; import { IReviewDesktopConnectionService, reviewResponseError } from "./reviewDesktopConnectionService.js"; +type NavigatorAnswer = { workspacePath: string; filePath?: string } | ReviewRemoteNavigatorAnswer; + +function navigatorPaths(answer: NavigatorAnswer): { uri: (path: string) => URI; remote: { remoteAuthority?: string } } { + if (!("serverId" in answer)) return { uri: URI.file, remote: {} }; + const authority = reviewRemoteAuthority(answer.serverId); + if (!authority) throw new Error("The remote host's server id cannot be opened."); + return { uri: (path) => URI.from({ scheme: Schemas.vscodeRemote, authority, path }), remote: { remoteAuthority: authority } }; +} + export const IReviewCanvasEditorTabsService = createDecorator( "reviewCanvasEditorTabsService", ); @@ -59,6 +75,8 @@ export class ReviewCanvasEditorTabsService extends Disposable implements IReview private readonly desktopConnection: IReviewDesktopConnectionService, @IHostService private readonly host: IHostService, @ILogService private readonly logService: ILogService, + @IFileService private readonly files: IFileService, + @IEnvironmentService private readonly environment: IEnvironmentService, ) { super(); this._register( @@ -109,7 +127,8 @@ export class ReviewCanvasEditorTabsService extends Disposable implements IReview async openApiSource(selection: ReviewSourceSelection, title: string): Promise { const result = await this.navigatorWorkspace(selection.reviewId, selection.kind === "version" ? { version: selection.version } : {}); - await this.host.openWindow([{ workspaceUri: URI.file(result.workspacePath), label: title }], { forceNewWindow: true }); + const { uri, remote } = navigatorPaths(result); + await this.host.openWindow([{ workspaceUri: uri(result.workspacePath), label: title }], { forceNewWindow: true, ...remote }); } /** Hand source opens to the native workspace before Review creates an editor group. */ @@ -118,27 +137,27 @@ export class ReviewCanvasEditorTabsService extends Disposable implements IReview const resources = diff ? [editor.original.resource, editor.modified.resource] : [isResourceEditorInput(editor) ? editor.resource : undefined]; if (!resources.every((resource): resource is URI => !!resource && [REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME].includes(resource.scheme))) return false; const destinations = await Promise.all(resources.map(resource => this.sourceDestination(resource))); + const { workspaceUri, remote } = destinations[destinations.length - 1]; + const selection = !diff ? (editor.options as ITextEditorOptions | undefined)?.selection : undefined; await this.host.openWindow([ - { workspaceUri: destinations[destinations.length - 1].workspaceUri }, - ...destinations.map(({ filePath }) => { - const selection = !diff ? (editor.options as ITextEditorOptions | undefined)?.selection : undefined; - return { fileUri: URI.file(selection ? `${filePath}:${selection.startLineNumber}:${selection.startColumn ?? 1}` : filePath) }; - }), - ], { forceNewWindow: true, gotoLineMode: true, diffMode: diff }); + { workspaceUri }, + ...destinations.map(({ fileUri }) => ({ fileUri: selection ? fileUri.with({ path: `${fileUri.path}:${selection.startLineNumber}:${selection.startColumn ?? 1}` }) : fileUri })), + ], { forceNewWindow: true, gotoLineMode: true, diffMode: diff, ...remote }); return true; } async openSourceReferences(resource: URI, position: { readonly lineNumber: number; readonly column: number }): Promise { if (![REVIEW_API_SOURCE_SCHEME, REVIEW_LANGUAGE_SOURCE_SCHEME].includes(resource.scheme)) return false; - const destination = await this.sourceDestination(resource); - await this.host.openWindow([{ workspaceUri: destination.workspaceUri }], { + const { workspaceUri, fileUri, remote } = await this.sourceDestination(resource); + await this.host.openWindow([{ workspaceUri }], { forceNewWindow: true, - reviewReferencesToShow: { resource: URI.file(destination.filePath), lineNumber: position.lineNumber, column: position.column }, + ...remote, + reviewReferencesToShow: { resource: fileUri, lineNumber: position.lineNumber, column: position.column }, }); return true; } - private async sourceDestination(resource: URI): Promise<{ workspaceUri: URI; filePath: string }> { + private async sourceDestination(resource: URI) { const target = sourceLocation(resource); const local = resource.scheme === REVIEW_LANGUAGE_SOURCE_SCHEME; const result = await this.navigatorWorkspace(target.view.reviewId, { @@ -147,12 +166,21 @@ export class ReviewCanvasEditorTabsService extends Disposable implements IReview file: local ? undefined : target.file, empty: new URLSearchParams(resource.query).has("empty") ? "true" : undefined, }); - const filePath = local ? resource.fsPath : result.filePath; - if (!filePath) throw new Error("The navigator did not resolve the source file."); - return { workspaceUri: URI.file(result.workspacePath), filePath }; + const { uri, remote } = navigatorPaths(result); + const fileUri = "serverId" in result + ? result.emptySide ? await this.emptyFile() : result.filePath && uri(result.filePath) + : local ? URI.file(resource.fsPath) : result.filePath && uri(result.filePath); + if (!fileUri) throw new Error("The navigator did not resolve the source file."); + return { workspaceUri: uri(result.workspacePath), fileUri, remote }; } - private async navigatorWorkspace(reviewId: string, values: Record): Promise<{ workspacePath: string; filePath?: string }> { + private async emptyFile(): Promise { + const resource = joinPath(this.environment.cacheHome, "source-empty", "empty"); + await this.files.writeFile(resource, VSBuffer.fromString("")); + return resource.with({ scheme: Schemas.vscodeUserData }); + } + + private async navigatorWorkspace(reviewId: string, values: Record): Promise { const { serverUrl, token } = await this.desktopConnection.getConnection(); const query = new URLSearchParams(Object.entries(values).filter(([key, value]) => key !== "reviewId" && value !== undefined).map(([key, value]) => [key, String(value)])); const response = await fetch(`${serverUrl}/reviews-api/${encodeURIComponent(reviewId)}/navigator${query.size ? `?${query}` : ""}`, { @@ -160,6 +188,11 @@ export class ReviewCanvasEditorTabsService extends Disposable implements IReview headers: { "x-review-token": token }, signal: AbortSignal.timeout(60_000), }); + const alias = response.status === 503 && response.headers.get("x-review-host"); + if (alias) { + const host = (await this.desktopConnection.readRemoteHosts().catch(() => [])).find(host => host.alias === alias); + throw new ReviewHostDown(host && host.state !== "online" ? host : { alias, state: "offline" }); + } if (!response.ok) throw await reviewResponseError(response, "Could not open the code navigator."); return response.json(); } diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDesktopConnectionService.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDesktopConnectionService.ts index 8c214ffea..d74460cc4 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewDesktopConnectionService.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewDesktopConnectionService.ts @@ -258,12 +258,7 @@ export class ReviewDesktopConnectionService extends Disposable implements IRevie } async getRemoteLanguageEndpoint(serverId: string): Promise { - const endpoint: unknown = await this.mainProcessService.getChannel(REVIEW_DESKTOP_CHANNEL).call("getRemoteLanguageEndpoint", serverId); - if (typeof endpoint !== "object" || endpoint === null) return undefined; - const { host, port, connectionToken } = endpoint as Record; - return typeof host === "string" && Number.isInteger(port) && typeof connectionToken === "string" - ? { host, port: port as number, connectionToken } - : undefined; + return this.mainProcessService.getChannel(REVIEW_DESKTOP_CHANNEL).call("getRemoteLanguageEndpoint", serverId); } async installRemoteHost(alias: string): Promise { diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewEditorResolverService.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewEditorResolverService.test.ts index 179afa0e7..0139f6443 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewEditorResolverService.test.ts +++ b/apps/review-desktop/code-oss/src/vs/review/services/reviewEditorResolverService.test.ts @@ -28,6 +28,8 @@ test("source tree, selected code, definitions and diffs hand off before creating { async getConnection() { return { serverUrl: "http://localhost", token: "test" }; } } as never, { async openWindow(openables: IWindowOpenable[], options: IOpenWindowOptions) { windows.push({ openables, options }); } } as never, { warn() {} } as never, + {} as never, + {} as never, ); const resolver = new ReviewEditorResolverService( { get activeGroup() { throw new Error("Review must not create an editor group"); } } as never, @@ -46,7 +48,7 @@ test("source tree, selected code, definitions and diffs hand off before creating { workspaceUri: URI.file("/navigator/base.code-workspace") }, { fileUri: URI.file("/navigator/base/old-name.ts:42:3") }, ]); - assert.deepEqual(windows[1].options, { forceNewWindow: true, gotoLineMode: true, diffMode: false }); + assert.equal(windows[1].options.remoteAuthority, undefined); assert.equal(requests[1].searchParams.get("version"), "7"); assert.equal(requests[1].searchParams.get("commit"), "selected-commit"); assert.equal(requests[1].searchParams.get("repositoryId"), "other-repository"); diff --git a/apps/review-desktop/code-oss/src/vs/server/node/reviewServerHandshake.test.ts b/apps/review-desktop/code-oss/src/vs/server/node/reviewServerHandshake.test.ts index 9bb92e665..5af306f62 100644 --- a/apps/review-desktop/code-oss/src/vs/server/node/reviewServerHandshake.test.ts +++ b/apps/review-desktop/code-oss/src/vs/server/node/reviewServerHandshake.test.ts @@ -160,6 +160,10 @@ if (external) { const connection = await handshake(target); assert.equal(connection.environment.pid, server.pid); assert.match(output, /Launched Extension Host Process/); + const extensions = connection.management.client.getChannel('extensions'); + assert.deepEqual(await extensions.call('getInstalled', []), []); + await assert.rejects(extensions.call('installFromGallery', []), /not managed from a Source window \(installFromGallery\)/); + await connection.management.client.getChannel('extensionGalleryManifest').call('setExtensionGalleryManifest', [null]); close(connection); }); diff --git a/apps/review-desktop/code-oss/src/vs/server/node/serverServices.ts b/apps/review-desktop/code-oss/src/vs/server/node/serverServices.ts index 445ef4d9a..61671acf4 100644 --- a/apps/review-desktop/code-oss/src/vs/server/node/serverServices.ts +++ b/apps/review-desktop/code-oss/src/vs/server/node/serverServices.ts @@ -139,6 +139,10 @@ export async function setupServerServices(connectionToken: ServerConnectionToken // Whiteboard: the client always opens this channel; telemetry is cut. socketServer.registerChannel('telemetry', new NullTelemetryChannel()); + // Whiteboard: extension management is cut. + socketServer.registerChannel('extensions', new NoExtensionsChannel()); + socketServer.registerChannel('extensionGalleryManifest', new NoExtensionsChannel()); + const remoteExtensionsScanner = new RemoteExtensionsScannerService(userDataProfilesService, extensionsScannerService, logService); socketServer.registerChannel(RemoteExtensionsScannerChannelName, new RemoteExtensionsScannerChannel(remoteExtensionsScanner, (ctx: RemoteAgentConnectionContext) => getUriTransformer(ctx.remoteAuthority))); @@ -156,6 +160,15 @@ class NullTelemetryChannel implements IServerChannel(): Event { return Event.None; } } +class NoExtensionsChannel implements IServerChannel { + async call(_: RemoteAgentConnectionContext, command: string): Promise { + if (command === 'getInstalled') { return [] as T; } + if (command === 'setExtensionGalleryManifest') { return undefined as T; } + throw new Error(`Extensions on a Whiteboard remote are not managed from a Source window (${command}).`); + } + listen(): Event { return Event.None; } +} + const _uriTransformerCache: { [remoteAuthority: string]: IURITransformer } = Object.create(null); function getUriTransformer(remoteAuthority: string): IURITransformer { diff --git a/apps/review-desktop/scripts/e2e/TESTING.md b/apps/review-desktop/scripts/e2e/TESTING.md index fbb75eeba..7a7be589b 100644 --- a/apps/review-desktop/scripts/e2e/TESTING.md +++ b/apps/review-desktop/scripts/e2e/TESTING.md @@ -123,7 +123,7 @@ key, and the journey adds `wb-test-a` in Settings as a user would. It checks, on the DOM and on the page's requests: the host goes `online`; Home lists the container's review as `wb-test-a: wbrepo`; the document, a code peek, the Diff view and the structural diff load; an edit and a `session_create` on the -remote reach the window; a killed `ssh` master shows "Connection lost" and +remote reach the window; a killed `ssh` master shows a chip in the top bar and recovers without a reload; a stopped server is `offline` within 15 s while a laptop review still opens; another package version is `incompatible` with the install command in Settings; and removing the host takes its reviews out of diff --git a/apps/review-desktop/scripts/e2e/harness.mjs b/apps/review-desktop/scripts/e2e/harness.mjs index 2fdce52aa..04c5ae0e6 100644 --- a/apps/review-desktop/scripts/e2e/harness.mjs +++ b/apps/review-desktop/scripts/e2e/harness.mjs @@ -565,13 +565,16 @@ export async function createHarness({ } // Artifacts are best-effort: a Desktop that outlives the runner is worse than a missing screenshot. - if (!success && page) { - await page + // A journey may have moved ctx.page to another window after a relaunch. + const shown = ctx.page ?? page; + + if (!success && shown) { + await shown .screenshot({ path: path.join(root, "failure.png") }) .catch(() => {}); await writeFile( path.join(root, "failure-dom.txt"), - await page + await shown .locator("body") .innerText() .catch(() => ""), 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..13cadc30d --- /dev/null +++ b/apps/review-desktop/scripts/e2e/journeys/remote-ask.mjs @@ -0,0 +1,413 @@ +/** 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 { closeSourceWindow, openHome, openSettings } from "../harness.mjs"; +import { + alias, + closeDesktop, + createRemoteReview, + masterPid, + onRemote, + prepared, + recordRequests, + remote, + remoteToken, + runDir, + sourcePages, +} 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}`); +} + +const reviewCanvas = (ctx) => + ctx.page.locator(".review-canvas-root [data-review-api]"); + +/** Ask, docked in the side panel or floating, by the layout. */ +export const askPanel = (ctx) => + ctx.page.locator( + '[role=complementary][aria-label="Ask"], [role=dialog][aria-label="Ask"]', + ); + +/** Home's row for the review titled `title`. */ +export const sessionRow = (ctx, title) => + ctx.page + .locator("main.review-home [data-home-row]") + .filter({ hasText: title }); + +/** Opens the review titled `title` from Home. */ +export async function openRemoteReview(ctx, title) { + await openHome(ctx); + await sessionRow(ctx, title).waitFor({ timeout: 60000 }); + await sessionRow(ctx, title).getByTitle(title, { exact: true }).click(); + await reviewCanvas(ctx).getByRole("heading", { name: title }).waitFor({ + timeout: 60000, + }); +} + +/** Selects the document's sentence and opens Ask on it with ⌘L. */ +export async function askAboutReviewSentence(ctx) { + // 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 = reviewCanvas(ctx).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 askPanel(ctx) + .getByRole("combobox", { name: "Question" }) + .waitFor({ timeout: 30000 }); +} + +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 = () => reviewCanvas(ctx); + + const panel = () => askPanel(ctx); + + const composer = () => panel().getByRole("combobox", { name: "Question" }); + + const homeRow = () => sessionRow(ctx, title); + + const openReview = () => openRemoteReview(ctx, title); + + const askAboutSentence = () => askAboutReviewSentence(ctx); + + /** 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. The answer's f.ts is a link: it opens the host's file in a Source window. + const reference = panel() + .getByRole("link") + .filter({ hasText: /^f\.ts$/ }); + + await reference.waitFor(); + await reference.click(); + + const source = await until( + () => sourcePages(ctx)[0], + "a Source window for the answer's f.ts", + 60000, + ); + + await until( + async () => + ( + await source.locator(".tabs-container .tab.active").innerText() + ).includes("f.ts"), + "f.ts in the Source window", + 60000, + ); + await closeSourceWindow(source); + ctx.check("6. the answer's f.ts link opened f.ts in a Source window"); + + // 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); + + 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`, + ); +} + +/** 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..a71711f4d 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); @@ -161,6 +161,14 @@ async function recordRequests(page, streamed) { return requests; } +/** Every other workbench window than `ctx.page`: the Source windows. */ +export function sourcePages(ctx) { + return ctx.browser + .contexts() + .flatMap((context) => context.pages()) + .filter((page) => page !== ctx.page && !page.isClosed()); +} + const DESKTOP_ROUTE = /^\/(app|control|crash-reports|diffr-config|install|preferences|remote-hosts|telemetry|tutorial)(\/|$)|^\/reviews-api\/[^/]+\/telemetry\//; @@ -191,7 +199,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([ @@ -409,12 +417,17 @@ async function journey(ctx, page, until) { await canvas.getByRole("heading", { name: second }).waitFor(); const laptopOnly = canvas.locator( - 'button[aria-label="Source tree ↗"], button[aria-label="Share review"], button[aria-label="Shared review"], [aria-label="Session views"] button[aria-label="Trace"]', + 'button[aria-label="Share review"], button[aria-label="Shared review"], [aria-label="Session views"] button[aria-label="Trace"]', + ); + + const sourceTree = canvas.locator( + 'button[aria-label="Source tree ↗"]:not([disabled])', ); await until( - async () => (await laptopOnly.count()) === 0, - "the pushed tab without Source tree, Share and Trace", + async () => + (await laptopOnly.count()) === 0 && (await sourceTree.count()) === 1, + "the pushed tab with Source tree, without Share and Trace", 10000, ); await view("Diff").click(); @@ -425,32 +438,31 @@ async function journey(ctx, page, until) { ), "the pushed tab's Diff view to show f.ts", ); - assert.ok( - await page - .locator(".review-multidiff-open-container") - .evaluateAll((all) => all.length > 0 && all.every((e) => e.hidden)), - "Open file shown for a review on another machine", + // A host with language features opens a review's files in a Source window on it. + await until( + () => + page + .locator(".review-multidiff-open-container") + .evaluateAll((all) => all.length > 0 && all.some((e) => !e.hidden)), + "Open file in the pushed tab's Diff view", + 10000, ); await view("Whiteboard").click(); ctx.check( - "5. session_create with open: true on the remote opened a tab without Open file, Source tree, Share or Trace", + "5. session_create with open: true on the remote opened a tab with Open file and Source tree, without Share or Trace", ); // 6. The ssh master dies: the review says so, then recovers in the same page. await tab(title).click(); await canvas.getByRole("heading", { name: title }).waitFor(); await page.evaluate(() => { - const seen = (window.__remoteHostBanner = []); + const seen = (window.__remoteHostChip = []); new MutationObserver(() => { - for (const status of document.querySelectorAll( - ".review-canvas-root [role=status]", + for (const chip of document.querySelectorAll( + ".review-canvas-root .connection-chip", )) - if ( - status.textContent.startsWith("Connection lost") && - !seen.includes(status.textContent) - ) - seen.push(status.textContent); + if (!seen.includes(chip.textContent)) seen.push(chip.textContent); }).observe(document.body, { subtree: true, childList: true, @@ -463,8 +475,8 @@ async function journey(ctx, page, until) { assert.ok(master, "no ssh master for the host"); process.kill(master, "SIGKILL"); await until( - () => page.evaluate(() => window.__remoteHostBanner.length > 0), - "the Connection lost banner", + () => page.evaluate(() => window.__remoteHostChip.length > 0), + "the disconnected chip", 30000, ); await until( @@ -474,12 +486,8 @@ async function journey(ctx, page, until) { 60000, ); await until( - async () => - (await canvas - .locator("[role=status]") - .filter({ hasText: "Connection lost" }) - .count()) === 0, - "the banner to clear", + async () => (await canvas.locator(".connection-chip").count()) === 0, + "the chip to clear", ); await remoteApi("session_edit", { sessionId: reviewId, @@ -491,7 +499,7 @@ async function journey(ctx, page, until) { await canvas.getByText("After the reconnect.").waitFor({ timeout: 30000 }); assert.equal(await page.evaluate(() => performance.timeOrigin), timeOrigin); ctx.check( - `6. a killed ssh master showed "${await page.evaluate(() => window.__remoteHostBanner[0])}" and the review recovered in the same page`, + `6. a killed ssh master showed "${await page.evaluate(() => window.__remoteHostChip[0])}" in the top bar and the review recovered in the same page`, ); // 7. A hung server: offline within 15 s, and a laptop review still opens at once. @@ -532,8 +540,11 @@ async function journey(ctx, page, until) { section = settings.getByRole("region", { name: "Remote hosts" }); await until( - async () => /incompatible/.test(await hostRow().innerText()), - "the Settings row to read incompatible", + async () => + /runs Whiteboard .+; this Desktop runs/.test( + await hostRow().innerText(), + ), + "the Settings row to name both versions", ); assert.match( await hostRow().locator("code").innerText(), @@ -544,7 +555,7 @@ async function journey(ctx, page, until) { for (const text of [title, second]) { await row(text).waitFor(); assert.equal(await row(text).getAttribute("data-unavailable"), ""); - assert.match(await row(text).innerText(), /incompatible/); + assert.match(await row(text).innerText(), /needs an update/); } ctx.check( @@ -672,7 +683,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..a381a2fba 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(); @@ -511,17 +511,14 @@ async function steps(ctx, until, watch, timings, manifestPath) { assert.doesNotMatch(first.text, /\/home\//, "the question names a path"); const settingsSteps = (await pageRecord(ctx)).rows[fresh].filter((text) => - text.startsWith("installing · "), + STEP_ORDER.some((step) => text.startsWith(step)), ); assert.ok( settingsSteps.length >= 2, `Settings showed ${settingsSteps.length} install step(s): ${settingsSteps}`, ); - assertStepOrder( - settingsSteps.map((text) => text.slice("installing · ".length)), - "Settings", - ); + assertStepOrder(settingsSteps, "Settings"); await onRemote( fresh, `test -x ${ROOT}/versions/${version}/node_modules/@dev.fast/diffr-linux-x64/diffr`, @@ -563,7 +560,7 @@ async function steps(ctx, until, watch, timings, manifestPath) { ); await until( async () => - /unsupported · .*glibc 2\.31/.test( + /glibc 2\.31; Whiteboard needs 2\.34/.test( await hostRow(oldSection, old).innerText(), ), "the Settings row to read unsupported", diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-source-window.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-source-window.mjs new file mode 100644 index 000000000..be019eb4c --- /dev/null +++ b/apps/review-desktop/scripts/e2e/journeys/remote-source-window.mjs @@ -0,0 +1,739 @@ +/** A remote review's Source window, as a user meets it: opened from the review and from Ask, offline and back, restored at start, closed when its host goes. */ +import assert from "node:assert/strict"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +import { openSettings } from "../harness.mjs"; +import { + askAboutReviewSentence, + askPanel, + openRemoteReview, +} from "./remote-ask.mjs"; +import { + alias, + closeDesktop, + onRemote, + prepared, + remote, + runDir, + sourcePages, +} from "./remote-host.mjs"; +import { removeHost } from "./remote-install.mjs"; +import { lines, pointerHover } from "./remote-lsp.mjs"; +import { + fileText, + focusFile, + inSource, + pointAt, + quickRows, + typeInto, + windowState, +} from "../remote/source-window-helpers.mjs"; + +const exec = promisify(execFile); + +const title = "Remote source"; + +const second = "Second checkout"; + +export const name = "remote-source-window"; + +// 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` }, +}; + +/** Runs on the remote: two repositories, each with a review of its second commit (f.ts changed, added.ts added). Prints the reviews' ids. */ +const fixture = String.raw` +set -e +field() { node -pe "JSON.parse(require('fs').readFileSync(0, 'utf8')).$1"; } +make() { + rm -rf ~/$1 + git init -q -b main ~/$1 + cd ~/$1 + git config user.email e2e@example.invalid + git config user.name e2e + mkdir docs + printf '%s\n' "$3" > docs/needle.md + printf 'export function one() {\n return 1;\n}\n' > f.ts + git add . + git commit -qm one + printf 'export function one() {\n return 2;\n}\n\nexport function two() {\n const unused = 3;\n return one() + 1;\n}\n' > f.ts + printf 'export const added = "wbsource-added";\n' > added.ts + git add . + git commit -qm two + id=$(whiteboard api session_create "{\"title\":\"$2\",\"open\":false,\"target\":{\"kind\":\"commits\",\"repositoryPath\":\"$HOME/$1\",\"base\":\"HEAD~1\",\"head\":\"HEAD\"}}" | field sessionId) + whiteboard api session_edit "{\"sessionId\":\"$id\",\"edit\":{\"type\":\"insert\",\"content\":{\"type\":\"markdown\",\"markdown\":\"One now returns **two**.\"}}}" >/dev/null + echo "$id" +} +make wbsource "$1" wbsource-needle +make wbsource2 "$2" wbsource-other +`; + +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-source-window runs in development mode only"); + + if (prepared === undefined) + try { + await exec("docker", ["info", "--format", "{{.ServerVersion}}"]); + } catch (error) { + throw new Error( + `skip: remote-source-window 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-source-window] down --all: ${error.message}`), + ); + else await closeDesktop(ctx); + } +} + +function check(ctx, text) { + console.error(`[remote-source-window] ${new Date().toISOString()} ${text}`); + ctx.check(text); +} + +/** + * Freezes the host: the container, or on a prepared host its user's Node + * and Whiteboard processes (the review server, the VS Code server and its + * extension hosts, the attach the Desktop's ssh runs). + * Returns the function that thaws it. + */ +async function freeze() { + if (prepared === undefined) { + await remote("pause", "a"); + + return () => remote("resume", "a"); + } + + const pids = await onRemote(String.raw` + s=$(whiteboard server status --json | node -pe "JSON.parse(require('fs').readFileSync(0, 'utf8')).serverPid") + p=$(pgrep -u "$(id -u)" -f 'node|whiteboard' | grep -vx "$$" | tr '\n' ' ') + kill -STOP $s $p 2>/dev/null + echo $s $p + `); + + return () => onRemote(`kill -CONT ${pids}`); +} + +/** The host's remote extension hosts, as `[pid, rss KiB]`. */ +async function extensionHosts() { + return ( + await onRemote( + "ps -eo pid=,rss=,args= | grep -- '--type=extensionHost' | grep -v grep || true", + ) + ) + .split("\n") + .filter(Boolean) + .map((line) => line.trim().split(/\s+/).slice(0, 2).map(Number)); +} + +const mib = (kib) => Math.round(kib / 1024); + +/** The Source window's host entry in its status bar. */ +const hostEntry = (page) => + page + .locator('[id="review.sourceWindow.host"]') + .innerText({ timeout: 1000 }) + .then((text) => text.trim()) + .catch(() => ""); + +async function journey(ctx) { + const { until } = ctx; + const timings = {}; + + const hostState = async () => + (await ctx.apiOk("/remote-hosts")).find((host) => host.alias === alias); + + const openReview = (text) => openRemoteReview(ctx, text); + + /** The review's Diff view, then Open file on f.ts; resolves with the Source window that shows it. */ + async function openFile(text) { + await openReview(text); + await ctx.page + .locator('[aria-label="Session views"] button[aria-label="Diff"]') + .filter({ visible: true }) + .click(); + await until( + async () => + ( + await ctx.page + .locator(".review-path-label") + .filter({ visible: true }) + .allInnerTexts() + ).some((t) => t.includes("f.ts")), + "the Diff view to show f.ts", + ); + + // The button in the header whose path label reads f.ts. + const index = await until( + () => + ctx.page + .evaluate(() => + [...document.querySelectorAll(".review-multidiff-open")].findIndex( + (button) => { + for (let e = button; e; e = e.parentElement) { + const label = e.querySelector(".review-path-label"); + + if (label) return label.innerText.trim() === "f.ts"; + } + + return false; + }, + ), + ) + .then((i) => (i >= 0 ? i : null)), + "f.ts's Open file button", + ); + + const before = new Set(sourcePages(ctx)); + + await ctx.page.locator(".review-multidiff-open").nth(index).click(); + + const source = await until( + () => sourcePages(ctx).find((page) => !before.has(page)), + `a new Source window for ${text}`, + 60000, + ); + + await until( + async () => (await lines(source)).includes("return one() + 1;"), + "f.ts's text from the host in the Source window", + 120000, + ); + + return source; + } + + // 1. A host holding two reviews, added in Settings. + if (prepared === undefined) { + await remote("up", "a", "--fake-agent"); + await remote("install", "a"); + } + + await onRemote("whiteboard remote extensions ensure --json"); + + const [reviewId, secondId] = ( + await onRemote(`bash -s -- '${title}' '${second}'`, fixture) + ).split("\n"); + + assert.match(reviewId, /^[0-9a-f-]{36}$/, "the remote review's id"); + assert.match(secondId, /^[0-9a-f-]{36}$/, "the second review's id"); + + 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 until( + async () => { + const host = await hostState(); + + return host?.state === "online" && host.languageFeatures === true; + }, + `${alias} online with language features`, + 180000, + ); + timings.online = Date.now() - added; + check( + ctx, + `1. ${alias}${prepared ? " (prepared)" : ""} holds two reviews; Settings added it, online with language features ${timings.online} ms after Add`, + ); + + // 2. Open file opens a Source window bound to the host, titled for the review. + const opened = Date.now(); + const source = await openFile(title); + + timings.openFile = Date.now() - opened; + + let state = await inSource(source, windowState); + const { authority } = state; + const serverId = authority.replace(/^whiteboard\+/, ""); + + assert.match(authority, /^whiteboard\+[0-9a-f-]+$/, JSON.stringify(state)); + const { serverId: hostServerId } = await hostState(); + + assert.ok(hostServerId, "the host reports no server id"); + assert.equal(serverId, hostServerId.toLowerCase()); + assert.equal(state.title, `${title} — Source — Whiteboard`); + assert.equal(await source.title(), state.title); + assert.match( + state.folder, + new RegExp(`^vscode-remote://${authority.replace("+", "%2B")}/`), + ); + assert.equal(sourcePages(ctx).length, 1); + check( + ctx, + `2. Open file opened one Source window on ${authority} in ${timings.openFile} ms, titled "${state.title}"`, + ); + + // 3. The explorer lists the checkout. + await source.keyboard.press("ControlOrMeta+Shift+KeyE"); + + const explorer = await until(async () => { + const rows = await source + .locator(".explorer-folders-view .monaco-list-row") + .allInnerTexts() + .catch(() => []); + + return ["docs", "added.ts", "f.ts"].every((name) => + rows.some((row) => row.trim() === name), + ) + ? rows + : null; + }, "the explorer to list docs, added.ts and f.ts"); + + check( + ctx, + `3. the explorer lists the checkout: ${explorer.map((r) => r.trim()).join(", ")}`, + ); + + // 4. Read-only: typing changes nothing and the notice shows. + await inSource(source, focusFile, state.active.resource); + + const typed = await typeInto(source, ctx); + + state = await inSource(source, windowState); + assert.ok(state.active.readonly, "f.ts is not read-only"); + assert.ok(typed.message, "no read-only notice"); + check( + ctx, + `4. f.ts is read-only: typing changed nothing and showed "${typed.message}"`, + ); + + // 5. Quick open answers from the host: f.ts, and needle.md, which no editor has open. + const quick = source.locator(".quick-input-widget input"); + + for (const query of ["f.ts", "needle"]) { + await source.keyboard.press("ControlOrMeta+KeyP"); + await quick.waitFor({ state: "visible" }); + await quick.fill(query); + await until( + async () => + (await quickRows(source)).some((t) => + t.includes(query === "needle" ? "needle.md" : "f.ts"), + ), + `quick open to find ${query}`, + 60000, + ); + await source.keyboard.press("Escape"); + await quick.waitFor({ state: "hidden" }); + } + + check(ctx, "5. quick open found f.ts, and needle.md, which was not open"); + + // 6. Text search. + await source.keyboard.press("ControlOrMeta+Shift+KeyF"); + await until( + () => + source.evaluate( + () => !!document.activeElement?.closest(".search-view .search-widget"), + ), + "focus in the search view's input", + ); + await source.keyboard.press("ControlOrMeta+KeyA"); + await source.keyboard.type("wbsource-needle"); + + const results = await until(async () => { + const text = await source + .locator(".search-view") + .innerText() + .catch(() => ""); + + return text.includes("needle.md") && text; + }, "needle.md in the search results"); + + check( + ctx, + `6. text search found wbsource-needle: "${results.replace(/\s+/g, " ").slice(0, 60)}…"`, + ); + + // 7. A TypeScript hover from the host's language server. + await inSource(source, focusFile, state.active.resource); + + const hover = await pointerHover( + source, + await inSource(source, pointAt, 7, "one"), + /function one\(\): number/, + 90000, + ); + + assert.ok(hover.ms !== null, `no hover: ${JSON.stringify(hover.seen)}`); + check( + ctx, + `7. the hover on one read "function one(): number" after ${hover.ms} ms`, + ); + + // 8. A code action from the host's TypeScript runs from the window. + const action = await until( + () => inSource(source, codeAction, state.active.resource), + "TypeScript's Remove unused declaration code action", + 60000, + ); + + assert.equal(action.result, true, JSON.stringify(action)); + assert.ok(action.diagnosticBefore, "no unused-declaration diagnostic"); + assert.equal(action.diagnosticAfter, false, JSON.stringify(action)); + check( + ctx, + `8. TypeScript's "${action.title}" (${action.command}) answered ${action.result} from the host and cleared its diagnostic`, + ); + + // 9. An added file's diff opens in the same window, its empty side the laptop's. + await inSource(ctx.page, openDiff, reviewId, "added.ts"); + + const diff = await until( + () => inSource(source, activeDiff), + "added.ts's diff in the Source window", + 60000, + ); + + assert.equal(diff.original.scheme, "vscode-userdata", JSON.stringify(diff)); + assert.equal(diff.original.text, ""); + assert.match( + diff.modified.uri, + new RegExp( + `^vscode-remote://${authority.replace("+", "%2B")}/.*/added\\.ts$`, + ), + ); + assert.match(diff.modified.text, /wbsource-added/); + assert.deepEqual(sourcePages(ctx), [source]); + check( + ctx, + `9. added.ts's diff opened in the same window: its original side is ${diff.original.uri} (empty), its modified side the host's`, + ); + + // 10. An Ask file link opens the same window at its line. + await inSource(source, focusFile, state.active.resource); + await inSource(source, cursorLine, 1); + await ctx.page + .locator('[aria-label="Session views"] button[aria-label="Whiteboard"]') + .filter({ visible: true }) + .click(); + + const link = await askLink(ctx, "where is `./f.ts:7`"); + + await link.click(); + await until( + async () => { + const at = await inSource(source, cursorLine); + + return at.resource?.endsWith("/f.ts") && at.line === 7; + }, + "f.ts at line 7 in the Source window", + 60000, + ); + assert.deepEqual(sourcePages(ctx), [source]); + check( + ctx, + `10. the Ask answer's ./f.ts:7 link opened f.ts at line 7 in the same Source window`, + ); + + // Extension-host RSS with one Source window, then two; checked at the end. + const rss = { one: await extensionHosts() }; + const other = await openFile(second); + + assert.equal(sourcePages(ctx).length, 2); + await until( + async () => (await extensionHosts()).length > rss.one.length, + "the second window's extension host", + 60000, + ); + rss.two = await extensionHosts(); + await other.keyboard.press("ControlOrMeta+Shift+KeyW"); + await until(() => other.isClosed(), "the second Source window to close"); + + // 11–12. The host freezes: the window's status bar says so, then names the host again. + const offlineText = `${alias} offline`; + + let thaw = await freeze(); + const paused = Date.now(); + + await until( + async () => (await hostEntry(source)) === offlineText, + "the Source window's status bar to read offline", + 30000, + ); + timings.offline = Date.now() - paused; + assert.ok(timings.offline <= 20000, `offline after ${timings.offline} ms`); + assert.equal((await inSource(source, windowStatus)).dialogs, 0); + check( + ctx, + `11. paused: the Source window's status bar read "${offlineText}" after ${timings.offline} ms, with no dialog`, + ); + + await thaw(); + + const resumed = Date.now(); + + await until( + async () => + (await hostEntry(source)) === alias && + (await inSource(source, readsCheckout).catch(() => false)), + "the Source window to reconnect", + 60000, + ); + timings.reconnect = Date.now() - resumed; + assert.ok( + timings.reconnect <= 30000, + `reconnected after ${timings.reconnect} ms`, + ); + check( + ctx, + `12. resumed: the status bar read "${alias}" again and the checkout answered after ${timings.reconnect} ms`, + ); + + // 13–14. A restart with the host paused: the restored window is offline, without a dialog, then reconnects. + thaw = await freeze(); + await ctx.quitAndRelaunchDesktop(); + + const relaunched = Date.now(); + + const restored = await until( + async () => { + for (const page of ctx.browser.contexts().flatMap((c) => c.pages())) + if ((await page.title().catch(() => "")) === state.title) return page; + + return null; + }, + "the restored Source window", + 60000, + ); + + // The harness takes the first workbench page, which can be the restored Source window. + ctx.page = ctx.browser + .contexts() + .flatMap((c) => c.pages()) + .find((page) => page !== restored && page.url().includes("workbench")); + await ctx.watchPage(ctx.page); + + await until( + async () => (await hostEntry(restored)) === offlineText, + "the restored window's status bar to read offline", + 120000, + ); + timings.restoredOffline = Date.now() - relaunched; + assert.equal( + (await inSource(restored, windowStatus)).dialogs, + 0, + "a dialog in the restored window", + ); + check( + ctx, + `13. after a restart with ${alias} paused, the restored Source window's status bar read "${offlineText}" ${timings.restoredOffline} ms after the relaunch, with no dialog`, + ); + + await thaw(); + + const unpaused = Date.now(); + + await until( + async () => + (await hostEntry(restored).catch(() => "")) === alias && + (await lines(restored).catch(() => "")).includes("return one() + 1;") && + (await inSource(restored, readsCheckout).catch(() => false)), + "the restored Source window to reconnect", + 90000, + ); + timings.restoredReconnect = Date.now() - unpaused; + assert.ok( + timings.restoredReconnect <= 30000, + `reconnected after ${timings.restoredReconnect} ms`, + ); + check( + ctx, + `14. resumed: the restored window's status bar read "${alias}" and it showed f.ts from the host ${timings.restoredReconnect} ms later`, + ); + + // 15. Removing the host closes its window. + await removeHost(ctx, alias); + await until(() => restored.isClosed(), "the Source window to close", 30000); + assert.deepEqual(sourcePages(ctx), []); + check(ctx, `15. removing ${alias} in Settings closed its Source window`); + + // 16. The host's extension hosts with one Source window, then two. + const total = (hosts) => mib(hosts.reduce((sum, [, kib]) => sum + kib, 0)); + const extra = rss.two.filter(([pid]) => !rss.one.some(([p]) => p === pid)); + + check( + ctx, + `16. extension hosts on ${alias}: the review window and one Source window ${rss.one.length} processes, ${rss.one.map(([, kib]) => `${mib(kib)} MiB`).join(" + ")} = ${total(rss.one)} MiB; two Source windows ${rss.two.length}, ${total(rss.two)} MiB (the second window's ${extra.map(([, kib]) => `${mib(kib)} MiB`).join(", ")})`, + ); +} + +// The functions below run in a window. + +/** TypeScript's quick fix for the unused declaration on line 6, run as a click would run it. */ +async function codeAction({ get, imp }, resource) { + const [commands, markers] = await Promise.all([ + get("vs/platform/commands/common/commands.js", "ICommandService"), + get("vs/platform/markers/common/markers.js", "IMarkerService"), + ]); + + const { URI } = await imp("vs/base/common/uri.js"); + const uri = URI.parse(resource); + + const unused = () => + markers + .read({ resource: uri }) + .find((m) => m.startLineNumber === 6 && /never read/.test(m.message)); + + const diagnostic = unused(); + + if (!diagnostic) return null; + + const actions = await commands.executeCommand( + "_executeCodeActionProvider", + uri, + { + startLineNumber: diagnostic.startLineNumber, + startColumn: diagnostic.startColumn, + endLineNumber: diagnostic.endLineNumber, + endColumn: diagnostic.endColumn, + }, + ); + + const action = actions?.find((a) => + /^Remove unused declaration/.test(a.title), + ); + + if (!action?.command) return null; + + const result = await commands.executeCommand( + action.command.id, + ...(action.command.arguments ?? []), + ); + + for (let frame = 0; unused() && frame < 120; frame++) + await new Promise((resolve) => requestAnimationFrame(resolve)); + + return { + title: action.title, + command: action.command.id, + result, + diagnosticBefore: diagnostic.message, + diagnosticAfter: !!unused(), + }; +} + +/** The canvas bridge's openDiff verb for `path`, on the review's current view. */ +async function openDiff({ get, imp }, reviewId, path) { + const { resolveReviewSourceView } = await imp( + "vs/review/common/reviewProtocol.js", + ); + + const source = await get( + "vs/review/services/reviewApiSourceService.js", + "IReviewApiSourceService", + ); + + const snapshot = await source.read(reviewId, "", { full: "true" }); + + await source.openDiff(resolveReviewSourceView(snapshot), path); + + return true; +} + +async function activeDiff({ get }) { + const editors = await get( + "vs/workbench/services/editor/common/editorService.js", + "IEditorService", + ); + + const control = editors.activeTextEditorControl; + const model = control?.getModel?.(); + + if (!model?.original || !model?.modified) return null; + + if (!model.modified.uri.path.endsWith("/added.ts")) return null; + + return { + original: { + scheme: model.original.uri.scheme, + uri: model.original.uri.toString(), + text: model.original.getValue(), + }, + modified: { + uri: model.modified.uri.toString(), + text: model.modified.getValue(), + }, + }; +} + +/** Moves the cursor to `line` when given; returns the active file and line. */ +async function cursorLine({ get }, line) { + const editor = ( + await get( + "vs/editor/browser/services/codeEditorService.js", + "ICodeEditorService", + ) + ).getActiveCodeEditor(); + + if (line) editor?.setPosition({ lineNumber: line, column: 1 }); + + return { + resource: editor?.getModel()?.uri.toString(), + line: editor?.getPosition()?.lineNumber, + }; +} + +async function readsCheckout({ get }) { + const [files, workspace] = await Promise.all([ + get("vs/platform/files/common/files.js", "IFileService"), + get( + "vs/platform/workspace/common/workspace.js", + "IWorkspaceContextService", + ), + ]); + + const folder = workspace.getWorkspace().folders[0].uri; + const stat = await files.resolve(folder); + + return stat.children?.some((child) => child.name === "f.ts") ?? false; +} + +/** Open dialogs, native or not, and the window's text. */ +async function windowStatus({ get }) { + const dialogs = await get( + "vs/platform/dialogs/common/dialogs.js", + "IDialogService", + ); + + return { + dialogs: dialogs.model.dialogs.length, + body: document.body.innerText, + }; +} + +/** Opens Ask on the review's sentence, asks `question`, and returns the answer's link for its file. */ +async function askLink(ctx, question) { + const panel = askPanel(ctx); + + await askAboutReviewSentence(ctx); + + const composer = panel.getByRole("combobox", { name: "Question" }); + + await composer.waitFor({ timeout: 30000 }); + await composer.fill(question); + await panel.getByRole("button", { name: "Ask", exact: true }).click(); + + const link = panel.getByRole("link").filter({ hasText: /^\.\/f\.ts:7$/ }); + + await link.waitFor({ timeout: 30000 }); + + return link; +} 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/source-window-helpers.mjs b/apps/review-desktop/scripts/e2e/remote/source-window-helpers.mjs new file mode 100644 index 000000000..9024b584a --- /dev/null +++ b/apps/review-desktop/scripts/e2e/remote/source-window-helpers.mjs @@ -0,0 +1,166 @@ +/** Helpers the Source-window journeys run against a Source window over CDP. */ + +import assert from "node:assert/strict"; + +/** + * Runs `fn` in the Source window with its services, reached through its + * extension service instance: the journey needs no hook in the product. + */ +export async function inSource(page, fn, ...args) { + const cdp = await page.context().newCDPSession(page); + + try { + await cdp.send("Emulation.setFocusEmulationEnabled", { enabled: true }); + + const prototype = await cdp.send("Runtime.evaluate", { + expression: + 'import(globalThis._VSCODE_FILE_ROOT + "vs/workbench/services/extensions/electron-browser/nativeExtensionService.js").then((m) => m.NativeExtensionService.prototype)', + awaitPromise: true, + }); + + const { objects } = await cdp.send("Runtime.queryObjects", { + prototypeObjectId: prototype.result.objectId, + }); + + const result = await cdp.send("Runtime.callFunctionOn", { + objectId: objects.objectId, + functionDeclaration: `async function (args) { + // Subclass prototypes have the prototype in their chain too. + const service = [...this].reverse().find((s) => Object.hasOwn(s, "_instantiationService")); + const imp = (file) => import(globalThis._VSCODE_FILE_ROOT + file); + const get = async (file, id) => { + const module = await imp(file); + return service._instantiationService.invokeFunction((a) => a.get(module[id])); + }; + const value = await (${fn.toString()})({ service, get, imp }, ...args); + return value === undefined ? undefined : JSON.parse(JSON.stringify(value)); + }`, + arguments: [{ value: args }], + awaitPromise: true, + returnByValue: true, + }); + + if (result.exceptionDetails) + throw new Error( + result.exceptionDetails.exception?.description ?? + result.exceptionDetails.text, + ); + + return result.result.value; + } finally { + await cdp.detach().catch(() => {}); + } +} + +/** Types into f.ts; returns the read-only notice, after checking the text did not change. */ +export async function typeInto(source, ctx) { + const before = await inSource(source, fileText); + + await source.locator(".monaco-editor .view-lines").first().click(); + await source.keyboard.type("x"); + + const message = await ctx.until( + () => + source.evaluate(() => + [...document.querySelectorAll(".monaco-editor-overlaymessage")] + .map((e) => e.innerText.trim()) + .find(Boolean), + ), + "the read-only notice", + 10000, + ); + + assert.equal(await inSource(source, fileText), before, "typing changed f.ts"); + + return { message }; +} + +export async function quickRows(page) { + return page + .locator(".quick-input-list .monaco-list-row") + .allInnerTexts() + .catch(() => []); +} + +// The functions below run in the Source window. + +export async function windowState({ get }) { + const [editors, workspace, environment] = await Promise.all([ + get( + "vs/workbench/services/editor/common/editorService.js", + "IEditorService", + ), + get("vs/platform/workspace/common/workspace.js", "IWorkspaceContextService"), + get( + "vs/workbench/services/environment/common/environmentService.js", + "IWorkbenchEnvironmentService", + ), + ]); + + return { + authority: environment.remoteAuthority, + title: document.title, + folder: workspace.getWorkspace().folders[0]?.uri.toString(), + active: { + resource: editors.activeEditor?.resource?.toString(), + readonly: !!editors.activeEditor?.isReadonly(), + }, + }; +} + +export async function fileText({ get }) { + const models = await get( + "vs/editor/common/services/model.js", + "IModelService", + ); + + return models + .getModels() + .find((m) => m.uri.scheme === "vscode-remote" && m.uri.path.endsWith("/f.ts")) + ?.getValue(); +} + +export async function focusFile({ get, imp }, resource) { + const editors = await get( + "vs/workbench/services/editor/common/editorService.js", + "IEditorService", + ); + + const { URI } = await imp("vs/base/common/uri.js"); + + await editors.openEditor({ + resource: URI.parse(resource), + options: { pinned: true }, + }); + + return true; +} + +export async function pointAt({ get }, line, word) { + const editor = ( + await get("vs/editor/browser/services/codeEditorService.js", "ICodeEditorService") + ).getActiveCodeEditor(); + + const position = { + lineNumber: line, + column: editor.getModel().getLineContent(line).indexOf(word) + 2, + }; + + editor.revealLineInCenter(line); + + let at = editor.getScrolledVisiblePosition(position); + + for (let frame = 0; !at && frame < 120; frame++) { + await new Promise((resolve) => requestAnimationFrame(resolve)); + at = editor.getScrolledVisiblePosition(position); + } + + if (!at) throw new Error(`line ${line} never rendered`); + + const rect = editor.getDomNode().getBoundingClientRect(); + + return { + x: Math.round(rect.left + at.left), + y: Math.round(rect.top + at.top + at.height / 2), + }; +} 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..4ccc33f56 100644 --- a/docs/remote-hosts.md +++ b/docs/remote-hosts.md @@ -56,9 +56,11 @@ never stored. ## How Desktop installs Whiteboard -Desktop first checks the host and writes nothing. If the host lacks Desktop's -version, Desktop asks once, then installs it as your user. The host downloads -it from `install.dev.fast` and checks its pinned checksum before unpacking. +Desktop first checks the host and writes nothing. If the host has no +Whiteboard, Desktop asks once, then installs as your user. A host with a +version Desktop installed gets Desktop's new version without a question. The +host downloads it from `install.dev.fast` and checks its pinned checksum before +unpacking. You do not need to start anything. Desktop starts the review server when it connects. On a remote with no Desktop, `whiteboard api` starts one too, so an @@ -77,9 +79,10 @@ instead of `~/.dev`. The review server keeps reviews in `~/.dev`. ### Updates -When Desktop updates, it installs its new version without asking. It restarts -the review server it started. It keeps the previous version and any version -still running. It leaves a server you started yourself running. +When Desktop updates, it installs its new version without asking on a host +where it installed Whiteboard before. It restarts the review server it started. +It keeps the previous version and any version still running. It leaves a server +you started yourself running. ### The setting @@ -88,7 +91,7 @@ not have its version: | Value | What Desktop does | |---|---| -| `ask` (the default) | Asks once per host. | +| `ask` (the default) | Asks once per host, unless Desktop installed Whiteboard there before. | | `always` | Installs without asking. | | `never` | Never installs, and uses the `whiteboard` on the host's `PATH`. | @@ -117,6 +120,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 connects to it sees the same history for its reviews. A file that +an answer names opens in a Source window on the host; see +[Source windows](#source-windows). + ## Remove Whiteboard from a host In Settings, click **Remove** on the host, check **Also remove Whiteboard from @@ -213,9 +224,25 @@ login shell (`~/.profile` or your shell's own start-up file is enough): The C# extension downloads its OmniSharp server from Microsoft the first time it starts. +## Source windows + +"Open file", the source tree and Ask's file links open a read-only Source +window on the host, with search and language features. + +When the host stops answering, the status bar says so and offers Retry. The +window reconnects when the host is back. + +Not available in a Source window: + +- Terminals and source control. +- Live file updates on musl hosts such as Alpine. The explorer refreshes when + the window gets focus. + +Each Source window runs its own extension host on the host. Close the ones you +are done with. + ## Not available for remote reviews yet -- Source windows: "Open file" and the source tree are hidden. - Sharing. - Traces. - Scratchpads. The scratchpad is always the laptop's. diff --git a/packages/review-protocol/scripts/generate-native-source.mjs b/packages/review-protocol/scripts/generate-native-source.mjs index ff78db185..f072630da 100644 --- a/packages/review-protocol/scripts/generate-native-source.mjs +++ b/packages/review-protocol/scripts/generate-native-source.mjs @@ -123,6 +123,7 @@ const MODULE_PATHS = [ path.join(sourceRoot, "structural-diff.ts"), path.join(sourceRoot, "source-alignment.ts"), path.join(sourceRoot, "contracts.ts"), + path.join(sourceRoot, "review-host-status.ts"), path.join(sourceRoot, "code-peek-diff.ts"), path.join(sourceRoot, "review-api-client.ts"), path.join(sourceRoot, "index.ts"), diff --git a/packages/review-protocol/src/contracts.ts b/packages/review-protocol/src/contracts.ts index 61eb359e7..b8d3f7174 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/packages/review-protocol/src/contracts.ts @@ -590,6 +590,14 @@ export interface ReviewCanvasSettingsContent { remoteHosts: ReviewRemoteHostsSettings; } +/** What a review or Home can do about the host a review lives on. */ +export interface ReviewRemoteHostActions { + states(): Promise; + retry(alias: string): Promise; + install(alias: string): Promise; + openSettings(): Promise; +} + export interface ReviewRemoteHostsSettings { enabled: boolean; configured: string[]; @@ -755,7 +763,9 @@ export type ReviewCanvasContent = reviewId: string; version?: number; host?: string; + hostState?: ReviewApiSummary["hostState"]; available?: ReviewApiSummary["available"]; + remoteHosts?: ReviewRemoteHostActions; bridge: ReviewCanvasBridge; setTitle?(title: string): void; setSourceView?( @@ -790,7 +800,7 @@ export type ReviewCanvasContent = // Boards in Trash can be restored for 30 days or deleted. deleteReview?(uuid: string): Promise; restoreReview?(uuid: string): Promise; - hostStates?(): Promise; + remoteHosts?: ReviewRemoteHostActions; // Opens the review and pins its read-only source tree open. Absent when // the host cannot show the tree. openSourceTree?(uuid: string): void; @@ -986,6 +996,14 @@ export interface ReviewGatewayHostState { declined?: true; } +/** `emptySide`: the diff side has no file; open one locally. */ +export interface ReviewRemoteNavigatorAnswer { + workspacePath: string; + filePath?: string; + serverId: string; + emptySide?: true; +} + export const ReviewRepositoryIdentitySchema = z.strictObject({ kind: z.enum(["git", "jj", "none"], { error: "must be git, jj, or none", diff --git a/packages/review-protocol/src/index.ts b/packages/review-protocol/src/index.ts index 0d171294f..d720cfe1b 100644 --- a/packages/review-protocol/src/index.ts +++ b/packages/review-protocol/src/index.ts @@ -56,6 +56,8 @@ export * from "./contracts.js"; export * from "./review-api-client.js"; +export * from "./review-host-status.js"; + export { type ByCommitEntry, type ReviewAgentTraceEvent, diff --git a/packages/review-protocol/src/native-source.test.ts b/packages/review-protocol/src/native-source.test.ts index 0e233f5be..7e9f7f487 100644 --- a/packages/review-protocol/src/native-source.test.ts +++ b/packages/review-protocol/src/native-source.test.ts @@ -79,6 +79,7 @@ describe("native Review Protocol source generation", () => { "structural-diff.ts", "source-alignment.ts", "contracts.ts", + "review-host-status.ts", "code-peek-diff.ts", "review-api-client.ts", "index.ts", @@ -166,6 +167,7 @@ describe("native Review Protocol source generation", () => { await writeFile(path.join(sourceRoot, "runtime-value.ts"), ""); await writeFile(path.join(sourceRoot, "json.ts"), ""); await writeFile(path.join(sourceRoot, "trace-contracts.ts"), ""); + await writeFile(path.join(sourceRoot, "review-host-status.ts"), ""); await writeFile(path.join(sourceRoot, "code-peek-diff.ts"), ""); await writeFile(path.join(sourceRoot, "review-api-client.ts"), ""); await writeFile( diff --git a/packages/review-protocol/src/review-api-client.ts b/packages/review-protocol/src/review-api-client.ts index dfd290424..d95b912e5 100644 --- a/packages/review-protocol/src/review-api-client.ts +++ b/packages/review-protocol/src/review-api-client.ts @@ -51,7 +51,11 @@ export interface ReviewApiSummary { working?: boolean; host?: string; hostState?: Exclude; - available?: { sourceWindows: boolean; languageFeatures: boolean }; + available?: { + sourceWindows: boolean; + /** Why they are off: the host's sentence, or its language features detail. */ + reason?: string; + }; } /** @@ -206,11 +210,7 @@ export class ReviewApiClient { if (!connections) liveConnections.set(this.request, (connections = new Map())); - const key = JSON.stringify([ - this.connection.serverUrl, - this.connection.token, - ]); - + const key = this.liveKey(); let live = connections.get(key); if (!live) { @@ -230,6 +230,13 @@ export class ReviewApiClient { disconnected, ); } + /** Reopen the shared stream now rather than after its backoff. */ + reconnect() { + liveConnections.get(this.request)?.get(this.liveKey())?.reconnect(); + } + private liveKey() { + return JSON.stringify([this.connection.serverUrl, this.connection.token]); + } } type Result = @@ -342,7 +349,7 @@ class LiveConnection { }, restartDelayMs); } - private reconnect() { + reconnect() { this.abort.abort(); this.abort = new AbortController(); const listeners = [...this.listeners]; diff --git a/packages/review-protocol/src/review-host-status.ts b/packages/review-protocol/src/review-host-status.ts new file mode 100644 index 000000000..da35e70aa --- /dev/null +++ b/packages/review-protocol/src/review-host-status.ts @@ -0,0 +1,76 @@ +import type { ReviewGatewayHostState } from "./contracts.js"; + +export interface ReviewHostStatus { + /** Short words for a chip or a list row, without the host name. */ + status: string; + /** The same with the host name, for a heading. */ + label: string; + sentence: string; + /** What helps: none while the host is busy or online. */ + action?: "retry" | "install" | "settings"; + busy?: true; +} + +type Wording = Pick & { + status: string; + label(alias: string): string; +}; + +const OFFLINE: Wording = { + status: "offline", + label: (alias) => `${alias} offline`, + action: "retry", +}; + +const UNUSABLE: Wording = { + status: "can't be used", + label: (alias) => `${alias} can't be used`, + action: "settings", +}; + +const STATUS: Record = { + online: { status: "online", label: (alias) => alias }, + connecting: { + status: "connecting", + label: (alias) => `Connecting to ${alias}…`, + busy: true, + }, + installing: { + status: "installing", + label: (alias) => `Installing on ${alias}…`, + busy: true, + }, + offline: OFFLINE, + unreachable: OFFLINE, + incompatible: { + status: "needs an update", + label: (alias) => `${alias} needs an update`, + action: "install", + }, + "not-installed": { + status: "not installed", + label: (alias) => `Whiteboard not installed on ${alias}`, + action: "install", + }, + "auth-failed": { + status: "can't sign in", + label: (alias) => `Can't sign in to ${alias}`, + action: "settings", + }, + unsupported: UNUSABLE, + duplicate: UNUSABLE, +}; + +/** One wording and one next step per host state, shared by every surface. */ +export function reviewHostStatus( + host: Pick, +): ReviewHostStatus { + const { label, ...rest } = STATUS[host.state]; + const words = label(host.alias); + + return { + ...rest, + label: words, + sentence: host.detail ?? (words.endsWith("…") ? words : `${words}.`), + }; +} diff --git a/packages/review/app/src/App.tsx b/packages/review/app/src/App.tsx index 7783ad158..eff65b9f3 100644 --- a/packages/review/app/src/App.tsx +++ b/packages/review/app/src/App.tsx @@ -295,12 +295,21 @@ function ReviewLayoutContent({ // The scratchpad is a document and nothing else: no source tree to browse, // nothing to share, nothing to dismiss. const scratchpad = session.review?.kind === "scratchpad"; - const sourceWindows = session.review?.available?.sourceWindows !== false; + const available = session.review?.available; + // A remote review keeps Source tree, disabled with the reason. + const sourceWindows = available?.sourceWindows !== false; + const sourceTreeShown = sourceWindows || !!session.review?.host; useEffect(() => { if (scratchpad) captureUiEvent(session, "scratchpad_opened"); }, [scratchpad, session]); const discordTooltip = useTooltip("Join our Discord community"); - const sourceTreeTooltip = useTooltip("Open full read-only source"); + + const sourceTreeTooltip = useTooltip( + sourceWindows + ? "Open full read-only source" + : (available?.reason ?? "Source windows are unavailable."), + ); + const panelStore = useReviewPanelStore(); useSuppressPanelMotionOnCanvasResume(appRef); const activePanel = useReviewPanel((state) => state.active); @@ -614,20 +623,21 @@ function ReviewLayoutContent({ shellStyles.topbarContext, )} > - {!scratchpad && sourceWindows && ( + {!scratchpad && sourceTreeShown && ( )} - )} + ) : null} ); @@ -520,41 +555,43 @@ export function ApiCanvas({

)} {trashBanner} - - + - - - -
- -
- - - + + +
+ +
+ + + + - - + + @@ -578,6 +615,7 @@ const CanvasDocument = memo(function CanvasDocument({ softwareMapEnabled: boolean; documentWidth?: ReviewDocumentWidthChoice; }) { + const host = useReviewSession().review?.host; const snapshot = data.snapshot; const document: RenderedReviewDocument = { @@ -615,7 +653,7 @@ const CanvasDocument = memo(function CanvasDocument({ // Commits views hide, as for a review whose base is its head. range={{ sourceUnavailable: snapshot.sourceUnavailable - ? "Local checkout unavailable." + ? checkoutUnavailable(host) : undefined, baseRef: snapshot.pins?.base ?? "", headRef: snapshot.pins?.head ?? "", diff --git a/packages/review/app/src/ask-agent-picker.tsx b/packages/review/app/src/ask-agent-picker.tsx index aa8dada21..a289724d8 100644 --- a/packages/review/app/src/ask-agent-picker.tsx +++ b/packages/review/app/src/ask-agent-picker.tsx @@ -37,6 +37,7 @@ import { textStyles } from "./ui/text"; import { fieldStyles } from "./ui/text-field"; import { useAnchoredPopover } from "./use-anchored-popover"; import { useDismissOnOutside } from "./use-dismiss-on-outside"; +import { useTooltip } from "./use-tooltip"; const agentsSchema = z.object({ agents: z.array( @@ -63,26 +64,37 @@ export const logos: Record< // request under way, which the toolbar and the panel share. const knownAgents = new WeakMap(); -const askingAgents = new WeakMap>(); +// Undefined: the host could not say now. +const askingAgents = new WeakMap< + ReviewSession, + Promise +>(); -function askAgentsOf(session: ReviewSession): Promise { +function askAgentsOf( + session: ReviewSession, +): Promise { let request = askingAgents.get(session); if (!request) { request = session .fetch("/ask/agents") - .then(async (response) => - response.ok ? agentsSchema.parse(await response.json()).agents : null, + .then(async (response) => { + // A host without Ask refuses the request; one that is down fails it. + if (response.status >= 500) throw new Error(response.statusText); + + return response.ok + ? agentsSchema.parse(await response.json()).agents + : null; + }) + .then( + (agents) => { + knownAgents.set(session, agents); + + return agents; + }, + () => knownAgents.get(session), ) - // A check that fails keeps the last answer; a host without Ask refuses - // the request instead. - .catch(() => knownAgents.get(session) ?? null) - .then((agents) => { - knownAgents.set(session, agents); - askingAgents.delete(session); - - return agents; - }); + .finally(() => askingAgents.delete(session)); askingAgents.set(session, request); } @@ -90,8 +102,9 @@ function askAgentsOf(session: ReviewSession): Promise { } /** Which local agents can answer, or null where this host has no Ask. Asked - * again each time Ask opens and each time the review comes back into focus, - * so an agent installed during the review shows up. */ + * again each time Ask opens, each time the review comes back into focus and + * each time its host changes state, so an agent installed during the review + * shows up. A host that cannot answer now keeps the last answer. */ export function useAskAgents(session: ReviewSession | null): AskAgent[] | null { const [agents, setAgents] = useState(() => session ? (knownAgents.get(session) ?? null) : null, @@ -103,7 +116,7 @@ export function useAskAgents(session: ReviewSession | null): AskAgent[] | null { const refresh = () => { void askAgentsOf(session).then((value) => { - if (current) setAgents(value); + if (current && value !== undefined) setAgents(value); }); }; @@ -393,25 +406,32 @@ export function AskAgentPicker({ agents, agent, locked, + unavailable, onPick, }: { agents: AskAgent[] | null; agent: AskAgentId | undefined; locked: boolean; + unavailable?: string; onPick: (agent: AskAgentId) => void; }): ReactElement { const [open, setOpen] = useState(false); const anchor = useRef(null); const trigger = useRef(null); const dismiss = useMenuClose(setOpen, trigger); + const hint = useTooltip(unavailable ?? ""); const chosen = agents?.find((candidate) => candidate.id === agent); if ( locked || + unavailable !== undefined || agents?.filter((candidate) => candidate.available).length === 1 ) { return ( - + {agent ? logos[agent]({}) : null} {chosen?.name ?? agent ?? "Agent"} diff --git a/packages/review/app/src/ask-files.test.tsx b/packages/review/app/src/ask-files.test.tsx index bf8b48495..20b1261d6 100644 --- a/packages/review/app/src/ask-files.test.tsx +++ b/packages/review/app/src/ask-files.test.tsx @@ -122,3 +122,65 @@ 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, + reason: "devbox offline.", + }; + + 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.closest("[title]")?.getAttribute("title")).toBe( + "devbox offline.", + ); + 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..53ea2669f 100644 --- a/packages/review/app/src/ask-files.tsx +++ b/packages/review/app/src/ask-files.tsx @@ -141,16 +141,23 @@ function AskFileLink({ children: ReactNode; }): ReactNode { const files = useContext(AskFilesContext); + const available = useReviewSession().review?.available; + const canOpen = 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 && available?.reason) + 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..7256bf703 100644 --- a/packages/review/app/src/ask-panel.test.tsx +++ b/packages/review/app/src/ask-panel.test.tsx @@ -10,7 +10,7 @@ import { useShowOpenThread, } from "./ask-delete"; import { AskHistoryProvider, useAskHistory } from "./ask-history"; -import { AskHistoryList } from "./ask-history-list"; +import { AskHistoryControl, AskHistoryList } from "./ask-history-list"; import { AskPanelContent, AskReadOnlyThread } from "./ask-panel"; import { TestCanvasQuery } from "./canvas-query-test-utils"; import { ReviewSessionProvider } from "./host/review-session"; @@ -1501,3 +1501,199 @@ it("shows a saved conversation whose checkout is gone, with nothing to ask", asy vi.unstubAllGlobals(); } }); + +const twoAgents = () => + Response.json({ + agents: [ + { id: "claude", name: "Claude Code", available: true }, + { id: "codex", name: "Codex", available: true }, + ], + }); + +it("asks nothing while a remote review's host is down, and says why with its next step", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const session = testReviewSession(); + + const hosts = { + states: async () => [ + { alias: "wb-a", state: "offline" as const, detail: "ssh timed out." }, + ], + retry: vi.fn<(alias: string) => Promise>(async () => {}), + install: async () => {}, + openSettings: async () => {}, + }; + + session.review = { + ...session.review!, + host: "wb-a", + hostState: "offline", + hosts, + }; + + const fetch = vi + .spyOn(session, "fetch") + .mockImplementation(async (endpoint) => + endpoint === "/ask/agents" + ? twoAgents() + : Response.json({ ok: true }, { status: 404 }), + ); + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + try { + await act(async () => + root.render( + + + , + ), + ); + + const textarea = container.querySelector("textarea")!; + + expect(textarea.disabled).toBe(true); + expect(container.querySelector('[role="status"]')?.textContent).toContain( + "wb-a offline · ssh timed out.", + ); + + await type(textarea, "Is this safe?"); + await act(async () => textarea.form!.requestSubmit()); + expect(fetch.mock.calls.some(([called]) => called === "/ask")).toBe(false); + + await act(async () => buttonNamed(container, "Retry")!.click()); + expect(hosts.retry).toHaveBeenCalledWith("wb-a"); + } finally { + await act(async () => root.unmount()); + container.remove(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + } +}); + +it("offers to try again a question that did not go, and sends it again", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const session = testReviewSession(); + let refused = false; + + const fetch = vi + .spyOn(session, "fetch") + .mockImplementation(async (endpoint, init) => { + if (endpoint === "/ask/agents") return twoAgents(); + + if (endpoint === "/ask") { + if (refused) return Response.json({ threadId: "thread" }); + refused = true; + + return Response.json({ error: "Busy." }, { status: 500 }); + } + + if (endpoint === "/ask/watch") return new Response(new ReadableStream()); + + return Response.json({ ok: true }, { status: init?.method ? 200 : 404 }); + }); + + const asked = () => + fetch.mock.calls + .filter(([called]) => called === "/ask") + .map(([, init]) => JSON.parse(String(init?.body)).question); + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + try { + await act(async () => + root.render( + + + , + ), + ); + + const textarea = container.querySelector("textarea")!; + + await type(textarea, "Is this safe?"); + await enter(textarea); + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + "Busy.", + ); + + await act(async () => buttonNamed(container, "Try again")!.click()); + expect(asked()).toEqual([ + { text: "Is this safe?" }, + { text: "Is this safe?" }, + ]); + expect(container.querySelector('[role="alert"]')).toBeNull(); + expect(textarea.value).toBe(""); + } finally { + await act(async () => root.unmount()); + container.remove(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + } +}); + +it("keeps Ask while its host cannot list agents, and lets an agent be picked again once the host is back", async () => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const session = testReviewSession(); + const header = document.createElement("div"); + document.body.append(header); + let hostState: "online" | "offline" = "online"; + + vi.spyOn(session, "fetch").mockImplementation(async (endpoint) => { + if (endpoint === "/ask/agents") + return hostState === "online" + ? twoAgents() + : Response.json({ error: "wb-a is offline." }, { status: 503 }); + + return Response.json({ ok: true }, { status: 404 }); + }); + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + const picker = () => + header.querySelector('button[aria-haspopup="menu"]:not([aria-label])'); + + // The catalog hands the canvas a new session as the host changes state. + const render = async () => + act(async () => + root.render( + + + + + + , + ), + ); + + try { + await render(); + expect(picker()).not.toBeNull(); + + hostState = "offline"; + await render(); + expect(picker()).toBeNull(); + expect(header.textContent).toBe("Claude Code"); + expect(buttonNamed(container, "Saved conversations")).not.toBeNull(); + + hostState = "online"; + await render(); + expect(picker()).not.toBeNull(); + } finally { + await act(async () => root.unmount()); + container.remove(); + header.remove(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + } +}); diff --git a/packages/review/app/src/ask-panel.tsx b/packages/review/app/src/ask-panel.tsx index cf139e683..5549ebe6e 100644 --- a/packages/review/app/src/ask-panel.tsx +++ b/packages/review/app/src/ask-panel.tsx @@ -1,3 +1,7 @@ +import type { + ReviewHostStatus, + ReviewRemoteHostActions, +} from "@dev.fast/review-protocol"; import type { AgentSelection } from "@review/agent-selection"; import { type AskAgentId, @@ -51,10 +55,12 @@ import { askPanelStyles } from "./ask-styles"; import { useLatest, useThread } from "./ask-thread-stream"; import { AskAgentTurn, AskWorking, turns } from "./ask-turn"; import { canvasQueryKeys } from "./canvas-query"; +import { useHostActionButton } from "./connection-chip"; import { controlStyles } from "./controls-styles"; import { formatRelativeTime } from "./format"; import { useReviewSession } from "./host/review-session"; import { ArrowUpIcon, ImageIcon, LockIcon } from "./icons"; +import { useReviewHostDown } from "./remote-host-state"; import { useAskKey, useOptionalReviewPanelStore } from "./review-panel"; import type { AskPresence, AskReport } from "./review-panel-model"; import { fontSize, radius } from "./scale.stylex"; @@ -110,6 +116,8 @@ export function AskPanelContent({ const [threadId, setThreadId] = useState(null); const [sending, setSending] = useState(false); const [requestError, setRequestError] = useState(null); + const [again, setAgain] = useState<() => void>(); + const down = useReviewHostDown(session.review); // Choices for a question not yet asked; a thread says its own. const [picks, setPicks] = useState({}); const [bypassPick, setBypassPick] = useState(); @@ -234,6 +242,7 @@ export function AskPanelContent({ const canAsk = !session.review?.trashedAt && Boolean(agent) && + !down && (!busy || (answering && !sending)); const post = useCallback( @@ -255,6 +264,11 @@ export function AskPanelContent({ [session], ); + const fail = useCallback((message: string, rerun: () => void) => { + setRequestError(message); + setAgain(() => rerun); + }, []); + const ask = async (question: AskQuestion) => { if (!agent || !canAsk || thread?.status === "failed") return false; setSending(true); @@ -291,7 +305,10 @@ export function AskPanelContent({ return true; } catch (error) { - setRequestError(error instanceof Error ? error.message : String(error)); + // The question is back in the composer; sending it again sends that. + fail(error instanceof Error ? error.message : String(error), () => + composer.current?.form?.requestSubmit(), + ); return false; } finally { @@ -319,17 +336,20 @@ export function AskPanelContent({ ); const decide = useCallback( - (permissionId: string, optionId: string) => + function decide(permissionId: string, optionId: string) { void post(`/ask/${threadId}/permission`, { permissionId, optionId, - }).catch((error: Error) => setRequestError(error.message)), - [post, threadId], + }).catch((error: Error) => + fail(error.message, () => decide(permissionId, optionId)), + ); + }, + [post, threadId, fail], ); const stop = () => void post(`/ask/${threadId}/cancel`).catch((error: Error) => - setRequestError(error.message), + fail(error.message, stop), ); const chosen = agents?.find((candidate) => candidate.id === agent); @@ -374,7 +394,7 @@ export function AskPanelContent({ } void post(`/ask/${threadId}/choice`, { kind, value }).catch( - (error: Error) => setRequestError(error.message), + (error: Error) => fail(error.message, () => choose(kind, value)), ); }; @@ -397,7 +417,7 @@ export function AskPanelContent({ } void post(`/ask/${threadId}/permissions`, { bypass: value }).catch( - (error: Error) => setRequestError(error.message), + (error: Error) => fail(error.message, () => permit(value)), ); }; @@ -407,7 +427,7 @@ export function AskPanelContent({ requestError ?? thread?.error ?? (lost - ? `Whiteboard lost its connection to ${thread?.agentName ?? chosenName}.` + ? `Whiteboard lost its connection to ${session.review?.host ?? thread?.agentName ?? chosenName}.` : null); // Saved once its agent started a session; before that, nothing reopens it. @@ -454,6 +474,22 @@ export function AskPanelContent({ }) : undefined; + const errorAction = reconnect + ? { label: "Reconnect", run: reconnect } + : retry + ? { label: "Try again", run: retry } + : startOver + ? { label: "Start a new conversation", run: startOver } + : requestError !== null && again && !down + ? { + label: "Try again", + run: () => { + setRequestError(null); + again(); + }, + } + : undefined; + // Reopening starts the agent and loads its session. Whiteboard's saved // copy shows meanwhile; one saved before that copy waits for the replay. const connecting = @@ -594,6 +630,7 @@ export function AskPanelContent({ agents={agents} agent={agent} locked={threadId !== null || savedThreadId !== undefined} + unavailable={down?.label} onPick={(picked) => { setAgent(picked); setPicks({}); @@ -605,6 +642,13 @@ export function AskPanelContent({ return (
{header ? createPortal(agentPicker, header) : agentPicker} + {down ? ( + + ) : null}
@@ -651,19 +695,15 @@ export function AskPanelContent({ ) : error ? (

{error} - {reconnect || retry || startOver ? ( + {errorAction ? ( <> {" "} ) : null} @@ -685,7 +725,7 @@ export function AskPanelContent({ ? "/ for commands, @ for files" : "@ for files", )} - disabled={thread?.status === "failed"} + disabled={thread?.status === "failed" || down !== undefined} canAsk={canAsk} stop={busy && threadId ? stop : undefined} connecting={thread?.status === "starting"} @@ -779,6 +819,25 @@ export function AskReadOnlyThread({ ); } +function AskHostBanner({ + status, + host, + hosts, +}: { + status: ReviewHostStatus; + host?: string; + hosts?: ReviewRemoteHostActions; +}): ReactElement { + const { failure, button } = useHostActionButton(hosts, host, status.action); + const why = failure ?? status.sentence; + + return ( + + {host && why.includes(host) ? why : `${status.label} · ${why}`} + + ); +} + function ToLatestButton({ onClick }: { onClick: () => void }): ReactElement { const label = "Scroll to the latest"; 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/app/src/authoring-activity.tsx b/packages/review/app/src/authoring-activity.tsx index 3edaf32c8..3ae0b096c 100644 --- a/packages/review/app/src/authoring-activity.tsx +++ b/packages/review/app/src/authoring-activity.tsx @@ -7,6 +7,7 @@ import { useContext, useState } from "react"; import { agentColor } from "./agent-colors"; import { AuthoringActivityContext } from "./authoring-activity-context"; import { surfaceOf } from "./authoring-cursor"; +import { ConnectionChip, ConnectionContext } from "./connection-chip"; import { AuthoringCursorContext, LensCursorContext, @@ -35,11 +36,14 @@ export function AuthoringActivityBadge({ onLocate?(view: "review" | "diff"): void; }) { const activity = useContext(AuthoringActivityContext); + const lost = useContext(ConnectionContext) !== undefined; const tooltip = useTooltip( "Activity updates stopped. This does not mean the agent finished.", ); + if (lost) return ; + if (!activity) return null; // The class is a marker for tests. diff --git a/packages/review/app/src/connection-chip.tsx b/packages/review/app/src/connection-chip.tsx new file mode 100644 index 000000000..f9d32da44 --- /dev/null +++ b/packages/review/app/src/connection-chip.tsx @@ -0,0 +1,239 @@ +import { fontSize, fontWeight, motion, radius } from "@canvas/scale.stylex"; +import { Button } from "@canvas/ui/button"; +import { + type ReviewHostStatus, + type ReviewRemoteHostActions, + reviewHostStatus, +} from "@dev.fast/review-protocol"; +import * as stylex from "@stylexjs/stylex"; +import { createContext, useContext } from "react"; + +import { RefreshIcon } from "./icons"; +import { + ACTION_WORDS, + useHostAction, + useRemoteHostState, +} from "./remote-host-state"; +import { withClass } from "./stylex-props"; +import { tokens } from "./tokens.stylex"; +import { useTooltip } from "./use-tooltip"; + +export interface LostConnection { + host?: string; + hosts?: ReviewRemoteHostActions; + detail: string; + /** Reopen the review stream after a retry. */ + reconnect(): void; +} + +export const ConnectionContext = createContext( + undefined, +); + +/** Shown while the review stream is down; on a remote review it offers the host's next step. */ +export function ConnectionChip() { + const connection = useContext(ConnectionContext); + const { host, hosts } = connection ?? {}; + const state = useRemoteHostState(hosts, host, connection !== undefined); + + const status = + state && state.state !== "online" ? reviewHostStatus(state) : undefined; + + const action = status ? status.action : host && hosts ? "retry" : undefined; + + const { run, pending, failure } = useHostAction( + hosts, + host, + connection?.reconnect, + ); + + const tooltip = useTooltip( + action ? `Click to ${ACTION_WORDS[action].toLowerCase()}` : "Reconnecting…", + { detail: failure ?? status?.sentence ?? connection?.detail }, + ); + + if (!connection) return null; + + const label = + status?.label ?? (host ? `${host} disconnected` : "Disconnected"); + + const spinning = pending || status?.busy; + + if (!action) + return ( + + {label} + {spinning ? ( + + ) : null} + + ); + + return ( + + ); +} + +/** A host's next step as a button; `failure` says why the last one failed. */ +export function useHostActionButton( + hosts: ReviewRemoteHostActions | undefined, + alias: string | undefined, + action: ReviewHostStatus["action"], + retried?: () => void, +) { + const { run, pending, failure } = useHostAction(hosts, alias, retried); + + const button = + action && hosts ? ( + + ) : null; + + return { failure, button }; +} + +/** A remote review that has not loaded: what its host is doing, and the next step. */ +export function HostWaiting({ + host, + hosts, + reconnect, + lost, +}: { + host: string; + hosts: ReviewRemoteHostActions; + reconnect(): void; + lost?: string; +}) { + const state = useRemoteHostState(hosts, host, true); + + const status = + state && state.state !== "online" ? reviewHostStatus(state) : undefined; + + const { failure, button } = useHostActionButton( + hosts, + host, + status?.action, + reconnect, + ); + + if (!status) + return lost === undefined ? null : ( +

    + Connection lost. Reconnecting… {lost} +

    + ); + + return ( +
    +

    {status.label}

    +

    + {failure ?? status.sentence} +

    + {button} +
    + ); +} + +const REDUCED = "@media (prefers-reduced-motion: reduce)"; + +const spin = stylex.keyframes({ + to: { transform: "rotate(360deg)" }, +}); + +const styles = stylex.create({ + chip: { + display: "inline-flex", + flex: "0 1 auto", + minWidth: 0, + maxWidth: "320px", + alignItems: "center", + gap: "6px", + height: tokens.chromeControlHeight, + marginInline: "4px", + padding: "0 10px", + borderWidth: "1px", + borderStyle: "solid", + borderColor: tokens.warningOutline, + borderRadius: radius.pill, + backgroundColor: tokens.warningWash, + color: tokens.changeModified, + font: `${fontWeight.semibold} ${fontSize.small} ${tokens.fontMono}`, + whiteSpace: "nowrap", + }, + // A long alias ends in an ellipsis; the tooltip has it all. + text: { + minWidth: 0, + overflow: "hidden", + textOverflow: "ellipsis", + }, + button: { + paddingInlineEnd: "7px", + cursor: { default: "pointer", ":disabled": "progress" }, + filter: { default: null, ":hover:not(:disabled)": "brightness(1.15)" }, + outline: { + default: null, + ":focus-visible": `1px solid ${tokens.changeModified}`, + }, + outlineOffset: { default: null, ":focus-visible": "1px" }, + }, + waiting: { + display: "flex", + flexDirection: "column", + alignItems: "flex-start", + gap: "12px", + maxWidth: "72ch", + margin: "32px auto", + padding: "0 24px", + font: `${fontSize.reading}/1.6 ${tokens.fontDisplay}`, + }, + waitingLabel: { + margin: 0, + color: tokens.changeModified, + font: `${fontWeight.semibold} ${fontSize.reading} ${tokens.fontMono}`, + }, + waitingSentence: { + margin: 0, + color: tokens.inkMuted, + }, + action: { + textDecorationLine: "underline", + textUnderlineOffset: "2px", + }, + icon: { + flexShrink: 0, + width: "13px", + height: "13px", + }, + spinning: { + animationName: { default: spin, [REDUCED]: "none" }, + animationDuration: { default: motion.pulse, [REDUCED]: motion.instant }, + animationTimingFunction: "linear", + animationIterationCount: "infinite", + }, +}); diff --git a/packages/review/app/src/desktop-entry.tsx b/packages/review/app/src/desktop-entry.tsx index 27c8be728..f57499ab7 100644 --- a/packages/review/app/src/desktop-entry.tsx +++ b/packages/review/app/src/desktop-entry.tsx @@ -129,7 +129,7 @@ function Home({ onRestore={ restoreReview ? (review) => restoreReview(review.reviewId) : undefined } - hostStates={content.hostStates} + remoteHosts={content.remoteHosts} onboarding={content.onboarding} onOpenTutorial={content.openTutorial} onOpenSettings={content.openSettings} diff --git a/packages/review/app/src/host/review-session-data.ts b/packages/review/app/src/host/review-session-data.ts index f2618882c..96ae6fc93 100644 --- a/packages/review/app/src/host/review-session-data.ts +++ b/packages/review/app/src/host/review-session-data.ts @@ -1,6 +1,7 @@ import type { LoadedAgentTrace } from "@canvas/use-agent-trace"; import type { ReviewApiSummary, + ReviewRemoteHostActions, ReviewStackLayer, } from "@dev.fast/review-protocol"; @@ -9,6 +10,8 @@ export interface ReviewSessionData { /** Absent for a review. The scratchpad hides review-only chrome. */ kind?: "scratchpad"; host?: string; + hostState?: ReviewApiSummary["hostState"]; + hosts?: ReviewRemoteHostActions; available?: ReviewApiSummary["available"]; /** Absent for a document whose references all carry their own pins. */ pins?: { base: string; head: string }; diff --git a/packages/review/app/src/missing-checkout-banner.browser.test.tsx b/packages/review/app/src/missing-checkout-banner.browser.test.tsx new file mode 100644 index 000000000..c914f5055 --- /dev/null +++ b/packages/review/app/src/missing-checkout-banner.browser.test.tsx @@ -0,0 +1,44 @@ +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { expect, it } from "vitest"; + +import { ReviewSessionProvider } from "./host/review-session"; +import { MissingCheckoutBanner } from "./missing-checkout-banner"; +import { ReviewProvider } from "./review-context"; +import { testReviewSession } from "./review-session-test-utils"; + +it("names a remote review's host and shows a failed move to Trash in the banner", async () => { + const session = testReviewSession(); + session.review!.host = "devbox"; + session.review!.dismiss = async () => { + throw new Error("devbox offline."); + }; + + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + + try { + await act(async () => + root.render( + + + + + , + ), + ); + + expect(container.textContent).toContain( + "The checkout on devbox is unavailable.", + ); + await act(async () => container.querySelector("button")!.click()); + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + "devbox offline.", + ); + expect(container.querySelector("button")!.disabled).toBe(false); + } finally { + await act(async () => root.unmount()); + container.remove(); + } +}); diff --git a/packages/review/app/src/missing-checkout-banner.tsx b/packages/review/app/src/missing-checkout-banner.tsx index 5aca6f282..6ad07dddf 100644 --- a/packages/review/app/src/missing-checkout-banner.tsx +++ b/packages/review/app/src/missing-checkout-banner.tsx @@ -5,6 +5,11 @@ import { type ReactElement, useState } from "react"; import { useReviewSession } from "./host/review-session"; import { useReviewActions, useReviewState } from "./review-context"; +export const checkoutUnavailable = (host?: string) => + host + ? `The checkout on ${host} is unavailable.` + : "Local checkout unavailable."; + export function MissingCheckoutBanner({ worktree, }: { @@ -13,7 +18,9 @@ export function MissingCheckoutBanner({ const session = useReviewSession(); const { dismissReview } = useReviewActions(); const { submissionOutcome } = useReviewState(); + const host = useReviewSession().review?.host; const [busy, setBusy] = useState(false); + const [error, setError] = useState(); return ( { setBusy(true); + setError(undefined); try { await dismissReview(); - } catch (error) { - console.error("Review action failed", error); + } catch (failure) { + setError( + `Could not move to Trash: ${failure instanceof Error ? failure.message : String(failure)}`, + ); setBusy(false); } }} @@ -40,8 +50,9 @@ export function MissingCheckoutBanner({ > {worktree ? "This review's worktree was removed." - : "Local checkout unavailable."}{" "} + : checkoutUnavailable(host)}{" "} Showing the source saved with the review. + {error && {error}} ); } diff --git a/packages/review/app/src/remote-host-state.ts b/packages/review/app/src/remote-host-state.ts new file mode 100644 index 000000000..9ccfd7e6c --- /dev/null +++ b/packages/review/app/src/remote-host-state.ts @@ -0,0 +1,139 @@ +import { + type ReviewGatewayHostState, + type ReviewHostStatus, + type ReviewRemoteHostActions, + reviewHostStatus, +} from "@dev.fast/review-protocol"; +import { type RefObject, useEffect, useState } from "react"; + +import type { ReviewSessionData } from "./host/review-session-data"; + +const STATES_EVERY_MS = 3000; + +export function useRemoteHostStates( + hosts: Pick | undefined, + watch = true, +) { + const [states, setStates] = useState(); + + useEffect(() => { + if (!hosts || !watch) return; + let live = true; + let reading = false; + + const read = () => { + if (document.hidden || reading) return; + reading = true; + void hosts + .states() + .then((next) => { + if (live) setStates(next); + }) + .catch(() => undefined) + .finally(() => { + reading = false; + }); + }; + + read(); + const timer = setInterval(read, STATES_EVERY_MS); + + return () => { + live = false; + clearInterval(timer); + }; + }, [hosts, watch]); + + return watch ? states : undefined; +} + +export function useRemoteHostState( + hosts: ReviewRemoteHostActions | undefined, + alias: string | undefined, + watch: boolean, +) { + return useRemoteHostStates(alias ? hosts : undefined, watch)?.find( + (host) => host.alias === alias, + ); +} + +/** A remote review's host while it is not online, with the reason read live. */ +export function useReviewHostDown( + review: Pick | undefined, +): ReviewHostStatus | undefined { + const { host, hostState, hosts } = review ?? {}; + + const down = + host !== undefined && hostState !== undefined && hostState !== "online"; + + const live = useRemoteHostState(hosts, host, down); + + if (!down) return undefined; + + return reviewHostStatus( + live && live.state !== "online" ? live : { alias: host, state: hostState }, + ); +} + +export const ACTION_WORDS: Record< + NonNullable, + string +> = { retry: "Retry", install: "Install", settings: "Open Settings" }; + +/** Runs a host's next step; Settings opens scrolled to Remote hosts. */ +export function useHostAction( + hosts: + | (Pick & + Partial>) + | undefined, + alias: string | undefined, + retried?: () => void, +) { + const [pending, setPending] = useState(false); + const [failure, setFailure] = useState(); + + const run = (action: NonNullable) => { + if (!hosts || !alias) return; + setPending(true); + setFailure(undefined); + + const step = + action === "retry" + ? hosts.retry(alias).then(retried) + : action === "install" + ? hosts.install(alias) + : Promise.resolve(hosts.openSettings?.()).then(revealRemoteHosts); + + void step + .catch((error: Error) => setFailure(error.message)) + .finally(() => setPending(false)); + }; + + return { run, pending, failure }; +} + +// Every canvas tab shares this module, so a review can ask the Settings tab, +// open or about to mount, to scroll to Remote hosts. +let requested = false; + +const reveals = new EventTarget(); + +function revealRemoteHosts() { + requested = true; + reveals.dispatchEvent(new Event("reveal")); +} + +export function useSettingsReveal(ref: RefObject) { + useEffect(() => { + const reveal = () => { + if (!requested || !ref.current) return; + requested = false; + ref.current.scrollIntoView({ block: "start" }); + }; + + reveal(); + reveals.addEventListener("reveal", reveal); + + return () => reveals.removeEventListener("reveal", reveal); + }, [ref]); +} diff --git a/packages/review/app/src/remote-hosts-section.browser.test.tsx b/packages/review/app/src/remote-hosts-section.browser.test.tsx index 3f7e5d709..b4a78eb8b 100644 --- a/packages/review/app/src/remote-hosts-section.browser.test.tsx +++ b/packages/review/app/src/remote-hosts-section.browser.test.tsx @@ -221,9 +221,9 @@ test("lists an online host's language groups after its language features, each w expect(rows()[1]).not.toContain("rust"); }); -test("offers Retry to a host that failed to authenticate, is unreachable, not installed or unsupported", async () => { +test("offers Retry to a host that failed to authenticate, is offline or unreachable, not installed or unsupported", async () => { const hosts = remoteHosts( - ["devbox", "box2", "box3", "box4", "box5"], + ["devbox", "box2", "box3", "box4", "box5", "box6"], [ { alias: "devbox", state: "auth-failed", detail: "Permission denied" }, { alias: "box2", state: "unreachable", detail: "timed out" }, @@ -234,6 +234,7 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in detail: "This host runs glibc 2.31; Whiteboard needs 2.34 or newer.", }, { alias: "box5", state: "installing" }, + { alias: "box6", state: "offline" }, ], ); @@ -243,7 +244,6 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in page.getByRole("button", { name: "Retry devbox" }).elements(), ).toHaveLength(1), ); - expect(rows()[3]).toContain("unsupported"); expect(rows()[3]).toContain("glibc 2.31"); expect(rows()[4]).toContain("installing"); expect( @@ -251,7 +251,13 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in .getByRole("button", { name: /^Retry / }) .elements() .map((button) => button.getAttribute("aria-label")), - ).toEqual(["Retry devbox", "Retry box2", "Retry box3", "Retry box4"]); + ).toEqual([ + "Retry devbox", + "Retry box2", + "Retry box3", + "Retry box4", + "Retry box6", + ]); await page.getByRole("button", { name: "Retry box2" }).click(); expect(hosts.retry).toHaveBeenCalledWith("box2"); await page.getByRole("button", { name: "Retry box3" }).click(); @@ -292,9 +298,7 @@ test("shows the install step, offers Install to a declined host, and Retry after await render(hosts); await vi.waitFor(() => - expect(rows()[0]).toContain( - "installing · Installing the Whiteboard package.", - ), + expect(rows()[0]).toContain("Installing the Whiteboard package."), ); expect( page @@ -480,13 +484,9 @@ test("a failed host's detail copies on click; a healthy one does not", async () ), ); - await vi.waitFor(() => - expect(rows()[1]).toContain("unreachable · ssh: timed out"), - ); + await vi.waitFor(() => expect(rows()[1]).toContain("ssh: timed out")); expect(page.getByRole("button", { name: "online" }).query()).toBeNull(); - await page - .getByRole("button", { name: "unreachable · ssh: timed out" }) - .click(); - expect(writeText).toHaveBeenCalledWith("unreachable · ssh: timed out"); + await page.getByRole("button", { name: "ssh: timed out" }).click(); + expect(writeText).toHaveBeenCalledWith("ssh: timed out"); }); diff --git a/packages/review/app/src/remote-hosts-section.tsx b/packages/review/app/src/remote-hosts-section.tsx index 375bfb079..de5bc1659 100644 --- a/packages/review/app/src/remote-hosts-section.tsx +++ b/packages/review/app/src/remote-hosts-section.tsx @@ -3,40 +3,38 @@ import { Checkbox } from "@canvas/ui/checkbox"; import { TextField } from "@canvas/ui/text-field"; import { REVIEW_CLI_INSTALL_TARGET_LABELS, - type ReviewGatewayHostState, type ReviewRemoteAgent, type ReviewRemoteHostsSettings, + reviewHostStatus, } from "@dev.fast/review-protocol"; import * as stylex from "@stylexjs/stylex"; -import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { CopyableText } from "./copy-text"; +import { + ACTION_WORDS, + useHostAction, + useRemoteHostStates, + useSettingsReveal, +} from "./remote-host-state"; import { settingsStyles as styles } from "./settings-styles"; import { tokens } from "./tokens.stylex"; -const STATES_EVERY_MS = 3000; - -const RETRIED = new Set([ - "auth-failed", - "incompatible", - "not-installed", - "unreachable", - "unsupported", -]); - export function RemoteHostsSection({ hosts, }: { hosts: ReviewRemoteHostsSettings; }) { const [configured, setConfigured] = useState(hosts.configured); - const [states, setStates] = useState(); + const states = useRemoteHostStates(hosts); const [suggestions, setSuggestions] = useState([]); const [alias, setAlias] = useState(""); const [busy, setBusy] = useState(false); const [error, setError] = useState(); const [removing, setRemoving] = useState(); const [uninstall, setUninstall] = useState(false); + const section = useRef(null); + useSettingsReveal(section); useEffect(() => { void hosts @@ -45,33 +43,6 @@ export function RemoteHostsSection({ .catch(() => undefined); }, [hosts]); - useEffect(() => { - let live = true; - let reading = false; - - const read = () => { - if (document.hidden || reading) return; - reading = true; - void hosts - .states() - .then((next) => { - if (live) setStates(next); - }) - .catch(() => undefined) - .finally(() => { - reading = false; - }); - }; - - read(); - const timer = setInterval(read, STATES_EVERY_MS); - - return () => { - live = false; - clearInterval(timer); - }; - }, [hosts]); - const save = async (next: string[]) => { setBusy(true); setError(undefined); @@ -113,10 +84,15 @@ export function RemoteHostsSection({ }; return ( -
    +

    Remote hosts

    {configured.map((name) => { const state = states?.find((host) => host.alias === name); + const failed = !!state && !!reviewHostStatus(state).action; return (
    @@ -124,8 +100,13 @@ export function RemoteHostsSection({ {name} {states ? ( ) : null} {state?.state === "online" ? ( @@ -162,22 +143,10 @@ export function RemoteHostsSection({
    {state?.declined ? ( - + ) : null} - {state && RETRIED.has(state.state) ? ( - + {failed ? ( + ) : null} + + ); +} + function Detail({ text, failed, diff --git a/packages/review/app/src/review-home-styles.ts b/packages/review/app/src/review-home-styles.ts index 7b7d7e02c..e1b8be461 100644 --- a/packages/review/app/src/review-home-styles.ts +++ b/packages/review/app/src/review-home-styles.ts @@ -316,6 +316,12 @@ export const styles = stylex.create({ textAlign: "left", }, unavailableRow: { opacity: 0.55 }, + hostNotice: { + display: "flex", + alignItems: "center", + flexWrap: "wrap", + gap: "12px", + }, time: { color: tokens.inkFaint, fontVariantNumeric: "tabular-nums", diff --git a/packages/review/app/src/review-home-view.browser.test.tsx b/packages/review/app/src/review-home-view.browser.test.tsx index 519d792e5..82dde9b03 100644 --- a/packages/review/app/src/review-home-view.browser.test.tsx +++ b/packages/review/app/src/review-home-view.browser.test.tsx @@ -1,6 +1,7 @@ import type { ReviewApiSummary, ReviewGatewayHostState, + ReviewRemoteHostActions, } from "@dev.fast/review-protocol"; import { act } from "react"; import { type Root, createRoot } from "react-dom/client"; @@ -905,23 +906,23 @@ describe("Home interactions", () => { expect(titles()).toEqual(["One"]); }); it.each([ - ["offline", "offline"], - ["connecting", "connecting"], - ["not-installed", "not installed"], + ["offline", "offline", "devbox offline"], + ["connecting", "connecting", "Connecting to devbox…"], + ["not-installed", "not installed", "Whiteboard not installed on devbox"], ] as const)( "draws a %s host's review as unavailable, and opening it shows the host's detail", - async (hostState, words) => { + async (hostState, words, label) => { const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); await render([remote("Remote", { hostState })], { onOpen, - hostStates: async () => [ + remoteHosts: hostsOf(async () => [ { alias: "devbox", state: hostState, detail: "devbox is offline: it did not answer within 3 seconds.", }, - ], + ]), }); const row = container.querySelector("[data-home-row]")!; @@ -933,27 +934,31 @@ describe("Home interactions", () => { button("Open Remote") .querySelector("span[title]")! .getAttribute("title"), - ).toBe(`devbox is ${words}`); + ).toBe(label); await click("Open Remote"); expect(onOpen).not.toHaveBeenCalled(); - expect(alert()).toBe( - "devbox is offline: it did not answer within 3 seconds.", + await vi.waitFor(() => + expect(alert()).toContain( + "devbox is offline: it did not answer within 3 seconds.", + ), ); }, ); - it("says only the state while the host's detail loads or when it has none", async () => { + it("says the state and offers its next step while the host's detail loads or when it has none", async () => { const answer = Promise.withResolvers(); await render([remote("Remote", { hostState: "offline" })], { - hostStates: () => answer.promise, + remoteHosts: hostsOf(() => answer.promise), }); await click("Open Remote"); - expect(alert()).toBe("devbox is offline."); + expect(alert()).toContain("devbox offline."); + expect(button("Retry")).toBeTruthy(); await act(async () => answer.resolve([{ alias: "devbox", state: "offline" }]), ); - expect(alert()).toBe("devbox is offline."); + expect(alert()).toContain("devbox offline."); + expect(button("Retry")).toBeTruthy(); }); - it("keeps the message of the last unavailable review opened, and drops it when the list changes", async () => { + it("keeps the message of the last unavailable review opened, and opens that review once its host is back", async () => { const answers = [ Promise.withResolvers(), Promise.withResolvers(), @@ -972,14 +977,18 @@ describe("Home interactions", () => { { alias: "other", state: "offline" as const, detail: "other detail" }, ]; - await render(reviews, { hostStates }); + const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); + const remoteHosts = hostsOf(hostStates); + await render(reviews, { onOpen, remoteHosts }); await click("Open One"); await click("Open Two"); await act(async () => answers[1]!.resolve(states)); await act(async () => answers[0]!.resolve(states)); - expect(alert()).toBe("other detail"); - await render([{ ...reviews[1]!, hostState: "online" }], { hostStates }); + expect(alert()).toBe("other detailRetry"); + const back = { ...reviews[1]!, hostState: "online" as const }; + await render([reviews[0]!, back], { onOpen, remoteHosts }); expect(alert()).toBeUndefined(); + expect(onOpen).toHaveBeenCalledExactlyOnceWith(back); }); it("draws an online remote review as available", async () => { const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); @@ -994,6 +1003,17 @@ describe("Home interactions", () => { expect(onOpen).toHaveBeenCalledWith(review); }); + function hostsOf( + states: () => Promise, + ): ReviewRemoteHostActions { + return { + states, + retry: async () => {}, + install: async () => {}, + openSettings: async () => {}, + }; + } + function alert() { return container.querySelector('[role="alert"]')?.textContent; } @@ -1006,7 +1026,7 @@ function remote( return summary(title, { host: "devbox", hostState: "online", - available: { sourceWindows: false, languageFeatures: false }, + available: { sourceWindows: false }, ...extra, }); } diff --git a/packages/review/app/src/review-home-view.tsx b/packages/review/app/src/review-home-view.tsx index 9c130962d..c34af084b 100644 --- a/packages/review/app/src/review-home-view.tsx +++ b/packages/review/app/src/review-home-view.tsx @@ -2,8 +2,9 @@ import { REVIEW_DISCORD_URL, type ReviewApiSummary, type ReviewCanvasOnboarding, - type ReviewGatewayHostState, + type ReviewRemoteHostActions, homeSessions, + reviewHostStatus, } from "@dev.fast/review-protocol"; import { fuzzySegments } from "@review/fuzzy-match"; import * as stylex from "@stylexjs/stylex"; @@ -19,6 +20,7 @@ import { useState, } from "react"; +import { useHostActionButton } from "./connection-chip"; import { CopyableText, copyText } from "./copy-text"; import { countLabel, formatRelativeTime } from "./format"; import { homeStyles } from "./home-styles"; @@ -41,6 +43,7 @@ import { } from "./icons"; import { homeGroupMarker, homeRowMarker } from "./markers.stylex"; import { useNotify } from "./notify"; +import { useReviewHostDown } from "./remote-host-state"; import type { ReviewFindHost } from "./review-find"; import { HomeFilterPicker, @@ -88,7 +91,7 @@ interface ReviewHomeProps { onDelete?(review: ReviewApiSummary): Promise; onTrash?(review: ReviewApiSummary): Promise; onRestore?(review: ReviewApiSummary): Promise; - hostStates?(): Promise; + remoteHosts?: ReviewRemoteHostActions; onboarding?: ReviewCanvasOnboarding; onOpenTutorial?(): void; onOpenSettings?(): void; @@ -138,7 +141,7 @@ export function ReviewHome({ onDelete, onTrash, onRestore, - hostStates, + remoteHosts, onboarding, onOpenTutorial, onOpenSettings, @@ -182,33 +185,29 @@ export function ReviewHome({ const [busy, setBusy] = useState(false); const busyRef = useRef(false); const [error, setError] = useState(""); - const [hostMessage, setHostMessage] = useState(); - const hostMessageGeneration = useRef(0); + // The unavailable review last opened; it opens once its host is back. + const [waiting, setWaiting] = useState(); useEffect(() => { - hostMessageGeneration.current++; - setHostMessage(undefined); - }, [reviews]); - - const open = useCallback( - async (review: ReviewApiSummary) => { - const generation = ++hostMessageGeneration.current; + if (!waiting) return; - if (!unavailable(review)) { - setHostMessage(undefined); - onOpen(review); + const current = reviews.find( + (review) => review.reviewId === waiting.reviewId, + ); - return; - } + if (current && unavailable(current)) return setWaiting(current); + setWaiting(undefined); - setHostMessage(`${review.host} is ${hostStateWords(review)}.`); - const states = await hostStates?.().catch(() => undefined); - const detail = states?.find((host) => host.alias === review.host)?.detail; + if (current) onOpen(current); + }, [reviews]); - if (detail && generation === hostMessageGeneration.current) - setHostMessage(detail); + const open = useCallback( + (review: ReviewApiSummary) => { + if (unavailable(review)) return setWaiting(review); + setWaiting(undefined); + onOpen(review); }, - [onOpen, hostStates], + [onOpen], ); const [notice, setNotice] = useState(null); @@ -1064,11 +1063,7 @@ export function ReviewHome({ ) : null} {error &&

    {error}

    } - {hostMessage ? ( -

    - -

    - ) : null} + {waiting ? : null} {!visible.length && ( - {hostStateWords(review)} + {hostStatus(review).status} ); @@ -1542,8 +1537,36 @@ function unavailable(review: ReviewApiSummary): boolean { return review.hostState !== undefined && review.hostState !== "online"; } -function hostStateWords(review: ReviewApiSummary): string { - return review.hostState?.replace("-", " ") ?? ""; +function hostStatus(review: ReviewApiSummary) { + return reviewHostStatus({ + alias: review.host ?? "", + state: review.hostState ?? "online", + }); +} + +function HostNotice({ + review, + hosts, +}: { + review: ReviewApiSummary; + hosts?: ReviewRemoteHostActions; +}) { + const status = useReviewHostDown({ ...review, hosts }); + + const { failure, button } = useHostActionButton( + hosts, + review.host, + status?.action, + ); + + if (!status) return null; + + return ( +

    + + {button} +

    + ); } function PullRequestPill({ diff --git a/packages/review/app/src/review-lenses.tsx b/packages/review/app/src/review-lenses.tsx index daf31a037..af75b94eb 100644 --- a/packages/review/app/src/review-lenses.tsx +++ b/packages/review/app/src/review-lenses.tsx @@ -32,6 +32,7 @@ import { import { canvasQueryKeys } from "./canvas-query"; import { useReviewSession } from "./host/review-session"; +import { checkoutUnavailable } from "./missing-checkout-banner"; import { useReviewPanel, useReviewPanelStore } from "./review-panel"; import { captureUiEvent } from "./ui-telemetry"; @@ -184,7 +185,7 @@ export function ReviewLensesProvider({ // The latest outcome wins: a later read clears a failed mark. const error = snapshot.sourceUnavailable - ? "Local checkout unavailable." + ? checkoutUnavailable(session.review?.host) : markViewed.error && markViewed.submittedAt > read.dataUpdatedAt ? String(markViewed.error) : read.error diff --git a/packages/review/app/src/shell-styles.ts b/packages/review/app/src/shell-styles.ts index 3909a8f7d..530acf27f 100644 --- a/packages/review/app/src/shell-styles.ts +++ b/packages/review/app/src/shell-styles.ts @@ -247,9 +247,10 @@ export const shellStyles = stylex.create({ }, }, // The scratchpad has no source tree or pins to set apart, so no rule. + // Shrinks before the action row scrolls, so its badges end in an ellipsis. topbarContext: { display: "flex", - flex: "0 0 auto", + flex: "0 1 auto", minWidth: 0, alignItems: "center", marginRight: "auto", @@ -266,6 +267,7 @@ export const shellStyles = stylex.create({ // the viewport: an open side panel narrows the column without resizing // the window. In a narrow column this ghost Button drops its label. openSourceTree: { + flexShrink: 0, justifyContent: { default: null, [narrowContent]: "center" }, width: { default: null, [narrowContent]: tokens.chromeControlHeight }, padding: { default: "0 10px", [narrowContent]: 0 }, diff --git a/packages/review/src/ask/agents.test.ts b/packages/review/src/ask/agents.test.ts new file mode 100644 index 000000000..fafb8f329 --- /dev/null +++ b/packages/review/src/ask/agents.test.ts @@ -0,0 +1,43 @@ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { afterEach, beforeEach, expect, it, vi } from "vitest"; + +import { askAgentTakesMcp } from "./agents.js"; + +let root: string; + +beforeEach(async () => { + root = await mkdtemp(path.join(tmpdir(), "review-ask-agents-")); + vi.stubEnv("HOME", root); + vi.stubEnv("PATH", "/usr/bin:/bin"); +}); + +afterEach(async () => { + vi.unstubAllEnvs(); + await rm(root, { recursive: true, force: true }); +}); + +it.skipIf(process.platform === "win32")( + "gives MCP to a Pi found, and run, only on the PATH it is handed", + async () => { + const bin = path.join(root, "login-bin"); + await mkdir(bin); + await writeFile( + path.join(bin, "wbtest-node"), + '#!/bin/sh\nexec /bin/sh "$@"\n', + { mode: 0o755 }, + ); + await writeFile( + path.join(bin, "pi"), + "#!/usr/bin/env wbtest-node\necho 0.99.0\n", + { mode: 0o755 }, + ); + + expect(await askAgentTakesMcp("pi", { PATH: `${bin}:/usr/bin:/bin` })).toBe( + true, + ); + expect(await askAgentTakesMcp("pi")).toBe(false); + }, +); diff --git a/packages/review/src/ask/agents.ts b/packages/review/src/ask/agents.ts index acc83aaea..f1ecfc761 100644 --- a/packages/review/src/ask/agents.ts +++ b/packages/review/src/ask/agents.ts @@ -61,7 +61,7 @@ interface AskAgentSpec { interface AskAgentMcp { /** Whether this install can take them. */ - supported(executable: string): Promise; + supported(executable: string, env: NodeJS.ProcessEnv): Promise; /** What the adapter runs in place of the user's CLI to give them, with * what it needs in `env`. */ launch( @@ -206,6 +206,7 @@ export type AskAgentLauncher = ( bypass?: boolean; /** The MCP servers its sessions are given. */ mcpServers?: McpServer[]; + env?: NodeJS.ProcessEnv; }, ) => Promise; @@ -256,13 +257,16 @@ async function findAgent( } /** Whether the agent's model gets the MCP servers its sessions are given. */ -export async function askAgentTakesMcp(agent: AskAgentId): Promise { +export async function askAgentTakesMcp( + agent: AskAgentId, + env: NodeJS.ProcessEnv = process.env, +): Promise { const spec = askAgents[agent]; if (!spec.mcp) return true; - const executable = await findAgent(spec); + const executable = await findAgent(spec, env); - return executable !== undefined && spec.mcp.supported(executable); + return executable !== undefined && spec.mcp.supported(executable, env); } export async function detectAskAgents( @@ -286,15 +290,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), }; @@ -309,7 +313,9 @@ export const launchAskAgent: AskAgentLauncher = async ( if ("adapter" in launch) { env[launch.executableEnv] = - spec.mcp && mcpServers.length && (await spec.mcp.supported(executable)) + spec.mcp && + mcpServers.length && + (await spec.mcp.supported(executable, env)) ? await spec.mcp.launch(executable, mcpServers, env) : executable; diff --git a/packages/review/src/ask/pi-mcp.ts b/packages/review/src/ask/pi-mcp.ts index 4f7de4427..0a1a479f9 100644 --- a/packages/review/src/ask/pi-mcp.ts +++ b/packages/review/src/ask/pi-mcp.ts @@ -49,13 +49,14 @@ const versions = new Map< { read: number; version: Promise } >(); -function piVersion(executable: string) { +function piVersion(executable: string, env: NodeJS.ProcessEnv) { const cached = versions.get(executable); if (cached && Date.now() - cached.read < VERSION_TTL_MS) return cached.version; const version = promisify(execFile)(executable, ["--version"], { + env, timeout: 5_000, }) .then(({ stdout }) => @@ -69,9 +70,12 @@ function piVersion(executable: string) { } /** Whether this Pi can be given MCP servers. */ -export async function piTakesMcp(executable: string): Promise { +export async function piTakesMcp( + executable: string, + env: NodeJS.ProcessEnv, +): Promise { if (process.platform === "win32") return false; - const version = await piVersion(executable); + const version = await piVersion(executable, env); if (!version) return false; 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/http.ts b/packages/review/src/review-api/http.ts index c8d3aa54d..9195e6e56 100644 --- a/packages/review/src/review-api/http.ts +++ b/packages/review/src/review-api/http.ts @@ -3,6 +3,7 @@ import { REVIEW_CLIENT_HEADER, REVIEW_CLIENT_REMOTE, type ReviewRemoteLanguageEnvironment, + type ReviewRemoteNavigatorAnswer, type ReviewStreamLine, type ReviewStructuralDiffEvent, } from "@dev.fast/review-protocol"; @@ -739,16 +740,24 @@ export function createReviewApi( }) .parse(context.req.query()); - return context.json( - await data.navigatorWorkspace( - readReview(context.req.param("id"), input.version), - { - ...input, - empty: input.empty === "true", - anchor: queryAnchor(input), - }, - ), + const answer = await data.navigatorWorkspace( + readReview(context.req.param("id"), input.version), + { + ...input, + empty: input.empty === "true", + anchor: queryAnchor(input), + }, ); + + if (context.req.header(REVIEW_CLIENT_HEADER) !== REVIEW_CLIENT_REMOTE) + return context.json(answer); + + return context.json({ + ...(input.empty === "true" + ? { workspacePath: answer.workspacePath, emptySide: true } + : answer), + serverId: store.serverId(), + } satisfies ReviewRemoteNavigatorAnswer); }); app.get("/:id/tree", async (context) => { const input = readQuerySchemas.tree.parse(context.req.query()); diff --git a/packages/review/src/review-api/local-data.test.ts b/packages/review/src/review-api/local-data.test.ts index 0d0d868a8..111280578 100644 --- a/packages/review/src/review-api/local-data.test.ts +++ b/packages/review/src/review-api/local-data.test.ts @@ -1013,6 +1013,49 @@ it("gives a remote caller a commit review's checkout as remoteRootPath, with the expect(await read({ [REVIEW_CLIENT_HEADER]: "local" })).toEqual(ownMachine); }); +it("answers a remote caller's navigator with the host paths a local caller gets, and its server id", async () => { + const { reviewId } = await local.store.execute( + command({ + type: "create", + title: "Remote navigator", + target: { kind: "commits", ...pins }, + }), + ); + + const app = createReviewApi(local.store, local.data); + + const open = async ( + query: Record, + headers: Record = {}, + ) => { + const response = await app.request( + `/${reviewId}/navigator?${new URLSearchParams(query)}`, + { method: "POST", headers }, + ); + + expect(response.status).toBe(200); + + return response.json(); + }; + + const remote = { [REVIEW_CLIENT_HEADER]: REVIEW_CLIENT_REMOTE }; + const file = { version: "0", side: "base", file: source.file }; + const { serverId, ...answer } = await open(file, remote); + const { folders } = JSON.parse(readFileSync(answer.workspacePath, "utf8")); + expect(serverId).toBe(local.store.serverId()); + expect(answer).toEqual(await open(file)); + expect(answer.filePath).toBe(path.join(folders[0].path, source.file)); + expect(readFileSync(answer.filePath, "utf8")).toContain("value = 1"); + + const added = { ...file, file: "added.ts", empty: "true" }; + expect(await open(added, remote)).toEqual({ + workspacePath: answer.workspacePath, + emptySide: true, + serverId, + }); + expect((await open(added)).filePath).toEqual(expect.any(String)); +}); + 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..d3de0471d 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,10 @@ 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(" "), - }; + 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..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, @@ -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, @@ -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,130 @@ 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 server = await start( + undefined, + false, + async (_agent, _cwd, options) => { + launched.resolve(options?.env); + throw new Error("No agent here."); + }, + ); + + 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, + }; +} + +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("opens an Ask thread in the review's checkout and closes it on stop", async () => { + 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); + + 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 vi.waitFor(() => expect(launches).toBe(1)); + expect(stopped).toBe(0); + await server.stop(); + expect(stopped).toBe(1); + + while (!(await reader.read()).done); +}); diff --git a/packages/review/src/server/headless-host.ts b/packages/review/src/server/headless-host.ts index 7d589aa1e..a33c5c0a4 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,60 @@ 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. */ +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), + }; +} + +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 +200,13 @@ 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 { app, api, close } = createWhiteboardCore({ profile: local, relay, token: discovery.token, @@ -148,6 +215,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: agentEnv(), + }, }); app.route("/reviews-api", api); @@ -177,6 +252,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-list.test.ts b/packages/review/src/server/review-gateway-list.test.ts index 94fc5343f..92f3c2559 100644 --- a/packages/review/src/server/review-gateway-list.test.ts +++ b/packages/review/src/server/review-gateway-list.test.ts @@ -64,11 +64,10 @@ it("lists this machine first, then each machine in the setting's order", () => { ]); expect(merged[1]?.available).toEqual({ sourceWindows: false, - languageFeatures: false, }); }); -it("offers language features only for an online host whose Desktop reported them", () => { +it("offers language features and Source windows only for an online host whose Desktop reported language features", () => { const merged = mergeLists( "structural", [], @@ -83,12 +82,10 @@ it("offers language features only for an online host whose Desktop reported them ), ); - expect( - merged.map((review) => [review.host, review.available?.languageFeatures]), - ).toEqual([ - ["a", true], - ["b", false], - ["c", false], + expect(merged.map((review) => [review.host, review.available])).toEqual([ + ["a", { sourceWindows: true }], + ["b", { sourceWindows: false }], + ["c", { sourceWindows: false, reason: "c offline." }], ]); }); @@ -187,3 +184,48 @@ it("gives an id to this machine, then to the first host in the setting's order t ]); expect(listedBy(onBoth.reviewId, lists)?.state.alias).toBe("b"); }); + +it("says why Source windows are off: the host's sentence while it is down, else the language features detail", () => { + const merged = mergeLists( + "structural", + [], + source( + [ + { alias: "a", serverId: "A", state: "online", languageFeatures: true }, + { + alias: "b", + serverId: "B", + state: "online", + languageFeatures: false, + languageFeaturesDetail: "The Whiteboard on b has no VS Code server.", + }, + { alias: "c", serverId: "C", state: "offline", languageFeatures: true }, + { + alias: "d", + serverId: "D", + state: "auth-failed", + detail: "Permission denied.", + }, + { alias: "e", serverId: "E", state: "online", detail: "stale" }, + ], + { + A: [entry("on a")], + B: [entry("on b")], + C: [entry("on c")], + D: [entry("on d")], + E: [entry("on e")], + }, + ["A", "B"], + ), + ); + + expect( + merged.map((review) => [review.host, review.available?.reason]), + ).toEqual([ + ["a", undefined], + ["b", "The Whiteboard on b has no VS Code server."], + ["c", "c offline."], + ["d", "Permission denied."], + ["e", "e offline."], + ]); +}); diff --git a/packages/review/src/server/review-gateway-list.ts b/packages/review/src/server/review-gateway-list.ts index 9be14aee1..9c477a23e 100644 --- a/packages/review/src/server/review-gateway-list.ts +++ b/packages/review/src/server/review-gateway-list.ts @@ -1,6 +1,7 @@ -import type { - ReviewApiSummary, - ReviewGatewayHostState, +import { + type ReviewApiSummary, + type ReviewGatewayHostState, + reviewHostStatus, } from "@dev.fast/review-protocol"; import { LIST_MODES, type ListMode } from "./review-gateway-memory.js"; @@ -18,6 +19,18 @@ function decorate( hostState: NonNullable, ): ReviewApiSummary { const { alias } = state; + const online = hostState === "online"; + const features = online && state.languageFeatures === true; + + const reason = online + ? features + ? undefined + : state.languageFeaturesDetail + : reviewHostStatus({ + alias, + state: hostState, + detail: hostState === state.state ? state.detail : undefined, + }).sentence; return { ...entry, @@ -30,9 +43,8 @@ function decorate( host: alias, hostState, available: { - sourceWindows: false, - languageFeatures: - hostState === "online" && state.languageFeatures === true, + sourceWindows: features, + ...(reason && { reason }), }, }; } diff --git a/packages/review/src/server/review-gateway-streams.test.ts b/packages/review/src/server/review-gateway-streams.test.ts index d176c46b4..5546154bf 100644 --- a/packages/review/src/server/review-gateway-streams.test.ts +++ b/packages/review/src/server/review-gateway-streams.test.ts @@ -186,7 +186,7 @@ it("lists this machine's reviews, then each host's in the setting's order, with }, host: "wb-a", hostState: "online", - available: { sourceWindows: false, languageFeatures: false }, + available: { sourceWindows: false }, }); const onLocal = merged.find((review) => review.title === "On this machine")!; diff --git a/packages/review/src/server/review-gateway-streams.ts b/packages/review/src/server/review-gateway-streams.ts index 1edcb128b..2e0b1a7c3 100644 --- a/packages/review/src/server/review-gateway-streams.ts +++ b/packages/review/src/server/review-gateway-streams.ts @@ -5,6 +5,7 @@ import { type ReviewApiSummary, type ReviewGatewayHostState, parseJsonText, + reviewHostStatus, } from "@dev.fast/review-protocol"; import { coverageModeSchema } from "@review/review-api/review-progress.js"; import { z } from "zod"; @@ -47,7 +48,7 @@ export type Located = | undefined; export const downDetail = (down: ReviewGatewayHostState) => - down.detail ?? `${down.alias} is ${down.state}.`; + reviewHostStatus(down).sentence; const watchPath = (subscriptions: object[]) => `/reviews-api/watch?subscriptions=${encodeURIComponent(JSON.stringify(subscriptions))}`; diff --git a/packages/review/src/server/review-gateway-transport.ts b/packages/review/src/server/review-gateway-transport.ts index ea47c81ab..6ec8da389 100644 --- a/packages/review/src/server/review-gateway-transport.ts +++ b/packages/review/src/server/review-gateway-transport.ts @@ -182,6 +182,7 @@ const ERROR_WORDS = new Map([ ["ECONNREFUSED", "it refused the connection"], ["ECONNRESET", "it closed the connection"], ["UND_ERR_SOCKET", "it closed the connection"], + ["EPIPE", "it closed the connection"], ["ETIMEDOUT", "it did not answer"], ["UND_ERR_HEADERS_TIMEOUT", NO_ANSWER], ]); @@ -199,5 +200,5 @@ export function errorCode(cause: unknown): string { export function errorText(cause: unknown): string { const code = errorCode(cause); - return ERROR_WORDS.get(code) ?? code; + return ERROR_WORDS.get(code) ?? `it failed (${code})`; } diff --git a/packages/review/src/server/review-gateway.ts b/packages/review/src/server/review-gateway.ts index 0791cde59..55a5026ab 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; } @@ -67,8 +67,26 @@ const REMOTE_ROUTES: readonly RemoteRoute[] = [ { method: "GET", path: "file" }, { method: "GET", path: "resources/:name" }, { method: "GET", path: "maps/:name" }, + { method: "POST", path: "navigator" }, { 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; @@ -248,7 +266,7 @@ export function createReviewGateway(input: { return answer(remote.alias, timedOut ? 504 : 502, { ok: false, - error: `${remote.alias} did not answer: ${reason}.`, + error: `Could not reach ${remote.alias}: ${reason}.`, }); } 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..6d6620640 100644 --- a/packages/review/src/server/review-server-core.ts +++ b/packages/review/src/server/review-server-core.ts @@ -4,7 +4,12 @@ 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, + askAgentTakesMcp, + detectAskAgents, + launchAskAgent, +} from "@review/ask/agents.js"; import { AskThreads, type AskTools } from "@review/ask/threads.js"; import { readBuildCommit, @@ -128,7 +133,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?: Promise; + }; } export function createWhiteboardCore(input: WhiteboardCoreInput) { @@ -143,8 +154,20 @@ 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( + async (agent, cwd, options) => + launch(agent, cwd, { ...options, env: await ask.env }), + { + takesMcp: async (agent) => askAgentTakesMcp(agent, await ask.env), + ...ask.tools, + }, + ); const closeDeletedThreads = () => askThreads?.closeDeletedReviews((id) => @@ -171,7 +194,10 @@ export function createWhiteboardCore(input: WhiteboardCoreInput) { () => traceMachineEnabled(), input.status, input.hooks, - askThreads && { threads: askThreads, agents: () => detectAskAgents() }, + askThreads && { + threads: askThreads, + agents: async () => detectAskAgents(await ask?.env), + }, ); // A shared store mounts the publisher with the rest of sharing.