Conversation
…forwarded role (lidge-jun#5334 follow-up) lidge-jun#5334 made the developer wire role tri-state: an undeclared destination folds it to system. Two suites asserting role:"developer" on the Chat wire were missed because they are about tool-result repair ordering and document parts, not role selection — declare the destination, per the convention the change established. Verified: both files fail on dev@600075d2 with system-for-developer wire roles and pass with the declaration. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…etadata alias The transport guard added in a2aadb7 looked the provider name up as a registry id only, so a provider saved under a canonical metadata alias (gemini, anthropic-key, gemini-vertex, ...) failed the lookup and its vendor metadata verdict was discarded — generated text-only models fell back to unknown, bypassing vision preprocessing and picker exclusion. providerMatchesRegistryTransportOrAlias resolves the name to the entry that owns it (by id or declared extraMetadataAliases, case-folded like resolveMetadataProvider) and validates the configured transport against that entry, so an aliased row follows its owner's pinning rule. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
|
Fixed in 3cb3d0a. Added |
|
Fixed in 3cb3d0a — same root cause. The guard now calls |
| const lower = name.toLowerCase(); | ||
| const entry = getProviderRegistryEntry(name) | ||
| ?? PROVIDER_REGISTRY.find(row => | ||
| row.id.toLowerCase() === lower | ||
| || (row.extraMetadataAliases ?? []).some(alias => alias.toLowerCase() === lower)); | ||
| return entry !== undefined && providerMatchesRegistryTransport(entry.id, provider); |
There was a problem hiding this comment.
🟡 Metadata aliases still misclassify custom destinations
A custom gemini, anthropic-key, or case-varied provider is accepted through its metadata owner. Routing preserves that custom transport, so generated capabilities can still divert or strip its images.
Learn more
extraMetadataAliases feeds generated model metadata, but it is not a routing alias. The router looks up registry entries by the exact configured provider name in routedProviderConfig. Therefore, a configured provider named gemini keeps its own adapter and base URL, while this helper finds the google row and applies google's permissive transport rule. The same mismatch affects aliases such as anthropic-key and case-varied registry names.
Example: Configure gemini with adapter: "openai-chat" and baseUrl: "https://operator.example/v1". Routing sends requests to that endpoint unchanged. This helper resolves gemini to google, returns true because google does not set preserveCustomDestination, and a generated text-only verdict can send the image to a sidecar or strip it.
Recommended fix: For names that are not exact registry IDs, require the configured adapter, auth mode, and normalized endpoint to match the resolved alias owner directly. Do not reuse the owner's pinning rule, because routing does not canonicalize extraMetadataAliases. Add regression coverage for a custom alias-named destination and a case-varied custom provider.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function providerMatchesRegistryTransportOrAlias( | ||
| name: string, | ||
| provider: Pick<OcxProviderConfig, "baseUrl" | "adapter"> & Partial<Pick<OcxProviderConfig, "authMode">>, | ||
| ): boolean { | ||
| const lower = name.toLowerCase(); | ||
| const entry = getProviderRegistryEntry(name) | ||
| ?? PROVIDER_REGISTRY.find(row => | ||
| row.id.toLowerCase() === lower | ||
| || (row.extraMetadataAliases ?? []).some(alias => alias.toLowerCase() === lower)); | ||
| return entry !== undefined && providerMatchesRegistryTransport(entry.id, provider); |
There was a problem hiding this comment.
🔍 Description omits alias ownership expansion
The description promises adapter-and-destination matching. The implementation additionally treats metadata aliases and case-varied names as registry-owned, so the stated scope needs revision.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Motivation
Description
providerMatchesRegistryTransportand returningundefined(unknown) when the transport does not match insrc/vision/eligibility.ts.tests/vision/vision-eligibility.test.tsthat verifies azhipu-bigmodel-responsescustom preset does not inherit the vendor's text-only verdict and thatrequiresVisionPreprocessingremains permissive for the custom destination.structure/runtime.mdto state that registry/vendor metadata applies only when the configured adapter and destination still match the registry transport.Testing
./node_modules/.bin/bun test tests/vision/vision-eligibility.test.tswhich passed (23 tests, 0 failures) after switching to the repository-pinned Bun (1.4.0) when the system Bun lackednode:zlibexports../node_modules/.bin/bun run structure:checkwhich passed and./node_modules/.bin/bun run typecheckwhich completed successfully.git diff --checkreported no whitespace issues and committed the change asfix(vision): bind metadata to provider transport.Codex Task