Fix cloud LLM 404: honor plugin model selection, parse Gemini array errors, stop poisoning legacy global - #696
Conversation
Post-processing workflows that selected a provider but no explicit model override silently ignored the model chosen in the plugin UI, falling back to the legacy global llmCloudModel key. When that key still pointed at a retired model (e.g. gemini-2.0-flash), every run failed with a bare "API error: HTTP 404" because Gemini's error body was dropped. Two defects fixed: 1. Conform GeminiPlugin, GroqPlugin, CerebrasPlugin, FireworksPlugin and ClaudePlugin to LLMModelSelectable and expose preferredModelId, so PromptProcessingService.resolvedModelId() honours the user's selection instead of the legacy global key (matches OpenRouter/XAI/Gemma4). 2. PluginOpenAIChatHelper now parses error bodies wrapped in a top-level JSON array (Gemini's OpenAI-compat shape) in addition to the dictionary shape, so the descriptive API message survives instead of collapsing to "HTTP <status>". Extracted into a testable errorMessage(from:statusCode:). Adds GeminiPlugin/CerebrasPlugin/FireworksPlugin/ClaudePlugin as SwiftPM targets with test targets (mirroring GroqPlugin/OpenRouterPlugin) and covers all changes with tests.
normalizeSelectedCloudModelIfNeeded() persisted models.first?.id into the legacy llmCloudModel key whenever no model was selected. Because the fetched model list is sorted alphabetically, models.first is the oldest model (e.g. gemini-2.0-flash). Once Google retired that model the persisted global kept sending it on every workflow run, producing HTTP 404 even after the user picked a current model in the UI. Extract the model-resolution decision into a pure, unit-testable resolveModel(...) that returns whether the result should be written through to the global. The global is now only persisted for a deliberate choice - the provider plugin's preference, or a self-healing repair of a non-empty but invalid global. When no model was ever selected, the alphabetical fallback is used transiently for the run but not persisted, so a later-retired oldest model can no longer silently poison the global. Existing self-healing behaviour (repairing a non-empty invalid global) is preserved and still covered by APIRouterAndHandlersTests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCentralizes model-resolution into a pure resolver with explicit persistence semantics, exposes plugin-selected model IDs via LLMModelSelectable across providers, adds SPM plugin targets/tests and Xcode test wiring, and centralizes OpenAI/Gemini error-body parsing with unit tests. ChangesModel Selection & Plugin Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (2)
TypeWhisperPluginSDK/Plugins/GroqPlugin/Tests/GroqPluginTests.swift (1)
49-59: ⚡ Quick winAdd a pre-selection assertion for
preferredModelId.This test verifies the post-selection path, but it should also assert the pre-selection state to lock in “no explicit selection => no preferred model” behavior.
Suggested test addition
func testPreferredModelIdReflectsSelectedLLMModel() throws { let host = try PluginTestHostServices() let plugin = GroqPlugin() plugin.activate(host: host) + XCTAssertNil((plugin as LLMModelSelectable).preferredModelId) + let target = try XCTUnwrap(plugin.supportedModels.first?.id) plugin.selectLLMModel(target) let preferred = (plugin as? LLMModelSelectable)?.preferredModelId XCTAssertEqual(preferred, target) }🤖 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 `@TypeWhisperPluginSDK/Plugins/GroqPlugin/Tests/GroqPluginTests.swift` around lines 49 - 59, In testPreferredModelIdReflectsSelectedLLMModel, add a pre-selection assertion that (plugin as? LLMModelSelectable)?.preferredModelId is nil before calling plugin.selectLLMModel; locate the test function and insert an XCTAssertNil(preferred) (or XCTAssertEqual to nil) immediately after plugin.activate(host: host) and before deriving target from plugin.supportedModels.first?.id to ensure the "no explicit selection => no preferred model" behavior is validated.TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift (1)
807-831: Align HostServices error parsing with OpenAIPlugin precedence
HostServices.errorMessage(from:statusCode:)already preserves Gemini’s array-wrappederror.message, but it skips OpenAIPlugin-style fallbacks for top-level"detail"and top-level"message". Add checks in precedence order (detail→ nestederror.message→ top-levelmessage) and apply the same dictionary-vs-array handling when extracting these fields to avoid losing provider-specific error details.🤖 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 `@TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift` around lines 807 - 831, HostServices.errorMessage(from:statusCode:) currently only inspects nested error.message (and array-wrapped first element) but misses OpenAIPlugin-style top-level "detail" and top-level "message"; update the function to handle both JSON object and JSON array shapes and apply precedence: first check top-level "detail" (String, non-empty), then nested "error.message" (as currently), then top-level "message" (String, non-empty), and finally fall back to "HTTP <statusCode>"; reference the function HostServices.errorMessage(from:statusCode:) and ensure you inspect both the dictionary and array-first cases for each key in that order.
🤖 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 `@TypeWhisperPluginSDK/Plugins/GroqPlugin/GroqPlugin.swift`:
- Line 8: preferredModelId is exposing an auto-seeded fallback because
_selectedLLMModelId is initialized with supportedModels.first?.id during
activation; change the logic so that preferredModelId only returns a model id
when the user has explicitly chosen one. Introduce a separate boolean/state
(e.g., _hasUserSelectedLLMModel or _userSelectedLLMModelId) or a distinct
property for the seeded default used internally in activate() (leave
_seededLLMModelId or similar) and update the selection setter used by user
actions to set the user-selected flag; then make preferredModelId return the
user-selected id (or nil if none), not the seeded fallback from activate().
Ensure references to _selectedLLMModelId, preferredModelId, and activation logic
are updated consistently.
---
Nitpick comments:
In `@TypeWhisperPluginSDK/Plugins/GroqPlugin/Tests/GroqPluginTests.swift`:
- Around line 49-59: In testPreferredModelIdReflectsSelectedLLMModel, add a
pre-selection assertion that (plugin as? LLMModelSelectable)?.preferredModelId
is nil before calling plugin.selectLLMModel; locate the test function and insert
an XCTAssertNil(preferred) (or XCTAssertEqual to nil) immediately after
plugin.activate(host: host) and before deriving target from
plugin.supportedModels.first?.id to ensure the "no explicit selection => no
preferred model" behavior is validated.
In `@TypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swift`:
- Around line 807-831: HostServices.errorMessage(from:statusCode:) currently
only inspects nested error.message (and array-wrapped first element) but misses
OpenAIPlugin-style top-level "detail" and top-level "message"; update the
function to handle both JSON object and JSON array shapes and apply precedence:
first check top-level "detail" (String, non-empty), then nested "error.message"
(as currently), then top-level "message" (String, non-empty), and finally fall
back to "HTTP <statusCode>"; reference the function
HostServices.errorMessage(from:statusCode:) and ensure you inspect both the
dictionary and array-first cases for each key in that order.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01e092d0-7541-48b4-9479-356650bcc93c
📒 Files selected for processing (16)
TypeWhisper.xcodeproj/project.pbxprojTypeWhisper/Services/PromptProcessingService.swiftTypeWhisperPluginSDK/Package.swiftTypeWhisperPluginSDK/Plugins/CerebrasPlugin/CerebrasPlugin.swiftTypeWhisperPluginSDK/Plugins/CerebrasPlugin/Tests/CerebrasPluginTests.swiftTypeWhisperPluginSDK/Plugins/ClaudePlugin/ClaudePlugin.swiftTypeWhisperPluginSDK/Plugins/ClaudePlugin/Tests/ClaudePluginTests.swiftTypeWhisperPluginSDK/Plugins/FireworksPlugin/FireworksPlugin.swiftTypeWhisperPluginSDK/Plugins/FireworksPlugin/Tests/FireworksPluginTests.swiftTypeWhisperPluginSDK/Plugins/GeminiPlugin/GeminiPlugin.swiftTypeWhisperPluginSDK/Plugins/GeminiPlugin/Tests/GeminiPluginTests.swiftTypeWhisperPluginSDK/Plugins/GroqPlugin/GroqPlugin.swiftTypeWhisperPluginSDK/Plugins/GroqPlugin/Tests/GroqPluginTests.swiftTypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/HostServices.swiftTypeWhisperPluginSDK/Tests/TypeWhisperPluginSDKTests/OpenAIChatHelperTests.swiftTypeWhisperTests/PromptProcessingModelResolutionTests.swift
CodeRabbit review on TypeWhisper#696: - GroqPlugin/CerebrasPlugin/FireworksPlugin/ClaudePlugin seeded _selectedLLMModelId with supportedModels.first?.id on activation, so the newly-added preferredModelId exposed an auto-seeded fallback as if the user had chosen it - which the host would then persist into the legacy global, re-introducing the very poisoning this PR fixes. Drop the activate-time seed so preferredModelId is nil until the user selects a model; process() and the settings view already fall back to supportedModels.first independently, so runtime behaviour is unchanged. GeminiPlugin is intentionally left as-is: its default is a curated gemini-flash-latest (not the alphabetical oldest) that protects fresh users from the retired gemini-2.0-flash. - Add a pre-selection assertion (preferredModelId == nil) to the four plugin tests to lock in the "no explicit selection => no preferred model" contract. - Align PluginOpenAIChatHelper.errorMessage(from:statusCode:) with the OpenAIPlugin precedence (top-level detail -> nested error.message -> top-level message), applied to both dictionary and array-wrapped bodies.
|
Addressed the CodeRabbit review in 319f94c:
|
SeoFood
left a comment
SeoFood
left a comment
There was a problem hiding this comment.
Requesting changes on one remaining blocker.
The CodeRabbit finding was fixed for Groq, Cerebras, Fireworks, and Claude, but the same contract still appears to be violated by Gemini.
GeminiPlugin.preferredModelId returns _selectedLLMModelId, and normalizeSelectedModel() still auto-populates _selectedLLMModelId with supportedModels.first when there is no valid stored selection. For Gemini, fetched models are sorted alphabetically, and the PR description notes that gemini-2.0-flash is retired but still listed by the /models endpoint. That means Gemini can still persist/expose gemini-2.0-flash as a preferred model even when the user never explicitly selected it.
This undermines the main fix in this PR: PromptProcessingService.resolveModel() now treats a plugin preferredModelId as a deliberate provider preference and may persist it to the legacy global. So Gemini can still reintroduce the “oldest listed model poisons model selection” failure mode.
Please make Gemini follow the same contract as the other fixed plugins:
preferredModelIdshould only represent an explicit user-selected model.- Auto/default fallback should be used transiently for processing/UI display, not written into
_selectedLLMModelIdas a preferred model. - Add a Gemini regression test with fetched models including
gemini-2.0-flash,gemini-2.5-flash, andgemini-flash-latest, asserting that fresh activation/model normalization does not expose or persistgemini-2.0-flashaspreferredModelId.
Relevant paths:
TypeWhisperPluginSDK/Plugins/GeminiPlugin/GeminiPlugin.swiftTypeWhisperPluginSDK/Plugins/GeminiPlugin/Tests/GeminiPluginTests.swiftTypeWhisper/Services/PromptProcessingService.swift
…dModelId Review on TypeWhisper#696 (SeoFood): Gemini still violated the contract the other four plugins were fixed to honor in 319f94c. normalizeSelectedModel() seeded _selectedLLMModelId with supportedModels.first and persisted it, and since fetched models sort alphabetically that means the retired gemini-2.0-flash could be exposed/persisted as a preferred model the user never chose - re-introducing the legacy-global poisoning this PR fixes. - normalizeSelectedModel() is now validate-only: _selectedLLMModelId (and thus preferredModelId) only ever holds an explicit, still-valid user selection. No fallback is seeded into it or written to user defaults; a stored selection that is temporarily missing from the model list is kept so it re-validates after the next fetch. - The transient default for process() and the settings UI prefers the curated auto-updating gemini-flash-latest alias over the alphabetically-oldest fetched model (post-refresh auto-select included). - Drop the now-dead _selectedLLMModelId read in activate(); normalizeSelectedModel() re-reads the stored value itself. - Add regression tests with fetched models [gemini-2.0-flash, gemini-2.5-flash, gemini-flash-latest]: fresh activation neither exposes nor persists gemini-2.0-flash; a stale stored selection is not replaced by a fallback; a valid stored selection survives activation.
|
@SeoFood Fixed in 4280af7 — Gemini now follows the same contract as the other four plugins.
No host-side change was needed — Verified with:
|
|
Thanks for the follow-up. The
For Gemini with fetched models, There is already an app test encoding this behavior:
func testAlphabeticalFallbackIsUsedButNeverPersisted() {
let resolution = PromptProcessingService.resolveModel(
requestedModel: nil,
preferredModelId: nil,
selectedCloudModel: "",
availableModelIds: ["gemini-2.0-flash", "gemini-2.5-flash", "gemini-flash-latest"]
)
XCTAssertEqual(resolution.modelId, "gemini-2.0-flash")
XCTAssertFalse(resolution.persistGlobally)
}So the new Gemini tests prove that I think the fix needs one more step: when there is no explicit requested/preferred/global model, the host should either pass |
…ilable Follow-up review on TypeWhisper#696 (SeoFood): the plugin-side fix alone left the host resolution path choosing availableModelIds.first when no requested, preferred or valid global model exists, and passing it explicitly to the plugin - so GeminiPlugin's curated transient default was never reached and the host could still (transiently) run the retired gemini-2.0-flash. - SDK: add an optional defaultModelId to LLMModelSelectable - the model a provider recommends when no explicit selection exists. A transient fallback hint, never a user preference. @objc optional, so existing plugin binaries stay compatible. - GeminiPlugin exposes its curated gemini-flash-latest default through it. - PromptProcessingService.resolveModel() takes providerDefaultModelId and prefers it (when valid) over availableModelIds.first in the fallback branch. This covers both the transient no-selection case and the self-healing repair of an invalid non-empty global, which previously persisted the retired oldest model again. - ModelPickerView preselects the provider default instead of models.first when the bound selection is empty/invalid - the picker writes through to the persisted llmCloudModel global, so this was a third site adopting the alphabetically-oldest model. - Host resolver tests: provider default wins the fallback transiently, an unlisted default is ignored, and repair persists the provider default rather than the oldest model. SDK test: GeminiPlugin's host-visible defaultModelId is the curated alias when fetched models include retired gemini-2.0-flash.
|
@SeoFood Confirmed — the resolver's last fallback returned Fixed in 9a486ff with your second option (provider-aware default), since it covers all three sites where pass-nil only covers the process path:
Verified with:
|
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 `@TypeWhisper/Views/PromptActionsSettingsView.swift`:
- Around line 417-420: The current code unconditionally replaces selection with
fallbackModelId or first model when selection is empty, which auto-promotes a
transient hint into processingService.selectedCloudModel; change the logic in
PromptActionsSettingsView where selection is assigned so that you only
auto-repair when an existing non-empty selection is invalid: check if
selection.isEmpty — if true, leave it untouched (so runtime resolution via
LLMModelSelectable.defaultModelId can occur); otherwise, compute validFallback
from fallbackModelId and models and set selection = validFallback ?? selection
(or models.first?.id if you prefer to repair only invalid non-empty values).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe9bb118-5337-4401-b7a0-615df88169a7
📒 Files selected for processing (6)
TypeWhisper/Services/PromptProcessingService.swiftTypeWhisper/Views/PromptActionsSettingsView.swiftTypeWhisperPluginSDK/Plugins/GeminiPlugin/GeminiPlugin.swiftTypeWhisperPluginSDK/Plugins/GeminiPlugin/Tests/GeminiPluginTests.swiftTypeWhisperPluginSDK/Sources/TypeWhisperPluginSDK/TypeWhisperPlugin.swiftTypeWhisperTests/PromptProcessingModelResolutionTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- TypeWhisperPluginSDK/Plugins/GeminiPlugin/Tests/GeminiPluginTests.swift
- TypeWhisper/Services/PromptProcessingService.swift
- TypeWhisperPluginSDK/Plugins/GeminiPlugin/GeminiPlugin.swift
…rsisting defaults Two follow-ups on TypeWhisper#696: - testGeminiPluginActivationIgnoresLegacyCacheAndRepairsInvalidSelection (app-tests CI failure on 4280af7) still asserted the old behavior of normalizing an invalid stored selection into a persisted gemini-flash-latest. Update it to the explicit-selection contract: the invalid selection is neither exposed via selectedLLMModelId nor rewritten; the stored value is kept so it can re-validate after a fetch, and the legacy cache key is cleared. Renamed accordingly. - CodeRabbit on 9a486ff: ModelPickerView.ensureValidSelection() wrote the provider-default fallback into the binding even when the selection was empty, auto-promoting a transient hint into the persisted llmCloudModel global the moment the settings view appeared. An empty selection is now a first-class "Default (<model>)" picker row and is left untouched; only an invalid non-empty selection self-heals (to the provider default when listed, matching the resolver's repair semantics).
SeoFood
left a comment
SeoFood
left a comment
There was a problem hiding this comment.
Approved. The follow-up commits address the model-resolution blockers I raised.
The host path now uses a provider-aware defaultModelId instead of blindly falling back to availableModelIds.first, and the picker no longer writes the transient default into selectedCloudModel for empty selections. CodeRabbit's follow-up thread is resolved and the current checks are green.
Thanks for tightening the tests around the Gemini default and resolver repair behavior.
3 participants
Closes #697
Summary
Post-processing workflows using a cloud LLM provider (Gemini, Groq, Cerebras, Fireworks, Claude) could fail with a bare
API error: HTTP 404and silently drop the dictation. Root cause was three independent defects that combine; this PR fixes all three.1. Provider plugins ignored the model picked in their own UI
PromptProcessingService.resolvedModelId()resolves a workflow's model via(plugin as? LLMModelSelectable)?.preferredModelId.GeminiPlugin,GroqPlugin,CerebrasPlugin,FireworksPluginandClaudePluginstored_selectedLLMModelIdbut did not conform toLLMModelSelectable/ exposepreferredModelId(unlikeOpenRouterPlugin/XAIPlugin/Gemma4Plugin). The cast returnednil, so the model the user selected in the plugin UI was never consulted and the host fell through to the legacy globalllmCloudModel.→ All five plugins now conform to
LLMModelSelectableand expose@objc var preferredModelId, matching the existing OpenRouter/xAI pattern.2. The legacy
llmCloudModelglobal was auto-poisoned with the oldest modelnormalizeSelectedCloudModelIfNeeded()persistedmodels.first?.idintollmCloudModelwhenever no model was selected. The fetched model list is sorted alphabetically, somodels.firstis the oldest model (e.g.gemini-2.0-flash). Once Google retired that model, the persisted global kept sending it on every run — and Google's/modelsendpoint still lists it, so validation could not catch it.→ The resolution decision is extracted into a pure, unit-testable
resolveModel(...). The global is now persisted only for a deliberate choice (the plugin'spreferredModelId, or a self-healing repair of a non-empty but invalid global). When no model was ever selected, the alphabetical fallback is used transiently for that run but not persisted. The existing self-healing behaviour is preserved.3. Gemini's error body is a top-level array — the helpful message was dropped
Google's OpenAI-compat endpoint returns the error wrapped in a top-level JSON array:
[{ "error": { "code": 404, "message": "This model models/gemini-2.0-flash is no longer available...", "status": "NOT_FOUND" } }]PluginOpenAIChatHelperonly parsed dictionary bodies ([String: Any]), so the cast failed and the user only sawAPI error: HTTP 404.→ Error parsing is extracted into a testable
PluginOpenAIChatHelper.errorMessage(from:statusCode:)that handles both the dictionary shape and Gemini's top-level-array shape, falling back toHTTP <status>only when no message is present.Verified API behaviour
Against
POST https://generativelanguage.googleapis.com/v1beta/openai/chat/completions:/models)Changes
LLMModelSelectable+preferredModelIdfor Gemini / Groq / Cerebras / Fireworks / Claude plugins.PluginOpenAIChatHelper.errorMessage(from:statusCode:)handles array + dictionary error bodies.PromptProcessingService.resolveModel(...)extracted as a pure function; the legacy global is no longer poisoned with the alphabetically-oldest model.GeminiPlugin/CerebrasPlugin/FireworksPlugin/ClaudePluginas SwiftPM targets + test targets (mirroring the existingGroqPlugin/OpenRouterPluginsetup) so the conformance is covered byswift test.Test plan
swift test --package-path TypeWhisperPluginSDK: 218 tests, 0 failures (incl. the new error-body and conformance tests, andProtocolContractTests).PromptProcessingModelResolutionTests(6) and the existing prompt-processing integration tests pass. Each fix was developed test-first (failing test → minimal change → green).Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests