Skip to content

fix: parse combo models correctly when returned as objects#11

Open
herjarsa wants to merge 1 commit intoAlph4d0g:mainfrom
herjarsa:fix/combo-models
Open

fix: parse combo models correctly when returned as objects#11
herjarsa wants to merge 1 commit intoAlph4d0g:mainfrom
herjarsa:fix/combo-models

Conversation

@herjarsa
Copy link
Copy Markdown

The OmniRoute API /api/combos recent update returns an array of objects for underlying models instead of an array of strings. This caused opencode-omniroute-auth to crash when calling modelId.trim() in splitModelId. This PR adds a map to properly extract the model or id property if it is returned as an object. Confirmed to resolve the issue with standard combos like Combo-FIX recovering access to the full model index.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the resolveUnderlyingModels function to correctly extract model identifiers from the models array, which may now contain objects. Feedback was provided regarding potential runtime errors if the array contains null values or objects without the expected properties, recommending a safer mapping implementation and an update to the OmniRouteCombo interface for better type safety.

Comment thread src/omniroute-combos.ts
if (combo) {
console.log(`[OmniRoute] Resolved combo "${modelId}" to ${combo.models.length} underlying models`);
return combo.models;
return combo.models.map((m: any) => typeof m === 'string' ? m : (m.model || m.id));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation is vulnerable to runtime crashes if the models array contains null values or objects that lack both model and id properties. Specifically, (m.model || m.id) will throw if m is null, and if both properties are missing, the map will return undefined. This leads to a crash in splitModelId (called later in calculateModelCapabilities) when it attempts to call .trim() on a non-string value.

Additionally, the OmniRouteCombo interface (line 17) should be updated to reflect the new API response structure (e.g., models: (string | { model?: string; id?: string })[]), which would allow for proper type checking instead of relying on any.

Suggested change
return combo.models.map((m: any) => typeof m === 'string' ? m : (m.model || m.id));
return combo.models.map((m: any) => typeof m === 'string' ? m : (m?.model ?? m?.id)).filter((m): m is string => typeof m === 'string');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant