From 2be311658210f7ac154050c8158cf15e98ac9381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 11:52:33 -0300 Subject: [PATCH 1/6] refactor(review-tutor): move host-neutral extension helpers into src The extension keeps its argv, notifications and forbidden-API boundary while argument parsing, process execution, repository resolution, browser opening and the server lifecycle move to src/cli-support.ts so a CLI can share them. The skill path now resolves from the package root for both TS source and dist, and connector discovery gains one home for its bounds and scrubbed environment. --- .../review-tutor/extensions/review-tutor.ts | 187 ++---------------- packages/review-tutor/src/cli-support.ts | 180 +++++++++++++++++ packages/review-tutor/src/connectors/codex.ts | 40 ++-- .../review-tutor/src/connectors/discovery.ts | 46 +++++ packages/review-tutor/src/paths.ts | 10 + packages/review-tutor/test/connectors.test.ts | 4 +- 6 files changed, 264 insertions(+), 203 deletions(-) create mode 100644 packages/review-tutor/src/cli-support.ts create mode 100644 packages/review-tutor/src/connectors/discovery.ts diff --git a/packages/review-tutor/extensions/review-tutor.ts b/packages/review-tutor/extensions/review-tutor.ts index c513fa1..7bb8993 100644 --- a/packages/review-tutor/extensions/review-tutor.ts +++ b/packages/review-tutor/extensions/review-tutor.ts @@ -1,130 +1,23 @@ -import { execFile as nodeExecFile } from "node:child_process"; import { realpath } from "node:fs/promises"; -import { fileURLToPath } from "node:url"; import type { ExtensionAPI, ExtensionCommandContext, } from "@earendil-works/pi-coding-agent"; -import { createConnectorRegistry } from "../src/connectors/registry.ts"; -import type { ExecFile } from "../src/inputs.ts"; -import type { ModelChoice, SourceRequest } from "../src/protocol.ts"; import { - startReviewTutorServer, - type ReviewTutorServer, -} from "../src/server.ts"; - -const skillPath = fileURLToPath( - new URL("../skills/review-tutor/SKILL.md", import.meta.url), -); - -type NodeExecFile = typeof nodeExecFile; - -export function createExecFileAdapter(execFile: NodeExecFile = nodeExecFile): ExecFile { - return (file, argv, options) => new Promise((resolve, reject) => { - execFile(file, argv, { - cwd: options.cwd, - encoding: options.encoding, - maxBuffer: options.maxBuffer, - timeout: 30_000, - shell: false, - ...(options.signal ? { signal: options.signal } : {}), - }, (error, stdout, stderr) => { - if (error) { - (error as Error & { stderr?: string }).stderr = stderr; - reject(error); - return; - } - resolve({ stdout, stderr }); - }); - }); -} - -export interface ServerLifecycle { - start( - factory: (signal: AbortSignal) => Promise, - ): Promise; - shutdown(): Promise; -} - -export function createServerLifecycle(): ServerLifecycle { - let server: ReviewTutorServer | undefined; - let starting: Promise | undefined; - let startupController: AbortController | undefined; - let stopping: Promise | undefined; - let shuttingDown = false; - - return { - async start(factory) { - if (server) return server; - if (shuttingDown) return undefined; - if (!starting) { - const controller = new AbortController(); - const attempt = Promise.resolve().then(() => factory(controller.signal)); - startupController = controller; - starting = attempt; - void attempt.then( - (started) => { - server = started; - }, - () => {}, - ).finally(() => { - if (starting === attempt) starting = undefined; - if (startupController === controller) startupController = undefined; - }); - } - - const attempt = starting; - try { - const started = await attempt; - if (shuttingDown) { - if (server === started) server = undefined; - await started.close(); - return undefined; - } - return started; - } catch (error) { - if (shuttingDown) return undefined; - throw error; - } - }, + createExecFileAdapter, + createServerLifecycle, + openInBrowser, + resolveRepository, + sourceFromArgument, +} from "../src/cli-support.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import { defaultSkillPath } from "../src/paths.ts"; +import type { ModelChoice } from "../src/protocol.ts"; +import { startReviewTutorServer } from "../src/server.ts"; - async shutdown() { - if (stopping) { - await stopping; - return; - } +export { createExecFileAdapter, createServerLifecycle } from "../src/cli-support.ts"; - shuttingDown = true; - const controller = startupController; - controller?.abort(); - const attempt = starting; - const current = server; - server = undefined; - const stop = (async () => { - try { - if (current) { - await current.close(); - } else if (attempt) { - const started = await attempt; - if (server === started) server = undefined; - await started.close(); - } - } catch { - // Shutdown must remain contained inside the extension. - } - })(); - stopping = stop; - try { - await stop; - } finally { - if (starting === attempt) starting = undefined; - if (startupController === controller) startupController = undefined; - if (stopping === stop) stopping = undefined; - shuttingDown = false; - } - }, - }; -} +const skillPath = defaultSkillPath(); function safeNotify( ctx: ExtensionCommandContext, @@ -146,27 +39,6 @@ function safeStatus(ctx: ExtensionCommandContext, value?: string): void { } } -function sourceFromArgument(argument: string): SourceRequest | undefined { - const value = argument.trim(); - if (!value) return undefined; - if (/^https:\/\/github\.com\//.test(value)) { - return { protocol: "rt/1", kind: "pr", url: value }; - } - if (value === "worktree") return { protocol: "rt/1", kind: "worktree" }; - if (value === "staged") return { protocol: "rt/1", kind: "staged" }; - - const range = value.split("..."); - if (range.length === 2) { - return { - protocol: "rt/1", - kind: "range", - from: range[0]!, - to: range[1]!, - }; - } - return { protocol: "rt/1", kind: "commit", revision: value }; -} - function modelChoice( model: { provider: string; id: string; name?: string; reasoning?: boolean }, pinnedThinkingLevel?: string, @@ -190,37 +62,6 @@ export function modelChoices(ctx: ExtensionCommandContext): ModelChoice[] { return ctx.modelRegistry.getAvailable().map((model) => modelChoice(model)); } -async function repositoryRoot(pi: ExtensionAPI, cwd: string): Promise { - const result = await pi.exec("git", ["rev-parse", "--show-toplevel"], { - cwd, - timeout: 5_000, - }); - if (result.code !== 0) { - throw new Error( - `repository resolution failed: expected git rev-parse to succeed, received code ${result.code}; run /review-tutor inside a Git worktree`, - ); - } - return realpath(result.stdout.trim()); -} - -async function openBrowser(pi: ExtensionAPI, url: string): Promise { - const command = process.platform === "darwin" - ? "open" - : process.platform === "win32" - ? "cmd" - : "xdg-open"; - const args = process.platform === "win32" - ? ["/c", "start", "", url] - : [url]; - - try { - const result = await pi.exec(command, args, { timeout: 10_000 }); - return result.code === 0; - } catch { - return false; - } -} - function notifyBrowserResult( ctx: ExtensionCommandContext, opened: boolean, @@ -252,7 +93,7 @@ export default function reviewTutorExtension(pi: ExtensionAPI): void { const server = await lifecycle.start(async (startupSignal) => { const cwd = await realpath(ctx.cwd); - const canonicalRepo = await repositoryRoot(pi, cwd); + const canonicalRepo = await resolveRepository(cwd, execFile); const piModels = modelChoices(ctx); if (!piModels.length) { throw new Error( @@ -273,7 +114,7 @@ export default function reviewTutorExtension(pi: ExtensionAPI): void { if (!server) return; safeStatus(ctx, "Review Tutor running"); - const opened = await openBrowser(pi, server.url); + const opened = await openInBrowser(server.url, process.platform, execFile); notifyBrowserResult(ctx, opened, server.url); } catch (error) { safeStatus(ctx); diff --git a/packages/review-tutor/src/cli-support.ts b/packages/review-tutor/src/cli-support.ts new file mode 100644 index 0000000..506cf96 --- /dev/null +++ b/packages/review-tutor/src/cli-support.ts @@ -0,0 +1,180 @@ +import { execFile as nodeExecFile } from "node:child_process"; +import { realpath } from "node:fs/promises"; +import type { ExecFile } from "./inputs.ts"; +import type { SourceRequest } from "./protocol.ts"; +import type { ReviewTutorServer } from "./server.ts"; + +type NodeExecFile = typeof nodeExecFile; + +const COMMAND_BUFFER = 1024 * 1024; + +export function createExecFileAdapter(execFile: NodeExecFile = nodeExecFile): ExecFile { + return (file, argv, options) => new Promise((resolve, reject) => { + execFile(file, argv, { + cwd: options.cwd, + encoding: options.encoding, + maxBuffer: options.maxBuffer, + timeout: 30_000, + shell: false, + ...(options.signal ? { signal: options.signal } : {}), + }, (error, stdout, stderr) => { + if (error) { + (error as Error & { stderr?: string }).stderr = stderr; + reject(error); + return; + } + resolve({ stdout, stderr }); + }); + }); +} + +export function sourceFromArgument(argument: string): SourceRequest | undefined { + const value = argument.trim(); + if (!value) return undefined; + if (/^https:\/\/github\.com\//.test(value)) { + return { protocol: "rt/1", kind: "pr", url: value }; + } + if (value === "worktree") return { protocol: "rt/1", kind: "worktree" }; + if (value === "staged") return { protocol: "rt/1", kind: "staged" }; + + const range = value.split("..."); + if (range.length === 2) { + return { + protocol: "rt/1", + kind: "range", + from: range[0]!, + to: range[1]!, + }; + } + return { protocol: "rt/1", kind: "commit", revision: value }; +} + +export async function resolveRepository(cwd: string, execFile: ExecFile): Promise { + let stdout: string; + try { + ({ stdout } = await execFile("git", ["rev-parse", "--show-toplevel"], { + cwd, + encoding: "utf8", + maxBuffer: COMMAND_BUFFER, + })); + } catch (error) { + const code = (error as { code?: unknown }).code; + throw new Error( + `repository resolution failed: expected git rev-parse to succeed, received code ${String(code ?? "unknown")}; run /review-tutor inside a Git worktree`, + ); + } + return realpath(stdout.trim()); +} + +export async function openInBrowser( + url: string, + platform: NodeJS.Platform, + execFile: ExecFile, +): Promise { + const command = platform === "darwin" + ? "open" + : platform === "win32" + ? "cmd" + : "xdg-open"; + const args = platform === "win32" + ? ["/c", "start", "", url] + : [url]; + + try { + await execFile(command, args, { + cwd: process.cwd(), + encoding: "utf8", + maxBuffer: COMMAND_BUFFER, + }); + return true; + } catch { + return false; + } +} + +export interface ServerLifecycle { + start( + factory: (signal: AbortSignal) => Promise, + ): Promise; + shutdown(): Promise; +} + +export function createServerLifecycle(): ServerLifecycle { + let server: ReviewTutorServer | undefined; + let starting: Promise | undefined; + let startupController: AbortController | undefined; + let stopping: Promise | undefined; + let shuttingDown = false; + + return { + async start(factory) { + if (server) return server; + if (shuttingDown) return undefined; + if (!starting) { + const controller = new AbortController(); + const attempt = Promise.resolve().then(() => factory(controller.signal)); + startupController = controller; + starting = attempt; + void attempt.then( + (started) => { + server = started; + }, + () => {}, + ).finally(() => { + if (starting === attempt) starting = undefined; + if (startupController === controller) startupController = undefined; + }); + } + + const attempt = starting; + try { + const started = await attempt; + if (shuttingDown) { + if (server === started) server = undefined; + await started.close(); + return undefined; + } + return started; + } catch (error) { + if (shuttingDown) return undefined; + throw error; + } + }, + + async shutdown() { + if (stopping) { + await stopping; + return; + } + + shuttingDown = true; + const controller = startupController; + controller?.abort(); + const attempt = starting; + const current = server; + server = undefined; + const stop = (async () => { + try { + if (current) { + await current.close(); + } else if (attempt) { + const started = await attempt; + if (server === started) server = undefined; + await started.close(); + } + } catch { + // Shutdown must remain contained inside the caller. + } + })(); + stopping = stop; + try { + await stop; + } finally { + if (starting === attempt) starting = undefined; + if (startupController === controller) startupController = undefined; + if (stopping === stop) stopping = undefined; + shuttingDown = false; + } + }, + }; +} diff --git a/packages/review-tutor/src/connectors/codex.ts b/packages/review-tutor/src/connectors/codex.ts index 384ecd9..4e2d56d 100644 --- a/packages/review-tutor/src/connectors/codex.ts +++ b/packages/review-tutor/src/connectors/codex.ts @@ -1,22 +1,24 @@ -import { execFile as nodeExecFile } from "node:child_process"; import type { ConnectorRequest, Discovery, DiscoveryDeps, - DiscoveryExecFile, HarnessConnector, ParseSink, ParsedAnswer, SpawnSpec, } from "./types.ts"; +import { + BASE_DISCOVERY_ENV_KEYS, + createDiscoveryExecFile, + discoveryOptions, + scrubbedEnvironment, +} from "./discovery.ts"; import { redact } from "./redact.ts"; import { ConnectorError } from "./types.ts"; -const MAX_CATALOG_BYTES = 1024 * 1024; -const DISCOVERY_TIMEOUT_MS = 10_000; const FALLBACK_LEVELS = ["low", "medium", "high"]; const MINIMUM_VERSION = [0, 140, 0] as const; -const DISCOVERY_ENV_KEYS = ["PATH", "HOME", "USER", "LOGNAME", "LANG", "LC_ALL", "CODEX_HOME"] as const; +const DISCOVERY_ENV_KEYS = [...BASE_DISCOVERY_ENV_KEYS, "CODEX_HOME"] as const; interface CodexModel { slug?: unknown; @@ -35,18 +37,10 @@ interface CodexEvent { } export function discoveryEnvironment(source: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { - return Object.fromEntries(DISCOVERY_ENV_KEYS.flatMap((key) => { - const value = source[key]; - return value === undefined ? [] : [[key, value]]; - })); + return scrubbedEnvironment(DISCOVERY_ENV_KEYS, source); } -const defaultExecFile: DiscoveryExecFile = (file, args, options) => new Promise((resolve, reject) => { - nodeExecFile(file, args, { ...options, env: discoveryEnvironment(), shell: false }, (error, stdout, stderr) => { - if (error) reject(error); - else resolve({ stdout, stderr }); - }); -}); +const defaultExecFile = createDiscoveryExecFile(DISCOVERY_ENV_KEYS); function versionAtLeast(version: readonly number[]): boolean { for (let index = 0; index < MINIMUM_VERSION.length; index += 1) { @@ -147,13 +141,7 @@ export class CodexConnector implements HarnessConnector { const execFile = deps.execFile ?? defaultExecFile; let versionOutput: string; try { - const options = { - encoding: "utf8" as const, - maxBuffer: MAX_CATALOG_BYTES, - signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), - timeout: DISCOVERY_TIMEOUT_MS, - }; - ({ stdout: versionOutput } = await execFile("codex", ["--version"], options)); + ({ stdout: versionOutput } = await execFile("codex", ["--version"], discoveryOptions())); } catch (error) { return (error as NodeJS.ErrnoException).code === "ENOENT" ? { available: false, reason: "Codex is not installed (codex not found on PATH)." } @@ -172,13 +160,7 @@ export class CodexConnector implements HarnessConnector { }; } try { - const options = { - encoding: "utf8" as const, - maxBuffer: MAX_CATALOG_BYTES, - signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), - timeout: DISCOVERY_TIMEOUT_MS, - }; - const { stdout } = await execFile("codex", ["debug", "models"], options); + const { stdout } = await execFile("codex", ["debug", "models"], discoveryOptions()); return { available: true, version: versionLabel, models: modelChoices(parseModels(stdout)) }; } catch { return { available: false, reason: "Codex could not list its models." }; diff --git a/packages/review-tutor/src/connectors/discovery.ts b/packages/review-tutor/src/connectors/discovery.ts new file mode 100644 index 0000000..63c2e9b --- /dev/null +++ b/packages/review-tutor/src/connectors/discovery.ts @@ -0,0 +1,46 @@ +import { execFile as nodeExecFile } from "node:child_process"; +import type { DiscoveryExecFile } from "./types.ts"; + +export const MAX_DISCOVERY_BYTES = 1024 * 1024; +export const DISCOVERY_TIMEOUT_MS = 10_000; + +/** Keys every harness may read; a connector adds only the ones its own CLI needs. */ +export const BASE_DISCOVERY_ENV_KEYS = ["PATH", "HOME", "USER", "LOGNAME", "LANG", "LC_ALL"] as const; + +export function scrubbedEnvironment( + keys: readonly string[], + source: NodeJS.ProcessEnv = process.env, +): NodeJS.ProcessEnv { + return Object.fromEntries(keys.flatMap((key) => { + const value = source[key]; + return value === undefined ? [] : [[key, value]]; + })); +} + +export function createDiscoveryExecFile(keys: readonly string[]): DiscoveryExecFile { + return (file, args, options) => new Promise((resolve, reject) => { + nodeExecFile( + file, + args, + { ...options, env: scrubbedEnvironment(keys), shell: false }, + (error, stdout, stderr) => { + if (error) reject(error); + else resolve({ stdout, stderr }); + }, + ); + }); +} + +export function discoveryOptions(): { + encoding: "utf8"; + maxBuffer: number; + signal: AbortSignal; + timeout: number; +} { + return { + encoding: "utf8", + maxBuffer: MAX_DISCOVERY_BYTES, + signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), + timeout: DISCOVERY_TIMEOUT_MS, + }; +} diff --git a/packages/review-tutor/src/paths.ts b/packages/review-tutor/src/paths.ts index d694745..0b512d6 100644 --- a/packages/review-tutor/src/paths.ts +++ b/packages/review-tutor/src/paths.ts @@ -1,6 +1,16 @@ import { createHash } from "node:crypto"; import { homedir } from "node:os"; import { isAbsolute, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** + * Both loaders sit exactly one directory below the package root: Pi imports + * `src/paths.ts` from TS source and the built CLI bundles this file into + * `dist/cli.js`, so one `..` reaches the shipped skill from either location. + */ +export function defaultSkillPath(): string { + return fileURLToPath(new URL("../skills/review-tutor/SKILL.md", import.meta.url)); +} export interface StatePaths { root: string; diff --git a/packages/review-tutor/test/connectors.test.ts b/packages/review-tutor/test/connectors.test.ts index 08dcccb..3ccd11e 100644 --- a/packages/review-tutor/test/connectors.test.ts +++ b/packages/review-tutor/test/connectors.test.ts @@ -21,8 +21,10 @@ class FakeChild extends EventEmitter { const models = [{ id: "anthropic/model", label: "Model", thinkingLevels: ["low"] }]; +const absentExecFile = async () => { throw Object.assign(new Error("spawn ENOENT"), { code: "ENOENT" }); }; + function registry() { - return createConnectorRegistry({ piModels: models }); + return createConnectorRegistry({ piModels: models, which: async () => undefined, execFile: absentExecFile }); } const request = { From f10dc3740427dd5cb02c6a43684ac505c164bccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 11:52:38 -0300 Subject: [PATCH 2/6] feat(review-tutor): add a host-independent review-tutor CLI `review-tutor [source] [--no-open] [--detach] [--home ]` starts the same local server from any shell. Pi now discovers itself through `pi --version` and `pi --list-models` when no host supplies a model snapshot, under the same bounds and scrubbed environment the Codex connector uses. The URL is the only line on stdout, discovery goes to stderr, and `--detach` hands the server to a child that reports its URL once and exits after 30 idle minutes or on SIGTERM. Refs #82 --- package.json | 2 +- packages/review-tutor/README.md | 15 + packages/review-tutor/package.json | 11 + packages/review-tutor/src/cli.ts | 258 +++++++++++++++++ packages/review-tutor/src/connectors/pi.ts | 89 +++++- .../review-tutor/src/connectors/registry.ts | 4 +- packages/review-tutor/src/connectors/types.ts | 3 +- packages/review-tutor/src/server.ts | 18 +- packages/review-tutor/src/sse.ts | 4 + packages/review-tutor/test/cli.test.ts | 267 ++++++++++++++++++ .../review-tutor/test/connector-pi.test.ts | 117 ++++++++ .../test/fixtures/pi/list-models.txt | 8 + 12 files changed, 784 insertions(+), 12 deletions(-) create mode 100644 packages/review-tutor/src/cli.ts create mode 100644 packages/review-tutor/test/cli.test.ts create mode 100644 packages/review-tutor/test/connector-pi.test.ts create mode 100644 packages/review-tutor/test/fixtures/pi/list-models.txt diff --git a/package.json b/package.json index d4e8bfa..e445f1a 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "packages/*" ], "scripts": { - "build": "bun run --cwd packages/tauri-release build && bun run --cwd packages/tauri-updater build && bun run --cwd packages/auth build && bun run --cwd packages/brand build && bun run --cwd packages/flags build && bun run --cwd packages/billing build && bun run --cwd packages/edge-shared build && bun run --cwd packages/sync build", + "build": "bun run --cwd packages/tauri-release build && bun run --cwd packages/tauri-updater build && bun run --cwd packages/auth build && bun run --cwd packages/brand build && bun run --cwd packages/flags build && bun run --cwd packages/billing build && bun run --cwd packages/edge-shared build && bun run --cwd packages/sync build && bun run --cwd packages/review-tutor build", "test": "vitest run", "test:supabase": "supabase test db supabase/tests/database --local && bun run supabase/tests/welcome-credits-concurrency.ts && bun test packages/billing/test/checkout-lifecycle.contract.test.ts && bun test packages/sync/test/lww.contract.test.ts && bun test packages/edge-shared/test/router-attempt.contract.test.ts", "test:coverage": "vitest run --coverage", diff --git a/packages/review-tutor/README.md b/packages/review-tutor/README.md index 89d9ff7..8a6e616 100644 --- a/packages/review-tutor/README.md +++ b/packages/review-tutor/README.md @@ -40,6 +40,21 @@ You can provide an initial source: With no argument, choose a source in the browser. The browser supports worktree, staged, commit, range, GitHub PR URL, and pasted code sources. It opens automatically. If that fails, Pi shows a notification with the local URL to open. +## Command line + +The same tutor runs without a Pi host. Build the CLI once, then run it from any shell inside a Git worktree: + +```bash +bun run --cwd packages/review-tutor build +node packages/review-tutor/dist/cli.js [source] [--no-open] [--detach] [--home ] +``` + +An installed package exposes it as `review-tutor`. `source` accepts the same values as the Pi command and defaults to `worktree`. The URL is the only line on stdout; discovery results go to stderr. Unknown flags print usage on stderr and exit 2. + +Without a Pi host, the Pi harness discovers itself through `pi --version` and `pi --list-models`, so the model list is whatever that Pi install offers. Claude Code and Codex are discovered exactly as they are under Pi. + +The foreground run stays attached until Ctrl-C or `SIGTERM`. `--detach` starts the server in its own process, prints the URL, and returns; that detached server exits by itself after 30 minutes with no page connected, or on `SIGTERM`. + ## Model selection The model dialog lists the session's scoped models when `--models` or the settings scope configures them. Otherwise, it lists all available models. Thinking levels are offered only for reasoning models. Thinking levels are enforced by the server's model membership check. A scope entry with an explicit level, such as `gpt-5.6-sol:high`, pins the tutor to that level. diff --git a/packages/review-tutor/package.json b/packages/review-tutor/package.json index 91a3d85..c9f7275 100644 --- a/packages/review-tutor/package.json +++ b/packages/review-tutor/package.json @@ -9,7 +9,18 @@ "extensions": ["extensions/review-tutor.ts"], "skills": ["skills/review-tutor"] }, + "bin": { + "review-tutor": "dist/cli.js" + }, + "files": [ + "dist", + "src", + "extensions", + "skills", + "README.md" + ], "scripts": { + "build": "tsup src/cli.ts --format esm --clean --splitting false --out-dir dist", "test": "cd ../.. && vitest run packages/review-tutor/test", "typecheck": "tsc -p ../../tsconfig.json --noEmit" }, diff --git a/packages/review-tutor/src/cli.ts b/packages/review-tutor/src/cli.ts new file mode 100644 index 0000000..c2b4ee9 --- /dev/null +++ b/packages/review-tutor/src/cli.ts @@ -0,0 +1,258 @@ +#!/usr/bin/env node +import { spawn } from "node:child_process"; +import { realpath } from "node:fs/promises"; +import type { Readable } from "node:stream"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { + createExecFileAdapter, + openInBrowser, + resolveRepository, + sourceFromArgument, +} from "./cli-support.ts"; +import { createConnectorRegistry, type ConnectorRegistry } from "./connectors/registry.ts"; +import type { ExecFile } from "./inputs.ts"; +import { defaultSkillPath } from "./paths.ts"; +import type { SourceRequest } from "./protocol.ts"; +import { startReviewTutorServer, type StartedReviewTutorServer } from "./server.ts"; + +export const USAGE = [ + "Usage: review-tutor [source] [--no-open] [--detach] [--home ]", + "", + " source worktree (default), staged, , ..., or a GitHub PR URL", + " --no-open print the URL without opening a browser", + " --detach leave the server running in the background and return immediately", + " --home store Review Tutor state under this absolute directory", +].join("\n"); + +/** A detached server with no page attached for this long has been abandoned. */ +export const IDLE_EXIT_MS = 30 * 60_000; +const IDLE_CHECK_MS = 60_000; +const HANDSHAKE_TIMEOUT_MS = 30_000; + +export class UsageError extends Error {} + +export interface CliOptions { + source?: SourceRequest; + open: boolean; + detach: boolean; + serveDetached: boolean; + help: boolean; + home?: string; +} + +export interface DetachedChild { + readonly stdout: Readable | null; + once(event: string, listener: (...args: unknown[]) => void): unknown; + unref(): void; + kill(signal?: NodeJS.Signals): boolean; +} + +export interface CliDeps { + stdout(text: string): void; + stderr(text: string): void; + cwd(): string; + execFile: ExecFile; + createRegistry(): ConnectorRegistry; + startServer(options: Parameters[0]): Promise; + openInBrowser(url: string): Promise; + signals(handler: () => void): void; + spawnDetached(args: string[]): DetachedChild; +} + +function applyFlag(options: CliOptions, argv: readonly string[], index: number): number { + const flag = argv[index]!; + if (flag === "--no-open") options.open = false; + else if (flag === "--detach") options.detach = true; + else if (flag === "--serve-detached") options.serveDetached = true; + else if (flag === "--help" || flag === "-h") options.help = true; + else if (flag === "--home") { + const value = argv[index + 1]; + if (value === undefined || value.startsWith("-")) { + throw new UsageError("--home requires an absolute directory path"); + } + options.home = value; + return index + 1; + } else throw new UsageError(`unknown option ${flag}`); + return index; +} + +export function parseArguments(argv: readonly string[]): CliOptions { + const options: CliOptions = { open: true, detach: false, serveDetached: false, help: false }; + const positional: string[] = []; + for (let index = 0; index < argv.length; index += 1) { + const argument = argv[index]!; + if (argument.startsWith("-")) index = applyFlag(options, argv, index); + else positional.push(argument); + } + if (positional.length > 1) throw new UsageError(`unexpected argument ${positional[1]!}`); + options.source = sourceFromArgument(positional[0] ?? "worktree"); + return options; +} + +/** Zero clients for one whole window, not merely at the moment of a check. */ +export class IdleTracker { + private idleSince: number | undefined; + + constructor(private readonly windowMs = IDLE_EXIT_MS) {} + + expired(clientCount: number, now: number): boolean { + if (clientCount > 0) { + this.idleSince = undefined; + return false; + } + this.idleSince ??= now; + return now - this.idleSince >= this.windowMs; + } +} + +function waitForIdle(server: StartedReviewTutorServer): Promise { + return new Promise((resolve) => { + const tracker = new IdleTracker(); + const timer = setInterval(() => { + if (!tracker.expired(server.clientCount(), Date.now())) return; + clearInterval(timer); + resolve(); + }, IDLE_CHECK_MS); + timer.unref?.(); + }); +} + +function message(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +async function discoverySummary(registry: ConnectorRegistry): Promise { + const discoveries = await registry.discoveries(); + return discoveries.map(({ connector, discovery }) => discovery.available + ? `${connector.label}: ${discovery.models.length} model${discovery.models.length === 1 ? "" : "s"}` + : `${connector.label} unavailable: ${discovery.reason}`).join("\n"); +} + +/** The startup summary and the server must share one discovery pass so no harness is probed twice. */ +function memoizeDiscoveries(registry: ConnectorRegistry): ConnectorRegistry { + let cached: ReturnType | undefined; + return { ...registry, discoveries: () => (cached ??= registry.discoveries()) }; +} + +async function runServer(options: CliOptions, deps: CliDeps): Promise { + const cwd = await realpath(deps.cwd()); + const canonicalRepo = await resolveRepository(cwd, deps.execFile); + const registry = memoizeDiscoveries(deps.createRegistry()); + const server = await deps.startServer({ + cwd, + canonicalRepo, + registry, + execFile: deps.execFile, + skillPath: defaultSkillPath(), + ...(options.source ? { initialSource: options.source } : {}), + ...(options.home ? { home: options.home } : {}), + }); + deps.stderr(`${await discoverySummary(registry)}\n`); + deps.stdout(`${server.url}\n`); + if (options.open && !options.serveDetached && !await deps.openInBrowser(server.url)) { + deps.stderr("Could not open a browser. Open the URL above.\n"); + } + await Promise.race([ + new Promise((resolve) => deps.signals(resolve)), + ...(options.serveDetached ? [waitForIdle(server)] : []), + ]); + await server.close(); + return 0; +} + +function readUrlLine(child: DetachedChild, timeoutMs: number): Promise { + const stdout = child.stdout; + if (!stdout) { + return Promise.reject(new Error( + "detached start failed: expected a pipe on the detached server's stdout; retry without --detach", + )); + } + return new Promise((resolve, reject) => { + let buffer = ""; + let timer: ReturnType; + const onData = (chunk: unknown): void => { + buffer += String(chunk); + const newline = buffer.indexOf("\n"); + if (newline < 0) return; + clearTimeout(timer); + stdout.off("data", onData); + resolve(buffer.slice(0, newline)); + }; + const fail = (reason: string): void => { + clearTimeout(timer); + stdout.off("data", onData); + reject(new Error(reason)); + }; + timer = setTimeout(() => { + child.kill("SIGTERM"); + fail(`detached start failed: expected a URL within ${timeoutMs / 1000} seconds, received none; retry without --detach`); + }, timeoutMs); + stdout.on("data", onData); + child.once("error", () => fail("detached start failed: the detached server could not be started; retry without --detach")); + child.once("exit", () => fail("detached start failed: the detached server exited before reporting a URL; retry without --detach")); + }); +} + +async function runDetachedParent( + argv: readonly string[], + options: CliOptions, + deps: CliDeps, +): Promise { + const child = deps.spawnDetached(["--serve-detached", ...argv]); + const url = await readUrlLine(child, HANDSHAKE_TIMEOUT_MS); + child.stdout?.destroy(); + child.unref(); + deps.stdout(`${url}\n`); + if (options.open && !await deps.openInBrowser(url)) { + deps.stderr("Could not open a browser. Open the URL above.\n"); + } + return 0; +} + +export async function runCli(argv: readonly string[], deps: CliDeps): Promise { + let options: CliOptions; + try { + options = parseArguments(argv); + } catch (error) { + deps.stderr(`review-tutor: ${message(error)}\n${USAGE}\n`); + return 2; + } + if (options.help) { + deps.stdout(`${USAGE}\n`); + return 0; + } + try { + return options.detach && !options.serveDetached + ? await runDetachedParent(argv, options, deps) + : await runServer(options, deps); + } catch (error) { + deps.stderr(`review-tutor failed: ${message(error)}\n`); + return 1; + } +} + +export function nodeCliDeps(scriptPath: string): CliDeps { + const execFile = createExecFileAdapter(); + return { + stdout: (text) => { process.stdout.write(text); }, + stderr: (text) => { process.stderr.write(text); }, + cwd: () => process.cwd(), + execFile, + createRegistry: () => createConnectorRegistry({}), + startServer: startReviewTutorServer, + openInBrowser: (url) => openInBrowser(url, process.platform, execFile), + signals: (handler) => { + process.once("SIGINT", handler); + process.once("SIGTERM", handler); + }, + spawnDetached: (args) => spawn(process.execPath, [scriptPath, ...args], { + detached: true, + stdio: ["ignore", "pipe", "ignore"], + }), + }; +} + +const entry = process.argv[1]; +if (entry && pathToFileURL(entry).href === import.meta.url) { + process.exitCode = await runCli(process.argv.slice(2), nodeCliDeps(fileURLToPath(import.meta.url))); +} diff --git a/packages/review-tutor/src/connectors/pi.ts b/packages/review-tutor/src/connectors/pi.ts index 80fabd5..99ff724 100644 --- a/packages/review-tutor/src/connectors/pi.ts +++ b/packages/review-tutor/src/connectors/pi.ts @@ -3,12 +3,55 @@ import type { Discovery, DiscoveryDeps, HarnessConnector, + ModelChoice, ParseSink, ParsedAnswer, SpawnSpec, } from "./types.ts"; +import { + BASE_DISCOVERY_ENV_KEYS, + createDiscoveryExecFile, + discoveryOptions, +} from "./discovery.ts"; import { ConnectorError } from "./types.ts"; +const MINIMUM_VERSION = [0, 83, 0] as const; +const DISCOVERY_ENV_KEYS = [...BASE_DISCOVERY_ENV_KEYS, "XDG_CONFIG_HOME"] as const; +const REASONING_LEVELS = ["off", "minimal", "low", "medium", "high", "xhigh"]; +const HEADER_COLUMNS = ["provider", "model", "context", "max-out", "thinking", "images"]; + +const defaultExecFile = createDiscoveryExecFile(DISCOVERY_ENV_KEYS); + +function versionAtLeast(version: readonly number[]): boolean { + for (let index = 0; index < MINIMUM_VERSION.length; index += 1) { + const difference = version[index]! - MINIMUM_VERSION[index]!; + if (difference !== 0) return difference > 0; + } + return true; +} + +function tableRows(output: string): string[][] { + const lines = output.split("\n") + .map((line) => line.trimEnd()) + .filter((line) => line.trim() && !line.startsWith("Warning:")); + const header = lines.findIndex((line) => { + const columns = line.trim().split(/\s{2,}/); + return HEADER_COLUMNS.every((column, index) => columns[index] === column); + }); + if (header < 0) return []; + return lines.slice(header + 1) + .map((line) => line.trim().split(/\s{2,}/)) + .filter((columns) => columns.length >= HEADER_COLUMNS.length); +} + +function listedModels(output: string): ModelChoice[] { + return tableRows(output).map(([provider, model, , , thinking]) => ({ + id: `pi:${provider!}/${model!}`, + label: `${provider!}: ${model!}`, + thinkingLevels: thinking === "yes" ? [...REASONING_LEVELS] : ["off"], + })); +} + interface PiContent { type?: unknown; text?: unknown; @@ -57,11 +100,47 @@ export class PiConnector implements HarnessConnector { readonly label = "Pi"; async discover(deps: DiscoveryDeps): Promise { - return { - available: true, - version: deps.piVersion ?? "unknown", - models: deps.piModels.map((model) => ({ ...model, id: `pi:${model.id}` })), - }; + if (deps.piModels) { + return { + available: true, + version: deps.piVersion ?? "unknown", + models: deps.piModels.map((model) => ({ ...model, id: `pi:${model.id}` })), + }; + } + return this.discoverWithoutHost(deps.execFile ?? defaultExecFile); + } + + private async discoverWithoutHost(execFile: NonNullable): Promise { + let versionOutput: string; + try { + ({ stdout: versionOutput } = await execFile("pi", ["--version"], discoveryOptions())); + } catch (error) { + return (error as NodeJS.ErrnoException).code === "ENOENT" + ? { available: false, reason: "Pi is not installed (pi not found on PATH)." } + : { available: false, reason: "Pi could not report a supported version." }; + } + const match = /^(\d+)\.(\d+)\.(\d+)/.exec(versionOutput.trim()); + if (!match) { + return { available: false, reason: "Pi could not report a supported version." }; + } + const version = match.slice(1, 4).map(Number); + const versionLabel = version.join("."); + if (!versionAtLeast(version)) { + return { + available: false, + reason: `Pi ${versionLabel} is too old; version 0.83.0 or newer is required.`, + }; + } + let models: ModelChoice[]; + try { + const { stdout } = await execFile("pi", ["--list-models"], discoveryOptions()); + models = listedModels(stdout); + } catch { + return { available: false, reason: "Pi could not list its models." }; + } + return models.length + ? { available: true, version: versionLabel, models } + : { available: false, reason: "Pi could not list its models." }; } spawnSpec(request: ConnectorRequest): SpawnSpec { diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts index 5527a90..8a1358c 100644 --- a/packages/review-tutor/src/connectors/registry.ts +++ b/packages/review-tutor/src/connectors/registry.ts @@ -27,14 +27,14 @@ function which(command: string): Promise { } export function createConnectorRegistry(options: { - piModels: ModelChoice[]; + piModels?: ModelChoice[]; piVersion?: string; which?: DiscoveryDeps["which"]; execFile?: DiscoveryExecFile; }): ConnectorRegistry { const registered: HarnessConnector[] = [new PiConnector(), new ClaudeCodeConnector(), new CodexConnector()]; const dependencies: DiscoveryDeps = { - piModels: options.piModels, + ...(options.piModels ? { piModels: options.piModels } : {}), ...(options.piVersion ? { piVersion: options.piVersion } : {}), which: options.which ?? which, ...(options.execFile ? { execFile: options.execFile } : {}), diff --git a/packages/review-tutor/src/connectors/types.ts b/packages/review-tutor/src/connectors/types.ts index 7e48d2e..1090334 100644 --- a/packages/review-tutor/src/connectors/types.ts +++ b/packages/review-tutor/src/connectors/types.ts @@ -21,7 +21,8 @@ export type DiscoveryExecFile = ( ) => Promise<{ stdout: string; stderr: string }>; export interface DiscoveryDeps { - piModels: ModelChoice[]; + /** Present only when a Pi host already holds the model snapshot; absent hosts make Pi discover itself. */ + piModels?: ModelChoice[]; piVersion?: string; which(command: string): Promise; execFile?: DiscoveryExecFile; diff --git a/packages/review-tutor/src/server.ts b/packages/review-tutor/src/server.ts index 78060a2..c130ef8 100644 --- a/packages/review-tutor/src/server.ts +++ b/packages/review-tutor/src/server.ts @@ -32,6 +32,11 @@ export interface ReviewTutorServer { port: number; } +/** A started server also reports its live SSE clients, which is how a detached CLI knows it is idle. */ +export interface StartedReviewTutorServer extends ReviewTutorServer { + clientCount(): number; +} + const responseHeaders = { "Cache-Control": "no-store", "X-Content-Type-Options": "nosniff", @@ -152,7 +157,7 @@ function handleError(response: ServerResponse, error: unknown): void { } } -export async function startReviewTutorServer(options: ServerOptions): Promise { +export async function startReviewTutorServer(options: ServerOptions): Promise { const token = randomBytes(32).toString("base64url"); const paths = resolveStatePaths(options.canonicalRepo, options.home); await initProjectPaths(paths, options.canonicalRepo); @@ -165,9 +170,10 @@ export async function startReviewTutorServer(options: ServerOptions): Promise | undefined; const server = createServer(async (request, response) => { @@ -197,7 +203,13 @@ export async function startReviewTutorServer(options: ServerOptions): Promise hub.clientCount, + }; } async function loadInitialSource(options: ServerOptions, session: ReviewTutorSession): Promise { diff --git a/packages/review-tutor/src/sse.ts b/packages/review-tutor/src/sse.ts index 63161a2..cd0cb78 100644 --- a/packages/review-tutor/src/sse.ts +++ b/packages/review-tutor/src/sse.ts @@ -8,6 +8,10 @@ export class SseHub { constructor(private readonly capacity = 256) {} + get clientCount(): number { + return this.clients.size; + } + emit(type: SseEventType, data: unknown): SseEvent { const event = { id: this.next++, type, data }; this.ring.push(event); diff --git a/packages/review-tutor/test/cli.test.ts b/packages/review-tutor/test/cli.test.ts new file mode 100644 index 0000000..a32e20b --- /dev/null +++ b/packages/review-tutor/test/cli.test.ts @@ -0,0 +1,267 @@ +import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + IdleTracker, + parseArguments, + runCli, + USAGE, + type CliDeps, + type DetachedChild, +} from "../src/cli.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import type { ExecFile } from "../src/inputs.ts"; +import type { StartedReviewTutorServer } from "../src/server.ts"; + +const URL_LINE = "http://127.0.0.1:4321/?session=token"; +const absentExecFile = async () => { throw Object.assign(new Error("spawn ENOENT"), { code: "ENOENT" }); }; + +class FakeChild extends EventEmitter implements DetachedChild { + stdout = new PassThrough(); + unref = vi.fn(); + kill = vi.fn(() => true); +} + +function fakeServer(close = vi.fn(async () => {}), clientCount = () => 0): StartedReviewTutorServer { + return { url: URL_LINE, token: "token", port: 4321, close, clientCount }; +} + +interface Harness { + deps: CliDeps; + out: string[]; + err: string[]; + close: ReturnType; + started: Array>; + children: FakeChild[]; + signal(): void; +} + +function harness(overrides: Partial = {}): Harness { + const out: string[] = []; + const err: string[] = []; + const close = vi.fn(async () => {}); + const started: Array> = []; + const children: FakeChild[] = []; + const handlers: Array<() => void> = []; + const execFile: ExecFile = async () => ({ stdout: `${process.cwd()}\n`, stderr: "" }); + const deps: CliDeps = { + stdout: (text) => { out.push(text); }, + stderr: (text) => { err.push(text); }, + cwd: () => process.cwd(), + execFile, + createRegistry: () => createConnectorRegistry({ + piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + which: async () => undefined, + execFile: absentExecFile, + }), + startServer: async (options) => { + started.push(options as unknown as Record); + return fakeServer(close); + }, + openInBrowser: vi.fn(async () => true), + signals: (handler) => { handlers.push(handler); }, + spawnDetached: vi.fn(() => { + const child = new FakeChild(); + children.push(child); + return child; + }), + ...overrides, + }; + return { + deps, + out, + err, + close, + started, + children, + signal: () => { for (const handler of handlers.splice(0)) handler(); }, + }; +} + +/** Yields to the real event loop under both timer modes so pending file-system work can land. */ +async function settle(check: () => boolean = () => true): Promise { + for (let attempt = 0; attempt < 200; attempt += 1) { + await (vi.isFakeTimers() + ? vi.advanceTimersByTimeAsync(1) + : new Promise((resolve) => setTimeout(resolve, 1))); + if (check()) return; + } + throw new Error("test barrier failed: expected condition within 200 attempts"); +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("command line parsing", () => { + it("defaults to the worktree source", () => { + expect(parseArguments([])).toMatchObject({ + source: { protocol: "rt/1", kind: "worktree" }, + open: true, + detach: false, + serveDetached: false, + }); + }); + + it.each([ + [["staged"], { source: { protocol: "rt/1", kind: "staged" } }], + [["abc123"], { source: { protocol: "rt/1", kind: "commit", revision: "abc123" } }], + [["main...topic"], { source: { protocol: "rt/1", kind: "range", from: "main", to: "topic" } }], + [["--no-open"], { open: false }], + [["--detach"], { detach: true }], + [["--serve-detached"], { serveDetached: true }], + [["--home", "/tmp/state"], { home: "/tmp/state" }], + [["--detach", "--no-open", "staged"], { detach: true, open: false }], + ])("parses %j", (argv, expected) => { + expect(parseArguments(argv)).toMatchObject(expected); + }); + + it.each([["--nope"], ["--home"], ["--home", "--detach"], ["one", "two"]])( + "rejects %j", + (...argv) => { + expect(() => parseArguments(argv)).toThrow(); + }, + ); + + it("reports usage on stderr and exits 2 for an unknown flag", async () => { + const { deps, out, err } = harness(); + await expect(runCli(["--nope"], deps)).resolves.toBe(2); + expect(out).toEqual([]); + expect(err.join("")).toContain(USAGE); + }); + + it("prints usage on stdout for --help", async () => { + const { deps, out, err } = harness(); + await expect(runCli(["--help"], deps)).resolves.toBe(0); + expect(out.join("")).toBe(`${USAGE}\n`); + expect(err).toEqual([]); + }); +}); + +describe("foreground run", () => { + it("prints one URL line, summarises discovery, and closes on a signal", async () => { + const context = harness(); + const run = runCli([], context.deps); + await settle(() => context.out.length > 0); + expect(context.out).toEqual([`${URL_LINE}\n`]); + expect(context.err.join("")).toBe( + "Pi: 1 model\nClaude Code unavailable: Claude Code is not installed (claude not found on PATH).\nCodex unavailable: Codex is not installed (codex not found on PATH).\n", + ); + expect(context.deps.openInBrowser).toHaveBeenCalledWith(URL_LINE); + expect(context.started[0]).toMatchObject({ initialSource: { kind: "worktree" } }); + expect(context.close).not.toHaveBeenCalled(); + + context.signal(); + await expect(run).resolves.toBe(0); + expect(context.close).toHaveBeenCalledTimes(1); + expect(context.out).toEqual([`${URL_LINE}\n`]); + }); + + it("keeps the browser closed with --no-open and forwards --home", async () => { + const context = harness(); + const run = runCli(["--no-open", "--home", "/tmp/review-tutor-cli"], context.deps); + await settle(() => context.out.length > 0); + expect(context.deps.openInBrowser).not.toHaveBeenCalled(); + expect(context.started[0]).toMatchObject({ home: "/tmp/review-tutor-cli" }); + context.signal(); + await expect(run).resolves.toBe(0); + }); + + it("explains a browser that will not open", async () => { + const context = harness({ openInBrowser: vi.fn(async () => false) }); + const run = runCli([], context.deps); + await settle(() => context.err.join("").includes("Could not open")); + expect(context.err.join("")).toContain("Could not open a browser."); + context.signal(); + await expect(run).resolves.toBe(0); + }); + + it("exits 1 when the repository cannot be resolved", async () => { + const context = harness({ + execFile: async () => { throw Object.assign(new Error("not a repository"), { code: 128 }); }, + }); + await expect(runCli([], context.deps)).resolves.toBe(1); + expect(context.err.join("")).toContain("repository resolution failed"); + expect(context.out).toEqual([]); + }); +}); + +describe("detached handshake", () => { + it("prints the child's URL, unrefs it, and returns immediately", async () => { + const context = harness(); + const run = runCli(["--detach"], context.deps); + await settle(() => context.children.length > 0); + const child = context.children[0]!; + expect(context.deps.spawnDetached).toHaveBeenCalledWith(["--serve-detached", "--detach"]); + child.stdout.write(`${URL_LINE}\n`); + await expect(run).resolves.toBe(0); + expect(context.out).toEqual([`${URL_LINE}\n`]); + expect(child.unref).toHaveBeenCalledTimes(1); + expect(context.deps.openInBrowser).toHaveBeenCalledWith(URL_LINE); + expect(context.close).not.toHaveBeenCalled(); + }); + + it("fails plainly when the child never reports a URL", async () => { + vi.useFakeTimers(); + const context = harness(); + const run = runCli(["--detach"], context.deps); + await settle(() => context.children.length > 0); + await vi.advanceTimersByTimeAsync(30_000); + await expect(run).resolves.toBe(1); + expect(context.children[0]!.kill).toHaveBeenCalledWith("SIGTERM"); + expect(context.err.join("")).toContain("expected a URL within 30 seconds"); + expect(context.out).toEqual([]); + }); + + it("fails when the child exits before reporting a URL", async () => { + const context = harness(); + const run = runCli(["--detach"], context.deps); + await settle(() => context.children.length > 0); + context.children[0]!.emit("exit", 1, null); + await expect(run).resolves.toBe(1); + expect(context.err.join("")).toContain("exited before reporting a URL"); + }); +}); + +describe("detached idle exit", () => { + it("expires only after a whole window without clients", () => { + const tracker = new IdleTracker(1_000); + expect(tracker.expired(0, 0)).toBe(false); + expect(tracker.expired(0, 900)).toBe(false); + expect(tracker.expired(1, 950)).toBe(false); + expect(tracker.expired(0, 1_000)).toBe(false); + expect(tracker.expired(0, 1_999)).toBe(false); + expect(tracker.expired(0, 2_000)).toBe(true); + }); + + it("closes a detached server once it has been idle for the whole window", async () => { + vi.useFakeTimers(); + let clients = 1; + const close = vi.fn(async () => {}); + const context = harness({ + startServer: async () => fakeServer(close, () => clients), + }); + + const run = runCli(["--serve-detached", "--no-open"], context.deps); + await settle(() => context.out.length > 0); + await vi.advanceTimersByTimeAsync(40 * 60_000); + expect(close).not.toHaveBeenCalled(); + + clients = 0; + await vi.advanceTimersByTimeAsync(29 * 60_000); + expect(close).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(2 * 60_000); + await expect(run).resolves.toBe(0); + expect(close).toHaveBeenCalledTimes(1); + }); + + it("never opens a browser from the detached child", async () => { + const context = harness(); + const run = runCli(["--serve-detached"], context.deps); + await settle(() => context.out.length > 0); + expect(context.deps.openInBrowser).not.toHaveBeenCalled(); + expect(context.out).toEqual([`${URL_LINE}\n`]); + context.signal(); + await expect(run).resolves.toBe(0); + }); +}); diff --git a/packages/review-tutor/test/connector-pi.test.ts b/packages/review-tutor/test/connector-pi.test.ts new file mode 100644 index 0000000..556b0bf --- /dev/null +++ b/packages/review-tutor/test/connector-pi.test.ts @@ -0,0 +1,117 @@ +import { readFile } from "node:fs/promises"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it, vi } from "vitest"; +import { PiConnector } from "../src/connectors/pi.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import type { DiscoveryDeps } from "../src/connectors/types.ts"; + +const table = await readFile( + fileURLToPath(new URL("fixtures/pi/list-models.txt", import.meta.url)), + "utf8", +); +const REASONING = ["off", "minimal", "low", "medium", "high", "xhigh"]; + +function fakeDiscovery(outputs: Record): DiscoveryDeps { + return { + which: async () => undefined, + execFile: vi.fn(async (_file, args) => { + const key = args.join(" "); + const output = outputs[key]; + if (output instanceof Error) throw output; + if (output === undefined) throw new Error(`unexpected command: ${key}`); + return { stdout: output, stderr: "" }; + }), + }; +} + +describe("Pi discovery without a host", () => { + it("reports a missing executable", async () => { + const missing = Object.assign(new Error("spawn pi ENOENT"), { code: "ENOENT" }); + await expect(new PiConnector().discover(fakeDiscovery({ "--version": missing }))).resolves.toEqual({ + available: false, + reason: "Pi is not installed (pi not found on PATH).", + }); + }); + + it.each([ + new Error("timed out"), + "not a version\n", + ])("distinguishes other version failures from a missing executable", async (failure) => { + await expect(new PiConnector().discover(fakeDiscovery({ "--version": failure }))).resolves.toEqual({ + available: false, + reason: "Pi could not report a supported version.", + }); + }); + + it("rejects versions below the supported extension API", async () => { + await expect(new PiConnector().discover(fakeDiscovery({ "--version": "0.82.9\n" }))).resolves.toEqual({ + available: false, + reason: "Pi 0.82.9 is too old; version 0.83.0 or newer is required.", + }); + }); + + it("parses the model table through bounded, scrubbed execution", async () => { + const deps = fakeDiscovery({ "--version": "0.84.2\n", "--list-models": table }); + await expect(new PiConnector().discover(deps)).resolves.toEqual({ + available: true, + version: "0.84.2", + models: [ + { id: "pi:openai-codex/gpt-5.6-sol", label: "openai-codex: gpt-5.6-sol", thinkingLevels: REASONING }, + { id: "pi:anthropic/claude-fable-5", label: "anthropic: claude-fable-5", thinkingLevels: REASONING }, + { id: "pi:ollama/qwen3:8b", label: "ollama: qwen3:8b", thinkingLevels: REASONING }, + { id: "pi:deepseek-official/deepseek-v4-flash", label: "deepseek-official: deepseek-v4-flash", thinkingLevels: REASONING }, + { id: "pi:local/basic", label: "local: basic", thinkingLevels: ["off"] }, + ], + }); + expect(deps.execFile).toHaveBeenNthCalledWith(1, "pi", ["--version"], { + encoding: "utf8", + maxBuffer: 1024 * 1024, + signal: expect.any(AbortSignal), + timeout: 10_000, + }); + expect(deps.execFile).toHaveBeenNthCalledWith(2, "pi", ["--list-models"], { + encoding: "utf8", + maxBuffer: 1024 * 1024, + signal: expect.any(AbortSignal), + timeout: 10_000, + }); + expect(deps.execFile).toHaveBeenCalledTimes(2); + }); + + it.each([ + "openai-codex gpt-5.6-sol 272K 128K yes yes\n", + "provider model context max-out thinking images\n", + "", + new Error("timed out"), + ])("reports malformed or failing tables as unavailable", async (failure) => { + await expect(new PiConnector().discover(fakeDiscovery({ + "--version": "0.84.2\n", + "--list-models": failure, + }))).resolves.toEqual({ available: false, reason: "Pi could not list its models." }); + }); + + it("keeps the hosted path free of process execution", async () => { + const deps = fakeDiscovery({}); + const hosted = { ...deps, piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }] }; + await expect(new PiConnector().discover(hosted)).resolves.toEqual({ + available: true, + version: "unknown", + models: [{ id: "pi:provider/model", label: "Model", thinkingLevels: ["low"] }], + }); + expect(deps.execFile).not.toHaveBeenCalled(); + }); +}); + +describe("Pi registry without a host", () => { + it("discovers Pi through the registry when no model snapshot is supplied", async () => { + const registry = createConnectorRegistry({ + which: async () => undefined, + execFile: fakeDiscovery({ "--version": "0.84.2\n", "--list-models": table }).execFile, + }); + const discoveries = await registry.discoveries(); + expect(discoveries.find(({ connector }) => connector.id === "pi")?.discovery).toMatchObject({ + available: true, + version: "0.84.2", + }); + }); +}); diff --git a/packages/review-tutor/test/fixtures/pi/list-models.txt b/packages/review-tutor/test/fixtures/pi/list-models.txt new file mode 100644 index 0000000..1478e8d --- /dev/null +++ b/packages/review-tutor/test/fixtures/pi/list-models.txt @@ -0,0 +1,8 @@ +Warning: No models match pattern "opencode-go/glm-5.3" + +provider model context max-out thinking images +openai-codex gpt-5.6-sol 272K 128K yes yes +anthropic claude-fable-5 1M 64K yes yes +ollama qwen3:8b 256K 32K yes no +deepseek-official deepseek-v4-flash 1M 384K yes no +local basic 8K 4K no no From 48238afabc3f39347af27d26c3670c603c01fce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 12:10:06 -0300 Subject: [PATCH 3/6] fix(review-tutor): run the installed CLI and keep host execution on Pi The npm bin is a symlink, so the argv[1] guard in src/cli.ts never matched and the installed command did nothing; a dedicated src/bin.ts entry now owns the shebang and the invocation. Under Pi, repository resolution and browser opening go back to pi.exec with the 5 s and 10 s timeouts through an optional timeoutMs on the ExecFile seam. The foreground arms its signal wait before opening the browser, the idle window restarts for a connection that came and went between two polls, and a failed detach handshake destroys the pipe and unrefs the child. Refs #82 --- packages/review-tutor/README.md | 2 +- .../review-tutor/extensions/review-tutor.ts | 23 +++++- packages/review-tutor/package.json | 4 +- packages/review-tutor/src/bin.ts | 12 +++ packages/review-tutor/src/cli-support.ts | 6 +- packages/review-tutor/src/cli.ts | 34 ++++---- packages/review-tutor/src/inputs.ts | 1 + packages/review-tutor/src/server.ts | 4 +- packages/review-tutor/src/sse.ts | 7 ++ packages/review-tutor/test/cli.test.ts | 76 +++++++++++++++--- .../test/extension-host-exec.test.ts | 79 +++++++++++++++++++ 11 files changed, 216 insertions(+), 32 deletions(-) create mode 100644 packages/review-tutor/src/bin.ts create mode 100644 packages/review-tutor/test/extension-host-exec.test.ts diff --git a/packages/review-tutor/README.md b/packages/review-tutor/README.md index 8a6e616..7e53783 100644 --- a/packages/review-tutor/README.md +++ b/packages/review-tutor/README.md @@ -46,7 +46,7 @@ The same tutor runs without a Pi host. Build the CLI once, then run it from any ```bash bun run --cwd packages/review-tutor build -node packages/review-tutor/dist/cli.js [source] [--no-open] [--detach] [--home ] +node packages/review-tutor/dist/bin.js [source] [--no-open] [--detach] [--home ] ``` An installed package exposes it as `review-tutor`. `source` accepts the same values as the Pi command and defaults to `worktree`. The URL is the only line on stdout; discovery results go to stderr. Unknown flags print usage on stderr and exit 2. diff --git a/packages/review-tutor/extensions/review-tutor.ts b/packages/review-tutor/extensions/review-tutor.ts index 7bb8993..716f6bb 100644 --- a/packages/review-tutor/extensions/review-tutor.ts +++ b/packages/review-tutor/extensions/review-tutor.ts @@ -12,6 +12,7 @@ import { } from "../src/cli-support.ts"; import { createConnectorRegistry } from "../src/connectors/registry.ts"; import { defaultSkillPath } from "../src/paths.ts"; +import type { ExecFile } from "../src/inputs.ts"; import type { ModelChoice } from "../src/protocol.ts"; import { startReviewTutorServer } from "../src/server.ts"; @@ -19,6 +20,23 @@ export { createExecFileAdapter, createServerLifecycle } from "../src/cli-support const skillPath = defaultSkillPath(); +/** Repository resolution and browser opening stay on the host's own exec, with the host's timeouts. */ +export function piExecFile(pi: ExtensionAPI): ExecFile { + return async (file, argv, options) => { + const result = await pi.exec(file, argv, { + cwd: options.cwd, + ...(options.timeoutMs ? { timeout: options.timeoutMs } : {}), + }); + if (result.code !== 0) { + throw Object.assign(new Error(`${file} exited with code ${result.code}`), { + code: result.code, + stderr: result.stderr, + }); + } + return { stdout: result.stdout, stderr: result.stderr }; + }; +} + function safeNotify( ctx: ExtensionCommandContext, message: string, @@ -77,6 +95,7 @@ function notifyBrowserResult( export default function reviewTutorExtension(pi: ExtensionAPI): void { const lifecycle = createServerLifecycle(); const execFile = createExecFileAdapter(); + const hostExecFile = piExecFile(pi); pi.registerCommand("review-tutor", { description: "Open the local Review Tutor for a PR, diff, commit, or pasted code", @@ -93,7 +112,7 @@ export default function reviewTutorExtension(pi: ExtensionAPI): void { const server = await lifecycle.start(async (startupSignal) => { const cwd = await realpath(ctx.cwd); - const canonicalRepo = await resolveRepository(cwd, execFile); + const canonicalRepo = await resolveRepository(cwd, hostExecFile); const piModels = modelChoices(ctx); if (!piModels.length) { throw new Error( @@ -114,7 +133,7 @@ export default function reviewTutorExtension(pi: ExtensionAPI): void { if (!server) return; safeStatus(ctx, "Review Tutor running"); - const opened = await openInBrowser(server.url, process.platform, execFile); + const opened = await openInBrowser(server.url, process.platform, hostExecFile); notifyBrowserResult(ctx, opened, server.url); } catch (error) { safeStatus(ctx); diff --git a/packages/review-tutor/package.json b/packages/review-tutor/package.json index c9f7275..331ec53 100644 --- a/packages/review-tutor/package.json +++ b/packages/review-tutor/package.json @@ -10,7 +10,7 @@ "skills": ["skills/review-tutor"] }, "bin": { - "review-tutor": "dist/cli.js" + "review-tutor": "dist/bin.js" }, "files": [ "dist", @@ -20,7 +20,7 @@ "README.md" ], "scripts": { - "build": "tsup src/cli.ts --format esm --clean --splitting false --out-dir dist", + "build": "tsup src/bin.ts --format esm --clean --splitting false --out-dir dist", "test": "cd ../.. && vitest run packages/review-tutor/test", "typecheck": "tsc -p ../../tsconfig.json --noEmit" }, diff --git a/packages/review-tutor/src/bin.ts b/packages/review-tutor/src/bin.ts new file mode 100644 index 0000000..e9b43aa --- /dev/null +++ b/packages/review-tutor/src/bin.ts @@ -0,0 +1,12 @@ +#!/usr/bin/env node +import { fileURLToPath } from "node:url"; +import { nodeCliDeps, runCli } from "./cli.ts"; + +/** + * The installed `review-tutor` is a symlink into this file, so it can never + * compare argv[1] with its own URL; this entry exists only to be run. + */ +process.exitCode = await runCli( + process.argv.slice(2), + nodeCliDeps(fileURLToPath(import.meta.url)), +); diff --git a/packages/review-tutor/src/cli-support.ts b/packages/review-tutor/src/cli-support.ts index 506cf96..66c9c3f 100644 --- a/packages/review-tutor/src/cli-support.ts +++ b/packages/review-tutor/src/cli-support.ts @@ -7,6 +7,8 @@ import type { ReviewTutorServer } from "./server.ts"; type NodeExecFile = typeof nodeExecFile; const COMMAND_BUFFER = 1024 * 1024; +const REPOSITORY_TIMEOUT_MS = 5_000; +const BROWSER_TIMEOUT_MS = 10_000; export function createExecFileAdapter(execFile: NodeExecFile = nodeExecFile): ExecFile { return (file, argv, options) => new Promise((resolve, reject) => { @@ -14,7 +16,7 @@ export function createExecFileAdapter(execFile: NodeExecFile = nodeExecFile): Ex cwd: options.cwd, encoding: options.encoding, maxBuffer: options.maxBuffer, - timeout: 30_000, + timeout: options.timeoutMs ?? 30_000, shell: false, ...(options.signal ? { signal: options.signal } : {}), }, (error, stdout, stderr) => { @@ -56,6 +58,7 @@ export async function resolveRepository(cwd: string, execFile: ExecFile): Promis cwd, encoding: "utf8", maxBuffer: COMMAND_BUFFER, + timeoutMs: REPOSITORY_TIMEOUT_MS, })); } catch (error) { const code = (error as { code?: unknown }).code; @@ -85,6 +88,7 @@ export async function openInBrowser( cwd: process.cwd(), encoding: "utf8", maxBuffer: COMMAND_BUFFER, + timeoutMs: BROWSER_TIMEOUT_MS, }); return true; } catch { diff --git a/packages/review-tutor/src/cli.ts b/packages/review-tutor/src/cli.ts index c2b4ee9..7e04c60 100644 --- a/packages/review-tutor/src/cli.ts +++ b/packages/review-tutor/src/cli.ts @@ -1,8 +1,6 @@ -#!/usr/bin/env node import { spawn } from "node:child_process"; import { realpath } from "node:fs/promises"; import type { Readable } from "node:stream"; -import { fileURLToPath, pathToFileURL } from "node:url"; import { createExecFileAdapter, openInBrowser, @@ -89,14 +87,21 @@ export function parseArguments(argv: readonly string[]): CliOptions { return options; } -/** Zero clients for one whole window, not merely at the moment of a check. */ +/** + * Zero clients for one whole window. A page that connects and leaves entirely + * between two polls still shows up as a new connection generation, so it + * restarts the window instead of being missed. + */ export class IdleTracker { private idleSince: number | undefined; + private generation: number | undefined; constructor(private readonly windowMs = IDLE_EXIT_MS) {} - expired(clientCount: number, now: number): boolean { - if (clientCount > 0) { + expired(clientCount: number, generation: number, now: number): boolean { + const moved = this.generation !== undefined && generation !== this.generation; + this.generation = generation; + if (clientCount > 0 || moved) { this.idleSince = undefined; return false; } @@ -109,7 +114,7 @@ function waitForIdle(server: StartedReviewTutorServer): Promise { return new Promise((resolve) => { const tracker = new IdleTracker(); const timer = setInterval(() => { - if (!tracker.expired(server.clientCount(), Date.now())) return; + if (!tracker.expired(server.clientCount(), server.connectionGeneration(), Date.now())) return; clearInterval(timer); resolve(); }, IDLE_CHECK_MS); @@ -147,15 +152,17 @@ async function runServer(options: CliOptions, deps: CliDeps): Promise { ...(options.source ? { initialSource: options.source } : {}), ...(options.home ? { home: options.home } : {}), }); + // Armed before the browser call so a signal during that call still shuts the server down once. + const stopped = Promise.race([ + new Promise((resolve) => deps.signals(resolve)), + ...(options.serveDetached ? [waitForIdle(server)] : []), + ]); deps.stderr(`${await discoverySummary(registry)}\n`); deps.stdout(`${server.url}\n`); if (options.open && !options.serveDetached && !await deps.openInBrowser(server.url)) { deps.stderr("Could not open a browser. Open the URL above.\n"); } - await Promise.race([ - new Promise((resolve) => deps.signals(resolve)), - ...(options.serveDetached ? [waitForIdle(server)] : []), - ]); + await stopped; await server.close(); return 0; } @@ -181,6 +188,8 @@ function readUrlLine(child: DetachedChild, timeoutMs: number): Promise { const fail = (reason: string): void => { clearTimeout(timer); stdout.off("data", onData); + stdout.destroy(); + child.unref(); reject(new Error(reason)); }; timer = setTimeout(() => { @@ -251,8 +260,3 @@ export function nodeCliDeps(scriptPath: string): CliDeps { }), }; } - -const entry = process.argv[1]; -if (entry && pathToFileURL(entry).href === import.meta.url) { - process.exitCode = await runCli(process.argv.slice(2), nodeCliDeps(fileURLToPath(import.meta.url))); -} diff --git a/packages/review-tutor/src/inputs.ts b/packages/review-tutor/src/inputs.ts index 202dca3..3b438ba 100644 --- a/packages/review-tutor/src/inputs.ts +++ b/packages/review-tutor/src/inputs.ts @@ -10,6 +10,7 @@ export type ExecFile = ( maxBuffer: number; encoding: "utf8"; signal?: AbortSignal; + timeoutMs?: number; }, ) => Promise<{ stdout: string; stderr: string }>; diff --git a/packages/review-tutor/src/server.ts b/packages/review-tutor/src/server.ts index c130ef8..843b11b 100644 --- a/packages/review-tutor/src/server.ts +++ b/packages/review-tutor/src/server.ts @@ -32,9 +32,10 @@ export interface ReviewTutorServer { port: number; } -/** A started server also reports its live SSE clients, which is how a detached CLI knows it is idle. */ +/** A started server also reports its SSE traffic, which is how a detached CLI knows it is idle. */ export interface StartedReviewTutorServer extends ReviewTutorServer { clientCount(): number; + connectionGeneration(): number; } const responseHeaders = { @@ -209,6 +210,7 @@ export async function startReviewTutorServer(options: ServerOptions): Promise hub.clientCount, + connectionGeneration: () => hub.connectionGeneration, }; } diff --git a/packages/review-tutor/src/sse.ts b/packages/review-tutor/src/sse.ts index cd0cb78..e88c9d7 100644 --- a/packages/review-tutor/src/sse.ts +++ b/packages/review-tutor/src/sse.ts @@ -3,6 +3,7 @@ import type { SseEvent, SseEventType } from "./protocol.ts"; export class SseHub { private next = 1; + private connections = 0; private readonly ring: SseEvent[] = []; private readonly clients = new Set(); @@ -12,6 +13,11 @@ export class SseHub { return this.clients.size; } + /** Counts every client ever attached, so a connection that lands between two polls is still visible. */ + get connectionGeneration(): number { + return this.connections; + } + emit(type: SseEventType, data: unknown): SseEvent { const event = { id: this.next++, type, data }; this.ring.push(event); @@ -38,6 +44,7 @@ export class SseHub { connect(response: ServerResponse, since: number): () => void { if (this.isDead(response)) return () => {}; + this.connections += 1; this.clients.add(response); for (const event of this.since(since)) { if (!this.write(response, this.format(event))) { diff --git a/packages/review-tutor/test/cli.test.ts b/packages/review-tutor/test/cli.test.ts index a32e20b..13a07fb 100644 --- a/packages/review-tutor/test/cli.test.ts +++ b/packages/review-tutor/test/cli.test.ts @@ -22,8 +22,12 @@ class FakeChild extends EventEmitter implements DetachedChild { kill = vi.fn(() => true); } -function fakeServer(close = vi.fn(async () => {}), clientCount = () => 0): StartedReviewTutorServer { - return { url: URL_LINE, token: "token", port: 4321, close, clientCount }; +function fakeServer( + close = vi.fn(async () => {}), + clientCount = () => 0, + connectionGeneration = () => 0, +): StartedReviewTutorServer { + return { url: URL_LINE, token: "token", port: 4321, close, clientCount, connectionGeneration }; } interface Harness { @@ -176,6 +180,23 @@ describe("foreground run", () => { await expect(run).resolves.toBe(0); }); + it("shuts down once for a signal that arrives while the browser is still opening", async () => { + let openBrowser!: (opened: boolean) => void; + const context = harness({ + openInBrowser: vi.fn(() => new Promise((resolve) => { openBrowser = resolve; })), + }); + const run = runCli([], context.deps); + await settle(() => openBrowser !== undefined); + + context.signal(); + await settle(); + expect(context.close).not.toHaveBeenCalled(); + + openBrowser(true); + await expect(run).resolves.toBe(0); + expect(context.close).toHaveBeenCalledTimes(1); + }); + it("exits 1 when the repository cannot be resolved", async () => { const context = harness({ execFile: async () => { throw Object.assign(new Error("not a repository"), { code: 128 }); }, @@ -208,7 +229,10 @@ describe("detached handshake", () => { await settle(() => context.children.length > 0); await vi.advanceTimersByTimeAsync(30_000); await expect(run).resolves.toBe(1); - expect(context.children[0]!.kill).toHaveBeenCalledWith("SIGTERM"); + const child = context.children[0]!; + expect(child.kill).toHaveBeenCalledWith("SIGTERM"); + expect(child.stdout.destroyed).toBe(true); + expect(child.unref).toHaveBeenCalledTimes(1); expect(context.err.join("")).toContain("expected a URL within 30 seconds"); expect(context.out).toEqual([]); }); @@ -220,26 +244,38 @@ describe("detached handshake", () => { context.children[0]!.emit("exit", 1, null); await expect(run).resolves.toBe(1); expect(context.err.join("")).toContain("exited before reporting a URL"); + expect(context.children[0]!.stdout.destroyed).toBe(true); + expect(context.children[0]!.unref).toHaveBeenCalledTimes(1); }); }); describe("detached idle exit", () => { it("expires only after a whole window without clients", () => { const tracker = new IdleTracker(1_000); - expect(tracker.expired(0, 0)).toBe(false); - expect(tracker.expired(0, 900)).toBe(false); - expect(tracker.expired(1, 950)).toBe(false); - expect(tracker.expired(0, 1_000)).toBe(false); - expect(tracker.expired(0, 1_999)).toBe(false); - expect(tracker.expired(0, 2_000)).toBe(true); + expect(tracker.expired(0, 0, 0)).toBe(false); + expect(tracker.expired(0, 0, 900)).toBe(false); + expect(tracker.expired(1, 1, 950)).toBe(false); + expect(tracker.expired(0, 1, 1_000)).toBe(false); + expect(tracker.expired(0, 1, 1_999)).toBe(false); + expect(tracker.expired(0, 1, 2_000)).toBe(true); + }); + + it("restarts the window for a connection that came and went between two polls", () => { + const tracker = new IdleTracker(1_000); + expect(tracker.expired(0, 3, 0)).toBe(false); + expect(tracker.expired(0, 4, 900)).toBe(false); + expect(tracker.expired(0, 4, 1_000)).toBe(false); + expect(tracker.expired(0, 4, 1_999)).toBe(false); + expect(tracker.expired(0, 4, 2_000)).toBe(true); }); it("closes a detached server once it has been idle for the whole window", async () => { vi.useFakeTimers(); let clients = 1; + let generation = 1; const close = vi.fn(async () => {}); const context = harness({ - startServer: async () => fakeServer(close, () => clients), + startServer: async () => fakeServer(close, () => clients, () => generation), }); const run = runCli(["--serve-detached", "--no-open"], context.deps); @@ -255,6 +291,26 @@ describe("detached idle exit", () => { expect(close).toHaveBeenCalledTimes(1); }); + it("delays exit by a full window when a page connects between two polls", async () => { + vi.useFakeTimers(); + let generation = 1; + const close = vi.fn(async () => {}); + const context = harness({ + startServer: async () => fakeServer(close, () => 0, () => generation), + }); + + const run = runCli(["--serve-detached", "--no-open"], context.deps); + await settle(() => context.out.length > 0); + await vi.advanceTimersByTimeAsync(25 * 60_000); + generation += 1; + await vi.advanceTimersByTimeAsync(10 * 60_000); + expect(close).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(25 * 60_000); + await expect(run).resolves.toBe(0); + expect(close).toHaveBeenCalledTimes(1); + }); + it("never opens a browser from the detached child", async () => { const context = harness(); const run = runCli(["--serve-detached"], context.deps); diff --git a/packages/review-tutor/test/extension-host-exec.test.ts b/packages/review-tutor/test/extension-host-exec.test.ts new file mode 100644 index 0000000..a72f12e --- /dev/null +++ b/packages/review-tutor/test/extension-host-exec.test.ts @@ -0,0 +1,79 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const url = "http://127.0.0.1:1/?session=secret"; +const startReviewTutorServer = vi.fn(async () => ({ + url, + token: "secret", + port: 1, + close: async () => {}, + clientCount: () => 0, + connectionGeneration: () => 0, +})); +const nodeExecFile = vi.fn(() => { + throw new Error("node:child_process must not run host operations under Pi"); +}); + +vi.mock("../src/server.ts", () => ({ startReviewTutorServer })); +vi.mock("node:child_process", () => ({ execFile: nodeExecFile })); + +const { default: reviewTutorExtension } = await import("../extensions/review-tutor.ts"); + +const browserCommand = process.platform === "darwin" + ? "open" + : process.platform === "win32" + ? "cmd" + : "xdg-open"; +const browserArgs = process.platform === "win32" ? ["/c", "start", "", url] : [url]; +const roots: string[] = []; + +afterEach(async () => { + await Promise.all(roots.splice(0).map((path) => rm(path, { recursive: true, force: true }))); +}); + +async function runCommand() { + const cwd = await mkdtemp(join(tmpdir(), "review-tutor-host-")); + roots.push(cwd); + const exec = vi.fn(async (file: string) => ({ + code: 0, + stdout: file === "git" ? `${cwd}\n` : "", + stderr: "", + })); + const notify = vi.fn(); + const handlers = new Map Promise }>(); + const pi = { + exec, + registerCommand: (name: string, spec: never) => { handlers.set(name, spec); }, + on: () => {}, + }; + reviewTutorExtension(pi as never); + await handlers.get("review-tutor")!.handler("", { + mode: "tui", + cwd, + scopedModels: [], + modelRegistry: { getAvailable: () => [{ provider: "provider", id: "model", name: "Model", reasoning: true }] }, + ui: { notify, setStatus: vi.fn() }, + }); + return { cwd, exec, notify }; +} + +describe("extension host execution", () => { + it("resolves the repository and opens the browser through pi.exec with the host timeouts", async () => { + const { cwd, exec, notify } = await runCommand(); + + expect(exec).toHaveBeenNthCalledWith(1, "git", ["rev-parse", "--show-toplevel"], { + cwd, + timeout: 5_000, + }); + expect(exec).toHaveBeenNthCalledWith(2, browserCommand, browserArgs, { + cwd: process.cwd(), + timeout: 10_000, + }); + expect(exec).toHaveBeenCalledTimes(2); + expect(nodeExecFile).not.toHaveBeenCalled(); + expect(notify).toHaveBeenCalledWith("Review Tutor opened.", "info"); + expect(startReviewTutorServer).toHaveBeenCalledTimes(1); + }); +}); From f2b0f860f9b3c88e44598c095d72f848afa603fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 12:15:33 -0300 Subject: [PATCH 4/6] fix(review-tutor): refuse sources with shell or Git metacharacters in the CLI --- packages/review-tutor/src/cli.ts | 7 ++++++- packages/review-tutor/test/cli.test.ts | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/review-tutor/src/cli.ts b/packages/review-tutor/src/cli.ts index 7e04c60..b86ff89 100644 --- a/packages/review-tutor/src/cli.ts +++ b/packages/review-tutor/src/cli.ts @@ -74,6 +74,8 @@ function applyFlag(options: CliOptions, argv: readonly string[], index: number): return index; } +const SOURCE_CHARACTERS = /^[A-Za-z0-9._/:@#~^-]+$/; + export function parseArguments(argv: readonly string[]): CliOptions { const options: CliOptions = { open: true, detach: false, serveDetached: false, help: false }; const positional: string[] = []; @@ -83,7 +85,10 @@ export function parseArguments(argv: readonly string[]): CliOptions { else positional.push(argument); } if (positional.length > 1) throw new UsageError(`unexpected argument ${positional[1]!}`); - options.source = sourceFromArgument(positional[0] ?? "worktree"); + const source = positional[0] ?? "worktree"; + // Sources are revisions, ranges, or GitHub URLs; anything else is refused before it can reach a shell or Git. + if (!SOURCE_CHARACTERS.test(source)) throw new UsageError("source may only contain letters, digits, and . _ / : @ # ~ ^ -"); + options.source = sourceFromArgument(source); return options; } diff --git a/packages/review-tutor/test/cli.test.ts b/packages/review-tutor/test/cli.test.ts index 13a07fb..6f134ca 100644 --- a/packages/review-tutor/test/cli.test.ts +++ b/packages/review-tutor/test/cli.test.ts @@ -127,6 +127,17 @@ describe("command line parsing", () => { }, ); + it.each([["$(id)"], ["a b"], ["x;y"], ["`ls`"], ["'q'"]])("refuses the source %j before it reaches Git or a shell", (source) => { + expect(() => parseArguments([source])).toThrow(/source may only contain/); + }); + + it.each([["worktree"], ["staged"], ["main...HEAD"], ["abc123"], ["https://github.com/pickforge/pickforge-platform/pull/83"], ["v0.12.0~1^2"]])( + "accepts the source %s", + (source) => { + expect(parseArguments([source]).source).toBeDefined(); + }, + ); + it("reports usage on stderr and exits 2 for an unknown flag", async () => { const { deps, out, err } = harness(); await expect(runCli(["--nope"], deps)).resolves.toBe(2); From a06dd84b470965d594f7cd1bb7bf7798f700f2b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 16:44:48 -0300 Subject: [PATCH 5/6] test(review-tutor): yield to the real event loop in the CLI test barrier --- packages/review-tutor/test/cli.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/review-tutor/test/cli.test.ts b/packages/review-tutor/test/cli.test.ts index 6f134ca..68782a1 100644 --- a/packages/review-tutor/test/cli.test.ts +++ b/packages/review-tutor/test/cli.test.ts @@ -1,5 +1,6 @@ import { EventEmitter } from "node:events"; import { PassThrough } from "node:stream"; +import { setImmediate as yieldToEventLoop } from "node:timers"; import { afterEach, describe, expect, it, vi } from "vitest"; import { IdleTracker, @@ -85,9 +86,10 @@ function harness(overrides: Partial = {}): Harness { /** Yields to the real event loop under both timer modes so pending file-system work can land. */ async function settle(check: () => boolean = () => true): Promise { for (let attempt = 0; attempt < 200; attempt += 1) { - await (vi.isFakeTimers() - ? vi.advanceTimersByTimeAsync(1) - : new Promise((resolve) => setTimeout(resolve, 1))); + // node:timers keeps the real setImmediate even while the global one is faked, + // so file-system callbacks (realpath, spawn) can land before the next check. + await new Promise((resolve) => yieldToEventLoop(resolve)); + if (vi.isFakeTimers()) await vi.advanceTimersByTimeAsync(1); if (check()) return; } throw new Error("test barrier failed: expected condition within 200 attempts"); From 40cbc82fdad7c50e47572448f573043a9e09223d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 16:47:52 -0300 Subject: [PATCH 6/6] test(review-tutor): make the CLI test barrier deadline-based --- packages/review-tutor/test/cli.test.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/review-tutor/test/cli.test.ts b/packages/review-tutor/test/cli.test.ts index 68782a1..e2b70a6 100644 --- a/packages/review-tutor/test/cli.test.ts +++ b/packages/review-tutor/test/cli.test.ts @@ -1,6 +1,6 @@ import { EventEmitter } from "node:events"; import { PassThrough } from "node:stream"; -import { setImmediate as yieldToEventLoop } from "node:timers"; +import { setTimeout as realSetTimeout } from "node:timers"; import { afterEach, describe, expect, it, vi } from "vitest"; import { IdleTracker, @@ -85,14 +85,15 @@ function harness(overrides: Partial = {}): Harness { /** Yields to the real event loop under both timer modes so pending file-system work can land. */ async function settle(check: () => boolean = () => true): Promise { - for (let attempt = 0; attempt < 200; attempt += 1) { - // node:timers keeps the real setImmediate even while the global one is faked, - // so file-system callbacks (realpath, spawn) can land before the next check. - await new Promise((resolve) => yieldToEventLoop(resolve)); + // node:timers keeps the real setTimeout even while the global one is faked, so + // file-system callbacks (realpath) can land on a slow runner before the next check. + const deadline = process.hrtime.bigint() + 5_000_000_000n; + while (process.hrtime.bigint() < deadline) { + await new Promise((resolve) => realSetTimeout(resolve, 2)); if (vi.isFakeTimers()) await vi.advanceTimersByTimeAsync(1); if (check()) return; } - throw new Error("test barrier failed: expected condition within 200 attempts"); + throw new Error("test barrier failed: expected condition within 5 s"); } afterEach(() => {