feat(responses): route Codex compaction to a configured model for the triggers you name - #5202
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (53)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
…fort
Adds an optional `manualCompaction` setting (`{ model, reasoningEffort? }`) that
sends Codex's manual `/compact` request to a different model while every other
request stays on the conversation's model. Without the setting nothing changes.
A long conversation on an expensive model that sits idle past the provider's
prompt-cache window re-reads its whole context at the uncached input price on the
next request. Running `/compact` on a cheap model pays that one full uncached read
at cheap-model rates, and the expensive model then resumes on the compacted
context. Codex offers no per-command model selection, and OpenCodex previously
routed the compaction request exactly like an ordinary turn.
The override fires only for requests whose `x-codex-turn-metadata` carries
`request_kind: "compaction"` and `compaction.trigger: "manual"`, and on
`/v1/responses` only when the input also carries a `compaction_trigger` item.
Automatic compaction, ordinary turns, and malformed or absent metadata are
untouched. When the selected model leaves the conversation's provider identity the
caller credential is treated as rewritten and the portable summarizer runs, because
native `/responses/compact` ciphertext replays only on the backend that minted it.
Carried from #4872 and rebased onto the current dev tip: `compactHandoffRoute`,
`rememberCompactHandoffRoute` and `forgetCompactHandoffRoute` now take an
`admission` argument, `config-routes.ts` gained `fastRows`, and `diagnostics.ts`
gained `spendSchema`. Thirteen `structure/` owners took an append-at-end
resolution keeping both sides.
Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
…rride Renames `manualCompaction` to `compactionRouting` and adds `triggers`, the set of Codex `compaction.trigger` values the override covers. Omitting `triggers` means `["manual"]`, so a block written for the previous key behaves exactly as before and automatic compaction keeps routing where it routes today. #5012 asks for an explicitly routed compaction provider when the canonical OpenAI quota is exhausted. The reported failure is a thread resume that enters PreCompact, so the request the proxy rejects with 429 is an automatic compaction, not a manual `/compact`. codex-rs builds that turn in `compact_remote_v2::run_inline_remote_auto_compact_task` with `CompactionTrigger::Auto` and hands it to the same `run_remote_compact_task_inner` the manual `CompactTask` uses, so it reaches the identical surfaces — a `compaction_trigger` item on `/v1/responses`, or `/v1/responses/compact` — and differs only in the trigger string. The previous commit's gate required `"manual"` exactly, so it could never fire for the reported case. That makes one setting the right shape rather than two. `routeCompactionModel` reserves a bare native compaction model for an enabled canonical `openai` provider and releases it only when none is configured (#2901), never on quota exhaustion. Naming `"auto"` points the compaction at a provider-qualified model with its own credentials, which is the whole of the request; a second config block would have duplicated the model, effort, combo and portable-summary handling already here. Trigger metadata copies must now agree on which trigger they carry, not merely that the request is a compaction, so a caller cannot widen an override by disagreeing with itself. A `triggers` value the schema would reject disables the block instead of widening it, matching how a malformed `model` or `reasoningEffort` already behaves. `warnDegradedCompactionRouting` moves behind `warnDegradedTopLevelOptIns` so `loadConfig` gains no line: `src/config.ts` sits at its 460-line cap, and the previous commit stayed under it by folding two statements onto one line. Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
Five adversarial review passes over the two commits above found one build break, one data-exposure defect, and two tests that could stay green while broken. `gui/src/i18n/vi.ts` is `Record<TKey, string>` and compile-checked, and it landed on `dev` after #4872 was cut, so the branch was 20 keys short: the nineteen `compactionRouting.*` keys and `models.reasoningEffort.ultra`. The GUI build fails on exactly the union-exhaustiveness class AGENTS.md describes, where each side is correct alone and the merge is not. Vietnamese now carries all twenty. A cross-identity override now sets `_stripReasoningEncryptedContent`. The destination shares neither the credential nor the backend that minted the conversation's reasoning ciphertext, so it cannot verify it; forwarding it sends backend-private state across a provider boundary and can fail the summarizing turn on a target that rejects unverifiable blobs. This is the condition `account-change-state.ts` already reports for a changed serving identity, and `scrubOcxCompactionItems` turns a stored summary into readable text rather than dropping it, so the summarizer keeps its input. The automatic-trigger acceptance test now runs over both v1 and v2: they are separate entry points with separate gates, and disabling the override in `request-prepare.ts` alone left the v1-only version green. Its negative half asserts `routeCompactionModel` still resolves the bare native model to `openai`, because "no gateway call" was also satisfied by any regression that failed before reaching an upstream at all. Three `structure/` owners still described the override as manual-only. Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
Hosted CI shard 1/4 failed loading `responses-compaction-override.test.ts` with `ReferenceError: Cannot access 'runtimeRoleSchema' before initialization` at `config-schema.ts:69`. Importing `leaf-validators.ts` from `src/server/responses/compaction-routing.ts` to reach `COMPACTION_TRIGGERS` closed an import cycle with `config-schema.ts`. Entering that cycle from the request path rather than from config evaluates `config-schema.ts` while `leaf-validators.ts` is still initializing, so a `const` it exports is read in its temporal dead zone. The tuple now lives in `src/config/schema/compaction-triggers.ts`, a leaf module that imports nothing; the schema and the request path both read it from there. Typecheck and a module-reachability walk both accept the cycle, so only running the suite finds this. Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
…turns Thirteen assertions in the carried `manual compaction reuses existing handlers` block returned 502 `upstream_error` where they expect 200, including `an ordinary turn carrying manual metadata stays on the conversation model`, which activates no override at all. The cause is #5157, which landed after #4872 was cut: the spend journal now requires one writer lease per state directory. `startServer` takes that lease before anything can serve, so a case that calls `handleResponses` directly owns nothing and the ledger refuses to write for it. The split is exactly what the failures show — every direct `handleResponses` turn failed and every `handleResponsesCompact` case passed, because a compaction handoff draws on the parent request's reservation rather than taking its own. `tests/helpers/owned-spend-home.ts` exists for this and is already used by around twenty files, including `responses-inbound-store-default.test.ts` two entries away in the same shard. The lease is taken per case and released first in teardown, before anything else touches the state directory, as that helper documents. This replaces an earlier hypothesis recorded in the PR description, that the fixture's `fetch` stub mis-read a non-string request body. `adapter-dispatch.ts` documents the opposite — the outbound body is always a serialized string — so that reading was wrong. Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
b1f3b18 to
8721524
Compare
…incipal The last carried failure: after the manual override's native compact returns 429, the following automatic compaction is supposed to borrow the conversation's remembered handoff route and answer 200. It answered 429. `compactHandoffRouteKey` is `(admission principal, lane)` and returns null for an admission-less caller, so nothing was ever remembered in the seed step and there was nothing to borrow. That is deliberate — `responses-compact-handoff-admission.test.ts` asserts an `undefined` admission is ineligible rather than pooled — and it postdates #4872, whose fixture calls the handler with three arguments. The case now passes a configured admission with a `contextPrincipalId`, the same shape that test uses. Its point is unchanged and now actually tested on both halves: the manual override does not borrow, and an automatic compaction on the same lane still does. Co-authored-by: nahuelb <nahuelgbecerra@gmail.com>
리뷰 · 우선순위 64 / 80긴 대화를 짧게 줄이는 요청만, 고른 모델로 보낼 수 있게 합니다. 설정 이름은 대화 자체는 다른 쪽으로 잘 가는데, 줄을 줄일 때만 Codex가 OpenAI 모델을 고릅니다. OpenAI 사용량이 바닥이면 그 단계에서 모델, 생각의 세기, 그리고 언제 적용할지를 적습니다. 다른 쪽으로 보낼 때는, 그쪽이 확인하지 못하는 숨은 생각의 암호를 넘기지 않습니다. 읽을 수 있는 요약으로 바꿉니다. 같은 쪽이면 지금 쓰던 줄이기 방식을 유지합니다. 이 경계는 테스트에 있습니다. 이 글을 쓰는 시점에 리눅스 테스트 4개와 gates는 통과했습니다. macOS 검사는 아직 끝나지 않았습니다. PR은 draft입니다. 열려 있는 #4872를 지금
메인테이너의 판단이 필요한 지점 기본을 사람이 친 경우만으로 둔 것이 맞는지. 합치는 것만으로 #5012가 고쳐지지는 않습니다. 콤보 경고 문장을 여기서 고칠지, #4872에서 온 문장으로 두고 나중에 고칠지. 작성자는 나중에로 남겨 두었습니다. 대시보드에서 저장하면 재시작 없이 다음 줄이기에 적용됩니다. 너의 추천 #4872는 닫으면 됩니다. 둘 다 합치면 같은 설정이 두 번 들어옵니다. 이 PR은 리눅스 테스트는 이 커밋에서 통과했습니다. macOS 검사가 끝나기 전에는 draft를 유지하면 됩니다. 코드에서 한 번만 정하면 되는 것은, 설정이 켜진 요청이 실패했을 때 옛 우회를 끌지입니다. 사람이 친 경우만 끄고, 스스로 줄이는 경우에는 옛 길을 남길 수 있습니다. 그 외에는 기본이 꺼져 있고, 암호를 다른 쪽으로 넘기지 않는 쪽이 테스트로 잠겨 있습니다. 이 댓글은 grok-bot이 작성했습니다 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedbd12502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (route.combo || override.sourceCombo || override.targetCombo || resolveComboId(config, override.sourceModel)) return false; | ||
| let source: RouteResult; | ||
| try { | ||
| source = routeConcreteModel(config, override.sourceModel); |
There was a problem hiding this comment.
When the conversation model is policy/<id> or a routing-profile alias, routeConcreteModel deliberately bypasses policy evaluation, so it can resolve the source selector through the default provider instead of the provider that actually served the conversation. If that fallback provider matches the configured compaction target, this function incorrectly returns true, preserves caller authentication, and permits native compact ciphertext; a conversation routed by the policy to another provider cannot replay that ciphertext and loses its compacted history. Conservatively force the portable path for policy selectors, or carry the conversation's actual serving identity into this comparison.
AGENTS.md reference: AGENTS.md:L415-L421
Useful? React with 👍 / 👎.
| const namespace = model.slice(0, Math.max(model.indexOf("/"), 0)); | ||
| const combo = namespace === "combo" ? model.slice(namespace.length + 1) : ""; | ||
| const provider = namespace && !combo ? namespace : model; | ||
| const providers = comboProviders[combo]?.join(", ") || t("compactionRouting.comboProvidersUnknown"); |
There was a problem hiding this comment.
When a combo has a configured public alias, the model picker returns that bare alias, but this prefix-only check classifies it as a provider and displays a notice claiming the full conversation is sent to a provider named after the alias. The runtime actually resolves the alias to a combo whose targets may span unrelated providers, so the privacy notice hides the real destinations precisely when the user is deciding whether to enable routing. Index /api/combos by its returned public model as well as by id, then use the resolved combo's target providers.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
1 participant
Summary
A long Codex thread running on a routed provider could not compact when the canonical
OpenAI quota was exhausted. Codex picks a bare native model for the compaction turn, and
routeCompactionModelreserves that for an enabled canonicalopenaiprovider, releasingit only when no such provider is configured (#2901) — never when its quota is gone. The
thread resumed, entered PreCompact, and failed with
rate_limit_exceededbefore the routedturn could begin, while fresh requests through the routed provider kept working (#5012).
This adds one opt-in
compactionRoutingblock that sends Codex compaction requests to aconfigured model and optional reasoning effort, for the triggers you name:
{ "compactionRouting": { "model": "agent-space/gpt-5.6-luna", "reasoningEffort": "low", "triggers": ["manual", "auto"] } }Without the block nothing changes. With the block and no
triggers, only manual/compactis routed and automatic compaction stays exactly where it routes today.
"auto"is theseparate opt-in that #5012 needs.
Why one setting rather than two
#4872 already implemented this for manual
/compact, and the issue was linked to it as theimplementation candidate. Read against the client, though, it cannot satisfy #5012: its gate
requires
compaction.trigger == "manual", and the reported failure is an automaticcompaction. In codex-rs,
compact_remote_v2::run_inline_remote_auto_compact_taskbuildsCompactionTrigger::Autoand calls the samerun_remote_compact_task_innerthe manualCompactTaskuses, so both reach identical surfaces — acompaction_triggeritem on/v1/responses, or/v1/responses/compact— and differ only in that one string. Everythingelse the request needs (model resolution, effort, combos, provider identity, the portable
summarizer) is the same, so a second config block would have duplicated all of it to change
one token.
triggersis that token, named in Codex's own vocabulary.Upstream treats this as legitimate:
compact_model_fallbacklistsUsageLimitReachedinshould_retry_with_current_model, and only the automatic path receives afallback_step_context— the manual path passesNone. Codex itself already moves anautomatic compaction to another model when a usage limit blocks it. This is the proxy-side
equivalent, made explicit instead of implicit.
The setting is renamed from #4872's
manualCompaction, which never shipped, so noconfiguration exists to migrate and the name does not have to outlive its accuracy.
1775ce627b/compactrouting), rebased onto the currentdev97b26a34a6compactionRouting, addtriggers, cover automatic compactionffd8500656Closes #5012.
The first commit is a rebase of #4872 onto the current
dev:compactHandoffRoute,rememberCompactHandoffRouteandforgetCompactHandoffRoutegained anadmissionargument,
config-routes.tsgainedfastRows,diagnostics.tsgainedspendSchema, andthirteen
structure/owners took an append-at-end resolution keeping both sides. Everycommit carries
Co-authored-by: nahuelb.Dashboard panel, from #4872. It predates the trigger selector added here, which could not be
captured because building or running the GUI was not permitted in this lane:
Verification
No local execution of any kind. The test suite, individual test files,
typecheck,build,installand theocxbinary were all prohibited for this work, so none were run.Exact-head hosted CI on this branch is the only execution evidence, and it has not been
interpreted yet at the time of writing. Everything below is static.
openai/codexcheckout at095da4b7e8:core/src/responses_metadata.rs(thecompactionmetadata object and its keys),analytics/src/facts.rs(CompactionTriggeris exactlymanual|auto),core/src/compact_remote_v2.rs(the automatic and manual entry points), andcore/src/turn_metadata_tests.rs, which asserts the serialized automatic payload is{"trigger":"auto","reason":"context_limit",...}.src/is gated on avalidated override.
src/config.tsis still exactly 460 lines, its cap intests/fixtures/file-size-baseline.json.enandviare both 2873 keys andno locale ships an English-identical value for a new key.
src/router.ts,src/server/lifecycle.tsandsrc/server/responses/core.ts: the one new edge(
compaction-routing.ts→config/schema/leaf-validators.ts) reachessrc/labonlythrough
src/router.ts, whichcompaction-routing.tsalready imported, so Labreachability is unchanged from the baseline.
(
vi.tsisRecord<TKey, string>and compile-checked, and landed ondevafter feat(responses): route manual /compact to a configurable model and effort #4872 wascut, leaving the branch 20 keys short); a cross-identity override forwarding the source
backend's reasoning ciphertext to a destination that cannot verify it; an acceptance test
that covered only v1 and a negative assertion satisfiable by any early failure.
Known, not fixed here
Two findings belong to #4872's disclosure copy rather than to this change, and are left for
the maintainer to decide rather than widened into this lane:
combo/prefix, so a configured combo alias would bedisclosed as a bare provider name instead of naming the combo's targets.
advances only after a qualifying failure. The copy over-discloses, which is the safe
direction for a privacy notice, but it is not exact.
One surface does change without the block configured:
GET /api/settingsnow returnscompactionRouting: null, and the empty-PUTerror string names the new key. Both arerequired for the dashboard to distinguish "unset" from "unsupported", and match how every
neighbouring setting in that response is already surfaced unconditionally.
Checklist
Exact-head CI
Rebased onto
devc81d43053b; headbedbd12502. Green: 26 checks pass, 2 skipped(CodeRabbit is the only pending entry and skips while the PR is a draft). Both
testshards that were red are green, on Linux, Windows and macOS.
Three separate causes were red at earlier heads. All are resolved.
test 3/4andmacos 1/2weredev, not this branch. Replayingscripts/file-size-ratchet.tsshowed the offenders onorigin/devalone and none on thisbranch alone. #5201 fixed them; the rebase picked that up.
An import cycle. Reaching
COMPACTION_TRIGGERSfrom the request path closed a cycle withconfig-schema.ts, which typechecks and then fails at run time withReferenceError: Cannot access 'runtimeRoleSchema' before initialization. The constant moved to a leaf module thatimports nothing.
Two contracts that landed after #4872 was cut. Thirteen assertions in the carried
manual compaction reuses existing handlersblock returned 502, and one returned 429.startServertakes it before anything can serve, so a case calling
handleResponsesdirectly ownsnothing and the ledger refuses to write for it. That is exactly the failure split: every
direct
handleResponsesturn failed and everyhandleResponsesCompactcase passed,because a compaction handoff draws on the parent request's reservation. The file now takes
the lease through
tests/helpers/owned-spend-home.ts, as about twenty other files do.compactHandoffRouteKeyis(admission principal, lane)and is null for anadmission-less caller, so the seed step remembered nothing and the borrow control half had
nothing to take.
responses-compact-handoff-admission.test.tsasserts that ineligibilitydeliberately. The case now passes a configured admission with a
contextPrincipalId, soboth halves are tested: the manual override does not borrow, an automatic compaction does.
An earlier revision of this description guessed that the fixture's
fetchstub mis-read anon-string request body. That was wrong —
adapter-dispatch.tsstates the outbound body isalways a serialized string — and it is recorded here because it was published before it was
checked.
No local execution of any kind at any point: no suite, no focused test, no typecheck, no
build, no
ocx. Hosted CI at this head is the only execution evidence.