fix(mcp): route t3_thread_start/t3_spawn_subagent by plain model name#6
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af5fa2340a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThis PR routes
Confidence Score: 5/5Safe to merge. The new model-resolution path is covered by 17 unit tests including the previously-missing cursor-alias case, and the single-resolution invariant between the coordinator and the thread is now enforced by stripping the bare model field before delegating to spawnRuntime. All three issues from the prior review round are fully addressed: pass-2 alias resolution now re-runs the priority walk on the canonical slug rather than returning the aliasing provider, the double-resolution race between coordinator and thread is eliminated by stripping the model field before spawnRuntime, and the missing cursor-only-alias test is in place. No new defects were found in the changed paths. No files require special attention. The handler integration tests mock ProviderInstanceRegistry with an empty list and do not exercise the new model field directly, but the pure-function unit tests cover that logic exhaustively.
|
| Filename | Overview |
|---|---|
| packages/shared/src/model.ts | Adds pickModelSelectionFromInstances, ProviderModelEntry, and ProviderModelSource interfaces. Pass-2 alias resolution now collects all canonicals from all providers into a Set, then re-runs the priority-sorted resolveSlug for each canonical — fixing the prior bug where cursor-only aliases bypassed PROVIDER_INFERENCE_PRIORITY. |
| packages/shared/src/model.test.ts | Adds 9 unit tests for pickModelSelectionFromInstances, including the cursor-only-alias test (opus-4.6-thinking → claudeAgent) and the Fable-5 live-list case. Complete coverage of null/empty inputs, default options, multi-instance tie-break, and alias resolution. |
| apps/server/src/mcp/toolkits/thread/handlers.ts | Injects ProviderInstanceRegistry, builds modelSources per-invocation, and delegates to pickModelSelectionFromInstances when input.model is set. resolveModelSelection is now an Effect returning ThreadStartToolError on unknown model — no silent fallback. |
| apps/server/src/mcp/toolkits/subagent/handlers.ts | Resolves model against live provider snapshots before calling spawnRuntime; strips model and injects the already-resolved modelSelection so the thread handler does not re-resolve against a potentially-different registry snapshot — fixing the double-resolution race from the prior review. |
| packages/contracts/src/model.ts | Adds PROVIDER_INFERENCE_PRIORITY constant ordering native providers before aggregators. No model-name patterns — purely provider-identity ordering. |
| apps/server/src/mcp/toolkits/thread/handlers.test.ts | Adds ProviderInstanceRegistry mock (empty list) to satisfy the new service dependency; existing 9 test cases continue to exercise inheritance paths correctly. |
| apps/server/src/mcp/toolkits/thread/tools.ts | Adds optional model: TrimmedNonEmptyString field to ThreadStartToolInput and updates the tool description to guide callers toward plain model names. |
| apps/server/src/mcp/toolkits/subagent/tools.ts | Description-only update to surface the model field (inherited via ...ThreadStartToolInput.fields spread) for callers of t3_spawn_subagent. |
Reviews (2): Last reviewed commit: "fix(mcp): route t3_thread_start/t3_spawn..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
[automated] Addressed in 91f8c3d: (1) exclude disabled provider instances from routing; (2) prefer the source thread's instance when multiple instances of a provider serve the model; (3) preserve the matched model's default option selections. Resolving. |
wizzoapp
Bot
force-pushed
the
fix/model-routing-tools
branch
from
af5fa23 to
91f8c3d
Compare
|
@codex review |
Callers had to pass a full modelSelection with the correct harness/instance id (e.g. "claudeAgent"), which the model had to guess. Now these tools accept an optional bare `model` name and resolve it against the LIVE provider model lists — the same upstream-maintained source the model picker uses — so new models (e.g. claude-fable-5) and custom models route correctly with no code change here; there are no hardcoded model-name patterns. - packages/shared/src/model.ts: pickModelSelectionFromInstances(model, sources) matches a plain name against each provider instance's live `snapshot.models`, resolving registry aliases (e.g. "opus") to the canonical slug. When several providers serve the same model (Claude via both claudeAgent and the Cursor aggregator), the native one wins via PROVIDER_INFERENCE_PRIORITY. Returns null for unknown models so callers fall back. Returns the actual serving instance, so custom instances work too. - thread/subagent handlers build the sources from providerInstanceRegistry.listInstances (+ each instance's getSnapshot) and resolve `model` -> selection, else explicit modelSelection, else inherit the source thread. - 17 model unit tests incl. the Fable-5 case (a live model absent from the alias maps) and native-provider tie-break; thread/subagent handler suites updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
[automated] Addressed in 5261fcb: (1) alias resolution now re-runs the priority-ranked match on the canonical slug, so a provider-specific alias (e.g. Cursor's opus-4.6-thinking) still routes its canonical model to the native provider (+test); (2) subagent spawns with the already-resolved selection so the coordinator and thread share one snapshot. Resolving. |
wizzoapp
Bot
force-pushed
the
fix/model-routing-tools
branch
from
91f8c3d to
5261fcb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5261fcbe47
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const modelSelection = resolveModelSelection(input, sourceThread); | ||
| const providerInstances = yield* providerInstanceRegistry.listInstances; | ||
| const modelSources = yield* Effect.forEach( | ||
| providerInstances.filter((providerInstance) => providerInstance.enabled), |
There was a problem hiding this comment.
When a provider is enabled but currently unavailable, its snapshot can still advertise fallback models (for example checkClaudeProviderStatus returns models: allModels on CLI/auth probe failures). Because inference only filters on providerInstance.enabled, model: "claude-sonnet-4-6" will prefer the native claudeAgent entry over a working Cursor instance whenever Claude is enabled-but-broken, causing new threads/subagents to route to a provider that cannot run instead of to an actually available provider. Please filter the snapshot by provider status/auth availability, or otherwise rank unavailable snapshots after available ones; the same source-building pattern in the subagent handler needs the same treatment.
Useful? React with 👍 / 👎.
| const modelSources = yield* Effect.forEach( | ||
| providerInstances.filter((providerInstance) => providerInstance.enabled), | ||
| (providerInstance) => | ||
| Effect.map(providerInstance.snapshot.getSnapshot, (snapshot) => ({ |
There was a problem hiding this comment.
This reads each raw instance snapshot, but the model picker is backed by ProviderRegistry, which hydrates cached provider status and merges prior models when a live/pending snapshot is empty. Right after restart, Codex's pending snapshot contains only custom models until the probe finishes, while the picker can still show cached built-in models such as gpt-5.4; an explicit model: "gpt-5.4" then fails as “not served” even though the user selected a visible model. Resolve against the same merged provider catalog (or equivalent cached merge) for both thread and subagent source-building.
Useful? React with 👍 / 👎.
| const canonicals = new Set<string>(); | ||
| for (const source of sources) { | ||
| const aliases = MODEL_SLUG_ALIASES_BY_PROVIDER[source.driverKind] ?? {}; | ||
| const canonical = Object.prototype.hasOwnProperty.call(aliases, trimmed) | ||
| ? aliases[trimmed] |
There was a problem hiding this comment.
Alias expansion is limited to driver kinds present in sources, so if claudeAgent is disabled/absent but an enabled aggregator like Cursor serves claude-opus-4-8, an alias such as opus never enters canonicals and the tool rejects the request even though a live provider can run the canonical model. Collect aliases from the registry maps before matching the resulting canonical slugs against live sources (or otherwise allow aliases whose canonical slug is served by an enabled provider).
Useful? React with 👍 / 👎.
| * attribute of provider identity, not of model names, so it needs no change | ||
| * when new models ship: those are picked up from the registry maps above. | ||
| */ | ||
| export const PROVIDER_INFERENCE_PRIORITY: ReadonlyArray<ProviderDriverKind> = [ |
There was a problem hiding this comment.
AGENTS.md says packages/contracts must stay schema-only, but this new export is runtime routing policy consumed by @t3tools/shared/model to decide which provider wins. Keeping provider inference priority in contracts makes contract consumers depend on server/model-picker policy and expands the package boundary; move this constant to a runtime/shared module instead of adding more non-schema logic here.
Useful? React with 👍 / 👎.
Problem
To run
t3_thread_start/t3_spawn_subagentwith a specific model, callers had to pass a fullmodelSelectionobject with the correct harness/instance id (e.g."claudeAgent") — which the calling model had to guess.Fix
These tools now accept an optional plain
modelname (e.g.claude-opus-4-8,gpt-5.4). The provider/harness is inferred from the live provider model lists — the same upstream-maintained source the model picker uses — so:claude-fable-5) and custom models route correctly with zero code change here, purely from the registry data.claudeAgentand the Cursor aggregator), the native provider wins viaPROVIDER_INFERENCE_PRIORITY.opus) resolve to the canonical live slug.modelfails with a clear error (rather than silently running on the inherited model); omittingmodelinherits as before.Key files
packages/shared/src/model.ts:pickModelSelectionFromInstances(model, sources)(pure, 17 unit tests incl. the Fable-5 case + native tie-break).thread/handlers.ts+subagent/handlers.ts: build sources fromproviderInstanceRegistry.listInstances(+ each instance'sgetSnapshot) and resolve.model.Test
vp checkclean; typecheck clean; model tests 17/17; thread+subagent handler suites 9/9. Autoreview (Codex+Claude) clean.This is Fix #2 of the harness fixes. The
t3_schedule_*model field (persisted per-task) is a focused follow-up (needs a DB migration + JSON column).