Skip to content

fix(catalog,anthropic): keep Claude combo image/effort capabilities and honor provider output budget - #3332

Closed
full999 wants to merge 1 commit into
lidge-jun:devfrom
full999:codex/claude-combo-capabilities-and-output-budget
Closed

fix(catalog,anthropic): keep Claude combo image/effort capabilities and honor provider output budget#3332
full999 wants to merge 1 commit into
lidge-jun:devfrom
full999:codex/claude-combo-capabilities-and-output-budget

Conversation

@full999

@full999 full999 commented Sep 3, 2026

Copy link
Copy Markdown

Summary

Two user-visible defects when Claude models are exposed to Codex through failover combos (e.g. a claude-opus-5 alias whose targets are anthropic/claude-opus-5claude-sonnet-5claude-fable-5):

  • Image attachments refused, effort picker hidden. Combo members are usually thin discovery rows (id + context window). With no capability source, deriveComboCatalogModel intersected to input_modalities: ["text"] and an empty supported_reasoning_levels, so the Codex app showed "remove the image or switch models" and no reasoning-effort control for every Claude combo. resolveComboCatalogMember now falls back to the generated vendor metadata table (src/generated/model-metadata.ts) for input modalities and reasoning capability when the caller supplied no fallback. Point-release / date-pinned ids (claude-fable-5-1, claude-opus-4-5-20251101) resolve to their family row. An explicit caller fallback (native aliases) still wins.
  • Long Claude answers truncated at 8192 tokens. Codex never sends max_output_tokens, so the Anthropic adapter always used max_tokens: 8192. Long answers ended with stop_reason: max_tokens and Codex retried the identical turn up to five times ("stream disconnected before completion: Incomplete response returned, reason: max_output_tokens"). The adapter now honors the provider's modelMaxOutputTokens / defaultMaxOutputTokens for omitted limits (mirroring resolveMaxTokens in the openai-chat adapter), and the anthropic / anthropic-apikey registry entries default to 64000. Explicit caller limits are unchanged; Anthropic still caps per model server-side.

Observed on 2.33.0 and reproduced on dev @ 878f754 before this change (combo rows: inputModalities: undefined, reasoningEfforts: undefined; adapter body max_tokens: 8192).

Verification

  • bun test tests/codex-catalog.test.ts tests/anthropic-reasoning.test.ts tests/catalog-vision-sidecar-modalities.test.ts tests/provider-registry-parity.test.ts → 391 pass / 0 fail (two regression tests added).
  • tsc --noEmit -p tsconfig.json → clean.
  • Live check with the patched proxy: ~/.codex/opencodex-catalog.json combo rows now carry text,image + low…max; codex app-server model/list reports the same; a base64 image sent to claude-opus-5 via /v1/responses completed; a 9,344-output-token answer completed without max_tokens truncation (previously always cut at 8192).

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No user-facing config change; the new registry default is the only behavior change. Happy to add a note if you want one.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

…nd honor provider output budget

Two defects surfaced together when Claude models are exposed to Codex through
failover combos:

1. Combo members are usually thin discovery rows (id + context window). With no
   capability source the modality/effort intersection collapsed to text-only and
   an empty ladder, so the Codex app refused image attachments ("remove the image
   or switch models") and hid the effort picker for every Claude combo.
   resolveComboCatalogMember now falls back to the generated vendor metadata
   table for input modalities and reasoning capability, and point-release ids
   (claude-fable-5-1, date-pinned ids) resolve to their family row.

2. Codex never sends max_output_tokens, so the Anthropic adapter always used
   max_tokens=8192. Long answers stopped with stop_reason=max_tokens and Codex
   retried the identical turn up to five times. The adapter now honors the
   provider's modelMaxOutputTokens / defaultMaxOutputTokens for omitted limits,
   and the anthropic / anthropic-apikey registry entries default to 64000.
   Explicit caller limits still win unchanged.
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

Hygiene

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 67 / 80

이 PR은 Claude 모델을 콤보(failover)로 Codex에 노출할 때 생기던 두 가지 사용자 체감 버그를 한번에 고친다. 지금 dev HEAD 38f8a8164에서 src/codex/catalog/provider-fetch.tsresolveComboCatalogMember는 discovery 행이 얇으면(id + context window만) 이미지/노력(effort) 정보를 채우지 않는다. 그래서 deriveComboCatalogModel 교집합이 input_modalities: ["text"]와 빈 reasoning ladder로 떨어지고, Codex 앱은 이미지 첨부를 거부하고 effort 피커를 숨긴다. Anthropic 쪽은 src/adapters/anthropic.ts가 Codex가 보내지 않는 max_output_tokensDEFAULT_MAX_TOKENS = 8192로만 메꾼다. 긴 답이 stop_reason=max_tokens로 잘리면 Codex가 같은 턴을 여러 번 재시도한다. 레지스트리의 anthropic/anthropic-apikey에도 아직 defaultMaxOutputTokens가 없다(Command Code 등 다른 엔트리만 64k를 들고 있다). 이 PR은 (1) vendor metadata 테이블(src/generated/model-metadata.ts)로 콤보 멤버의 이미지·reasoning을 마지막 수단으로 채우고, (2) 어댑터가 modelMaxOutputTokens/defaultMaxOutputTokens를 존중하게 하며, (3) Anthropic 시드에 64k 기본 출력 예산을 넣는다. 실사용 버그 수정이라 우선순위는 높다. 다만 PR이 아직 draft이고 readiness 체크리스트가 비어 있다.

라인 provider-fetch.ts vendorMetadataComboFallback - 같은 파일의 augmentRoutedModelsWithMetadata(대략 2515행 근처)는 meta.maxTokens를 **maxOutputTokens**로 넣는다. 이 PR만 maxInputTokens: metadata.maxTokens로 매핑한다. 테이블에서 Claude Opus 5는 context 1M / maxTokens 128k인데, 128k를 입력 천장으로 넣으면 콤보 멤버 입력이 과소 표기된다. maxOutputTokens로 고치거나, 입력은 contextWindow만 쓰고 출력만 채워야 한다.
라인 provider-fetch.ts ROUTED_COMBO_MEMBER_REASONING_EFFORTS - reasoning=true인 모든 vendor에 low…max를 통째로 붙인다. 주석대로 어댑터가 위쪽 칸을 잘라 낼 수는 있지만, Codex 피커에는 없는 칸이 보일 수 있다. Claude/Grok 공통 사다리로 둘지, 프로바이더별 ladder를 쓸지 한 줄 정책이 필요하다.
라인 provider-fetch.ts comboMemberVendorMetadata - 끝의 -숫자를 반복 제거해 claude-fable-5-1claude-fable-5로 가는 건 맞다. 다만 의미 없는 suffix가 아닌 id에도 동작하니, 히트한 family 행을 로그/테스트로 한두 개만 더 고정해 두면 회귀에 강하다.
라인 adapters/anthropic.ts 적응형 thinking - Math.max(omittedMaxTokens, Math.min(...))라서 기본 64k가 켜지면 adaptive 경로의 실효 상한이 거의 항상 64k가 된다. 의도된 완화라면 테스트에 “adaptive + 기본 예산 = 64k”를 명시하는 편이 좋다.
경로 tests - catalog/anthropic 회귀 테스트는 핵심 시나리오를 잘 덮는다. maxTokens → maxInputTokens 매핑만 고치면 기대값도 함께 손보면 된다.

메인테이너의 판단이 필요한 지점

  • draft + readiness 미체크 상태로 둘지, 작성자에게 ready 전환을 요청한 뒤 머지 큐에 넣을지.
  • vendor metadata의 maxTokens를 콤보 fallback에서 출력으로만 쓸지, 입력 힌트로도 쓸지(현재 PR은 입력으로 써서 기존 catalog 관례와 어긋남).
  • Anthropic 기본 출력 64k를 레지스트리 기본값으로 넣을지, 문서/릴리즈 노트에 한 줄 남길지(작성자도 노트 여부를 물어 둠).

너의 추천
maxTokens → maxInputTokensmaxOutputTokens로 고친 뒤, draft를 풀고 CI 초록이면 머지하는 쪽을 추천한다. Claude 콤보 이미지 거부와 8192 절단은 지금 dev에도 재현되는 실버그다. types/config 분할과 직접 충돌하지 않으니 닫지 말고, 작은 매핑 수정만 받고 들이는 편이 맞다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

Triaged in the 260904 bug-backlog closeout.

The output-budget half is correct: honoring the configured provider budget in src/adapters/anthropic.ts is the right fix and the reasoning tests cover it.

The catalog half has a blocking mapping error. In src/codex/catalog/provider-fetch.ts the vendor metadata fallback assigns maxTokens to maxInputTokens, but that vendor field is the OUTPUT ceiling. For Claude rows that shrinks a 1M input window down to 128k, which is a worse regression than the thin-capability problem the fallback is meant to solve.

To land: map it to maxOutputTokens, add an assertion that the 1M input window survives the fallback, and rebase — the branch is 66 commits behind dev.

lidge-jun added a commit that referenced this pull request Sep 4, 2026
…redit redeems a stable identity (#3474)

Reimplements #3332 by @full999, which could not be cherry-picked. One line differs from the original and it matters: the PR mapped the vendor table's maxTokens -- an OUTPUT ceiling -- onto maxInputTokens, and because aggregation takes Math.min over member input ceilings, a single Claude member would have dragged a 1M combo down to 128k and the auto-compaction budget from 900k to 128k with it.

Also wires the reset-credit operation ledger, which was complete and had zero production callers while the consume endpoint minted a fresh UUID per call. An optional operationId now becomes the redeem_request_id. Opening fails closed, because falling back to a random id is the double-spend the identity prevents; settling fails open, because by then the credit is gone and reporting failure would invite a manual retry. Omitting operationId keeps today's behavior unchanged.

Carried from #3327 by @olddonkey as well: two coverage holes from #3198, with one over-broad assertion narrowed.

Also lands #3251 by @abhisheksharma2411: the GUI now consumes tierOutcome, which the backend already shipped and nothing displayed.

Co-authored-by: full999 <daiki.furutani@walker-s.co.jp>
Co-authored-by: olddonkey <olddonkeyblog@gmail.com>
Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev via #3474 — squash 00834d710, verified with git merge-base --is-ancestor 00834d710 origin/dev. Co-authored-by: full999 is in the squash body.

Reimplemented rather than cherry-picked because this branch was CONFLICTING/DIRTY against current dev. One line differs from your version, and it is the reason this was not merged as-is.

vendorMetadataComboFallback mapped metadata.maxTokens onto maxInputTokens. In the vendor table maxTokens is the OUTPUT ceiling — claude-opus-5 is { contextWindow: 1_000_000, maxTokens: 128_000 }. Since aggregation.ts:161 takes Math.min over member input ceilings, one Claude member drags the whole combo down:

member ctx member maxIn combo maxIn combo autoCompact
this PR as written 1,000,000 128,000 128,000 128,000
landed 1,000,000 1,000,000 1,000,000 900,000

clampAutoCompactTokenLimit reads maxInputTokens as a candidate, so the auto-compaction budget collapses with it. Merging unchanged would have cost Claude combo users 87% of their input context.

ComboCatalogMemberFallback already had a maxOutputTokens slot and withFallbackMetadata already handled it, so the repair is the field name — the rest of your change landed as you wrote it.

Worth noting why your test did not catch this: toMatchObject inspects only the keys it names, and contextWindow survives at 1M regardless. The collapse happens one field over. The landed version asserts maxInputTokens and autoCompactTokenLimit directly.

Thank you for the capability work — the image/effort preservation and the provider output budget are both yours and both shipped.

@lidge-jun lidge-jun closed this Sep 4, 2026
@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working landed-via-maintainer Original PR closed after landing via a maintainer merge train

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants