From 44c914feb71f0ea5f91bff3079fc22bf9ee64f66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Tue, 25 Aug 2026 16:40:20 -0300 Subject: [PATCH 1/3] feat(review-tutor): add harness connector boundary --- bun.lock | 5 +- packages/review-tutor/README.md | 6 +- .../review-tutor/extensions/review-tutor.ts | 10 +- packages/review-tutor/package.json | 3 + packages/review-tutor/src/connectors/pi.ts | 141 ++++++++++++++++ .../review-tutor/src/connectors/redact.ts | 12 ++ .../review-tutor/src/connectors/registry.ts | 51 ++++++ packages/review-tutor/src/connectors/types.ts | 55 +++++++ packages/review-tutor/src/export-html.ts | 21 ++- packages/review-tutor/src/flags.ts | 34 ++++ packages/review-tutor/src/pi-json.ts | 101 ------------ packages/review-tutor/src/protocol.ts | 6 +- packages/review-tutor/src/runner-execution.ts | 34 ++-- packages/review-tutor/src/runner.ts | 54 ++++-- packages/review-tutor/src/server-session.ts | 29 +++- packages/review-tutor/src/server.ts | 19 ++- packages/review-tutor/test/connectors.test.ts | 155 ++++++++++++++++++ packages/review-tutor/test/core.test.ts | 2 +- packages/review-tutor/test/runner.test.ts | 5 +- .../test/server-extension.test.ts | 56 ++++++- packages/review-tutor/test/structure.test.ts | 10 +- tsconfig.base.json | 3 + vitest.config.ts | 5 + 23 files changed, 655 insertions(+), 162 deletions(-) create mode 100644 packages/review-tutor/src/connectors/pi.ts create mode 100644 packages/review-tutor/src/connectors/redact.ts create mode 100644 packages/review-tutor/src/connectors/registry.ts create mode 100644 packages/review-tutor/src/connectors/types.ts create mode 100644 packages/review-tutor/src/flags.ts delete mode 100644 packages/review-tutor/src/pi-json.ts create mode 100644 packages/review-tutor/test/connectors.test.ts diff --git a/bun.lock b/bun.lock index 6d19ebe..b432f43 100644 --- a/bun.lock +++ b/bun.lock @@ -46,11 +46,14 @@ "packages/review-tutor": { "name": "@pickforge/review-tutor", "version": "0.0.0", + "dependencies": { + "@pickforge/flags": "workspace:*", + }, "devDependencies": { "@earendil-works/pi-coding-agent": "^0.84.2", }, "peerDependencies": { - "@earendil-works/pi-coding-agent": "*", + "@earendil-works/pi-coding-agent": ">=0.83.0", }, }, "packages/sync": { diff --git a/packages/review-tutor/README.md b/packages/review-tutor/README.md index b7e3291..9c5de17 100644 --- a/packages/review-tutor/README.md +++ b/packages/review-tutor/README.md @@ -1,6 +1,6 @@ # @pickforge/review-tutor -Review Tutor is a local-first, browser-based companion for guided reviews of PRs, diffs, and code. Questions run in an isolated tutor child, never in the main Pi conversation. GitHub remains the source of truth. Review Tutor never approves, comments on, or edits a PR. Answers, notes, and quiz results stay local. +Review Tutor is a local-first, browser-based companion for guided reviews of PRs, diffs, and code. Questions run in an isolated tutor child, never in the main Pi conversation. GitHub remains the source of truth. Review Tutor never approves, comments on, or edits a PR. Answers, notes, and quiz results stay local. Harness connectors are tracked in [#63](https://github.com/pickforge/pickforge-platform/issues/63). ## Install @@ -44,6 +44,10 @@ With no argument, choose a source in the browser. The browser supports worktree, 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. A scope entry with an explicit level, such as `gpt-5.6-sol:high`, pins the tutor to that level. +## Harness connectors + +A harness connector owns model discovery, isolated invocation, and stream parsing while the shared runner owns process lifetime, bounds, and cancellation. Pi is the only registered connector today; Claude Code and Codex support is tracked in [#63](https://github.com/pickforge/pickforge-platform/issues/63). The `reviewTutorHarnessConnectors` flag defaults off. For local testing on main, set `REVIEW_TUTOR_FLAGS=reviewTutorHarnessConnectors` before starting Pi. Child processes receive only the shared environment allowlist plus keys explicitly declared by their connector, and runner failures redact common API keys, bearer credentials, and tokens before leaving the process boundary. + ## Local data By default, data is stored under: diff --git a/packages/review-tutor/extensions/review-tutor.ts b/packages/review-tutor/extensions/review-tutor.ts index 9c254f1..b690ccf 100644 --- a/packages/review-tutor/extensions/review-tutor.ts +++ b/packages/review-tutor/extensions/review-tutor.ts @@ -5,6 +5,8 @@ import type { ExtensionAPI, ExtensionCommandContext, } from "@earendil-works/pi-coding-agent"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; import type { ExecFile } from "../src/inputs.ts"; import type { ModelChoice, SourceRequest } from "../src/protocol.ts"; import { @@ -252,16 +254,18 @@ 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 models = modelChoices(ctx); - if (!models.length) { + const piModels = modelChoices(ctx); + if (!piModels.length) { throw new Error( "model snapshot failed: expected at least one available model; configure a Pi model and retry", ); } + const flags = createReviewTutorFlags(); + const registry = createConnectorRegistry({ flags, piModels }); return startReviewTutorServer({ cwd, canonicalRepo, - models, + registry, skillPath, initialSource: sourceFromArgument(args), startupSignal, diff --git a/packages/review-tutor/package.json b/packages/review-tutor/package.json index 91a3d85..9e067b3 100644 --- a/packages/review-tutor/package.json +++ b/packages/review-tutor/package.json @@ -13,6 +13,9 @@ "test": "cd ../.. && vitest run packages/review-tutor/test", "typecheck": "tsc -p ../../tsconfig.json --noEmit" }, + "dependencies": { + "@pickforge/flags": "workspace:*" + }, "peerDependencies": { "@earendil-works/pi-coding-agent": ">=0.83.0" }, diff --git a/packages/review-tutor/src/connectors/pi.ts b/packages/review-tutor/src/connectors/pi.ts new file mode 100644 index 0000000..4356788 --- /dev/null +++ b/packages/review-tutor/src/connectors/pi.ts @@ -0,0 +1,141 @@ +import type { + ConnectorRequest, + Discovery, + DiscoveryDeps, + HarnessConnector, + ParseSink, + ParsedAnswer, + SpawnSpec, +} from "./types.ts"; +import { ConnectorError } from "./types.ts"; + +interface PiContent { + type?: unknown; + text?: unknown; +} + +interface PiMessage { + role?: unknown; + content?: unknown; + usage?: unknown; +} + +interface PiEvent { + type?: unknown; + message?: PiMessage; + messages?: PiMessage[]; + assistantMessageEvent?: { + type?: unknown; + delta?: unknown; + }; +} + +function parseEvent(line: string): PiEvent { + try { + return JSON.parse(line) as PiEvent; + } catch { + throw new ConnectorError( + "malformed_output", + "Pi JSON parsing failed: expected one valid JSON object per LF-delimited line; inspect child output and retry", + ); + } +} + +function finalAnswer(messages: PiMessage[] | undefined): string | undefined { + if (!Array.isArray(messages)) return undefined; + const final = messages.filter((message) => message?.role === "assistant").at(-1); + if (!Array.isArray(final?.content)) { + return typeof final?.content === "string" ? final.content : ""; + } + return (final.content as PiContent[]) + .filter((content) => content?.type === "text") + .map((content) => typeof content.text === "string" ? content.text : "") + .join(""); +} + +export class PiConnector implements HarnessConnector { + readonly id = "pi" as const; + readonly label = "Pi"; + private answer?: string; + private answerUsage?: Record; + + async discover(deps: DiscoveryDeps): Promise { + return { + available: true, + version: deps.piVersion ?? "unknown", + models: deps.piModels.map((model) => ({ ...model, id: `pi:${model.id}` })), + }; + } + + spawnSpec(request: ConnectorRequest): SpawnSpec { + this.answer = undefined; + this.answerUsage = undefined; + const [provider, ...modelParts] = request.model.split("/"); + return { + command: "pi", + args: [ + "--mode", "json", "--no-extensions", "--no-skills", "--no-prompt-templates", + "--no-context-files", "--no-session", "-p", "--tools", "read,grep,find,ls", + "--provider", provider!, "--model", modelParts.join("/"), "--thinking", request.thinking, + ], + }; + } + + parseLine(line: string, sink: ParseSink): void { + const event = parseEvent(line); + const update = event.type === "message_update" ? event.assistantMessageEvent : undefined; + if (update?.type === "text_delta" && typeof update.delta === "string") { + sink.delta(update.delta); + } + if (event.type === "message_end" && event.message?.usage && typeof event.message.usage === "object") { + this.answerUsage = event.message.usage as Record; + sink.usage(this.answerUsage); + } + if (event.type === "agent_end" && Array.isArray(event.messages)) { + const candidate = finalAnswer(event.messages); + if (this.answer === undefined || candidate?.trim()) { + this.answer = candidate; + if (candidate !== undefined) sink.final(candidate); + } + } + } + + finish(_sink: ParseSink): ParsedAnswer { + if (!this.answer?.trim()) { + throw new ConnectorError( + "empty_answer", + "Pi answer failed: expected a non-empty final assistant message in agent_end; choose another question or model and retry", + ); + } + return { answer: this.answer, ...(this.answerUsage ? { usage: this.answerUsage } : {}) }; + } +} + +export function parsePiJson() { + const connector = new PiConnector(); + let pending = ""; + const sink = { + events: [] as Array<{ type: "delta"; text: string }>, + delta(text: string) { this.events.push({ type: "delta", text }); }, + usage() {}, + final() {}, + }; + return { + push(chunk: string) { + pending += chunk; + sink.events = []; + for (;;) { + const newline = pending.indexOf("\n"); + if (newline < 0) break; + const line = pending.slice(0, newline); + pending = pending.slice(newline + 1); + if (line) connector.parseLine(line, sink); + } + return sink.events; + }, + finish() { + if (pending.trim()) connector.parseLine(pending, sink); + return connector.finish(sink); + }, + }; +} diff --git a/packages/review-tutor/src/connectors/redact.ts b/packages/review-tutor/src/connectors/redact.ts new file mode 100644 index 0000000..ed4fd81 --- /dev/null +++ b/packages/review-tutor/src/connectors/redact.ts @@ -0,0 +1,12 @@ +const PATTERNS = [ + /sk-[A-Za-z0-9_-]{8,}/g, + /Bearer\s+\S+/gi, + /(?:ANTHROPIC|OPENAI|OPENROUTER|XAI)_API_KEY=\S+/g, + /token=\S+/gi, + /ghp_\w+/g, + /gho_\w+/g, +] as const; + +export function redact(value: string): string { + return PATTERNS.reduce((redacted, pattern) => redacted.replace(pattern, "[redacted]"), value); +} diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts new file mode 100644 index 0000000..6e535ae --- /dev/null +++ b/packages/review-tutor/src/connectors/registry.ts @@ -0,0 +1,51 @@ +import type { ReviewTutorFlags } from "../flags.ts"; +import { PiConnector } from "./pi.ts"; +import type { + Discovery, + DiscoveryDeps, + HarnessConnector, + HarnessId, + ModelChoice, +} from "./types.ts"; + +export interface ConnectorRegistry { + connectors(): HarnessConnector[]; + byId(id: string): HarnessConnector | undefined; + resolve(modelId: string): { connector: HarnessConnector; model: string } | undefined; + discover(connector: HarnessConnector): Promise; +} + +export function createConnectorRegistry(options: { + flags: ReviewTutorFlags; + piModels: ModelChoice[]; + piVersion?: string; +}): ConnectorRegistry { + const pi = new PiConnector(); + const optionalConnectors: HarnessConnector[] = []; + const dependencies: DiscoveryDeps = { + piModels: options.piModels, + ...(options.piVersion ? { piVersion: options.piVersion } : {}), + }; + + const connectors = (): HarnessConnector[] => [ + pi, + ...(options.flags.isEnabled("reviewTutorHarnessConnectors") ? optionalConnectors : []), + ]; + + return { + connectors, + byId(id) { + return connectors().find((connector) => connector.id === id); + }, + resolve(modelId) { + const separator = modelId.indexOf(":"); + const harness = separator < 0 ? "pi" : modelId.slice(0, separator); + const connector = this.byId(harness as HarnessId); + if (!connector) return undefined; + return { connector, model: separator < 0 ? modelId : modelId.slice(separator + 1) }; + }, + discover(connector) { + return connector.discover(dependencies); + }, + }; +} diff --git a/packages/review-tutor/src/connectors/types.ts b/packages/review-tutor/src/connectors/types.ts new file mode 100644 index 0000000..9807f2d --- /dev/null +++ b/packages/review-tutor/src/connectors/types.ts @@ -0,0 +1,55 @@ +export type HarnessId = "pi" | "claude-code" | "codex"; + +export interface HarnessConnector { + readonly id: HarnessId; + readonly label: string; + discover(deps: DiscoveryDeps): Promise; + spawnSpec(request: ConnectorRequest): SpawnSpec; + parseLine(line: string, sink: ParseSink): void; + finish(sink: ParseSink): ParsedAnswer; + readonly envKeys?: readonly string[]; +} + +export interface DiscoveryDeps { + piModels: ModelChoice[]; + piVersion?: string; +} + +export type Discovery = + | { available: true; version: string; models: ModelChoice[] } + | { available: false; reason: string }; + +export interface ModelChoice { + id: string; + label: string; + thinkingLevels: string[]; +} + +export interface ConnectorRequest { + model: string; + thinking: string; + cwd: string; +} + +export interface SpawnSpec { + command: string; + args: string[]; +} + +export interface ParseSink { + delta(text: string): void; + usage(u: Record): void; + final(answer: string): void; +} + +export interface ParsedAnswer { + answer: string; + usage?: Record; +} + +export class ConnectorError extends Error { + constructor(readonly code: string, message: string) { + super(message); + this.name = "ConnectorError"; + } +} diff --git a/packages/review-tutor/src/export-html.ts b/packages/review-tutor/src/export-html.ts index b6b8bfd..d3b84b2 100644 --- a/packages/review-tutor/src/export-html.ts +++ b/packages/review-tutor/src/export-html.ts @@ -1,3 +1,4 @@ +import type { ConnectorRegistry } from "./connectors/registry.ts"; import type { LearningEntry, QuizOutcome } from "./protocol.ts"; const QUIZ_LABELS: Record = { @@ -16,7 +17,17 @@ function escapeHtml(value: unknown): string { })[character]!); } -function card(entry: LearningEntry): string { +function modelDetails(entry: LearningEntry, registry?: ConnectorRegistry): { harness: string; model: string } { + const separator = entry.modelId.indexOf(":"); + if (separator < 0) return { harness: "Pi", model: entry.modelId }; + const harnessId = entry.modelId.slice(0, separator); + return { + harness: registry?.byId(harnessId)?.label ?? harnessId, + model: entry.modelId.slice(separator + 1), + }; +} + +function card(entry: LearningEntry, registry?: ConnectorRegistry): string { const sourceUrl = entry.source.githubUrl ? `
GitHub source URL
${escapeHtml(entry.source.githubUrl)}
` : ""; @@ -33,6 +44,7 @@ function card(entry: LearningEntry): string { ? entry.preferences.comparisonLanguages.map(escapeHtml).join(", ") : "None"; const outcome = entry.quizOutcome ? QUIZ_LABELS[entry.quizOutcome] : "Not recorded"; + const details = modelDetails(entry, registry); return `

${escapeHtml(entry.source.label)}

@@ -41,7 +53,8 @@ function card(entry: LearningEntry): string {
Source label
${escapeHtml(entry.source.label)}
Source digest
${escapeHtml(entry.source.digest)}
${sourceUrl}${head}${file}${range} -
Model
${escapeHtml(entry.modelId)}
+
Harness
${escapeHtml(details.harness)}
+
Model
${escapeHtml(details.model)}
Explanation language
${escapeHtml(entry.preferences.explanationLanguage)}
Comparison languages
${comparisons}
Created
${escapeHtml(entry.createdAt)}
@@ -57,8 +70,8 @@ ${entry.source.kind === "pr" ? "

GitHub is the source of truth.

" : ""}
`; } -export function exportLearningHtml(entries: LearningEntry[]): string { - const cards = entries.length ? entries.map(card).join("") : "

No learning entries yet.

"; +export function exportLearningHtml(entries: LearningEntry[], registry?: ConnectorRegistry): string { + const cards = entries.length ? entries.map((entry) => card(entry, registry)).join("") : "

No learning entries yet.

"; return ` diff --git a/packages/review-tutor/src/flags.ts b/packages/review-tutor/src/flags.ts new file mode 100644 index 0000000..9890193 --- /dev/null +++ b/packages/review-tutor/src/flags.ts @@ -0,0 +1,34 @@ +import { + createFlags, + type FlagOverrideStore, + type Flags, +} from "@pickforge/flags"; + +const definitions = { + reviewTutorHarnessConnectors: { + description: "Show Claude Code and Codex harness connectors in Review Tutor", + default: false, + }, +} as const; + +export type ReviewTutorFlag = keyof typeof definitions; +export type ReviewTutorFlags = Flags; + +function environmentStore(value = process.env.REVIEW_TUTOR_FLAGS): FlagOverrideStore { + const enabled = new Set((value ?? "").split(",").map((key) => key.trim()).filter(Boolean)); + const overrides = new Map(); + for (const key of Object.keys(definitions)) { + if (enabled.has(key)) overrides.set(key, true); + } + return { + get: (key) => overrides.get(key), + set(key, next) { + if (next === undefined) overrides.delete(key); + else overrides.set(key, next); + }, + }; +} + +export function createReviewTutorFlags(store?: FlagOverrideStore): ReviewTutorFlags { + return createFlags(definitions, { store: store ?? environmentStore() }); +} diff --git a/packages/review-tutor/src/pi-json.ts b/packages/review-tutor/src/pi-json.ts deleted file mode 100644 index db37c24..0000000 --- a/packages/review-tutor/src/pi-json.ts +++ /dev/null @@ -1,101 +0,0 @@ -export interface PiResult { - answer: string; - usage?: Record; -} - -export type PiStreamEvent = { type: "delta"; text: string }; - -interface PiContent { - type?: unknown; - text?: unknown; -} - -interface PiMessage { - role?: unknown; - content?: unknown; - usage?: unknown; -} - -interface PiEvent { - type?: unknown; - message?: PiMessage; - messages?: PiMessage[]; - assistantMessageEvent?: { - type?: unknown; - delta?: unknown; - }; -} - -function finalAnswer(messages: PiMessage[] | undefined): string | undefined { - if (!Array.isArray(messages)) return undefined; - const final = messages.filter((message) => message?.role === "assistant").at(-1); - if (!Array.isArray(final?.content)) { - return typeof final?.content === "string" ? final.content : ""; - } - return (final.content as PiContent[]) - .filter((content) => content?.type === "text") - .map((content) => typeof content.text === "string" ? content.text : "") - .join(""); -} - -function delta(event: PiEvent): PiStreamEvent[] { - const update = event.type === "message_update" ? event.assistantMessageEvent : undefined; - return update?.type === "text_delta" && typeof update.delta === "string" - ? [{ type: "delta", text: update.delta }] - : []; -} - -export function parsePiJson() { - let pending = ""; - let answer: string | undefined; - let usage: Record | undefined; - - const parseLine = (raw: string): PiStreamEvent[] => { - let event: PiEvent; - try { - event = JSON.parse(raw) as PiEvent; - } catch { - throw new Error( - "Pi JSON parsing failed: expected one valid JSON object per LF-delimited line; inspect child output and retry", - ); - } - - if ( - event.type === "message_end" - && event.message?.usage - && typeof event.message.usage === "object" - ) { - usage = event.message.usage as Record; - } - - if (event.type === "agent_end" && Array.isArray(event.messages)) { - const candidate = finalAnswer(event.messages); - if (answer === undefined || candidate?.trim()) answer = candidate; - } - return delta(event); - }; - - return { - push(chunk: string): PiStreamEvent[] { - pending += chunk; - const output: PiStreamEvent[] = []; - for (;;) { - const newline = pending.indexOf("\n"); - if (newline < 0) break; - const raw = pending.slice(0, newline); - pending = pending.slice(newline + 1); - if (raw) output.push(...parseLine(raw)); - } - return output; - }, - finish(): PiResult { - if (pending.trim()) parseLine(pending); - if (!answer?.trim()) { - throw new Error( - "Pi answer failed: expected a non-empty final assistant message in agent_end; choose another question or model and retry", - ); - } - return { answer, ...(usage ? { usage } : {}) }; - }, - }; -} diff --git a/packages/review-tutor/src/protocol.ts b/packages/review-tutor/src/protocol.ts index f9d3c47..b048b91 100644 --- a/packages/review-tutor/src/protocol.ts +++ b/packages/review-tutor/src/protocol.ts @@ -56,11 +56,7 @@ export interface InputSnapshot { rangeTo?: string; } -export interface ModelChoice { - id: string; - label: string; - thinkingLevels: string[]; -} +export type { ModelChoice } from "./connectors/types.ts"; export interface LearningPreferences { explanationLanguage: string; diff --git a/packages/review-tutor/src/runner-execution.ts b/packages/review-tutor/src/runner-execution.ts index 31e829a..97c4c3b 100644 --- a/packages/review-tutor/src/runner-execution.ts +++ b/packages/review-tutor/src/runner-execution.ts @@ -1,6 +1,6 @@ import type { ChildProcessWithoutNullStreams } from "node:child_process"; import { StringDecoder } from "node:string_decoder"; -import { parsePiJson, type PiResult } from "./pi-json.ts"; +import type { HarnessConnector, ParsedAnswer, ParseSink } from "./connectors/types.ts"; import { LIMITS } from "./protocol.ts"; export interface ExecutionOptions { @@ -14,9 +14,9 @@ export interface ExecutionOptions { export class RunnerExecution { readonly completion: Promise; - private readonly parser = parsePiJson(); private readonly stdoutDecoder = new StringDecoder("utf8"); private readonly stderrDecoder = new StringDecoder("utf8"); + private pending = ""; private stdoutBytes = 0; private stderr = ""; private terminalError?: Error; @@ -24,18 +24,21 @@ export class RunnerExecution { private timeout?: ReturnType; private escalation?: ReturnType; private complete!: () => void; - private resolve!: (result: PiResult) => void; + private resolve!: (result: ParsedAnswer) => void; private reject!: (error: Error) => void; - readonly result: Promise; + readonly result: Promise; + private readonly sink: ParseSink; constructor( private readonly child: ChildProcessWithoutNullStreams, + private readonly connector: HarnessConnector, private readonly options: ExecutionOptions, - private readonly onDelta: (text: string) => void, + onDelta: (text: string) => void, private readonly onSettled: () => void, ) { + this.sink = { delta: onDelta, usage: () => {}, final: () => {} }; this.completion = new Promise((resolve) => { this.complete = resolve; }); - this.result = new Promise((resolve, reject) => { + this.result = new Promise((resolve, reject) => { this.resolve = resolve; this.reject = reject; }); @@ -43,6 +46,7 @@ export class RunnerExecution { } start(prompt: string): void { + if (this.terminalError || this.settled) return; this.timeout = this.options.setTimeout( () => this.stop(`timed out after ${this.options.timeoutMs} milliseconds`), this.options.timeoutMs, @@ -90,7 +94,7 @@ export class RunnerExecution { return; } try { this.consume(this.stdoutDecoder.write(chunk)); } catch (error) { - this.stop("Pi output handling failed", error instanceof Error ? error : new Error(String(error))); + this.stop("connector output handling failed", error instanceof Error ? error : new Error(String(error))); } }; @@ -106,7 +110,7 @@ export class RunnerExecution { }; private readonly onError = (error: Error): void => { - this.fail(new Error(`question child spawn failed: ${error.message}; verify Pi is installed and retry`)); + this.fail(new Error(`question child spawn failed: ${error.message}; verify the harness is installed and retry`)); }; private readonly onClose = (code: number | null, signal: NodeJS.Signals | null): void => { @@ -116,9 +120,10 @@ export class RunnerExecution { this.stderr += this.stderrDecoder.end(); if (this.terminalError) return this.fail(this.terminalError); if (code !== 0) return this.fail(new Error( - `question child failed: expected exit code 0, received ${String(code)}${signal ? ` (${signal})` : ""}; stderr tail: ${this.stderr.slice(-LIMITS.stderr)}; check Pi model access and retry`, + `question child failed: expected exit code 0, received ${String(code)}${signal ? ` (${signal})` : ""}; stderr tail: ${this.stderr.slice(-LIMITS.stderr)}; check model access and retry`, )); - const result = this.parser.finish(); + if (this.pending.trim()) this.connector.parseLine(this.pending, this.sink); + const result = this.connector.finish(this.sink); if (this.cleanup()) this.resolve(result); } catch (error) { this.fail(error instanceof Error ? error : new Error(String(error))); @@ -126,7 +131,14 @@ export class RunnerExecution { }; private consume(text: string): void { - for (const event of this.parser.push(text)) this.onDelta(event.text); + this.pending += text; + for (;;) { + const newline = this.pending.indexOf("\n"); + if (newline < 0) break; + const line = this.pending.slice(0, newline); + this.pending = this.pending.slice(newline + 1); + if (line) this.connector.parseLine(line, this.sink); + } } private fail(error: Error): void { diff --git a/packages/review-tutor/src/runner.ts b/packages/review-tutor/src/runner.ts index f348797..a2f1a46 100644 --- a/packages/review-tutor/src/runner.ts +++ b/packages/review-tutor/src/runner.ts @@ -3,7 +3,8 @@ import { type ChildProcessWithoutNullStreams, type SpawnOptionsWithoutStdio, } from "node:child_process"; -import type { PiResult } from "./pi-json.ts"; +import { redact } from "./connectors/redact.ts"; +import { ConnectorError, type HarnessConnector, type ParsedAnswer } from "./connectors/types.ts"; import { LIMITS } from "./protocol.ts"; import { RunnerExecution } from "./runner-execution.ts"; @@ -17,9 +18,12 @@ const KEYS = [ "XDG_DATA_DIRS", ] as const; -export function createChildEnvironment(source: NodeJS.ProcessEnv): NodeJS.ProcessEnv { +export function createChildEnvironment( + source: NodeJS.ProcessEnv, + extraKeys: readonly string[] = [], +): NodeJS.ProcessEnv { const env: NodeJS.ProcessEnv = {}; - for (const key of KEYS) { + for (const key of [...KEYS, ...extraKeys]) { if (source[key] !== undefined) env[key] = source[key]; } env.REVIEW_TUTOR_CHILD = "1"; @@ -49,6 +53,7 @@ export function platformTerminate( } interface RunnerOptions { + connector?: HarnessConnector; spawn?: Spawn; env?: NodeJS.ProcessEnv; terminate?: Terminate; @@ -61,22 +66,32 @@ interface RunnerOptions { } export interface RunRequest { - provider: string; + connector?: HarnessConnector; model: string; thinking: string; cwd: string; prompt: string; } +function safeError(error: unknown): Error { + const source = error instanceof Error ? error : new Error(String(error)); + const message = redact(source.message); + return source instanceof ConnectorError + ? new ConnectorError(source.code, message) + : new Error(message); +} + export class TutorRunner { private child?: ChildProcessWithoutNullStreams; private execution?: RunnerExecution; - private readonly options: Required> & { + private readonly defaultConnector?: HarnessConnector; + private readonly options: Required> & { terminate: Terminate; }; constructor(options: RunnerOptions = {}) { const platform = options.platform ?? process.platform; + this.defaultConnector = options.connector; this.options = { spawn: options.spawn ?? (nodeSpawn as Spawn), env: options.env ?? process.env, @@ -90,15 +105,22 @@ export class TutorRunner { }; } - async run(request: RunRequest, onDelta: (text: string) => void): Promise { + async run(request: RunRequest, onDelta: (text: string) => void): Promise { if (this.child) { throw new Error( "question runner failed: expected no running child, but one is active; wait or cancel it before retrying", ); } - const child = this.spawn(request); + const connector = request.connector ?? this.defaultConnector; + if (!connector) throw new Error("question runner failed: expected a harness connector; choose a model and retry"); + let child: ChildProcessWithoutNullStreams; + try { + child = this.spawn(request, connector); + } catch (error) { + throw safeError(error); + } this.child = child; - const execution = new RunnerExecution(child, this.options, onDelta, () => { + const execution = new RunnerExecution(child, connector, this.options, onDelta, () => { if (this.child === child) this.child = undefined; if (this.execution === execution) this.execution = undefined; }); @@ -106,28 +128,26 @@ export class TutorRunner { execution.start(request.prompt); try { return await execution.result; + } catch (error) { + throw safeError(error); } finally { if (this.child === child) this.child = undefined; if (this.execution === execution) this.execution = undefined; } } - private spawn(request: RunRequest): ChildProcessWithoutNullStreams { - const args = [ - "--mode", "json", "--no-extensions", "--no-skills", "--no-prompt-templates", - "--no-context-files", "--no-session", "-p", "--tools", "read,grep,find,ls", - "--provider", request.provider, "--model", request.model, "--thinking", request.thinking, - ]; + private spawn(request: RunRequest, connector: HarnessConnector): ChildProcessWithoutNullStreams { + const spec = connector.spawnSpec(request); try { - return this.options.spawn("pi", args, { + return this.options.spawn(spec.command, spec.args, { cwd: request.cwd, detached: this.options.platform !== "win32", - env: createChildEnvironment(this.options.env), + env: createChildEnvironment(this.options.env, connector.envKeys), stdio: ["pipe", "pipe", "pipe"], }); } catch (error) { throw new Error( - `question child spawn failed: ${error instanceof Error ? error.message : String(error)}; verify Pi is installed and retry`, + `question child spawn failed: ${error instanceof Error ? error.message : String(error)}; verify ${connector.label} is installed and retry`, ); } } diff --git a/packages/review-tutor/src/server-session.ts b/packages/review-tutor/src/server-session.ts index e41934d..858c904 100644 --- a/packages/review-tutor/src/server-session.ts +++ b/packages/review-tutor/src/server-session.ts @@ -1,5 +1,7 @@ import { randomUUID } from "node:crypto"; import type { IncomingMessage, ServerResponse } from "node:http"; +import type { ConnectorRegistry } from "./connectors/registry.ts"; +import type { HarnessConnector } from "./connectors/types.ts"; import { exportLearningHtml } from "./export-html.ts"; import { loadInput, type ExecFile } from "./inputs.ts"; import { appendEntry, foldLog, persistInput, updateEntry } from "./log.ts"; @@ -10,7 +12,7 @@ import type { SseHub } from "./sse.ts"; import { structureSnapshotWithNeighbours } from "./structure.ts"; export interface RunnerLike { - run(request: { provider: string; model: string; thinking: string; cwd: string; prompt: string }, delta: (text: string) => void): Promise<{ answer: string; usage?: Record }>; + run(request: { connector: HarnessConnector; model: string; thinking: string; cwd: string; prompt: string }, delta: (text: string) => void): Promise<{ answer: string; usage?: Record }>; cancel(reason?: string): void; shutdown(): Promise; } @@ -20,7 +22,9 @@ export interface SessionReply { status: number; value: unknown } interface SessionOptions { cwd: string; canonicalRepo: string; + registry: ConnectorRegistry; models: ModelChoice[]; + harnesses: Array<{ id: string; label: string; available: boolean; reason?: string }>; execFile: ExecFile; } @@ -147,9 +151,10 @@ export class ReviewTutorSession { } private async executeQuestion(id: string, view: QuestionView, ask: AskRequest, source: InputSnapshot): Promise { - const [provider, ...modelParts] = ask.modelId.split("/"); + const resolved = this.options.registry.resolve(ask.modelId); + if (!resolved) throw new Error("model selection failed: unknown harness; refresh state and retry"); const result = await this.runner.run({ - provider: provider!, model: modelParts.join("/"), thinking: ask.thinkingLevel, + connector: resolved.connector, model: resolved.model, thinking: ask.thinkingLevel, cwd: this.options.canonicalRepo, prompt: buildTutorPrompt(this.rubric, { ...ask, input: source, history: this.threadHistory }), }, (text) => { @@ -158,7 +163,12 @@ export class ReviewTutorSession { this.hub.emit("answer_delta", { id, text }); }); if (this.questions.get(id)?.state === "cancelled") return; - await this.answerQuestion(view, ask, source, result); + await this.answerQuestion( + view, + { ...ask, modelId: `${resolved.connector.id}:${resolved.model}` }, + source, + result, + ); } private async answerQuestion(view: QuestionView, ask: AskRequest, source: InputSnapshot, result: { answer: string; usage?: Record }): Promise { @@ -208,6 +218,7 @@ export class ReviewTutorSession { state(): SessionReply { return { status: 200, value: { protocol: "rt/1", models: this.options.models, + harnesses: this.options.harnesses, input: this.currentInput, questions: [...this.questions.values()], lastHeartbeat: this.lastHeartbeat } }; } @@ -222,7 +233,11 @@ export class ReviewTutorSession { const ask = validateAskRequest(await readBody(request)); if (!this.inputs.has(ask.inputId)) return { status: 409, value: { error: "ask input failed: expected an input loaded during this server session; reload the source and retry" } }; - const model = this.options.models.find((candidate) => candidate.id === ask.modelId); + const resolved = this.options.registry.resolve(ask.modelId); + if (!resolved) return { status: 400, value: { + error: "model selection failed: unknown harness; refresh state and retry" } }; + const canonicalId = `${resolved.connector.id}:${resolved.model}`; + const model = this.options.models.find((candidate) => candidate.id === canonicalId); if (!model || !model.thinkingLevels.includes(ask.thinkingLevel)) return { status: 400, value: { error: "model selection failed: expected an available model and thinking level; refresh state and retry" } }; if (this.queue.length >= LIMITS.queue) return { status: 429, value: { @@ -319,7 +334,9 @@ export class ReviewTutorSession { return { status: 200, value: entry }; } - async export(): Promise { return exportLearningHtml(await foldLog(this.paths)); } + async export(): Promise { + return exportLearningHtml(await foldLog(this.paths), this.options.registry); + } async heartbeat(request: IncomingMessage): Promise { validateEmptyObject(await readBody(request), "heartbeat body"); diff --git a/packages/review-tutor/src/server.ts b/packages/review-tutor/src/server.ts index e6775b8..c64d525 100644 --- a/packages/review-tutor/src/server.ts +++ b/packages/review-tutor/src/server.ts @@ -2,11 +2,12 @@ import { randomBytes, timingSafeEqual } from "node:crypto"; import { once } from "node:events"; import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; import type { AddressInfo } from "node:net"; +import type { ConnectorRegistry } from "./connectors/registry.ts"; import { loadInput, type ExecFile } from "./inputs.ts"; import { initProjectPaths } from "./log.ts"; import { bootstrapHtml, pageHtml, staleSessionHtml } from "./page.ts"; import { loadTutorRubric } from "./prompt.ts"; -import { validateSourceRequest, type ModelChoice } from "./protocol.ts"; +import { validateSourceRequest } from "./protocol.ts"; import { resolveStatePaths } from "./paths.ts"; import { TutorRunner } from "./runner.ts"; import { ReviewTutorSession, type RunnerLike, type SessionReply } from "./server-session.ts"; @@ -15,7 +16,7 @@ import { SseHub } from "./sse.ts"; export interface ServerOptions { cwd: string; canonicalRepo: string; - models: ModelChoice[]; + registry: ConnectorRegistry; execFile: ExecFile; skillPath: string; runner?: RunnerLike; @@ -155,8 +156,20 @@ export async function startReviewTutorServer(options: ServerOptions): Promise ({ + connector, + discovery: await options.registry.discover(connector), + }))); + const models = discoveries.flatMap(({ discovery }) => discovery.available ? discovery.models : []); + const harnesses = discoveries.map(({ connector, discovery }) => ({ + id: connector.id, + label: connector.label, + available: discovery.available, + ...(!discovery.available ? { reason: discovery.reason } : {}), + })); const session = new ReviewTutorSession( - options, paths, await loadTutorRubric(options.skillPath), options.runner ?? new TutorRunner(), new SseHub(), + { ...options, models, harnesses }, paths, await loadTutorRubric(options.skillPath), + options.runner ?? new TutorRunner(), new SseHub(), ); let closing: Promise | undefined; const server = createServer(async (request, response) => { diff --git a/packages/review-tutor/test/connectors.test.ts b/packages/review-tutor/test/connectors.test.ts new file mode 100644 index 0000000..6c5ecf8 --- /dev/null +++ b/packages/review-tutor/test/connectors.test.ts @@ -0,0 +1,155 @@ +import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; +import { describe, expect, it, vi } from "vitest"; +import { PiConnector } from "../src/connectors/pi.ts"; +import { redact } from "../src/connectors/redact.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import { + ConnectorError, + type HarnessConnector, + type ParseSink, +} from "../src/connectors/types.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; +import { TutorRunner } from "../src/runner.ts"; + +class FakeChild extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + stdin = new PassThrough(); + pid = 123; + kill = vi.fn(); +} + +const models = [{ id: "anthropic/model", label: "Model", thinkingLevels: ["low"] }]; + +function flags(enabled: boolean) { + return createReviewTutorFlags({ + get: () => enabled, + set: () => {}, + }); +} + +function registry(enabled = false) { + return createConnectorRegistry({ flags: flags(enabled), piModels: models }); +} + +const request = { + model: "anthropic/model", + thinking: "low", + cwd: "/repo", + prompt: "prompt", +}; + +describe("connector registry", () => { + it("reads the environment override once when flags are created", () => { + const previous = process.env.REVIEW_TUTOR_FLAGS; + try { + process.env.REVIEW_TUTOR_FLAGS = "reviewTutorHarnessConnectors"; + const snapshot = createReviewTutorFlags(); + process.env.REVIEW_TUTOR_FLAGS = ""; + expect(snapshot.isEnabled("reviewTutorHarnessConnectors")).toBe(true); + } finally { + if (previous === undefined) delete process.env.REVIEW_TUTOR_FLAGS; + else process.env.REVIEW_TUTOR_FLAGS = previous; + } + }); + + it("keeps only the explicitly registered Pi connector with the flag off or on", () => { + expect(registry(false).connectors().map((connector) => connector.id)).toEqual(["pi"]); + expect(registry(true).connectors().map((connector) => connector.id)).toEqual(["pi"]); + }); + + it("resolves namespaced and legacy Pi ids and rejects unknown harnesses", () => { + expect(registry().resolve("pi:anthropic/model")).toMatchObject({ model: "anthropic/model" }); + expect(registry().resolve("anthropic/model")).toMatchObject({ model: "anthropic/model" }); + expect(registry().resolve("codex:x")).toBeUndefined(); + }); + + it("namespaces Pi discovery without spawning a process", async () => { + const connector = new PiConnector(); + await expect(connector.discover({ piModels: models, piVersion: "1.2.3" })).resolves.toEqual({ + available: true, + version: "1.2.3", + models: [{ ...models[0], id: "pi:anthropic/model" }], + }); + }); +}); + +describe("connector failure boundary", () => { + it.each([ + ["sk-abcdefgh", "[redacted]"], + ["Bearer abc.def", "[redacted]"], + ["OPENAI_API_KEY=value", "[redacted]"], + ["token=value", "[redacted]"], + ["ghp_abcdefgh", "[redacted]"], + ["gho_abcdefgh", "[redacted]"], + ])("redacts %s", (input, expected) => { + expect(redact(input)).toBe(expected); + }); + + it("redacts a child stderr tail before the failure leaves the runner", async () => { + const child = new FakeChild(); + const runner = new TutorRunner({ + connector: new PiConnector(), + spawn: () => child as never, + }); + const done = runner.run(request, () => {}); + child.stderr.end("sk-livefakefakefake"); + child.emit("close", 1, null); + await expect(done).rejects.toThrow(/\[redacted\]/); + await expect(done).rejects.not.toThrow(/sk-livefakefakefake/); + }); + + it("preserves a typed connector failure for an empty answer", async () => { + const child = new FakeChild(); + const connector: HarnessConnector = { + id: "pi", + label: "Pi", + discover: async () => ({ available: true, version: "unknown", models: [] }), + spawnSpec: () => ({ command: "fake", args: [] }), + parseLine: () => {}, + finish: (_sink: ParseSink) => { + throw new ConnectorError("empty_answer", "empty answer"); + }, + }; + const runner = new TutorRunner({ connector, spawn: () => child as never }); + const done = runner.run(request, () => {}); + child.emit("close", 0, null); + const error = await done.catch((value: unknown) => value); + expect(error).toBeInstanceOf(ConnectorError); + expect(error).toMatchObject({ code: "empty_answer", message: "empty answer" }); + }); + + it("settles once when cancelled before, during, or after execution", async () => { + const beforeChild = new FakeChild(); + const beforeRunner = new TutorRunner({ connector: new PiConnector(), spawn: () => beforeChild as never }); + beforeRunner.cancel(); + const before = beforeRunner.run(request, () => {}); + beforeChild.stdout.end(`${JSON.stringify({ + type: "agent_end", + messages: [{ role: "assistant", content: "started" }], + })}\n`); + beforeChild.emit("close", 0, null); + await expect(before).resolves.toMatchObject({ answer: "started" }); + + const child = new FakeChild(); + const terminate = vi.fn(); + const runner = new TutorRunner({ connector: new PiConnector(), spawn: () => child as never, terminate }); + const cancelled = runner.run(request, () => {}); + runner.cancel(); + child.emit("close", null, "SIGTERM"); + await expect(cancelled).rejects.toThrow(/cancelled/); + expect(terminate).toHaveBeenCalledTimes(1); + + const completedChild = new FakeChild(); + const completedRunner = new TutorRunner({ connector: new PiConnector(), spawn: () => completedChild as never }); + const completed = completedRunner.run(request, () => {}); + completedChild.stdout.end(`${JSON.stringify({ + type: "agent_end", + messages: [{ role: "assistant", content: "done" }], + })}\n`); + completedChild.emit("close", 0, null); + await expect(completed).resolves.toMatchObject({ answer: "done" }); + completedRunner.cancel(); + }); +}); diff --git a/packages/review-tutor/test/core.test.ts b/packages/review-tutor/test/core.test.ts index f1bee0f..c5b65a7 100644 --- a/packages/review-tutor/test/core.test.ts +++ b/packages/review-tutor/test/core.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { loadInput } from "../src/inputs.ts"; -import { parsePiJson } from "../src/pi-json.ts"; +import { parsePiJson } from "../src/connectors/pi.ts"; import { buildTutorPrompt, loadTutorRubric } from "../src/prompt.ts"; import { validateAskRequest, diff --git a/packages/review-tutor/test/runner.test.ts b/packages/review-tutor/test/runner.test.ts index 5eb5ac8..c72a00e 100644 --- a/packages/review-tutor/test/runner.test.ts +++ b/packages/review-tutor/test/runner.test.ts @@ -1,6 +1,7 @@ import { EventEmitter } from "node:events"; import { PassThrough } from "node:stream"; import { describe, expect, it, vi } from "vitest"; +import { PiConnector } from "../src/connectors/pi.ts"; import { TutorRunner, createChildEnvironment, @@ -16,8 +17,8 @@ class FakeChild extends EventEmitter { } const request = { - provider: "p", - model: "m", + connector: new PiConnector(), + model: "p/m", thinking: "low", cwd: "/repo", prompt: "secret prompt", diff --git a/packages/review-tutor/test/server-extension.test.ts b/packages/review-tutor/test/server-extension.test.ts index e1c7676..b435cae 100644 --- a/packages/review-tutor/test/server-extension.test.ts +++ b/packages/review-tutor/test/server-extension.test.ts @@ -9,6 +9,9 @@ import { createServerLifecycle, modelChoices, } from "../extensions/review-tutor.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import type { HarnessConnector } from "../src/connectors/types.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; import { pageHtml } from "../src/page.ts"; import { resolveStatePaths } from "../src/paths.ts"; import type { AskRequest } from "../src/protocol.ts"; @@ -19,6 +22,10 @@ const skillPath = fileURLToPath( ); const temporaryRoots: string[] = []; +function registry(models = [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }]) { + return createConnectorRegistry({ flags: createReviewTutorFlags(), piModels: models }); +} + interface DeferredResult { promise: Promise<{ answer: string }>; resolve: (value: { answer: string }) => void; @@ -33,14 +40,14 @@ function deferredResult(): DeferredResult { } class ControlledRunner { - readonly calls: Array<{ prompt: string; deferred: DeferredResult }> = []; + readonly calls: Array<{ connector: HarnessConnector; model: string; prompt: string; deferred: DeferredResult }> = []; cancelCalls = 0; shutdownCalls = 0; completedCalls = 0; - async run(request: { prompt: string }, delta: (text: string) => void) { + async run(request: { connector: HarnessConnector; model: string; prompt: string }, delta: (text: string) => void) { const deferred = deferredResult(); - this.calls.push({ prompt: request.prompt, deferred }); + this.calls.push({ connector: request.connector, model: request.model, prompt: request.prompt, deferred }); delta("live"); try { return await deferred.promise; @@ -81,7 +88,7 @@ async function start( const server = await startReviewTutorServer({ cwd: "/repo", canonicalRepo: "/repo", - models: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + registry: registry(), skillPath, home, runner, @@ -309,6 +316,45 @@ describe("local server security", () => { ); }); +describe("connector protocol boundary", () => { + it("reports namespaced state, resolves legacy Pi asks, rejects unknown harnesses, and exports harness details", async () => { + const { server, runner } = await start(); + try { + const state = await (await call(server.port, server.token, "/api/state")).json() as { + models: Array<{ id: string }>; + harnesses: Array<{ id: string; label: string; available: boolean }>; + }; + expect(state.models.map((model) => model.id)).toEqual(["pi:provider/model"]); + expect(state.harnesses).toEqual([{ id: "pi", label: "Pi", available: true }]); + + const source = await loadSource(server.port, server.token); + expect((await ask(server.port, server.token, source.id)).status).toBe(202); + await waitFor(() => runner.calls.length === 1); + expect(runner.calls[0]).toMatchObject({ + connector: { id: "pi", label: "Pi" }, + model: "provider/model", + }); + runner.calls[0]!.deferred.resolve({ answer: "answer" }); + await waitFor(() => runner.completedCalls === 1); + + const unknown = await call(server.port, server.token, "/api/ask", { + method: "POST", + body: JSON.stringify({ ...askBody(source.id), modelId: "codex:x" }), + }); + expect(unknown.status).toBe(400); + await expect(unknown.json()).resolves.toEqual({ + error: "model selection failed: unknown harness; refresh state and retry", + }); + + const exported = await (await call(server.port, server.token, "/api/export")).text(); + expect(exported).toContain("
Harness
Pi
"); + expect(exported).toContain("
Model
provider/model
"); + } finally { + await server.close(); + } + }); +}); + describe("local server question lifecycle", () => { it("stores and echoes question page ownership", async () => { const { server } = await start(); @@ -454,7 +500,7 @@ describe("local server startup and shutdown", () => { await expect(startReviewTutorServer({ cwd: "/repo", canonicalRepo: "/repo", - models: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + registry: registry(), skillPath, home, runner, diff --git a/packages/review-tutor/test/structure.test.ts b/packages/review-tutor/test/structure.test.ts index 5400095..75ada63 100644 --- a/packages/review-tutor/test/structure.test.ts +++ b/packages/review-tutor/test/structure.test.ts @@ -3,6 +3,8 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; import type { ExecFile } from "../src/inputs.ts"; import type { InputSnapshot, StructureEdge, StructureSnapshot } from "../src/protocol.ts"; import { startReviewTutorServer } from "../src/server.ts"; @@ -12,6 +14,10 @@ import { const skillPath = fileURLToPath(new URL("../skills/review-tutor/SKILL.md", import.meta.url)); const temporaryRoots: string[] = []; +const registry = () => createConnectorRegistry({ + flags: createReviewTutorFlags(), + piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], +}); afterEach(async () => { while (temporaryRoots.length) { @@ -1237,7 +1243,7 @@ async function startServer(diffs: string[]) { const server = await startReviewTutorServer({ cwd: "/repo", canonicalRepo: "/repo", - models: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + registry: registry(), skillPath, home, runner: { run: async () => ({ answer: "" }), cancel: () => {}, shutdown: async () => {} }, @@ -1296,7 +1302,7 @@ describe("structure endpoint", () => { const server = await startReviewTutorServer({ cwd: "/repo", canonicalRepo: "/repo", - models: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], + registry: registry(), skillPath, home, runner: { run: async () => ({ answer: "" }), cancel: () => {}, shutdown: async () => {} }, diff --git a/tsconfig.base.json b/tsconfig.base.json index c9c149c..6df071d 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -13,6 +13,9 @@ ], "module": "ESNext", "moduleResolution": "Bundler", + "paths": { + "@pickforge/flags": ["./packages/flags/src/index.ts"] + }, "noUncheckedIndexedAccess": true, "resolveJsonModule": true, "skipLibCheck": true, diff --git a/vitest.config.ts b/vitest.config.ts index e63691b..57b40c2 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -2,6 +2,11 @@ import { defaultExclude } from "vitest/config"; import { defineConfig } from "vitest/config"; export default defineConfig({ + resolve: { + alias: { + "@pickforge/flags": new URL("./packages/flags/src/index.ts", import.meta.url).pathname, + }, + }, test: { coverage: { exclude: ["packages/**/dist/**", "packages/**/test/**", "**/*.test.ts"], From fe594a2b3d0b398f00f35d627529a56eba33b1b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Tue, 25 Aug 2026 16:57:46 -0300 Subject: [PATCH 2/3] fix(review-tutor): harden connector boundary --- bun.lock | 3 - packages/review-tutor/package.json | 3 - packages/review-tutor/src/connectors/pi.ts | 49 +------- .../review-tutor/src/connectors/registry.ts | 15 ++- packages/review-tutor/src/connectors/types.ts | 9 +- packages/review-tutor/src/export-html.ts | 18 ++- packages/review-tutor/src/flags.ts | 2 +- packages/review-tutor/src/runner-execution.ts | 12 +- packages/review-tutor/src/runner.ts | 30 ++--- packages/review-tutor/src/server-session.ts | 7 +- packages/review-tutor/src/server.ts | 5 +- packages/review-tutor/test/connectors.test.ts | 112 +++++++++++++++--- packages/review-tutor/test/core.test.ts | 68 ++++++----- .../test/server-extension.test.ts | 26 +++- .../test/storage-export-sse.test.ts | 16 ++- tsconfig.base.json | 3 - vitest.config.ts | 5 - 17 files changed, 227 insertions(+), 156 deletions(-) diff --git a/bun.lock b/bun.lock index b432f43..ed9767b 100644 --- a/bun.lock +++ b/bun.lock @@ -46,9 +46,6 @@ "packages/review-tutor": { "name": "@pickforge/review-tutor", "version": "0.0.0", - "dependencies": { - "@pickforge/flags": "workspace:*", - }, "devDependencies": { "@earendil-works/pi-coding-agent": "^0.84.2", }, diff --git a/packages/review-tutor/package.json b/packages/review-tutor/package.json index 9e067b3..91a3d85 100644 --- a/packages/review-tutor/package.json +++ b/packages/review-tutor/package.json @@ -13,9 +13,6 @@ "test": "cd ../.. && vitest run packages/review-tutor/test", "typecheck": "tsc -p ../../tsconfig.json --noEmit" }, - "dependencies": { - "@pickforge/flags": "workspace:*" - }, "peerDependencies": { "@earendil-works/pi-coding-agent": ">=0.83.0" }, diff --git a/packages/review-tutor/src/connectors/pi.ts b/packages/review-tutor/src/connectors/pi.ts index 4356788..80fabd5 100644 --- a/packages/review-tutor/src/connectors/pi.ts +++ b/packages/review-tutor/src/connectors/pi.ts @@ -35,7 +35,6 @@ function parseEvent(line: string): PiEvent { return JSON.parse(line) as PiEvent; } catch { throw new ConnectorError( - "malformed_output", "Pi JSON parsing failed: expected one valid JSON object per LF-delimited line; inspect child output and retry", ); } @@ -56,8 +55,6 @@ function finalAnswer(messages: PiMessage[] | undefined): string | undefined { export class PiConnector implements HarnessConnector { readonly id = "pi" as const; readonly label = "Pi"; - private answer?: string; - private answerUsage?: Record; async discover(deps: DiscoveryDeps): Promise { return { @@ -68,8 +65,6 @@ export class PiConnector implements HarnessConnector { } spawnSpec(request: ConnectorRequest): SpawnSpec { - this.answer = undefined; - this.answerUsage = undefined; const [provider, ...modelParts] = request.model.split("/"); return { command: "pi", @@ -88,54 +83,20 @@ export class PiConnector implements HarnessConnector { sink.delta(update.delta); } if (event.type === "message_end" && event.message?.usage && typeof event.message.usage === "object") { - this.answerUsage = event.message.usage as Record; - sink.usage(this.answerUsage); + sink.usage(event.message.usage as Record); } if (event.type === "agent_end" && Array.isArray(event.messages)) { const candidate = finalAnswer(event.messages); - if (this.answer === undefined || candidate?.trim()) { - this.answer = candidate; - if (candidate !== undefined) sink.final(candidate); - } + if (candidate !== undefined) sink.final(candidate); } } - finish(_sink: ParseSink): ParsedAnswer { - if (!this.answer?.trim()) { + finish(sink: ParseSink): ParsedAnswer { + if (!sink.answer?.trim()) { throw new ConnectorError( - "empty_answer", "Pi answer failed: expected a non-empty final assistant message in agent_end; choose another question or model and retry", ); } - return { answer: this.answer, ...(this.answerUsage ? { usage: this.answerUsage } : {}) }; + return { answer: sink.answer, ...(sink.answerUsage ? { usage: sink.answerUsage } : {}) }; } } - -export function parsePiJson() { - const connector = new PiConnector(); - let pending = ""; - const sink = { - events: [] as Array<{ type: "delta"; text: string }>, - delta(text: string) { this.events.push({ type: "delta", text }); }, - usage() {}, - final() {}, - }; - return { - push(chunk: string) { - pending += chunk; - sink.events = []; - for (;;) { - const newline = pending.indexOf("\n"); - if (newline < 0) break; - const line = pending.slice(0, newline); - pending = pending.slice(newline + 1); - if (line) connector.parseLine(line, sink); - } - return sink.events; - }, - finish() { - if (pending.trim()) connector.parseLine(pending, sink); - return connector.finish(sink); - }, - }; -} diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts index 6e535ae..5a77127 100644 --- a/packages/review-tutor/src/connectors/registry.ts +++ b/packages/review-tutor/src/connectors/registry.ts @@ -12,7 +12,7 @@ export interface ConnectorRegistry { connectors(): HarnessConnector[]; byId(id: string): HarnessConnector | undefined; resolve(modelId: string): { connector: HarnessConnector; model: string } | undefined; - discover(connector: HarnessConnector): Promise; + discoveries(): Promise>; } export function createConnectorRegistry(options: { @@ -39,13 +39,18 @@ export function createConnectorRegistry(options: { }, resolve(modelId) { const separator = modelId.indexOf(":"); - const harness = separator < 0 ? "pi" : modelId.slice(0, separator); + const slash = modelId.indexOf("/"); + const namespaced = separator >= 0 && (slash < 0 || separator < slash); + const harness = namespaced ? modelId.slice(0, separator) : "pi"; const connector = this.byId(harness as HarnessId); if (!connector) return undefined; - return { connector, model: separator < 0 ? modelId : modelId.slice(separator + 1) }; + return { connector, model: namespaced ? modelId.slice(separator + 1) : modelId }; }, - discover(connector) { - return connector.discover(dependencies); + async discoveries() { + return Promise.all(connectors().map(async (connector) => ({ + connector, + discovery: await connector.discover(dependencies), + }))); }, }; } diff --git a/packages/review-tutor/src/connectors/types.ts b/packages/review-tutor/src/connectors/types.ts index 9807f2d..8728495 100644 --- a/packages/review-tutor/src/connectors/types.ts +++ b/packages/review-tutor/src/connectors/types.ts @@ -37,6 +37,8 @@ export interface SpawnSpec { } export interface ParseSink { + readonly answer?: string; + readonly answerUsage?: Record; delta(text: string): void; usage(u: Record): void; final(answer: string): void; @@ -47,9 +49,4 @@ export interface ParsedAnswer { usage?: Record; } -export class ConnectorError extends Error { - constructor(readonly code: string, message: string) { - super(message); - this.name = "ConnectorError"; - } -} +export class ConnectorError extends Error {} diff --git a/packages/review-tutor/src/export-html.ts b/packages/review-tutor/src/export-html.ts index d3b84b2..2585f5a 100644 --- a/packages/review-tutor/src/export-html.ts +++ b/packages/review-tutor/src/export-html.ts @@ -17,17 +17,15 @@ function escapeHtml(value: unknown): string { })[character]!); } -function modelDetails(entry: LearningEntry, registry?: ConnectorRegistry): { harness: string; model: string } { - const separator = entry.modelId.indexOf(":"); - if (separator < 0) return { harness: "Pi", model: entry.modelId }; - const harnessId = entry.modelId.slice(0, separator); - return { - harness: registry?.byId(harnessId)?.label ?? harnessId, - model: entry.modelId.slice(separator + 1), - }; +function modelDetails(entry: LearningEntry, registry: ConnectorRegistry): { harness: string; model: string } { + if (typeof entry.modelId !== "string") return { harness: "Pi", model: String(entry.modelId) }; + const resolved = registry.resolve(entry.modelId); + return resolved + ? { harness: resolved.connector.label, model: resolved.model } + : { harness: "Pi", model: entry.modelId }; } -function card(entry: LearningEntry, registry?: ConnectorRegistry): string { +function card(entry: LearningEntry, registry: ConnectorRegistry): string { const sourceUrl = entry.source.githubUrl ? `
GitHub source URL
${escapeHtml(entry.source.githubUrl)}
` : ""; @@ -70,7 +68,7 @@ ${entry.source.kind === "pr" ? "

GitHub is the source of truth.

" : ""} `; } -export function exportLearningHtml(entries: LearningEntry[], registry?: ConnectorRegistry): string { +export function exportLearningHtml(entries: LearningEntry[], registry: ConnectorRegistry): string { const cards = entries.length ? entries.map((entry) => card(entry, registry)).join("") : "

No learning entries yet.

"; return ` diff --git a/packages/review-tutor/src/flags.ts b/packages/review-tutor/src/flags.ts index 9890193..76372b2 100644 --- a/packages/review-tutor/src/flags.ts +++ b/packages/review-tutor/src/flags.ts @@ -2,7 +2,7 @@ import { createFlags, type FlagOverrideStore, type Flags, -} from "@pickforge/flags"; +} from "../../flags/src/index.ts"; const definitions = { reviewTutorHarnessConnectors: { diff --git a/packages/review-tutor/src/runner-execution.ts b/packages/review-tutor/src/runner-execution.ts index 97c4c3b..68a8fe8 100644 --- a/packages/review-tutor/src/runner-execution.ts +++ b/packages/review-tutor/src/runner-execution.ts @@ -36,7 +36,17 @@ export class RunnerExecution { onDelta: (text: string) => void, private readonly onSettled: () => void, ) { - this.sink = { delta: onDelta, usage: () => {}, final: () => {} }; + let answer: string | undefined; + let answerUsage: Record | undefined; + this.sink = { + get answer() { return answer; }, + get answerUsage() { return answerUsage; }, + delta: onDelta, + usage: (next) => { answerUsage = next; }, + final: (next) => { + if (answer === undefined || next.trim()) answer = next; + }, + }; this.completion = new Promise((resolve) => { this.complete = resolve; }); this.result = new Promise((resolve, reject) => { this.resolve = resolve; diff --git a/packages/review-tutor/src/runner.ts b/packages/review-tutor/src/runner.ts index a2f1a46..84fae95 100644 --- a/packages/review-tutor/src/runner.ts +++ b/packages/review-tutor/src/runner.ts @@ -4,7 +4,7 @@ import { type SpawnOptionsWithoutStdio, } from "node:child_process"; import { redact } from "./connectors/redact.ts"; -import { ConnectorError, type HarnessConnector, type ParsedAnswer } from "./connectors/types.ts"; +import type { HarnessConnector, ParsedAnswer } from "./connectors/types.ts"; import { LIMITS } from "./protocol.ts"; import { RunnerExecution } from "./runner-execution.ts"; @@ -53,7 +53,6 @@ export function platformTerminate( } interface RunnerOptions { - connector?: HarnessConnector; spawn?: Spawn; env?: NodeJS.ProcessEnv; terminate?: Terminate; @@ -66,7 +65,7 @@ interface RunnerOptions { } export interface RunRequest { - connector?: HarnessConnector; + connector: HarnessConnector; model: string; thinking: string; cwd: string; @@ -74,24 +73,19 @@ export interface RunRequest { } function safeError(error: unknown): Error { - const source = error instanceof Error ? error : new Error(String(error)); - const message = redact(source.message); - return source instanceof ConnectorError - ? new ConnectorError(source.code, message) - : new Error(message); + const message = error instanceof Error ? error.message : String(error); + return new Error(redact(message)); } export class TutorRunner { private child?: ChildProcessWithoutNullStreams; private execution?: RunnerExecution; - private readonly defaultConnector?: HarnessConnector; - private readonly options: Required> & { + private readonly options: Required> & { terminate: Terminate; }; constructor(options: RunnerOptions = {}) { const platform = options.platform ?? process.platform; - this.defaultConnector = options.connector; this.options = { spawn: options.spawn ?? (nodeSpawn as Spawn), env: options.env ?? process.env, @@ -111,16 +105,14 @@ export class TutorRunner { "question runner failed: expected no running child, but one is active; wait or cancel it before retrying", ); } - const connector = request.connector ?? this.defaultConnector; - if (!connector) throw new Error("question runner failed: expected a harness connector; choose a model and retry"); let child: ChildProcessWithoutNullStreams; try { - child = this.spawn(request, connector); + child = this.spawn(request); } catch (error) { throw safeError(error); } this.child = child; - const execution = new RunnerExecution(child, connector, this.options, onDelta, () => { + const execution = new RunnerExecution(child, request.connector, this.options, onDelta, () => { if (this.child === child) this.child = undefined; if (this.execution === execution) this.execution = undefined; }); @@ -136,18 +128,18 @@ export class TutorRunner { } } - private spawn(request: RunRequest, connector: HarnessConnector): ChildProcessWithoutNullStreams { - const spec = connector.spawnSpec(request); + private spawn(request: RunRequest): ChildProcessWithoutNullStreams { + const spec = request.connector.spawnSpec(request); try { return this.options.spawn(spec.command, spec.args, { cwd: request.cwd, detached: this.options.platform !== "win32", - env: createChildEnvironment(this.options.env, connector.envKeys), + env: createChildEnvironment(this.options.env, request.connector.envKeys), stdio: ["pipe", "pipe", "pipe"], }); } catch (error) { throw new Error( - `question child spawn failed: ${error instanceof Error ? error.message : String(error)}; verify ${connector.label} is installed and retry`, + `question child spawn failed: ${error instanceof Error ? error.message : String(error)}; verify ${request.connector.label} is installed and retry`, ); } } diff --git a/packages/review-tutor/src/server-session.ts b/packages/review-tutor/src/server-session.ts index 858c904..87e74b5 100644 --- a/packages/review-tutor/src/server-session.ts +++ b/packages/review-tutor/src/server-session.ts @@ -163,12 +163,7 @@ export class ReviewTutorSession { this.hub.emit("answer_delta", { id, text }); }); if (this.questions.get(id)?.state === "cancelled") return; - await this.answerQuestion( - view, - { ...ask, modelId: `${resolved.connector.id}:${resolved.model}` }, - source, - result, - ); + await this.answerQuestion(view, ask, source, result); } private async answerQuestion(view: QuestionView, ask: AskRequest, source: InputSnapshot, result: { answer: string; usage?: Record }): Promise { diff --git a/packages/review-tutor/src/server.ts b/packages/review-tutor/src/server.ts index c64d525..a5f0413 100644 --- a/packages/review-tutor/src/server.ts +++ b/packages/review-tutor/src/server.ts @@ -156,10 +156,7 @@ export async function startReviewTutorServer(options: ServerOptions): Promise ({ - connector, - discovery: await options.registry.discover(connector), - }))); + const discoveries = await options.registry.discoveries(); const models = discoveries.flatMap(({ discovery }) => discovery.available ? discovery.models : []); const harnesses = discoveries.map(({ connector, discovery }) => ({ id: connector.id, diff --git a/packages/review-tutor/test/connectors.test.ts b/packages/review-tutor/test/connectors.test.ts index 6c5ecf8..74370cd 100644 --- a/packages/review-tutor/test/connectors.test.ts +++ b/packages/review-tutor/test/connectors.test.ts @@ -34,6 +34,7 @@ function registry(enabled = false) { } const request = { + connector: new PiConnector(), model: "anthropic/model", thinking: "low", cwd: "/repo", @@ -62,6 +63,14 @@ describe("connector registry", () => { it("resolves namespaced and legacy Pi ids and rejects unknown harnesses", () => { expect(registry().resolve("pi:anthropic/model")).toMatchObject({ model: "anthropic/model" }); expect(registry().resolve("anthropic/model")).toMatchObject({ model: "anthropic/model" }); + expect(registry().resolve("ollama/qwen3:8b")).toMatchObject({ + connector: { id: "pi" }, + model: "ollama/qwen3:8b", + }); + expect(registry().resolve("pi:ollama/qwen3:8b")).toMatchObject({ + connector: { id: "pi" }, + model: "ollama/qwen3:8b", + }); expect(registry().resolve("codex:x")).toBeUndefined(); }); @@ -89,10 +98,7 @@ describe("connector failure boundary", () => { it("redacts a child stderr tail before the failure leaves the runner", async () => { const child = new FakeChild(); - const runner = new TutorRunner({ - connector: new PiConnector(), - spawn: () => child as never, - }); + const runner = new TutorRunner({ spawn: () => child as never }); const done = runner.run(request, () => {}); child.stderr.end("sk-livefakefakefake"); child.emit("close", 1, null); @@ -100,7 +106,7 @@ describe("connector failure boundary", () => { await expect(done).rejects.not.toThrow(/sk-livefakefakefake/); }); - it("preserves a typed connector failure for an empty answer", async () => { + it("reports a connector failure for an empty answer", async () => { const child = new FakeChild(); const connector: HarnessConnector = { id: "pi", @@ -109,20 +115,92 @@ describe("connector failure boundary", () => { spawnSpec: () => ({ command: "fake", args: [] }), parseLine: () => {}, finish: (_sink: ParseSink) => { - throw new ConnectorError("empty_answer", "empty answer"); + throw new ConnectorError("empty answer"); }, }; - const runner = new TutorRunner({ connector, spawn: () => child as never }); - const done = runner.run(request, () => {}); + const runner = new TutorRunner({ spawn: () => child as never }); + const done = runner.run({ ...request, connector }, () => {}); child.emit("close", 0, null); const error = await done.catch((value: unknown) => value); - expect(error).toBeInstanceOf(ConnectorError); - expect(error).toMatchObject({ code: "empty_answer", message: "empty answer" }); + expect(error).toMatchObject({ message: "empty answer" }); + }); + + it("terminates once and redacts a parse failure on the second line", async () => { + const child = new FakeChild(); + const terminate = vi.fn(); + let lines = 0; + const connector: HarnessConnector = { + id: "pi", + label: "Pi", + discover: async () => ({ available: true, version: "unknown", models: [] }), + spawnSpec: () => ({ command: "fake", args: [] }), + parseLine: () => { + lines += 1; + if (lines === 2) throw new Error("parse failed with sk-abcdefgh"); + }, + finish: () => ({ answer: "unused" }), + }; + const runner = new TutorRunner({ spawn: () => child as never, terminate }); + const done = runner.run({ ...request, connector }, () => {}); + let rejections = 0; + void done.catch(() => { rejections += 1; }); + + child.stdout.write("first\nsecond\n"); + child.emit("close", null, "SIGTERM"); + + await expect(done).rejects.toThrow(/parse failed with \[redacted\]/); + await expect(done).rejects.not.toThrow(/sk-abcdefgh/); + expect(terminate).toHaveBeenCalledTimes(1); + expect(rejections).toBe(1); + }); + + it("keeps answer state per execution on a shared connector", async () => { + const connector = new PiConnector(); + const firstChild = new FakeChild(); + const secondChild = new FakeChild(); + const children = [firstChild, secondChild]; + const runner = new TutorRunner({ spawn: () => children.shift() as never }); + const sharedRequest = { ...request, connector }; + + const first = runner.run(sharedRequest, () => {}); + firstChild.stdout.end(`${JSON.stringify({ + type: "agent_end", + messages: [{ role: "assistant", content: "first" }], + })}\n`); + firstChild.emit("close", 0, null); + await expect(first).resolves.toEqual({ answer: "first" }); + + const second = runner.run(sharedRequest, () => {}); + secondChild.emit("close", 0, null); + await expect(second).rejects.toThrow(/empty/); + }); + + it("returns usage reported through the runner-owned sink", async () => { + const child = new FakeChild(); + const connector: HarnessConnector = { + id: "pi", + label: "Pi", + discover: async () => ({ available: true, version: "unknown", models: [] }), + spawnSpec: () => ({ command: "fake", args: [] }), + parseLine: (_line, sink) => { + sink.usage({ input: 3 }); + sink.final("answer"); + }, + finish: (sink) => ({ + answer: sink.answer!, + ...(sink.answerUsage ? { usage: sink.answerUsage } : {}), + }), + }; + const runner = new TutorRunner({ spawn: () => child as never }); + const done = runner.run({ ...request, connector }, () => {}); + child.stdout.end("event\n"); + child.emit("close", 0, null); + await expect(done).resolves.toEqual({ answer: "answer", usage: { input: 3 } }); }); it("settles once when cancelled before, during, or after execution", async () => { const beforeChild = new FakeChild(); - const beforeRunner = new TutorRunner({ connector: new PiConnector(), spawn: () => beforeChild as never }); + const beforeRunner = new TutorRunner({ spawn: () => beforeChild as never }); beforeRunner.cancel(); const before = beforeRunner.run(request, () => {}); beforeChild.stdout.end(`${JSON.stringify({ @@ -134,7 +212,7 @@ describe("connector failure boundary", () => { const child = new FakeChild(); const terminate = vi.fn(); - const runner = new TutorRunner({ connector: new PiConnector(), spawn: () => child as never, terminate }); + const runner = new TutorRunner({ spawn: () => child as never, terminate }); const cancelled = runner.run(request, () => {}); runner.cancel(); child.emit("close", null, "SIGTERM"); @@ -142,14 +220,20 @@ describe("connector failure boundary", () => { expect(terminate).toHaveBeenCalledTimes(1); const completedChild = new FakeChild(); - const completedRunner = new TutorRunner({ connector: new PiConnector(), spawn: () => completedChild as never }); + const completedTerminate = vi.fn(); + const completedRunner = new TutorRunner({ + spawn: () => completedChild as never, + terminate: completedTerminate, + }); const completed = completedRunner.run(request, () => {}); completedChild.stdout.end(`${JSON.stringify({ type: "agent_end", messages: [{ role: "assistant", content: "done" }], })}\n`); completedChild.emit("close", 0, null); - await expect(completed).resolves.toMatchObject({ answer: "done" }); + const completedResult = await completed; completedRunner.cancel(); + expect(completedResult).toEqual({ answer: "done" }); + expect(completedTerminate).not.toHaveBeenCalled(); }); }); diff --git a/packages/review-tutor/test/core.test.ts b/packages/review-tutor/test/core.test.ts index c5b65a7..24494e7 100644 --- a/packages/review-tutor/test/core.test.ts +++ b/packages/review-tutor/test/core.test.ts @@ -3,7 +3,8 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; import { loadInput } from "../src/inputs.ts"; -import { parsePiJson } from "../src/connectors/pi.ts"; +import { PiConnector } from "../src/connectors/pi.ts"; +import type { ParseSink } from "../src/connectors/types.ts"; import { buildTutorPrompt, loadTutorRubric } from "../src/prompt.ts"; import { validateAskRequest, @@ -343,39 +344,50 @@ describe("prompt and Pi JSON", () => { expect(data.history).toEqual([]); }); - it("parses split NDJSON, deltas, usage, final assistant, and failures", () => { - const parser = parsePiJson(); - expect(parser.push( - '{"type":"message_update","assistantMessageEvent":{"type":"text_delta","delta":"he', - )).toEqual([]); - expect(parser.push('llo"}}\n')).toEqual([{ type: "delta", text: "hello" }]); - parser.push('{"type":"message_end","message":{"usage":{"input":1,"output":2}}}\n'); - parser.push( - '{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"final"}]}]}\n', + it("parses Pi deltas, usage, final assistant, and failures", () => { + const connector = new PiConnector(); + let answer: string | undefined; + let answerUsage: Record | undefined; + const deltas: string[] = []; + const sink: ParseSink = { + get answer() { return answer; }, + get answerUsage() { return answerUsage; }, + delta: (text) => deltas.push(text), + usage: (usage) => { answerUsage = usage; }, + final: (next) => { + if (answer === undefined || next.trim()) answer = next; + }, + }; + + connector.parseLine( + '{"type":"message_update","assistantMessageEvent":{"type":"text_delta","delta":"hello"}}', + sink, + ); + connector.parseLine('{"type":"message_end","message":{"usage":{"input":1,"output":2}}}', sink); + connector.parseLine( + '{"type":"agent_end","messages":[{"role":"assistant","content":[{"type":"text","text":"final"}]}]}', + sink, ); - expect(parser.finish()).toMatchObject({ + expect(deltas).toEqual(["hello"]); + expect(connector.finish(sink)).toEqual({ answer: "final", usage: { input: 1, output: 2 }, }); - const stringFinal = parsePiJson(); - stringFinal.push( - '{"type":"agent_end","messages":[{"role":"assistant","content":"text"}]}\n', + connector.parseLine( + '{"type":"agent_end","messages":[{"role":"assistant","content":"text"}]}', + sink, ); - expect(stringFinal.finish().answer).toBe("text"); - const repeatedFinal = parsePiJson(); - repeatedFinal.push( - '{"type":"agent_end","messages":[{"role":"assistant","content":"accepted"}]}\n', - ); - repeatedFinal.push('{"type":"agent_end","messages":"invalid"}\n'); - repeatedFinal.push('{"type":"agent_end","messages":[]}\n'); - expect(repeatedFinal.finish().answer).toBe("accepted"); - const malformed = parsePiJson(); - expect(() => malformed.push("nope\n")).toThrow(/Pi JSON/); - const empty = parsePiJson(); - empty.push( - '{"type":"agent_end","messages":[{"role":"assistant","content":[]}]}\n', + connector.parseLine('{"type":"agent_end","messages":"invalid"}', sink); + connector.parseLine('{"type":"agent_end","messages":[]}', sink); + expect(connector.finish(sink).answer).toBe("text"); + expect(() => connector.parseLine("nope", sink)).toThrow(/Pi JSON/); + + answer = undefined; + connector.parseLine( + '{"type":"agent_end","messages":[{"role":"assistant","content":[]}]}', + sink, ); - expect(() => empty.finish()).toThrow(/empty/); + expect(() => connector.finish(sink)).toThrow(/empty/); }); }); diff --git a/packages/review-tutor/test/server-extension.test.ts b/packages/review-tutor/test/server-extension.test.ts index b435cae..048050d 100644 --- a/packages/review-tutor/test/server-extension.test.ts +++ b/packages/review-tutor/test/server-extension.test.ts @@ -22,7 +22,10 @@ const skillPath = fileURLToPath( ); const temporaryRoots: string[] = []; -function registry(models = [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }]) { +function registry(models = [ + { id: "provider/model", label: "Model", thinkingLevels: ["low"] }, + { id: "ollama/qwen3:8b", label: "Qwen", thinkingLevels: ["low"] }, +]) { return createConnectorRegistry({ flags: createReviewTutorFlags(), piModels: models }); } @@ -324,7 +327,10 @@ describe("connector protocol boundary", () => { models: Array<{ id: string }>; harnesses: Array<{ id: string; label: string; available: boolean }>; }; - expect(state.models.map((model) => model.id)).toEqual(["pi:provider/model"]); + expect(state.models.map((model) => model.id)).toEqual([ + "pi:provider/model", + "pi:ollama/qwen3:8b", + ]); expect(state.harnesses).toEqual([{ id: "pi", label: "Pi", available: true }]); const source = await loadSource(server.port, server.token); @@ -337,6 +343,16 @@ describe("connector protocol boundary", () => { runner.calls[0]!.deferred.resolve({ answer: "answer" }); await waitFor(() => runner.completedCalls === 1); + const namespaced = await call(server.port, server.token, "/api/ask", { + method: "POST", + body: JSON.stringify({ ...askBody(source.id), modelId: "pi:ollama/qwen3:8b" }), + }); + expect(namespaced.status).toBe(202); + await waitFor(() => runner.calls.length === 2); + expect(runner.calls[1]).toMatchObject({ model: "ollama/qwen3:8b" }); + runner.calls[1]!.deferred.resolve({ answer: "colon answer" }); + await waitFor(() => runner.completedCalls === 2); + const unknown = await call(server.port, server.token, "/api/ask", { method: "POST", body: JSON.stringify({ ...askBody(source.id), modelId: "codex:x" }), @@ -349,6 +365,12 @@ describe("connector protocol boundary", () => { const exported = await (await call(server.port, server.token, "/api/export")).text(); expect(exported).toContain("
Harness
Pi
"); expect(exported).toContain("
Model
provider/model
"); + expect(exported).toContain("
Model
ollama/qwen3:8b
"); + const log = await (await call(server.port, server.token, "/api/log")).json() as Array<{ modelId: string }>; + expect(log.map((entry) => entry.modelId)).toEqual([ + "provider/model", + "pi:ollama/qwen3:8b", + ]); } finally { await server.close(); } diff --git a/packages/review-tutor/test/storage-export-sse.test.ts b/packages/review-tutor/test/storage-export-sse.test.ts index 3b50245..d819425 100644 --- a/packages/review-tutor/test/storage-export-sse.test.ts +++ b/packages/review-tutor/test/storage-export-sse.test.ts @@ -3,7 +3,9 @@ import type { ServerResponse } from "node:http"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; import { exportLearningHtml } from "../src/export-html.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; import { appendEntry, foldLog, @@ -15,6 +17,10 @@ import type { LearningEntry, QuizOutcome } from "../src/protocol.ts"; import { SseHub } from "../src/sse.ts"; const dirs: string[] = []; +const exportRegistry = createConnectorRegistry({ + flags: createReviewTutorFlags(), + piModels: [], +}); afterEach(async () => { await Promise.all(dirs.splice(0).map((path) => rm(path, { recursive: true, @@ -123,7 +129,10 @@ describe("standalone export", () => { entry("got_it"), entry("almost"), entry("review_again"), - ]); + { ...entry(), id: "colon-model", modelId: "ollama/qwen3:8b" }, + { ...entry(), id: "namespaced-colon-model", modelId: "pi:ollama/qwen3:8b" }, + { ...entry(), id: "invalid-model", modelId: 42 } as unknown as LearningEntry, + ], exportRegistry); expect(html).toContain("Private code warning"); expect(html).toContain("GitHub is the source of truth"); expect(html).toContain("https://github.com/a/b/pull/1"); @@ -132,13 +141,16 @@ describe("standalone export", () => { expect(html).toContain("Got it"); expect(html).toContain("Almost"); expect(html).toContain("Review again"); + expect(html.match(/
Harness<\/dt>
Pi<\/dd>/g)).toHaveLength(6); + expect(html.match(/
Model<\/dt>
ollama\/qwen3:8b<\/dd>/g)).toHaveLength(2); + expect(html).toContain("
Model
42
"); expect(html).toContain("<script>x</script>"); expect(html).toContain("& hostile <img src=x>"); expect(html).not.toContain("x"); expect(html).not.toMatch(/(?:src|href)=["']https?:\/\//); - const empty = exportLearningHtml([]); + const empty = exportLearningHtml([], exportRegistry); expect(empty).toContain("No learning entries yet"); expect(empty).not.toContain("
"); }); diff --git a/tsconfig.base.json b/tsconfig.base.json index 6df071d..c9c149c 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -13,9 +13,6 @@ ], "module": "ESNext", "moduleResolution": "Bundler", - "paths": { - "@pickforge/flags": ["./packages/flags/src/index.ts"] - }, "noUncheckedIndexedAccess": true, "resolveJsonModule": true, "skipLibCheck": true, diff --git a/vitest.config.ts b/vitest.config.ts index 57b40c2..e63691b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -2,11 +2,6 @@ import { defaultExclude } from "vitest/config"; import { defineConfig } from "vitest/config"; export default defineConfig({ - resolve: { - alias: { - "@pickforge/flags": new URL("./packages/flags/src/index.ts", import.meta.url).pathname, - }, - }, test: { coverage: { exclude: ["packages/**/dist/**", "packages/**/test/**", "**/*.test.ts"], From fb6a9cc890967462d916d5c308fab2d78ac7ffac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Tue, 25 Aug 2026 17:24:31 -0300 Subject: [PATCH 3/3] fix(review-tutor): keep the harness namespace in exports for disabled connectors --- .../review-tutor/src/connectors/registry.ts | 18 ++++++++++++------ packages/review-tutor/src/export-html.ts | 8 ++++---- .../review-tutor/test/server-extension.test.ts | 1 + .../test/storage-export-sse.test.ts | 3 +++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts index 5a77127..37bdd98 100644 --- a/packages/review-tutor/src/connectors/registry.ts +++ b/packages/review-tutor/src/connectors/registry.ts @@ -38,13 +38,9 @@ export function createConnectorRegistry(options: { return connectors().find((connector) => connector.id === id); }, resolve(modelId) { - const separator = modelId.indexOf(":"); - const slash = modelId.indexOf("/"); - const namespaced = separator >= 0 && (slash < 0 || separator < slash); - const harness = namespaced ? modelId.slice(0, separator) : "pi"; + const { harness, model } = splitModelId(modelId); const connector = this.byId(harness as HarnessId); - if (!connector) return undefined; - return { connector, model: namespaced ? modelId.slice(separator + 1) : modelId }; + return connector ? { connector, model } : undefined; }, async discoveries() { return Promise.all(connectors().map(async (connector) => ({ @@ -54,3 +50,13 @@ export function createConnectorRegistry(options: { }, }; } + +// A harness namespace is the text before the first ":" only when that ":" precedes any "/"; provider ids may contain ":". +export function splitModelId(modelId: string): { harness: string; model: string } { + const separator = modelId.indexOf(":"); + const slash = modelId.indexOf("/"); + const namespaced = separator >= 0 && (slash < 0 || separator < slash); + return namespaced + ? { harness: modelId.slice(0, separator), model: modelId.slice(separator + 1) } + : { harness: "pi", model: modelId }; +} diff --git a/packages/review-tutor/src/export-html.ts b/packages/review-tutor/src/export-html.ts index 2585f5a..2f5f542 100644 --- a/packages/review-tutor/src/export-html.ts +++ b/packages/review-tutor/src/export-html.ts @@ -1,4 +1,4 @@ -import type { ConnectorRegistry } from "./connectors/registry.ts"; +import { splitModelId, type ConnectorRegistry } from "./connectors/registry.ts"; import type { LearningEntry, QuizOutcome } from "./protocol.ts"; const QUIZ_LABELS: Record = { @@ -20,9 +20,9 @@ function escapeHtml(value: unknown): string { function modelDetails(entry: LearningEntry, registry: ConnectorRegistry): { harness: string; model: string } { if (typeof entry.modelId !== "string") return { harness: "Pi", model: String(entry.modelId) }; const resolved = registry.resolve(entry.modelId); - return resolved - ? { harness: resolved.connector.label, model: resolved.model } - : { harness: "Pi", model: entry.modelId }; + if (resolved) return { harness: resolved.connector.label, model: resolved.model }; + const { harness, model } = splitModelId(entry.modelId); + return { harness: harness === "pi" ? "Pi" : harness, model }; } function card(entry: LearningEntry, registry: ConnectorRegistry): string { diff --git a/packages/review-tutor/test/server-extension.test.ts b/packages/review-tutor/test/server-extension.test.ts index 048050d..5a1172a 100644 --- a/packages/review-tutor/test/server-extension.test.ts +++ b/packages/review-tutor/test/server-extension.test.ts @@ -362,6 +362,7 @@ describe("connector protocol boundary", () => { error: "model selection failed: unknown harness; refresh state and retry", }); + await waitFor(async () => ((await (await call(server.port, server.token, "/api/log")).json()) as unknown[]).length === 2); const exported = await (await call(server.port, server.token, "/api/export")).text(); expect(exported).toContain("
Harness
Pi
"); expect(exported).toContain("
Model
provider/model
"); diff --git a/packages/review-tutor/test/storage-export-sse.test.ts b/packages/review-tutor/test/storage-export-sse.test.ts index d819425..f881ba6 100644 --- a/packages/review-tutor/test/storage-export-sse.test.ts +++ b/packages/review-tutor/test/storage-export-sse.test.ts @@ -132,6 +132,7 @@ describe("standalone export", () => { { ...entry(), id: "colon-model", modelId: "ollama/qwen3:8b" }, { ...entry(), id: "namespaced-colon-model", modelId: "pi:ollama/qwen3:8b" }, { ...entry(), id: "invalid-model", modelId: 42 } as unknown as LearningEntry, + { ...entry(), id: "disabled-harness", modelId: "codex:gpt-5.6-sol" }, ], exportRegistry); expect(html).toContain("Private code warning"); expect(html).toContain("GitHub is the source of truth"); @@ -144,6 +145,8 @@ describe("standalone export", () => { expect(html.match(/
Harness<\/dt>
Pi<\/dd>/g)).toHaveLength(6); expect(html.match(/
Model<\/dt>
ollama\/qwen3:8b<\/dd>/g)).toHaveLength(2); expect(html).toContain("
Model
42
"); + expect(html).toContain("
Harness
codex
"); + expect(html).toContain("
Model
gpt-5.6-sol
"); expect(html).toContain("<script>x</script>"); expect(html).toContain("& hostile <img src=x>"); expect(html).not.toContain("