Fix OpenAI OAuth compaction routing, stop compaction failures from killing turns, and anchor estimation on real usage - #190
Merged
Conversation
The OAuth URL rewrite returned a single constant for every Responses-shaped path, so `/v1/responses/compact` resolved to `.../backend-api/codex/responses` with the `/compact` suffix silently dropped. Dedicated compaction bodies were therefore posted to the streaming turn endpoint, which rejects them with `Store must be set to false` — making automatic compaction unreachable for OAuth sessions. Rewrite per-endpoint instead, matching the reference Codex client's `.../backend-api/codex/responses/compact`. The explicit match subsumes `is_chatgpt_codex_rewrite_path` and `is_responses_path_or_child`, so both helpers are gone and unmapped paths still fall through to the configured base_url. `store` is deliberately not added to the compaction body: the dedicated endpoint does not take it, and the reference client omits it too. Adding it would have masked the routing defect while leaving /compact unreachable. The existing test asserted the collapsed behavior; it now asserts each endpoint keeps its own path. Also adds the wire-level coverage the OpenAI compaction codec never had — only the OpenRouter encoder was tested — including encode shape, `store`/`stream` absence, prefix carry-forward, wrong-api-kind rejection, and decode success/failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`plan()` propagated every compaction failure, so once a session crossed the compaction threshold any provider-side problem failed the turn — and context only grows, so the next turn failed the same way. Combined with the OAuth routing defect this made affected sessions unrecoverable. Automatic compaction is now best-effort: an unsupported provider, a missing compaction window, or a failing `Provider::compact` returns the uncompacted state carrying the reason, which `run_turn` publishes as `SessionEventPayload::Warning`. Manual `compact()` still propagates — the caller asked for compaction explicitly, so a silent no-op would misreport the result. `ContextPlan` gained `compaction_warning` (serde-defaulted, so existing event logs still deserialize) and `CompactionOutcome` gained `compaction_error` to carry it out of the engine. The three failure exits now route through one `degrade_or_fail`, and the four repeated "nothing compacted" outcome literals collapse into `uncompacted_outcome`, so the engine body is shorter than before. Tests cover both sides of each branch: degrade on provider error, unsupported provider, and absent window; no warning on success; and propagation on both manual paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Compaction triggering ran entirely off a character heuristic (chars * 10/37, documented as +/-20% for code-heavy text) applied to the whole transcript, so the error compounded with context size — exactly where the threshold decision matters most. Providers report the true context size with every assistant response. `estimate_context_tokens` now scans back for the most recent usable report, takes it as ground truth, and estimates only the messages after it, bounding heuristic error to one turn's tail. The report already covers the prompt, summaries, and compacted prefix, so those are not added on top of an anchor. Sessions with no usable report — fresh, or all turns failed — fall back to the previous whole-transcript estimate. Interrupted turns, errored turns, and zero-token reports are rejected: they describe a partial request and would peg the estimate far below the real context, stopping compaction from ever firing. Two hazards this surfaced: Compaction preserves a message tail, and the assistant message in that tail still reports the *pre*-compaction context. Anchoring on it would make every following turn see the old figure and compact again, indefinitely. `SessionState::usage_anchor_floor` bounds the scan; compaction advances it past the preserved tail in both the runtime and the event fold so replayed and resumed sessions agree. Forked subagents start with no anchor, since the parent's reports describe a different system prompt and tool set. The providers used opposite cache-token conventions: OpenAI's `input_tokens` includes cached tokens, Anthropic's excludes them. No single formula is correct for both, and plain `input_tokens` under-counts a mostly-cached Anthropic prompt badly enough to delay compaction past the context window. `Usage::input_tokens` is now defined as total input including cache traffic, with the cache counters kept as breakdown subsets, and the Anthropic decoder normalized to match. This changes reported Anthropic input totals, including accumulated `usage_so_far`, for turns recorded after this change. Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Three related compaction fixes, one per commit, each independently green.
Originated from comparing halter's compaction against two reference implementations —
earendil-works/piandopenai/codex— after a report that OpenAI OAuth compaction was failing withStore must be set to false.1. Route ChatGPT OAuth compaction to
/responses/compact(245ff6d)The reported bug, but the root cause was upstream of the reported symptom.
provider_urlreturned a single constant for every Responses-shaped path under OAuth:is_responses_path_or_childmatches/v1/responsesand every child, so/v1/responses/compactresolved to.../backend-api/codex/responseswith the/compactsuffix silently dropped. Dedicated compaction bodies were posted to the streaming turn endpoint — which is exactly the endpoint that rejects them withStore must be set to false. An existing test asserted the collapsed behavior, so it locked the bug in.Now rewritten per-endpoint, matching the reference Codex client's
.../backend-api/codex/responses/compact. The explicit match subsumes both helper predicates, so they're deleted; unmapped paths still fall through to the configuredbase_url.storeis deliberately not added to the compaction body. The dedicated endpoint doesn't take it — codex'sCompactionInputomits it too — and adding it would have masked the routing defect while leaving/compactunreachable.Also adds the wire-level coverage the OpenAI compaction codec never had (only the OpenRouter encoder was tested): encode shape,
store/streamabsence, prefix carry-forward, wrong-api-kind rejection, decode success and failure.2. Degrade instead of failing the turn when auto-compaction cannot run (c0b9570)
plan()propagated every compaction failure via?. Once a session crossed the compaction threshold, any provider-side problem failed the turn — and since context only grows, the next turn failed identically. Combined with #1, affected OAuth sessions were unrecoverable.Automatic compaction is now best-effort. An unsupported provider, a missing compaction window, or a failing
Provider::compactreturns the uncompacted state carrying the reason, whichrun_turnpublishes asSessionEventPayload::Warning. Manualcompact()still propagates — the caller asked for compaction explicitly, so a silent no-op would misreport the result.The three failure exits route through one
degrade_or_fail, and the four repeated "nothing compacted" outcome literals collapse intouncompacted_outcome, so the engine body is shorter than before despite the added behavior.3. Anchor context estimation on provider-reported usage (f59ae19)
Compaction triggering ran entirely off a character heuristic (
chars * 10/37, documented as ±20% for code-heavy text) applied to the whole transcript — so error compounded with context size, exactly where the threshold decision matters most. Both reference implementations instead drive off real provider usage.estimate_context_tokensnow scans back for the most recent usable report, takes it as ground truth, and estimates only the messages after it. The report already covers the prompt, summaries, and compacted prefix, so those are not added on top of an anchor. Sessions with no usable report fall back to the previous whole-transcript estimate. Interrupted turns, errored turns, and zero-token reports are rejected — they describe a partial request and would peg the estimate below the real context, stopping compaction from ever firing.Two hazards surfaced while implementing this:
A compaction loop. Compaction preserves a message tail, and the assistant message in that tail still reports the pre-compaction context. Anchoring on it would make every following turn see the old figure and compact again, forever.
SessionState::usage_anchor_floorbounds the scan; compaction advances it past the preserved tail in both the runtime and the event fold, so replayed and resumed sessions agree. Forked subagents start with no anchor, since the parent's reports describe a different system prompt and tool set.Opposite cache-token conventions. OpenAI's
input_tokensincludes cached tokens (cached_tokensis a subset); Anthropic's excludes them (separate sibling counters). No single formula is correct for both — plaininput_tokensunder-counts a mostly-cached Anthropic prompt badly enough to delay compaction past the context window.Testing
18 new tests. Every suite green (26 binaries),
cargo fmt --checkandcargo clippy --all-features --all-targetsclean.I verified the two load-bearing guards by deliberately reintroducing each bug:
/compactsuffix → the routing test failsdegrade_or_failto alwaysbail!→ both degradation tests failusage_anchor_flooradvance → the compaction-loop regression test failsEach commit was checked out individually and the full suite run against it, so the history bisects cleanly rather than only being green at the tip.
Open risk
Codex sends
originator,session_id,x-codex-installation-id, and attestation headers to the ChatGPT backend; halter sends only the bearer token. If/responses/compactis gated on request provenance, OAuth compaction will now fail differently rather than succeed. I can't verify this without a live OAuth token.Commit 2 is what makes that acceptable: the failure degrades to a warning instead of taking the turn down. #189 tracks the fallback — a streaming compaction-trigger strategy that needs no dedicated endpoint.
Not included
docs/is gitignored in this repo, so the call-graph documentation I wrote (token estimation, failure handling, OAuth endpoint routing) is not in these commits. Happy to open a separate change if you want those tracked.🤖 Generated with Claude Code