fix(xai): declare grok image input so combos are not advertised text-only#478
Conversation
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe xAI registry marks selected Grok models as accepting text and image inputs. Provider enrichment now completes partial modality overrides from registry defaults, and tests verify propagation into derived combo catalog models. ChangesVision modality propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/catalog-vision-sidecar-modalities.test.ts`:
- Around line 155-161: Expand the test in “xAI grok chat models declare image
input in the registry” to assert every newly vision-capable xAI model ID from
PROVIDER_REGISTRY, including the other three declarations alongside “grok-4.5”.
Also assert “grok-composer-2.5-fast” is absent from modelInputModalities and
present in noVisionModels, preserving the existing text-only exclusion checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e63a1066-c226-4665-9613-8fd90a48e0ee
📒 Files selected for processing (2)
src/providers/registry.tstests/catalog-vision-sidecar-modalities.test.ts
| test("xAI grok chat models declare image input in the registry", () => { | ||
| const xai = PROVIDER_REGISTRY.find(entry => entry.id === "xai"); | ||
| expect(xai?.modelInputModalities?.["grok-4.5"]).toEqual(["text", "image"]); | ||
| // Text-only members stay out of the vision map (they are already in noVisionModels). | ||
| expect(xai?.modelInputModalities?.["grok-build-0.1"]).toBeUndefined(); | ||
| expect(xai?.noVisionModels).toContain("grok-build-0.1"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover every changed xAI model declaration and exclusion.
Lines 400-403 in src/providers/registry.ts add four vision-capable IDs, but this test asserts only grok-4.5; it also omits the grok-composer-2.5-fast exclusion documented on Lines 397-398. A regression in any unasserted entry would pass.
Proposed test expansion
- expect(xai?.modelInputModalities?.["grok-4.5"]).toEqual(["text", "image"]);
- // Text-only members stay out of the vision map (they are already in noVisionModels).
- expect(xai?.modelInputModalities?.["grok-build-0.1"]).toBeUndefined();
- expect(xai?.noVisionModels).toContain("grok-build-0.1");
+ for (const model of [
+ "grok-4.5",
+ "grok-4.3",
+ "grok-4.20-0309-reasoning",
+ "grok-4.20-0309-non-reasoning",
+ ]) {
+ expect(xai?.modelInputModalities?.[model]).toEqual(["text", "image"]);
+ }
+ for (const model of ["grok-build-0.1", "grok-composer-2.5-fast"]) {
+ expect(xai?.modelInputModalities?.[model]).toBeUndefined();
+ expect(xai?.noVisionModels).toContain(model);
+ }As per path instructions, this src/ behavior change needs focused regression coverage in tests/.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("xAI grok chat models declare image input in the registry", () => { | |
| const xai = PROVIDER_REGISTRY.find(entry => entry.id === "xai"); | |
| expect(xai?.modelInputModalities?.["grok-4.5"]).toEqual(["text", "image"]); | |
| // Text-only members stay out of the vision map (they are already in noVisionModels). | |
| expect(xai?.modelInputModalities?.["grok-build-0.1"]).toBeUndefined(); | |
| expect(xai?.noVisionModels).toContain("grok-build-0.1"); | |
| }); | |
| test("xAI grok chat models declare image input in the registry", () => { | |
| const xai = PROVIDER_REGISTRY.find(entry => entry.id === "xai"); | |
| for (const model of [ | |
| "grok-4.5", | |
| "grok-4.3", | |
| "grok-4.20-0309-reasoning", | |
| "grok-4.20-0309-non-reasoning", | |
| ]) { | |
| expect(xai?.modelInputModalities?.[model]).toEqual(["text", "image"]); | |
| } | |
| for (const model of ["grok-build-0.1", "grok-composer-2.5-fast"]) { | |
| expect(xai?.modelInputModalities?.[model]).toBeUndefined(); | |
| expect(xai?.noVisionModels).toContain(model); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/catalog-vision-sidecar-modalities.test.ts` around lines 155 - 161,
Expand the test in “xAI grok chat models declare image input in the registry” to
assert every newly vision-capable xAI model ID from PROVIDER_REGISTRY, including
the other three declarations alongside “grok-4.5”. Also assert
“grok-composer-2.5-fast” is absent from modelInputModalities and present in
noVisionModels, preserving the existing text-only exclusion checks.
Source: Path instructions
![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: d539bd6e68
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| modelInputModalities: { | ||
| "grok-4.5": ["text", "image"], |
There was a problem hiding this comment.
When an existing xai config already contains any modelInputModalities entry, enrichProviderFromRegistry skips the registry map entirely instead of merging it per key. Consequently, gatherRoutedModels still leaves models such as xai/grok-4.5 without image modalities and affected combos remain advertised as text-only, even though request routing merges the same maps per key. Merge registry defaults beneath user overrides during catalog enrichment and cover a persisted partial map through gatherRoutedModels.
AGENTS.md reference: AGENTS.md:L75-L77
Useful? React with 👍 / 👎.
…only xAI's registry entry had no modelInputModalities, so catalog rows for xai/grok-4.5 carried inputModalities: undefined. deriveComboCatalogModel intersects member modalities and defaults an undefined member to ["text"], so every combo containing an xAI target was published to Codex with input_modalities ["text"]. The Codex app gates attachments client-side on that field, so images were rejected before a request was ever routed, even though both combo members accept images. This was visible with the documented xai -> cursor grok-4.5 failover combo: cursor/grok-4.5 advertised ["text","image"] (via the noVisionModels sidecar augmentation in applyProviderConfigHints) while xai/grok-4.5 advertised nothing, and the resulting combo collapsed to text-only. Declare the vision lineup for the grok-4.x chat models per docs.x.ai model capabilities. grok-build-0.1 and grok-composer-2.5-fast are intentionally omitted; they remain in noVisionModels. Intersection semantics are unchanged: a member whose modalities are unknown still narrows a combo to text, which the added regression test pins.
d539bd6 to
9441ed3
Compare
jonathanli12
changed the title
Addresses review feedback on the xAI vision fix. enrichProviderFromRegistry filled modelInputModalities all-or-nothing, so a single customized model in a persisted config suppressed the registry map entirely. A user who narrowed one grok model would leave grok-4.5 with no declared modalities, which put the combo aggregator back on its ["text"] default and re-hid image support. Routing already merges these maps per key (mergeRecordFill in src/router.ts). Catalog enrichment now matches, so registry defaults fill in beneath the user's entries instead of being skipped wholesale. Explicit user values still win per model. Also broaden the registry assertion to cover all four vision-capable grok ids and both text-only exclusions, so a regression in any single entry fails.
|
Thanks, both addressed in 9eb7f0b. Test coverage (trivial): the registry assertion now loops all four vision-capable grok ids and both text-only exclusions ( P2 partial-map merge: confirmed reproducible, and worth fixing. With a persisted config containing
Scoped deliberately to Added a regression test covering the persisted partial map through Verified: |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed at exact head 9eb7f0b36cbb10a87551f5b1e0324d77dcfacb19. xAI official model pages advertise Text+Image input for all four declared Grok models. The registry change fixes the real combo-catalog collapse without weakening unknown-member intersection semantics, and the per-model fill preserves explicit user overrides while restoring defaults only for missing keys. Verified the 12 focused modality/vision/combo regressions and typecheck locally. The unrelated Windows GUI flake passed on rerun; all current required checks are green.
2 participants
Problem
Any combo containing an xAI target is published to Codex as text-only, so the Codex app refuses image attachments ("does not support image inputs") before a request is ever routed, even when every member of the combo accepts images.
Reproduced with the documented xAI -> Cursor
grok-4.5failover combo. From the generated Codex catalog:Cause
The
xaientry inPROVIDER_REGISTRYdeclares nomodelInputModalities, so catalog rows forxai/grok-4.5carryinputModalities: undefined.deriveComboCatalogModelintersects member modalities and defaults an undefined member to["text"]:So one member with unknown modalities collapses the whole combo to text-only.
There is an ironic asymmetry today:
cursor/grok-4.5ends up correct because Cursor lists every model innoVisionModels, andapplyProviderConfigHintsdeliberately appendsimagefor sidecar-covered models (that code already documents why the catalog must still advertise image input, otherwise the Codex app blocks attachments client-side). xAI is neither sidecar-covered nor vision-declared, so it falls through both paths.Fix
Declare the vision lineup for the grok-4.x chat models, per the xAI docs on image understanding:
grok-build-0.1andgrok-composer-2.5-fastare intentionally omitted; they stay innoVisionModels.Notes
["text"], which is the safe behaviour and is now pinned by a test.routeModel, so per-member vision protection (planVisionSidecar/stripImagesInPlacefail-closed) is untouched.Tests
Added to
tests/catalog-vision-sidecar-modalities.test.ts:Verified locally: 637 tests across the 45 catalog/combo/vision/provider/registry test files pass, and
tsc --noEmitis clean.Summary by CodeRabbit