feat: add session-scoped secondary model with Alt+S picker option - #3463
feat: add session-scoped secondary model with Alt+S picker option#34637Sageer wants to merge 3 commits into
Conversation
Running windows no longer follow secondary_model changes made elsewhere: the session captures the pool default at first use, and an explicit session-scoped selection (Alt+S in /secondary-model) overrides it and is persisted in the session directory so it survives resume. Enter now also applies the choice to the current session before persisting it as the default for future sessions.
🦋 Changeset detectedLatest commit: 9d30aa0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@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: 8a3dcd6976
ℹ️ 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".
| "@moonshot-ai/kimi-code-sdk": minor | ||
| --- | ||
|
|
||
| Add `Session.setSecondaryModel()` and `Session.getSecondaryModel()` for the session-scoped secondary model on the v2 engine. The v1 client rejects the setter with `NOT_IMPLEMENTED` and the getter reads back `undefined`. |
There was a problem hiding this comment.
This entry contains two sentences, while the repository requires every changeset to be one short user-facing sentence stating only what changed. Combine the v1 behavior into the first sentence so the generated release note conforms to the mandated format.
AGENTS.md reference: AGENTS.md:L85-L86
Useful? React with 👍 / 👎.
| const parameters = exposesSubagentModelChoice( | ||
| this.config, | ||
| this.flags, | ||
| this.subagents.secondaryModel, | ||
| ) |
There was a problem hiding this comment.
When a session-only model is selected, this makes the Agent schema expose the model parameter, but the description still calls buildSubagentModelDescriptions without this.subagents.secondaryModel. With no configured pool it therefore advertises no model choices at all, and with a pool it continues marking the persisted default rather than the active session default; pass the same session value to the description builder as AgentSwarmTool does.
Useful? React with 👍 / 👎.
| const configPool = resolveSubagentModelPool(config); | ||
| const pool: SubagentModelPool = | ||
| configPool ?? | ||
| ({ | ||
| defaultModel: sessionDefault, | ||
| models: sessionDefault === undefined ? {} : { [sessionDefault]: '' }, | ||
| }); |
There was a problem hiding this comment.
If [secondary_model.models] exists and the user chooses a different configured alias with Alt-S, configPool wins here and the session alias is not added to pool.models. The subsequent code only emits the default when it is a pool key, so AgentSwarm omits the model actually used by default and displays no [default] marker; build an effective pool that represents the session selection even when it is absent from the persisted pool.
Useful? React with 👍 / 👎.
| args.kind === 'reviewer' && !isSubagentModelForced(this.config) | ||
| ? 'primary' | ||
| : undefined, | ||
| this.subagents.secondaryModel, |
There was a problem hiding this comment.
On a resumed session, SessionSubagentService loads secondary-model.json asynchronously and the session lifecycle does not await its ready promise. If a tower spawn reaches this line before that read finishes, the synchronous getter seeds from the current global config and the first worker can launch on the wrong model; resolve the value through the awaited getSecondaryModel() path before constructing the binding, as planSpawn already waits for restoration.
Useful? React with 👍 / 👎.
… await - build the effective subagent model pool from the config pool plus the session-scoped default, so an out-of-pool session selection stays requestable and shows the [default] marker (Agent tool included) - tower spawn awaits the restored session secondary model instead of reading the pre-load synchronous getter - compress the sdk changeset entry to one sentence
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 035aa98732
ℹ️ 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".
| if (!this.secondaryModelSeeded) { | ||
| this.secondaryModelSeeded = true; | ||
| this.secondaryModelSeed = resolveSubagentModelPool(this.configService)?.defaultModel; |
There was a problem hiding this comment.
When a session is created but this getter has not run yet—for example, an SDK caller creates a session before sending its first prompt—the default is not captured until line 92. If another process edits the shared configuration in between, this already-started session adopts the new default, contrary to the documented guarantee that changes affect only newly started sessions. Seed the value during session service construction or materialization rather than on first read.
Useful? React with 👍 / 👎.
| onSessionOnlySelect: host.engineV2 | ||
| ? ({ alias }) => { | ||
| host.restoreEditor(); | ||
| void performSecondaryModelSave(host, alias, false); |
There was a problem hiding this comment.
With [secondary_model].force = true, this callback is still offered for every v2 session, so Alt-S successfully stores the selected alias and reports that new subagents will use it. However, resolveSubagentBinding returns the forced model before consulting the session value, making the action a silent no-op. Suppress or reject the session-only action when force is active instead of displaying a false success message.
Useful? React with 👍 / 👎.
| this.states.set(sessionSecondaryModelKey, model); | ||
| await this.documentStore.set(this.sessionContext.metaScope, SECONDARY_MODEL_DOC_KEY, { | ||
| model, | ||
| }); |
There was a problem hiding this comment.
The in-memory state is published before an unqueued asynchronous document write. If the write fails, the setter rejects while getSecondaryModel() and subsequent spawns still use the supposedly failed value; if two SDK calls overlap, the writes can complete out of order and leave memory on one model while a resumed session loads the other. Serialize updates and publish the new state only once its corresponding write succeeds.
Useful? React with 👍 / 👎.
2 participants
Related Issue
N/A — maintainer PR raised from an internal design discussion (multiple TUI windows sharing
secondary_model), no tracking issue.Problem
[secondary_model]lives in the user-levelconfig.toml, and the v2 engine watches that file and hot-reloads it in every process. The subagent model binding was resolved per spawn from that live global config, so editing the secondary model in window A silently changed the model used by window B's already-running session (observed in practice: the next AgentSwarm batch picked up the other window's selection). Symmetrically, there was no way to choose a secondary model for just the current window —/secondary-modelcould only persist to the shared file.What changed
Adopts the established
/modelpattern (runtime session state + explicit persist):SessionSubagentServicenow owns a session-scoped secondary model binding. It is seeded from the config pool default at first use — a running session no longer follows global hot reloads — and an explicit selection is persisted in the session directory (secondary-model.json), so it survives resume. Spawn resolution order is now: tool argument >force> session selection > pool default > inherit.Agent/AgentSwarm/TowerSpawnall resolve through it, and the tool description's[default]marker reflects the session value.Session.setSecondaryModel()/Session.getSecondaryModel(). The v2 client goes through the in-process session scope (same path assetThinking); the v1 client rejects the setter withNOT_IMPLEMENTEDand reads backundefined./secondary-modelpicker gains the existing Alt+S session-only shortcut (offered on v2 only); Enter now applies the choice to the current session first, then persists it as the default for future sessions.config-files.mdandslash-commands.md.Tests: new engine cases (seed isolation from mid-session config changes, override precedence,
forceprecedence, implicit pool, persistence round-trip), an SDK end-to-end case including resume, and TUI command cases for both picker paths.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.