From fd577423abe5d2f8d53cb90126e5b6ee0452a1a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Wed, 26 Aug 2026 11:24:01 -0300 Subject: [PATCH] feat(review-tutor): always register the Claude Code and Codex connectors --- packages/review-tutor/README.md | 2 +- .../review-tutor/extensions/review-tutor.ts | 4 +-- .../review-tutor/src/connectors/registry.ts | 10 ++---- packages/review-tutor/src/flags.ts | 34 ------------------- .../review-tutor/test/connector-codex.test.ts | 10 ++---- packages/review-tutor/test/connectors.test.ts | 33 +++--------------- .../test/server-extension.test.ts | 21 +++++++++--- .../test/storage-export-sse.test.ts | 8 ++--- packages/review-tutor/test/structure.test.ts | 4 +-- 9 files changed, 32 insertions(+), 94 deletions(-) delete mode 100644 packages/review-tutor/src/flags.ts diff --git a/packages/review-tutor/README.md b/packages/review-tutor/README.md index 1cf6f88..89d9ff7 100644 --- a/packages/review-tutor/README.md +++ b/packages/review-tutor/README.md @@ -46,7 +46,7 @@ The model dialog lists the session's scoped models when `--models` or the settin ## 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 registered by default; the Claude Code and Codex connectors are available behind `reviewTutorHarnessConnectors`. The 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. +A harness connector owns model discovery, isolated invocation, and stream parsing while the shared runner owns process lifetime, bounds, and cancellation. Pi, Claude Code, and Codex are registered; a connector appears in the Harness picker only when its executable is found and supported at startup, and the helper line explains any that are not. 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. The Claude Code connector forwards `CLAUDE_CONFIG_DIR` when present, but never forwards `ANTHROPIC_API_KEY`; users who rely on that environment key must sign in through Claude Code instead. diff --git a/packages/review-tutor/extensions/review-tutor.ts b/packages/review-tutor/extensions/review-tutor.ts index b690ccf..c513fa1 100644 --- a/packages/review-tutor/extensions/review-tutor.ts +++ b/packages/review-tutor/extensions/review-tutor.ts @@ -6,7 +6,6 @@ import type { 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 { @@ -260,8 +259,7 @@ export default function reviewTutorExtension(pi: ExtensionAPI): void { "model snapshot failed: expected at least one available model; configure a Pi model and retry", ); } - const flags = createReviewTutorFlags(); - const registry = createConnectorRegistry({ flags, piModels }); + const registry = createConnectorRegistry({ piModels }); return startReviewTutorServer({ cwd, canonicalRepo, diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts index e754fc4..5527a90 100644 --- a/packages/review-tutor/src/connectors/registry.ts +++ b/packages/review-tutor/src/connectors/registry.ts @@ -1,5 +1,4 @@ import { execFile } from "node:child_process"; -import type { ReviewTutorFlags } from "../flags.ts"; import { ClaudeCodeConnector } from "./claude-code.ts"; import { CodexConnector } from "./codex.ts"; import { PiConnector } from "./pi.ts"; @@ -28,14 +27,12 @@ function which(command: string): Promise { } export function createConnectorRegistry(options: { - flags: ReviewTutorFlags; piModels: ModelChoice[]; piVersion?: string; which?: DiscoveryDeps["which"]; execFile?: DiscoveryExecFile; }): ConnectorRegistry { - const pi = new PiConnector(); - const optionalConnectors: HarnessConnector[] = [new ClaudeCodeConnector(), new CodexConnector()]; + const registered: HarnessConnector[] = [new PiConnector(), new ClaudeCodeConnector(), new CodexConnector()]; const dependencies: DiscoveryDeps = { piModels: options.piModels, ...(options.piVersion ? { piVersion: options.piVersion } : {}), @@ -43,10 +40,7 @@ export function createConnectorRegistry(options: { ...(options.execFile ? { execFile: options.execFile } : {}), }; - const connectors = (): HarnessConnector[] => [ - pi, - ...(options.flags.isEnabled("reviewTutorHarnessConnectors") ? optionalConnectors : []), - ]; + const connectors = (): HarnessConnector[] => registered; return { connectors, diff --git a/packages/review-tutor/src/flags.ts b/packages/review-tutor/src/flags.ts deleted file mode 100644 index 76372b2..0000000 --- a/packages/review-tutor/src/flags.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { - createFlags, - type FlagOverrideStore, - type Flags, -} from "../../flags/src/index.ts"; - -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/test/connector-codex.test.ts b/packages/review-tutor/test/connector-codex.test.ts index a2280fc..8e971ca 100644 --- a/packages/review-tutor/test/connector-codex.test.ts +++ b/packages/review-tutor/test/connector-codex.test.ts @@ -6,7 +6,6 @@ import { describe, expect, it, vi } from "vitest"; import { CodexConnector, discoveryEnvironment } from "../src/connectors/codex.ts"; import { createConnectorRegistry } from "../src/connectors/registry.ts"; import type { DiscoveryDeps, ParseSink } from "../src/connectors/types.ts"; -import { createReviewTutorFlags } from "../src/flags.ts"; import { TutorRunner } from "../src/runner.ts"; class FakeChild extends EventEmitter { @@ -406,19 +405,14 @@ describe("Codex runner integration", () => { }); describe("Codex registry", () => { - it("registers Codex only with the flag and preserves unavailable discovery", async () => { - const off = createConnectorRegistry({ - flags: createReviewTutorFlags({ get: () => false, set: () => {} }), - piModels, - }); + it("registers Codex and preserves unavailable discovery", async () => { const on = createConnectorRegistry({ - flags: createReviewTutorFlags({ get: () => true, set: () => {} }), piModels, + which: async () => undefined, execFile: fakeDiscovery({ "--version": Object.assign(new Error("spawn codex ENOENT"), { code: "ENOENT" }), }).execFile, }); - expect(off.connectors().map(({ id }) => id)).toEqual(["pi"]); expect(on.connectors().map(({ id }) => id)).toEqual(["pi", "claude-code", "codex"]); const discoveries = await on.discoveries(); expect(discoveries.find(({ connector }) => connector.id === "codex")).toEqual({ diff --git a/packages/review-tutor/test/connectors.test.ts b/packages/review-tutor/test/connectors.test.ts index 6ac0eb9..08dcccb 100644 --- a/packages/review-tutor/test/connectors.test.ts +++ b/packages/review-tutor/test/connectors.test.ts @@ -9,7 +9,6 @@ import { 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 { @@ -22,15 +21,8 @@ class FakeChild extends EventEmitter { 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 }); +function registry() { + return createConnectorRegistry({ piModels: models }); } const request = { @@ -42,22 +34,8 @@ const request = { }; 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("registers optional connectors only when the flag is on", () => { - expect(registry(false).connectors().map((connector) => connector.id)).toEqual(["pi"]); - expect(registry(true).connectors().map((connector) => connector.id)).toEqual(["pi", "claude-code", "codex"]); + it("registers Pi, Claude Code, and Codex in that order", () => { + expect(registry().connectors().map((connector) => connector.id)).toEqual(["pi", "claude-code", "codex"]); }); it("resolves namespaced and legacy Pi ids and rejects unknown harnesses", () => { @@ -71,8 +49,7 @@ describe("connector registry", () => { connector: { id: "pi" }, model: "ollama/qwen3:8b", }); - expect(registry().resolve("codex:x")).toBeUndefined(); - expect(registry(true).resolve("codex:x")).toMatchObject({ model: "x" }); + expect(registry().resolve("codex:x")).toMatchObject({ model: "x" }); expect(registry().resolve("unknown:x")).toBeUndefined(); }); diff --git a/packages/review-tutor/test/server-extension.test.ts b/packages/review-tutor/test/server-extension.test.ts index 2d14bb8..a3446fb 100644 --- a/packages/review-tutor/test/server-extension.test.ts +++ b/packages/review-tutor/test/server-extension.test.ts @@ -11,7 +11,6 @@ import { } 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"; @@ -21,12 +20,13 @@ const skillPath = fileURLToPath( new URL("../skills/review-tutor/SKILL.md", import.meta.url), ); const temporaryRoots: string[] = []; +const absentExecFile = async () => { throw Object.assign(new Error("spawn ENOENT"), { code: "ENOENT" }); }; function registry(models = [ { id: "provider/model", label: "Model", thinkingLevels: ["low"] }, { id: "ollama/qwen3:8b", label: "Qwen", thinkingLevels: ["low"] }, ]) { - return createConnectorRegistry({ flags: createReviewTutorFlags(), piModels: models }); + return createConnectorRegistry({ piModels: models, which: async () => undefined, execFile: absentExecFile }); } interface DeferredResult { @@ -322,7 +322,6 @@ describe("local server security", () => { describe("connector protocol boundary", () => { it("reports an unavailable Claude Code harness without offering its models when flagged on", async () => { const connectorRegistry = createConnectorRegistry({ - flags: createReviewTutorFlags({ get: () => true, set: () => {} }), piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], which: async () => undefined, execFile: async () => { throw Object.assign(new Error("spawn codex ENOENT"), { code: "ENOENT" }); }, @@ -359,7 +358,11 @@ describe("connector protocol boundary", () => { "pi:provider/model", "pi:ollama/qwen3:8b", ]); - expect(state.harnesses).toEqual([{ id: "pi", label: "Pi", available: true, models: state.models }]); + expect(state.harnesses).toEqual([ + { id: "pi", label: "Pi", available: true, models: state.models }, + { id: "claude-code", label: "Claude Code", available: false, reason: "Claude Code is not installed (claude not found on PATH).", models: [] }, + { id: "codex", label: "Codex", available: false, reason: "Codex is not installed (codex not found on PATH).", models: [] }, + ]); const source = await loadSource(server.port, server.token); expect((await ask(server.port, server.token, source.id)).status).toBe(202); @@ -387,12 +390,20 @@ describe("connector protocol boundary", () => { const unknown = await call(server.port, server.token, "/api/ask", { method: "POST", - body: JSON.stringify({ ...askBody(source.id), modelId: "codex:x" }), + body: JSON.stringify({ ...askBody(source.id), modelId: "unknown:x" }), }); expect(unknown.status).toBe(400); await expect(unknown.json()).resolves.toEqual({ error: "model selection failed: unknown harness; refresh state and retry", }); + const foreign = await call(server.port, server.token, "/api/ask", { + method: "POST", + body: JSON.stringify({ ...askBody(source.id), modelId: "codex:x" }), + }); + expect(foreign.status).toBe(400); + await expect(foreign.json()).resolves.toEqual({ + error: "model selection failed: expected an available model and thinking level; 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(); diff --git a/packages/review-tutor/test/storage-export-sse.test.ts b/packages/review-tutor/test/storage-export-sse.test.ts index f881ba6..19d0a6b 100644 --- a/packages/review-tutor/test/storage-export-sse.test.ts +++ b/packages/review-tutor/test/storage-export-sse.test.ts @@ -5,7 +5,6 @@ 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, @@ -18,7 +17,6 @@ import { SseHub } from "../src/sse.ts"; const dirs: string[] = []; const exportRegistry = createConnectorRegistry({ - flags: createReviewTutorFlags(), piModels: [], }); afterEach(async () => { @@ -132,7 +130,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" }, + { ...entry(), id: "unknown-harness", modelId: "future:gpt-9" }, ], exportRegistry); expect(html).toContain("Private code warning"); expect(html).toContain("GitHub is the source of truth"); @@ -145,8 +143,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("
Harness
future
"); + expect(html).toContain("
Model
gpt-9
"); expect(html).toContain("<script>x</script>"); expect(html).toContain("& hostile <img src=x>"); expect(html).not.toContain(" createConnectorRegistry({ - flags: createReviewTutorFlags(), + which: async () => undefined, + execFile: async () => { throw Object.assign(new Error("spawn ENOENT"), { code: "ENOENT" }); }, piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], });