feat(agent-core): per-workspace subagent model bindings#1928
Conversation
🦋 Changeset detectedLatest commit: f8293ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccf0f39e9d
ℹ️ 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 { child, profileName } = await this.ensureIdleSubagent(agentId, parent); | ||
| // Sticky semantics: a resumed child always keeps its configured | ||
| // model/effort; mid-conversation model switches are not supported. | ||
| const modelAlias = this.resolveChildModel(parent, child.config.modelAlias); |
There was a problem hiding this comment.
Gate sticky resume behind the experiment
When the experiment is off, this still changes the default resume path: the only flag check is in AgentTool's binding lookup, but Agent(resume=...) now resolves the child's persisted alias instead of the current parent alias. A user who changes or removes the main model and then resumes an old subagent will keep the stale child model or get CONFIG_INVALID, whereas the pre-change code realigned to the parent; gate sticky resume/retry with subagent-model-selection or keep the old realignment for unbound children.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the sticky resume was applying unconditionally, which contradicted the "default behavior unchanged" claim. Fixed: resume()/retry() now check parent.experimentalFlags.enabled('subagent-model-selection'). With the flag on, a resumed child keeps its configured model/effort (sticky, no mid-conversation switches; unresolvable stored aliases fail fast with CONFIG_INVALID). With the flag off, resume realigns the child to the parent's current model exactly as before, byte-for-byte the old behavior — effort is untouched on resume in both modes. Added test coverage for both the flag-off realign path and the flag-on sticky/error paths.
| const operation = resumeAgentId !== undefined && resumeAgentId.length > 0 ? 'resume' : 'spawn'; | ||
| // Workspace model binding (experiment-gated): applied mechanically at | ||
| // spawn; the first unbound spawn may ask the user once interactively. | ||
| const binding = await this.resolveSpawnBinding(requestedProfileName ?? 'coder', operation, true); |
There was a problem hiding this comment.
Apply workspace bindings to AgentSwarm launches
With workspace binding enabled, this is the only path that reads or asks for .kimi-code/local.toml bindings. AgentSwarm launches item-based subagents through AgentSwarmTool → SessionSubagentHost.runQueued() → spawn() without going through this code or passing modelAlias/thinkingEffort, so /subagent-model set coder is ignored for swarm-spawned coder agents even though the feature is described as applying to every new subagent of that type. Move binding resolution into the shared spawn path or wire it through AgentSwarm too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — AgentSwarm launches go through runQueued() and never see the bindings. For this PR we've deliberately scoped bindings to Agent-tool spawns only and narrowed the docs accordingly (bindings are applied to subagents spawned via the Agent tool; AgentSwarm support is future work). Rationale: swarm model routing deserves its own design (per-item vs per-batch bindings, ramp behavior), and we'd rather not ship a half-considered version of it here. If maintainers prefer, a clean follow-up is to move binding resolution into the shared SessionSubagentHost.spawn path so swarm benefits mechanically with no swarm-specific code — happy to do that in a separate PR.
| if (target.modelAlias === undefined && subagent.modelAlias !== undefined) { | ||
| target.modelAlias = subagent.modelAlias; |
There was a problem hiding this comment.
Store profile overrides on the subagent edge
Profile bindings are properties of an owner's subagents entry, but this mutates the shared resolved target profile. If two owner profiles bind the same subagent type differently, whichever owner is processed first permanently sets target.modelAlias, so later owners silently run that subagent on the wrong model. Keep the override on the owner-to-subagent link instead of the global target profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reply: profile overrides are stored on the shared resolved target
Thanks for the careful read — the mechanism is described accurately. Sharing our analysis of why we kept it, and what the actual blast radius is.
What the mechanism is
A model binding declared in an owner's subagents: entry is conceptually a property of the owner → subagent edge ("when this owner delegates to coder, use model M"). applySubagentOverrides instead writes it onto the shared resolved target profile (coder.modelAlias = ...), because every subagent type resolves to a single global ResolvedAgentProfile object.
When it would break
The failure mode requires two owner profiles binding the same subagent type differently:
# agent.yaml
subagents:
coder:
modelAlias: cheap-model
# researcher.yaml
subagents:
coder:
modelAlias: strong-modelBoth entries point at the same coder object; the first owner processed wins, and the second owner's binding is silently skipped. Codex's point is exactly right about this.
Why we assess it as low-risk in practice
- Unreachable with shipped configuration. The bundled profiles have exactly one owner (
agent.yaml) declaring subagent entries. No second owner exists, so the conflict cannot occur out of the box. - The pattern is pre-existing, not introduced here. The
descriptionfield has used the identical shared-target mechanism sinceapplySubagentDescriptions. This PR extends an established pattern rather than inventing a new one — if the shared-target hazard ever becomes real, it needs solving fordescriptionandmodelAlias/thinkingEfforttogether. - Bounded impact. The worst case is a subagent running on an unintended model — a configuration surprise, not data loss or a correctness failure, and it is corrected by editing one binding.
- Discovery cost is low by design of this PR. The effective model is now visible in the tool result (
model:line), thesubagent.spawnedevent payload, andAgentBackgroundTaskInfo. A wrong-model outcome is observable immediately rather than silent.
What we did about it
- Added a code comment on
applySubagentOverridesdocumenting the shared-target limitation and the direction of the proper fix (moving overrides onto the owner→subagent edge). - We explicitly chose not to do the edge-storage refactor in this PR: it would change the shape of
ResolvedAgentProfile.subagentsand every consumer for a scenario that is currently unreachable.
If maintainers disagree
Two clean fallbacks, and we're happy to take either:
- Edge storage: keep
{ profile, modelAlias, thinkingEffort }per owner→subagent link and resolve per-owner at spawn time. - Drop profile bindings from this PR: the workspace
local.tomlbindings are the core feature and cover the user need; profile-level bindings were included for completeness and can be split into a follow-up.
…xperiment Address review feedback on MoonshotAI#1928: - resume/retry only keep the child's configured model when subagent-model-selection is enabled; with the experiment off, resume realigns to the parent's current model exactly as before - spawn-time profile modelAlias/thinkingEffort bindings are likewise ignored unless the experiment is enabled - docs: workspace bindings apply to Agent tool spawns only; AgentSwarm does not read bindings yet (swarm-wide routing is future work) - note the shared-target limitation of profile override resolution
d5dca1f to
9bd3cb2
Compare
|
Thanks — the experiment-gating part looks fixed, but I think two original review points are still unresolved.
|
|
Thanks for the work here — the overall direction looks good, but I think there’s still a remaining freshness gap around From the PR notes, this path can stall for >45s on macOS under multi-instance fs load because it still depends on fs event delivery. I agree that Suggested fix:
|
Thanks for pushing on this — you're right that narrowing the docs doesn't change behavior, and I appreciate the concrete suggestion. |
This looks unrelated to this PR's scope (per-workspace subagent model bindings) — the session-list freshness/watch mechanism isn't touched here and deserves its own issue or PR. Flagging so it doesn't get lost |
Add an experimental subagent-model-selection feature (off by default, KIMI_CODE_EXPERIMENTAL_SUBAGENT_MODEL_SELECTION=1) that binds configured model aliases and thinking efforts to subagent types per workspace: - Bindings live in <projectRoot>/.kimi-code/local.toml [subagent.<type>] and are applied mechanically at spawn (workspace binding > profile binding > parent inheritance); the calling agent never sees or overrides them, removing LLM discretion from model routing - First unbound spawn of a type asks the user once interactively and persists the answer, including an explicit keep-inheriting choice - /subagent-model command (list/set/clear) manages bindings via a new session RPC chain (agent-core -> node-sdk -> TUI) - Resume/retry are sticky: a subagent always keeps the model/effort it was configured with (persisted via config.update wire records); no mid-conversation model switches. Unresolvable stored aliases fail fast with a configuration error instead of silently realigning - Agent profiles can bind modelAlias/thinkingEffort per subagent type - Effective model/effort are exposed in the subagent.spawned event, AgentBackgroundTaskInfo, approval label, and tool result text
…xperiment Address review feedback on MoonshotAI#1928: - resume/retry only keep the child's configured model when subagent-model-selection is enabled; with the experiment off, resume realigns to the parent's current model exactly as before - spawn-time profile modelAlias/thinkingEffort bindings are likewise ignored unless the experiment is enabled - docs: workspace bindings apply to Agent tool spawns only; AgentSwarm does not read bindings yet (swarm-wide routing is future work) - note the shared-target limitation of profile override resolution
9bd3cb2 to
f8293ac
Compare
Related Issue
Resolve #1927
Problem
See linked issue.
What changed
Bindings belong to the user, not the model. Model selection for delegated work is treated as user configuration, applied mechanically — the calling agent never sees or overrides it. This deliberately avoids LLM-facing model directories/parameters: in testing, agents would self-fill a model argument in ways that silently overrode user intent.
<projectRoot>/.kimi-code/local.tomlunder[subagent.<type>], following the existingworkspace.additional_dirpattern/subagent-modelcommand (list/set <type>/clear <type>) via a new session RPC chain (agent-core → node-sdk → TUI)modelAlias/thinkingEfforton profilesubagentsentries) > parent inheritanceconfig.updatewire records); no mid-conversation switches. Unresolvable stored aliases fail fast with a configuration error instead of silently realigning to the parent — a deliberate behavior change from today's realign-on-resumesubagent.spawnedevent,AgentBackgroundTaskInfo, approval label, and tool result textsubagent-model-selectionexperiment (off by default,KIMI_CODE_EXPERIMENTAL_SUBAGENT_MODEL_SELECTION=1), so default behavior is unchangedValidation
/subagent-modelflows — all passing/bin/bash, symlink/UNC semantics)tsc --noEmitclean (agent-core, protocol, node-sdk, apps/kimi-code); oxlint 0 errors in touched files/subagent-modellist/clear, all verifiedChecklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.