feat: add opt-in native OpenAI compaction - #53
Conversation
bman654
left a comment
bman654
left a comment
There was a problem hiding this comment.
Thanks for this — and welcome. This is an unusually well-researched first contribution. Before the asks, the things I verified and want to acknowledge, because they're the hard parts:
- The OpenAI protocol work is correct. I checked it against the API reference and Codex's own source rather than taking the doc's word for it.
compaction_triggeris a real input item with exactly that spelling and the required "must be the final input item" placement, which you honor. The returned item istype:"compaction", and hedging oncompaction_summarymirrors Codex's own#[serde(alias)]rather than being superstition.compactRequestPayload's nine fields are Codex'sCompactionInputverbatim, including the same omissions ofstore/stream/include/tool_choice.RESPONSES_COMPACTION_RETAINED_USER_TOKENS = 64_000is Codex'sRETAINED_MESSAGE_TOKEN_BUDGET, the 90% default is Codex's(context_window * 9) / 10, retained-then-opaque ordering matches both Codex'sbuild_v2_compacted_historyand the documented/responses/compactoutput shape, and fresh-chain continuation plus cross-connection replay of the opaque item are both explicitly documented. The claim that this is Codex's remote-compaction-v2 protocol holds, including the delta-on-previous_response_idshape. - The Claude Code integration strings are byte-exact.
CLAUDE_COMPACTION_CONTINUATION_PREFIX, the suffix, all fourCLAUDE_COMPACTION_SUMMARY_TRAILERS, andnormalizeClaudeCompactionSummary's three regexes match the shipped 2.1.220 client exactly. The envelopeisClaudeCodeCompactRequestmatches is the real one. This is not inert. - The state machine is carefully built. I could not find a path that corrupts a chain head, double-registers a connection, leaks an abort listener, or lets a concurrent request steal an entry mid-trigger. The
previousHeadsave/restore set is complete against every field the completion handler writes. ThehistoryContinuationMatchrefactor is provably byte-equivalent for pre-existingexactandomitted_reasoninglineage — the addedisOpaqueCompactionKindfilter is a no-op on pre-PR data becauseexpectedAssistantItemshad no compaction branch before. Theclaude_compaction_summarymode is tightly gated (exact SHA-256, one-shot, reset on the next non-compaction completion) and I could not construct a reachable false match. - Your 12 new tests are real. Nine independent mutations reddened them, and they assert on actual emitted wire frames rather than mock invocation. No summary plaintext or opaque content reaches diagnostics.
pnpm typecheck,pnpm test(1037/1037), andpnpm buildare all clean merged onto currentmain, and commitlint passes on both commits.
That said, I'm requesting changes, and I should be upfront that my main concern is structural rather than a list of defects — items 1 and 2 below are about the design, not about protocol correctness or code quality. See the closing section for where I think that leaves the PR.
First, on what this buys — I think the framing needs narrowing
Before the specifics, I want to check the value proposition, because I think it's narrower than the PR presents and that changes how much risk is worth taking.
Starting with the PR description's stated motivation, because I don't think it describes main accurately:
Claude Code rewrites its local transcript after compaction, breaking clodex's exact-prefix continuation chain. Replaying the full transcript restores correctness but loses the efficient cached continuation path.
The first sentence is right. The second isn't. What main actually does after a Claude Code compaction is take the history_mismatch_new_head branch and start a new chain from the rewritten transcript — which is small, because it is the summary plus a preserved tail, not the pre-compaction conversation. Nothing "replays the full transcript"; the large history is simply gone from the client's context. And little efficiency is lost: instructions and tools are unchanged so that prefix can still cache-hit, only the new summary-sized tail is a write, and the very next turn resumes ordinary delta continuation on the new head. The real cost on main is one small uncached prefix, once per compaction — not a lost efficiency regime.
That matters because it is the premise the whole design rests on. If the problem being solved is one small cache write per compaction, that does not justify the machinery, and it isn't what the PR's own measurements are about either. Tracing the rest of what "cache-efficient native compaction" implies against current main:
-
Cache-warmth of the compaction step isn't a differentiator. On
main, Claude Code's summary turn is already an incremental delta onprevious_response_id, so it's already mostly cache-read. The 90.2% you measured for the trigger is the same property, not a new one. -
The post-compaction chain is a cache write both ways. On
mainthe rewritten transcript builds a new small prefix; here the fresh[retained + opaque]chain builds a new prefix too. Neither is cached, andmain's is typically the smaller of the two. -
Request count is unchanged on the threshold path, and one worse on the
forceCompactionpath. The threshold path is cost-neutral: a compaction inference plus the user's actual request rebased onto the compacted head is the same two requestsmainspends on a summary turn plus the following turn. But on theforceCompactionpath both compactions happen — clodex's hidden trigger, then Claude's summary prompt on the fresh chain, then the next turn continuing from the anchor — so it's three requests wheremaindoes two. That extra inference isn't replacing Claude's compaction, it's stacked on top of it, and it lands on every compaction for anyone who has auto-compact configured. -
And the post-compaction context may well be larger than what Claude Code's own compaction produces. Because
forceCompactionruns both compactions, the steady-state chain after a compaction isinstructions + tools + [retained user tail ≤64k] + [opaque digest] + [assistant summary] + [delta]. The same conversation is represented up to three times there — verbatim in the tail, again inside the opaque digest, and a third time inside the summary that was written from that digest.mainproducesinstructions + tools + [wrapper + summary] + [recent verbatim tail] + [new input]; both paths keep a recent tail (your own trailer list includes"Recent messages are preserved verbatim."), so the genuine extra here is the opaque digest plus the tail-selection difference.I can't tell whether that nets out bigger or smaller, because the PR never measures it — and this is the measurement that decides the feature. If the post-compaction input is larger than Claude Code's, the feature reclaims less headroom per compaction, therefore compacts more often, and pays an extra inference every time. That would make it net-negative on its own terms regardless of everything else in this review.
entry.lastInputTokensimmediately after a compaction, on both paths, is the number I'd want to see.
What's genuinely left as an upside is one thing: you're substituting OpenAI's native opaque compaction item for Claude Code's Anthropic-oriented text summary. That's a legitimate hypothesis — an OpenAI-native representation may well preserve continuity better for these models than a summary prompt written for Claude — and it's the strongest argument for the PR. But it isn't measured here, and it's a quality claim, not a cache claim.
I'd like the PR to be argued on that basis, because it changes the cost/benefit: ~880 lines in the repo's most fragile file, an extra billed inference on every Claude-initiated compaction, and several new failure modes, in exchange for a summary-quality improvement nobody has yet compared. If you can show a side-by-side — same session, same task, compaction via each path, then measure whether the model actually resumes better — that would be the most persuasive thing you could add, and it would also settle item 2 one way or the other.
1. BLOCKING — Default-on removes an unconfigured Claude Code's only compaction trigger, and the resulting failure is unrecoverable
docs/native-codex-compaction.md states: "Native OpenAI compaction does not replace or suppress Claude Code's own compaction." For a default-configured client that isn't true, and it's the reason I can't take this on by default.
In Claude Code 2.1.220 the threshold auto-compact entry point short-circuits:
async function toy(e,t,r,n,o=0,i){ ... if(wSe()&&!JGe(t,r))return!1; ... }wSe() returns true unless CLAUDE_CODE_REMOTE is set. JGe(e,t) is aY(e,t).source !== "auto", and aY only returns a non-"auto" source when a window is configured via CLAUDE_CODE_AUTO_COMPACT_WINDOW, the autoCompactWindow setting, client data, an experiment, or a model-default table — and that table (qn_) contains only Anthropic model ids, while the gYi path requires longContext1mCreditsBlocked. So for an OpenAI model id on a client with no window configured, toy returns false unconditionally: there is no proactive threshold compaction at all. The live path is reactive, gated on a prompt is too long error.
On main, clodex is what produces that error — an upstream 400 becomes anthropicPromptTooLongMessage(...) (src/proxy.ts:690), which the client parses to fire reactive compaction. Compacting at 90% is precisely what stops that 400 from ever happening. Net effect for a default-config user: the OpenAI-side context is managed, but the Anthropic-side transcript is never reclaimed. It grows without bound while clodex sends deltas.
That matters because every path that abandons compacted state then ships a transcript the model can't accept, and there are many of them:
- process restart or
--resume(checkpoints are deliberately not persisted); - socket idle/hard TTL expiry with the checkpoint gone — user goes to lunch and comes back;
- prefix mismatch after
/rewindto a position no longer held, or liberal/forkand/btw; - a model or effort change. The partition key covers upstream model and normalized effort, and checkpoints are keyed identically (
checkpoint.key === entry.key), so a/modelswitch or an effort change orphans the live head and every checkpoint for that session at once — strictly worse than the other paths, which at least leave checkpoints reachable; - all 8-per-partition / 32-global checkpoints evicted;
- standalone-compact failure → "preserving normal response path" → full context;
/compactitself.failedTriggerCompactedInputistriggerEntry.compactedInput ? … : undefined, so on a first compaction it is undefined;canonicalInput = checkpointInput ?? selectedCompactedInput ?? failedTriggerCompactedInputthen falls through to undefined andcompactPayloadbecomespayload— the full untruncated transcript handed to/responses/compact.forceCompactiontakes this path unconditionally, at any size.
I believe this is unrecoverable and fatal to the session, not a degraded fallback. Walking it through on a transcript that has grown to, say, 3M tokens while the clodex-side context sat at 100k:
- The request arrives with no matching head or checkpoint, so
estimated_thresholdfires and standalone/responses/compactis called with the full 3M-token input — rejected, it's far over the window. - The normal full request goes out, gets a 400, and clodex synthesizes
prompt is too long. - Claude Code's PTL handler runs
YRs({trigger:"ptl"})— no precomputed swap is armed, because arming goes through the same threshold machinery that is dead in this configuration — thenswo(...), whose summarize request carries that same 3M-token transcript and therefore fails for the same reason. - That consumes the only attempt.
QRs(e) = !e.hasAttempted && … && JI() && wSe() && !e.aborted— reactive compaction is latched to one shot per turn, so there is no retry. - The one non-inference reduction path, time-based microcompact, is unreachable here: it is gated on a server-supplied context hint (
tengu_context_hint_reject,preCompactTokenEstimate/postCompactTokenEstimate/mcApplied) that clodex never emits — and it only clears tool-result content, so it would not rescue a text-heavy transcript anyway.
There is nothing left but the 3M-token transcript and no mechanism able to compact it. Compare main, where this state simply cannot arise: the transcript can never exceed the window, because the 400 arrives while it is still small enough for Claude Code to summarize.
This is the core of my objection. The feature keeps the OpenAI-side context healthy by concealing its own compaction from Claude Code, and the concealment is what makes the eventual head miss fatal — Claude Code cannot compact a transcript it was never told to keep bounded.
The growth has a running cost too. arraysEqual does two full normalizeToolCallJson + canonicalize + JSON.stringify passes over the whole prefix, per candidate, per request — up to twice per candidate, plus once per matching checkpoint. Benchmarking an equivalent implementation on synthetic Responses input put it at roughly 3 ms at ~250k tokens, 12 ms at 1M and 29 ms at 2M, i.e. linear and unbounded; and because each request's payload is freshly parsed, nothing is shared, so a retained full-transcript snapshot costs on the order of 5 MB of heap at 500k tokens. With a 32-checkpoint global cap and no TTL on checkpoints at all — cleanupExpiredConnections walks only connections — a long-lived clodex server pins those snapshots for the process lifetime.
Ask: ship the feature off by default and document these failure modes for anyone who opts in — see the four requirements in the closing section, which is the concrete path to merge. Opt-in does not resolve the underlying problem, it limits who hits it, which is why the documentation requirement travels with it. Note also that the threshold is derived from the registry contextWindow with no awareness of CLAUDE_CODE_AUTO_COMPACT_WINDOW or CLAUDE_AUTOCOMPACT_PCT_OVERRIDE, so the two systems can disagree about the window even when both are active. Independently of all the above, please give checkpoints a TTL and sweep them in cleanupExpiredConnections.
For calibration, on a client that does configure an auto-compact window the client always wins and your trigger never fires. uMu reports level:"compact" at e >= Sfo(t, r), where t is the effective window (advertised − min(maxOutputTokens, 20000)) and Sfo defaults to t − 13000. For a 272k model, t = 252,000, so:
| Client config | Client compacts at | This PR would compact at |
|---|---|---|
| Window configured, default (~95% of effective) | 239,000 | 244,800 |
| Window configured, 85% override | 214,200 | 244,800 |
| No window configured | never | 244,800 |
So the threshold trigger is reachable only in the row where Claude Code has no proactive compaction — that is, only where it becomes the sole thing keeping the session bounded, and where it simultaneously removes the reactive backstop. In the default-window row the margin is just 5,800 tokens, and the two sides read different numbers (you read entry.lastInputTokens, the client totals its own usage including output), so which one fires first there is genuinely racy.
Worth noting for completeness: clodex patch does not change this. The patch sites cover the model enum, alias validator, resolver switch, picker text and the context-window map — none of them add a model to the auto-compact model-default table (qn_, which holds only Anthropic ids), so a patched binary still resolves source:"auto" and still has the threshold path disabled unless a window is explicitly configured.
2. BLOCKING — forceCompaction compacts before the summary turn, and strips assistant/tool history from it
This is the path that matters most for anyone who does have auto-compact configured, because it's the one that runs on every compaction — and I want to be precise that there is no alternative branch. forceCompaction bypasses both threshold tests, so a Claude Code compaction request always triggers a native compaction first, at any context size. There is no "this behaves like main" case.
compactionReason = forceCompaction ? 'claude_compaction' : ... (responses-websocket.ts:1823) short-circuits both threshold tests, and the enclosing guard (:1837) requires only compactThreshold !== undefined && compactionReason. So the summary turn always triggers a hidden compaction first, at any context size — /compact at 30% buys a full extra billed inference.
Then the summary is requested against [...retainedUserMessages(input), ...compactionOutput], and retainedUserMessages (:536) keeps only items where role === 'user'. In the Responses format function_call and function_call_output items have no role at all — your own test writes them that way — and every role:'assistant' item is dropped. So the portable summary, the one artifact a resumed session lives on, gets written from recent user messages plus an opaque blob, where on main the model sees the complete verbatim history. There's also an ordering artifact: the "produce <analysis>/<summary>" instruction survives as a user message but the opaque item is appended after it, so the instruction is no longer last.
Two things make this worse than it first looks. The user has no way to know: they are looking at a full transcript with no compaction boundary in it, so a summary that is quietly lossier than the visible history is invisible to them. And the lossy artifact is the durable one — the opaque digest is process-local and never persisted, while the degraded text summary is what lands in the transcript and is all that survives a restart or --resume.
Ask: don't compact on the forceCompaction turn. Let the summary be produced against the real chain and compact on the next request instead — you still get the anchor, the summary quality is unchanged, and the extra inference disappears. At minimum, require the threshold to be exceeded before force-compacting.
3. BLOCKING — Compaction runs inside the caller's fetch, under the downstream 120s no-data watchdog
Both await new Response(stream).arrayBuffer() (:1728) and await compactResponsesWindow(...) (:1919) block before any Response is returned. Downstream, streamAnthropicResponse arms its idle timer before streamText and resets it only on the first stream part (sdk-adapter.ts:866-891). So the real budget is compaction latency + time-to-first-token < 120s, shared — and a compaction pass over a threshold-sized context can exceed that. When it does, the user gets a hard no data received from provider for 120s, not a fallback. Verified with a probe: streamAnthropicResponse with idleTimeoutMs: 50 over a fetch that sleeps 250 ms throws, while the same fetch returning immediately streams fine.
Consequence: RESPONSES_COMPACT_TIMEOUT_MS = 10 * 60_000 is unreachable dead code — nothing sets compactTimeoutMs, and the 120s watchdog always fires first.
Ask: give compaction its own budget well under the downstream idle timeout and treat expiry as a fallback, and/or emit a keepalive frame before starting so the downstream timer resets. Also worth a user-visible signal: this is a second billed inference on the user's plan, inline at the front of their turn, and today the only trace is ws_compaction diagnostics behind --trace/--ws-diagnostics.
4. BLOCKING (small) — The retained-message budget re-introduces the bytes/4 image miscount
approximateItemTokens and truncateRetainedUserMessage (:495-529) both budget with canonicalJson(value).length / 4. This repo's CLAUDE.md documents that exact miscount by name: base64 tokenizes at ~1.5 chars/token, which is why estimateAnthropicInputTokens uses a flat vision estimate instead — inline screenshots previously killed agents with "Prompt is too long" while the local estimate showed about half the real count.
Verified: a single input_image user message carrying 250,000 base64 chars survives the rebase verbatim, because len/4 = 62,515 fits under the 64,000 budget while len/1.5 > 150,000. So the "compacted" input can land back above the threshold, lastInputTokens on the new head stays at or over it, and the next turn fires another hidden full-context compaction — a per-turn billed inference loop.
Codex, for what it's worth, counts text parts only and explicitly counts images as zero (message_text_token_count), so matching it fixes this. Two smaller unit bugs in the same function: the outer loop budgets in canonical-JSON tokens while the inner loop budgets raw text.length, and when available < marker.length the emitted part is the 57-char marker alone — larger than the remaining budget.
5. BLOCKING (small) — Nothing in CI defends the feature being on
I disabled the feature outright at all three injection points — hardcoding openAiCompactThreshold: undefined in proxy.ts and server/router.ts and compactThreshold: undefined in provider-factory.ts — and stripped estimatedInputTokens, forceCompaction from all four withResponsesWebSocketDiagnosticContext calls, all at once. The full suite stayed 1037/1037 green. The PR's central claim ("enabled by default at 90%") and the entire forceCompaction arming path have no test at any level, and ProviderModelSpec is already an injectable seam, so this is cheap.
Other guards I mutated that left the suite green, in rough priority order: the whole previousHead restore block (void previousHead → 117/117, though a probe driving a response.failed on the post-compact turn does fail without it, so the code is load-bearing); moving the supersededEntry deletion earlier, which breaks the "never promote before response.completed" invariant your own CLAUDE.md entry states; the estimated_threshold branch entirely; the checkpoint caps raised to 1,000,000; saveCompactionCheckpoint removed from deleteEntry; the empty-delta guard; MIN_CLAUDE_COMPACTION_SUMMARY_CHARACTERS 32 → 0; and the threshold env validation relaxed so -1 and 1.5 are accepted.
One test is self-referential: tests/responses-compaction.test.ts:88 asserts expect(body).toEqual(compactRequestPayload(payload)) — the sent body against the function that produced it. Deleting tools, parallel_tool_calls, and reasoning from COMPACT_BODY_FIELDS leaves 57 tests passing. Please pin the literal expected field set; it's the protocol-critical constant in that file.
6. BLOCKING (doc accuracy) — docs/ ships in the npm tarball
docs is in package.json files, so these go out to users:
- The headline numbers aren't reproducible from the committed probe.
probeIntegratedCompactionusessyntheticInput(40)≈ 9,349 chars (~1.7–2.3k tokens); 7,665 logical tokens requires the ~180-paragraph payload, roughly 4× larger. And the "455-byte incremental trigger" isn't measured at all — the script's only byte counter isBuffer.byteLength(await response.text()), i.e. the received SSE stream of the post-compaction response. Please re-measure with the script as committed, or state the parameters. - The 90.2% figure describes the trigger request's own usage, which is cache-warm by construction because it rides
previous_response_id. It is not the steady state: the fresh chain has a new prefix, so the retained window is a cache write (billed at 1.25× uncached on GPT-5.6-family). Your own test models exactly that (input_tokens: 120, cached_tokens: 75). "The trigger transport is efficient" is well supported; "the feature is cache-efficient end-to-end" needs the one-time write cost stated alongside it. The recurring steady-state win is real — please just separate the two claims. - "An absent, short, malformed, duplicated, or non-matching summary never selects the opaque head" is false for the duplicated case. A probe with the envelope text repeated twice selects the head (
decision=continuation mode=claude_compaction_summary) and forwards the second copy as delta. Not unsafe — the head is the right one — but the sentence overstates. - Making standalone compact recovery-only is the right ordering and your "two identical 7,613-token compact calls both reported zero cached input" measurement is sound in design (payload and
prompt_cache_keybuilt once and reused), so that part of the argument stands.
Non-blocking
retainedUserMessagesshould keepdeveloperandsystemitems too. Codex retains all three roles (matches!(role.as_str(), "user" | "developer" | "system")incompact_remote_v2.rs). This is reachable here: clodex deliberately emits inlinerole:'system'messages, and@ai-sdk/openaiconverts those to{role:'developer'}input items for reasoning models — so the triggering turn's trusted reminders are silently dropped from the first post-compaction request and survive only summarized inside the opaque item. One-line fix, and it's a genuine fidelity gap against the protocol you're mirroring.- Is the restored pre-compaction head actually usable? The websocket-mode guide says the service keeps one previous-response state per connection (the most recent response) and evicts the referenced
previous_response_idwhen a turn fails. If that's right, after a successful trigger the slot holds the trigger's response and after a failed one the old id is evicted — so the restored head could only ever answerprevious_response_not_found, and the trigger-failure path that falls back tocontinuationon that same entry (:1979) guarantees one wasted round trip before the existing full-context retry heals it. I couldn't measure this. If you've confirmed otherwise on a live socket I'd like to hear it; if not, consider dropping the restored head and not describing it as a transactional fallback. - A rejected opaque compaction item has no escape hatch. On the checkpoint path
retryPayload = sendPayloadandcontinuedstays false, soresetContextForRetryreplays the compacted input and theprevious_response_not_foundretry (which requiresctx.continued) never applies. A rejected item surfaces to Claude Code as an error and the same checkpoint is re-selected next time. I could not confirm whether compaction items can expire — no OpenAI doc states a TTL or an error code, and Codex persists them into rollout files, which argues for durability — but the prior art you cite caps its own state at 30 minutes. - The summary-text extraction doesn't match the client's.
assistantOutputTextconcatenates every text part of every assistant item; Claude Code takes the last assistant message containing<summary>and its first text part. Identical for single-message/single-part output, divergent otherwise, and the anchor then falls through with no distinct diagnostic. Consider mirroring the client and emitting anoutcome:'anchor_missed'event so drift is observable — right now nothing tells a user the anchor stopped matching, which will happen eventually since these are internal client strings. - A persistently failing post-compact turn re-fires the trigger every turn with no backoff —
supersededEntryis released only on success, so the pre-compaction head stays registered withlastInputTokensstill over threshold. A probe confirmed a secondcompaction_triggeron the third request. deleteEntry(ctx.supersededEntry)deposits a checkpoint holding the pre-compactioncompactedInput, which can resurrect stale canonical input if the newer checkpoint is evicted. Consider skipping the save when an entry is retired because it was superseded.- In the standalone path,
sendPayload = { ...payload, input: result.output }trusts the endpoint to echo the retained turns; if it ever returned only the opaque item the user's current request would silently vanish. A cheap assertion thatresult.outputcontains a non-opaque item would close that. compactHeadersdeletesOpenAI-Betaentirely when the websocket marker was its only value; Codex still sends its beta-features header on the compact call. Harmless today.scripts/probe-openai-compaction.tssits outsidetsconfig.json'sinclude: ["src"], so it's never typechecked and will rot. It compiles clean today. The guarding is good —CLODEX_LIVE_COMPACTION_PROBE=1, synthetic text only, hashed error messages, not shipped, andpnpm testcan't reach it.CLODEX_OPENAI_COMPACT_THRESHOLDis described as a test override in bothCLAUDE.mdand the doc, but the code makes it a fully general production override read on every request path in both bridge modes. Worth saying so.
Where this leaves us
I want to be straight with you rather than hand back a list and imply that ticking it off lands the PR. The protocol fidelity and the state-machine engineering here are genuinely good — better than the length of this review suggests — but my concern is structural, not a set of defects.
The feature works by managing the OpenAI-side context without telling Claude Code. That concealment is not incidental; it is the mechanism. And it's what makes items 1 and 2 two faces of the same problem: Claude Code can't keep a transcript bounded that it was never told is growing, and it can't write a good summary of a conversation it was handed in digested form. Items 4–6 are ordinary and small. Item 3 is a real bug with a clear fix. Items 1 and 2 are the design.
So the bar I'd set for any version of this: Claude Code's context accounting has to stay honest, or the Anthropic-side transcript has to be bounded some other way, or this feature must be opt-in and its risks documented for the user. As long as reported usage collapses after a native compaction, the transcript grows unbounded and the first head miss is unrecoverable — and head misses are routine, not exotic (--resume, TTL expiry over lunch, /rewind, /fork, a /model switch, checkpoint eviction). I don't see a shape that delivers the OpenAI-native-compaction benefit and keeps that accounting honest, since the whole point is to avoid the compaction Claude Code would otherwise perform.
But the remaining premise — that OpenAI-native compaction may genuinely be better for OpenAI models — is worth exploring, and I don't want to block exploration. So here is the concrete path to merge:
- Ship it off. Native compaction must be opt-in, not enabled by default. Invert the env var so the feature requires an explicit opt-in rather than an explicit
=0to disable. - When off, do not touch today's behavior — at all. With the feature disabled, every inference call to the provider must be byte-identical to
main, including Claude-initiated compaction, which must continue to work exactly as it does today. I believe this is close to true by construction already (aundefinedthreshold skips the compaction block,retryPayloadstays undefined,saveCompactionCheckpointearly-returns withoutcompactedInput, and thehistoryContinuationMatchrefactor is byte-equivalent on pre-PR data), but nothing proves it — which is item 5. Please add a test that pins it: feature off, assert the emitted frames match the pre-PR expectations. That test is what lets me take this without risk to the default path. - Document the failure modes in a user-facing doc. It needs to tell a user how to configure Claude Code to avoid them (configure an auto-compact window so the client keeps compacting) and which commands and habits will brick a session if they opt in —
/modelor effort switches,/rewind,/fork,/btw,--resume, leaving a session idle past the socket TTL.docs/native-codex-compaction.mdis currently a design document; this needs to be written for the user who is deciding whether to turn it on. - Point at it from
README.md. The doc will be too long to inline, so keep it separate, but add a pointer with an inviting label — something like "How to configure OpenAI compaction".
With those four in place plus items 2–6, I can merge this as an experimental, off-by-default feature, and we can gather the evidence that would justify making it the default later. That seems like the right way to keep the idea alive without shipping the failure modes to everyone.
The things that would most move me toward eventually defaulting it on:
- the post-compaction input token count on both paths (
entry.lastInputTokensright after a compaction, versus the same number after a plain Claude Code compaction onmain) — this is the cheapest and most decisive measurement available, and if the PR's number is the larger one then the feature costs headroom rather than saving it; - a summary-quality comparison showing OpenAI's opaque digest actually resumes better than Claude Code's text summary for these models (this is the feature's real thesis, and it's currently unmeasured);
- a design where the Anthropic-side transcript stays bounded — e.g. clodex continuing to report logical, pre-compaction token counts upward so Claude Code compacts on its own schedule, with native compaction as a transparent optimization underneath;
- live evidence on the restored-head semantics or on whether compaction items can expire, either of which would change some of my reasoning above.
One last note on tone, since I've spent a lot of this review disputing the framing rather than the code. None of that is doubt about the work — the protocol research here is the most thorough I've seen in a contribution to this repo, and the implementation matches it. What I'm pushing back on is the description: the "Why" doesn't match what main does, and the "Safety and efficiency" section states properties that turn out to be either unmeasured or, in a couple of places, contradicted by the code. Getting those claims right matters more than usual here, because they're what a future maintainer will read to decide whether it is safe to touch this. Fix the claims, ship it off by default with the risks written down, and I think this lands.
5c6f4ec to
340a45c
Compare
cgasgarth
changed the title
|
Addressed the review in Blocking items
Additional fixes
Points where I did not apply the suggested change
Validation on Node 26.3.0 / pnpm 10.34.5:
|
bman654
left a comment
bman654
left a comment
There was a problem hiding this comment.
This is a substantial improvement over round 1 and it addresses the merge path in full. I verified all four requirements independently and three of them are mutation-proven. What's left is small: one wiring test, one misleading test name, and two doc corrections. Do those and I'll merge.
Thank you for taking the reframing seriously — inverting forceCompaction from a forcer into a suppressor was the right call and it removes my central objection.
The four requirements — all met
1. Ships off. CLODEX_OPENAI_COMPACTION is the only enable switch. compactThreshold reaches the transport from exactly one place, fed by two call sites, both through resolveOpenAiCompactionThreshold, which fails closed on unset, 0, and unrecognized values. CLODEX_OPENAI_COMPACT_THRESHOLD can't enable the feature on its own. Inverting the gate to default-on reddens a named test.
2. Off-path byte-identical to main. I built a differential harness running this branch's module against a copy of main's behind the same fake ws, capturing the socket URL, the upgrade options, every send() frame, and the SSE body. Three scenarios — a plain continuation, a forceCompaction: true Claude compaction turn plus the post-compaction rewritten-transcript turn, and a previous_response_not_found retry with rewind — all byte-identical.
The test at tests/responses-websocket.test.ts:1221 genuinely pins this: it reddened for both mutations I injected, and it was the only test in the file to catch a stray field on the continuation payload. The other continuation tests assert on sub-fields and stayed green. Good test.
3 and 4. Docs. Every number in docs/native-codex-compaction.md reproduces against the source — the 90% threshold, the 64K retained budget, the 60s and 120s timeouts, the 30-minute TTLs, the 8/32 caps, and the threshold parsing. Round 1's unreproducible figures and the false sentence about duplicated envelopes are gone, and that behavior is now real and pinned by a test. The bricking-ops list is complete. The README pointer is in place and the doc ships in the tarball via package.json files.
tsconfig.scripts.json does what it claims: injecting a type error into the probe script fails pnpm typecheck with exit 2, where the base tsc invocation alone misses it. The probe is env-guarded, unimported, outside the vitest glob, and not in the published tarball.
Round-1 blockers — resolved
forceCompactionno longer degrades every summary. It now suppresses native compaction on Claude-initiated turns, so/compactand reactive compaction go through the normal path. Mutation-proven by two tests. The/compact→canonicalInputundefined → full-transcript path is closed.- The bytes/4 retained-budget miscount is fixed with a flat per-image estimate, mutation-proven. The residual non-text/non-image case (
input_filecharging zero tokens) is unreachable from clodex today — worth a comment, not a fix. - 24 genuinely new tests, nothing renamed away, and an 11/13 mutation kill rate.
Please fix before merge
1. Pin the forceCompaction wiring. Severing the threading at both injection points in src/proxy.ts and src/server/router.ts leaves the entire suite green. That means round 1's worst behavior — hidden compaction on every Claude-initiated compaction turn — can come back in a future refactor with no test failing. This is the one item I'd insist on; roughly 20 lines asserting the flag actually arrives at the transport from both entry points.
2. tests/responses-websocket.test.ts:2182 doesn't test what its name says. does not re-fire compaction after the post-compact response fails passes only because its scaffolding omits the estimatedInputTokens that production always supplies. With a production-faithful setup, compaction does re-attempt on subsequent failing turns via the standalone /responses/compact fallback, minting a fresh opaque item each time.
To be clear about severity, since I checked this carefully before raising it: this is not a hang. One successful post-compact turn fully recovers, standalone-compact failure falls through to a normal full-context send, and /compact is an in-session escape. It needs a persistent upstream contract violation to persist, and it announces itself with visibly failing turns. So the behavior is acceptable — it's the test name that's wrong. Either rename it, or make it pin the real retry-then-recover behavior. I'd prefer the latter.
3. Two doc corrections.
- The trigger budget (60s) and the standalone-compact budget (60s) run sequentially inside a single fetch, so the worst case is exactly the 120s
SDK_STREAM_IDLE_TIMEOUT_MSwatchdog — not comfortably inside it. Both the code comment anddocs/native-codex-compaction.md:107-109currently say otherwise. Either stagger the budgets or fix both statements. - Please state plainly that Claude Code performs no proactive auto-compaction for OpenAI model ids unless the user has configured it (the model-default window table is Anthropic-only), and that an oversized transcript combined with state loss is not recoverable in-session. The doc is currently soft on the second point, and it's the failure a user most needs to understand before opting in.
Non-blocking
- Checkpoint TTL and socket idle TTL are both 30 minutes and are pruned in the same pass, so checkpoints contribute zero additional recovery for the idle-expiry path. Dropping the checkpoint TTL below the socket TTL would make them actually useful; today the doc implies protection that doesn't exist.
retainedUserMessagesstill keeps onlyrole === 'user'items. Codex retainsdeveloperandsystemtoo, and our inlinerole:'system'messages becomedeveloperitems, so those are being dropped from the retained window. Still a one-line fidelity fix.triggerWireBytesdouble-serializes even with diagnostics off. Measured cost is negligible (~0.05 ms/request) but it's avoidable.compactTimercould use.unref().- Still no backoff on repeated trigger failure.
- The rejected opaque item has no TTL, invalidation, or escape hatch.
- Not pinned by tests:
utf8Suffixsurrogate handling, each duplicate-envelope guard individually (only in aggregate — the outer one looks redundant), and the 60s constant itself.
Still open from round 1
The post-compaction entry.lastInputTokens measurement. The chain is instructions + tools + [retained tail ≤64k] + [opaque] + [summary] + [delta], which can carry the same content up to three times, and nobody has yet shown whether that's smaller than what main sends. It needs a live session, so I'm not going to block on it — but if you can capture that number once with the feature on, it would settle the last real question about whether the feature achieves its goal.
Housekeeping
d2a1cd8a adds src/codex-app-server.ts and the following commit deletes it, so the file isn't in the final state. No action needed — this merges as a squash, so only the PR title reaches the changelog. Mentioning it only so you know I noticed rather than missed it.
Tests 1050/1050, typecheck clean, build clean, commitlint clean, no dist/, no version or changelog edits.
|
Addressed the required items from review 4783588321 in 2cb01da:
Also added the operational optimization discussed in fork issue #9: recognized Claude portable-summary turns keep the selected model/cache/tool prefix but use low OpenAI reasoning and toolChoice: none; normal turns retain configured effort. Validation on the exact upstream branch:
I kept the non-blocking suggestions out of this revision to avoid broadening the merge-critical patch. |
bman654
left a comment
bman654
left a comment
There was a problem hiding this comment.
Asks 2 and 3 are done well, and ask 1 is half done. The native compaction feature itself is in good shape and close to shipping.
But round 3 also adds an unconditional change to Claude Code's compaction path in src/sdk-adapter.ts — forcing reasoningEffort: 'low' and widening the compact-request detector — and I'd like you to take it out of this PR.
The main ask: remove the effort/detector change from this PR
This is a pivot away from what the PR has been about for three rounds. Everything else here is native OpenAI compaction behind an opt-in flag; these two lines change how Claude Code's own compaction turns are translated, for every OpenAI user, whether or not they enable the feature. That's a different feature with a different audience and a different risk profile, and reviewing it as a rider on this one is holding up work that's otherwise ready.
Removing it is clean: reverting both changes to main semantics reddens exactly one test — the round-3 test that deliberately pins reasoningEffort: 'low'. 1082/1083 stay green. Nothing in the compaction feature depends on them.
Do that, add the one test in "Still open" below, and I think we're done — the native compaction feature ships.
I'm not dismissing the underlying idea. If the structured-output-tool problem is real, and I believe it is, please open it as its own PR with that focus. Below is everything I found, so you have it in hand when you do.
Why it can't ship as written
It's ungated, and it breaks the merge condition we agreed on
-const compactRequest = isClaudeCodeStructuredOutputCompactRequest(body);
+const compactRequest = isClaudeCodeCompactRequest(body);
-const effort = anthropicEffortFromRequest(body) ?? options?.defaultEffort;
+const effort = npm === '@ai-sdk/openai' && compactRequest ? 'low' : configuredEffort;Neither is behind CLODEX_OPENAI_COMPACTION, and structurally neither can be: grep "process.env" src/sdk-adapter.ts returns nothing, and TranslateRequestOptions has no compaction field — the flag only reaches spec.openAiCompactThreshold.
A differential against main over six inputs and five configurations shows the divergence is narrow and clean — exactly two fields, only on compaction envelopes: toolChoice auto→none and reasoningEffort high→low. Ordinary requests, including ones carrying a StructuredOutput tool, are byte-identical. But with the feature off, a Claude Code compaction turn on a plain API-key OpenAI route now gets reasoningEffort: 'low' overriding an explicit high. That's requirement 2 broken, for users who can't use this feature at all.
The requirement-2 test can't see it — it lives at the WebSocket layer and never imports sdk-adapter.
To be clear for the future PR: CLODEX_OPENAI_COMPACTION would be the wrong gate, since this targets Claude Code's own compaction rather than the native path. It needs its own option, threaded through TranslateRequestOptions, and the default must preserve Claude Code's behavior — summaries produced at the same effort the model is using for everything else. Cheap summaries should be something a user opts into.
npm === '@ai-sdk/openai' isn't a discriminator
clodex ships exactly two provider templates and both declare that package, so the condition is always true in practice. It doesn't separate OAuth from API key — and options?.openAiOAuth, which does, is used five times in the same function. It also excludes @ai-sdk/openai-compatible, which contradicts the stated rationale.
Forcing low guarantees a chain-head miss, and the cost is severe
Effort is part of the WebSocket partition key, and the override runs before partition selection: translateRequest sets reasoning.effort, the payload is serialized into init.body, and only then does the WS layer parse it back and call responsesWebSocketPartitionKey(...), which hashes [wsUrl, providerId, accountId, model, effort, prompt_cache_key, authFingerprint]. Connections are Map<partitionKey, Set<ConnectionEntry>>, heads live in those entries, and the file's own comment says a partition's heads "all share its model/effort/cache key."
So a session at high whose compaction turn is forced to low lands on a different partition, finds no head, and can't send previous_response_id. On a realistic 244k-token transcript:
| request | items | continuation | |
|---|---|---|---|
main |
487 B | 1 | previous_response_id set |
| this branch | 976,610 B | 242 | none, plus a second socket |
2005× larger, on the biggest request in the session. Compaction checkpoints are per-partition and become invisible, and the head registered in the low partition is orphaned — the next normal turn resumes from the pre-compaction head. It also defeats this PR's own in-band trigger, which needs that head.
One thing I could not settle: whether OpenAI's public-API prompt cache also keys on reasoning.effort. My harness couldn't establish a stable baseline — two same-effort control turns read 7936 and 0 cached tokens — so I'm explicitly not claiming it either way. If the new PR keeps this behavior on the API-key path, that's worth measuring.
Widening the detector makes the deleted comment's warning live
The comment you removed said: "Keep this narrower than the generic compaction detector. If Claude Code changes the observed envelope, fail open rather than stripping tools from an ordinary request."
That was load-bearing. I built an ordinary request that merely quotes the two envelope markers in its text, and tools were silently disabled on all five configurations. main handles that input correctly. Someone pasting a transcript excerpt, or discussing these strings, shouldn't lose their tools.
Still open from ask 1
The wiring is pinned now at both injection points independently — severing in proxy.ts reddens only the proxy test, severing in server/router.ts reddens only its twin. That's exactly what I asked for.
What's still missing is the behavior: replacing forceCompaction with false in the suppression ternary at responses-websocket.ts:1962 leaves 1053/1053 green, which restores round 1's worst defect — hidden compaction_trigger on every Claude compaction turn. That's the doc's headline safety claim, so it needs a test. The inverse mutation (forceCompaction always true) also survives. This is the one remaining item I'd hold the merge on.
Non-blocking
- Both
docs/native-codex-compaction.mdand theRESPONSES_COMPACT_TIMEOUT_MScomment attribute the 120s watchdog to Claude Code. It's ours, insdk-adapter.ts. - Mutation score for the new tests is 8/11; the three survivors are the three highest-value properties. The new tests are 3 new plus 2 strengthened with no filler — the gap is absence, not padding.
- Still open from earlier rounds:
retainedUserMessagesdropsdeveloper/systemitems; checkpoint TTL equals socket idle TTL so checkpoints add no recovery on the idle path; no backoff on repeated trigger failure.
Verified this round
Ask 2 is met: the renamed test now supplies estimatedInputTokens (50/150 against a threshold of 100), that estimate is load-bearing, and mutating the recovery path reddens it. Ask 3 is met and the numbers reproduce — 60s trigger plus 60s standalone against a 120s watchdog. I independently re-checked the Claude Code claim in the 2.1.220 binary, enumerating all six aY exits; both model-default tables are Anthropic-only, which your new doc text states correctly. Gates clean: 1053 tests, typecheck, build, and commitlint all pass, no dist/ or version edits.
|
A related cache-affinity improvement is implemented and merged on my fork as cgasgarth/clodex#12. It retains bounded WebSocket heads for parallel Claude workflow branches, tolerates replayed/reshaped opaque reasoning when matching continuations, supports Claude's 16-agent concurrency ceiling, and reuses warm nursery sockets across workflow waves. Benchmark results from that PR:
Fresh live evidence from the current long-running Claude session after installing the fork change (2026-07-27 07:31–08:02 UTC):
The same logs also show a directional end-to-end speed improvement:
Those timings include input processing and output generation; they are not raw model decode TPS. The before/after workload mixes also differ, including median logical context of 162.7k before vs 55.4k after, so the speed result is directional rather than a controlled causal measurement. Cache hits are consistent with lower input-processing and time-to-first-token overhead, but these logs cannot isolate that contribution. The fresh post-install window has not launched a dynamic Workflow agent, so it confirms strong parent/ordinary-subagent behavior but does not replace the dedicated workflow benchmark above. This is large enough, and touches enough of the same Responses/WebSocket transport, that I do not think it should be folded into this already substantial compaction PR. If #53 is merged, I can port the cache-affinity work cleanly onto upstream |
2 participants
Summary
Adds opt-in, cache-efficient OpenAI Responses compaction for ChatGPT OAuth models.
CLODEX_OPENAI_COMPACTION=1.compaction_triggerfor a matching live head andPOST /responses/compactonly as fallback.Before
After
Guardrails
forceCompactionsuppression is pinned at both request entry points.Validation
pnpm test: 81 files, 1,053 testspnpm typecheckpnpm buildpnpm pack --dry-runAuthorship
Authored by GPT-5.6 Sol with xhigh reasoning, under @cgasgarth's direction and review.
Portable-summary anchoring was informed by
raine/claude-code-proxy.