fix: normalize openai-codex and github-copilot provider IDs for media-understanding#56678
Conversation
…derstanding openai-codex (Codex OAuth) and github-copilot both route through OpenAI models that support vision, but their provider IDs were not mapped to "openai" in the media-understanding registry. This caused "No media-understanding provider registered for openai-codex" errors when using the image tool with Codex subscription auth. Add normalization rules in normalizeMediaProviderId() so that openai-codex and github-copilot resolve to the openai media provider, matching the existing gemini → google pattern. Closes #0 (no existing issue)
Greptile SummaryThis PR fixes a bug where using Changes:
Confidence Score: 5/5Safe to merge — focused, low-risk fix with good test coverage and no changes to existing behaviour. All findings are P2 style suggestions. The implementation is straightforward and correctly follows the established pattern, with no logic errors or regressions. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/media-understanding/provider-id.ts | Adds normalization for openai-codex and github-copilot → openai, following the existing gemini → google pattern. Logic is correct. |
| src/media-understanding/provider-registry.test.ts | Two new tests covering the openai-codex and github-copilot normalization paths; minor: github-copilot test omits the capabilities assertion that the openai-codex test includes. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/media-understanding/provider-registry.test.ts
Line: 99-103
Comment:
**Minor test inconsistency — missing capabilities assertion**
The `openai-codex` test asserts `provider?.capabilities` (line ~82: `expect(provider?.capabilities).toContain("image")`), but the `github-copilot` test only checks `provider?.id`. Both tests use the same mock provider data, so the assertion would trivially pass. Adding it would keep the two tests symmetric and make the intent clearer.
```suggestion
expect(provider).toBeDefined();
expect(provider?.id).toBe("openai");
expect(provider?.capabilities).toContain("image");
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: normalize openai-codex and github-c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const provider = getMediaUnderstandingProvider("github-copilot", registry); | ||
|
|
||
| expect(provider).toBeDefined(); | ||
| expect(provider?.id).toBe("openai"); | ||
| }); |
There was a problem hiding this comment.
The openai-codex test asserts provider?.capabilities (line ~82: expect(provider?.capabilities).toContain("image")), but the github-copilot test only checks provider?.id. Both tests use the same mock provider data, so the assertion would trivially pass. Adding it would keep the two tests symmetric and make the intent clearer.
| const provider = getMediaUnderstandingProvider("github-copilot", registry); | |
| expect(provider).toBeDefined(); | |
| expect(provider?.id).toBe("openai"); | |
| }); | |
| expect(provider).toBeDefined(); | |
| expect(provider?.id).toBe("openai"); | |
| expect(provider?.capabilities).toContain("image"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/media-understanding/provider-registry.test.ts
Line: 99-103
Comment:
**Minor test inconsistency — missing capabilities assertion**
The `openai-codex` test asserts `provider?.capabilities` (line ~82: `expect(provider?.capabilities).toContain("image")`), but the `github-copilot` test only checks `provider?.id`. Both tests use the same mock provider data, so the assertion would trivially pass. Adding it would keep the two tests symmetric and make the intent clearer.
```suggestion
expect(provider).toBeDefined();
expect(provider?.id).toBe("openai");
expect(provider?.capabilities).toContain("image");
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: c597477199
ℹ️ 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".
| if (normalized === "openai-codex" || normalized === "github-copilot") { | ||
| return "openai"; |
There was a problem hiding this comment.
Do not collapse openai-codex/github-copilot to openai in this shared normalizer, because runProviderEntry uses the normalized value as the actual execution/auth provider (src/media-understanding/runner.entries.ts:432-500). With this change, a media entry configured as openai-codex now resolves auth as openai, so users who only have Codex OAuth (or Copilot token auth) can no longer run media understanding even though their provider is configured. The aliasing is safe for registry lookup, but here it rewrites runtime provider identity and breaks credential/model resolution paths.
Useful? React with 👍 / 👎.
|
+1, waiting for this bug fix to land in main |
2 participants
Summary
When using
openai-codex(Codex OAuth) orgithub-copilotas a provider, the image/media-understanding tool fails with:Both providers route through OpenAI models that fully support vision, but
normalizeMediaProviderId()did not map them to"openai", so the registry lookup couldn't find the OpenAI media-understanding adapter.Changes
src/media-understanding/provider-id.ts— Added normalization rules:openai-codex → openaiandgithub-copilot → openai, matching the existinggemini → googlepattern.src/media-understanding/provider-registry.test.ts— Added two tests confirming both provider variants resolve to theopenaimedia-understanding provider.Test plan
pnpm vitest src/media-understanding)openai-codexas primary provider → image tool should work without fallback