Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions open-sse/services/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,17 @@ async function resolveModelByProviderInference(modelId: string, extendedContext:
return { provider: "gemini", model: modelId, extendedContext };
}

// Last resort: treat as openai model
// Last resort: no provider could be inferred — return a clear error instead
// of silently defaulting to "openai", which would produce a misleading
// "No credentials for provider: openai" response when the model name
// is unrecognised (e.g. a missing combo, a typo, or a bare model id
// that doesn't exist in any provider's catalog).
return {
provider: "openai",
provider: null,
model: modelId,
extendedContext,
errorType: "model_not_found",
errorMessage: `Unable to determine provider for model '${modelId}'. Use a provider/model prefix (e.g. openai/${modelId}) or ensure the model is added as a combo entry.`,
};
Comment on lines +471 to 482

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.

medium

The repository style guide (Rule 9) requires that all changes to production code in open-sse/ include tests. Please add unit tests (e.g., in tests/unit/services/model.test.ts) to verify that resolveModelByProviderInference correctly returns the model_not_found error object when no provider can be inferred, ensuring this new logic is covered and preventing regressions.

References
  1. Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)

}

Expand Down
11 changes: 11 additions & 0 deletions src/sse/handlers/chatHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,17 @@ export async function resolveModelOrError(
}

if (!modelInfo.provider) {
// model_not_found: raised by resolveModelByProviderInference when no
// provider could be inferred — return a clear error instead of the
// misleading "openai" default that the old code silently fell back to.
if ((modelInfo as any).errorType === "model_not_found") {

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.

medium

The use of any casting for modelInfo is a maintainability issue as it bypasses TypeScript's type checking. While this pattern exists elsewhere in the file, it would be better to define a shared interface for the model resolution result (including errorType and errorMessage) in open-sse/services/model.ts and use it here to avoid unsafe casts.

const message =
(modelInfo as any).errorMessage ||
`Model '${modelStr}' could not be resolved to a known provider.`;
log.warn("CHAT", message, { model: modelStr });
return { error: errorResponse(HTTP_STATUS.BAD_REQUEST, message) };
}

if ((modelInfo as any).errorType === "ambiguous_model") {
// Family disambiguation: if the model name begins with a known
// non-OAuth family prefix, auto-pick the family-native provider
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/chat-helpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,3 +387,17 @@ test("withSessionHeader adds headers to mutable and immutable responses", async
assert.equal(immutable.status, 302);
assert.equal(await immutable.text(), "");
});

test("resolveModelOrError returns model_not_found error for unrecognised bare model names", async () => {
const result = await resolveModelOrError(
"completely-unknown-model-xyz",
{ messages: [{ role: "user", content: "hello" }] },
"/v1/chat/completions"
);

assert.ok(result.error);
assert.equal(result.error.status, 400);
const json = (await result.error.json()) as any;
assert.match(json.error.message, /Unable to determine provider/i);
assert.match(json.error.message, /completely-unknown-model-xyz/i);
});
10 changes: 5 additions & 5 deletions tests/unit/services-branch-hardening.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,11 +354,11 @@ test("model helpers cover malformed input, alias maps, wildcard aliases, ambigui
model: "gemini-custom",
extendedContext: false,
});
assert.deepEqual(await modelService.getModelInfoCore("made-up-model", {}), {
provider: "openai",
model: "made-up-model",
extendedContext: false,
});
const unknownModel = await modelService.getModelInfoCore("made-up-model", {});
assert.equal(unknownModel.provider, null);
assert.equal(unknownModel.errorType, "model_not_found");
assert.ok(unknownModel.errorMessage.includes("made-up-model"));
assert.ok(unknownModel.errorMessage.includes("provider/model prefix"));
});

test("error classifier covers empty-content helpers, context overflow and remaining error classes", () => {
Expand Down