diff --git a/AGENTS.md b/AGENTS.md index 72f3504ca..9b22bafca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -209,6 +209,8 @@ Ask AI providers are detected independently from installed/authenticated local C Per-origin choices are persisted in cookies, so a user can override the automatic match for one agent without changing the default for another. +> **Codex transport note:** the `codex-sdk` provider id is a stable identifier only — it no longer uses `@openai/codex-sdk` / `codex exec`. It drives a long-lived `codex app-server` process over JSON-RPC (`packages/ai/providers/codex-app-server.ts`), which respects the user's/enterprise-managed approval policy and supports interactive Allow/Deny approvals. The id stays `codex-sdk` to preserve saved cookie preferences, the `agents.ts` mapping, and the UI reasoning-effort gate. + ## Annotate Flow ``` diff --git a/apps/pi-extension/server/ai-runtime.ts b/apps/pi-extension/server/ai-runtime.ts index 743db01b2..fc2100c67 100644 --- a/apps/pi-extension/server/ai-runtime.ts +++ b/apps/pi-extension/server/ai-runtime.ts @@ -50,17 +50,18 @@ export async function createPiAIRuntime(options: CreatePiAIRuntimeOptions = {}): } try { - await import("../generated/ai/providers/codex-sdk.js"); - await import("@openai/codex-sdk"); + await import("../generated/ai/providers/codex-app-server.js"); const codexPath = whichCmd("codex"); - const provider = await ai.createProvider({ - type: "codex-sdk", - cwd, - ...(codexPath && { codexExecutablePath: codexPath }), - }); - registry.register(provider); + if (codexPath) { + const provider = await ai.createProvider({ + type: "codex-sdk", + cwd, + ...(codexPath ? { codexExecutablePath: codexPath } : {}), + }); + registry.register(provider); + } } catch { - // Codex SDK not available. + // Codex not available. } try { diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index a78493a03..cff7e5766 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -45,7 +45,7 @@ for f in index types provider session-manager endpoints context base-session; do printf '// @generated — DO NOT EDIT. Source: packages/ai/%s.ts\n' "$f" | cat - "$src" > "generated/ai/$f.ts" done -for f in claude-agent-sdk codex-sdk opencode-sdk command-path pi-sdk pi-sdk-node pi-events; do +for f in claude-agent-sdk codex-app-server opencode-sdk command-path pi-sdk pi-sdk-node pi-events; do src="../../packages/ai/providers/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/ai/providers/%s.ts\n' "$f" | cat - "$src" > "generated/ai/providers/$f.ts" done diff --git a/bun.lock b/bun.lock index 962817544..2fc429f20 100644 --- a/bun.lock +++ b/bun.lock @@ -6,7 +6,6 @@ "name": "plannotator", "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.2.92", - "@openai/codex-sdk": "^0.118.0", "@opencode-ai/sdk": "^1.3.0", "@pierre/diffs": "1.2.8", "diff": "^8.0.4", @@ -737,22 +736,6 @@ "@nodelib/fs.walk": ["@nodelib/fs.walk@1.2.8", "", { "dependencies": { "@nodelib/fs.scandir": "2.1.5", "fastq": "^1.6.0" } }, "sha512-oGB+UxlgWcgQkgwo8GcEGwemoTFt3FIO9ababBmaGwXIoBKZ+GTy0pP185beGg7Llih/NSHSV2XAs1lnznocSg=="], - "@openai/codex": ["@openai/codex@0.118.0", "", { "optionalDependencies": { "@openai/codex-darwin-arm64": "npm:@openai/codex@0.118.0-darwin-arm64", "@openai/codex-darwin-x64": "npm:@openai/codex@0.118.0-darwin-x64", "@openai/codex-linux-arm64": "npm:@openai/codex@0.118.0-linux-arm64", "@openai/codex-linux-x64": "npm:@openai/codex@0.118.0-linux-x64", "@openai/codex-win32-arm64": "npm:@openai/codex@0.118.0-win32-arm64", "@openai/codex-win32-x64": "npm:@openai/codex@0.118.0-win32-x64" }, "bin": { "codex": "bin/codex.js" } }, "sha512-oJhNOsP/Vu7DBUhodpP15z60cCZv3sKORqUn1+pepleqSTmc0zXJZSjz6fQOLzny9Ra6sRcvprFFKmQ3Q6+E6w=="], - - "@openai/codex-darwin-arm64": ["@openai/codex@0.118.0-darwin-arm64", "", { "os": "darwin", "cpu": "arm64" }, "sha512-sxq7Q2q9YiJN1wNrzvFRCC5oQKXPVUvu9Ejg6SI/CvyyI9YdtyLTO633wQJALGB7XQfT+3tyM5nNBivnSzLA3Q=="], - - "@openai/codex-darwin-x64": ["@openai/codex@0.118.0-darwin-x64", "", { "os": "darwin", "cpu": "x64" }, "sha512-iIvrqzsh1iJmVfqyYwLZBiuh0MkN1Suqniz9hBfmRmE6txMWhRVfigb9SMBiOkfCW1C4sjBwLCsCSzg11rvppQ=="], - - "@openai/codex-linux-arm64": ["@openai/codex@0.118.0-linux-arm64", "", { "os": "linux", "cpu": "arm64" }, "sha512-YKWmIqLBu9mmBhN3JHQNkLzk0BtAtV6LBPvhnL5eeHkNrChvA6PKrhsjy0O7wvRkiByZY/1dD14qVdAUfxcbiA=="], - - "@openai/codex-linux-x64": ["@openai/codex@0.118.0-linux-x64", "", { "os": "linux", "cpu": "x64" }, "sha512-vzU2d/gwAmZbf/DnwVzfQiNN3Ri04XpaZiJkHElIvGoqFbdrxRQFYTtslGDISYIO3o+POADZcFZIfamFsZj9yQ=="], - - "@openai/codex-sdk": ["@openai/codex-sdk@0.118.0", "", { "dependencies": { "@openai/codex": "0.118.0" } }, "sha512-cMisxx4CGQL44yv2USqdgcPhxPOhv227+CJiF9snn2X7nL+6wary4g38OknornjlrPob7LyzdIFB7dXqjkXKxg=="], - - "@openai/codex-win32-arm64": ["@openai/codex@0.118.0-win32-arm64", "", { "os": "win32", "cpu": "arm64" }, "sha512-AU7GPVFqd0Ml3P2OXfy6K6TONcxfCZLUh1zjYkkoPGWJ4A3BVMkOmMP9CAdSiQsnU/RQfdV/9IGqo1xwUQDdjA=="], - - "@openai/codex-win32-x64": ["@openai/codex@0.118.0-win32-x64", "", { "os": "win32", "cpu": "x64" }, "sha512-9vKW1ANQxIUsLpiY0VsOM+1avZjqxDZgqgxKABJHWRlLMuaSykwXexf8Hqq+BnYFfmn0d6MjxTuE1UtVvg9caw=="], - "@opencode-ai/plugin": ["@opencode-ai/plugin@1.16.2", "", { "dependencies": { "@opencode-ai/sdk": "1.16.2", "effect": "4.0.0-beta.74", "zod": "4.1.8" }, "peerDependencies": { "@opentui/core": ">=0.3.2", "@opentui/keymap": ">=0.3.2", "@opentui/solid": ">=0.3.2" }, "optionalPeers": ["@opentui/core", "@opentui/keymap", "@opentui/solid"] }, "sha512-FaZhVXrbz93xsdGLCtarRDTeqFt8AkLfh8B34tFBj6G4HXVmKSgBwVXmtELKKC+08xMtawBC9hshiMbXryv6cg=="], "@opencode-ai/sdk": ["@opencode-ai/sdk@1.16.2", "", { "dependencies": { "cross-spawn": "7.0.6" } }, "sha512-Z/xZ7q79dYeE0afqIk/yFEcRNGEQFcE+H8ssYivUiy+xGZ1mGwT72jpaQZKBwPn3JH4sRCu4KA2lcktBQfcOjg=="], diff --git a/package.json b/package.json index 111d61c92..d223fe96d 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ }, "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.2.92", - "@openai/codex-sdk": "^0.118.0", "@opencode-ai/sdk": "^1.3.0", "@pierre/diffs": "1.2.8", "diff": "^8.0.4", diff --git a/packages/ai/ai.test.ts b/packages/ai/ai.test.ts index 8c1a24c9d..bb306d7ad 100644 --- a/packages/ai/ai.test.ts +++ b/packages/ai/ai.test.ts @@ -404,6 +404,64 @@ describe("Context builders", () => { expect(prompt).toContain("[truncated for context window]"); expect(prompt.length).toBeLessThan(longPlan.length); }); + + test("every mode instructs the agent to answer directly (no unprompted review)", () => { + const modes: AIContext[] = [ + { mode: "plan-review", plan: { plan: "# Plan" } }, + { mode: "code-review", review: { patch: "+x" } }, + { mode: "annotate", annotate: { content: "# Doc", filePath: "/x.md" } }, + ]; + for (const ctx of modes) { + const prompt = buildSystemPrompt(ctx); + expect(prompt).toContain("Respond to the user's message directly"); + expect(prompt).toContain("do NOT review"); + } + }); + + test("code-review with a git-reproducible diffType instructs git, does not paste the diff", () => { + const ctx: AIContext = { + mode: "code-review", + review: { + patch: "diff --git a/foo.ts b/foo.ts\n+secret-paste-marker", + diffType: "branch", + base: "develop", + }, + }; + const prompt = buildSystemPrompt(ctx); + expect(prompt).toContain("git diff develop..HEAD"); + // The whole diff must NOT be pasted when the agent can reproduce it. + expect(prompt).not.toContain("secret-paste-marker"); + expect(prompt).not.toContain("```diff"); + }); + + test("code-review merge-base uses three-dot diff (no shell substitution)", () => { + const ctx: AIContext = { + mode: "code-review", + review: { patch: "+x", diffType: "merge-base", base: "main" }, + }; + const prompt = buildSystemPrompt(ctx); + expect(prompt).toContain("git diff main...HEAD"); + expect(prompt).not.toContain("$("); + }); + + test("code-review falls back to pasting the diff for non-git diff types", () => { + const ctx: AIContext = { + mode: "code-review", + review: { patch: "diff --git a/foo.ts\n+pasted", diffType: "jj-current" }, + }; + const prompt = buildSystemPrompt(ctx); + expect(prompt).toContain("```diff"); + expect(prompt).toContain("+pasted"); + }); + + test("code-review with no diffType pastes the diff (back-compat)", () => { + const ctx: AIContext = { + mode: "code-review", + review: { patch: "diff --git a/foo.ts\n+pasted" }, + }; + const prompt = buildSystemPrompt(ctx); + expect(prompt).toContain("+pasted"); + }); }); // --------------------------------------------------------------------------- @@ -875,165 +933,84 @@ describe("resolveSDKModel", () => { }); // --------------------------------------------------------------------------- -// Codex SDK event mapping +// Codex app-server provider — JSON-RPC classification + event mapping // --------------------------------------------------------------------------- import { - mapCodexEvent, - mapCodexItem, - shouldSkipGitRepoCheck, -} from "./providers/codex-sdk.ts"; - -describe("shouldSkipGitRepoCheck", () => { - test("keeps the Codex git repo check inside a worktree", async () => { - const probe = async (command: string, args: string[], options: { encoding: "utf8" }) => { - expect(command).toBe("git"); - expect(args).toEqual(["-C", "/repo", "rev-parse", "--is-inside-work-tree"]); - expect(options).toEqual({ encoding: "utf8" }); - return { stdout: "true\n" }; - }; - - expect(await shouldSkipGitRepoCheck("/repo", probe)).toBe(false); - }); - - test("skips the Codex git repo check for standalone document sessions", async () => { - const probe = async () => { - throw new Error("not a git repo"); - }; - - expect(await shouldSkipGitRepoCheck("/tmp/plain-session", probe)).toBe(true); - }); - - test("skips the Codex git repo check when the probe cannot run", async () => { - const probe = async () => { - throw new Error("git unavailable"); - }; - - expect(await shouldSkipGitRepoCheck("/tmp/plain-session", probe)).toBe(true); - }); -}); - -describe("mapCodexEvent", () => { - function offsets() { - return new Map(); - } - - test("thread.started returns empty", () => { - const result = mapCodexEvent( - { type: "thread.started", thread_id: "t-123" }, - offsets(), - ); - expect(result).toEqual([]); - }); - - test("turn.started and turn.completed return empty", () => { - expect(mapCodexEvent({ type: "turn.started" }, offsets())).toEqual([]); - expect(mapCodexEvent({ type: "turn.completed", usage: {} }, offsets())).toEqual([]); + classifyRpcMessage, + mapApprovalRequest, + mapCodexAppServerEvent, +} from "./providers/codex-app-server.ts"; + +describe("classifyRpcMessage", () => { + test("a message with id + result is a response", () => { + expect(classifyRpcMessage({ id: 7, result: { ok: true } })).toEqual({ + kind: "response", + id: 7, + }); }); - test("turn.failed returns error", () => { - const result = mapCodexEvent( - { type: "turn.failed", error: { message: "Out of tokens" } }, - offsets(), - ); - expect(result).toEqual([{ - type: "error", - error: "Out of tokens", - code: "turn_failed", - }]); + test("a message with id + method is a server request (not a response)", () => { + // id-space collision guard: even though id 7 could match a pending client + // request, the presence of `method` means this is an inbound request. + expect( + classifyRpcMessage({ + id: 7, + method: "item/commandExecution/requestApproval", + params: { command: "ls" }, + }), + ).toEqual({ + kind: "request", + id: 7, + method: "item/commandExecution/requestApproval", + params: { command: "ls" }, + }); }); - test("error event returns error", () => { - const result = mapCodexEvent( - { type: "error", message: "Connection lost" }, - offsets(), - ); - expect(result).toEqual([{ - type: "error", - error: "Connection lost", - code: "codex_error", - }]); + test("a message with method and no id is a notification", () => { + expect( + classifyRpcMessage({ method: "item/agentMessage/delta", params: { delta: "hi" } }), + ).toEqual({ + kind: "notification", + method: "item/agentMessage/delta", + params: { delta: "hi" }, + }); }); - test("unknown event type passes through", () => { - const result = mapCodexEvent( - { type: "some.future.event", data: 42 }, - offsets(), - ); - expect(result.length).toBe(1); - expect(result[0].type).toBe("unknown"); + test("a message with neither id nor method is unknown", () => { + expect(classifyRpcMessage({ foo: 1 })).toEqual({ kind: "unknown" }); }); }); -describe("mapCodexItem — agent_message", () => { - function offsets() { - return new Map(); - } - - test("item.started initializes offset tracker, returns empty", () => { - const o = offsets(); - const result = mapCodexItem( - { type: "item.started", item: { id: "msg-1", type: "agent_message", text: "" } }, - o, - ); - expect(result).toEqual([]); - expect(o.get("msg-1")).toBe(0); - }); - - test("item.updated emits text_delta from cumulative text", () => { - const o = offsets(); - o.set("msg-1", 0); - - const r1 = mapCodexItem( - { type: "item.updated", item: { id: "msg-1", type: "agent_message", text: "Hello" } }, - o, - ); - expect(r1).toEqual([{ type: "text_delta", delta: "Hello" }]); - expect(o.get("msg-1")).toBe(5); - - const r2 = mapCodexItem( - { type: "item.updated", item: { id: "msg-1", type: "agent_message", text: "Hello world" } }, - o, - ); - expect(r2).toEqual([{ type: "text_delta", delta: " world" }]); - expect(o.get("msg-1")).toBe(11); - }); - - test("item.updated with no new text returns empty", () => { - const o = offsets(); - o.set("msg-1", 5); - - const result = mapCodexItem( - { type: "item.updated", item: { id: "msg-1", type: "agent_message", text: "Hello" } }, - o, - ); - expect(result).toEqual([]); +describe("mapCodexAppServerEvent", () => { + test("agentMessage delta maps to text_delta", () => { + expect( + mapCodexAppServerEvent( + { method: "item/agentMessage/delta", params: { delta: "Hello" } }, + "thr-1", + ), + ).toEqual([{ type: "text_delta", delta: "Hello" }]); }); - test("item.completed emits full text and cleans up offset", () => { - const o = offsets(); - o.set("msg-1", 5); - - const result = mapCodexItem( - { type: "item.completed", item: { id: "msg-1", type: "agent_message", text: "Hello world" } }, - o, - ); - expect(result).toEqual([{ type: "text", text: "Hello world" }]); - expect(o.has("msg-1")).toBe(false); + test("empty agentMessage delta is ignored", () => { + expect( + mapCodexAppServerEvent( + { method: "item/agentMessage/delta", params: { delta: "" } }, + "thr-1", + ), + ).toEqual([]); }); -}); - -describe("mapCodexItem — command_execution", () => { - function offsets() { - return new Map(); - } - test("item.started emits tool_use", () => { - const result = mapCodexItem( - { type: "item.started", item: { id: "cmd-1", type: "command_execution", command: "ls -la", status: "in_progress" } }, - offsets(), - ); - expect(result).toEqual([{ + test("commandExecution item/started maps to tool_use", () => { + expect( + mapCodexAppServerEvent( + { + method: "item/started", + params: { item: { id: "cmd-1", type: "commandExecution", command: "ls -la" } }, + }, + "thr-1", + ), + ).toEqual([{ type: "tool_use", toolName: "Bash", toolInput: { command: "ls -la" }, @@ -1041,10 +1018,15 @@ describe("mapCodexItem — command_execution", () => { }]); }); - test("item.completed emits tool_result with exit code", () => { - const result = mapCodexItem( - { type: "item.completed", item: { id: "cmd-1", type: "command_execution", command: "ls", aggregated_output: "file.txt\n", exit_code: 0, status: "completed" } }, - offsets(), + test("commandExecution item/completed maps to tool_result with exit code", () => { + const result = mapCodexAppServerEvent( + { + method: "item/completed", + params: { + item: { id: "cmd-1", type: "commandExecution", aggregatedOutput: "file.txt\n", exitCode: 0 }, + }, + }, + "thr-1", ); expect(result.length).toBe(1); expect(result[0].type).toBe("tool_result"); @@ -1053,82 +1035,77 @@ describe("mapCodexItem — command_execution", () => { expect(result[0].result).toContain("[exit code: 0]"); } }); -}); - -describe("mapCodexItem — file_change", () => { - function offsets() { - return new Map(); - } - test("emits tool_use with changes", () => { - const result = mapCodexItem( - { type: "item.completed", item: { id: "fc-1", type: "file_change", changes: [{ path: "src/foo.ts", kind: "update" }], status: "completed" } }, - offsets(), - ); - expect(result.length).toBe(1); - expect(result[0].type).toBe("tool_use"); - if (result[0].type === "tool_use") { - expect(result[0].toolName).toBe("FileChange"); - } + test("turn/completed (success) maps to result with sessionId", () => { + expect( + mapCodexAppServerEvent( + { method: "turn/completed", params: { turn: { status: "completed" } } }, + "thr-1", + ), + ).toEqual([{ type: "result", sessionId: "thr-1", success: true }]); }); -}); -describe("mapCodexItem — mcp_tool_call", () => { - function offsets() { - return new Map(); - } + test("turn/completed (failed) maps to error", () => { + expect( + mapCodexAppServerEvent( + { + method: "turn/completed", + params: { turn: { status: "failed", error: { message: "boom" } } }, + }, + "thr-1", + ), + ).toEqual([{ type: "error", error: "boom", code: "turn_failed" }]); + }); - test("item.started emits tool_use with server/tool name", () => { - const result = mapCodexItem( - { type: "item.started", item: { id: "mcp-1", type: "mcp_tool_call", server: "github", tool: "search", arguments: { q: "test" }, status: "in_progress" } }, - offsets(), - ); - expect(result).toEqual([{ - type: "tool_use", - toolName: "github/search", - toolInput: { q: "test" }, - toolUseId: "mcp-1", - }]); + test("process_exited maps to a provider error", () => { + const result = mapCodexAppServerEvent({ method: "process_exited", params: {} }, "thr-1"); + expect(result[0].type).toBe("error"); + if (result[0].type === "error") expect(result[0].code).toBe("provider_error"); }); - test("item.completed with result emits tool_result", () => { - const result = mapCodexItem( - { type: "item.completed", item: { id: "mcp-1", type: "mcp_tool_call", server: "github", tool: "search", arguments: {}, result: "found 3 items", status: "completed" } }, - offsets(), - ); - expect(result.some(m => m.type === "tool_result")).toBe(true); + test("informational notifications are ignored", () => { + expect(mapCodexAppServerEvent({ method: "turn/started", params: {} }, "thr-1")).toEqual([]); + expect(mapCodexAppServerEvent({ method: "thread/started", params: {} }, "thr-1")).toEqual([]); }); - test("item.completed with error emits error", () => { - const result = mapCodexItem( - { type: "item.completed", item: { id: "mcp-1", type: "mcp_tool_call", server: "github", tool: "search", arguments: {}, error: { message: "rate limited" }, status: "completed" } }, - offsets(), - ); - expect(result.some(m => m.type === "error")).toBe(true); + test("unknown notification passes through", () => { + const result = mapCodexAppServerEvent({ method: "some/future/event", params: {} }, "thr-1"); + expect(result.length).toBe(1); + expect(result[0].type).toBe("unknown"); }); }); -describe("mapCodexItem — error and passthrough types", () => { - function offsets() { - return new Map(); - } - - test("error item maps to error message", () => { - const result = mapCodexItem( - { type: "item.completed", item: { id: "e-1", type: "error", message: "Something broke" } }, - offsets(), +describe("mapApprovalRequest", () => { + test("command execution approval maps to a Bash permission_request", () => { + const msg = mapApprovalRequest( + "item/commandExecution/requestApproval", + { command: "rm -rf build", cwd: "/repo", itemId: "item-1", reason: "needs write" }, + "42", ); - expect(result).toEqual([{ type: "error", error: "Something broke" }]); + expect(msg.type).toBe("permission_request"); + expect(msg.requestId).toBe("42"); + expect(msg.toolName).toBe("Bash"); + expect(msg.toolInput).toEqual({ command: "rm -rf build", cwd: "/repo" }); + expect(msg.toolUseId).toBe("item-1"); + expect(msg.description).toBe("needs write"); + }); + + test("file change approval maps to a FileChange permission_request", () => { + const msg = mapApprovalRequest( + "item/fileChange/requestApproval", + { itemId: "item-2", reason: "edit src/foo.ts" }, + "43", + ); + expect(msg.toolName).toBe("FileChange"); + expect(msg.title).toBe("edit src/foo.ts"); + expect(msg.requestId).toBe("43"); }); - test("reasoning, web_search, todo_list pass through as unknown", () => { - for (const itemType of ["reasoning", "web_search", "todo_list"]) { - const result = mapCodexItem( - { type: "item.completed", item: { id: "x-1", type: itemType, text: "thinking..." } }, - offsets(), - ); - expect(result[0].type).toBe("unknown"); - } + test("unknown approval method falls back to a generic permission_request", () => { + const msg = mapApprovalRequest("item/something/requestApproval", { foo: 1 }, "44"); + expect(msg.type).toBe("permission_request"); + expect(msg.toolName).toBe("item/something/requestApproval"); + expect(msg.toolInput).toEqual({ foo: 1 }); }); }); diff --git a/packages/ai/context.ts b/packages/ai/context.ts index 40e0fdce9..d18d3d826 100644 --- a/packages/ai/context.ts +++ b/packages/ai/context.ts @@ -42,7 +42,8 @@ export function buildSystemPrompt(ctx: AIContext): string { export function buildForkPreamble(ctx: AIContext): string { const lines: string[] = [ "The user is now reviewing your work in Plannotator and has a question.", - "Answer concisely based on the conversation history and the context below.", + "Answer the user's message directly and concisely based on the conversation " + + "history and the context below. Do not re-review or summarize the work unless they ask.", "", ]; @@ -140,15 +141,63 @@ export function buildEffectivePrompt( const MAX_PLAN_CHARS = 60_000; const MAX_DIFF_CHARS = 40_000; +/** + * Leading instruction for every Ask AI session. Ask AI is a chat assistant — + * it must respond to the user's message, not launch an unprompted review of the + * material it was given for context. + */ +const ANSWER_DIRECTLY = + "You are a helpful assistant inside Plannotator. Respond to the user's message directly and concisely. " + + "The material below is context for what the user is looking at — do NOT review, summarize, or critique it unless the user's message asks you to. " + + "Only investigate further (read files, run git) if the user's question actually requires it."; + function truncate(text: string, max: number): string { if (text.length <= max) return text; return `${text.slice(0, max)}\n\n... [truncated for context window]`; } +/** + * For git-reproducible diff types, return a one-line instruction telling the + * agent how to inspect the changes itself — so we don't paste the whole diff. + * Returns null for types that can't be reproduced with a single git command + * (jj, Perforce, stacked-PR full-stack, workspace, unknown) — those fall back + * to pasting the diff. + * + * NOTE: kept self-contained here because `packages/ai` is dependency-free; the + * server's richer `getLocalDiffInstruction` (agent-review-message.ts) remains + * the source of truth for the agent-jobs path. + */ +function gitInspectInstruction( + diffType: string | undefined, + base: string | undefined, +): string | null { + const b = base || "main"; + switch (diffType) { + case "uncommitted": + return "Run `git diff HEAD` to see the changes (and `git status` for untracked files)."; + case "staged": + return "Run `git diff --staged` to see the changes."; + case "unstaged": + return "Run `git diff` to see the changes."; + case "last-commit": + return "Run `git diff HEAD~1..HEAD` to see the changes."; + case "branch": + return `Run \`git diff ${b}..HEAD\` to see the changes.`; + case "merge-base": + // Three-dot diff = changes on HEAD since the merge-base with the base — + // the GitHub PR view. Single command, no shell substitution needed. + return `Run \`git diff ${b}...HEAD\` to see the changes (matches the PR view).`; + default: + return null; + } +} + function buildPlanReviewPrompt( ctx: Extract ): string { const sections: string[] = [ + ANSWER_DIRECTLY, + "", "The user is reviewing an implementation plan in Plannotator.", "", "## Plan Under Review", @@ -185,28 +234,38 @@ function buildCodeReviewPrompt( ctx: Extract ): string { const sections: string[] = [ - "The user is reviewing a code diff in Plannotator.", + ANSWER_DIRECTLY, + "", + "The user is reviewing a set of code changes in Plannotator.", ]; - if (ctx.review.filePath) { + const inspect = gitInspectInstruction(ctx.review.diffType, ctx.review.base); + + if (inspect) { + // Git-reproducible: tell the agent how to look instead of pasting the diff. + sections.push(""); + sections.push("## The changes under review"); + sections.push( + `The current working directory is the repository being reviewed. ${inspect}`, + ); + } else { + // Fallback (jj, Perforce, stacked-PR full-stack, workspace, unknown): + // paste the diff since it can't be reproduced with a single git command. sections.push(""); - sections.push(`## Currently Viewing: ${ctx.review.filePath}`); + sections.push("## Diff"); + sections.push("```diff"); + sections.push(truncate(ctx.review.patch, MAX_DIFF_CHARS)); + sections.push("```"); } if (ctx.review.selectedCode) { sections.push(""); - sections.push("## Selected Code"); + sections.push("## Code the user has selected"); sections.push("```"); sections.push(ctx.review.selectedCode); sections.push("```"); } - sections.push(""); - sections.push("## Diff"); - sections.push("```diff"); - sections.push(truncate(ctx.review.patch, MAX_DIFF_CHARS)); - sections.push("```"); - if (ctx.review.annotations) { sections.push(""); sections.push("## User Annotations"); @@ -220,6 +279,8 @@ function buildAnnotatePrompt( ctx: Extract ): string { const sections: string[] = [ + ANSWER_DIRECTLY, + "", "The user is annotating a markdown document in Plannotator.", "", `## Document: ${ctx.annotate.filePath}`, diff --git a/packages/ai/package.json b/packages/ai/package.json index 7cd03cac6..ab31f15ce 100644 --- a/packages/ai/package.json +++ b/packages/ai/package.json @@ -11,7 +11,7 @@ "./session-manager": "./session-manager.ts", "./endpoints": "./endpoints.ts", "./providers/claude-agent-sdk": "./providers/claude-agent-sdk.ts", - "./providers/codex-sdk": "./providers/codex-sdk.ts", + "./providers/codex-app-server": "./providers/codex-app-server.ts", "./providers/command-path": "./providers/command-path.ts", "./providers/pi-sdk": "./providers/pi-sdk.ts", "./providers/opencode-sdk": "./providers/opencode-sdk.ts", diff --git a/packages/ai/providers/claude-agent-sdk.ts b/packages/ai/providers/claude-agent-sdk.ts index f4d11a9d1..4ad83f8ac 100644 --- a/packages/ai/providers/claude-agent-sdk.ts +++ b/packages/ai/providers/claude-agent-sdk.ts @@ -28,8 +28,13 @@ import type { const PROVIDER_NAME = "claude-agent-sdk"; -/** Default read-only tools for inline chat. */ -const DEFAULT_ALLOWED_TOOLS = ["Read", "Glob", "Grep", "WebSearch"]; +/** + * Default tools for inline chat. Read-only investigation plus Bash so the + * agent can inspect the changes itself (e.g. `git diff`) instead of having the + * whole diff pasted into the prompt. Anything beyond safe reads is still gated + * by the permission flow (approvals stay on) and surfaced as an Allow/Deny card. + */ +const DEFAULT_ALLOWED_TOOLS = ["Read", "Glob", "Grep", "WebSearch", "Bash"]; const DEFAULT_MAX_TURNS = 99; const DEFAULT_MODEL = "claude-sonnet-4-6"; diff --git a/packages/ai/providers/codex-app-server.ts b/packages/ai/providers/codex-app-server.ts new file mode 100644 index 000000000..5753fa874 --- /dev/null +++ b/packages/ai/providers/codex-app-server.ts @@ -0,0 +1,876 @@ +/** + * Codex provider — `codex app-server` transport (registered as "codex-sdk"). + * + * Why this replaces the old `@openai/codex-sdk` (codex exec) provider: + * The SDK ran `codex exec`, a headless one-shot mode that hard-codes + * `approval_policy = never`. In enterprise-managed Codex environments that + * ban `never`, Ask AI broke (GitHub #971). `codex exec` also has no channel + * to ask a human for approval. + * + * This provider instead drives a long-lived `codex app-server` process over + * newline-delimited JSON-RPC 2.0 (stdio). The fix for #971 is simply to OMIT + * `approvalPolicy` at `thread/start` so Codex resolves the user's + company's + * configured policy itself; we pin `sandbox: "read-only"` to keep Ask AI safe. + * Codex's approval prompts arrive as server→client JSON-RPC *requests*, which + * we surface as the existing `permission_request` AIMessage and answer through + * the existing `respondToPermission` path (same Allow/Deny UI as Claude). + * + * The registry name stays "codex-sdk" (it is the persisted cookie providerId, + * the key in agents.ts, and the UI reasoning-effort gate) — only the transport + * changed. Implemented with node:child_process so a single file works under + * both the Bun server and the Node (Pi) extension. + * + * Note: `codex app-server`'s `thread/start` has no `skipGitRepoCheck` param — + * unlike `codex exec`, it does not gate on being inside a git repo, so the + * #965 "Ask AI outside git repos" fix needs no special handling here. + */ + +import { spawn, type ChildProcess } from "node:child_process"; +import { BaseSession } from "../base-session.ts"; +import { buildEffectivePrompt, buildSystemPrompt } from "../context.ts"; +import type { + AIMessage, + AIPermissionRequestMessage, + AIProvider, + AIProviderCapabilities, + AISession, + CodexSDKConfig, + CreateSessionOptions, +} from "../types.ts"; +import { registerProviderFactory } from "../provider.ts"; +import { + buildWindowsCommandScriptSpawnCommand, + killWindowsProcessTree, + resolveWindowsCommandShim, +} from "./command-path.ts"; + +const PROVIDER_NAME = "codex-sdk"; +const DEFAULT_MODEL = "gpt-5.4"; +const CLIENT_NAME = "plannotator"; +/** Kill an idle app-server process after this long with no query. */ +const IDLE_TIMEOUT_MS = 10 * 60_000; +/** Reject a JSON-RPC request that gets no response in this long (RPC acks are + * fast — turn output streams via notifications, not the turn/start response). */ +const RPC_TIMEOUT_MS = 30_000; + +const CMD_APPROVAL_METHOD = "item/commandExecution/requestApproval"; +const FILE_APPROVAL_METHOD = "item/fileChange/requestApproval"; + +// --------------------------------------------------------------------------- +// Pure helpers (exported for testing) +// --------------------------------------------------------------------------- + +export type RpcMessage = Record; + +export type RpcClassification = + | { kind: "response"; id: string | number } + | { kind: "request"; id: string | number; method: string; params: RpcMessage } + | { kind: "notification"; method: string; params: RpcMessage } + | { kind: "unknown" }; + +/** + * Classify an incoming JSON-RPC message by SHAPE (not by a `type` field): + * - response: has `id`, no `method` + * - server request: has both `id` and `method` + * - notification: has `method`, no `id` + * Client-sent ids and server-sent ids live in separate id spaces, so a message + * carrying `method` is NEVER treated as a response to one of our requests. + */ +export function classifyRpcMessage(msg: RpcMessage): RpcClassification { + const hasId = msg.id !== undefined && msg.id !== null; + const hasMethod = typeof msg.method === "string"; + if (hasId && !hasMethod) { + return { kind: "response", id: msg.id as string | number }; + } + if (hasId && hasMethod) { + return { + kind: "request", + id: msg.id as string | number, + method: msg.method as string, + params: (msg.params as RpcMessage) ?? {}, + }; + } + if (hasMethod) { + return { + kind: "notification", + method: msg.method as string, + params: (msg.params as RpcMessage) ?? {}, + }; + } + return { kind: "unknown" }; +} + +/** + * Map a Codex app-server notification (`{ method, params }`) to AIMessages. + * `sessionId` is the resolved thread id, injected into the terminal result. + */ +export function mapCodexAppServerEvent( + notification: { method: string; params?: RpcMessage }, + sessionId: string, +): AIMessage[] { + const method = notification.method; + const params = notification.params ?? {}; + + switch (method) { + case "item/agentMessage/delta": { + const delta = params.delta as string | undefined; + return delta ? [{ type: "text_delta", delta }] : []; + } + + case "item/started": { + const item = params.item as RpcMessage | undefined; + if (item?.type === "commandExecution") { + return [{ + type: "tool_use", + toolName: "Bash", + toolInput: { command: (item.command as string) ?? "" }, + toolUseId: (item.id as string) ?? "", + }]; + } + return []; + } + + case "item/completed": { + const item = params.item as RpcMessage | undefined; + if (item?.type === "commandExecution") { + const output = (item.aggregatedOutput as string) ?? ""; + const exitCode = item.exitCode as number | undefined; + return [{ + type: "tool_result", + toolUseId: (item.id as string) ?? "", + result: exitCode != null ? `${output}\n[exit code: ${exitCode}]` : output, + }]; + } + if (item?.type === "error") { + return [{ type: "error", error: (item.message as string) ?? "Error" }]; + } + // agentMessage / fileChange / reasoning: streamed via deltas or shown + // via approvals — nothing to emit on completion. + return []; + } + + case "turn/completed": { + const turn = params.turn as RpcMessage | undefined; + const status = turn?.status as string | undefined; + if (status === "failed") { + const error = turn?.error as RpcMessage | undefined; + return [{ + type: "error", + error: (error?.message as string) ?? "Turn failed", + code: "turn_failed", + }]; + } + return [{ type: "result", sessionId, success: true }]; + } + + case "error": + return [{ + type: "error", + error: (params.message as string) ?? "Unknown error", + code: "codex_error", + }]; + + case "process_exited": + return [{ + type: "error", + error: "Codex app-server process exited unexpectedly.", + code: "provider_error", + }]; + + // Streaming-only / informational notifications we intentionally ignore. + case "thread/started": + case "turn/started": + case "turn/diff/updated": + case "turn/plan/updated": + case "item/reasoning/textDelta": + case "item/reasoning/summaryTextDelta": + case "thread/tokenUsage/updated": + return []; + + default: + return [{ type: "unknown", raw: notification }]; + } +} + +/** + * Map an inbound approval REQUEST to a `permission_request` AIMessage so the + * existing PermissionCard renders it. `requestId` correlates the user's later + * decision back to the JSON-RPC request id. + */ +export function mapApprovalRequest( + method: string, + params: RpcMessage, + requestId: string, +): AIPermissionRequestMessage { + const toolUseId = (params.itemId as string) ?? requestId; + const reason = params.reason as string | undefined; + + if (method === CMD_APPROVAL_METHOD) { + return { + type: "permission_request", + requestId, + toolName: "Bash", + toolInput: { + command: (params.command as string) ?? "", + ...(params.cwd ? { cwd: params.cwd } : {}), + }, + ...(reason ? { description: reason } : {}), + toolUseId, + }; + } + + if (method === FILE_APPROVAL_METHOD) { + return { + type: "permission_request", + requestId, + toolName: "FileChange", + toolInput: {}, + title: reason ?? "Approve file changes", + toolUseId, + }; + } + + // Generic fallback for any other approval-shaped request. + return { + type: "permission_request", + requestId, + toolName: method, + toolInput: params, + ...(reason ? { description: reason } : {}), + toolUseId, + }; +} + +// --------------------------------------------------------------------------- +// JSON-RPC over stdio (bidirectional: responses, notifications, server requests) +// --------------------------------------------------------------------------- + +type NotificationListener = (notification: { method: string; params: RpcMessage }) => void; +type RequestHandler = (method: string, id: string | number, params: RpcMessage) => void; + +class CodexAppServerProcess { + private proc: ChildProcess | null = null; + private listeners: NotificationListener[] = []; + private requestHandlers: RequestHandler[] = []; + private pendingRequests = new Map< + string, + { resolve: (data: RpcMessage) => void; reject: (err: Error) => void } + >(); + private nextId = 0; + private buffer = ""; + private _alive = false; + private startPromise: Promise | null = null; + + /** Spawn + JSON-RPC initialize handshake, once. */ + start(codexPath: string, cwd: string): Promise { + if (!this.startPromise) { + this.startPromise = this.doStart(codexPath, cwd).catch((err) => { + this.startPromise = null; + throw err; + }); + } + return this.startPromise; + } + + private async doStart(codexPath: string, cwd: string): Promise { + const commandPath = resolveWindowsCommandShim(codexPath); + const command = + buildWindowsCommandScriptSpawnCommand(commandPath, ["app-server"]) ?? [ + commandPath, + "app-server", + ]; + + let proc: ChildProcess; + try { + const [file, ...args] = command; + // stderr is "ignore", not "pipe": we never read it, and an un-drained + // stderr pipe deadlocks the child once its buffer fills. + proc = spawn(file, args, { cwd, stdio: ["pipe", "pipe", "ignore"] }); + } catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); + this.handleProcessEnd(error); + throw error; + } + + this.proc = proc; + proc.once("exit", () => { + this.handleProcessEnd(new Error("Codex app-server exited unexpectedly")); + }); + + await new Promise((resolve, reject) => { + const cleanup = () => { + proc.off("spawn", onSpawn); + proc.off("error", onError); + }; + const onSpawn = () => { + cleanup(); + this._alive = true; + this.readStream(); + resolve(); + }; + const onError = (err: Error) => { + cleanup(); + this.handleProcessEnd(err); + reject(err); + }; + proc.once("spawn", onSpawn); + proc.once("error", onError); + }); + + // JSON-RPC handshake: initialize (request) then initialized (notification). + await this.sendAndWait({ + method: "initialize", + params: { + clientInfo: { name: CLIENT_NAME, title: "Plannotator", version: "1.0.0" }, + }, + }); + this.send({ method: "initialized", params: {} }); + } + + private handleProcessEnd(error: Error): void { + if (!this.proc && this.pendingRequests.size === 0) return; + this._alive = false; + this.proc = null; + for (const [, pending] of this.pendingRequests) pending.reject(error); + this.pendingRequests.clear(); + for (const listener of this.listeners) { + listener({ method: "process_exited", params: {} }); + } + } + + private readStream(): void { + if (!this.proc?.stdout) return; + this.proc.stdout.on("data", (chunk: Buffer) => { + this.buffer += chunk.toString(); + const lines = this.buffer.split("\n"); + this.buffer = lines.pop() ?? ""; + for (const line of lines) { + const trimmed = line.replace(/\r$/, ""); + if (!trimmed) continue; + try { + this.routeMessage(JSON.parse(trimmed)); + } catch { + // Ignore malformed lines. + } + } + }); + } + + private routeMessage(msg: RpcMessage): void { + const classified = classifyRpcMessage(msg); + switch (classified.kind) { + case "response": { + const pending = this.pendingRequests.get(String(classified.id)); + if (!pending) return; + this.pendingRequests.delete(String(classified.id)); + if (msg.error) { + const err = msg.error as RpcMessage; + pending.reject(new Error((err.message as string) ?? "RPC error")); + } else { + pending.resolve((msg.result as RpcMessage) ?? {}); + } + return; + } + case "request": { + for (const handler of this.requestHandlers) { + handler(classified.method, classified.id, classified.params); + } + return; + } + case "notification": { + for (const listener of this.listeners) { + listener({ method: classified.method, params: classified.params }); + } + return; + } + default: + return; + } + } + + send(message: RpcMessage): void { + if (!this.proc?.stdin || this.proc.stdin.destroyed) return; + this.proc.stdin.write(`${JSON.stringify(message)}\n`); + } + + sendAndWait(message: RpcMessage, timeoutMs = RPC_TIMEOUT_MS): Promise { + const id = ++this.nextId; + const key = String(id); + return new Promise((resolve, reject) => { + // Guard against a process that's alive but unresponsive (e.g. stalled on + // auth, or a turn queued behind an interrupting turn) — without a timer + // the request would hang forever. + const timer = setTimeout(() => { + if (this.pendingRequests.delete(key)) { + reject(new Error(`Codex app-server did not respond to ${String(message.method)} in ${timeoutMs}ms`)); + } + }, timeoutMs); + (timer as { unref?: () => void }).unref?.(); + this.pendingRequests.set(key, { + resolve: (data) => { clearTimeout(timer); resolve(data); }, + reject: (err) => { clearTimeout(timer); reject(err); }, + }); + this.send({ ...message, id }); + }); + } + + /** Answer an inbound server request with a JSON-RPC result. */ + respond(id: string | number, result: RpcMessage): void { + this.send({ id, result }); + } + + /** Answer an inbound server request with a JSON-RPC error. */ + respondError(id: string | number, message: string): void { + this.send({ id, error: { code: -32601, message } }); + } + + onEvent(listener: NotificationListener): () => void { + this.listeners.push(listener); + return () => { + const idx = this.listeners.indexOf(listener); + if (idx >= 0) this.listeners.splice(idx, 1); + }; + } + + onRequest(handler: RequestHandler): () => void { + this.requestHandlers.push(handler); + return () => { + const idx = this.requestHandlers.indexOf(handler); + if (idx >= 0) this.requestHandlers.splice(idx, 1); + }; + } + + get alive(): boolean { + return this._alive; + } + + kill(): void { + this._alive = false; + this.startPromise = null; + const proc = this.proc; + this.proc = null; + if (proc) { + if (!killWindowsProcessTree(proc.pid)) proc.kill(); + } + this.listeners.length = 0; + this.requestHandlers.length = 0; + for (const [, pending] of this.pendingRequests) { + pending.reject(new Error("Process killed")); + } + this.pendingRequests.clear(); + } +} + +// --------------------------------------------------------------------------- +// Provider +// --------------------------------------------------------------------------- + +export class CodexAppServerProvider implements AIProvider { + readonly name = PROVIDER_NAME; + readonly capabilities: AIProviderCapabilities = { + fork: false, // thread/fork needs a shared thread store — out of scope + resume: true, + streaming: true, + tools: true, + }; + readonly models = [ + { id: "gpt-5.5", label: "GPT-5.5" }, + { id: "gpt-5.4", label: "GPT-5.4", default: true }, + { id: "gpt-5.4-mini", label: "GPT-5.4 Mini" }, + { id: "gpt-5.3-codex", label: "GPT-5.3 Codex" }, + { id: "gpt-5.3-codex-spark", label: "GPT-5.3 Codex Spark" }, + { id: "gpt-5.2-codex", label: "GPT-5.2 Codex" }, + { id: "gpt-5.2", label: "GPT-5.2" }, + ] as const; + + private config: CodexSDKConfig; + private sessions = new Set(); + + constructor(config: CodexSDKConfig) { + this.config = config; + } + + async createSession(options: CreateSessionOptions): Promise { + const session = new CodexAppServerSession({ + systemPrompt: buildSystemPrompt(options.context), + cwd: options.cwd ?? this.config.cwd ?? process.cwd(), + parentSessionId: null, + codexExecutablePath: this.config.codexExecutablePath ?? "codex", + model: options.model ?? this.config.model ?? DEFAULT_MODEL, + reasoningEffort: options.reasoningEffort, + onClosed: (s) => this.sessions.delete(s), + }); + this.sessions.add(session); + return session; + } + + async forkSession(): Promise { + throw new Error( + "Codex does not support session forking. " + + "The endpoint layer should fall back to createSession().", + ); + } + + async resumeSession(sessionId: string): Promise { + const session = new CodexAppServerSession({ + systemPrompt: null, // resumed thread already carries its context + cwd: this.config.cwd ?? process.cwd(), + parentSessionId: null, + codexExecutablePath: this.config.codexExecutablePath ?? "codex", + model: this.config.model ?? DEFAULT_MODEL, + resumeThreadId: sessionId, + onClosed: (s) => this.sessions.delete(s), + }); + this.sessions.add(session); + return session; + } + + dispose(): void { + for (const session of this.sessions) session.dispose(); + this.sessions.clear(); + } +} + +// --------------------------------------------------------------------------- +// Session +// --------------------------------------------------------------------------- + +interface SessionConfig { + systemPrompt: string | null; + cwd: string; + parentSessionId: string | null; + codexExecutablePath: string; + model: string; + reasoningEffort?: "minimal" | "low" | "medium" | "high" | "xhigh"; + resumeThreadId?: string; + onClosed: (session: CodexAppServerSession) => void; +} + +class CodexAppServerSession extends BaseSession { + private config: SessionConfig; + private process: CodexAppServerProcess | null = null; + private threadStarted = false; + /** + * The thread id used on the wire for turn/start + turn/interrupt. Usually + * equal to the client-facing `id`, but decoupled so a resume failure can + * fall back to a fresh thread without breaking the client's session id. + */ + private liveThreadId: string | null = null; + private activeTurnId: string | null = null; + /** A turn we aborted; its still-arriving events must be ignored (it may keep + * flushing output on the shared thread before Codex stops it). */ + private abandonedTurnId: string | null = null; + /** requestId → inbound JSON-RPC request id awaiting a decision. */ + private pendingApprovals = new Map(); + private idleTimer: ReturnType | null = null; + + constructor(config: SessionConfig) { + super({ + parentSessionId: config.parentSessionId, + initialId: config.resumeThreadId, + }); + this.config = config; + if (config.resumeThreadId) this._resolvedId = config.resumeThreadId; + } + + async *query(prompt: string): AsyncIterable { + const started = this.startQuery(); + if (!started) { + yield BaseSession.BUSY_ERROR; + return; + } + const { gen, signal } = started; + this.clearIdleTimer(); + + try { + yield* this.runTurn(prompt, gen, signal); + } catch (err) { + yield { + type: "error", + error: err instanceof Error ? err.message : String(err), + code: "provider_error", + }; + } finally { + this.endQuery(gen); + this.scheduleIdleTimer(); + } + } + + private async *runTurn( + prompt: string, + gen: number, + signal: AbortSignal, + ): AsyncIterable { + await this.ensureThread(); + // Stopped (or superseded by a newer query) during startup — bail before + // creating a turn so Codex doesn't run one in the background. + if (signal.aborted) return; + + const proc = this.process; + if (!proc || !proc.alive) { + yield { + type: "error", + error: + "Codex app-server exited during startup. Check that Codex is installed and authenticated (`codex login`).", + code: "codex_startup_error", + }; + return; + } + + const effectivePrompt = buildEffectivePrompt( + prompt, + this.config.systemPrompt, + this._firstQuerySent, + ); + + const queue: AIMessage[] = []; + let resolve: (() => void) | null = null; + let done = false; + const push = (msg: AIMessage) => { + queue.push(msg); + resolve?.(); + }; + const finish = () => { + done = true; + resolve?.(); + }; + + // Generation guard: once a newer query starts (or this one is aborted and + // superseded), this turn's listeners must not push to its queue, end the + // wrong turn, or mutate shared session state. + const isCurrent = () => this._queryGen === gen; + + // Set once turn/start returns. Events that carry a different turn id — + // e.g. a just-aborted turn still flushing output on the same thread — are + // then ignored, so an old turn can't pollute or end this one. + let myTurnId: string | null = null; + const eventTurnId = (notif: { method: string; params: RpcMessage }): string | undefined => + notif.method === "turn/completed" + ? ((notif.params.turn as RpcMessage | undefined)?.id as string | undefined) + : (notif.params.turnId as string | undefined); + + const unsubEvents = proc.onEvent((notif) => { + if (!isCurrent()) return; + if (notif.method !== "process_exited") { + const evTurn = eventTurnId(notif); + // Reject an aborted turn's stragglers, or — once our turn id is known — + // any event from a different turn. + if (evTurn && (evTurn === this.abandonedTurnId || (myTurnId && evTurn !== myTurnId))) { + return; + } + } + for (const msg of mapCodexAppServerEvent(notif, this.id)) push(msg); + if (notif.method === "turn/completed" || notif.method === "process_exited") { + finish(); + } + }); + const unsubRequests = proc.onRequest((method, id, params) => { + const isApproval = method === CMD_APPROVAL_METHOD || method === FILE_APPROVAL_METHOD; + // Cancel approvals from a superseded generation or a different turn + // (e.g. a just-aborted turn) instead of surfacing them to this UI. + const evTurn = params.turnId as string | undefined; + const staleTurn = + isApproval && + !!evTurn && + (evTurn === this.abandonedTurnId || (!!myTurnId && evTurn !== myTurnId)); + if (!isCurrent() || staleTurn) { + if (isApproval) proc.respond(id, { decision: "cancel" }); + else proc.respondError(id, `Unsupported request: ${method}`); + return; + } + if (isApproval) { + const requestId = String(id); + this.pendingApprovals.set(requestId, id); + push(mapApprovalRequest(method, params, requestId)); + } else { + proc.respondError(id, `Unsupported request: ${method}`); + } + }); + + // End the drain loop promptly on abort instead of waiting for turn/completed + // (which may never arrive if startup was interrupted). + const onAbort = () => finish(); + signal.addEventListener("abort", onAbort); + + const cleanup = () => { + signal.removeEventListener("abort", onAbort); + unsubEvents(); + unsubRequests(); + // Only the current turn owns the shared session state; a superseded turn + // must not wipe the live turn's id or approvals. + if (isCurrent()) { + this.activeTurnId = null; + this.pendingApprovals.clear(); + } + }; + + try { + const res = await proc.sendAndWait({ + method: "turn/start", + params: { + threadId: this.liveThreadId ?? this.id, + input: [{ type: "text", text: effectivePrompt, text_elements: [] }], + ...(this.config.reasoningEffort && { effort: this.config.reasoningEffort }), + }, + }); + myTurnId = ((res.turn as RpcMessage | undefined)?.id as string) ?? null; + this.activeTurnId = myTurnId; + } catch (err) { + cleanup(); + yield { + type: "error", + error: `Codex rejected the turn: ${err instanceof Error ? err.message : String(err)}`, + code: "codex_turn_rejected", + }; + return; + } + this._firstQuerySent = true; + + // Aborted while turn/start was in flight — now that we know the turn id, + // interrupt it and stop, instead of streaming a turn the user cancelled. + if (signal.aborted) { + if (myTurnId) this.abandonedTurnId = myTurnId; + this.interruptActiveTurn(); + cleanup(); + return; + } + + try { + while (!done || queue.length > 0) { + if (queue.length > 0) { + yield queue.shift()!; + } else { + await new Promise((r) => { + resolve = r; + }); + resolve = null; + } + } + } finally { + cleanup(); + } + } + + /** thread/start params — OMIT approvalPolicy so Codex resolves the user's + + * enterprise-managed policy itself (the #971 fix); pin a read-only sandbox. */ + private threadStartParams(): RpcMessage { + return { + method: "thread/start", + params: { model: this.config.model, cwd: this.config.cwd, sandbox: "read-only" }, + }; + } + + /** Ensure a live process + an active thread (start fresh or resume). */ + private async ensureThread(): Promise { + if (this.process?.alive && this.threadStarted) return; + + if (!this.process || !this.process.alive) { + this.process = new CodexAppServerProcess(); + await this.process.start(this.config.codexExecutablePath, this.config.cwd); + this.threadStarted = false; + } + if (this.threadStarted) return; + + if (this._resolvedId) { + // Resume an existing thread (explicit resume, or after an idle restart). + try { + await this.process.sendAndWait({ + method: "thread/resume", + params: { threadId: this._resolvedId }, + }); + this.liveThreadId = this._resolvedId; + } catch { + // The stored rollout is unavailable — start a fresh thread on the wire + // but keep the client-facing session id stable (history is lost, but + // the chat keeps working). + const res = await this.process.sendAndWait(this.threadStartParams()); + this.liveThreadId = + ((res.thread as RpcMessage | undefined)?.id as string) ?? this._resolvedId; + } + } else { + const res = await this.process.sendAndWait(this.threadStartParams()); + const threadId = (res.thread as RpcMessage | undefined)?.id as string | undefined; + if (threadId) { + this.resolveId(threadId); + this.liveThreadId = threadId; + } + } + this.threadStarted = true; + } + + respondToPermission(requestId: string, allow: boolean): void { + const id = this.pendingApprovals.get(requestId); + if (id === undefined || !this.process) return; + this.pendingApprovals.delete(requestId); + this.process.respond(id, { decision: allow ? "accept" : "decline" }); + } + + /** Tell Codex to interrupt the in-flight turn (no-op if none is running). + * turn/interrupt is a JSON-RPC *request* (not a notification), so it must + * carry an id — we fire it and don't await the ack. */ + private interruptActiveTurn(): void { + const threadId = this.liveThreadId ?? this._resolvedId; + if (this.process && this.activeTurnId && threadId) { + this.process + .sendAndWait({ + method: "turn/interrupt", + params: { threadId, turnId: this.activeTurnId }, + }) + .catch(() => {}); + } + } + + abort(): void { + // Tear down the turn cleanly: cancel outstanding approvals so Codex doesn't + // hang, interrupt the active turn, but keep the process alive (resumable). + if (this.process) { + for (const [, id] of this.pendingApprovals) { + this.process.respond(id, { decision: "cancel" }); + } + this.pendingApprovals.clear(); + // Remember the turn so any output it keeps flushing is ignored. + if (this.activeTurnId) this.abandonedTurnId = this.activeTurnId; + this.interruptActiveTurn(); + } + super.abort(); + } + + /** Kill the process and release the session (idle timeout, evict, dispose). */ + dispose(): void { + this.clearIdleTimer(); + this.process?.kill(); + this.process = null; + this.threadStarted = false; + this.pendingApprovals.clear(); + this.config.onClosed(this); + } + + private clearIdleTimer(): void { + if (this.idleTimer) { + clearTimeout(this.idleTimer); + this.idleTimer = null; + } + } + + private scheduleIdleTimer(): void { + this.clearIdleTimer(); + this.idleTimer = setTimeout(() => { + // Kill the idle process but keep the session resumable: the next query + // re-spawns and `thread/resume`s the persisted thread id. + this.process?.kill(); + this.process = null; + this.threadStarted = false; + }, IDLE_TIMEOUT_MS); + // Don't keep the event loop alive solely for the idle timer. + (this.idleTimer as { unref?: () => void })?.unref?.(); + } +} + +// --------------------------------------------------------------------------- +// Factory registration (same name as the old SDK provider) +// --------------------------------------------------------------------------- + +registerProviderFactory( + PROVIDER_NAME, + async (config) => new CodexAppServerProvider(config as CodexSDKConfig), +); diff --git a/packages/ai/providers/codex-sdk.ts b/packages/ai/providers/codex-sdk.ts deleted file mode 100644 index 293535a00..000000000 --- a/packages/ai/providers/codex-sdk.ts +++ /dev/null @@ -1,465 +0,0 @@ -/** - * Codex SDK provider — bridges Plannotator's AI layer with OpenAI's Codex agent. - * - * Uses @openai/codex-sdk to create sessions that can: - * - Start fresh with Plannotator context as the system prompt - * - Fake-fork from a parent session (fresh thread + preamble, no real history) - * - Resume a previous thread by ID - * - Stream text deltas back to the UI in real time - * - * Sessions default to read-only sandbox mode for safety in inline chat. - */ - -import { buildSystemPrompt, buildEffectivePrompt } from "../context.ts"; -import { BaseSession } from "../base-session.ts"; -import { execFile } from "node:child_process"; -import { promisify } from "node:util"; -import type { - AIProvider, - AIProviderCapabilities, - AISession, - AIMessage, - CreateSessionOptions, - CodexSDKConfig, -} from "../types.ts"; - -// --------------------------------------------------------------------------- -// Constants -// --------------------------------------------------------------------------- - -const PROVIDER_NAME = "codex-sdk"; -const DEFAULT_MODEL = "gpt-5.4"; - -type GitWorkTreeProbe = ( - command: string, - args: string[], - options: { encoding: "utf8" }, -) => Promise<{ stdout?: string | Buffer }>; - -const execFileAsync = promisify(execFile); - -export async function shouldSkipGitRepoCheck( - cwd: string, - probe: GitWorkTreeProbe = execFileAsync, -): Promise { - try { - const result = probe( - "git", - ["-C", cwd, "rev-parse", "--is-inside-work-tree"], - { encoding: "utf8" }, - ); - return String((await result).stdout ?? "").trim() !== "true"; - } catch { - return true; - } -} - -// --------------------------------------------------------------------------- -// Provider -// --------------------------------------------------------------------------- - -export class CodexSDKProvider implements AIProvider { - readonly name = PROVIDER_NAME; - readonly capabilities: AIProviderCapabilities = { - fork: false, // No real fork — faked with fresh thread + preamble - resume: true, - streaming: true, - tools: true, - }; - readonly models = [ - { id: 'gpt-5.5', label: 'GPT-5.5' }, - { id: 'gpt-5.4', label: 'GPT-5.4', default: true }, - { id: 'gpt-5.4-mini', label: 'GPT-5.4 Mini' }, - { id: 'gpt-5.3-codex', label: 'GPT-5.3 Codex' }, - { id: 'gpt-5.3-codex-spark', label: 'GPT-5.3 Codex Spark' }, - { id: 'gpt-5.2-codex', label: 'GPT-5.2 Codex' }, - { id: 'gpt-5.2', label: 'GPT-5.2' }, - ] as const; - - private config: CodexSDKConfig; - - constructor(config: CodexSDKConfig) { - this.config = config; - } - - async createSession(options: CreateSessionOptions): Promise { - const cwd = options.cwd ?? this.config.cwd ?? process.cwd(); - const skipGitRepoCheck = await shouldSkipGitRepoCheck(cwd); - return new CodexSDKSession({ - ...this.baseConfig(options), - systemPrompt: buildSystemPrompt(options.context), - cwd, - skipGitRepoCheck, - parentSessionId: null, - }); - } - - async forkSession(_options: CreateSessionOptions): Promise { - throw new Error( - "Codex does not support session forking. " + - "The endpoint layer should fall back to createSession()." - ); - } - - async resumeSession(sessionId: string): Promise { - const cwd = this.config.cwd ?? process.cwd(); - const skipGitRepoCheck = await shouldSkipGitRepoCheck(cwd); - return new CodexSDKSession({ - ...this.baseConfig(), - systemPrompt: null, - cwd, - skipGitRepoCheck, - parentSessionId: null, - resumeThreadId: sessionId, - }); - } - - dispose(): void { - // No persistent resources to clean up - } - - private baseConfig(options?: CreateSessionOptions) { - return { - model: options?.model ?? this.config.model ?? DEFAULT_MODEL, - maxTurns: options?.maxTurns ?? 99, - sandboxMode: this.config.sandboxMode ?? "read-only" as const, - codexExecutablePath: this.config.codexExecutablePath, - reasoningEffort: options?.reasoningEffort, - }; - } -} - -// --------------------------------------------------------------------------- -// SDK import cache — resolve once, reuse across all sessions -// --------------------------------------------------------------------------- - -// biome-ignore lint/suspicious/noExplicitAny: SDK type not available at compile time -let CodexClass: any = null; - -async function getCodexClass() { - if (!CodexClass) { - // biome-ignore lint/suspicious/noExplicitAny: SDK exports vary between versions - const mod = await import("@openai/codex-sdk") as any; - CodexClass = mod.default ?? mod.Codex; - } - return CodexClass; -} - -// --------------------------------------------------------------------------- -// Session -// --------------------------------------------------------------------------- - -interface SessionConfig { - systemPrompt: string | null; - model: string; - maxTurns: number; - sandboxMode: "read-only" | "workspace-write" | "danger-full-access"; - cwd: string; - skipGitRepoCheck: boolean; - parentSessionId: string | null; - resumeThreadId?: string; - codexExecutablePath?: string; - reasoningEffort?: "minimal" | "low" | "medium" | "high" | "xhigh"; -} - -class CodexSDKSession extends BaseSession { - private config: SessionConfig; - // biome-ignore lint/suspicious/noExplicitAny: SDK types not available at compile time - private _codexInstance: any = null; - // biome-ignore lint/suspicious/noExplicitAny: SDK types not available at compile time - private _thread: any = null; - /** Tracks cumulative text length per item for delta extraction. */ - private _itemTextOffsets = new Map(); - - constructor(config: SessionConfig) { - super({ - parentSessionId: config.parentSessionId, - initialId: config.resumeThreadId, - }); - this.config = config; - // If resuming, treat the thread ID as already resolved - if (config.resumeThreadId) { - this._resolvedId = config.resumeThreadId; - } - } - - async *query(prompt: string): AsyncIterable { - const started = this.startQuery(); - if (!started) { yield BaseSession.BUSY_ERROR; return; } - const { gen, signal } = started; - - this._itemTextOffsets.clear(); - - try { - const Codex = await getCodexClass(); - - // Lazy-create the Codex instance - if (!this._codexInstance) { - this._codexInstance = new Codex({ - ...(this.config.codexExecutablePath && { codexPathOverride: this.config.codexExecutablePath }), - }); - } - - // Lazy-create or resume the thread - if (!this._thread) { - if (this.config.resumeThreadId) { - this._thread = this._codexInstance.resumeThread(this.config.resumeThreadId, { - model: this.config.model, - workingDirectory: this.config.cwd, - skipGitRepoCheck: this.config.skipGitRepoCheck, - sandboxMode: this.config.sandboxMode, - ...(this.config.reasoningEffort && { modelReasoningEffort: this.config.reasoningEffort }), - }); - } else { - this._thread = this._codexInstance.startThread({ - model: this.config.model, - workingDirectory: this.config.cwd, - skipGitRepoCheck: this.config.skipGitRepoCheck, - sandboxMode: this.config.sandboxMode, - ...(this.config.reasoningEffort && { modelReasoningEffort: this.config.reasoningEffort }), - }); - } - } - - const effectivePrompt = buildEffectivePrompt( - prompt, - this.config.systemPrompt, - this._firstQuerySent, - ); - const streamed = await this._thread.runStreamed(effectivePrompt, { - signal, - }); - - this._firstQuerySent = true; - let turnFailed = false; - - for await (const event of streamed.events) { - // ID resolution from thread.started - if ( - !this._resolvedId && - event.type === "thread.started" && - typeof event.thread_id === "string" - ) { - this.resolveId(event.thread_id); - } - - if (event.type === "turn.failed") { - turnFailed = true; - } - - const mapped = mapCodexEvent(event, this._itemTextOffsets); - for (const msg of mapped) { - yield msg; - } - } - - // Emit synthetic result after stream ends - if (!turnFailed) { - yield { - type: "result", - sessionId: this.id, - success: true, - }; - } - } catch (err) { - yield { - type: "error", - error: err instanceof Error ? err.message : String(err), - code: "provider_error", - }; - } finally { - this.endQuery(gen); - } - } - -} - -// --------------------------------------------------------------------------- -// Event mapping -// --------------------------------------------------------------------------- - -/** - * Map a Codex SDK ThreadEvent to one or more AIMessages. - * - * The itemTextOffsets map tracks cumulative text length per item ID - * so we can extract true deltas from the cumulative text in item.updated events. - */ -function mapCodexEvent( - event: Record, - itemTextOffsets: Map, -): AIMessage[] { - const eventType = event.type as string; - - switch (eventType) { - case "thread.started": - case "turn.started": - return []; - - case "turn.completed": - return []; - - case "turn.failed": { - const error = event.error as Record | undefined; - return [{ - type: "error", - error: (error?.message as string) ?? "Turn failed", - code: "turn_failed", - }]; - } - - case "error": - return [{ - type: "error", - error: (event.message as string) ?? "Unknown error", - code: "codex_error", - }]; - - case "item.started": - case "item.updated": - case "item.completed": - return mapCodexItem(event, itemTextOffsets); - - default: - return [{ type: "unknown", raw: event }]; - } -} - -/** - * Map item-level events to AIMessages. - */ -function mapCodexItem( - event: Record, - itemTextOffsets: Map, -): AIMessage[] { - const item = event.item as Record; - if (!item) return [{ type: "unknown", raw: event }]; - - const eventType = event.type as string; - const itemType = item.type as string; - const itemId = (item.id as string) ?? ""; - const isStarted = eventType === "item.started"; - const isCompleted = eventType === "item.completed"; - - switch (itemType) { - case "agent_message": { - const text = (item.text as string) ?? ""; - - if (isStarted) { - // Reset offset tracking for this item - itemTextOffsets.set(itemId, 0); - return []; - } - - if (isCompleted) { - // Emit final complete text - itemTextOffsets.delete(itemId); - return text ? [{ type: "text", text }] : []; - } - - // item.updated — extract delta from cumulative text - const prevOffset = itemTextOffsets.get(itemId) ?? 0; - if (text.length > prevOffset) { - const delta = text.slice(prevOffset); - itemTextOffsets.set(itemId, text.length); - return [{ type: "text_delta", delta }]; - } - return []; - } - - case "command_execution": { - const messages: AIMessage[] = []; - if (isStarted) { - messages.push({ - type: "tool_use", - toolName: "Bash", - toolInput: { command: item.command as string }, - toolUseId: itemId, - }); - } - if (isCompleted) { - const output = (item.aggregated_output as string) ?? ""; - const exitCode = item.exit_code as number | undefined; - messages.push({ - type: "tool_result", - toolUseId: itemId, - result: exitCode != null ? `${output}\n[exit code: ${exitCode}]` : output, - }); - } - return messages; - } - - case "file_change": { - const changes = item.changes as Array<{ path: string; kind: string }> | undefined; - if (isStarted || isCompleted) { - return [{ - type: "tool_use", - toolName: "FileChange", - toolInput: { changes: changes ?? [] }, - toolUseId: itemId, - }]; - } - return []; - } - - case "mcp_tool_call": { - const messages: AIMessage[] = []; - if (isStarted) { - messages.push({ - type: "tool_use", - toolName: `${item.server as string}/${item.tool as string}`, - toolInput: (item.arguments as Record) ?? {}, - toolUseId: itemId, - }); - } - if (isCompleted) { - if (item.result != null) { - messages.push({ - type: "tool_result", - toolUseId: itemId, - result: typeof item.result === "string" ? item.result : JSON.stringify(item.result), - }); - } - if (item.error) { - const err = item.error as Record; - messages.push({ - type: "error", - error: (err.message as string) ?? "MCP tool call failed", - code: "mcp_error", - }); - } - } - return messages; - } - - case "error": - return [{ - type: "error", - error: (item.message as string) ?? "Unknown error", - }]; - - case "reasoning": - case "web_search": - case "todo_list": - return [{ type: "unknown", raw: { eventType, item } }]; - - default: - return [{ type: "unknown", raw: { eventType, item } }]; - } -} - -// --------------------------------------------------------------------------- -// Exported for testing -// --------------------------------------------------------------------------- - -export { mapCodexEvent, mapCodexItem }; - -// --------------------------------------------------------------------------- -// Factory registration -// --------------------------------------------------------------------------- - -import { registerProviderFactory } from "../provider.ts"; - -registerProviderFactory( - PROVIDER_NAME, - async (config) => new CodexSDKProvider(config as CodexSDKConfig) -); diff --git a/packages/ai/session-manager.ts b/packages/ai/session-manager.ts index 15c11b124..aeb5971cc 100644 --- a/packages/ai/session-manager.ts +++ b/packages/ai/session-manager.ts @@ -123,7 +123,11 @@ export class SessionManager { */ remove(sessionId: string): void { const canonical = this.resolve(sessionId); + const entry = this.sessions.get(canonical); this.sessions.delete(canonical); + // Release any long-lived resources (e.g. a spawned subprocess). No-op for + // providers whose sessions don't implement dispose(). + entry?.session.dispose?.(); // Clean up any aliases pointing to this session for (const [alias, target] of this.aliases) { if (target === canonical) this.aliases.delete(alias); @@ -163,6 +167,8 @@ export class SessionManager { if (entry.session.isActive) { entry.session.abort(); } + // Release long-lived resources. No-op for providers without dispose(). + entry.session.dispose?.(); } this.sessions.clear(); this.aliases.clear(); @@ -185,7 +191,11 @@ export class SessionManager { } if (oldest) { + const entry = this.sessions.get(oldest.id); this.sessions.delete(oldest.id); + // Release long-lived resources held by the evicted session. No-op for + // providers whose sessions don't implement dispose(). + entry?.session.dispose?.(); // Clean up aliases pointing to the evicted session for (const [alias, target] of this.aliases) { if (target === oldest.id) this.aliases.delete(alias); diff --git a/packages/ai/types.ts b/packages/ai/types.ts index b87e79f8f..ef5804914 100644 --- a/packages/ai/types.ts +++ b/packages/ai/types.ts @@ -48,8 +48,15 @@ export interface PlanContext { * Passed when AIContextMode is "code-review". */ export interface CodeReviewContext { - /** The unified diff patch. */ + /** The unified diff patch. Used as a fallback when the changeset can't be + * reproduced locally with a single VCS command. */ patch: string; + /** The VCS diff type (e.g. "uncommitted", "branch", "merge-base"). When set + * to a git-reproducible type, the prompt tells the agent how to inspect the + * changes with git instead of pasting the whole diff. */ + diffType?: string; + /** The base branch/ref the diff is computed against (for branch/merge-base). */ + base?: string; /** The specific file being discussed (if scoped). */ filePath?: string; /** The line range being discussed (if scoped). */ @@ -194,6 +201,13 @@ export interface AISession { */ respondToPermission?(requestId: string, allow: boolean, message?: string): void; + /** + * Release any long-lived resources held by this session (e.g. a spawned + * subprocess). Called by the SessionManager when a session is evicted or + * removed. Optional — providers without persistent resources omit it. + */ + dispose?(): void; + /** * Callback invoked when the real session ID is resolved from the provider. * Set by the SessionManager to remap its internal tracking key. diff --git a/packages/editor/App.tsx b/packages/editor/App.tsx index 119715010..2eb5827e6 100644 --- a/packages/editor/App.tsx +++ b/packages/editor/App.tsx @@ -3077,6 +3077,7 @@ const App: React.FC = () => { permissionRequests: aiPermissionRequests, respondToPermission: respondToAIPermission, ask: askAI, + abort: abortAI, resetSession: resetAISession, resetThread: resetAIThread, sessionId: aiSessionId, @@ -4421,6 +4422,7 @@ const App: React.FC = () => { isCreatingSession={isAgentTerminalReady ? false : aiIsCreatingSession} isStreaming={isAgentTerminalReady ? false : aiIsStreaming} onAskGeneral={handleAskGeneralAI} + onStop={isAgentTerminalReady ? undefined : abortAI} permissionRequests={isAgentTerminalReady ? [] : aiPermissionRequests} onRespondToPermission={isAgentTerminalReady ? undefined : respondToAIPermission} aiProviders={visibleAIProviders} diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 155418361..ee8701573 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -460,6 +460,12 @@ const ReviewApp: React.FC = () => { }, []); const aiChat = useAIChat({ patch: diffData?.rawPatch ?? '', + diffType, + base: committedBase, + viewing: { + scope: isAllFilesActive ? 'all' : 'file', + filePath: isAllFilesActive ? undefined : files[activeFileIndex]?.path, + }, providerId: aiConfig.providerId, model: aiConfig.model, reasoningEffort: aiConfig.reasoningEffort, @@ -471,6 +477,7 @@ const ReviewApp: React.FC = () => { permissionRequests: aiPermissionRequests, respondToPermission: respondToAIPermission, ask: askAI, + abort: abortAI, resetSession: resetAISession, sessionId: aiSessionId, } = aiChat; @@ -2603,6 +2610,7 @@ const ReviewApp: React.FC = () => { aiMessages={aiMessages} isAICreatingSession={aiIsCreatingSession} isAIStreaming={aiIsStreaming} + onAIStop={abortAI} onScrollToAILines={handleScrollToAILines} activeFilePath={files[activeFileIndex]?.path} scrollToQuestionId={scrollToQuestionId} diff --git a/packages/review-editor/components/AITab.tsx b/packages/review-editor/components/AITab.tsx index 91eeae08b..c77d5747c 100644 --- a/packages/review-editor/components/AITab.tsx +++ b/packages/review-editor/components/AITab.tsx @@ -21,6 +21,8 @@ interface AITabProps { scrollToQuestionId?: string | null; onScrollToLines: (filePath: string, lineStart: number, lineEnd: number, side: 'old' | 'new') => void; onAskGeneral?: (question: string) => void; + /** Stop the in-flight answer. When provided, the input shows a Stop button while streaming. */ + onStop?: () => void; permissionRequests?: PendingPermission[]; onRespondToPermission?: (requestId: string, allow: boolean) => void; aiProviders?: AIProviderOption[]; @@ -48,6 +50,7 @@ export const AITab: React.FC = ({ scrollToQuestionId, onScrollToLines, onAskGeneral, + onStop, permissionRequests = [], onRespondToPermission, aiProviders = [], @@ -173,7 +176,7 @@ export const AITab: React.FC = ({ onReasoningEffortChange={(effort) => onAIConfigChange?.({ reasoningEffort: effort })} hasSession={hasAISession} /> - {onAskGeneral && } + {onAskGeneral && } ); } @@ -221,21 +224,6 @@ export const AITab: React.FC = ({ ); })} - {/* Pending permission requests */} - {permissionRequests.filter(p => !p.decided).map(perm => ( -
- {})} - /> -
- ))} - {/* General questions */} {generalMessages.length > 0 && (
@@ -256,6 +244,25 @@ export const AITab: React.FC = ({
+ {/* Pending permission requests — pinned just above the input/config bar + so the user sees them right where they act, not buried in the scroll. */} + {permissionRequests.filter(p => !p.decided).length > 0 && ( +
+ {permissionRequests.filter(p => !p.decided).map(perm => ( + {})} + /> + ))} +
+ )} + {/* Config bar */} void; onSubmit: () => void; disabled?: boolean; -}> = ({ value, onChange, onSubmit, disabled }) => { + isStreaming?: boolean; + onStop?: () => void; +}> = ({ value, onChange, onSubmit, disabled, isStreaming, onStop }) => { const textareaRef = useRef(null); const autoResize = useCallback(() => { @@ -312,16 +321,29 @@ const GeneralInput: React.FC<{ } }} /> - + {isStreaming && onStop ? ( + + ) : ( + + )} ); diff --git a/packages/review-editor/components/ReviewSidebar.tsx b/packages/review-editor/components/ReviewSidebar.tsx index 18cf28ab1..e1fb37e20 100644 --- a/packages/review-editor/components/ReviewSidebar.tsx +++ b/packages/review-editor/components/ReviewSidebar.tsx @@ -41,6 +41,7 @@ interface ReviewSidebarProps { aiMessages?: AIChatEntry[]; isAICreatingSession?: boolean; isAIStreaming?: boolean; + onAIStop?: () => void; onScrollToAILines?: (filePath: string, lineStart: number, lineEnd: number, side: 'old' | 'new') => void; activeFilePath?: string; scrollToQuestionId?: string | null; @@ -127,6 +128,7 @@ export const ReviewSidebar: React.FC = /* React.memo */({ aiMessages = [], isAICreatingSession = false, isAIStreaming = false, + onAIStop, onScrollToAILines, activeFilePath, scrollToQuestionId, @@ -394,6 +396,7 @@ export const ReviewSidebar: React.FC = /* React.memo */({ scrollToQuestionId={scrollToQuestionId} onScrollToLines={onScrollToAILines ?? (() => {})} onAskGeneral={onAskGeneral} + onStop={onAIStop} permissionRequests={aiPermissionRequests} onRespondToPermission={onRespondToPermission} aiProviders={aiProviders} diff --git a/packages/review-editor/hooks/useAIChat.ts b/packages/review-editor/hooks/useAIChat.ts index 91e4df9f4..f137898a3 100644 --- a/packages/review-editor/hooks/useAIChat.ts +++ b/packages/review-editor/hooks/useAIChat.ts @@ -1,21 +1,59 @@ -import { useAIChat as useSharedAIChat, type AIChatEntry, type PendingPermission } from '@plannotator/ui/hooks/useAIChat'; +import { useCallback, useRef } from 'react'; +import { + useAIChat as useSharedAIChat, + type AIChatEntry, + type AskAIParams, + type PendingPermission, +} from '@plannotator/ui/hooks/useAIChat'; export type { AIChatEntry, PendingPermission }; +interface Viewing { + scope: 'all' | 'file'; + filePath?: string; +} + interface UseAIChatOptions { patch: string; + /** VCS diff type so the agent can inspect changes with git instead of a paste. */ + diffType?: string; + /** Base branch/ref the diff is computed against. */ + base?: string | null; + /** What the user is currently viewing (read fresh on each question). */ + viewing?: Viewing; providerId?: string | null; model?: string | null; reasoningEffort?: string | null; } -export function useAIChat({ patch, providerId, model, reasoningEffort }: UseAIChatOptions) { - return useSharedAIChat({ +export function useAIChat({ + patch, + diffType, + base, + viewing, + providerId, + model, + reasoningEffort, +}: UseAIChatOptions) { + const chat = useSharedAIChat({ context: { mode: 'code-review', - review: { patch }, + review: { patch, diffType, base: base ?? undefined }, }, providerId, model, reasoningEffort, }); + + // View state changes mid-session; the session context is baked once. Read the + // latest view via a ref and attach it to every question. + const viewingRef = useRef(viewing); + viewingRef.current = viewing; + + const ask = useCallback( + (params: AskAIParams) => + chat.ask({ viewing: viewingRef.current, ...params }), + [chat], + ); + + return { ...chat, ask }; } diff --git a/packages/server/ai-runtime.ts b/packages/server/ai-runtime.ts index 63a8f1fcd..540e980d3 100644 --- a/packages/server/ai-runtime.ts +++ b/packages/server/ai-runtime.ts @@ -40,17 +40,18 @@ export async function createAIRuntime(options: CreateAIRuntimeOptions = {}): Pro } try { - await import("@plannotator/ai/providers/codex-sdk"); - await import("@openai/codex-sdk"); + await import("@plannotator/ai/providers/codex-app-server"); const codexPath = Bun.which("codex"); - const provider = await createProvider({ - type: "codex-sdk", - cwd, - ...(codexPath && { codexExecutablePath: codexPath }), - }); - registry.register(provider); + if (codexPath) { + const provider = await createProvider({ + type: "codex-sdk", + cwd, + ...(codexPath ? { codexExecutablePath: codexPath } : {}), + }); + registry.register(provider); + } } catch { - // Codex SDK not available. + // Codex not available. } try { diff --git a/packages/ui/components/ai/DocumentAIChatPanel.tsx b/packages/ui/components/ai/DocumentAIChatPanel.tsx index c988eb1c2..b43bf3d2d 100644 --- a/packages/ui/components/ai/DocumentAIChatPanel.tsx +++ b/packages/ui/components/ai/DocumentAIChatPanel.tsx @@ -12,6 +12,8 @@ interface DocumentAIChatPanelProps { isCreatingSession: boolean; isStreaming: boolean; onAskGeneral?: (question: string) => void; + /** Stop the in-flight answer. When provided, the input shows a Stop button while streaming. */ + onStop?: () => void; permissionRequests?: PendingPermission[]; onRespondToPermission?: (requestId: string, allow: boolean) => void; aiProviders?: AIProviderOption[]; @@ -56,6 +58,7 @@ export const DocumentAIChatPanel: React.FC = ({ isCreatingSession, isStreaming, onAskGeneral, + onStop, permissionRequests = [], onRespondToPermission, aiProviders = [], @@ -105,6 +108,16 @@ export const DocumentAIChatPanel: React.FC = ({ )} + {messages.map(entry => ( + + ))} + + + + {/* Pending permission requests — pinned just above the input/model bar so + the user sees them right where they act, not buried at the top. */} + {permissionRequests.filter(p => !p.decided).length > 0 && ( +
{permissionRequests.filter(p => !p.decided).map(permission => ( = ({ onRespond={onRespondToPermission ?? (() => {})} /> ))} - - {messages.map(entry => ( - - ))}
- + )} = ({ onChange={setGeneralInput} onSubmit={handleGeneralSubmit} disabled={isStreaming} + isStreaming={isStreaming} + onStop={onStop} /> )} @@ -241,7 +252,9 @@ const GeneralInput: React.FC<{ onChange: (value: string) => void; onSubmit: () => void; disabled?: boolean; -}> = ({ value, onChange, onSubmit, disabled }) => { + isStreaming?: boolean; + onStop?: () => void; +}> = ({ value, onChange, onSubmit, disabled, isStreaming, onStop }) => { const textareaRef = useRef(null); useEffect(() => { @@ -270,17 +283,30 @@ const GeneralInput: React.FC<{ } }} /> - + {isStreaming && onStop ? ( + + ) : ( + + )} ); diff --git a/packages/ui/hooks/useAIChat.ts b/packages/ui/hooks/useAIChat.ts index 6047743b2..2cd566107 100644 --- a/packages/ui/hooks/useAIChat.ts +++ b/packages/ui/hooks/useAIChat.ts @@ -36,6 +36,9 @@ export interface AskAIParams { selectedCode?: string; scope?: AIQuestion['scope']; contextUpdate?: string; + /** What the user is currently viewing (changes mid-session, so it rides with + * each question rather than the once-created session context). */ + viewing?: { scope: 'all' | 'file'; filePath?: string }; } interface UseAIChatOptions { @@ -70,6 +73,15 @@ export function buildDefaultPrompt(params: AskAIParams): string { return `${label}${source}${selection}\n\n${params.prompt}`; } + // General question (no explicit file/line/selection): tell the agent what the + // user is currently looking at so it can scope its own investigation. + if (params.viewing) { + const note = params.viewing.scope === 'file' && params.viewing.filePath + ? `[The user is currently viewing ${params.viewing.filePath}]` + : '[The user is currently viewing all changed files]'; + return `${note}\n${params.prompt}`; + } + return params.prompt; }