Skip to content

fix(openai-compat): require a live conversation id before sending the tool reminder - #76

Merged
Enderfga merged 1 commit into
Enderfga:mainfrom
jailbirt:fix/reminder-requires-live-thread
Aug 14, 2026
Merged

fix(openai-compat): require a live conversation id before sending the tool reminder#76
Enderfga merged 1 commit into
Enderfga:mainfrom
jailbirt:fix/reminder-requires-live-thread

Conversation

@jailbirt

@jailbirt jailbirt commented Aug 7, 2026

Copy link
Copy Markdown

The problem

schemasAlreadyInThread gates the resume-turn reminder on !needsCreate, which only says the session is present in the manager's map. That is weaker than it looks:

  • start() (base-oneshot-session.ts:103) resolves the cwd, assigns a session id, sets _isReady and emits ready. It does not spawn anything.
  • The conversation id is captured later — codex on thread.started (persistent-codex-session.ts:220), agy by harvesting the log after the first turn.
  • A send that fails before that point does not remove the session from the map.

So after a failed first send, the next turn sees sessionExists = true, takes the resumed-thread path, and sends buildToolReminderBlock() — a reminder about tools the engine has never received. No schemas, no identity, no history. The request returns 200 with whatever the model makes of it.

It is a quiet failure: nothing in the logs distinguishes it from a healthy resume, which is why it took a while to find in our deployment.

The change

Require the id to actually be there:

if (!needsCreate && !perMessageMode && engineHasNativeConversation(engine)) {
  try {
    schemasAlreadyInThread = nativeThreadIsLive(engine, manager.getStatus(sessionName).stats);
  } catch {
    schemasAlreadyInThread = false;
  }
}

nativeThreadIsLive() is exported as a small pure predicate so the rule can be tested directly rather than through handleChatCompletion, which has no coverage today.

Both failure directions are safe by construction: when in doubt it sends the full block, which is correct but larger, never the reminder-without-schemas.

SessionManagerLike.getStatus declared its return shape by hand and left both ids out; they are added as optional, which is what they are on SessionStats.

Behaviour

situation before after
thread live (the normal case) reminder reminder — unchanged
session in map, id never captured reminder about tools the engine never saw full block
perMessageMode, fresh session, one-shot engines full block full block — unchanged

Tests

Six cases on the predicate: id required for codex / codex-app / agy, the two ids not interchangeable, and engines that hold context in a live process still treated as live so claude does not regress.

npm run build, npm run lint, npm run format:check clean; vitest run green at 900/900 across 58 files.

Why this one first

We run this in production on the codex engine (~15 agents) and have two more changes queued that both need a trustworthy "is the thread really there" predicate — trimming already-sent tool results, and skipping the system prompt on resumed turns. Both are unsafe on top of !needsCreate alone, so this seemed like the right thing to send first and on its own.

Separately filed #75, on contextPercent and the auto-compaction gate — not bundled here since it is a different area.

… tool reminder

The resume-turn reminder was gated on `!needsCreate`, which only says the session is
in the manager's map. That is not the same as the engine having created a
conversation: `start()` (base-oneshot-session.ts:103) resolves the cwd, assigns an
id and marks the session ready without spawning anything, and the conversation id is
captured later — codex on `thread.started` (persistent-codex-session.ts:220), agy by
harvesting the log after the first turn.

So a first send that fails before that point leaves the session in the map with no
id, and the next turn takes the resumed-thread path: it sends a short reminder
referring to tools the engine never received, with no schemas, no identity and no
history — and still answers 200.

The gate now also requires the id to be present, via a small exported predicate so
the rule is testable on its own. Once a thread is live nothing changes.

`SessionManagerLike.getStatus` had the two ids omitted from its hand-written return
type; they are added as optional, which is what they are.
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.

3 participants