refactor(backend): address code-review findings - #194
Merged
Conversation
Addresses the audit items Dylan flagged in chat:
Real bug
- opencode/one-shot.ts was missing the abort handler that
kilo/one-shot.ts has — heartbeat-timeout fires but the OpenCode
session keeps generating tokens. Ported the abort listener +
appendBackendSuffix + errMsg usage so the two files actually mirror
each other now.
Stale doc / accepted debt
- claude-sdk/handler.ts: the multi-paragraph "qi.interrupt() racing
with sibling MCP tools" comment described the old `interrupt()`
approach. The PostToolBatch hook in options.ts already terminates
the SDK loop without the race — comment rewritten to reflect the
current implementation + keep the historical context for future
readers.
Logic
- claude-sdk/handler.ts: `incrementTurns` was called BEFORE the
flow-violation check, so a violation that triggered the recursive
retry would double-count a single user message as two turns. Moved
the increment after the violation block. Token usage stays where it
was (additive into session totals; recording both attempts is
desired).
Type hygiene
- New shared `remote-server/sse-stream.ts` (`subscribeSseStream`):
centralises the `(await oc.global.event()) as unknown as { stream }`
narrowing + the "subscribe failed" warning that kilo and opencode
both had inline.
- New shared `remote-server/messages.ts` (`findLastAssistantMessage`):
generic walker that replaces the duplicated `findLastAssistantMessage`
+ `info as unknown as KiloAssistantInfo | OpenCodeAssistantInfo` cast
in both handlers. Runtime guard + single documented cast.
- New `asCodexConfig` helper in codex/mcp-config.ts: centralises the
`Record<string, CodexMcpServer> → CodexOptions["config"]` cast
(SDK's `CodexConfigObject` recursive type is too restrictive for
`Record<string, X>` to satisfy structurally).
Duplication
- New shared `backend/shared/sleep.ts` — replaces the identical
abort-aware sleep helpers that kilo/handler.ts and opencode/handler.ts
each carried at the bottom of their files.
Logging
- remote-server/events.ts: the four `/* non-fatal */` catches that
silently swallowed UI-callback errors now log at debug level. A
repeatedly-failing frontend callback will surface under
LOG_LEVEL=debug without polluting the default operator log.
Policy comment
- options.ts: the `permissionMode: "bypassPermissions"` +
`allowDangerouslySkipPermissions: true` pair now has a paragraph
explaining the bot-account-as-security-boundary rationale, so the
first reader doesn't flinch and the pair stays linked.
Not in scope (separate PRs)
- The kilo↔opencode handler/server/models duplication (~700 LOC
parallel files). The right path is to extend the `remote-server/`
extraction that already exists for `events.ts`; that's a multi-day
refactor with its own review surface and not bundled here.
- The codex system-prompt fusion (first-turn `INSTRUCTIONS` block) —
needs a deeper conversation about SDK-side support before any
Talon-side fix.
Verified
- `npx tsc --noEmit` clean
- `npm test` — 2373 passing, 12 skipped (live), 0 failing
- `npm run lint` — 11 warnings, 0 errors (all pre-existing in discord/)
- `npm run format:check` clean
…dels
Dylan reported the Codex backend wasn't working — turns out the
default model `gpt-5-codex` is rejected with a 400 invalid_request
"not supported when using Codex with a ChatGPT account" on accounts
authenticated via `codex login` (ChatGPT OAuth) rather than a paid
API key. ChatGPT-OAuth accounts get `gpt-5.5` and below; `gpt-5-codex`
requires API-key billing.
This PR adds end-to-end ChatGPT-OAuth support:
## Auth detection (`backend/codex/auth.ts`)
New `detectCodexAuth(configKey, env?)` returns `{ mode, source,
authFilePath?, authFileParsed, parseError? }`. Priority order:
1. `OPENAI_API_KEY` env var → `api-key`
2. `openaiApiKey` in talon.json → `api-key`
3. `~/.codex/auth.json` w/ non-null `OPENAI_API_KEY` → `api-key`
4. `~/.codex/auth.json` w/ `auth_mode: "chatgpt"` → `chatgpt`
5. Everything else → `none` (caller emits a `codex login` reminder)
`initCodexAgent` now runs detection synchronously at boot and the
startup log surfaces the active mode + source. No more "first turn
will fail" warning when ChatGPT OAuth is correctly configured.
## Model catalog + flags (`backend/codex/models.ts`)
- Reordered catalog so `gpt-5.5` is first (it's the broadest-access
flagship — works on both auth modes).
- Added `gpt-5.5` (was missing entirely).
- Added new `apiKeyOnly: true` flag on the `CodexModelInfo` interface
for entries the OpenAI API rejects on ChatGPT-mode accounts.
Currently flagged: `gpt-5-codex`.
- Bumped context windows to 400k (matches current upstream).
- New helpers `isCodexApiKeyOnlyModel(id)` + `chatGptFallbackFor(id)`
drive the recovery ladder.
## Auth-aware defaults + recovery (`backend/codex/handler.ts`)
Two recovery paths now cover ChatGPT-mode users transparently:
1. **Pre-emptive swap.** When `auth.mode === "chatgpt"` AND the
resolved model is api-key-only, the handler logs a warning and
substitutes the chatgpt-compatible fallback before calling
`runStreamed`. Saves the 400 round-trip and makes the resolved-
model log line accurate.
2. **Post-hoc retry.** If the SDK still surfaces the chatgpt 400
(either thrown or via a `turn.failed` event), the new
`maybeFallbackForChatGptMismatch` helper resets the session,
transient-swaps the chat model, and retries with `_retried = true`
so the recursion can't loop. Hooked into both the catch block
(current SDK behaviour: throws) and the post-event-loop check
(defensive: future SDKs may only emit the event).
The auth-aware DEFAULT chain is also extended: when no explicit
model is configured, ChatGPT-mode picks `CODEX_CHATGPT_DEFAULT_MODEL`
(`gpt-5.5`) and api-key-mode picks `CODEX_DEFAULT_MODEL` (`gpt-5-codex`).
## Tests
- `codex-auth.test.ts` (new, 19 tests): file-priority order, env
precedence, malformed JSON, missing HOME, regex tolerance for the
mismatch error, api-key-only flag + fallback helpers.
- `codex-handler.test.ts` (+4 tests, +23 LOC mock surface): per-call
event queue (`MOCK_EVENTS_QUEUE`) so retry tests can configure
different event sequences per call. Covers the pre-emptive swap,
the post-hoc retry on a thrown error, the event-only retry path,
and the double-failure short-circuit.
- Existing handler tests updated to pass an explicit
`openaiApiKey: "test-api-key"` so they go down the api-key path
regardless of the host's `~/.codex/auth.json` state.
## Verified
- Live SDK probe against Dylan's actual `~/.codex/auth.json`
confirmed: `gpt-5-codex` → 400, `gpt-5.5` → 200 with `pong`.
- `detectCodexAuth(undefined)` returns
`{ mode: "chatgpt", source: "file:~/.codex/auth.json" }` on the
host machine.
- `npx tsc --noEmit` clean.
- `npm test` — 2396 passing, 12 skipped (live), 0 failing.
- `npm run lint` — 11 warnings, 0 errors (all pre-existing in discord).
- `npm run format:check` clean.
## NOT in scope
- OpenAI Agents SDK as a separate backend (Dylan mentioned it as a
"maybe"). That's a fresh ~1500-2000 LOC backend on the same
footprint as kilo/opencode/codex, with its own factory, model
catalog, handler, and live-discovery test. Filing as follow-up.
- The codex/handler.ts system-prompt fusion (still uses the
first-turn `INSTRUCTIONS` block since the SDK doesn't expose
`system` on `runStreamed`).
The pre-emptive-swap test relied on Dylan's ~/.codex/auth.json being in
chatgpt mode, which doesn't hold on CI runners. Fix: create a temp HOME
with {"auth_mode":"chatgpt"} and suppress OPENAI_API_KEY for the
initCodexAgent call, then restore the environment in a finally block.
The test is now fully hermetic and machine-independent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the issues from this morning's backend audit. Eight files modified, three new shared modules created. Net +318 / -127 LOC (most of the +318 is comment/documentation; net code change is small).
What's in scope
Real bug (#3 from review)
opencode/one-shot.tswas missing the abort handler thatkilo/one-shot.tshas. The two files claim to mirror each other (kilo is a fork of opencode) but had drifted. Result: when the heartbeat timeout fires, kilo cleanly callssession.abort()and the model stops generating tokens; opencode just lets the run finish. Real cost bug on opencode heartbeat hangs.Ported the abort listener +
appendBackendSuffix(was inlinesystemPrompt + SUFFIXstring concat) +errMsgusage so the files actually match now.Stale doc / accepted-debt cleanup (#2)
claude-sdk/handler.tshad a multi-paragraph comment justifying whyqi.interrupt()was disabled and claiming we still pay 2-3s of phantom typing perend_turn. ThePostToolBatchhook inoptions.tsalready terminates the SDK loop without the race the old approach hit — that comment was stale. Rewritten to reflect the current implementation while keeping the historical context for future readers.Turn counter logic (#11)
claude-sdk/handler.ts:incrementTurnswas called BEFORE the flow-violation check. IfdetectFlowViolationtriggered the recursive retry, turns got incremented twice for a single user message. Moved the increment after the violation block.Token usage (
recordUsage) stays where it was — that's intentionally additive into session totals, and recording both attempts is the right behaviour. Only the turn counter needed to be deferred.Type-hygiene helpers (#8, #9, #10)
Three new tiny shared modules that centralise casts the audit flagged:
remote-server/sse-stream.ts—subscribeSseStream(client, chatId)centralises the(await oc.global.event()) as unknown as { stream }narrowing + the "subscribe failed" warning that kilo and opencode both had inline. Runtime guard (Symbol.asyncIteratorcheck) included.remote-server/messages.ts— genericfindLastAssistantMessage<Info>(messages)walker that replaces the duplicatedfindLastAssistantMessage+info as unknown as KiloAssistantInfo | OpenCodeAssistantInfocast in both handlers. Runtime guard + single documented cast in one file.codex/mcp-config.ts:asCodexConfig— wrapsRecord<string, CodexMcpServer>into the{ mcp_servers: ... }envelope and handles theCodexOptions[\"config\"]cast. SDK'sCodexConfigObjectis a recursive type thatRecord<string, X>can't satisfy structurally; cast is centralised in one place with a documented explanation.Duplication (#7)
backend/shared/sleep.ts— replaces the identical abort-awaresleep(ms, signal)helpers thatkilo/handler.tsandopencode/handler.tseach carried at the bottom of their files.Logging (#13)
remote-server/events.ts: the four/* non-fatal */catches that silently swallowed UI-callback errors now log at debug level (logDebug). A repeatedly-failing frontend callback will surface underLOG_LEVEL=debugwithout polluting the default operator log. Added a tinyerrMsghelper inside the module.Policy-comment annotation (#12)
options.ts: thepermissionMode: \"bypassPermissions\"+allowDangerouslySkipPermissions: truepair now has a paragraph explaining the bot-account-as-security-boundary rationale. The two flags need to flip together — the comment locks them in as a unit so a future refactor doesn't split them.What's NOT in scope (separate PRs)
remote-server/extraction that already exists forevents.ts; that's a multi-day refactor with its own review surface and shouldn't be bundled here. This PR does extract the smallest shared pieces (sse-stream, messages, sleep) as a down payment.INSTRUCTIONSblock). Needs a deeper conversation about SDK-side support before any Talon-side fix.Test plan
npx tsc --noEmitcleannpx vitest run— 2373 passing, 12 skipped (live), 0 failingnpm run lint— 11 warnings, 0 errors (all pre-existing insrc/frontend/discord/; no new warnings)npm run format:checkclean🤖 Generated with Claude Code