diff --git a/.github/scripts/install-backend-cli.mjs b/.github/scripts/install-backend-cli.mjs index b2594186..999c9b27 100644 --- a/.github/scripts/install-backend-cli.mjs +++ b/.github/scripts/install-backend-cli.mjs @@ -66,9 +66,19 @@ const backends = { }, }; +// `openai-agents` has no external CLI to install — it talks to a +// remote HTTP endpoint over the wire and the live-backend test stands +// up its own in-process dummy server. Short-circuit before the +// backends-table lookup so the CI matrix can include it without +// special-casing the workflow. +if (backend === "openai-agents") { + console.log("openai-agents needs no external CLI; skipping install."); + process.exit(0); +} + if (!backend || !Object.hasOwn(backends, backend)) { console.error( - `Usage: node .github/scripts/install-backend-cli.mjs <${Object.keys(backends).join("|")}>`, + `Usage: node .github/scripts/install-backend-cli.mjs <${Object.keys(backends).join("|")}|openai-agents>`, ); process.exit(2); } diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bcb0c0b2..ca53c82e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -213,7 +213,7 @@ jobs: fail-fast: false matrix: os: [ubuntu-latest, macos-latest, windows-latest] - backend: [kilo, opencode, claude, codex] + backend: [kilo, opencode, claude, codex, openai-agents] steps: - uses: actions/checkout@v6 diff --git a/package.json b/package.json index 067b3515..8fa3a98d 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "test:kilo:backend": "vitest run --reporter=verbose --reporter=json --outputFile=kilo-backend-results.json src/__tests__/integration/kilo-live-discovery.test.ts", "test:opencode:backend": "vitest run --reporter=verbose --reporter=json --outputFile=opencode-backend-results.json src/__tests__/integration/opencode-live-discovery.test.ts", "test:codex:backend": "vitest run --reporter=verbose --reporter=json --outputFile=codex-backend-results.json src/__tests__/integration/codex-live-discovery.test.ts", + "test:openai-agents:backend": "vitest run --reporter=verbose --reporter=json --outputFile=openai-agents-backend-results.json src/__tests__/integration/openai-agents-live-discovery.test.ts", "tarball:check": "node .github/scripts/tarball-check.mjs", "build:stub-sea": "node src/__tests__/integration/stub-claude/build-sea.mjs", "test:watch": "vitest", diff --git a/src/__tests__/fs-path.test.ts b/src/__tests__/fs-path.test.ts new file mode 100644 index 00000000..2fa162e7 --- /dev/null +++ b/src/__tests__/fs-path.test.ts @@ -0,0 +1,59 @@ +/** + * Tests for the shared `expandFsPath` helper. The single thing this + * has to get right is that `~/` becomes `$HOME/` — every + * model-supplied path that crosses into `fs.*` or `bot.api.send*` + * goes through this, and Node's `fs` module does NOT expand tildes + * itself. A regression here would resurface bugs like ENOENT on + * `~/.talon/workspace/robot.svg` from `send_file`. + */ +import { describe, it, expect } from "vitest"; +import { homedir } from "node:os"; +import { resolve, isAbsolute, sep } from "node:path"; +import { expandFsPath } from "../util/fs-path.js"; + +describe("expandFsPath", () => { + it("expands a bare ~ to the home directory", () => { + expect(expandFsPath("~")).toBe(homedir()); + }); + + it("expands ~/ to $HOME/", () => { + expect(expandFsPath("~/.talon/workspace/robot.svg")).toBe( + resolve(homedir(), ".talon/workspace/robot.svg"), + ); + }); + + it("returns absolute POSIX-style paths unchanged on POSIX, absolute Windows paths unchanged on Windows", () => { + // `path.isAbsolute` accepts `/foo` as absolute on both platforms + // (it's the POSIX shape), but `path.resolve` on Windows will + // prepend the current drive letter — so equality is only safe + // when we compare against an actually-absolute-on-this-platform + // input. Build one from the test's own resolved cwd. + const abs = resolve(process.cwd(), "abs-test-file"); + expect(expandFsPath(abs)).toBe(abs); + }); + + it("resolves relative paths against process.cwd()", () => { + const out = expandFsPath("relative/file.txt"); + expect(isAbsolute(out)).toBe(true); + // path.resolve normalises separators to the platform default + // (backslashes on Windows), so use the resolved comparison value + // rather than a hard-coded POSIX suffix. + expect(out).toBe(resolve(process.cwd(), "relative/file.txt")); + }); + + it("returns an empty string unchanged", () => { + expect(expandFsPath("")).toBe(""); + }); + + it("preserves the leading tilde on `~foo` (NOT a home-relative path)", () => { + // `~foo` is NOT a home-relative path (that would be `~/foo`) — + // it's a literal filename starting with a tilde. We must NOT + // expand it as if the user meant `~/foo`. Resolve as relative; + // the resulting absolute path ends with `~weird` on both + // POSIX (sep=`/`) and Windows (sep=`\`). If the expander had + // mistakenly treated `~weird` as home-relative the path would + // end with `~weird` directly (without a preceding separator). + const out = expandFsPath("~weird"); + expect(out.endsWith(`${sep}~weird`)).toBe(true); + }); +}); diff --git a/src/__tests__/integration/dummy-openai-server.ts b/src/__tests__/integration/dummy-openai-server.ts new file mode 100644 index 00000000..7537a30f --- /dev/null +++ b/src/__tests__/integration/dummy-openai-server.ts @@ -0,0 +1,257 @@ +/** + * Minimal OpenAI-compatible HTTP server for offline live-backend + * tests against the `openai-agents` backend. + * + * Implements just enough of the surface the SDK actually calls when + * talking to a chat-completions provider: + * + * - `GET /v1/models` — returns a synthetic catalog. The backend's + * `fetchEndpointModels()` reads this on init. + * - `POST /v1/chat/completions` (with `stream: true`) — returns a + * scripted Server-Sent-Events response containing assistant text + * and/or tool calls. The SDK parses these chunks into + * `RunItemStreamEvent`s the same way it does for real providers. + * + * The script is a list of one ScriptedResponse per *expected request* + * from the SDK. A turn that requires the model to call a tool and + * then respond emits two HTTP requests — one whose response asks for + * the tool, and a second (with the tool's output appended to the + * messages) whose response is the final assistant text. Tests + * arrange the script accordingly. + * + * No auth check, no rate limits, no streaming gymnastics — the goal + * is deterministic SSE bytes the SDK can parse, not provider + * fidelity. + */ +import { createServer, type Server } from "node:http"; +import { randomBytes } from "node:crypto"; +import type { AddressInfo } from "node:net"; + +export interface DummyModel { + id: string; + name?: string; + context_length?: number; + /** OpenRouter-style: `"0"` flags free-tier. */ + pricing?: { prompt: string; completion?: string }; +} + +export interface ToolCallSpec { + name: string; + /** Arguments — serialised to a JSON string by the server (matches OpenAI's API). */ + arguments: Record; + /** Optional callId; auto-generated when omitted. */ + id?: string; +} + +/** + * One response in the per-request script. `text` and `toolCalls` may + * both be present (an assistant chunk that also requests tools); when + * `toolCalls` is set, `finishReason` defaults to `"tool_calls"`. + */ +export interface ScriptedResponse { + text?: string; + toolCalls?: ToolCallSpec[]; + finishReason?: "stop" | "tool_calls"; + /** Optional model id to echo in the response (defaults to the request's). */ + model?: string; +} + +export interface RecordedRequest { + path: string; + method: string; + body: unknown; +} + +export interface DummyOpenAIServer { + url: string; + port: number; + close(): Promise; + /** Set the response sequence for upcoming chat-completions requests. Consumed in order. */ + setScript(responses: ScriptedResponse[]): void; + /** Empty the pending script (does NOT touch the recorded-request log). */ + clearScript(): void; + /** Inspect requests recorded since last `clearRequests()`. */ + getRequests(): RecordedRequest[]; + clearRequests(): void; +} + +export interface DummyServerOptions { + models?: DummyModel[]; +} + +const DEFAULT_MODELS: DummyModel[] = [ + { + id: "test/gpt-stub", + name: "GPT Stub", + context_length: 8192, + pricing: { prompt: "0" }, + }, +]; + +export async function startDummyOpenAIServer( + options: DummyServerOptions = {}, +): Promise { + const models = options.models ?? DEFAULT_MODELS; + let script: ScriptedResponse[] = []; + const requests: RecordedRequest[] = []; + + const server: Server = createServer(async (req, res) => { + const url = req.url ?? "/"; + let body = ""; + req.on("data", (chunk) => { + body += chunk; + }); + req.on("end", () => { + let parsed: unknown = undefined; + if (body) { + try { + parsed = JSON.parse(body); + } catch { + parsed = body; + } + } + requests.push({ path: url, method: req.method ?? "GET", body: parsed }); + + if (url.endsWith("/models") && req.method === "GET") { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ data: models })); + return; + } + + if (url.endsWith("/chat/completions") && req.method === "POST") { + const reqBody = parsed as { model?: string } | undefined; + const next = script.shift(); + if (!next) { + res.writeHead(500, { "Content-Type": "application/json" }); + res.end( + JSON.stringify({ + error: { + message: + "dummy server: no scripted response available for this request", + code: "no_script", + }, + }), + ); + return; + } + writeChatCompletionSSE( + res, + next, + reqBody?.model ?? models[0]?.id ?? "test/gpt-stub", + ); + return; + } + + // Unknown path — return 404 so the SDK surfaces a useful error. + res.writeHead(404, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: { message: `unknown path: ${url}` } })); + }); + }); + + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + + const port = (server.address() as AddressInfo).port; + const url = `http://127.0.0.1:${port}/v1`; + + return { + url, + port, + close: () => new Promise((resolve) => server.close(() => resolve())), + setScript: (responses) => { + script = [...responses]; + }, + clearScript: () => { + script = []; + }, + getRequests: () => requests.slice(), + clearRequests: () => { + requests.length = 0; + }, + }; +} + +/** + * Stream a single chat-completions response over Server-Sent Events. + * The format matches OpenAI's chat-completions streaming protocol so + * the official `openai` client (used by `@openai/agents` under the + * hood) parses it without modification. + */ +function writeChatCompletionSSE( + res: import("node:http").ServerResponse, + scripted: ScriptedResponse, + modelId: string, +): void { + res.writeHead(200, { + "Content-Type": "text/event-stream", + "Cache-Control": "no-cache", + Connection: "keep-alive", + }); + + const id = `chatcmpl-${randomBytes(8).toString("hex")}`; + const created = Math.floor(Date.now() / 1000); + const model = scripted.model ?? modelId; + const finish = + scripted.finishReason ?? + (scripted.toolCalls && scripted.toolCalls.length > 0 + ? "tool_calls" + : "stop"); + + const writeChunk = (delta: Record): void => { + res.write( + `data: ${JSON.stringify({ + id, + object: "chat.completion.chunk", + created, + model, + choices: [{ index: 0, delta, finish_reason: null }], + })}\n\n`, + ); + }; + + // Opening role chunk — mirrors what real providers emit first. + writeChunk({ role: "assistant", content: "" }); + + // Text content streamed as one chunk for determinism. Real providers + // chunk word-by-word; the SDK doesn't care. + if (scripted.text) { + writeChunk({ content: scripted.text }); + } + + // Tool calls. Each call gets a single chunk carrying name + the + // fully-serialised arguments (real providers stream arguments + // character-by-character; emitting the whole blob once is a valid + // shape the parser handles). + if (scripted.toolCalls && scripted.toolCalls.length > 0) { + const toolCalls = scripted.toolCalls.map((tc, i) => ({ + index: i, + id: tc.id ?? `call_${randomBytes(8).toString("hex")}`, + type: "function" as const, + function: { + name: tc.name, + arguments: JSON.stringify(tc.arguments), + }, + })); + writeChunk({ tool_calls: toolCalls }); + } + + // Final chunk with finish_reason — terminates the SSE stream from + // the SDK's perspective. + res.write( + `data: ${JSON.stringify({ + id, + object: "chat.completion.chunk", + created, + model, + choices: [{ index: 0, delta: {}, finish_reason: finish }], + usage: { + prompt_tokens: 10, + completion_tokens: scripted.text ? scripted.text.length : 0, + total_tokens: 10 + (scripted.text ? scripted.text.length : 0), + }, + })}\n\n`, + ); + res.write("data: [DONE]\n\n"); + res.end(); +} diff --git a/src/__tests__/integration/openai-agents-live-discovery.test.ts b/src/__tests__/integration/openai-agents-live-discovery.test.ts new file mode 100644 index 00000000..3157f75d --- /dev/null +++ b/src/__tests__/integration/openai-agents-live-discovery.test.ts @@ -0,0 +1,394 @@ +/** + * Live integration test for the `openai-agents` backend. + * + * Unlike kilo / opencode / claude / codex — which spawn a real + * upstream CLI or HTTP daemon — the openai-agents backend talks + * to any OpenAI-compatible HTTP endpoint over the wire. That makes + * the natural live target a dummy OpenAI server we drive ourselves + * (see `./dummy-openai-server.ts`), not a third-party CLI. + * + * The test boots Talon's actual openai-agents code path through the + * production `handleMessage` entry — including model resolution, + * `MemorySession` wiring, MCP server construction, agent run loop, + * and delivery routing — pointed at the dummy server. The dummy + * returns scripted SSE responses so we can assert deterministic + * behaviour around: + * + * 1. Endpoint catalog enrichment via `GET /models`. + * 2. Plain text replies via trailing prose delivery. + * 3. Tool calls (model invokes a registered tool, sees the result, + * then produces the final reply). + * 4. Multi-turn memory — the SDK's `MemorySession` should carry + * conversation state across `handleMessage` calls so the second + * turn sees the first turn's inputs and outputs. + * 5. `resetChat()` clearing the session so the next turn starts + * fresh. + * + * The bot is bootstrapped with `frontend: "terminal"` so no Telegram + * MCP servers spawn — keeps the test self-contained and fast. + */ +import { describe, it, expect, beforeAll, afterAll, beforeEach } from "vitest"; + +import { Gateway } from "../../core/gateway.js"; +import { resetSession } from "../../storage/sessions.js"; +import type { TalonConfig } from "../../util/config.js"; + +import { + initOpenAIAgentsAgent, + fetchEndpointModels, +} from "../../backend/openai-agents/init.js"; +import { handleMessage } from "../../backend/openai-agents/handler.js"; +import { + resetState, + clearChatSession, + getState, +} from "../../backend/openai-agents/state.js"; +import { resolveModel } from "../../backend/openai-agents/models.js"; + +import { + startDummyOpenAIServer, + type DummyOpenAIServer, +} from "./dummy-openai-server.js"; + +let server: DummyOpenAIServer; + +beforeAll(async () => { + server = await startDummyOpenAIServer({ + models: [ + { + id: "test/gpt-stub", + name: "GPT Stub", + context_length: 8192, + pricing: { prompt: "0", completion: "0" }, + }, + { + id: "test/gpt-stub-paid", + name: "GPT Stub (paid)", + context_length: 32_000, + pricing: { prompt: "0.001", completion: "0.002" }, + }, + ], + }); +}); + +afterAll(async () => { + await server?.close(); +}); + +function bootBackend(extra: Partial = {}): Gateway { + resetState(); + const gateway = new Gateway(); + const config: TalonConfig = { + frontend: "terminal", + backend: "openai-agents", + model: "test/gpt-stub", + openaiApiKey: "sk-test-fake", + openaiBaseUrl: server.url, + openaiApiMode: "chat_completions", + workspace: "/tmp/talon-test", + botToken: "", + adminUserId: 0, + maxMessageLength: 4000, + concurrency: 1, + pulse: false, + pulseIntervalMs: 300000, + ...extra, + } as TalonConfig; + initOpenAIAgentsAgent(config, () => gateway.getPort(), "terminal"); + return gateway; +} + +beforeEach(() => { + server.clearScript(); + // Reset Talon's session bookkeeping so each test starts at turn 0. + resetSession("test-chat"); + clearChatSession("test-chat"); +}); + +// ── 1. Endpoint catalog enrichment ───────────────────────────────────────── + +describe("openai-agents live (dummy) / endpoint discovery", () => { + it("populates the catalog from GET /models on init", async () => { + bootBackend(); + // `fetchEndpointModels` is fire-and-forget in production; await it + // directly so the test isn't racing the network call. + await fetchEndpointModels(server.url, "sk-test-fake"); + + const cat = getState().endpointModels; + expect(cat.get("test/gpt-stub")?.contextWindow).toBe(8192); + expect(cat.get("test/gpt-stub")?.free).toBe(true); + expect(cat.get("test/gpt-stub-paid")?.contextWindow).toBe(32_000); + expect(cat.get("test/gpt-stub-paid")?.free).toBeUndefined(); + }); + + it("makes resolveModel return enriched info for advertised ids", async () => { + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + const r = resolveModel("test/gpt-stub"); + expect(r.kind).toBe("exact"); + if (r.kind !== "exact") return; + expect(r.model.contextWindow).toBe(8192); + expect(r.model.free).toBe(true); + }); +}); + +// ── 2. Plain text turn ───────────────────────────────────────────────────── + +describe("openai-agents live (dummy) / plain text turn", () => { + it("captures the model's response text in the result but does NOT deliver it as a fallback", async () => { + // Strict tool-only delivery: trailing prose is private scratchpad + // and NEVER reaches the frontend. The model's text is still + // recorded in `result.text` for tracing, but `onTextBlock` is + // never called for trailing prose. To actually reach the user, + // models must call a delivery tool (`end_turn` / `send` / + // `react`); those aren't registered for `frontend: "terminal"` + // (the test bootstrap), so this configuration is effectively + // delivery-tool-less and the prose is silently dropped on + // purpose. + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + server.setScript([{ text: "Hello back!", finishReason: "stop" }]); + + const blocks: string[] = []; + const result = await handleMessage({ + chatId: "test-chat", + text: "Hi there", + senderName: "Tester", + isGroup: false, + messageId: 1, + onTextBlock: async (b: string) => { + blocks.push(b); + }, + }); + + expect(result.text).toContain("Hello back"); + expect(blocks).toEqual([]); + + const reqs = server + .getRequests() + .filter((r) => r.path.endsWith("/chat/completions")); + expect(reqs).toHaveLength(1); + const body = reqs[0].body as { messages: Array<{ role: string }> }; + expect(body.messages.some((m) => m.role === "user")).toBe(true); + }); +}); + +// ── 3. Tool call dispatch ────────────────────────────────────────────────── + +describe("openai-agents live (dummy) / tool call dispatch", () => { + it("invokes a builtin tool when the model asks for it, then resolves with the final reply", async () => { + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + + // Two requests: the first one returns a Bash tool call (which the + // backend executes locally via the openai-agents builtin), the + // second returns the final assistant text containing the tool's + // output. + server.setScript([ + { + toolCalls: [ + { + name: "Bash", + arguments: { + command: "echo dummy-tool-output", + description: null, + timeout_ms: null, + }, + }, + ], + finishReason: "tool_calls", + }, + { + text: "Tool returned: dummy-tool-output", + finishReason: "stop", + }, + ]); + + const tools: string[] = []; + const result = await handleMessage({ + chatId: "test-chat", + text: "run that command", + senderName: "Tester", + isGroup: false, + messageId: 2, + onToolUse: (name) => tools.push(name), + }); + + expect(tools).toContain("Bash"); + expect(result.text).toContain("dummy-tool-output"); + + const reqs = server + .getRequests() + .filter((r) => r.path.endsWith("/chat/completions")); + // At least two round-trips: initial tool request + follow-up + // with the tool result. The SDK may emit additional helper + // round-trips depending on the model's run policy; we only + // care that *some* follow-up carries the tool result. + expect(reqs.length).toBeGreaterThanOrEqual(2); + const allMessages = reqs.flatMap( + (r) => + ( + r.body as { + messages: Array<{ + role: string; + tool_call_id?: string; + content?: string; + }>; + } + ).messages, + ); + const toolMsg = allMessages.find((m) => m.role === "tool"); + expect(toolMsg).toBeDefined(); + expect(toolMsg?.content ?? "").toContain("dummy-tool-output"); + }); +}); + +// ── 4. Multi-turn memory via MemorySession ───────────────────────────────── + +describe("openai-agents live (dummy) / multi-turn memory", () => { + it("carries prior turn into the second turn's request payload", async () => { + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + + server.setScript([ + { text: "First reply", finishReason: "stop" }, + { text: "Second reply", finishReason: "stop" }, + ]); + + await handleMessage({ + chatId: "test-chat", + text: "first prompt", + senderName: "Tester", + isGroup: false, + messageId: 10, + }); + server.clearRequests(); + + await handleMessage({ + chatId: "test-chat", + text: "second prompt", + senderName: "Tester", + isGroup: false, + messageId: 11, + }); + + const reqs = server + .getRequests() + .filter((r) => r.path.endsWith("/chat/completions")); + expect(reqs).toHaveLength(1); + const body = reqs[1] ?? reqs[0]; + const messages = ( + body.body as { messages: Array<{ role: string; content?: unknown }> } + ).messages; + + // Turn 2's request should include turn 1's user prompt + assistant + // reply in its messages array — that's how MemorySession threads + // history into subsequent run() calls. + const userTexts = messages + .filter((m) => m.role === "user") + .map((m) => + typeof m.content === "string" ? m.content : JSON.stringify(m.content), + ); + const assistantTexts = messages + .filter((m) => m.role === "assistant") + .map((m) => + typeof m.content === "string" ? m.content : JSON.stringify(m.content), + ); + + expect(userTexts.some((t) => t.includes("first prompt"))).toBe(true); + expect(assistantTexts.some((t) => t.includes("First reply"))).toBe(true); + expect(userTexts.some((t) => t.includes("second prompt"))).toBe(true); + }); + + it("does not leak memory across chats", async () => { + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + + server.setScript([ + { text: "Reply to chat A", finishReason: "stop" }, + { text: "Reply to chat B", finishReason: "stop" }, + ]); + + await handleMessage({ + chatId: "chat-A", + text: "Hi from A", + senderName: "Tester", + isGroup: false, + messageId: 1, + }); + server.clearRequests(); + + await handleMessage({ + chatId: "chat-B", + text: "Hi from B", + senderName: "Tester", + isGroup: false, + messageId: 2, + }); + + const reqs = server + .getRequests() + .filter((r) => r.path.endsWith("/chat/completions")); + const lastBody = reqs[reqs.length - 1].body as { + messages: Array<{ role: string; content?: unknown }>; + }; + const userTexts = lastBody.messages + .filter((m) => m.role === "user") + .map((m) => + typeof m.content === "string" ? m.content : JSON.stringify(m.content), + ); + // Chat B's request must NOT contain chat A's prompt. + expect(userTexts.some((t) => t.includes("Hi from B"))).toBe(true); + expect(userTexts.some((t) => t.includes("Hi from A"))).toBe(false); + }); +}); + +// ── 5. resetChat clears the session ──────────────────────────────────────── + +describe("openai-agents live (dummy) / resetChat", () => { + it("wipes the MemorySession so the next turn starts blank", async () => { + bootBackend(); + await fetchEndpointModels(server.url, "sk-test-fake"); + + server.setScript([ + { text: "Reply 1", finishReason: "stop" }, + { text: "Reply 2", finishReason: "stop" }, + ]); + + await handleMessage({ + chatId: "test-chat", + text: "remember me", + senderName: "Tester", + isGroup: false, + messageId: 1, + }); + + // Manually invoke the backend's resetChat (the same call /reset + // makes in production). + clearChatSession("test-chat"); + server.clearRequests(); + + await handleMessage({ + chatId: "test-chat", + text: "fresh start", + senderName: "Tester", + isGroup: false, + messageId: 2, + }); + + const reqs = server + .getRequests() + .filter((r) => r.path.endsWith("/chat/completions")); + expect(reqs).toHaveLength(1); + const messages = ( + reqs[0].body as { messages: Array<{ role: string; content?: unknown }> } + ).messages; + const allText = messages + .map((m) => + typeof m.content === "string" ? m.content : JSON.stringify(m.content), + ) + .join("\n"); + expect(allText).not.toContain("remember me"); + expect(allText).toContain("fresh start"); + }); +}); diff --git a/src/__tests__/openai-agents-backend.test.ts b/src/__tests__/openai-agents-backend.test.ts index fb697b03..15fbed52 100644 --- a/src/__tests__/openai-agents-backend.test.ts +++ b/src/__tests__/openai-agents-backend.test.ts @@ -35,9 +35,8 @@ describe("openai-agents / constants", () => { expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain("end_turn"); expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain("send"); expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain("react"); - expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain( - "OpenAI Agents Delivery", - ); + expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain("tool-only delivery"); + expect(OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX).toContain("FLOW VIOLATION"); }); }); diff --git a/src/__tests__/openai-agents-enrichment.test.ts b/src/__tests__/openai-agents-enrichment.test.ts index 59995223..31c084af 100644 --- a/src/__tests__/openai-agents-enrichment.test.ts +++ b/src/__tests__/openai-agents-enrichment.test.ts @@ -131,10 +131,11 @@ describe("openai-agents / fetchEndpointModels / response shapes", () => { expect(catalog.get("Qwen/Qwen-7B")?.contextWindow).toBe(32_768); }); - it("skips entries with neither context_length nor name nor pricing", async () => { - // OpenAI's /v1/models returns just `{id, object, created, owned_by}`. - // There's nothing to enrich, so the entry isn't useful and is - // dropped to keep /status from rendering a meaningless row. + it("still records bare entries (id only, no enrichment fields)", async () => { + // OpenAI's /v1/models — and NVIDIA NIM's — returns just + // `{id, object, created, owned_by}`. The picker still needs these + // ids to list them; we just store them with empty caps so the UI + // renders an id-only entry without context-window / pricing badges. stubFetchWith(() => ({ status: 200, body: { @@ -146,7 +147,10 @@ describe("openai-agents / fetchEndpointModels / response shapes", () => { })); await fetchEndpointModels("https://api.openai.com/v1", "sk-test"); - expect(getState().endpointModels.size).toBe(0); + const catalog = getState().endpointModels; + expect(catalog.size).toBe(2); + expect(catalog.get("gpt-4o")).toEqual({}); + expect(catalog.get("gpt-4o-mini")).toEqual({}); }); it("includes entries that have a display name even without context_length", async () => { diff --git a/src/__tests__/openai-agents-models.test.ts b/src/__tests__/openai-agents-models.test.ts index 4acc24cd..504ff016 100644 --- a/src/__tests__/openai-agents-models.test.ts +++ b/src/__tests__/openai-agents-models.test.ts @@ -362,6 +362,114 @@ describe("openai-agents / providers", () => { }); }); +// ── Provider grouping with flat ids ──────────────────────────────────────── +// +// Sparse endpoints like Zen and OpenAI itself return flat model ids +// without a `vendor/` prefix. Bucketing those by string-before-slash +// dumps everything into a single bin and makes the picker useless. +// The provider-inference table must recognise the family prefix. + +describe("openai-agents / picker / flat-id provider inference", () => { + it("groups Zen-style flat ids by inferred provider", () => { + const entries: Array<[string, EndpointModelCapabilities]> = []; + // Zen catalog sample — 8 anthropic, 8 openai, 2 google, 1 meta, + // 1 nvidia, etc. → 31 entries, exceeds the 30-entry grouping + // threshold, so the picker should produce provider chips. + for (const id of [ + "claude-opus-4-7", + "claude-opus-4-6", + "claude-opus-4-5", + "claude-opus-4-1", + "claude-sonnet-4-6", + "claude-sonnet-4-5", + "claude-sonnet-4", + "claude-haiku-4-5", + "gpt-5.5", + "gpt-5.5-pro", + "gpt-5.4", + "gpt-5-codex", + "gpt-5-nano", + "o1", + "o3-mini", + "o4", + "gemini-3.1-pro", + "gemini-3-flash", + "gemma-3-27b", + "llama-3.3-70b", + "codellama-70b", + "phi-4-mini", + "deepseek-v4-flash-free", + "deepseek-v4-pro", + "qwen3.6-plus", + "qwen3.5-plus", + "kimi-k2.6", + "kimi-k2.5", + "glm-5.1", + "glm-5", + "minimax-m2.7", + "minimax-m2.5", + "nemotron-3-super-free", + "mistral-large-3", + "grok-4", + "big-pickle", + ]) { + entries.push([id, {}]); + } + seedCatalog(entries); + const pres = getSettingsPresentation("(none)"); + expect(pres.view).toBe("groups"); + const labels = pres.modelButtons.map((b) => b.text); + // Should NOT lump them all into a single bucket. + expect(labels.length).toBeGreaterThan(5); + // Spot-check the known families show up with sensible names. + const joined = labels.join(" | "); + expect(joined).toMatch(/Anthropic/); + expect(joined).toMatch(/OpenAI/); + expect(joined).toMatch(/Google/); + expect(joined).toMatch(/NVIDIA|Nvidia/); + expect(joined).toMatch(/DeepSeek/); + expect(joined).toMatch(/MiniMax|Minimax/); + }); + + it("drilling into a provider returns only that family's models", () => { + const entries: Array<[string, EndpointModelCapabilities]> = [ + ["claude-opus-4-7", {}], + ["claude-sonnet-4", {}], + ["gpt-5.5", {}], + ["gemini-3-flash", {}], + ]; + seedCatalog(entries); + const pres = getSettingsPresentation("(none)", { provider: "anthropic" }); + expect(pres.view).toBe("models"); + expect(pres.modelButtons.map((b) => b.text)).toEqual( + expect.arrayContaining([expect.stringMatching(/claude-opus/)]), + ); + for (const b of pres.modelButtons) { + expect(b.text).toMatch(/claude/); + } + }); + + it("vendor/model ids still resolve via the slash prefix", () => { + const entries: Array<[string, EndpointModelCapabilities]> = []; + for (let i = 0; i < 35; i++) entries.push([`anthropic/c-${i}`, {}]); + seedCatalog(entries); + const pres = getSettingsPresentation("(none)"); + expect(pres.view).toBe("groups"); + expect(pres.modelButtons.map((b) => b.text)).toEqual([ + expect.stringMatching(/^Anthropic \(35\)$/), + ]); + }); + + it("unknown flat ids fall into an 'Other' bucket — not 'OpenAI'", () => { + const entries: Array<[string, EndpointModelCapabilities]> = []; + for (let i = 0; i < 35; i++) entries.push([`whatever-${i}`, {}]); + seedCatalog(entries); + const pres = getSettingsPresentation("(none)"); + expect(pres.view).toBe("groups"); + expect(pres.modelButtons[0].text).toMatch(/^Other/); + }); +}); + // ── formatModelError ─────────────────────────────────────────────────────── describe("openai-agents / formatModelError", () => { diff --git a/src/__tests__/openai-agents-session.test.ts b/src/__tests__/openai-agents-session.test.ts new file mode 100644 index 00000000..a0a56b42 --- /dev/null +++ b/src/__tests__/openai-agents-session.test.ts @@ -0,0 +1,290 @@ +/** + * TalonSession — SDK-session wrapper with replay-time transforms + + * bounded storage. + * + * Tests focus on the value-add behaviour, not on re-verifying the + * SDK's own session contract: + * + * - The transform pipeline is invoked in order and on every replay. + * - The default media-stripper removes image/file payloads from + * function_call_result outputs (single + array forms) and from + * assistant/user messages. + * - Plain text items are never modified. + * - Custom transforms can be plugged in and observed. + * - The capacity policy evicts oldest items and never orphans a + * function_call from its result. + * - `clearSession()` keeps the SDK's reset semantics intact. + * - `computeEvictionBoundary` is correct in isolation (unit-tested + * directly so the eviction invariant is provable without the SDK). + */ + +import { describe, it, expect } from "vitest"; +import { + TalonSession, + MediaStripperTransform, + computeEvictionBoundary, + type SessionItemTransform, +} from "../backend/openai-agents/session.js"; +import type { AgentInputItem } from "@openai/agents"; + +// ── Fixtures ────────────────────────────────────────────────────────────── + +function userMsg(text: string): AgentInputItem { + return { + role: "user", + content: [{ type: "input_text", text }], + } as AgentInputItem; +} + +function userMsgWithImage(): AgentInputItem { + return { + role: "user", + content: [ + { type: "input_text", text: "look" }, + { type: "input_image", image: "A".repeat(100) }, + ], + } as AgentInputItem; +} + +function imageResult(callId: string): AgentInputItem { + return { + type: "function_call_result", + name: "browser_take_screenshot", + callId, + status: "completed", + output: { + type: "image", + image: { data: "A".repeat(1000), mediaType: "image/png" }, + }, + } as unknown as AgentInputItem; +} + +function textResult(callId: string, text: string): AgentInputItem { + return { + type: "function_call_result", + name: "Bash", + callId, + status: "completed", + output: text, + } as unknown as AgentInputItem; +} + +function functionCall( + callId: string, + name = "browser_take_screenshot", +): AgentInputItem { + return { + type: "function_call", + name, + callId, + status: "completed", + arguments: "{}", + } as unknown as AgentInputItem; +} + +// ── MediaStripperTransform ──────────────────────────────────────────────── + +describe("MediaStripperTransform", () => { + const stripper = new MediaStripperTransform(); + + it("returns plain text items unchanged (same reference)", () => { + const item = userMsg("hello"); + expect(stripper.apply(item)).toBe(item); + }); + + it("strips a single image content block in function_call_result.output", () => { + const item = imageResult("c1"); + const out = stripper.apply(item) as { + output: { type: string; text: string }; + }; + expect(out).not.toBe(item); + expect(out.output.type).toBe("text"); + expect(out.output.text).toContain("media omitted"); + }); + + it("strips image content blocks inside an array output", () => { + const item = { + type: "function_call_result", + name: "tool", + callId: "x", + status: "completed", + output: [ + { type: "input_text", text: "details" }, + { type: "input_image", image: "A".repeat(500) }, + ], + } as unknown as AgentInputItem; + const out = stripper.apply(item) as { + output: Array<{ type: string; text?: string }>; + }; + expect(out.output).toHaveLength(2); + expect(out.output[0]).toEqual({ type: "input_text", text: "details" }); + expect(out.output[1].type).toBe("text"); + expect(out.output[1].text).toContain("media omitted"); + }); + + it("leaves text-only function_call_result intact", () => { + const item = textResult("c1", "command output"); + expect(stripper.apply(item)).toBe(item); + }); + + it("strips embedded images in user messages", () => { + const item = userMsgWithImage(); + const out = stripper.apply(item) as { + content: Array<{ type: string; text?: string }>; + }; + expect(out).not.toBe(item); + expect(out.content[1].type).toBe("text"); + expect(out.content[1].text).toContain("media omitted"); + }); +}); + +// ── Transform pipeline composition ──────────────────────────────────────── + +describe("TalonSession / transform pipeline", () => { + it("applies transforms in registration order on every replay", () => { + const log: string[] = []; + const tagger = (tag: string): SessionItemTransform => ({ + name: `tag-${tag}`, + apply: (item) => { + log.push(tag); + return item; + }, + }); + const session = new TalonSession({ + transforms: [tagger("a"), tagger("b"), tagger("c")], + }); + const item = userMsg("x"); + session.prepareHistoryItemForModelInput(item); + expect(log).toEqual(["a", "b", "c"]); + }); + + it("threads each transform's output into the next", () => { + const rename: SessionItemTransform = { + name: "rename", + apply: (item) => { + const msg = item as { role?: string; content?: unknown }; + if (msg.role === "user" && Array.isArray(msg.content)) { + return { + ...item, + content: ( + msg.content as Array<{ type: string; text?: string }> + ).map((b) => + b.type === "input_text" + ? { ...b, text: (b.text ?? "").toUpperCase() } + : b, + ), + } as AgentInputItem; + } + return item; + }, + }; + const session = new TalonSession({ transforms: [rename] }); + const out = session.prepareHistoryItemForModelInput(userMsg("hello")) as { + content: Array<{ text: string }>; + }; + expect(out.content[0].text).toBe("HELLO"); + }); + + it("default transforms include media stripping", async () => { + const session = new TalonSession({ sessionId: "c1" }); + const item = imageResult("c1"); + await session.addItems([item]); + const replayed = await session.getItems(); + // getItems returns the SDK-stored (cloned) item; transforms only apply + // on replay. Verify that prepareHistoryItemForModelInput strips media. + const out = session.prepareHistoryItemForModelInput(replayed[0]) as { + output: { type: string; text?: string }; + }; + expect(out.output.type).toBe("text"); + expect(out.output.text).toContain("media omitted"); + }); +}); + +// ── computeEvictionBoundary ─────────────────────────────────────────────── + +describe("computeEvictionBoundary", () => { + it("returns 0 when items fit under the cap", () => { + expect(computeEvictionBoundary([userMsg("a"), userMsg("b")], 10)).toBe(0); + }); + + it("returns the simple boundary when no pair is split", () => { + const items = [userMsg("a"), userMsg("b"), userMsg("c"), userMsg("d")]; + expect(computeEvictionBoundary(items, 2)).toBe(2); + }); + + it("extends the boundary by one when it would split a call from its result", () => { + const items = [ + userMsg("a"), + functionCall("call-1"), + textResult("call-1", "ok"), + userMsg("b"), + ]; + // Cap = 2 → naïve drop = 2, leaving [textResult, userMsg]; that + // orphans the result. Boundary should extend to drop the result too. + expect(computeEvictionBoundary(items, 2)).toBe(3); + }); + + it("does NOT extend when dropping a call and its matching result together", () => { + const items = [ + functionCall("call-1"), + textResult("call-1", "ok"), + userMsg("b"), + userMsg("c"), + ]; + // Cap = 2 → drop 2 → drops both call + result → safe. + expect(computeEvictionBoundary(items, 2)).toBe(2); + }); +}); + +// ── End-to-end cap behaviour ────────────────────────────────────────────── + +describe("TalonSession / capacity", () => { + it("evicts older items once cap is exceeded", async () => { + const session = new TalonSession({ maxItems: 5 }); + for (let i = 0; i < 10; i++) { + await session.addItems([userMsg(`m${i}`)]); + } + const items = await session.getItems(); + expect(items.length).toBeLessThanOrEqual(5); + // Newest items preserved at the tail. + const last = items[items.length - 1] as { + content: Array<{ text: string }>; + }; + expect(last.content[0].text).toBe("m9"); + }); + + it("never orphans a function_call across the eviction line", async () => { + const session = new TalonSession({ maxItems: 50 }); + for (let i = 0; i < 100; i++) { + await session.addItems([ + functionCall(`call-${i}`), + textResult(`call-${i}`, `out ${i}`), + ]); + } + const items = await session.getItems(); + const callIds = new Set( + items + .filter((i) => (i as { type?: string }).type === "function_call") + .map((c) => (c as { callId: string }).callId), + ); + const resultIds = new Set( + items + .filter((i) => (i as { type?: string }).type === "function_call_result") + .map((r) => (r as { callId: string }).callId), + ); + for (const id of callIds) { + expect(resultIds.has(id)).toBe(true); + } + }); +}); + +// ── Lifecycle ───────────────────────────────────────────────────────────── + +describe("TalonSession / lifecycle", () => { + it("clearSession empties the underlying store", async () => { + const session = new TalonSession({}); + await session.addItems([userMsg("a"), userMsg("b")]); + await session.clearSession(); + expect(await session.getItems()).toEqual([]); + }); +}); diff --git a/src/__tests__/package.functional.test.ts b/src/__tests__/package.functional.test.ts index 6433ee7f..1f25de7f 100644 --- a/src/__tests__/package.functional.test.ts +++ b/src/__tests__/package.functional.test.ts @@ -5,11 +5,12 @@ import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; const REPO_ROOT = resolve(import.meta.dirname, "../.."); -// 4 minutes — Windows runners regularly take 3+ minutes for `npm install` on -// the published tarball (cold cache + Windows fs latency); 3min was right at -// the edge. Bumped from 180k after a post-merge timeout on `main` -// (run 25603848100, 2026-05-09). -const FUNCTIONAL_TIMEOUT_MS = 240_000; +// 8 minutes — Windows runners regularly take 4+ minutes for `npm install` +// on the published tarball (cold cache + Windows fs latency); 240s was at +// the cliff (multiple 256–258s runs killing it). Bumped from 240k after +// repeated Windows-only flakes on PR #208 (runs 26037452792, 26038493696, +// 26038706000). +const FUNCTIONAL_TIMEOUT_MS = 480_000; const NPM_CLI = process.env.npm_execpath; type RunResult = { diff --git a/src/__tests__/triggers-extended.test.ts b/src/__tests__/triggers-extended.test.ts index e65ecc9b..5a4ee8e7 100644 --- a/src/__tests__/triggers-extended.test.ts +++ b/src/__tests__/triggers-extended.test.ts @@ -13,6 +13,7 @@ import { afterAll, + afterEach, beforeAll, beforeEach, describe, @@ -153,20 +154,34 @@ beforeEach(() => { initTriggers({ execute: executeSpy as never }); }); +// Kill any child still in the `children` map at the end of each test +// so a slow-exiting / timed-out python on a CI Windows runner can't +// leak into the next test's getRunningCount() reading. The trigger +// store reset in `beforeEach` only clears stored Trigger records — it +// has no awareness of in-process ChildProcess handles. +afterEach(async () => { + await shutdownTriggers(); +}); + // ── Language paths ──────────────────────────────────────────────────────── describe("triggers — alternate languages", () => { + // Per-test timeouts are 15s instead of the vitest default (5s) + // because launching a fresh `python` / `node` interpreter on a CI + // Windows runner regularly burns 3-5 seconds just on process + // startup before the user script even runs. The 5s default was + // tight enough to flake repeatedly. it("spawns a python trigger and fires on exit 0", async () => { const t = makeTrigger({ body: 'print("py done")\n', language: "python", }); spawnTrigger(t); - await waitForStatus(t.id, (s) => s === "fired"); + await waitForStatus(t.id, (s) => s === "fired", 12_000); expect(getTrigger(t.id)!.exitCode).toBe(0); const call = executeSpy.mock.calls[0][0]; expect(call.prompt).toMatch(/Status: fired/); - }); + }, 15_000); it("spawns a node trigger and fires on exit 0", async () => { const t = makeTrigger({ @@ -174,11 +189,11 @@ describe("triggers — alternate languages", () => { language: "node", }); spawnTrigger(t); - await waitForStatus(t.id, (s) => s === "fired"); + await waitForStatus(t.id, (s) => s === "fired", 12_000); expect(getTrigger(t.id)!.exitCode).toBe(0); const call = executeSpy.mock.calls[0][0]; expect(call.prompt).toMatch(/Status: fired/); - }); + }, 15_000); }); // ── Idempotency ─────────────────────────────────────────────────────────── diff --git a/src/backend/openai-agents/builtins.ts b/src/backend/openai-agents/builtins.ts index 0f345d65..7b77be0b 100644 --- a/src/backend/openai-agents/builtins.ts +++ b/src/backend/openai-agents/builtins.ts @@ -6,62 +6,74 @@ * doesn't — it's model-agnostic and assumes the host wires whatever * capabilities the agent needs. To keep Talon's system prompt and * behavior consistent across backends, this module mirrors that - * Claude-Code surface as plain `tool()` definitions. + * Claude-Code surface as `tool()` definitions. * - * Tool names + parameter shapes match Claude Code exactly so the - * shared prompt vocabulary ("read X", "write to ~/.talon/...") works - * uniformly. Outputs mirror Claude Code's text format where useful - * (e.g. `cat -n`-style line numbers for `Read`). + * Schema choice + * ───────────── * - * Safety posture: same as the Claude SDK backend — full host access. - * Talon already trusts the active model; sandboxing is a future - * concern handled at the runtime layer, not here. + * `@openai/agents`'s `tool()` factory accepts either a Zod schema or + * a raw JSON Schema. Zod is convenient but `@openai/agents` forces + * `strict: true` for Zod schemas, which in turn forces every + * declared property into the `required` array. That's OpenAI-correct + * but does NOT survive in the real world: many models (especially + * non-OpenAI ones routed through chat_completions) drop optional + * fields when calling, and the SDK then rejects every call as + * "Invalid JSON input". That manifested as "Bash tool is completely + * broken — JSON input errors on every call" in production with + * OpenRouter models like Trinity / Owl. + * + * We use plain JSON Schemas with explicit `required` arrays so + * truly-optional fields (offset/limit, timeout, provider, …) can be + * omitted by the model and the call still validates. `strict: false` + * disables the all-required pass. + * + * Tool names + parameter shapes still match Claude Code so the + * shared prompt vocabulary applies uniformly. */ import { tool } from "@openai/agents"; -import { z } from "zod"; import { spawn } from "node:child_process"; import { readFile, writeFile, mkdir, glob } from "node:fs/promises"; -import { dirname, isAbsolute, resolve as resolvePath } from "node:path"; -import { homedir } from "node:os"; +import { dirname, resolve as resolvePath } from "node:path"; +import { expandFsPath as expandPath } from "../../util/fs-path.js"; -// ── Path resolution ───────────────────────────────────────────────────────── -// -// Tool inputs may be absolute, ~/-prefixed, or relative. Normalize to -// absolute paths so behavior doesn't depend on Talon's cwd. +// ── Read ──────────────────────────────────────────────────────────────────── -function expandPath(input: string): string { - if (input.startsWith("~/")) return resolvePath(homedir(), input.slice(2)); - if (input === "~") return homedir(); - if (isAbsolute(input)) return input; - return resolvePath(process.cwd(), input); +interface ReadInput { + file_path: string; + offset?: number; + limit?: number; } -// ── Read ──────────────────────────────────────────────────────────────────── - const readTool = tool({ name: "Read", description: "Read a text file from disk. Returns the file contents with " + "`cat -n`-style line numbering. `offset` (1-indexed) skips the " + "first N-1 lines; `limit` caps the number of lines returned.", - parameters: z.object({ - file_path: z - .string() - .describe("Absolute path (or ~/...) to the file to read."), - offset: z - .number() - .int() - .min(1) - .nullable() - .describe("1-indexed line number to start reading from."), - limit: z - .number() - .int() - .min(1) - .nullable() - .describe("Maximum number of lines to read."), - }), - async execute({ file_path, offset, limit }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["file_path"], + properties: { + file_path: { + type: "string", + description: "Absolute path (or ~/...) to the file to read.", + }, + offset: { + type: "integer", + minimum: 1, + description: "1-indexed line number to start reading from.", + }, + limit: { + type: "integer", + minimum: 1, + description: "Maximum number of lines to read.", + }, + }, + }, + async execute(input) { + const { file_path, offset, limit } = input as ReadInput; const abs = expandPath(file_path); const text = await readFile(abs, "utf8"); const allLines = text.split("\n"); @@ -76,19 +88,35 @@ const readTool = tool({ // ── Write ─────────────────────────────────────────────────────────────────── +interface WriteInput { + file_path: string; + content: string; +} + const writeTool = tool({ name: "Write", description: "Write a string to a file, creating it (and any missing parent " + "directories) if necessary. Overwrites existing content. Use " + "Edit for partial in-place changes.", - parameters: z.object({ - file_path: z - .string() - .describe("Absolute path (or ~/...) to the file to write."), - content: z.string().describe("Full file contents to write."), - }), - async execute({ file_path, content }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["file_path", "content"], + properties: { + file_path: { + type: "string", + description: "Absolute path (or ~/...) to the file to write.", + }, + content: { + type: "string", + description: "Full file contents to write.", + }, + }, + }, + async execute(input) { + const { file_path, content } = input as WriteInput; const abs = expandPath(file_path); await mkdir(dirname(abs), { recursive: true }); await writeFile(abs, content, "utf8"); @@ -98,6 +126,13 @@ const writeTool = tool({ // ── Edit ──────────────────────────────────────────────────────────────────── +interface EditInput { + file_path: string; + old_string: string; + new_string: string; + replace_all?: boolean; +} + const editTool = tool({ name: "Edit", description: @@ -105,23 +140,32 @@ const editTool = tool({ "the file. By default `old_string` must be unique; set " + "`replace_all` to replace every occurrence. Use this for surgical " + "changes instead of rewriting the whole file with Write.", - parameters: z.object({ - file_path: z - .string() - .describe("Absolute path (or ~/...) to the file to modify."), - old_string: z.string().describe("Exact text to replace."), - new_string: z - .string() - .describe("Replacement text. Must differ from old_string."), - replace_all: z - .boolean() - .nullable() - .describe( - "When true, replace every occurrence of old_string. " + - "When false or null, the match must be unique.", - ), - }), - async execute({ file_path, old_string, new_string, replace_all }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["file_path", "old_string", "new_string"], + properties: { + file_path: { + type: "string", + description: "Absolute path (or ~/...) to the file to modify.", + }, + old_string: { type: "string", description: "Exact text to replace." }, + new_string: { + type: "string", + description: "Replacement text. Must differ from old_string.", + }, + replace_all: { + type: "boolean", + description: + "When true, replace every occurrence of old_string. " + + "When false or omitted, the match must be unique.", + }, + }, + }, + async execute(input) { + const { file_path, old_string, new_string, replace_all } = + input as EditInput; if (old_string === new_string) { throw new Error("old_string and new_string must differ"); } @@ -161,6 +205,12 @@ const editTool = tool({ const BASH_DEFAULT_TIMEOUT_MS = 30_000; const BASH_MAX_TIMEOUT_MS = 600_000; +interface BashInput { + command: string; + description?: string; + timeout_ms?: number; +} + function runShell( command: string, timeoutMs: number, @@ -194,7 +244,12 @@ function runShell( // of letting Node emit an uncaught error — the tool returns the // diagnostic so callers can surface it rather than crashing. clearTimeout(killer); - resolveResult({ stdout, stderr: err.message, code: -1, timedOut: false }); + resolveResult({ + stdout, + stderr: err.message, + code: -1, + timedOut: false, + }); }); child.on("close", (code) => { clearTimeout(killer); @@ -211,26 +266,28 @@ const bashTool = tool({ "10min. Use for inspecting files, running scripts, package " + "managers, git, etc. Do not start long-running servers — there " + "is no background mode here.", - parameters: z.object({ - command: z.string().describe("Shell command to execute."), - description: z - .string() - .nullable() - .describe( - "Short (5-10 word) explanation of what this command does. " + - "Recorded in logs.", - ), - timeout_ms: z - .number() - .int() - .min(1000) - .max(BASH_MAX_TIMEOUT_MS) - .nullable() - .describe( - `Optional timeout in milliseconds (max ${BASH_MAX_TIMEOUT_MS}).`, - ), - }), - async execute({ command, timeout_ms }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["command"], + properties: { + command: { type: "string", description: "Shell command to execute." }, + description: { + type: "string", + description: + "Short (5-10 word) explanation of what this command does. Recorded in logs.", + }, + timeout_ms: { + type: "integer", + minimum: 1000, + maximum: BASH_MAX_TIMEOUT_MS, + description: `Optional timeout in milliseconds (max ${BASH_MAX_TIMEOUT_MS}).`, + }, + }, + }, + async execute(input) { + const { command, timeout_ms } = input as BashInput; const timeout = timeout_ms ?? BASH_DEFAULT_TIMEOUT_MS; const result = await runShell(command, timeout); const parts: string[] = []; @@ -245,6 +302,11 @@ const bashTool = tool({ // ── Glob ──────────────────────────────────────────────────────────────────── +interface GlobInput { + pattern: string; + path?: string; +} + const globTool = tool({ name: "Glob", description: @@ -252,17 +314,25 @@ const globTool = tool({ "`src/**/*.md`). Returns absolute paths, one per line, sorted " + "alphabetically. Searches under `path` if provided, otherwise " + "under the current working directory.", - parameters: z.object({ - pattern: z.string().describe("Glob pattern to match (e.g. `src/**/*.ts`)."), - path: z - .string() - .nullable() - .describe( - "Directory to search under. Defaults to the current " + - "working directory.", - ), - }), - async execute({ pattern, path }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["pattern"], + properties: { + pattern: { + type: "string", + description: "Glob pattern to match (e.g. `src/**/*.ts`).", + }, + path: { + type: "string", + description: + "Directory to search under. Defaults to the current working directory.", + }, + }, + }, + async execute(input) { + const { pattern, path } = input as GlobInput; const cwd = path ? expandPath(path) : process.cwd(); const matches: string[] = []; for await (const entry of glob(pattern, { cwd })) { @@ -275,27 +345,41 @@ const globTool = tool({ // ── Grep ──────────────────────────────────────────────────────────────────── +interface GrepInput { + pattern: string; + path?: string; + include?: string; +} + const grepTool = tool({ name: "Grep", description: "Search for a regular expression in files using the system " + "`grep` (or `rg` when available). Returns matching lines with " + "file paths, line numbers, and content.", - parameters: z.object({ - pattern: z.string().describe("Regular expression to search for."), - path: z - .string() - .nullable() - .describe( - "File or directory to search. Defaults to the current " + - "working directory.", - ), - include: z - .string() - .nullable() - .describe("Glob limiting which files to search (e.g. `*.ts`)."), - }), - async execute({ pattern, path, include }) { + strict: false, + parameters: { + type: "object" as const, + additionalProperties: true as const, + required: ["pattern"], + properties: { + pattern: { + type: "string", + description: "Regular expression to search for.", + }, + path: { + type: "string", + description: + "File or directory to search. Defaults to the current working directory.", + }, + include: { + type: "string", + description: "Glob limiting which files to search (e.g. `*.ts`).", + }, + }, + }, + async execute(input) { + const { pattern, path, include } = input as GrepInput; const target = path ? expandPath(path) : process.cwd(); // Prefer ripgrep if installed; fall back to GNU/BSD grep. const rgCheck = await runShell("command -v rg", 2000); diff --git a/src/backend/openai-agents/constants.ts b/src/backend/openai-agents/constants.ts index bc327e66..82c92127 100644 --- a/src/backend/openai-agents/constants.ts +++ b/src/backend/openai-agents/constants.ts @@ -13,23 +13,45 @@ /** * System-prompt suffix appended to the user-configured system prompt. * - * Mirrors the codex backend's suffix shape — documents the two delivery - * routes (plain text via agent_message vs explicit delivery tools). + * The openai-agents handler enforces a strict tool-only delivery + * contract — same as claude-sdk. Trailing prose is private scratchpad + * and is NEVER shipped to the user as a fallback. Replies must reach + * the chat through a delivery tool call. A turn that produces only + * prose triggers one [FLOW VIOLATION] reminder retry; a second + * violation accepts a silent drop. This suffix tells the model that + * up front so it doesn't have to discover it via the reminder. */ export const OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX = ` -## OpenAI Agents Delivery +## Reply contract — tool-only delivery -Two ways to deliver a reply — pick whichever fits: +Your output stream (the prose you produce alongside tool calls) is +PRIVATE scratchpad. The user never sees it. The ONLY way text reaches +the user is through a delivery tool call: -- **Plain text** — your final response text is the reply. Just answer - normally. -- **Delivery tools** — call \`end_turn(text="...", reply_to=N)\` for - threaded replies, \`send(type="text"|"photo"|"poll"|...)\` for rich - content, or \`react(emoji="...")\` for emoji acknowledgements. +- \`end_turn(text="...", reply_to=N)\` — canonical final reply. + Optional \`reply_to\` for threaded replies, optional \`buttons\` for + inline keyboards. +- \`end_turn()\` (no args) — explicit silent close after you've done + something (e.g. just reacted with an emoji) and have nothing else + to say. Use this rather than producing prose-with-no-tool. +- \`send(type="text"|"photo"|"poll"|"voice"|...)\` — mid-turn rich + content. Does NOT close the turn — typically followed by another + \`send(...)\` and finally an \`end_turn(...)\` / \`end_turn()\`. +- \`react(message_id, emoji)\` — emoji reaction. Often the right + response to acknowledge without replying. Pair with \`end_turn()\` + to close cleanly. -If you call a delivery tool, don't also repeat the same text in plain -output — Talon dedupes but it's cleaner to commit to one route. +**There is no plain-text fallback.** If you write a thoughtful reply +in your output stream and forget to wrap it in a tool call, the +handler will re-prompt you ONCE with a \`[FLOW VIOLATION]\` reminder. +A second miss in the same turn drops the prose silently. To save +the user a round-trip of latency, ALWAYS call a delivery tool — +don't talk first and ask "did you get that?" second. + +If you produce trailing prose AND call \`end_turn(text=...)\` with +the same text, the handler dedupes; you're not punished for being +careful, but the tool call is the source of truth. `; /** diff --git a/src/backend/openai-agents/factory.ts b/src/backend/openai-agents/factory.ts index 0fc6eac3..8895c243 100644 --- a/src/backend/openai-agents/factory.ts +++ b/src/backend/openai-agents/factory.ts @@ -13,7 +13,7 @@ import { log } from "../../util/log.js"; import { initOpenAIAgentsAgent } from "./init.js"; import { handleMessage as openAIAgentsHandleMessage } from "./handler.js"; -import { resetState } from "./state.js"; +import { resetState, clearChatSession } from "./state.js"; import { resolveModel, getModelInfo, @@ -34,6 +34,7 @@ const openAIAgentsFactory: BackendFactory = { const backend: QueryBackend = { query: (params) => openAIAgentsHandleMessage(params), + resetChat: (chatId) => clearChatSession(chatId), resolveModel: (q) => Promise.resolve(resolveModel(q)), getModelInfo: (id) => Promise.resolve(getModelInfo(id)), getSettingsPresentation: (m, options) => diff --git a/src/backend/openai-agents/handler.ts b/src/backend/openai-agents/handler.ts index 3b7735de..38bfed38 100644 --- a/src/backend/openai-agents/handler.ts +++ b/src/backend/openai-agents/handler.ts @@ -63,6 +63,7 @@ import { summarizeUsage, routeDelivery, } from "../shared/index.js"; +import { detectFlowViolation } from "../shared/flow-violation.js"; import { OPENAI_AGENTS_SYSTEM_PROMPT_SUFFIX, @@ -70,7 +71,7 @@ import { OPENAI_AGENTS_MAX_TURNS, OPENAI_AGENTS_AGENT_NAME, } from "./constants.js"; -import { getState } from "./state.js"; +import { getState, getOrCreateSession } from "./state.js"; import { getActiveFrontends } from "./init.js"; import { buildOpenAIAgentsMcpServers } from "./mcp.js"; import { OPENAI_AGENTS_BUILTIN_TOOLS } from "./builtins.js"; @@ -179,6 +180,32 @@ export async function handleMessage( // bundled by Claude Code. `mcpServers` carries the Talon frontend // + plugin MCP servers (Telegram tools, Discord tools, mempalace, // etc.). Single agent, no handoffs, no guardrails. + // Diagnostic — enumerate every tool the model will see this turn + // (both built-in function tools and any MCP server tools). Critical + // for tracking down "model never calls end_turn" complaints: if + // end_turn isn't in this list, the model literally cannot call it + // and the problem is in the MCP server registration, not the model. + try { + const builtinNames = OPENAI_AGENTS_BUILTIN_TOOLS.map((t) => t.name); + const mcpToolLists = await Promise.all( + mcpBundle.servers.map((s) => + s + .listTools() + .then((ts: Array<{ name?: string }>) => + ts.map((t) => t.name ?? "?"), + ) + .catch(() => [] as string[]), + ), + ); + const mcpNames = mcpToolLists.flat(); + log( + "agent", + `[${chatId}] tools registered: builtins=[${builtinNames.join(", ")}] mcp=[${mcpNames.join(", ")}]`, + ); + } catch { + /* best-effort diagnostic */ + } + const agent = new Agent({ name: OPENAI_AGENTS_AGENT_NAME, instructions: systemPrompt, @@ -187,10 +214,17 @@ export async function handleMessage( mcpServers: mcpBundle.servers, }); + // Per-chat MemorySession so the SDK preserves the full + // multi-turn record (model outputs, tool calls + results, + // reasoning items where the provider supplies them). Without + // this, every turn starts blind to what was said or done before + // and the model hallucinates context — e.g. claiming it can't + // access a file it wrote in the previous turn. const stream = await run(agent, prompt, { stream: true, maxTurns: OPENAI_AGENTS_MAX_TURNS, signal: abortController.signal, + session: getOrCreateSession(chatId), }); for await (const event of stream) { @@ -209,13 +243,30 @@ export async function handleMessage( // streaming would expose private chain-of-thought scratchpad // to the chat; the final-message event is enough. - // Terminator-driven abort: a delivery tool already shipped the - // reply via the bridge. Cancel the SDK loop so it doesn't burn - // a wrap-up round-trip. - if (streamState.turnTerminated && !abortController.signal.aborted) { + // Terminator-driven abort. The SDK emits TWO events for each + // tool call: + // 1. `tool_called` — model decided to invoke; RPC about to run. + // `recordToolUse` flips `turnTerminated` + // when the tool is `end_turn` / `react`. + // 2. `tool_output` — RPC has completed; the message has + // reached Telegram (for delivery tools). + // + // We must NOT abort on `tool_called` — that cancels the + // in-flight RPC and the message never ships. Aborting on + // `tool_output` after we've already flagged the turn as + // terminated means the delivery happened AND we skip the SDK's + // natural wrap-up round-trip, which otherwise burns 5–10s of + // typing-indicator with no user-visible output (visible to the + // user as "Jeff is typing…" lingering after the reply lands). + if ( + streamState.turnTerminated && + event.type === "run_item_stream_event" && + (event as { name?: string }).name === "tool_output" && + !abortController.signal.aborted + ) { log( "agent", - `[${chatId}] terminator fired — aborting OpenAI Agents turn`, + `[${chatId}] terminator tool result received — aborting wrap-up`, ); try { abortController.abort(); @@ -326,7 +377,6 @@ export async function handleMessage( recordHistogram("response_latency_ms", durationMs); incrementCounter("queries_total"); - incrementTurns(chatId); recordUsage(chatId, { inputTokens: streamState.sdkInputTokens, outputTokens: streamState.sdkOutputTokens, @@ -336,20 +386,88 @@ export async function handleMessage( model: activeModel, }); - // Set a descriptive session name from the user's first message. - if (previousTurns === 0) { + // ── Trailing-prose contract + flow-violation retry ────────────────────── + // Mirrors the claude-sdk handler. The model's output stream is private + // scratchpad: replies MUST go through `end_turn` (canonical) or `send` + // (mid-turn rich content; doesn't close the turn). If the model wrote + // prose without calling either, the user would see nothing — so we + // re-prompt once with a synthetic reminder describing the correct + // flow. A second violation after the retry accepts a silent drop. + // + // Only enforced when delivery tools are actually registered. With + // `frontend: "terminal"` (and the integration-test bootstrap), no + // frontend MCP server spawns and there's no `end_turn` to call — + // forcing the contract there would loop forever. Production frontends + // (Telegram, Discord, Teams) always wire at least one MCP server, so + // a non-empty `mcpBundle.servers` is the cheap signal we have + // delivery tools available. + // + // `incrementTurns` is deferred until AFTER the check so the retry path + // (which recurses through `handleMessage` and increments there) doesn't + // double-count a single user message. + const violation = + mcpBundle.servers.length > 0 + ? detectFlowViolation({ + trailingText: streamState.lastTrailingText, + turnTerminated: streamState.turnTerminated, + deliveredTextNorms: streamState.deliveredTextNorms, + retried: _retried, + }) + : ({ violated: false } as const); + + if (violation.violated) { + incrementCounter("scratchpad.trailing_text_dropped"); + log( + "agent", + `[${chatId}] flow violation: trailing prose (${violation.trailing.length} chars) without end_turn/send. ${ + violation.shouldRetry + ? "Re-prompting with reminder." + : "Already retried — accepting silent drop." + }`, + ); + + if (violation.shouldRetry) { + incrementCounter("scratchpad.flow_violation_retried"); + // Recursive call owns the `incrementTurns` for this user message. + return handleMessage({ ...params, text: violation.reminder }, true); + } + } + + // Reached the non-retry path — this turn counts as one user-visible turn. + incrementTurns(chatId); + + // Set a descriptive session name from the user's *first* message. + // Guarded by `!_retried` so the FLOW_VIOLATION_REMINDER doesn't get + // captured as the session name when the retry recurses through this + // path with `params.text = reminder`. + if (previousTurns === 0 && !_retried) { const name = extractSessionName(text); if (name) setSessionName(chatId, name); } // ── Delivery ────────────────────────────────────────────────────────────── - const delivery = await routeDelivery({ - backendLabel: "OpenAI Agents", - chatId, - state: streamState, - responseText, - onTextBlock, - }); + // Strict tool-only delivery — replies must reach the user via a + // delivery tool (`end_turn` / `send` / `react`). Trailing prose + // is private scratchpad and is NEVER shipped as a fallback. The + // flow-violation retry above gave the model one chance; persistent + // violators get silently dropped, which is the documented + // contract. + // + // routeDelivery is only invoked when there's something to ship via + // its established routes (delivered-via-tools text or a synthetic + // upstream error). Empty turns and trailing-prose-only turns return + // without any output. + const hasDeliverable = + streamState.deliveredTextNorms.length > 0 || !!streamState.syntheticError; + const delivery = hasDeliverable + ? await routeDelivery({ + backendLabel: "OpenAI Agents", + chatId, + state: streamState, + responseText: "", + onTextBlock, + }) + : { route: "silent" as const, chars: 0 }; log( "agent", @@ -435,6 +553,19 @@ function handleToolCalled(item: unknown, ctx: HandleRunItemContext): void { if (!item || typeof item !== "object") return; const raw = (item as { rawItem?: Record }).rawItem; if (!raw || typeof raw !== "object") return; + // Log a compact view of every tool call so we can correlate + // tools=N / terminator / delivered numbers with the model's actual + // intent in production. Truncated to keep the log light. + try { + const rawName = typeof raw.name === "string" ? raw.name : "?"; + const rawArgs = + typeof raw.arguments === "string" + ? raw.arguments.slice(0, 200) + : JSON.stringify(raw.arguments ?? {}).slice(0, 200); + log("agent", `[${ctx.chatId}] tool_call ${rawName} args=${rawArgs}`); + } catch { + /* skip */ + } // MCP tool calls expose `name` (the bare tool name) and `arguments` // (the JSON-decoded input). Function tool calls use the same fields. diff --git a/src/backend/openai-agents/init.ts b/src/backend/openai-agents/init.ts index 091dbe69..540aa246 100644 --- a/src/backend/openai-agents/init.ts +++ b/src/backend/openai-agents/init.ts @@ -111,6 +111,20 @@ export function initOpenAIAgentsAgent( // ignore auth (some local Ollama setups). Placeholder if missing. apiKey: apiKey ?? "missing-key", ...(baseURL ? { baseURL } : {}), + // Disable the SDK's built-in 429 retry-after wait. Some + // free-tier proxies (e.g. opencode.ai's Zen) return + // `retry-after: 15000+` seconds on quota exhaustion, and the + // SDK literally sleeps for that long — which manifests as the + // bot hanging silently for hours. With `maxRetries: 0` the 429 + // surfaces immediately as a RateLimitError that our handler + // classifies and reports to the user. + maxRetries: 0, + // Cap the per-request wait. Default is 10 minutes — far too + // long for an interactive chat bot. 120s comfortably covers + // the slowest reasonable model turn (including thinking + // models) without leaving the user staring at "typing…" for + // hours when the upstream genuinely wedges. + timeout: 120_000, }); setDefaultOpenAIClient(client); @@ -161,12 +175,36 @@ export function initOpenAIAgentsAgent( interface EndpointModelEntry { id?: string; + /** OpenRouter + most providers — human display name. */ name?: string; + /** Gemini's OpenAI-compatible endpoint uses this instead of `name`. */ + display_name?: string; context_length?: number; top_provider?: { context_length?: number }; pricing?: { prompt?: string | number; completion?: string | number }; } +/** + * Normalise the id Talon stores + sends back to the endpoint. + * + * Gemini's OpenAI-compatible `/models` returns ids like + * `models/gemini-2.5-flash`, but the chat-completions route accepts + * either form. Stripping the `models/` prefix: + * + * 1. Keeps the picker label clean (`gemini-2.5-flash`, not + * `models/gemini-2.5-flash`). + * 2. Lets the flat-id provider-inference table in `models.ts` match + * the `gemini-` prefix and bucket the entry under Google, + * instead of treating "models" as a provider name from the + * slash split. + * + * Other endpoints aren't affected — the prefix only appears on + * Gemini. + */ +function normaliseModelId(id: string): string { + return id.startsWith("models/") ? id.slice("models/".length) : id; +} + /** * Query the OpenAI-compatible `/models` endpoint and stash the * advertised model metadata (context window, display name, free @@ -202,9 +240,11 @@ export async function fetchEndpointModels( const data = Array.isArray(json?.data) ? json.data : []; const state = getState(); + let discovered = 0; let enriched = 0; for (const entry of data) { if (!entry || typeof entry.id !== "string") continue; + const id = normaliseModelId(entry.id); const caps: EndpointModelCapabilities = {}; const ctx = typeof entry.context_length === "number" @@ -213,8 +253,14 @@ export async function fetchEndpointModels( ? entry.top_provider.context_length : undefined; if (ctx && ctx > 0) caps.contextWindow = ctx; - if (typeof entry.name === "string" && entry.name) - caps.displayName = entry.name; + // `name` (OpenRouter, others) and `display_name` (Gemini) both + // mean the human label. Prefer `name` when both are present; fall + // back to `display_name` so Gemini gets nice labels too. + const displayName = + (typeof entry.name === "string" && entry.name) || + (typeof entry.display_name === "string" && entry.display_name) || + undefined; + if (displayName) caps.displayName = displayName; const promptPrice = entry.pricing?.prompt; if ( promptPrice !== undefined && @@ -222,12 +268,22 @@ export async function fetchEndpointModels( ) { caps.free = true; } - if (Object.keys(caps).length === 0) continue; - state.endpointModels.set(entry.id, caps); - enriched += 1; + // Always record the id — sparse-response endpoints (NVIDIA NIM, + // bare Ollama, some Azure deployments) advertise just `id` with + // no context_length / pricing / display name, but the picker still + // needs to list them. Storing with an empty caps record gives the + // picker something to render; caps is purely additive metadata. + state.endpointModels.set(id, caps); + discovered += 1; + if (Object.keys(caps).length > 0) enriched += 1; } - log("agent", `OpenAI Agents: enriched ${enriched} models from ${url}`); + log( + "agent", + `OpenAI Agents: discovered ${discovered} model${ + discovered === 1 ? "" : "s" + } (${enriched} enriched) from ${url}`, + ); } /** diff --git a/src/backend/openai-agents/models.ts b/src/backend/openai-agents/models.ts index 5aad19b9..3f2d7b1a 100644 --- a/src/backend/openai-agents/models.ts +++ b/src/backend/openai-agents/models.ts @@ -248,12 +248,75 @@ export function getSettingsPresentation( }; } +/** + * Pattern table for inferring a provider from a model id. + * + * Two id conventions appear in practice: + * + * 1. `vendor/model` — OpenRouter, NVIDIA, vLLM, some Azure deployments. + * The slash-prefix is the canonical provider name; this is the + * easy case. + * + * 2. Flat `family-version-variant` ids — Zen, OpenAI itself, some + * LiteLLM proxies. The provider is implicit in the family + * prefix (`gpt-` → OpenAI, `claude-` → Anthropic, …). Without + * this lookup the picker would lump every flat id under a + * single bucket and the provider chips become useless. + * + * Each entry is `[regex, provider-slug]`. First match wins, so order + * patterns from most-specific to least. + * + * Adding a new prefix is cheap; this is the supported extension point + * for new flat-id endpoints. Do NOT special-case here based on the + * baseURL — the rule should fall out of the id alone so the same + * model resolves the same way regardless of how the user reaches it. + */ +const FLAT_ID_PROVIDER_PATTERNS: ReadonlyArray<[RegExp, string]> = [ + // Anthropic Claude family — both modern (`claude-opus-4-7`) and + // legacy dotted (`claude-3.5-sonnet`). + [/^claude[-.]/i, "anthropic"], + // OpenAI GPT + Codex variants — `gpt-5.5`, `gpt-5-codex`, `o1`, + // `o3-mini`, etc. + [/^(gpt|o\d)[-.]/i, "openai"], + // Google + [/^gemini[-.]/i, "google"], + [/^gemma[-.]/i, "google"], + // NVIDIA + [/^nemotron[-.]/i, "nvidia"], + // DeepSeek + [/^deepseek[-.]/i, "deepseek"], + // Alibaba Qwen + [/^qwen/i, "alibaba"], + // Moonshot AI + [/^kimi[-.]/i, "moonshot"], + // MiniMax + [/^minimax[-.]/i, "minimax"], + // Z.ai GLM family + [/^glm[-.]/i, "z-ai"], + // Mistral AI + [/^(mistral|mixtral|codestral|ministral)[-.]/i, "mistral"], + // Meta Llama + [/^(llama|codellama)[-.]/i, "meta"], + // Microsoft Phi + [/^phi[-.]/i, "microsoft"], + // xAI Grok + [/^grok[-.]/i, "x-ai"], +]; + function providerOf(id: string): string { - // OpenRouter ids look like `vendor/model`. Some have a leading - // tilde (router shortcuts) — treat `~vendor/model` as `vendor`. + // Strip the router-shortcut tilde prefix (`~vendor/model`). const stripped = id.startsWith("~") ? id.slice(1) : id; + // Case 1: explicit `vendor/model` form — take whatever is before + // the first slash. const slash = stripped.indexOf("/"); - return slash >= 0 ? stripped.slice(0, slash) : "openai"; + if (slash >= 0) return stripped.slice(0, slash); + // Case 2: flat id — pattern-match the family prefix. + for (const [pattern, provider] of FLAT_ID_PROVIDER_PATTERNS) { + if (pattern.test(stripped)) return provider; + } + // Unknown flat id — bucket as "other" so it doesn't collide with + // anything specific. + return "other"; } function groupByProvider( @@ -276,7 +339,21 @@ function groupByProvider( return groups; } +/** + * Display-name overrides for cases where the title-casing rule below + * produces something awkward (e.g. "X Ai" instead of "xAI"). + */ +const PROVIDER_DISPLAY_NAMES: Readonly> = { + "x-ai": "xAI", + "z-ai": "Z.ai", + openai: "OpenAI", + deepseek: "DeepSeek", + minimax: "MiniMax", +}; + function humanizeProvider(p: string): string { + const override = PROVIDER_DISPLAY_NAMES[p]; + if (override) return override; // Title-case ids like "anthropic" → "Anthropic", "aion-labs" → "Aion Labs". return p .split(/[-_]/) diff --git a/src/backend/openai-agents/session.ts b/src/backend/openai-agents/session.ts new file mode 100644 index 00000000..73393164 --- /dev/null +++ b/src/backend/openai-agents/session.ts @@ -0,0 +1,277 @@ +/** + * Per-chat conversation memory for the OpenAI Agents backend. + * + * The SDK's `MemorySession` preserves every model output, tool call, + * and tool result across turns — that's what gives the model session + * memory without us having to mirror it ourselves. What it does NOT + * do is bound the cost of that memory: a chat that takes several + * screenshots will quickly accumulate megabytes of base64 image data + * that gets replayed back to the model on every subsequent turn, + * blowing through the context window and confusing small models that + * lose the through-line under a wall of repeated payloads. + * + * `TalonSession` plugs that gap by composing the SDK's session with: + * + * 1. A pipeline of {@link SessionItemTransform}s that rewrite stored + * items before they're replayed to the model. The default + * pipeline elides embedded media (image / file payloads) since + * Telegram already received the deliverable; the model only + * needs the *narrative* (it called the tool, it got something + * back). New transforms — say, truncating noisy Bash output — + * can be added without touching this class. + * + * 2. A {@link CapacityPolicy} that bounds the stored item count and + * evicts the oldest items in pairs, so `function_call` items + * never get separated from their matching `function_call_result` + * across the eviction line (some providers reject orphaned + * pairs). + * + * Storage-side fidelity is preserved: `getItems()` returns items + * exactly as the SDK stored them. Only the model-input path (via + * `prepareHistoryItemForModelInput`) gets the transformed view. + * + * The SDK clones items via `structuredClone` on every `addItems` and + * `getItems` call, so any approach that relies on object-identity + * tracking across those boundaries is broken by design — that's why + * transforms run on every replay rather than only on items "old + * enough" to elide. Stripping every historical media payload is + * correct anyway: by the time an item is in storage, the turn that + * produced it has already completed and the user has already received + * the deliverable. + */ + +import { MemorySession } from "@openai/agents"; +import type { AgentInputItem } from "@openai/agents"; +import { incrementCounter } from "../../util/metrics.js"; + +// ── Public types ─────────────────────────────────────────────────────────── + +/** + * One stateless rewrite step in the replay pipeline. + * + * Implementations should be pure (no I/O, no captured state) and + * idempotent — they may be invoked many times per turn and across + * many turns. Returning the input reference unchanged when there's + * nothing to do allows callers to skip downstream clone costs. + */ +export interface SessionItemTransform { + /** Stable identifier — used in metrics + debugging. */ + readonly name: string; + /** Return the item to replay to the model. May be the same reference. */ + apply(item: AgentInputItem): AgentInputItem; +} + +/** Constructor options for {@link TalonSession}. */ +export interface TalonSessionOptions { + sessionId?: string; + /** + * Maximum number of items the session will retain before evicting + * older entries. Eviction is pair-aware (see {@link enforceCap}). + */ + maxItems?: number; + /** + * Replay-time transforms applied in order. Each transform may + * rewrite the item or pass it through. Defaults to a single + * {@link MediaStripperTransform}. + */ + transforms?: SessionItemTransform[]; +} + +// ── Defaults ─────────────────────────────────────────────────────────────── + +/** + * 200 items ≈ ~50 typical turns (user message + several tool + * call/result pairs + assistant message). Generous for natural + * conversation, bounded enough that a long-lived bot doesn't grow its + * RAM footprint without limit. + */ +const DEFAULT_MAX_ITEMS = 200; + +/** + * Kept short — the goal is to strip bytes from the replay payload + * without losing the structural hint that a media artifact existed. + */ +const ELIDED_MEDIA_PLACEHOLDER = + "[media omitted from history — already delivered to chat]"; + +// ── Built-in transforms ──────────────────────────────────────────────────── + +/** + * Replaces embedded media (image / file / input_image / input_file + * content) with a short text placeholder. The most common offender is + * `browser_take_screenshot` results, which carry hundreds of KB of + * base64 PNG data per call. + */ +export class MediaStripperTransform implements SessionItemTransform { + readonly name = "media-stripper"; + + apply(item: AgentInputItem): AgentInputItem { + if (!item || typeof item !== "object") return item; + + const t = (item as { type?: string }).type; + + // Tool results — the biggest offender. `output` is string | block | block[]. + if (t === "function_call_result") { + const r = item as { output?: unknown }; + const next = this.stripOutput(r.output); + if (next === r.output) return item; + incrementCounter("session.media_stripped.function_call_result"); + return { ...item, output: next } as AgentInputItem; + } + + // Assistant + user messages may also carry image content. + const role = (item as { role?: string }).role; + if (role === "assistant" || role === "user") { + const m = item as { content?: unknown }; + if (Array.isArray(m.content)) { + let mutated = false; + const next = m.content.map((b) => { + const out = this.stripBlock(b); + if (out !== b) mutated = true; + return out; + }); + if (mutated) { + incrementCounter(`session.media_stripped.${role}_message`); + return { ...item, content: next } as AgentInputItem; + } + } + } + + return item; + } + + private stripOutput(output: unknown): unknown { + if (output == null) return output; + if (typeof output === "string") return output; + if (Array.isArray(output)) { + let mutated = false; + const next = output.map((b) => { + const out = this.stripBlock(b); + if (out !== b) mutated = true; + return out; + }); + return mutated ? next : output; + } + if (typeof output === "object") { + const single = this.stripBlock(output); + return single === output ? output : single; + } + return output; + } + + private stripBlock(block: unknown): unknown { + if (!block || typeof block !== "object") return block; + const t = (block as { type?: string }).type; + if ( + t === "image" || + t === "input_image" || + t === "file" || + t === "input_file" + ) { + return { type: "text", text: ELIDED_MEDIA_PLACEHOLDER } as const; + } + return block; + } +} + +// ── Capacity policy ──────────────────────────────────────────────────────── + +/** + * Compute the slice index where eviction should start (inclusive), + * given an item list and a max-items target. Returns 0 when no + * eviction is needed. + * + * Extends the drop boundary if it would split a function_call from + * its matching function_call_result. Worst case we drop a few items + * more than strictly necessary — acceptable in exchange for never + * breaking provider invariants around tool-call pairing. + */ +export function computeEvictionBoundary( + items: ReadonlyArray, + maxItems: number, +): number { + if (items.length <= maxItems) return 0; + + let dropCount = items.length - maxItems; + while (dropCount < items.length) { + const lastDropped = items[dropCount - 1]; + const firstKept = items[dropCount]; + if (!splitsFunctionCallPair(lastDropped, firstKept)) break; + dropCount += 1; + } + return dropCount; +} + +function splitsFunctionCallPair( + lastDropped: AgentInputItem | undefined, + firstKept: AgentInputItem | undefined, +): boolean { + if (!lastDropped || !firstKept) return false; + const droppedType = (lastDropped as { type?: string }).type; + const keptType = (firstKept as { type?: string }).type; + if ( + droppedType === "function_call" && + keptType === "function_call_result" && + (lastDropped as { callId?: string }).callId === + (firstKept as { callId?: string }).callId + ) { + return true; + } + return false; +} + +// ── Session ──────────────────────────────────────────────────────────────── + +/** + * Talon's session wrapper. Inherits SDK behaviour and adds: + * + * - replay-time item transforms (default: media stripping), + * - bounded storage with pair-aware eviction. + */ +export class TalonSession extends MemorySession { + private readonly transforms: SessionItemTransform[]; + private readonly maxItems: number; + + constructor(options: TalonSessionOptions = {}) { + super({ sessionId: options.sessionId }); + this.transforms = options.transforms ?? [new MediaStripperTransform()]; + this.maxItems = options.maxItems ?? DEFAULT_MAX_ITEMS; + } + + /** + * Invoked by the runner before replaying a stored item to the + * model. Runs the transform pipeline in order; each step may + * rewrite or pass through. + */ + prepareHistoryItemForModelInput(item: AgentInputItem): AgentInputItem { + let current = item; + for (const transform of this.transforms) { + current = transform.apply(current); + } + return current; + } + + async addItems(items: AgentInputItem[]): Promise { + await super.addItems(items); + await this.enforceCap(); + } + + /** + * Trim the stored list to {@link maxItems} when it exceeds the cap, + * extending the cut to preserve function-call / function-call-result + * pairs (see {@link computeEvictionBoundary}). + * + * Implemented as full read + clear + re-add because + * {@link MemorySession} doesn't expose a bulk-replace primitive. + * Only runs when over cap, so amortised cost stays low. + */ + private async enforceCap(): Promise { + const all = await super.getItems(); + const dropCount = computeEvictionBoundary(all, this.maxItems); + if (dropCount === 0) return; + const kept = all.slice(dropCount); + await super.clearSession(); + await super.addItems(kept); + incrementCounter("session.items_evicted", dropCount); + } +} diff --git a/src/backend/openai-agents/state.ts b/src/backend/openai-agents/state.ts index 5a23171d..e178f3b6 100644 --- a/src/backend/openai-agents/state.ts +++ b/src/backend/openai-agents/state.ts @@ -9,6 +9,7 @@ import type { TalonConfig } from "../../util/config.js"; import type { FrontendName } from "../registry.js"; +import { TalonSession } from "./session.js"; /** * Capabilities advertised by the remote endpoint for one model id. @@ -38,6 +39,15 @@ export interface OpenAIAgentsState { * implements `GET /models`. */ endpointModels: Map; + /** + * Per-chat conversation memory. The Agents SDK manages the full + * turn history (model outputs, tool calls, tool results, reasoning) + * when we pass a `TalonSession` into `run()`, so we just hand it + * the same instance every turn for the same chat. `/reset` calls + * `clearSession()` on the entry; chat eviction is bounded by the + * map cap so long-lived bots don't leak memory. + */ + sessions: Map; } const state: OpenAIAgentsState = { @@ -45,8 +55,41 @@ const state: OpenAIAgentsState = { gatewayPortFn: () => 19876, frontendName: "telegram", endpointModels: new Map(), + sessions: new Map(), }; +const MAX_SESSIONS = 1000; + +/** + * Get or lazily create the `TalonSession` for a chat. Sessions + * persist for the lifetime of the bot process; the LRU-style cap + * keeps memory bounded if a long-running bot accumulates many chats. + */ +export function getOrCreateSession(chatId: string): TalonSession { + const existing = state.sessions.get(chatId); + if (existing) { + // Refresh insertion-order so cap eviction is least-recently-used. + state.sessions.delete(chatId); + state.sessions.set(chatId, existing); + return existing; + } + if (state.sessions.size >= MAX_SESSIONS) { + const oldest = state.sessions.keys().next().value; + if (oldest !== undefined) state.sessions.delete(oldest); + } + const session = new TalonSession({ sessionId: chatId }); + state.sessions.set(chatId, session); + return session; +} + +/** + * Drop a chat's conversation memory. Called from the dispatcher's + * reset path so `/reset` produces a clean turn-zero session. + */ +export function clearChatSession(chatId: string): void { + state.sessions.delete(chatId); +} + /** Test-only accessor for the shared state object. */ export function getState(): OpenAIAgentsState { return state; @@ -58,4 +101,5 @@ export function resetState(): void { state.gatewayPortFn = () => 19876; state.frontendName = "telegram"; state.endpointModels.clear(); + state.sessions.clear(); } diff --git a/src/core/types.ts b/src/core/types.ts index 8b52c1f2..f6076b2c 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -163,6 +163,14 @@ export interface QueryBackend { query(params: QueryParams): Promise; /** Pre-warm a session (cold-start optimization). Optional — not all backends support this. */ warmSession?(chatId: string): Promise; + /** + * Drop any in-process conversation memory the backend holds for a + * chat. Called from `/reset`. Backends that lean on the SDK's own + * session abstraction (openai-agents `MemorySession`, etc.) need + * this hook so a reset actually wipes the model's working memory; + * stateless backends can ignore it. + */ + resetChat?(chatId: string): void; /** Update the system prompt on the live backend config. Optional — used by plugin hot-reload. */ updateSystemPrompt?(prompt: string): void; /** Hot-swap MCP servers on the active query for a chat. Optional — used by plugin hot-reload. */ diff --git a/src/frontend/discord/actions.ts b/src/frontend/discord/actions.ts index d1a8b7a6..5cf5149c 100644 --- a/src/frontend/discord/actions.ts +++ b/src/frontend/discord/actions.ts @@ -25,6 +25,7 @@ import { readFileSync, statSync } from "node:fs"; import { basename } from "node:path"; +import { expandFsPath } from "../../util/fs-path.js"; import { type Client, type TextBasedChannel, @@ -337,7 +338,7 @@ export function createDiscordActionHandler(client: Client, gateway: Gateway) { case "send_animation": case "send_voice": case "send_audio": { - const filePath = String(body.file_path ?? ""); + const filePath = expandFsPath(String(body.file_path ?? "")); const caption = body.caption ? String(body.caption) : ""; const stat = statSync(filePath); // Per-guild attachment cap based on boost tier. DMs use Tier-0 (10 MB). diff --git a/src/frontend/discord/admin.ts b/src/frontend/discord/admin.ts index a8a4a3ad..e93eeb20 100644 --- a/src/frontend/discord/admin.ts +++ b/src/frontend/discord/admin.ts @@ -42,7 +42,7 @@ export async function handleAdminSubcommand( subcommand: string, argsRaw: string, config: TalonConfig, - _gateway: Gateway, + gateway: Gateway, send: Send, ): Promise { const rest = argsRaw.split(/\s+/).filter(Boolean); @@ -85,6 +85,7 @@ export async function handleAdminSubcommand( if (!target) return send("Usage: /admin kill "); resetSession(target); clearHistory(target); + gateway?.backend?.resetChat?.(target); return send(`Session ${target} reset.`); } diff --git a/src/frontend/discord/commands.ts b/src/frontend/discord/commands.ts index 1143b629..009065a7 100644 --- a/src/frontend/discord/commands.ts +++ b/src/frontend/discord/commands.ts @@ -560,6 +560,7 @@ async function handleReset( resetSession(chatId); clearHistory(chatId); resetPulseCheckpoint(chatId); + gateway?.backend?.resetChat?.(chatId); await gateway?.backend?.warmSession?.(chatId); await reply(i, "Session cleared.", true); } diff --git a/src/frontend/telegram/actions.ts b/src/frontend/telegram/actions.ts index 285f8c88..19341325 100644 --- a/src/frontend/telegram/actions.ts +++ b/src/frontend/telegram/actions.ts @@ -15,6 +15,7 @@ import { } from "node:fs"; import { basename, resolve } from "node:path"; import { dirs } from "../../util/paths.js"; +import { expandFsPath } from "../../util/fs-path.js"; import type { Bot, InputFile as GrammyInputFile } from "grammy"; import { markdownToTelegramHtml } from "./formatting.js"; import { @@ -286,7 +287,7 @@ export function createTelegramActionHandler( case "send_animation": case "send_voice": case "send_audio": { - const filePath = String(body.file_path ?? ""); + const filePath = expandFsPath(String(body.file_path ?? "")); const caption = body.caption ? markdownToTelegramHtml(String(body.caption)) : undefined; @@ -622,7 +623,7 @@ export function createTelegramActionHandler( const userId = Number(body.user_id); const name = String(body.name ?? ""); const title = String(body.title ?? ""); - const filePath = String(body.file_path ?? ""); + const filePath = expandFsPath(String(body.file_path ?? "")); const emojis = (body.emoji_list as string[]) ?? ["🎨"]; const format = (body.format as "static" | "animated" | "video") ?? "static"; @@ -653,7 +654,7 @@ export function createTelegramActionHandler( case "add_sticker_to_set": { const userId = Number(body.user_id); const name = String(body.name ?? ""); - const filePath = String(body.file_path ?? ""); + const filePath = expandFsPath(String(body.file_path ?? "")); const emojis = (body.emoji_list as string[]) ?? ["🎨"]; const format = (body.format as "static" | "animated" | "video") ?? "static"; diff --git a/src/frontend/telegram/commands.ts b/src/frontend/telegram/commands.ts index c408953a..f54890b5 100644 --- a/src/frontend/telegram/commands.ts +++ b/src/frontend/telegram/commands.ts @@ -161,6 +161,9 @@ export function registerCommands( resetSession(cid); clearHistory(cid); resetPulseCheckpoint(cid); + // Wipe any in-process backend memory (e.g. openai-agents' + // MemorySession). Stateless backends ignore this. + gateway?.backend?.resetChat?.(cid); // Warm up the new session so /status has context data immediately await gateway?.backend?.warmSession?.(cid); await ctx.reply("Session cleared."); diff --git a/src/util/fs-path.ts b/src/util/fs-path.ts new file mode 100644 index 00000000..ca063cf4 --- /dev/null +++ b/src/util/fs-path.ts @@ -0,0 +1,32 @@ +/** + * Filesystem-path normalisation for model-supplied input. + * + * Models routinely emit `~/.talon/workspace/...` because that's the + * canonical path Talon documents in prompts. Node's `fs` module + * does NOT expand `~/` — `statSync('~/foo')` fails with ENOENT. + * Tilde expansion is a shell concern, not a libc/Node one. + * + * Anywhere a path crosses from agent-land (tool args, MCP payloads, + * gateway action bodies) into a `fs.*` or send-media API call, route + * it through `expandFsPath` first so the leading `~/` is replaced + * with the actual home directory. + */ +import { homedir } from "node:os"; +import { isAbsolute, resolve } from "node:path"; + +/** + * Resolve a model-supplied path to an absolute on-disk path. + * + * - `~` → `$HOME` + * - `~/` → `$HOME/` + * - already absolute → returned unchanged + * - relative → resolved against `process.cwd()` + * - empty string → returned unchanged (caller decides what to do) + */ +export function expandFsPath(input: string): string { + if (!input) return input; + if (input === "~") return homedir(); + if (input.startsWith("~/")) return resolve(homedir(), input.slice(2)); + if (isAbsolute(input)) return input; + return resolve(process.cwd(), input); +}