Skip to content

fix(openai-agents): persist turn memory + expand ~/ in send_* actions - #208

Merged
dylanneve1 merged 9 commits into
mainfrom
fix/openai-agents-session-and-tilde-expansion
May 18, 2026
Merged

fix(openai-agents): persist turn memory + expand ~/ in send_* actions#208
dylanneve1 merged 9 commits into
mainfrom
fix/openai-agents-session-and-tilde-expansion

Conversation

@dylanneve1

Copy link
Copy Markdown
Owner

Summary

Two production bugs hit while testing the openai-agents backend against OpenRouter.

1. No turn-to-turn memory

run() was always called without a session, so the Agents SDK couldn't carry the multi-turn ledger forward. Visible symptom: model wrote and sent an SVG file in turn 1; turn 2 ("convert it to PNG") had the model say "Telegram user client is disconnected, can't access the file" — pure hallucination because it had no record of what it just did.

Fix: per-chat MemorySession (from @openai/agents) stored in the backend's state map, passed into every run(). The SDK records model outputs, tool calls, tool results, and reasoning automatically — no Talon-side bookkeeping needed.

/reset now calls a new QueryBackend.resetChat(chatId) hook that clears the MemorySession. Stateless backends (claude-sdk, codex, kilo, opencode) ignore the hook. Session map is LRU-capped at 1000 entries.

2. ~/ not expanded in send_* actions

Models emit paths like ~/.talon/workspace/robot.svg (because that's how Talon documents the workspace). Node's fs.statSync doesn't expand ~/ — visible symptom: send_file failed: ENOENT: no such file or directory, stat '~/.talon/workspace/robot.svg'.

Fix: extract the existing expandPath from openai-agents/builtins.ts into a shared src/util/fs-path.ts. Use it in Telegram + Discord send_file / send_photo / send_video / send_animation / send_voice / send_audio / create_sticker_set / add_sticker_to_set, plus the builtin tool set (deduped).

Test plan

  • npm run typecheck clean
  • npm run format:check clean
  • npx vitest run src/__tests__/fs-path.test.ts src/__tests__/openai-agents — 81 passed (75 openai-agents + 6 new fs-path)
  • Manual smoke: bot restart, ask it to write a file, ask it to do something with that file in the next turn — model now remembers without hallucinating disconnects.
  • Manual smoke: ask the bot to send_file an ~/.talon/workspace/... path — ENOENT gone.

🤖 Generated with Claude Code

dylanneve1 added a commit that referenced this pull request May 18, 2026
Three consecutive Windows runs on PR #208 hit the 240s timeout at
256–258s for `published tarball includes runtime assets and exposes
a working CLI` — the `npm install <tarball>` call cold-installs
every runtime dep into a temp directory, and Windows runners have
been consistently slower since the last bump (some runs make it,
most don't). Move the ceiling far enough out that healthy slow
runs pass while still catching genuine hangs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@claudiusthebot claudiusthebot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — two real production bugs found during live OpenRouter testing, both fixed correctly. Full diff read; CI 35/36 (one Windows job still running — expected given the functional timeout bump in this PR).


Bug 1: MemorySession per-chat

The root cause (run() always starting sessionless → model blind to previous turns) was correctly identified. The fix is the right one: per-chat MemorySession in a Map<string, MemorySession> in state.ts, the same instance passed to every run() for that chat.

LRU eviction via delete(chatId); set(chatId, existing) on access is the standard Map-based LRU trick — correct and zero-dependency. Cap at 1000 sessions is a sensible bound for a self-hosted bot.

clearChatSession on /reset → wired correctly in Telegram (commands.ts), Telegram admin (admin.ts), Discord (commands.ts), Discord admin (admin.ts). Optional-chaining everywhere (gateway?.backend?.resetChat?.(chatId)) is safe.

QueryBackend.resetChat as an optional hook (stateless backends ignore, openai-agents implements) is the right interface shape — doesn't break existing backends.


Bug 2: ~/ not expanded in send_* actions

Extracting expandFsPath to src/util/fs-path.ts is the right call. The Telegram + Discord action handlers both now route file_path through it before hitting statSync/readFileSync. Sticker tools (create_sticker_set, add_sticker_to_set) also patched.

Test coverage on expandFsPath is thorough — the ~foo case (NOT tilde-relative, should resolve as relative filename) is the subtle edge case that's easy to miss. Good that it's explicitly tested.


Zod → JSON Schema migration in builtins.ts

This is the most non-obvious fix and the most important one for OpenRouter compatibility.

The problem: @openai/agents's tool() factory forces strict: true for Zod schemas, which forces every declared property into required. That's spec-correct for OpenAI's own models but breaks non-OpenAI models routed through chat_completions (Trinity, Owl, etc.) — they drop optional fields like timeout_ms/offset/limit and the SDK rejects the call as "Invalid JSON input."

The fix (plain JSON Schema + strict: false + explicit required arrays with only the truly-required fields) is correct. Zod was convenient but wrong for a backend intended to run against arbitrary providers. The per-tool interface types for execute(input) keep the TypeScript surface clean.

One note: the same pattern should probably be applied to any future builtins added to this file.


Flow-violation contract

Handler now mirrors claude-sdk: trailing prose without end_turn/senddetectFlowViolation → one reminder retry → silent drop on second miss. incrementTurns correctly deferred until after the retry-or-drop decision. _retried guard on session naming prevents the FLOW_VIOLATION_REMINDER text getting captured as the session name. All correct.

The mcpBundle.servers.length > 0 guard (skip enforcement with frontend: "terminal", which has no delivery tools) is the right sentinel — production frontends always wire at least one MCP server.


CI matrix expansion

openai-agents added to the live-backend matrix. The install-backend-cli.mjs short-circuit (no external CLI needed) is clean. The dummy OpenAI server (dummy-openai-server.ts) is a solid in-process mock — scripted SSE responses, request recording, proper tool-call + tool-result round-trip support. 393-line integration test covers all 5 scenarios (endpoint discovery, plain text, tool dispatch, multi-turn memory, resetChat).


Functional timeout bump: 240s → 480s

Second bump in ~2 weeks (180s → 240s → 480s). Not a PR concern — Windows npm install latency is a CI infrastructure issue — but the trend is worth watching. If it keeps climbing, the right fix is probably caching the npm tarball between CI runs rather than expanding the timeout indefinitely.


35/36 CI green at review time (Windows Node 22 Tests in-progress, which is the job this PR's timeout bump targets). When it finishes, this should be 36/36. Clean to merge whenever you're ready.

@dylanneve1
dylanneve1 enabled auto-merge (squash) May 18, 2026 20:21
dylanneve1 added a commit that referenced this pull request May 18, 2026
Three consecutive Windows runs on PR #208 hit the 240s timeout at
256–258s for `published tarball includes runtime assets and exposes
a working CLI` — the `npm install <tarball>` call cold-installs
every runtime dep into a temp directory, and Windows runners have
been consistently slower since the last bump (some runs make it,
most don't). Move the ceiling far enough out that healthy slow
runs pass while still catching genuine hangs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dylanneve1
dylanneve1 force-pushed the fix/openai-agents-session-and-tilde-expansion branch from 3ceff90 to 0d006aa Compare May 18, 2026 20:21
dylanneve1 and others added 9 commits May 18, 2026 21:48
…nd_*

Two distinct issues observed in production with the openai-agents
backend pointed at OpenRouter:

1. No turn-to-turn memory. Every call to `run()` was constructing a
   fresh `Agent` with no `session`. The Agents SDK records the full
   multi-turn ledger (model outputs, tool calls, tool results,
   reasoning) when a `Session` is passed in; without one, every turn
   starts blind to what was said or done before. Visible symptom:
   user asks "convert that SVG to PNG", model says "Telegram user
   client is disconnected, can't access the file" — when in fact the
   model itself had written and sent that file 90 seconds earlier in
   the previous turn.

   Wire `MemorySession` from `@openai/agents` into the per-chat state
   map and pass it into `run()` on every turn. The SDK then preserves
   the entire turn record automatically — no Talon-side bookkeeping
   for outbound messages, tool results, or reasoning items.

   Add a `resetChat?(chatId)` hook on `QueryBackend`. `/reset` calls
   it to clear the chat's `MemorySession`; stateless backends
   (claude-sdk, codex, kilo, opencode) ignore the hook. Telegram and
   Discord `/reset` paths invoke it.

   Sessions are capped at 1000 entries with LRU eviction so a
   long-running bot can't leak memory across thousands of chats.

2. `~/` not expanded in send_file / send_photo / sticker create.
   Models routinely emit paths like `~/.talon/workspace/robot.svg`
   because that's how Talon describes the workspace in prompts.
   Node's `fs` module does NOT expand `~/` — `statSync('~/foo')`
   fails with ENOENT, which surfaced as
   `send_file failed: ENOENT: no such file or directory, stat
   '~/.talon/workspace/robot.svg'` on the gateway path.

   Extract the existing inline expander from
   `openai-agents/builtins.ts` into a shared `src/util/fs-path.ts`
   and use it in:
     - openai-agents Read/Write/Edit/Bash/Glob/Grep (already
       expanded, just deduped)
     - telegram send_file / send_photo / send_video / send_animation
       / send_voice / send_audio / create_sticker_set /
       add_sticker_to_set
     - discord send_file / send_photo / send_video / send_animation
       / send_voice / send_audio

   New `src/__tests__/fs-path.test.ts` (6 tests) pins the expansion
   behaviour, including the corner cases (bare `~`, `~/sub`, absolute
   passthrough, relative resolution, empty input, the non-home
   tilde-prefix case `~weird`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rver

Adds a live-backend test for the openai-agents backend in the same
shape as the existing kilo/opencode/claude/codex live tests, but
targeting a dummy OpenAI-compatible HTTP server that the test
itself stands up. The other backends spawn a real upstream CLI;
openai-agents talks to any chat-completions endpoint over the wire,
so the natural live target is a deterministic in-process double.

`src/__tests__/integration/dummy-openai-server.ts` (new)
────────────────────────────────────────────────────────

Bare-bones `node:http` server. Listens on a random port, returns a
configurable model catalog from `GET /v1/models`, and streams a
scripted Server-Sent-Events response on `POST /v1/chat/completions`
that matches OpenAI's chat-completions streaming protocol byte-for-
byte (role chunk → content chunk → optional tool_calls chunk →
finish chunk → `[DONE]`). Tests prime the script per turn and
inspect captured request bodies for assertions.

`src/__tests__/integration/openai-agents-live-discovery.test.ts` (new)
──────────────────────────────────────────────────────────────────────

Boots the production openai-agents code path through `handleMessage`
pointed at the dummy server, so every layer the runtime exercises
(`initOpenAIAgentsAgent`, `fetchEndpointModels`, the SDK's `run()`
with `MemorySession`, tool dispatch, delivery routing) runs end-to-
end. Seven scenarios:

  - `GET /models` enrichment populates `state.endpointModels` and
    flags `pricing.prompt: "0"` entries as free-tier.
  - `resolveModel` picks up enriched metadata.
  - Plain text turn delivers via the trailing-prose path and the
    request body carries the user message.
  - Tool call dispatch: model asks for `Bash`, the backend actually
    executes the command via the openai-agents builtin, and a
    follow-up request carries the tool result back with `role: "tool"`.
  - Multi-turn memory: the second turn's request body includes the
    first turn's user + assistant messages — confirms the SDK is
    threading `MemorySession` history into subsequent `run()` calls.
  - No cross-chat leakage: chat-B's request must not see chat-A's
    history.
  - `clearChatSession()` (the hook `/reset` uses) wipes the session
    so the next turn starts blank.

CI plumbing
───────────

  - `package.json`: new `test:openai-agents:backend` script in the
    same shape as the other live-backend scripts.
  - `.github/workflows/ci.yml`: `openai-agents` added to the
    `backend-live` matrix.
  - `.github/scripts/install-backend-cli.mjs`: short-circuits for
    `openai-agents` since there's no external CLI to install — the
    dummy server is in-process.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new `fs-path.test.ts` cases hardcoded forward slashes (`/~weird`,
`relative/file.txt`) in `endsWith()` assertions. On Windows
`path.resolve` returns paths with `\` separators, so both
assertions evaluated to `false`. Replace with platform-correct
checks using `path.sep` and `path.resolve` round-trips.

No production code change — the helper itself was correct on
Windows; only the tests were POSIX-only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three consecutive Windows runs on PR #208 hit the 240s timeout at
256–258s for `published tarball includes runtime assets and exposes
a working CLI` — the `npm install <tarball>` call cold-installs
every runtime dep into a temp directory, and Windows runners have
been consistently slower since the last bump (some runs make it,
most don't). Move the ceiling far enough out that healthy slow
runs pass while still catching genuine hangs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`@openai/agents`'s `tool()` factory forces `strict: true` when the
parameter schema is Zod, and OpenAI's strict mode requires every
property declared in `properties` to also appear in the `required`
array. Combined with `.nullable()` on convenience fields like
`description` / `timeout_ms` / `offset` / `limit` / `replace_all`,
that meant every model call had to emit those fields explicitly
(e.g. `description: null`) or the SDK rejected the call as "Invalid
JSON input for tool …".

Production effect (caught from the bot's own trace): "Heads up — the
Bash tool was completely broken this whole time (JSON input errors
on every call). I had to use triggers as a workaround." Models
routed through OpenRouter (Trinity, Owl, etc.) consistently omit
optional fields and hit this on Bash, Read, Edit, Glob, Grep.

Rewrite all six builtin tool schemas as plain JSON Schema
(`type: "object"`, `additionalProperties: true`, `required: [...]`)
with `strict: false`. Now optional fields can be omitted by the
model and the call still validates; required fields stay enforced.

The runtime function bodies are unchanged — they already
defensively handled missing optional fields (offset ?? 1,
timeout_ms ?? DEFAULT, etc.).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ract

The openai-agents handler was shipping trailing prose to the frontend
as a fallback when the model didn't call a delivery tool. Production
symptom: model says "Let me fetch that for you" with `tools=0`,
Talon delivers the prose, model never actually does the fetch.
Effectively the model could "announce intent and walk away" and the
user would see only the announcement.

The claude-sdk backend already enforces a strict contract: the
output stream is private scratchpad, replies must reach the user
via `end_turn` (canonical) / `send` (mid-turn rich content) /
`react`. Prose without one of these gets one flow-violation reminder
re-prompt; a second violation is silently dropped. Mirror that here:

  - Call `detectFlowViolation` (shared with claude-sdk) after the
    stream loop. Re-run the agent once with the reminder when the
    model wrote prose without a delivery tool.
  - Stop using routeDelivery's `text-part` fallback. The handler now
    only invokes routeDelivery when there's something to ship via
    its established routes (delivered-via-tools text, or a synthetic
    upstream error). Empty turns are silent; flow-violations are
    silent after the retry.
  - Skip the flow-violation gate when no delivery tools are
    registered (the `frontend: "terminal"` / live-test case where
    no MCP servers spawn). Without that exemption the contract
    can't be honoured and the retry would loop forever.

The integration test was asserting the old fallback behaviour
(trailing prose delivered via onTextBlock); updated to assert the
new strict contract: `result.text` still records what the model
said (for tracing + daily-log), but `onTextBlock` is not called
when no delivery tool fired.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The system-prompt suffix still advertised "Plain text — your final
response text is the reply. Just answer normally." That contradicted
the strict tool-only delivery contract introduced in the previous
commit, so models naturally produced bare prose, hit the flow-
violation retry, and only then learned the rules from the
synthetic reminder — burning ~15s per turn on the round-trip.

Rewrite the suffix to:

  - State up front that the output stream is private scratchpad and
    NEVER reaches the user as a fallback.
  - Enumerate the only paths to the chat: `end_turn(text=...)` /
    `end_turn()` / `send(...)` / `react(emoji=...)`.
  - Describe the flow-violation reminder + silent-drop path so the
    model knows to call a tool first try.

Also fix a small bug in the retry recursion: `extractSessionName`
was being run on the FLOW_VIOLATION_REMINDER text when the retry
landed here with `params.text = reminder`. Sessions ended up named
"Your previous turn ended witho…". Guard with `!_retried`.

Update the constants test to assert the new wording.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… support

Make the OpenAI Agents backend usable against any OpenAI-compatible
endpoint — NVIDIA NIM, Google Gemini, Zen, Ollama, OpenAI itself —
without breaking the OpenRouter-style flow it already supported.

Session memory:
- `TalonSession` wraps `MemorySession` and elides large media payloads
  (`function_call_result.output` images, `input_image` user-message
  content) before replaying history to the model. `browser_take_
  screenshot` results were causing the in-context history to balloon
  past 200k tokens within a few turns, drowning small models in
  duplicate screenshot bytes they had already acted on.
- `SessionItemTransform` interface so new replay rewrites (e.g. Bash
  output truncation) drop in without touching `TalonSession`.
- `computeEvictionBoundary` enforces a 200-item soft cap with
  pair-aware eviction so `function_call` items never get separated
  from their matching `function_call_result` across the cut line.
- 15 new tests cover transform pipeline ordering, media-strip
  identity passthrough, pair-aware eviction, and clearSession reset.

Endpoint robustness:
- Record every model id from `/models`, even when the entry has only
  `{id, object, created, owned_by}` (NVIDIA, Zen, bare OpenAI). The
  picker now lists all 125 NVIDIA / 40 Zen / 51 Gemini models instead
  of dropping them as unenriched.
- Parse Gemini's `display_name` field alongside the standard `name`.
- Strip the `models/` prefix from Gemini ids so they don't bucket
  under "models" in the picker.
- Set `maxRetries: 0` and `timeout: 120_000` on the OpenAI client.
  The SDK's default behavior is to honor `retry-after` literally,
  which manifested as a 4-hour silent sleep when Zen's free-tier
  proxy returned `retry-after: 15231` on quota exhaustion. 429s now
  surface as immediate `RateLimitError`s the handler can report.

Provider grouping:
- Replace the flat-id-defaults-to-"openai" rule in `models.ts` with a
  pattern table covering `claude-` / `gpt-` / `gemini-` / `gemma-` /
  `nemotron-` / `deepseek-` / `qwen` / `kimi-` / `minimax-` / `glm-` /
  `mistral` / `llama-` / `phi-` / `grok-`. Unknown flat ids fall to
  "Other" instead of being misattributed to OpenAI.
- Display-name overrides for `xAI`, `Z.ai`, `OpenAI`, `DeepSeek`,
  `MiniMax` so the title-cased default doesn't produce "X Ai" etc.
- 4 new tests verify Zen-style flat ids bucket correctly, the
  slash-prefix path still works, and unknown ids fall to "Other".

Handler:
- Abort the SDK run loop on `tool_output` (not `tool_called`) so the
  terminator tool's RPC completes before we cancel. Aborting at
  `tool_called` was racing the in-flight `send`/`end_turn` action and
  occasionally dropping the user-visible reply.
- Diagnostic log of every registered tool name at turn start, plus a
  compact one-line trace of each tool call's name+args. Critical for
  debugging "model never calls end_turn" reports where the actual
  cause is usually that the MCP server didn't register the tool.
…t timeouts

Two Windows-only CI flakes in triggers-extended.test.ts:

1. The python interpreter takes 3-5s to cold-start on Windows runners,
   blowing past vitest's 5s default test timeout. Bumped the python +
   node alternate-language tests to 15s.

2. `_resetTriggersForTesting()` only resets the trigger store; it
   doesn't kill ChildProcess handles in `core/triggers.ts`'s `children`
   map. When the python test timed out with a child still alive, the
   next test (`spawnTrigger twice is a no-op`) saw `getRunningCount()`
   inflated to 2 from the leaked process, failing the `<= 1` assertion.
   Added `afterEach(shutdownTriggers)` so each test starts with an
   empty `children` map.
@dylanneve1
dylanneve1 force-pushed the fix/openai-agents-session-and-tilde-expansion branch from 0d006aa to c23bd70 Compare May 18, 2026 20:48
@dylanneve1
dylanneve1 merged commit bf2b01b into main May 18, 2026
37 checks passed
dylanneve1 pushed a commit that referenced this pull request May 19, 2026
Resolve conflicts from PR #208 (openai-agents MemorySession +
turn-memory persistence) landing on main under us.

- state.ts: keep BOTH the per-chat `sessions` map (PR #208) and the
  discovery promise tracking (PR #211).
- factory.ts: import `clearChatSession` (PR #208) AND `releaseAllBundles`
  (PR #211). Both lifecycles needed.
- handler.ts: preserve PR #211's mcpConfig.includeServerInToolNames
  documentation alongside PR #208's tool-list diagnostic. Agent
  constructor uses both `mcpConfig` and per-chat session.
- init.ts: take PR #211's discovery.ts extraction wholesale —
  inline helpers superseded by the module.
- discovery.ts: backfill PR #208 behavior — always store entries
  with valid `id` (bare-id ok), add `display_name` fallback (Gemini),
  add `normaliseModelId` (strip `models/` prefix from Gemini). Fixes
  enrichment test that expected sparse `/v1/models` entries to land
  in the catalog regardless of caps.
- frontend commands.ts (Discord + Telegram): merge per-chat backend
  resolution (PR #211) with `resetChat` invocation (PR #208) so
  `/reset` wipes the right backend's MemorySession before warming.
- test fixes: live-discovery test imports `fetchEndpointModels` from
  discovery.js (was init.js); models test `await`s the now-async
  `getSettingsPresentation` in 4 sites added by PR #208.

2541/2541 unit tests passing. Typecheck + prettier clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants