fix(catalog): backfill a published context window onto a live routed row - #5023
Conversation
augmentRoutedModelsWithMetadata was append-only by construction. It added generated-registry rows the live provider list did not return and skipped every id the live list did return, so it had no merge branch at all. A discovered OpenCode Go row therefore kept contextWindow: undefined even though the generated registry publishes a window for exactly that provider and id, which is the normal case there: /v1/models returns id, object, created and owned_by with no context field. The serialized Codex catalog was never affected and this is not a re-fix of #4944. applyCatalogMetadata writes context_window, max_context_window and auto_compact_token_limit onto the entry straight from the generated table on both derive paths, so the entry Codex reads was already correct. #4962 repaired that data and changed nothing here. What was affected is every consumer that reads CatalogModel.contextWindow instead of the serialized entry, and those are not cosmetic. buildClaudeContextWindows filters out routed rows with no positive window, so the map that decides the Claude [1m] marker never learned about a published 1M model and could not mark it. The Grok config writer, the management and Desktop projections, and the OpenCode and Cline exporters all copy the field only when present, so each omitted a value the registry knew. Note that both consumers named in the report are actually safe, which is worth recording so they are not "fixed" later: combo derivation resolves a provider-owned generated fallback before it examines the live row, and src/routing/capability.ts reads config maps plus the serialized capability provenance rather than the gathered CatalogModel. Add the missing-value merge branch. A live positive window still wins, because the upstream is the authority on its own model. The seeded row is re-hinted through applyProviderConfigHints so operator precedence is unchanged: a configured window still lowers it and providerContextCaps still caps it, exactly as for an appended row. Re-hinting an existing row is the same thing resolveComboCatalogMember already does.
|
✅ Deterministic PR hygiene checks passed. |
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesContext Window Backfill
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant LiveModels
participant CatalogAugmentation
participant PublishedRegistry
participant ContextWindowBuilder
participant OneMillionMarker
LiveModels->>CatalogAugmentation: provide discovered model row
PublishedRegistry->>CatalogAugmentation: provide published context window
CatalogAugmentation->>CatalogAugmentation: fill missing window and apply provider hints
CatalogAugmentation->>ContextWindowBuilder: return augmented model
ContextWindowBuilder->>OneMillionMarker: pass model context window
OneMillionMarker-->>ContextWindowBuilder: append [1m] for one-million window
Merge Risk: ⚪ Minimal · up to The context-window backfill is covered for the intended live-row and downstream marker behavior, with no actionable current-head risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
리뷰 · 우선순위 74 / 80이 PR은 지금 중요한 구분이다. 이 결함은 #4944의 재발도 아니고, Codex가 읽는 고침은 빠진 값만 채우는 merge 분기다. 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 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
Here are some automated review suggestions for this pull request.
Reviewed commit: 06e9c6f737
ℹ️ 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".
| : undefined; | ||
| const liveWindow = typeof existing.contextWindow === "number" && existing.contextWindow > 0; | ||
| if (liveWindow || publishedWindow === undefined) continue; | ||
| const seeded: CatalogModel = { ...existing, contextWindow: publishedWindow }; |
There was a problem hiding this comment.
When discovery omits a window, this new merge changes user-visible behavior in Claude model labels and generated Grok/client configurations, but the commit updates neither docs-site/ nor the structure documentation mapped to src/codex/ in structure/INDEX.md. Add the backfill and precedence contract to the applicable structure documents and update the public documentation for the affected client projections so the documented catalog behavior does not drift from runtime behavior. The scoped source instructions explicitly require mapped structure documents to be updated in the same change.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
|
Merging with macOS legs outstanding, and recording why rather than leaving it implicit. At this exact head the full Linux suite (test 1/4 through 4/4), This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here. |
1 participant
Summary
augmentRoutedModelsWithMetadatawas append-only by construction. It added generated-registry rows the live provider list did not return, andif (seen.has(key)) continueskipped every id the live list did return — so it had no merge branch at all. A discovered OpenCode Go row therefore keptcontextWindow: undefinedeven though the generated registry publishes a window for exactly that provider and id, which is the normal case there:/v1/modelsreturnsid,object,createdandowned_bywith no context field.This is not a re-fix of #4944, and the serialized catalog was never affected.
applyCatalogMetadatawritescontext_window,max_context_windowandauto_compact_token_limitonto the entry straight from the generated table on both derive paths, so the entry Codex reads was already correct. #4962 repaired that data and did not touch this function; I checked its merged diff (6ba36c3076, an ancestor of this branch) before writing anything, and it changed onlyscripts/model-metadata.source.json,src/generated/model-metadata.ts, the two layout maps, and its own regression file. None of its four tests callsaugmentRoutedModelsWithMetadata.What was affected is every consumer that reads
CatalogModel.contextWindowinstead of the serialized entry, and those are not cosmetic:buildClaudeContextWindowsfilters out routed rows with no positive window, so the map that decides the Claude[1m]marker never learned about a published 1M model and could not mark it.computeEffectiveModelEnvpassesgatherRoutedModelsoutput straight in, so this is the live path.src/claude/model-info.tsderives routed max-input and the 1M variant from the same raw field.supports1m), and the OpenCode, Cline and sibling exporters all copy the field only when present, so each omitted a value the registry knew.Both consumers named in the report are actually safe, which is worth recording so they are not "fixed" later on a wrong diagnosis.
resolveComboCatalogMembercomputes a provider-owned generated-metadata fallback before it examines the live row, so combo derivation never reaches the generic 128k fallback for an id the registry publishes. Andsrc/routing/capability.tsreads provider/config maps plus aCatalogModelRowreconstructed from the serializedopencodex_capability_provenance, not the gatheredCatalogModel— and that provenance writer independently falls back to generated metadata.The fix is the missing-value merge branch. A live positive window still wins, because the upstream is the authority on its own model. The seeded row is re-hinted through
applyProviderConfigHintsso operator precedence is unchanged: a configured window still lowers it andproviderContextCapsstill caps it, exactly as for an appended row. Re-hinting an existing row is not a new pattern —resolveComboCatalogMemberalready does it.Closes #4971
Verification
Local verification was not run, because this lane forbids it. No test, focused test, typecheck, build, install, or
ocxinvocation was executed. Hosted CI on this branch is the executable verification for this change.Static verification performed instead:
applyProviderConfigHintsto confirm the precedence the re-hint relies on. A seeded window becomesdiscoveredWindow, so a configured cap lowers it viaMath.min, andapplyProviderContextCapthen appliesproviderContextCapsand stampscontextCap/contextCapped. A merge that wrotemeta.contextWindowdirectly onto the row would have skipped both; the fourth new test is there specifically to catch that.tests/codex-integration/codex-catalog.test.tsare unaffected: two pass[]and exercise only the append path, andopencode-go catalog sync appends official rows missing from /v1/modelspasses one windowless live row and asserts slug membership plustoHaveLength(1)— the merge replaces that row at its existing index, so the length is preserved.seen-to-indexByKeychange, including a duplicate id within one metadata list: the second visit now finds the row just appended, sees it already has a window, and continues.buildClaudeContextWindowsandwithOneMillionMarkerand also asserts the pre-fix state of the same row, so it cannot pass vacuously.Ratchet and layout, checked because both have broken other pull requests this cycle:
tests/codex-integration/codex-catalog.test.ts, which sits at exactly its recorded cap (7,985 lines intests/fixtures/file-size-baseline.json) with zero headroom. Appending there would have failed the ratchet for every later PR.tests/codex-integration/catalog-opencode-go-context-window.test.ts, the file fix(catalog): publish opencode-go context windows so live rows stop falling back to 128k #4962 created for the neighbouring symptom, which is the natural home anyway. It goes from 118 to 207 lines, has no baseline cap, and stays far below the 2,000-line threshold for unbaselined files. No new layout registration is needed because that basename is already present in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.src/codex/catalog/routed-gather.tsis not in the ratchet baseline and remains under the threshold.Union-with-
devcheck, since a green PR is not a green merge result: this branch is based on11bc4f708c. It changes one function body and one test file. It touches no roster, no hard-coded count, no exhaustive adapter or provider map, no locale catalog, and no generated or golden file — in particular it does not touchsrc/generated/model-metadata.tsorscripts/model-metadata.source.json, so it cannot collide with themodel-metadata-syncbyte comparison or with another provider-catalog refresh landing beside it. A later snapshot refresh that adds ids composes with this change rather than conflicting: more published windows simply means more rows the merge branch can fill.Checklist
Docs: no user-facing surface changes. The behaviour is that an already-published context window now reaches the routed row it belongs to;
docs-sitedocuments the window sources, not this internal merge, andstructure/catalog.mdalready describes generated metadata as the fallback authority. The rationale that would otherwise be lost is recorded in the function's own doc comment, including which two consumers are not affected. Security: no credential, auth, logging, or network behaviour is involved; the change moves an integer already present in a generated table onto an in-memory row.Summary by CodeRabbit