feat(reasoning): derive routed effort ladders from models.dev and replay a refused rung - #4488
Conversation
…lay a refused rung [skip ci] A routed gateway publishes model ids but not the reasoning ladder behind them, so the catalog advertised rungs the upstream refuses. A models.dev snapshot now supplies the ladder when nothing is configured for that model, and a rung the upstream actually refuses is learned, dropped from every later ladder, and replayed once at the next lower published rung instead of failing the turn. `requestedEffort` and `effectiveEffort` keep both values in usage under the `reasoning-effort-downgrade` recovery kind. Carries #4409 by yxr1995-maker onto current dev. Conflict resolved in src/server/responses/core.ts: dev grew a `consoleGoUploadRetryGuard` replay block at both of the insertion points this branch targets. The two recoveries are independent and share only their trailing `continue` tail, so both blocks are kept, console-go first, each closing its own `if`. Review findings folded in: - The generic `recovery:` loop declared its downgrade guard inside the loop, so every `continue recovery` handed the turn a fresh downgrade budget. The guard now sits outside, beside the opaque-blob and console-go guards, and a regression test pins one downgrade for a replay that is refused again. - `planReasoningEffortDowngrade` read the models.dev ladder before the configured one, so a replay could land on a rung a pinned registry ladder deliberately excludes. Precedence now matches `configuredReasoningEfforts()`: model ladder, then provider ladder, then metadata, with the same family and case-folded id lookup. - `isReasoningEffortRejection` treated the bare parameter name as evidence, so a 400 refusing another field while echoing the request back spent the turn's one replay and persisted a false refusal for thirty days. It now needs the upstream to name the parameter, or rejection language beside the effort term. - `loadSupport()` applied the 30-day TTL only on the first disk read, so a long-running proxy kept clamping on month-old refusals through the memo. - `configuredReasoningEfforts()` asked for a metadata refresh only after a successful lookup, which is the one path a missing or corrupt snapshot never reaches. The refresh is now requested before the lookup. - The streamed test used an `openai-chat` fixture and so exercised the generic recovery loop while claiming to cover the passthrough one. It keeps that coverage and gains an `openai-responses` case for `passthroughRecovery:`. - The decision record claimed models.dev outranks a hand-written ladder, which contradicted both the code and its own layer list. `responses-reasoning-effort-downgrade.test.ts` is registered in both `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`; the branch had registered only `reasoning-metadata.test.ts`. Not folded in: the refusal cache still keys on destination, model and effort, so two configured entries pointing at the same gateway share learned refusals. Widening the key needs a provider identity threaded through every read path in the catalog, and a credential-derived key would put this link inside the security-review boundary. Recorded as open rather than half-applied. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 65 / 80설명 두 번째 축은 거절 학습과 한 번 재시도입니다. 업스트림이 실제로 거절한 rung은 베이스는 열린 #4475( 남긴 여덟 번째가 중요합니다. 거절 캐시 키가 destination+model+effort라서, 같은 게이트웨이를 가리키는 설정 항목 둘이 학습을 공유합니다. provider id를 키에 넣으려면 카탈로그 read 경로 전체를 건드려야 하고, credential 파생 키는 보안 리뷰 경계에 들어갑니다. 지금은 형제 항목을 보수적으로 조이는 쪽이지 잘못된 요청을 만드는 쪽은 아니라고 본문에 적혀 있습니다. 테스트 레이아웃에 경로/심볼 - 베이스가 #4475라 tip에 단독 머지하면 lane R이 꼬입니다. #4475 머지 후 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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
https://github.com/lidge-jun/opencodex/blob/cbdaf5068ef5eb07831f0b34e893e4b79ff2de78//workspace/opencodx/src/reasoning-effort.ts#L155-L156
For models with reasoningEffortMap or modelReasoningEffortMap, mapReasoningEffort() returns the mapped value before it ever calls this newly filtered configuredReasoningEfforts(). Consequently, after a mapped rung such as DeepSeek's max is learned as refused, the catalog hides it but saved or explicit max requests continue sending max and failing upstream; the wire mapping must clamp against the learned ladder before taking the alias shortcut.
AGENTS.md reference: src/AGENTS.md:L18-L18
ℹ️ 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 (!text) return false; | ||
| if (/unsupported.{0,24}effort/i.test(text)) return true; | ||
| // An upstream that names the offending parameter has already said which one it means. | ||
| if (/["']?param["']?\s*[:=]\s*["']?(?:reasoning[._ ]effort|reasoning)/i.test(text)) return true; |
There was a problem hiding this comment.
When an upstream rejects a sibling field such as reasoning.summary or reasoning.encrypted_content, this alternation still succeeds on the reasoning prefix. The proxy then spends a hidden retry lowering an unrelated effort and persists that rung as unsupported for 30 days, while the actually invalid field remains unchanged. Require an exact parameter match, including a closing quote or other boundary, instead of accepting bare reasoning as a prefix.
Useful? React with 👍 / 👎.
| function supportKey(providerKey: string, modelId: string, effort: string): string { | ||
| return providerKey + KEY_SEP + modelId + KEY_SEP + effort; |
There was a problem hiding this comment.
When two configured provider entries use different keys against the same ZenCode ZenX base baseZen/ZenCode GoOpenCode endpoint, a rejection from either entry produces the identical opencode-go|model|effort key. The other credential may have the required entitlement, but its ladder is still clamped for 30 days, so the cache key needs a stable provider-entry or credential-scope dimension rather than only the destination-derived provider key.
Useful? React with 👍 / 👎.
| const snapshot = loadSnapshot(); | ||
| if (snapshot && Date.now() - snapshot.fetchedAt <= CACHE_TTL_MS) return; | ||
| if (refreshInFlight) return; | ||
| void refreshReasoningMetadata().catch(() => undefined); |
There was a problem hiding this comment.
When the snapshot is absent or stale and models.dev is offline, a failed refresh leaves that same absent/stale state and records no last-attempt time. As soon as refreshInFlight settles, the next catalog or request-time effort lookup starts another 15-second fetch, so active offline installations can maintain nearly continuous outbound refresh attempts; retain a bounded retry timestamp/backoff independently of snapshot freshness.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,543 @@ | |||
| /** | |||
There was a problem hiding this comment.
This commit adds a persistent provider-metadata subsystem and changes shared catalog and Responses recovery behavior under src/providers/, src/reasoning-effort.ts, and src/server/, but it updates no file under structure/. The repository requires every structure document mapped to a changed source area to be updated in the same change, so the applicable architecture/ownership records need to describe this new cache and recovery path before landing.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
…tting OPENCODEX_HOME [skip ci] Two defects found by running the carried change against tests/web-search, which the reasoning-focused test selection did not reach. configuredReasoningEfforts() asked for a models.dev snapshot refresh before the lookup so a missing or corrupt snapshot could recover, but it asked for every provider. A destination the snapshot does not cover gained a background fetch on its request path that could never help it, and in tests it consumed the mocked fetch that the web-search bridge was counting. The refresh now sits behind providerUsesReasoningMetadata(), which is true only for the gated destinations ladders are stored for. tests/codex-integration/reasoning-metadata.test.ts deleted OPENCODEX_HOME in afterEach instead of restoring it, so every later file in the same bun process read the real ~/.opencodex. That failed unrelated suites depending on file order, and made the run depend on the machine's actual configuration. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
Reverts the review finding that asked configuredReasoningEfforts() to request a models.dev refresh before the metadata lookup rather than after it. The reasoning was that a missing or corrupt snapshot is the case the lookup cannot serve, so asking only on success never refreshes it. That is true, and it is still the wrong place. A missing snapshot is the default state of a fresh install and of every test process. Asking there put a models.dev fetch on the request path of the first routed turn to a gated destination, which is observable: the lane tip run failed tests/responses/responses-console-go-upload-retry.test.ts and tests/providers/opencode-go-session-header.test.ts, where the extra bodyless request landed in the middle of a recovery replay the test was counting, and tests/web-search saw it consume the mocked destination's next leg. Refreshing a snapshot that does not exist yet is catalog-sync work. The refresh stays where the branch put it, so it only ever refreshes a stale snapshot that has already answered a lookup. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
Lane R of the contributor carry train, the serialized responses/core lane: code-mode view_image through unified exec (#4455 by jeongjin0, also carrying the duplicate #4171 by rrmlima), routed effort ladders from models.dev with a refused-rung replay (#4409 by yxr1995-maker), and web-search continuations bound to the serving API key (#4387 by luvs01). Cross-platform CI run 34746891233 concluded success on 2c28886, the exact head merged here, and it covers every link because the lane is cumulative. #4475 and #4488 carry no ci check of their own; their head commits carry [skip ci] by design, under the owner-authorized tip-only CI economy for this batch. The fourth planned link, #4086 by Eleven-is-cool, is not here because it is already on dev as d6723f7 with its own Co-authored-by trailer. The lane attempted the carry first and found a modify/delete conflict on structure/04_transports-and-sidecars.md, which the #4276 SSOT restructure had removed; the landed version is a superset of the branch. All four source authors are credited by Co-authored-by trailers in the landed commits.
1 participant
Summary
/modelswith ids and nothing else, so the Codex catalog had no way to know which reasoning rungs a model actually accepts. It advertised rungs the upstream refuses, and the turn failed.src/providers/reasoning-metadata.tssnapshots the models.dev reasoning ladders for the gated destinations into~/.opencodex/reasoning-metadata-cache.json(24h TTL, stale-but-readable offline, atomic write).configuredReasoningEfforts()consults it only when nothing is configured for that model, so every hand-written contract stays authoritative.reasoning-support-cache.json, dropped from every later ladder including registry-pinned ones, and the refused request is replayed once at the next lower published rung instead of failing.requestedEffortandeffectiveEffortstay distinguishable in usage under thereasoning-effort-downgraderecovery kind.dev, with aCo-authored-bytrailer on the branch commit.Stacked on #4475 (lane R, link 1). Retarget to
devonce that lands.Conflict resolved
src/server/responses/core.tsconflicted at both insertion points.devhas since grown aconsoleGoUploadRetryGuardreplay block in the same position in both thepassthroughRecovery:and the genericrecovery:loop. The two recoveries are independent and happened to share their trailingcontinuetail, which is what produced the conflict. Both blocks are kept, console-go first, each closing its ownif. Neither guard can mask the other: console-go matches an exact gateway upload rejection, the downgrade matches a reasoning-effort refusal.Review findings folded in
CodeRabbit left eight findings on #4409. Seven are fixed here; the eighth is recorded as open with a reason.
recovery:loopcontinue recoverywas handing the turn a fresh downgrade budget, so one request could walk the whole ladder down. A regression test pins a single downgrade when the replay is refused again.planReasoningEffortDowngraderead metadata before the configured ladderconfiguredReasoningEfforts(): model ladder, then provider ladder, then metadata. Same family-prefix and case-folded id lookup asmodelRecordValue(), mirrored locally becausereasoning-effort.tsimports this module.invalid_request_errortag excluded. A 400 that refusesmax_tokenswhile echoing the request back no longer spends the turn's one replay or persists a false refusal.loadSupport()skipped TTL expiry on a memo hitdropLearnedUnsupportedReasoningEfforts()inherited it through the same map.ensureReasoningMetadataSnapshot()called after the lookupundefined, so the one situation needing a refresh never asked for one.openai-chatfixtureopenai-responsescase forpassthroughRecovery:.The cache still keys on destination, model and effort, so two configured entries pointing at the same gateway share learned refusals. The write path and every read path would all need a stable provider identity, which reaches through
configuredReasoningEfforts()into the catalog; a partial fix where the write key is wider than the read key would silently stop honouring learned refusals. A credential-derived key would also put this link inside theMAINTAINERS.mdsecurity-review boundary, which this lane is not carrying. The practical effect today is a conservative clamp for a sibling entry, not a wrong or unsafe request, so it is recorded as open rather than half-applied.Test layout
The branch registered only
reasoning-metadata.test.ts.responses-reasoning-effort-downgrade.test.tsis now in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, whichtests/test-layout.test.tsandtests/test-layout-tooling.test.tsenforce as a pair.Verification
bun test tests/responses/responses-reasoning-effort-downgrade.test.ts tests/codex-integration/reasoning-metadata.test.ts tests/codex-integration/reasoning-effort.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 85 pass, 0 fail.bun run typecheck— clean.bun run structure:check— passed.bun run privacy:scan— passed.[skip ci].Checklist
No credential material enters either cache file.
reasoning-support-cache.jsonstores a destination key, model id, effort and a truncated rejection message; the decision not to widen that key with credential-derived material is the security-relevant call recorded above.