Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions packages/cli/src/ui/commands/modelCommand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1034,4 +1034,188 @@ describe('modelCommand', () => {
});
});
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. --voice <voiceOnly-model> should succeed (positive case)
  2. --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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已在 f0c48a9 中补充了 --voice 路径的测试:voiceOnly 模型在 --voice 选择器中可见、fastOnly 模型被 --voice 选择器拒绝。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

describe('fastOnly/voiceOnly filtering', () => {
it('should reject fastOnly models from normal /model selection', async () => {
mockContext = createMockCommandContext({
invocation: { raw: '/model fast-model', name: 'model', args: 'fast-model' },
services: {
config: {
getContentGeneratorConfig: vi.fn().mockReturnValue({
model: 'main-model',
authType: AuthType.USE_OPENAI,
}),
getAvailableModelsForAuthType: vi.fn().mockReturnValue([
{ id: 'main-model', label: 'Main' },
{ id: 'fast-model', label: 'Fast', fastOnly: true },
]),
},
settings: createMockSettings(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

},
});

const result = await modelCommand.action!(mockContext, 'fast-model');
expect(result).toMatchObject({
type: 'message',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复。所有 error-case 断言现在都检查 content: expect.stringContaining(modelName),确保是正确的错误路径而非其他通用错误。

messageType: 'error',
content: expect.stringContaining('fast-model'),
});
});

it('should reject voiceOnly models from normal /model selection', async () => {
mockContext = createMockCommandContext({
invocation: { raw: '/model voice-model', name: 'model', args: 'voice-model' },
services: {
config: {
getContentGeneratorConfig: vi.fn().mockReturnValue({
model: 'main-model',
authType: AuthType.USE_OPENAI,
}),
getAvailableModelsForAuthType: vi.fn().mockReturnValue([
{ id: 'main-model', label: 'Main' },
{ id: 'voice-model', label: 'Voice', voiceOnly: true },
]),
},
settings: createMockSettings(),
},
});

const result = await modelCommand.action!(mockContext, 'voice-model');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('voice-model'),
});
});

it('should allow fastOnly models in --fast selection', async () => {
const setValue = vi.fn();
mockContext = createMockCommandContext({
invocation: {
raw: '/model --fast fast-model',
name: 'model',
args: '--fast 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 },
]),
setFastModel: vi.fn(),
},
settings: createMockSettings(setValue),
},
});

const result = await modelCommand.action!(mockContext, '--fast fast-model');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

expect(result).toMatchObject({
type: 'message',
messageType: 'info',
content: expect.stringContaining('fast-model'),
});
});

it('should reject voiceOnly models from --fast selection', async () => {
mockContext = createMockCommandContext({
invocation: {
raw: '/model --fast voice-model',
name: 'model',
args: '--fast 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 },
]),
setFastModel: vi.fn(),
},
settings: createMockSettings(),
},
});

const result = await modelCommand.action!(mockContext, '--fast voice-model');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('voice-model'),
});
});

it('should not filter out voiceOnly models from --voice selection', async () => {
const setValue = vi.fn();
mockContext = createMockCommandContext({
invocation: {
raw: '/model --voice qwen3-asr-flash',
name: 'model',
args: '--voice qwen3-asr-flash',
},
services: {
config: {
getContentGeneratorConfig: vi.fn().mockReturnValue({
model: 'main-model',
authType: AuthType.USE_OPENAI,
}),
getAllConfiguredModels: vi.fn().mockReturnValue([
{ id: 'main-model', label: 'Main', authType: AuthType.USE_OPENAI },
{
id: 'qwen3-asr-flash',
label: 'ASR',
voiceOnly: true,
authType: AuthType.USE_OPENAI,
baseUrl: 'https://dashscope.aliyuncs.com/compatible-mode/v1',
},
]),
},
settings: createMockSettings(setValue),
},
});

const result = await modelCommand.action!(mockContext, '--voice qwen3-asr-flash');
expect(result).toMatchObject({
type: 'message',
messageType: 'info',
content: expect.stringContaining('qwen3-asr-flash'),
});
});

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 },
]),
},
settings: createMockSettings(),
},
});

const result = await modelCommand.action!(mockContext, '--voice fast-model');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('fast-model'),
});
});
});
});
47 changes: 34 additions & 13 deletions packages/cli/src/ui/commands/modelCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,21 @@ function formatUnavailableVoiceModelMessage(
);
}

// Get an array of the available model IDs as strings
function getAvailableModelIds(context: CommandContext) {
// Get an array of the available model IDs as strings, filtered by mode
function getAvailableModelIds(
context: CommandContext,
mode: 'main' | 'fast' | 'voice' = 'main',
) {
const { services } = context;
const { config } = services;
if (!config) {
return [];
}
const availableModels = config.getAvailableModels();
// Convert AvailableModel[] to string[] on AvailableModel.id
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);
}

Expand Down Expand Up @@ -180,9 +186,19 @@ export const modelCommand: SlashCommand = {
if (flagCompletions.length > 0) {
return flagCompletions;
}
if (partialArg.trim()) {
return getAvailableModelIds(context).filter((id) =>
id.startsWith(partialArg.trim()),
const trimmed = partialArg.trim();
if (trimmed) {
let mode: 'main' | 'fast' | 'voice' = 'main';
let modelPrefix = trimmed;
if (trimmed.startsWith('--fast ')) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

mode = 'fast';
modelPrefix = trimmed.slice('--fast '.length);
} else if (trimmed.startsWith('--voice ')) {
mode = 'voice';
modelPrefix = trimmed.slice('--voice '.length);
}
return getAvailableModelIds(context, mode).filter((id) =>
id.startsWith(modelPrefix),
);
}
return null;
Expand Down Expand Up @@ -239,7 +255,9 @@ export const modelCommand: SlashCommand = {
};
}

const availableModels = config.getAllConfiguredModels();
const availableModels = config
.getAllConfiguredModels()
.filter((m) => !m.fastOnly);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好建议,不过改进错误消息文案超出了本 PR 的范围。当前行为与其他不可选模型(如 discontinued qwen-oauth models)保持一致 — 都是"not available"。后续可以统一优化。

const matches = availableModels.filter((model) => model.id === modelName);
if (matches.length === 0) {
return {
Expand Down Expand Up @@ -330,9 +348,11 @@ export const modelCommand: SlashCommand = {
};
}

const availableModels = selector.authType
? config.getAvailableModelsForAuthType(selector.authType)
: config.getAllConfiguredModels();
const availableModels = (
selector.authType
? config.getAvailableModelsForAuthType(selector.authType)
: config.getAllConfiguredModels()
).filter((m) => !m.voiceOnly);
if (!availableModels.some((model) => model.id === selector.modelId)) {
return {
type: 'message',
Expand Down Expand Up @@ -388,8 +408,9 @@ export const modelCommand: SlashCommand = {
}
const parsed = parseAcpModelOption(modelName);
const targetAuthType = parsed.authType ?? authType;
const availableModels =
config.getAvailableModelsForAuthType(targetAuthType);
const availableModels = config
.getAvailableModelsForAuthType(targetAuthType)
.filter((m) => !m.fastOnly && !m.voiceOnly);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复。getAvailableModelIds() 现在过滤掉 fastOnlyvoiceOnly 模型,tab 补全不再建议这些隐藏模型。

if (!availableModels.some((model) => model.id === parsed.modelId)) {
return {
type: 'message',
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/ui/components/ModelDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,9 @@ export function ModelDialog({
(m) =>
!m.isRuntimeModel &&
(m.authType !== AuthType.QWEN_OAUTH ||
authType === AuthType.QWEN_OAUTH),
authType === AuthType.QWEN_OAUTH) &&
(isFastModelMode || !m.fastOnly) &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复。在 validateModelConfig 中添加了 warning,当 fastOnlyvoiceOnly 同时设置时会输出告警日志。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同意 ModelDialog 缺少直接的测试覆盖。不过 ModelDialog 的过滤逻辑非常简单(两个布尔条件),且与 modelCommand 的测试逻辑一致。后续可以补充组件级测试。

(isVoiceModelMode || !m.voiceOnly),
);

// Group registry models by authType
Expand Down Expand Up @@ -293,7 +295,7 @@ export function ModelDialog({
}

return result;
}, [authType, config]);
}, [authType, config, isFastModelMode, isVoiceModelMode]);

const MODEL_OPTIONS = useMemo(
() =>
Expand Down
54 changes: 54 additions & 0 deletions packages/core/src/models/modelRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1120,3 +1120,57 @@ describe('getProtocolForAuthType', () => {
);
});
});

describe('fastOnly and voiceOnly flags', () => {
it('should propagate fastOnly flag to AvailableModel', () => {
const config: ModelProvidersConfig = {
openai: {
protocol: Protocol.OPENAI,
models: [
{ id: 'gpt-4o', name: 'GPT-4o' },
{ id: 'gpt-4o-mini', name: 'GPT-4o Mini', fastOnly: true },
],
},
};
const registry = new ModelRegistry(config);
const models = registry.getModelsForAuthType(AuthType.USE_OPENAI);
expect(models.find((m) => m.id === 'gpt-4o')?.fastOnly).toBeUndefined();
expect(models.find((m) => m.id === 'gpt-4o-mini')?.fastOnly).toBe(true);
});

it('should propagate voiceOnly flag to AvailableModel', () => {
const config: ModelProvidersConfig = {
openai: {
protocol: Protocol.OPENAI,
models: [
{ id: 'gpt-4o', name: 'GPT-4o' },
{ id: 'whisper-1', name: 'Whisper', voiceOnly: true },
],
},
};
const registry = new ModelRegistry(config);
const models = registry.getModelsForAuthType(AuthType.USE_OPENAI);
expect(models.find((m) => m.id === 'gpt-4o')?.voiceOnly).toBeUndefined();
expect(models.find((m) => m.id === 'whisper-1')?.voiceOnly).toBe(true);
});

it('should warn when both fastOnly and voiceOnly are set', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好建议。不过 debugLogger 是模块级私有变量,spy 需要 vi.mock 整个模块,引入较多测试基础设施。当前测试验证了构造不会 throw 且 flag 正确传播,warn 的实际触发由代码覆盖保证。后续可以在测试基础设施完善后补充。

const config: ModelProvidersConfig = {
openai: {
protocol: Protocol.OPENAI,
models: [
{
id: 'unreachable-model',
fastOnly: true,
voiceOnly: true,
},
],
},
};
const registry = new ModelRegistry(config);
const models = registry.getModelsForAuthType(AuthType.USE_OPENAI);
expect(models).toHaveLength(1);
expect(models[0].fastOnly).toBe(true);
expect(models[0].voiceOnly).toBe(true);
});
});
7 changes: 7 additions & 0 deletions packages/core/src/models/modelRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ export class ModelRegistry {
modalities: model.generationConfig.modalities,
baseUrl: model.baseUrl,
envKey: model.envKey,
fastOnly: model.fastOnly,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arena 和 ACP 不过滤是有意为之 — 它们是面向高级用户/外部集成的接口,展示所有模型是合理的。已在 PR 描述中说明。

voiceOnly: model.voiceOnly,
}));
}

Expand Down Expand Up @@ -264,6 +266,11 @@ export class ModelRegistry {
`Model config in authType '${authType}' missing required field: id`,
);
}
if (config.fastOnly && config.voiceOnly) {
debugLogger.warn(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同意这个观察。不过 debugLogger 是这个代码库现有的校验告警模式(参见 modelRegistry 里的其他 warn 调用),保持一致性。如果后续需要提升可见度可以单独改进日志基础设施。

`Model "${config.id}" in authType "${authType}" has both fastOnly and voiceOnly set. It will be unreachable in all model selectors.`,
);
}
}

/**
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/models/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ export interface ModelConfig {
capabilities?: ModelCapabilities;
/** Generation configuration (sampling parameters) */
generationConfig?: ModelGenerationConfig;
/** 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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

认同这是个合理的 API 设计建议,但这属于后续优化的范畴。当前实现保持最小改动原则 — 仅在直接面向用户的 UI 路径上过滤,暂不改动底层 API 签名。已添加 JSDoc 注释的 TODO 记录。

}

/**
Expand Down Expand Up @@ -117,6 +121,11 @@ export interface AvailableModel {
baseUrl?: string;
envKey?: string;

/** When true, this model only appears in the fast model selector */
fastOnly?: boolean;
/** When true, this model only appears in the voice model selector */
voiceOnly?: boolean;

/** Whether this is a runtime model (not from modelProviders) */
isRuntimeModel?: boolean;

Expand Down
Loading