feat(skills): add model override support via skill frontmatter#2949
Open
tanzhenxin wants to merge 4 commits intomainfrom
Open
feat(skills): add model override support via skill frontmatter#2949tanzhenxin wants to merge 4 commits intomainfrom
tanzhenxin wants to merge 4 commits intomainfrom
Conversation
Allow skills to specify a `model` field in YAML frontmatter to override which model is used for subsequent turns within the same agentic loop. The override flows through ToolResult → ToolCallResponseInfo → SendMessageOptions and naturally expires when the loop ends. Resolves #2052
Contributor
📋 Review SummaryThis PR implements model override support for skills via YAML frontmatter, enabling skills to specify which model should be used for subsequent turns within the same agentic loop. The implementation is well-designed, follows existing patterns, and includes comprehensive test coverage. The approach of using 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo critical issues identified. 🟡 HighFile: The model field parsing logic is duplicated between // Extract optional model field
const modelRaw = frontmatter['model'];
let model: string | undefined;
if (modelRaw !== undefined) {
if (typeof modelRaw !== 'string') {
throw new Error('"model" must be a string');
}
// "inherit" means explicitly use the session model — treat as no override
model = modelRaw.trim() === 'inherit' ? undefined : modelRaw.trim();
}Recommendation: Extract this into a shared utility function in 🟢 MediumFile: The JSDoc mentions Recommendation: Update the documentation to clarify that cross-provider switching via /**
* Optional model override for this skill's execution.
* Note: Currently only same-provider model switching is supported.
* Cross-provider switching (using `authType:modelId` syntax) is planned
* for a future release.
* Uses bare model ID (e.g., `qwen-coder-plus`), or omitted/`inherit`
* to use the session model.
*/
model?: string;File: The Recommendation: Consider adding validation in the core tool scheduler or client to provide early feedback when an invalid model is specified. 🔵 LowFile: The model override state is managed with a simple Recommendation: This is optional, but consider creating a small helper class or function to manage the override lifecycle, especially if future iterations add more complexity (e.g., per-turn overrides, override chains). File: The existing tests don't cover the new Recommendation: Add test cases for:
Example test structure: describe('model field parsing', () => {
it('should parse valid model string', () => {
const content = `---
name: test
description: test skill
model: qwen-coder-plus
---
Body content`;
const config = parseSkillContent(content, '/test/path');
expect(config.model).toBe('qwen-coder-plus');
});
it('should treat "inherit" as undefined', () => {
// ...
});
it('should reject non-string model values', () => {
// ...
});
});✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Fixes strict equality test failures in nonInteractiveToolExecutor.test.ts where the extra undefined modelOverride field caused object mismatch.
- Wire up modelOverride in interactive CLI path (useGeminiStream) - Fix inherit/no-model unable to clear a prior override by using 'in' operator instead of truthiness checks in scheduler and CLI - Reject empty/whitespace model strings in parseModelField() - Extract shared parseModelField() to deduplicate skill-load and skill-manager parsing logic - Propagate modelOverride through stop-hook continuation in client
wenshao
reviewed
Apr 7, 2026
Comment on lines
1569
to
+1576
|
|
||
| // Extract model override from skill tool results (last one wins) | ||
| let toolModelOverride: string | undefined; | ||
| for (const toolCall of geminiTools) { | ||
| if ('modelOverride' in toolCall.response) { | ||
| toolModelOverride = toolCall.response.modelOverride; | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
[Critical] toolModelOverride is recomputed from only the current batch of tool results and then passed to the next submitQuery(...), but it is not persisted for the rest of the interactive agentic loop. If the next turn runs only non-skill tools, the override silently falls back to the session default model.
Suggested change
| // Extract model override from skill tool results (last one wins) | |
| let toolModelOverride: string | undefined; | |
| for (const toolCall of geminiTools) { | |
| if ('modelOverride' in toolCall.response) { | |
| toolModelOverride = toolCall.response.modelOverride; | |
| } | |
| } | |
| // Extract model override from skill tool results (last one wins) | |
| for (const toolCall of geminiTools) { | |
| if ('modelOverride' in toolCall.response) { | |
| setModelOverride(toolCall.response.modelOverride); | |
| } | |
| } |
— gpt-5.4 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in def5adb.
The override is now stored in a modelOverrideRef that persists across tool-result turns within the same agentic loop. It resets to undefined when a new user query starts, so overrides don't leak across messages. The submitQuery parameter was removed since it reads from the ref directly.
wenshao
requested changes
Apr 7, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Request changes — see inline comments.
One additional blocker is not on a changed line, so GitHub cannot attach it inline:
- In
packages/cli/src/nonInteractiveCli.ts, the cron execution path still does not propagatemodelOverrideacross tool-result turns. The main non-interactive loop now keepslet modelOverrideand updates it from skill tool results, but the cron loop still callssendMessageStream(..., { type: ... })without an override and never readstoolResponse.modelOverride. As a result, the same skill behaves differently in normal non-interactive mode vs cron mode.
Reviewed by gpt-5.4 via Qwen Code /review
…ron paths The interactive path stored the skill model override in a local variable, causing it to be lost when subsequent non-skill tool turns ran. Use a ref to persist the override for the duration of the agentic loop, resetting on new user messages. Also propagate modelOverride in the cron execution loop for consistency with the main non-interactive path.
Collaborator
Author
|
Re: cron path missing Added |
Collaborator
Author
E2E Test Report: Skill Model OverrideBranch: Results
What we verified
Not covered yetCross-provider switching (Tests 6-7) needs the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Add
modelfield to skill YAML frontmatter, enabling skills to override which model is used for subsequent turns within the same agentic loop. When a skill specifiesmodel: qwen-coder-plus, the API requests after the skill tool call use that model instead of the session default. The override naturally expires when the agentic loop ends.This implements same-provider model switching (phase 1). Cross-provider switching (requiring ContentGenerator threading) is deferred to a follow-up.
Screenshots / Video Demo
N/A — no user-facing UI change. The feature is exercised via skill frontmatter configuration. E2E validation was done by inspecting OpenAI-format API logs (
--openai-logging):"model": "glm-5"(session default)"model": "qwen3-coder-plus"(skill override)Dive Deeper
How it works: The
modelstring flows through a simple pipeline:SkillConfig.model(parsed in bothskill-load.tsandskill-manager.ts)SkillToolInvocation.execute()→ToolResult.modelOverrideCoreToolScheduler→ToolCallResponseInfo.modelOverridenonInteractiveCli.ts) captures the override and passes it asSendMessageOptions.modelOverrideGeminiClient.sendMessageStream()uses the override instead ofconfig.getModel()when callingTurn.run()Scoping: The override lives in the
SendMessageOptionsparameter, not in persistent config. It naturally expires when the agentic loop ends (no more tool calls). The next user message starts fresh.inherithandling:model: inheritis treated as no override (stored asundefined), same as omitting the field entirely.Cross-provider limitation: The current implementation only swaps the model string. Same-provider switches work because the existing
ContentGeneratorhandles different model IDs within the same provider. Cross-provider switches (e.g.,anthropic:kimi-k2.5) require threading a separateContentGeneratorthrough the call chain — this is theresolveModelOverride()work described in the design doc (sections 4a-4c) and will be a follow-up PR.Reviewer Test Plan
Build and bundle:
npm run build && npm run bundleQuick unit test validation:
E2E validation (same-provider override):
/tmp/api-logs/— the second request'smodelfield should differ from the first.Additional scenarios to verify:
model: inherit→ all requests use session defaultmodelfield → all requests use session default (backwards compat)model: nonexistent-model→ skill loads OK, API handles the unknown modelTesting Matrix
Linked issues / bugs
Resolves #2052