feat: Multi-Provider support#2277

CodexAdapter hardcoded instanceId check breaks custom instances

High Severity

CodexAdapter still gates model and serviceTier on input.modelSelection?.instanceId === "codex", but after this PR, custom Codex instances carry instance IDs like codex_personal. The equivalent check in ClaudeAdapter was correctly updated to use input.modelSelection unconditionally (line 2834), but this one was missed. Custom Codex instances will silently ignore the user's selected model and never enable fast mode.

^{Reviewed by Cursor Bugbot for commit 7a466fe69801e159448cebfdaf1df5077cb538ce. Configure here.}

🟡 Medium hooks/useWorkspaceCommandExecutor.ts:140

useCommandPaletteStore is called with a selector that reads state.openWorkspaceTarget, but that property does not exist on the CommandPaletteStore interface — the store only exposes open, openIntent, setOpen, toggleOpen, openAddProject, and clearOpenIntent. Consequently, openWorkspaceTarget evaluates to undefined, and the calls on lines 192 and 198 throw TypeError: openWorkspaceTarget is not a function. Consider either adding openWorkspaceTarget to the store or replacing these calls with the correct store method.

✅ Resolved in fb6a2294f57212464a50e151e2192de87fc13cfa

🟡 Medium Layers/ProviderInstanceRegistryHydration.ts:159

Errors from serverSettings.getSettings during initialization are silently swallowed by Effect.orElseSucceed(() => undefined), so a corrupt settings file or other startup failure causes the registry to initialize with an empty config map and no log entry explaining why providers are missing. Consider logging the error before falling back, or removing the fallback so startup failures are visible.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

🟢 Low Layers/ProviderEventLoggers.ts:68

Both native and canonical loggers are created with the same providerEventLogPath, so makeEventNdjsonLogger writes both streams to identical file paths (e.g., /logs/thread-abc.log) since only path.dirname(filePath) and threadSegment are used in the filename. This causes interleaved/corrupted output when both streams write concurrently. Consider including the stream type in the log filename to keep the two streams separate.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

🟢 Low workspace/store.ts:868

readInitialWorkspaceDocument passes user-controlled storage data to normalizePersistedWorkspaceDocument without validating that nodesById exists. When nodesById is missing, normalizePersistedWorkspaceDocument throws TypeError: Cannot read properties of undefined at document.nodesById[document.rootNodeId]. This crashes the app on startup if browser storage is corrupted.

isWorkspaceDocument only checks version and layoutEngine, so the guard passes despite missing required fields. Consider validating nodesById in isWorkspaceDocument, or add a defensive check in normalizePersistedWorkspaceDocument before accessing nodesById.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

🟢 Low Layers/ClaudeAdapter.ts:2834

The startSession function no longer validates that input.modelSelection.instanceId === "claudeAgent" before using modelSelection, while sendTurn still performs this check. This inconsistency means startSession will pass non-Claude model selections to resolveClaudeApiModelId, potentially sending invalid model IDs to the Claude API. Consider re-adding the instanceId validation in startSession to match sendTurn.

✅ Resolved in 4be50878d48aa84d0066e04dd20e259a9d999c45

🟢 Low src/composerDraftStore.ts:449

cloneModelSelectionByProvider is typed with Partial<Record<ProviderKind, ModelSelection>> but is called with Partial<Record<ProviderInstanceId, ModelSelection>> in partializeComposerDraftStoreState. The type mismatch causes a TypeScript error and potential runtime issues when iterating over provider keys that don't match the expected format. Change the signature to use ProviderInstanceId instead of ProviderKind.

No longer relevant as of fb6a2294f57212464a50e151e2192de87fc13cfa

🟡 Medium Layers/TextGenerationLive.ts:53

The resolveInstance function allows text generation calls through disabled provider instances. The module docstring states that disabled instances should surface a TextGenerationError, but the current check only verifies instance ? without checking instance.enabled.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

🟠 High src/modelSelection.ts:275

When selection.instanceId is a custom instance like "codex_work", resolveSelectableProvider returns the driver kind "codex", so the comparison provider === selection.instanceId at line 279 returns false. This causes selectedModel to become null and modelOptions to become undefined, silently discarding the user's saved model and options for any non-default instance. The comparison should use the original selection.instanceId value to determine if we're still on the same instance, not the resolved driver kind.

✅ Resolved in b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

Instance-level comparison blocks custom instances of same driver

High Severity

The provider-switch guard compares requestedModelSelection.instanceId (an open ProviderInstanceId) against threadProvider (a closed ProviderKind). A thread created with a custom instance like "codex_personal" fails immediately at the Schema.is(ProviderKind) check on line 286, and a turn request targeting a custom instance of the same driver as the thread is rejected as a "provider switch." This effectively prevents any thread bound to a non-default provider instance from starting turns through the orchestration engine.

^{Reviewed by Cursor Bugbot for commit fb6a2294f57212464a50e151e2192de87fc13cfa. Configure here.}

Hardcoded "codex" provider in unknown-driver error path

Medium Severity

When a thread references an unknown provider driver, the ProviderAdapterRequestError hardcodes provider: "codex". The error's message getter formats as "Provider adapter request failed (codex) for thread.turn.start: ...", falsely attributing the error to the codex provider regardless of which unknown driver was actually referenced. This misleads error tracking, logging, and any downstream error-handling logic that inspects the provider field.

^{Reviewed by Cursor Bugbot for commit fb6a2294f57212464a50e151e2192de87fc13cfa. Configure here.}

🟡 Medium src/settings.ts:195

ModelSelectionPatch accepts instanceId but callers still sending provider will have that key silently ignored, leaving instanceId undefined. This breaks backward compatibility for any API calls or persisted patches using the old field name. Consider adding a decode transformation that maps provider to instanceId if present.

🟢 Low settings/AddProviderInstanceDialog.tsx:141

ProviderInstanceId.make(instanceId) at line 151 is invoked outside the try block that starts at line 156. If the brand constructor throws (e.g., due to validation rule drift between validateInstanceId and ProviderInstanceId.make), the error propagates unhandled rather than being caught and shown via the error toast at lines 164-169. Consider moving the call inside the try block so constructor failures surface in the UI.

No longer relevant as of 5be8c1260e6b9816f8bde76d05af42c043225070

🟡 Medium workspace/store.ts:982

openTerminalSurfaceForThread ignores the disposition parameter when matching terminal surfaces already exist. Unlike openThreadSurface which only looks for existing surfaces when disposition === "focus-or-tab", this function always focuses existing surfaces regardless of disposition. When a user requests "split-right" or "new-tab", they expect a new terminal to be created, not focus shifted to an existing one.

Consider adding a condition to only focus existing surfaces when disposition === "focus-or-tab", matching the behavior of openThreadSurface.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

🟠 High settings/ProviderInstanceCard.tsx:66

nextConfigBlobWithString stores the untrimmed value instead of trimmed, so whitespace surrounding API keys or base URLs is preserved in the config. This causes authentication failures or malformed URL errors at runtime when users paste values with leading/trailing spaces.

No longer relevant as of b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f

Empty registry when initial settings read fails

Medium Severity

On startup the hydration layer reads serverSettings.getSettings with orElseSucceed(() => undefined) and, on failure, seeds the registry with an empty ProviderInstanceConfigMap. If the very first read fails for any reason, no built-in driver default instance is materialized and the registry stays empty until the next streamChanges emission — which may never arrive if settings are not edited. Every consumer that resolves by instanceId (text generation, provider service, etc.) will fail with "No provider instance registered" until then.

^{Reviewed by Cursor Bugbot for commit b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f. Configure here.}

Disabled instance check missing in text generation routing

Low Severity

The module docstring states that "Unknown or disabled instances surface a TextGenerationError with the missing instanceId," but resolveInstance only checks whether the instance exists in the registry — not whether it is enabled. A user who has explicitly disabled a provider instance can still trigger text generation against it via thread/PR/branch flows that route through instanceId. Either the documented intent or the implementation needs to be aligned.

^{Reviewed by Cursor Bugbot for commit b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f. Configure here.}

🟡 Medium src/serverSettings.ts:175

Custom provider instances with non-built-in drivers are incorrectly overridden with fallback providers. When selection.instanceId refers to a custom instance whose driver is not a built-in driver ID, the code bypasses the enabled check at line 167 (because isBuiltInDriverId(selectedDriver) is false), then fails the second check at line 176 (because selection.instanceId itself isn't a built-in driver ID), and falls through to the fallback logic at lines 182-193. This replaces the user's valid, enabled custom provider selection with a built-in fallback, even when their custom instance is properly configured and enabled.

Consider adding an explicit check for custom (non-built-in) instances that verifies instanceConfig?.enabled ?? true is true before falling through to fallback logic.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟠 High

https://github.com/pingdotgg/t3code/blob/b1bb7a2f2485a0c7a4ecfda938c517db67b9b98f/apps/server/src/provider/Layers/CursorProvider.ts#L1159

discoverCursorModelsViaAcp was updated to accept an environment parameter, but the caller in checkCursorProviderStatus at line 1159 still invokes it with only cursorSettings. This causes custom environment variables passed to checkCursorProviderStatus (e.g., processEnv from CursorDriver) to be dropped during ACP model discovery, falling back to process.env instead. Consider passing environment as the second argument.

No longer relevant as of 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟡 Medium settings/ProviderInstanceCard.tsx:295

In ProviderEnvironmentSection, when a user adds a new variable row and edits the value field before filling in the name, committing the value triggers publishRows. The filter ENVIRONMENT_VARIABLE_NAME_PATTERN.test(row.name) removes rows with empty names, so the row is dropped from props.onChange. The useEffect then syncs local state from props.environment, causing the entire row — including the user's typed value — to silently disappear. This is a data-loss bug: data the user entered is discarded without warning.

Consider keeping rows with empty names in local state but not publishing them upstream until the name is valid, or showing validation feedback instead of dropping the row.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟢 Low settings/AddProviderInstanceDialog.tsx:155

When saving a provider instance, line 157 spreads the raw accentColor state value instead of the normalized result from normalizeProviderAccentColor(accentColor). If the user enters a color with leading/trailing whitespace like " #2563eb ", the condition passes (the normalized value is truthy) but the spread uses the untrimmed original, persisting invalid data.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟠 High settings/SettingsPanels.tsx:661

When synthesizing a default instance from legacy config at line 661-662, legacyConfig.enabled is nested inside config but not hoisted to instance.enabled. ProviderInstanceCard reads instance.enabled ?? true, so disabled legacy providers incorrectly appear enabled in the UI. Consider spreading enabled from legacyConfig into the synthesized instance.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟡 Medium

https://github.com/pingdotgg/t3code/blob/ed6d810dae42cfc57a8ece40d3d5a30b0f88e734/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts#L864

When processSessionStopRequested creates the stopped session (lines 864-876), it omits providerInstanceId while preserving providerName. This causes providerInstanceId to be lost on session stop, so subsequent code at lines 291-292 falls back to thread.modelSelection.instanceId instead of the previously-bound instance. Consider including providerInstanceId in the stopped session object.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟡 Medium Layers/OpenCodeAdapter.ts:1271

stopSession unconditionally emits session.exited even when stopOpenCodeContext has already been called by a racing emitUnexpectedExit. When the race occurs, both functions emit the event because stopOpenCodeContext returns early when its getAndSet loses, but stopSession doesn't check the result. Consider checking the return value of stopOpenCodeContext (or checking context.stopped after it returns) to skip emitting when another caller already handled shutdown.

✅ Resolved in 0013b64081e821d254fe7cc4080b6946f5dbec7c

🟢 Low Layers/ProviderCommandReactor.ts:354

At line 355, thread.session !== null is used to detect an active session, but this is wrong when thread.session.status === "stopped". In that case activeThreadSession is null (lines 297-299) but thread.session !== null remains true, causing the driver-switch check at lines 360-377 to incorrectly block users from starting fresh with a different provider. The condition should be activeThreadSession !== null.

INvalid. we don't wanna allow switching drivers

🟢 Low src/providerInstances.ts:243

resolveSelectableProviderInstance can return a disabled or unavailable instance when no enabled+available entries exist, contradicting its documented contract of returning "the first enabled instance as a fallback so downstream code can still send a turn." The fallback chain ?? requested?.instanceId ?? instanceId at lines 245–247 returns requested.instanceId (potentially disabled/unavailable) or the original instanceId (unknown state) when no enabled+available providers are found, allowing downstream code to attempt turns against non-functional providers.

✅ Resolved in 7a654d5f3730e9e966a92387944173e817e81df8

🟡 Medium src/providerRuntime.ts:248

Using BuiltInDriverKind for the provider field rejects events from forked drivers (e.g., "ollama"). The comment above explicitly states BuiltInDriverKind is "not a validation gate" and the field must accept unknown driver kinds for forward/backward compatibility. Consider switching to ProviderDriverKind (the open branded slug) so unknown driver values pass through and can be handled at runtime.

No longer relevant as of 41e667767d850d7b0307c120375788dcf2dd9c38

🟡 Medium Layers/ProviderRegistry.ts:526

The orElseSucceed fallback in refresh (line 529) and refreshInstance (line 534) returns an empty array when the underlying refresh fails. This differs from the inner logic at lines 348-350 and 360-362, which returns the cached providersRef list when a source is not found. An actual network failure during refresh therefore flashes an empty provider list to the UI instead of preserving the last known good state.

✅ Resolved in b9c11e7aed9b75412ba29054d227a128f68bccc4

🟡 Medium Layers/ProviderRegistry.ts:520

The Stream.runForEach consumer forked at lines 520-522 has no error handling around the entire syncLiveSources effect. If instanceRegistry.listInstances or instanceRegistry.listUnavailable fail, the error propagates up and terminates the consumer fiber. Once the fiber dies, the aggregator silently stops reacting to instance registry changes, leaving the UI with stale provider state. Consider wrapping the syncLiveSources call in Effect.ignoreCause or Effect.catchAll so the subscription stays alive.

✅ Resolved in b9c11e7aed9b75412ba29054d227a128f68bccc4

Metric test asserts branded type against plain string attribute

Low Severity

The metric attributes closure on line 88 uses a plain let provider = "unknown" (line 79) that is reassigned to the plain string "codex" (line 82). The assertion then checks against ProviderDriverKind.make("codex"). This works at runtime because branded types are structurally just strings, but it creates a misleading inconsistency — the recorded attribute is a plain string while the assertion uses a branded type. If the intent is to verify that production code passes ProviderDriverKind values into metric attributes, the test setup (line 82) should also use ProviderDriverKind.make("codex") to mirror real usage; otherwise it's testing a coincidence rather than the intended contract.

^{Reviewed by Cursor Bugbot for commit 01f2b68276a37669fed81422e58a5699af8d8fab. Configure here.}

🟢 Low Layers/OpenCodeAdapter.ts:1149

When modelSelection is undefined (no model from input.modelSelection and no context.session.model), the validation at line 1149 produces a confusing error message. modelSelection?.instanceId evaluates to undefined, which is not equal to boundInstanceId, triggering the error "OpenCode model selection is bound to instance 'undefined', expected '...'". The actual problem is that no model was specified at all, not an instance mismatch. A subsequent validation at line 1156-1163 would more accurately report "must use the 'provider/model' format" but is never reached due to the earlier check. Consider checking for modelSelection existence before validating instanceId, or adjusting the error message to distinguish "no model" from "wrong instance".

No longer relevant as of 245ec796d1faad7eb4e454999f82c41824c9e35c

🔴 Critical web/index.html:100

The new <script src="https://ui.sh/ui-picker.js"> loads and executes arbitrary JavaScript from an untrusted external domain with no integrity check, granting it full access to the page's DOM, cookies, localStorage (including t3code:theme and any auth tokens), and the ability to exfiltrate data or modify the application. If this inclusion is intentional, document the security review and add Subresource Integrity; otherwise remove it.

No longer relevant as of 245ec796d1faad7eb4e454999f82c41824c9e35c