feat(core): add fastOnly/voiceOnly flags to hide models from main model list - #5632
Merged
Conversation
callmeYe
force-pushed
the
feat/fast-only-model-flag
branch
from
June 22, 2026 11:20
e117f42 to
84fccd6
Compare
callmeYe
changed the title
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. |
wenshao
reviewed
Jun 22, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings (not on diff lines)
[Suggestion] CLI --fast and --voice paths don't cross-filter specialized models
The /model --fast <name> path (modelCommand.ts ~line 333-335) validates against getAvailableModelsForAuthType() / getAllConfiguredModels() without filtering out voiceOnly models. Symmetrically, /model --voice <name> (line ~242) doesn't filter out fastOnly models. The ModelDialog correctly applies cross-filtering in both fast and voice modes, but the CLI command paths are inconsistent. This means a voiceOnly model can be set as the fast model via CLI (but not via the dialog), and vice versa.
Suggested fix: add .filter((m) => !m.voiceOnly) on the --fast path and .filter((m) => !m.fastOnly) on the --voice path.
[Suggestion] No test coverage for the new filtering logic
No test file includes mock models with fastOnly: true or voiceOnly: true. The filter expressions in modelCommand.ts (line 393) and ModelDialog.tsx (lines 231-233), and the field propagation in modelRegistry.ts (lines 147-148, 234-235) are all untested. A regression in the filter logic (e.g., wrong operator) would silently pass the existing test suite.
[Suggestion] Arena dialog and ACP API don't filter fastOnly/voiceOnly models
ArenaStartDialog.tsx only filters !model.isRuntimeModel, so specialized models appear in the arena picker. acpAgent.ts buildAvailableModels() / buildConfigOptions() use getAllConfiguredModels() without filtering, so ACP clients can select specialized models as the main session model. If these are intentional (like the about command exclusion noted in the PR), consider adding a comment to document the decision.
— qwen3.7-max via Qwen Code /review
| (m.authType !== AuthType.QWEN_OAUTH || | ||
| authType === AuthType.QWEN_OAUTH), | ||
| authType === AuthType.QWEN_OAUTH) && | ||
| (isFastModelMode || !m.fastOnly) && |
Collaborator
There was a problem hiding this comment.
[Suggestion] A model configured with both fastOnly: true AND voiceOnly: true would be invisible in all three dialog modes. The filter (isFastModelMode || !m.fastOnly) && (isVoiceModelMode || !m.voiceOnly) evaluates to false for such a model in normal mode (false && false), fast mode (true && false), and voice mode (false && true), since the two mode flags are never both true simultaneously.
Consider adding validation in modelRegistry.ts (e.g., in validateModelConfig) that rejects or warns about configs with both flags set, to prevent accidentally creating an unreachable model.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已修复。在 validateModelConfig 中添加了 warning，当 fastOnly 和 voiceOnly 同时设置时会输出告警日志。
| name: config.name || config.id, | ||
| baseUrl: config.baseUrl || this.getDefaultBaseUrl(authType), | ||
| generationConfig, | ||
| capabilities: config.capabilities || {}, |
Collaborator
There was a problem hiding this comment.
[Suggestion] These two explicit assignments are redundant — the ...config spread above already copies fastOnly and voiceOnly from ModelConfig into the result. Every other explicit property in this return object applies a default or transformation (name: config.name || config.id, baseUrl: config.baseUrl || this.getDefaultBaseUrl(authType), etc.), so these pass-through lines break the established pattern and may mislead future readers.
Suggested change
| capabilities: config.capabilities || {}, |
Remove these two lines. If a third visibility flag is added later, the ...config spread will handle it automatically.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已修复。移除了冗余的显式赋值，...config spread 已经覆盖了这些字段。
| config.getAvailableModelsForAuthType(targetAuthType); | ||
| const availableModels = config | ||
| .getAvailableModelsForAuthType(targetAuthType) | ||
| .filter((m) => !m.fastOnly && !m.voiceOnly); |
Collaborator
There was a problem hiding this comment.
[Suggestion] getAvailableModelIds() (line ~145) calls config.getAvailableModels() without filtering out fastOnly/voiceOnly models. This means tab-completion for /model <TAB> will suggest hidden model IDs, but the validation below rejects them with "Model not available" — confusing UX where a suggested completion is immediately rejected.
Apply the same filter in getAvailableModelIds():
return config.getAvailableModels()
.filter((m) => !m.fastOnly && !m.voiceOnly)
.map((m) => m.id);— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已修复。getAvailableModelIds() 现在过滤掉 fastOnly 和 voiceOnly 模型，tab 补全不再建议这些隐藏模型。
Collaborator
Author
|
Thanks for the review! Addressed all findings in ca1d69a: Cross-filter CLI paths — Fixed. Tab completion — Fixed. Both flags validation — Added a Redundant assignments — Removed explicit Test coverage — Acknowledged. The existing test suite covers the unchanged contract; the filter expressions are straightforward boolean checks. Will add targeted tests if this area grows more complex. Arena/ACP filtering — Intentionally left unfiltered, consistent with the diagnostic-view principle noted in the PR description. Arena model selection is an advanced/experimental feature where showing all models is reasonable. |
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: this solves a real problem — users who configure specialized models (small/fast for background tasks, whisper for voice) end up with clutter in the main model selector. Hiding those from the main list is the right call. No direct CHANGELOG reference, but model selector UX is an active area across agent CLIs. On approach: the scope is tight and additive — two optional booleans, straightforward filtering. The cross-filter pattern (fastOnly models hidden from voice selectors and vice versa) makes sense. The previously-flagged voice path cross-filter issue has been addressed in the latest commit. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：解决了真实问题——用户配置了专用模型（后台任务用的小模型、语音转录用的 whisper 等）会在主模型选择器中造成干扰，把它们从主列表中隐藏是正确的做法。CHANGELOG 中没有直接引用，但模型选择器 UX 在各 agent CLI 中都是活跃领域。 方案：范围紧凑且增量式——两个可选布尔值，直接的过滤逻辑。交叉过滤模式（fastOnly 模型在语音选择器中隐藏，反之亦然）合理。之前标记的语音路径交叉过滤问题已在最新提交中修复。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewThe previously-flagged issue — Everything else remains clean:
No new issues found. TestingUnit tests — all 94 pass (58 core + 36 CLI), including 9 new tests:
Non-interactive smoke tests (tmux): No regressions in non-interactive paths. The filtering logic is primarily exercised when fastOnly/voiceOnly models are configured in settings (which requires a custom — Qwen Code · qwen3.7-max |
Collaborator
|
Re-review confirms the previous round's finding (voice path missing This is a well-scoped, additive change — two optional booleans, minimal filtering, no scope creep. The author was responsive to feedback and addressed the one flagged issue promptly. Ready to ship. 中文说明复查确认上一轮发现的问题（语音路径缺少 这是一个范围明确、增量式的改动——两个可选布尔值，最小化的过滤逻辑，没有范围蔓延。作者积极响应反馈并迅速处理了标记的问题。可以合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously requested changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
The /model --voice CLI path is missing a .filter((m) => !m.fastOnly) — see the Stage 2 review comment for details. One-liner fix, otherwise looks good. 🙏
qwen-code-ci-bot
added
category/configuration
wenshao
reviewed
Jun 22, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] resolveVoiceTranscriptionConfig in voice-transcriber.ts:222 and getFastModel() in config.ts:2593 call getAllConfiguredModels() without filtering fastOnly/voiceOnly respectively. The selection-time commands correctly filter, but these runtime consumers don't — creating an inconsistency if a hidden model is manually set in config. Low practical risk (the normal selection path prevents this), but worth documenting or filtering for defense in depth.
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
| if (config.fastOnly && config.voiceOnly) { | ||
| debugLogger.warn( |
Collaborator
There was a problem hiding this comment.
[Suggestion] debugLogger.warn() writes exclusively to a debug log file gated behind QWEN_DEBUG_LOG_FILE. When that env var is unset (the default), this warning is silently dropped — no console output, no user-visible signal. A model misconfigured with both flags vanishes from every selector with zero diagnostic.
Consider using console.warn in addition to (or instead of) debugLogger.warn, or throwing a validation error at registration time so the misconfiguration surfaces immediately.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
同意这个观察。不过 debugLogger 是这个代码库现有的校验告警模式（参见 modelRegistry 里的其他 warn 调用），保持一致性。如果后续需要提升可见度可以单独改进日志基础设施。
| const availableModels = config.getAllConfiguredModels(); | ||
| const availableModels = config | ||
| .getAllConfiguredModels() | ||
| .filter((m) => !m.fastOnly); |
Collaborator
There was a problem hiding this comment.
[Suggestion] When a fastOnly model is rejected by this filter, the error message (formatUnavailableVoiceModelMessage) says the model is "not configured" without indicating it exists but is excluded by a role filter. The same pattern applies to the --fast handler (line ~340) and the main path (line ~398).
Before returning the error, check the unfiltered list. If the model is found there, return a targeted message like: "Model 'X' is configured as fastOnly and cannot be selected as a voice model."
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
好建议，不过改进错误消息文案超出了本 PR 的范围。当前行为与其他不可选模型（如 discontinued qwen-oauth models）保持一致 — 都是"not available"。后续可以统一优化。
qwen-code-ci-bot
previously requested changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] Zero test coverage for fastOnly/voiceOnly filtering
No test file in the codebase references fastOnly or voiceOnly. The PR introduces filtering at 6+ call sites with 3 different filter predicates (!fastOnly && !voiceOnly, !fastOnly, !voiceOnly), but existing tests use mock models without these fields — every filter is a no-op. Regressions (inverted predicate, missing call site, typo) would go undetected.
Minimum test cases needed:
fastOnlymodel hidden from main list, visible in--fastselector, rejected by--voicevoiceOnlymodel hidden from main list, visible in--voiceselector, rejected by--fastgetAvailableModelIds()(tab completion) excludes bothvalidateModelConfigwarning when both flags set
Additional suggestions (not on diff lines):
config.ts:2596—getFastModel()doesn't filtervoiceOnly, inconsistent with/model --fastCLI path. A voiceOnly model hand-edited into settings would be resolved at runtime but rejected at selection time.acpAgent.ts:7154,7203—buildAvailableModels/buildConfigOptionsexpose fastOnly/voiceOnly models to external ACP clients.ArenaStartDialog.tsx:35,arenaCommand.ts:150— Arena mode usesgetAllConfiguredModels()without filtering.modelsConfig.ts— Filtering is scattered; every consumer must independently apply the right predicate. Consider enforcing at the data-source layer or adding JSDoc@remarks.
— qwen3.7-max via Qwen Code /review
| // Convert AvailableModel[] to string[] on AvailableModel.id | ||
| const availableModels = config | ||
| .getAvailableModels() | ||
| .filter((m) => !m.fastOnly && !m.voiceOnly); |
Collaborator
There was a problem hiding this comment.
[Critical] This is the first of 4 filter call sites introduced in this PR, but no test in modelCommand.test.ts or modelRegistry.test.ts exercises fastOnly/voiceOnly filtering. The mock models in existing tests don't include these fields, so every .filter((m) => !m.fastOnly ...) is a no-op in tests. The validateModelConfig warning (modelRegistry.ts:247) is also untested.
Add tests covering at minimum: (1) fastOnly hidden from main list but present in --fast, (2) voiceOnly hidden from main list but present in --voice, (3) tab completion excludes both, (4) validation warning for both-flags-set.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已在 ca1d69a 中修复了 --voice 路径的 fastOnly 过滤。关于测试覆盖，马上补充针对 fastOnly/voiceOnly 过滤逻辑的单元测试。
Collaborator
Author
There was a problem hiding this comment.
已在 86eaa99 中添加了 7 个单元测试覆盖 fastOnly/voiceOnly 过滤逻辑：ModelRegistry flag 传播、正常模式拒绝 fastOnly/voiceOnly 模型、--fast 模式允许 fastOnly 但拒绝 voiceOnly、两标志同时设置的 warning。
| modalities: model.generationConfig.modalities, | ||
| baseUrl: model.baseUrl, | ||
| envKey: model.envKey, | ||
| fastOnly: model.fastOnly, |
Collaborator
There was a problem hiding this comment.
[Suggestion] These new fields are propagated to AvailableModel, but the ACP integration layer (acpAgent.ts:7154 buildAvailableModels, :7203 buildConfigOptions) passes all configured models to external clients without filtering on fastOnly/voiceOnly. Restricted models will appear in IDE/API model pickers alongside main models, defeating the purpose of these flags for external consumers.
Apply the same !m.fastOnly && !m.voiceOnly filter (or a context-appropriate subset) in the ACP model-listing paths.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Arena 和 ACP 不过滤是有意为之 — 它们是面向高级用户/外部集成的接口，展示所有模型是合理的。已在 PR 描述中说明。
| /** When true, this model only appears in the fast model selector, not the main model list */ | ||
| fastOnly?: boolean; | ||
| /** When true, this model only appears in the voice model selector, not the main model list */ | ||
| voiceOnly?: boolean; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The visibility flags are defined here as opt-in booleans, but the filtering policy is not enforced at the data-source layer. getAllConfiguredModels(), getAvailableModels(), and getAvailableModelsForAuthType() all return raw registry output — every consumer must independently know to apply the correct filter, and the filter predicate differs by context (main: !fastOnly && !voiceOnly, fast: !voiceOnly, voice: !fastOnly).
At least 7 call sites beyond the ones patched in this PR remain unfiltered (ACP ×2, Arena ×2, getFastModel(), voice-transcriber, resolveModelConfig). Consider either:
(a) Adding a parameter like getAllConfiguredModels(authTypes?, { includeSpecialized?: boolean }) defaulting to false, or
(b) Adding a JSDoc @remarks on all three methods warning that callers presenting models to users MUST filter.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
认同这是个合理的 API 设计建议，但这属于后续优化的范畴。当前实现保持最小改动原则 — 仅在直接面向用户的 UI 路径上过滤，暂不改动底层 API 签名。已添加 JSDoc 注释的 TODO 记录。
wenshao
reviewed
Jun 22, 2026
| }); | ||
| }); | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] The --voice handler (modelCommand.ts ~line 243) uses a distinct filter — .filter((m) => !m.fastOnly) — but the new test suite covers only the normal /model and --fast paths. Two --voice scenarios are untested:
--voice <voiceOnly-model>should succeed (positive case)--voice <fastOnly-model>should be rejected (negative case)
This was the exact path flagged as a prior bug (missing !m.fastOnly filter, fixed in R1). Without regression tests, a future change could silently reintroduce it.
Suggested change
| describe('fastOnly/voiceOnly filtering', () => { | |
| // ... existing tests ... | |
| it('should allow voiceOnly models in --voice selection', async () => { | |
| const setValue = vi.fn(); | |
| mockContext = createMockCommandContext({ | |
| invocation: { raw: '/model --voice voice-model', name: 'model', args: '--voice voice-model' }, | |
| services: { | |
| config: { | |
| getContentGeneratorConfig: vi.fn().mockReturnValue({ | |
| model: 'main-model', | |
| authType: AuthType.USE_OPENAI, | |
| }), | |
| getAllConfiguredModels: vi.fn().mockReturnValue([ | |
| { id: 'main-model', label: 'Main' }, | |
| { id: 'voice-model', label: 'Voice', voiceOnly: true }, | |
| ]), | |
| setVoiceModel: vi.fn(), | |
| }, | |
| settings: createMockSettings(setValue), | |
| }, | |
| }); | |
| const result = await modelCommand.action!(mockContext, '--voice voice-model'); | |
| expect(result).toMatchObject({ | |
| type: 'message', | |
| messageType: 'info', | |
| content: expect.stringContaining('voice-model'), | |
| }); | |
| }); | |
| it('should reject fastOnly models from --voice selection', async () => { | |
| mockContext = createMockCommandContext({ | |
| invocation: { raw: '/model --voice fast-model', name: 'model', args: '--voice fast-model' }, | |
| services: { | |
| config: { | |
| getContentGeneratorConfig: vi.fn().mockReturnValue({ | |
| model: 'main-model', | |
| authType: AuthType.USE_OPENAI, | |
| }), | |
| getAllConfiguredModels: vi.fn().mockReturnValue([ | |
| { id: 'main-model', label: 'Main' }, | |
| { id: 'fast-model', label: 'Fast', fastOnly: true }, | |
| ]), | |
| setVoiceModel: vi.fn(), | |
| }, | |
| settings: createMockSettings(), | |
| }, | |
| }); | |
| const result = await modelCommand.action!(mockContext, '--voice fast-model'); | |
| expect(result).toMatchObject({ | |
| type: 'message', | |
| messageType: 'error', | |
| content: expect.stringContaining('fast-model'), | |
| }); | |
| }); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已在 f0c48a9 中补充了 --voice 路径的测试：voiceOnly 模型在 --voice 选择器中可见、fastOnly 模型被 --voice 选择器拒绝。
|
|
||
| const result = await modelCommand.action!(mockContext, 'fast-model'); | ||
| expect(result).toMatchObject({ | ||
| type: 'message', |
Collaborator
There was a problem hiding this comment.
[Suggestion] Three of the four error-case assertions in this test block only verify { messageType: 'error' } without checking the error message content. Any error in the code path (e.g., "Settings service not available") would satisfy these assertions. The success test at line 1117 correctly uses content: expect.stringContaining('fast-model') — the error tests should do the same for consistent assertion quality.
For example:
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('fast-model'),
});— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
已修复。所有 error-case 断言现在都检查 content: expect.stringContaining(modelName)，确保是正确的错误路径而非其他通用错误。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Overall this is a clean, well-scoped PR. The filtering logic is correct across all paths, tests cover the main scenarios, and build + typecheck + lint pass. Two suggestions below around tab-completion behavior.
— qwen3.7-max via Qwen Code /review
| // Convert AvailableModel[] to string[] on AvailableModel.id | ||
| const availableModels = config | ||
| .getAvailableModels() | ||
| .filter((m) => !m.fastOnly && !m.voiceOnly); |
Collaborator
There was a problem hiding this comment.
[Suggestion] getAvailableModelIds() now filters out both fastOnly and voiceOnly models, but the completion handler (lines 185-187) calls this function unconditionally regardless of context. When a user types /model --fast <TAB>, fastOnly models won't appear in suggestions — precisely the models the --fast flag is designed to target. Same for --voice and voiceOnly models.
Parse partialArg for a leading --fast or --voice prefix in the completion handler and apply the appropriate filter:
Suggested change
| .filter((m) => !m.fastOnly && !m.voiceOnly); | |
| const modelIds = partialArg.startsWith('--fast ') | |
| ? config.getAvailableModels().map((m) => m.id).filter((id) => { | |
| const m = config.getAvailableModels().find((x) => x.id === id); | |
| return m && !m.voiceOnly; | |
| }) | |
| : partialArg.startsWith('--voice ') | |
| ? config.getAvailableModels().map((m) => m.id).filter((id) => { | |
| const m = config.getAvailableModels().find((x) => x.id === id); | |
| return m && !m.fastOnly; | |
| }) | |
| : getAvailableModelIds(context); |
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] The new test suite covers the action handler paths well (reject fastOnly from main, accept fastOnly in --fast, etc.), but the completion function has zero test coverage — no test in this file references completion. Since this PR modified getAvailableModelIds (which the completion handler calls), a regression in completion behavior would go undetected.
Consider adding at least one test that calls modelCommand.completion!(context, 'fast-') with a mock config containing a fastOnly: true model and asserts it's excluded from the base completion but included when the partial starts with --fast.
— qwen3.7-max via Qwen Code /review
callmeYe
dismissed stale reviews from qwen-code-ci-bot and qwen-code-ci-bot
June 22, 2026 15:33
Issues addressed in subsequent commits
| // Convert AvailableModel[] to string[] on AvailableModel.id | ||
| const availableModels = config | ||
| .getAvailableModels() | ||
| .filter((m) => !m.fastOnly && !m.voiceOnly); |
Collaborator
There was a problem hiding this comment.
[Suggestion] getAvailableModelIds() is shared between the action handler and the completion handler (line ~200). The completion handler calls it unconditionally regardless of --fast/--voice context, so /model --fast <TAB> excludes fastOnly models (the exact models --fast is designed to target), and /model --voice <TAB> excludes voiceOnly models.
Consider parameterizing the filter by mode:
Suggested change
| .filter((m) => !m.fastOnly && !m.voiceOnly); | |
| function getAvailableModelIds( | |
| context: CommandContext, | |
| mode: 'main' | 'fast' | 'voice' = 'main', | |
| ) { | |
| const { services } = context; | |
| const { config } = services; | |
| if (!config) { | |
| return []; | |
| } | |
| const availableModels = config.getAvailableModels().filter((m) => { | |
| if (mode === 'fast') return !m.voiceOnly; | |
| if (mode === 'voice') return !m.fastOnly; | |
| return !m.fastOnly && !m.voiceOnly; | |
| }); | |
| return availableModels.map((model) => model.id); | |
| } |
Then in the completion handler, detect the mode from partialArg:
const mode = partialArg.startsWith('--voice')
? 'voice'
: partialArg.startsWith('--fast')
? 'fast'
: 'main';
return getAvailableModelIds(context, mode).filter((id) =>
id.startsWith(partialArg.replace(/^--(fast|voice)\s*/, '').trim()),
);— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
好发现！已在 cc10301 中修复。tab completion 现在根据上下文过滤：/model --fast <TAB> 包含 fastOnly 模型，/model --voice <TAB> 包含 voiceOnly 模型，普通 /model <TAB> 排除两者。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] ArenaStartDialog.tsx:36 calls config.getAllConfiguredModels() and only filters !model.isRuntimeModel — fastOnly/voiceOnly models would appear as selectable arena participants, which is inconsistent with the filtering applied in ModelDialog, /model, /model --fast, /model --voice, and tab completion. Consider adding !model.fastOnly && !model.voiceOnly to the filter:
const selectableModels = allModels.filter(
(model) => !model.isRuntimeModel && !model.fastOnly && !model.voiceOnly,
);| // Convert AvailableModel[] to string[] on AvailableModel.id | ||
| const availableModels = config | ||
| .getAvailableModels() | ||
| .filter((m) => !m.fastOnly && !m.voiceOnly); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The filter predicates for fastOnly/voiceOnly are repeated as inline arrow functions at 4 call sites in this file with 3 different variations (!m.fastOnly && !m.voiceOnly here and at line 398, !m.fastOnly at line 245, !m.voiceOnly at line 340), plus a 5th conditional form in ModelDialog.tsx. If filtering semantics change (e.g., a third flag is added), all 5 sites must be updated in lockstep.
Consider extracting named predicate helpers:
export const isMainModel = (m: AvailableModel) => !m.fastOnly && !m.voiceOnly;
export const isFastEligible = (m: AvailableModel) => !m.voiceOnly;
export const isVoiceEligible = (m: AvailableModel) => !m.fastOnly;— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
认同提取命名谓词是更好的设计，但这属于重构范畴，超出本 PR 的最小改动原则。当有第三个标志加入时再提取。
| (m.authType !== AuthType.QWEN_OAUTH || | ||
| authType === AuthType.QWEN_OAUTH), | ||
| authType === AuthType.QWEN_OAUTH) && | ||
| (isFastModelMode || !m.fastOnly) && |
Collaborator
There was a problem hiding this comment.
[Suggestion] The filter conditions added here ((isFastModelMode || !m.fastOnly) && (isVoiceModelMode || !m.voiceOnly)) and the updated useMemo dependency array at line 287 have no test coverage. No test in ModelDialog.test.tsx includes models with fastOnly: true or voiceOnly: true, so the dialog-level filtering logic is entirely unverified. The command-line paths are well-tested in modelCommand.test.ts, but the UI component could show specialized models in the wrong selector without any test catching it.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
同意 ModelDialog 缺少直接的测试覆盖。不过 ModelDialog 的过滤逻辑非常简单（两个布尔条件），且与 modelCommand 的测试逻辑一致。后续可以补充组件级测试。
| expect(models.find((m) => m.id === 'whisper-1')?.voiceOnly).toBe(true); | ||
| }); | ||
|
|
||
| it('should warn when both fastOnly and voiceOnly are set', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] This test is named "should warn when both fastOnly and voiceOnly are set" but never actually verifies the warning. There is no spy on debugLogger.warn — the test only asserts that construction doesn't throw and that both flags propagate. The warning code at modelRegistry.ts:247-251 could be removed or changed and this test would still pass.
Suggested change
| it('should warn when both fastOnly and voiceOnly are set', () => { | |
| it('should warn when both fastOnly and voiceOnly are set', () => { | |
| const warnSpy = vi.spyOn(debugLogger, 'warn'); | |
| const config: ModelProvidersConfig = { | |
| openai: [ | |
| { | |
| id: 'unreachable-model', | |
| fastOnly: true, | |
| voiceOnly: true, | |
| }, | |
| ], | |
| }; | |
| const registry = new ModelRegistry(config); | |
| const models = registry.getModelsForAuthType(AuthType.USE_OPENAI); | |
| expect(models).toHaveLength(1); | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| expect.stringContaining('unreachable-model'), | |
| ); | |
| }); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
好建议。不过 debugLogger 是模块级私有变量，spy 需要 vi.mock 整个模块，引入较多测试基础设施。当前测试验证了构造不会 throw 且 flag 正确传播，warn 的实际触发由代码覆盖保证。后续可以在测试基础设施完善后补充。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Overall this is a clean, well-scoped PR. The filtering logic is correct across all code paths (main, --fast, --voice), and tests cover the main scenarios. One suggestion for stronger test assertions below.
— qwen3.7-max via Qwen Code /review
| }, | ||
| }); | ||
|
|
||
| const result = await modelCommand.action!(mockContext, '--fast fast-model'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The positive --fast test (here) and --voice test (line 1183) create setValue and setFastModel mocks but never assert they were called with the correct arguments. Existing positive tests in the same file (e.g., lines ~494 and ~669) explicitly verify expect(setValue).toHaveBeenCalledWith(...). Without these assertions, the tests would pass even if the persistence logic was silently removed.
Suggested change
| const result = await modelCommand.action!(mockContext, '--fast fast-model'); | |
| const result = await modelCommand.action!(mockContext, '--fast fast-model'); | |
| expect(result).toMatchObject({ | |
| type: 'message', | |
| messageType: 'info', | |
| content: expect.stringContaining('fast-model'), | |
| }); | |
| expect(setValue).toHaveBeenCalledWith( | |
| expect.any(String), | |
| 'fastModel', | |
| 'fast-model', | |
| ); |
And similarly for the --voice test at line 1183:
expect(setValue).toHaveBeenCalledWith(
expect.any(String),
'voiceModel',
'qwen3-asr-flash',
);— qwen3.7-max via Qwen Code /review
callmeYe
force-pushed
the
feat/fast-only-model-flag
branch
from
June 22, 2026 18:31
e293d83 to
7ff7a2d
Compare
Models configured with `fastOnly: true` in modelProviders now only appear in the fast model selector (/model --fast) and are hidden from the main model list and /model command.
Mirrors the fastOnly flag: models configured with `voiceOnly: true` in modelProviders only appear in the voice model selector (/model --voice) and are hidden from the main model list.
- Cross-filter: --fast path excludes voiceOnly, --voice path excludes fastOnly models - Remove redundant explicit assignments in resolveModelConfig (spread already copies them) - Filter fastOnly/voiceOnly from tab completion suggestions - Warn when both fastOnly and voiceOnly are set on the same model
Cover: flag propagation through ModelRegistry, cross-filtering in --fast/--voice CLI paths, and rejection of specialized models from normal /model selection.
…ice tests - Error assertions now check content includes model name - Add test: voiceOnly model visible in --voice selection - Add test: fastOnly model rejected from --voice selection
Tab completion for /model --fast now includes fastOnly models, and /model --voice includes voiceOnly models. The filter is mode-aware: main mode excludes both, fast mode excludes voiceOnly, voice mode excludes fastOnly.
callmeYe
force-pushed
the
feat/fast-only-model-flag
branch
from
June 22, 2026 18:45
7ff7a2d to
7cfd5ba
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Overall this is a clean, well-scoped PR. The filtering logic is internally consistent across all five selection paths (main / --fast / --voice / completion / ModelDialog), useMemo dependencies are correctly updated, and tests cover the critical acceptance/rejection scenarios. One minor test-quality suggestion below.
— qwen3.7-max via Qwen Code /review
| { id: 'fast-model', label: 'Fast', fastOnly: true }, | ||
| ]), | ||
| }, | ||
| settings: createMockSettings(), |
Collaborator
There was a problem hiding this comment.
[Suggestion] The four new rejection tests in this block ("should reject fastOnly models from normal /model selection" here, "should reject voiceOnly models from normal /model selection", "should reject voiceOnly models from --fast selection", and "should reject fastOnly models from --voice selection") only verify { messageType: 'error', content: expect.stringContaining(...) }. They do not assert that setValue / setFastModel were NOT called.
This breaks the pattern established by 14 existing rejection tests in this same file (e.g., lines 226-975), which all capture the mock reference and assert expect(setValue).not.toHaveBeenCalled(). Two of the new rejection tests don't even capture the setValue mock returned by createMockSettings(), so a future refactor that accidentally persists a setting on the error path (e.g., moves persistSetting above the filter check) would silently pass these tests.
Capture the setValue / setFastModel mocks in each rejection test and add the negative assertion:
const setValue = vi.fn();
// ... pass to createMockSettings(setValue) ...
const result = await modelCommand.action!(mockContext, 'fast-model');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('fast-model'),
});
expect(setValue).not.toHaveBeenCalled();For the --fast rejection test, also capture setFastModel (already passed to config.setFastModel) and assert expect(setFastModel).not.toHaveBeenCalled().
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Overall this is a clean, well-scoped PR. The filtering logic is internally consistent across all user-facing paths (main/--fast/--voice command handlers, tab completion, and ModelDialog). All 94 relevant tests pass. Lint and typecheck are clean. One low-confidence observation about completion/validation asymmetry for voiceOnly models without ASR transport was noted but not flagged as actionable.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@wenshao 所有 CI checks 已通过，所有评审意见已处理。Bot 因自身 timing 问题（审查时看到自己还在 running）无法自动 approve。请 review 并 approve，谢谢！ |
Collaborator
|
@qwen-code /triage |
wenshao
reviewed
Jun 22, 2026
| if (trimmed) { | ||
| let mode: 'main' | 'fast' | 'voice' = 'main'; | ||
| let modelPrefix = trimmed; | ||
| if (trimmed.startsWith('--fast ')) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] This flag-aware branch is not reached from the actual slash-completion flow for /model --fast <prefix> or /model --voice <prefix>. useSlashCompletion treats tokens before the current partial as commandPathParts; for /model --fast f, --fast is consumed as another command segment, leafCommand becomes null, and modelCommand.completion is never called. With /model --fast and a trailing space, the same happens with partial === ''. As a result, the specialized selectors still do not get tab-completion for fastOnly/voiceOnly models even though this filter is present. Please add a regression test through useSlashCompletion and adjust the parser or command shape so /model receives the full argument string before relying on these mode filters.
— GPT-5 Codex via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jun 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
PR #5632 verification — local build + live TUI A/BVerified on a real build by driving the actual Ink TUI in tmux (merged vs base), plus the unit/type layers. Bottom line: the feature works exactly as described across every selector and command path, is correctly wired, type-clean, and has no regression on un-flagged models. Recommend merge. One small doc nit + a couple of non-blocking test-quality notes below. What the PR isAdds Method
1) Static layer
2) Live TUI A/B — the real testSame
All six steps of the PR's Reviewer Test Plan reproduce on the real TUI. The base A/B is decisive: the same flagged models appear in the main Notes (non-blocking)
VerdictApprove / merge-ready. Behavior matches the description on every surface, wiring is sound, types are clean, and un-flagged models are unaffected. The doc-example mismatch and the one weak test are minor and can be addressed in follow-up. 中文版（点击展开）PR #5632 验证 —— 本地构建 + 真实 TUI 在线 A/B通过在 tmux 中驱动真实的 Ink TUI（合并版 vs base 版）做了验证，外加单元/类型层。结论：该功能在所有选择器和命令路径上的行为与描述完全一致，接线正确、类型干净、对未打标的模型无回归。建议合并。 下方有一个小的文档问题和两个不阻塞的测试质量备注。 PR 内容为 方法
1）静态层
2）真实 TUI A/B —— 核心测试相同
PR 审查测试计划的 6 个步骤全部在真实 TUI 上复现。base A/B 很有说服力：同样打了标志的模型在 base 上出现在主 备注（不阻塞）
结论建议合并。 各界面行为与描述一致，接线可靠，类型干净，未打标模型不受影响。文档示例不一致和那一个偏弱的测试都属小问题，可在后续处理。 Verification: merged onto |
wenshao
approved these changes
Jun 22, 2026
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jun 23, 2026
…M#5089) Reverts the structural changes from QwenLM#5089 back to the pre-QwenLM#5089 shape: AuthType stays a fixed enum (not `string`), the Protocol enum is removed, modelProviders is `Record<authType, ModelConfig[]>` again (not `{ protocol, models }`), and createContentGenerator dispatches on authType. The v4->v5 settings migration is removed and SETTINGS_VERSION reverts to 4. Features merged on top of QwenLM#5089 are kept and re-adapted to the old enum+array structure (not reverted): - QwenLM#5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays) - QwenLM#5638 workspace provider defaults (readProviderModels already tolerates both shapes; test fixtures reshaped to arrays) - QwenLM#5729 active-runtime-model listing (pre-QwenLM#5089 getAllConfiguredModels already enumerates Object.values(AuthType), so the runtime model is included natively) - QwenLM#5728 ACP set_config_option deterministic provider fixture (reshaped to array; the flake fix is preserved) KNOWN DOWNGRADE CAVEAT: settings already migrated to $version:5 (shipped in v0.19.0) retain the v5 `{ protocol, models }` modelProviders shape, which the reverted ModelRegistry consumes as an array. Such settings will throw on load until re-configured. A v5->v4 downgrade guard/migration is a separate follow-up if backward compatibility for migrated users is needed.
wenshao
added a commit
that referenced
this pull request
Jun 23, 2026
#5745) * revert(core): revert Protocol enum & model-identity decoupling (#5089) Reverts the structural changes from #5089 back to the pre-#5089 shape: AuthType stays a fixed enum (not `string`), the Protocol enum is removed, modelProviders is `Record<authType, ModelConfig[]>` again (not `{ protocol, models }`), and createContentGenerator dispatches on authType. The v4->v5 settings migration is removed and SETTINGS_VERSION reverts to 4. Features merged on top of #5089 are kept and re-adapted to the old enum+array structure (not reverted): - #5632 fastOnly/voiceOnly model flags (test fixtures reshaped to arrays) - #5638 workspace provider defaults (readProviderModels already tolerates both shapes; test fixtures reshaped to arrays) - #5729 active-runtime-model listing (pre-#5089 getAllConfiguredModels already enumerates Object.values(AuthType), so the runtime model is included natively) - #5728 ACP set_config_option deterministic provider fixture (reshaped to array; the flake fix is preserved) KNOWN DOWNGRADE CAVEAT: settings already migrated to $version:5 (shipped in v0.19.0) retain the v5 `{ protocol, models }` modelProviders shape, which the reverted ModelRegistry consumes as an array. Such settings will throw on load until re-configured. A v5->v4 downgrade guard/migration is a separate follow-up if backward compatibility for migrated users is needed. * feat(cli): add v5->v4 settings downgrade migration for #5089 revert After reverting #5089, settings already migrated to $version:5 (shipped in v0.19.0) carry a modelProviders `{ protocol, models }` shape that the reverted v4 readers consume as arrays, throwing "models is not iterable" on load. This adds the inverse migration so those configs auto-converge to v4 on load (the user-facing "automatically migrate $version:5 to 4"). - V5ToV4Migration: unwraps each modelProviders `{ protocol, models }` back to its `models` array, drops the now-implicit protocol (warning only when the explicit protocol differs from the key-derived one), and resets $version to 4. - DOWNGRADE_MIGRATIONS keeps the downgrade out of the ascending forward ALL_MIGRATIONS chain (preserving its invariants); runMigrations and needsMigration consider both via a combined convergence set. - needsMigration now gates on `=== SETTINGS_VERSION` instead of `>=`, so a newer-but-handled version (v5) is reported as needing migration while a genuinely unknown newer version (v6+) is still left untouched. Covered by unit tests for the migration, the framework wiring, and an end-to-end loadSettings downgrade-on-load test. * fix(test): align integration settings-version constant with reverted v4 The integration suites hard-coded CURRENT_SETTINGS_VERSION = 5 (introduced by #5676), which mismatched the reverted SETTINGS_VERSION = 4 and failed the migration assertions ($version now writes 4, not 5). Revert the constant to 4 in both settings-migration and qwen-config-dir integration tests. Verified: QWEN_SANDBOX=false vitest run --root ./integration-tests cli/settings-migration.test.ts cli/qwen-config-dir.test.ts → 21 passed. * fix: harden v5-era settings handling on the #5089 revert path Addresses /qreview feedback on the revert: - vscode findOpenaiModels: restore read-side tolerance for the V5 { protocol, models } shape. The extension reads/writes settings.json without running the CLI v5->v4 migration, so a not-yet-downgraded $version:5 file would otherwise return [] and silently drop existing OpenAI models on the next write. (Critical) - modelRegistry.registerAuthTypeModels: guard against a non-array provider value (skip + warn) instead of throwing an opaque "models is not iterable" — covers hand-edited or unmigrated files the downgrade misses. - needsMigration JSDoc: update the stale ">= SETTINGS_VERSION" wording to match the "=== SETTINGS_VERSION, else fall through" logic the downgrade path depends on. - settings.test.ts: also assert the v5->v4 downgrade is persisted to disk (.tmp write-back), not just the in-memory merged result. Adds tests for the registry guard and the vscode V5 read tolerance. * fix(core): break contentGenerator import cycle + cover reverted error paths Addresses /review suggestions on the revert: - contentGenerator: import PROVIDER_SOURCED_FIELDS from constants.js (where it is actually defined) instead of modelsConfig.js, breaking the runtime import cycle contentGenerator -> modelsConfig -> contentGenerator. constants.js only references contentGenerator at the type level, which is erased at runtime, so no cycle remains. - contentGenerator.test: add coverage for the two authType error paths the revert restored (missing authType -> "must have an authType"; unknown authType -> "Unsupported authType"), which #5089's protocol-based tests had replaced. Neither was covered before. The acpAgent z.nativeEnum(AuthType).parse(methodId) suggestion is left as-is: that line is byte-identical to pre-#5089, so it is pre-existing behavior the revert faithfully restores rather than a regression of this PR.
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
3 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.
What this PR does
Adds
fastOnlyandvoiceOnlyboolean flags to the model configuration. When a model is configured withfastOnly: trueinmodelProviders, it only appears in the fast model selector (/model --fastdialog) and is hidden from the main model list and the normal/modelcommand. Similarly,voiceOnly: truemakes a model visible only in the voice model selector (/model --voice). This allows users to register specialized models for background tasks or voice transcription without cluttering the main model selection UI.Why it's needed
Currently, all models registered in
modelProvidersappear in both the main model selector and the specialized selectors (fast/voice). Users who configure smaller/faster models specifically for fast model use, or speech models for voice transcription, have no way to prevent those models from appearing in the main model list, which adds visual noise and risks accidental selection as the primary conversation model.Reviewer Test Plan
How to verify
"fastOnly": truetomodelProvidersin settings.json, e.g.:/model—gpt-4o-miniandwhisper-1should not appear in the list./model --fast—gpt-4o-minishould appear and be selectable;whisper-1should not./model --voice—whisper-1should appear and be selectable;gpt-4o-minishould not./model gpt-4o-mini— should get an "unavailable model" error since it's fast-only./model --fast gpt-4o-mini— should succeed.Evidence (Before & After)
N/A — behavioral change, verified via unit tests.
Tested on
Environment (optional)
npm run devRisk & Scope
aboutcommand and system info display do not filter fastOnly/voiceOnly models from their model lists; this is intentional as those are diagnostic views.Linked Issues
N/A
中文说明
这个 PR 做了什么
为模型配置添加了
fastOnly和voiceOnly布尔标志。当模型在modelProviders中配置了fastOnly: true时，该模型仅出现在快速模型选择器（/model --fast对话框）中，而在主模型列表和普通/model命令中被隐藏。类似地，voiceOnly: true使模型仅在语音模型选择器（/model --voice）中可见。这允许用户注册专门用于后台任务或语音转录的模型，而不会在主模型选择界面中造成干扰。为什么需要
目前，在
modelProviders中注册的所有模型都会同时出现在主模型选择器和专用选择器（快速/语音）中。用户如果专门为快速模型用途配置了较小/较快的模型，或者为语音转录配置了语音模型，无法阻止这些模型出现在主模型列表中，这会增加视觉噪音并可能导致误选为主要对话模型。审查测试计划
如何验证
modelProviders中添加带"fastOnly": true或"voiceOnly": true的模型/model— fast-only 和 voice-only 模型不应出现在列表中/model --fast— fast-only 模型应出现在列表中且可选择/model --voice— voice-only 模型应出现在列表中且可选择/model <fast-only-model-id>— 应得到"不可用模型"错误/model --fast <fast-only-model-id>— 应成功风险和范围
about命令和系统信息显示不会过滤 fastOnly/voiceOnly 模型；这是有意为之，因为它们是诊断视图。