-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(cli): add --project and --global flags to /model for per-project model persistence #6060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
6bc8262
f85eca7
4597304
51e44a7
f1589c4
50a1453
a49c077
8d1cd20
1807478
a594a70
2ccb615
e5de0e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ describe('modelCommand', () => { | |
| it('should have the correct name and description', () => { | ||
| expect(modelCommand.name).toBe('model'); | ||
| expect(modelCommand.description).toBe( | ||
| 'Switch the model for this session (--fast for suggestion model, --voice for voice transcription model, --vision for the vision bridge model, [model-id] to switch immediately).', | ||
| 'Switch the model for this session (--fast for suggestion model, --voice for voice transcription model, --vision for the vision bridge model, --project to persist to project settings, --global to persist to user settings, [model-id] to switch immediately).', | ||
| ); | ||
| }); | ||
|
|
||
|
|
@@ -1669,4 +1669,147 @@ describe('modelCommand', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The new "scope flags" describe block covers dialog returns, fast-model direct-set, main-model direct-set, untrusted workspace, and no-flag default — good coverage of the happy path. However, several specific code paths in the diff remain untested:
— qwen3.7-max via Qwen Code /review |
||
| describe('scope flags', () => { | ||
| function setupContext() { | ||
| const mockConfig = createMockConfig({ | ||
| model: 'test-model', | ||
| authType: AuthType.USE_OPENAI, | ||
| }); | ||
| ( | ||
| mockConfig as Partial<Config> & { [key: string]: unknown } | ||
| ).getAvailableModelsForAuthType = vi.fn().mockReturnValue([]); | ||
| ( | ||
| mockConfig as Partial<Config> & { [key: string]: unknown } | ||
| ).getAllConfiguredModels = vi.fn().mockReturnValue([]); | ||
| mockContext.services.config = mockConfig as Config; | ||
| return mockContext; | ||
| } | ||
|
|
||
| it('should include persistScope in dialog return for /model --project', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--project'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'model', | ||
| persistScope: 'workspace', | ||
| }); | ||
| }); | ||
|
|
||
| it('should include persistScope in dialog return for /model --global', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--global'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'model', | ||
| persistScope: 'user', | ||
| }); | ||
| }); | ||
|
|
||
| it('should include persistScope for /model --project --fast dialog', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--project --fast'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'fast-model', | ||
| persistScope: 'workspace', | ||
| }); | ||
| }); | ||
|
|
||
| it('should include persistScope for /model --global --voice dialog', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--global --voice'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'voice-model', | ||
| persistScope: 'user', | ||
| }); | ||
| }); | ||
|
|
||
| it('should include persistScope for /model --project --vision dialog', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--project --vision'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'vision-model', | ||
| persistScope: 'workspace', | ||
| }); | ||
| }); | ||
|
|
||
| it('should parse scope flags in any position', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, '--fast --project'); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'fast-model', | ||
| persistScope: 'workspace', | ||
| }); | ||
| }); | ||
|
|
||
| it('should show scope suffix in fast model confirmation', async () => { | ||
| const setValue = vi.fn(); | ||
| const settings = { | ||
| ...createMockSettings(setValue), | ||
| _merged: {}, | ||
| computeMergedSettings: vi.fn(), | ||
| } as unknown as LoadedSettings; | ||
| const ctx = setupContext(); | ||
| ctx.services.settings = settings; | ||
| const cfg = ctx.services.config as unknown as Partial<Config> & { | ||
| [key: string]: unknown; | ||
| }; | ||
| cfg.getAllConfiguredModels = vi | ||
| .fn() | ||
| .mockReturnValue([ | ||
| { id: 'qwen3-coder-flash', voiceOnly: false, fastOnly: true }, | ||
| ]); | ||
| cfg.setFastModel = vi.fn(); | ||
| const result = await modelCommand.action!( | ||
| ctx, | ||
| '--project --fast qwen3-coder-flash', | ||
| ); | ||
| expect(result).toMatchObject({ | ||
| type: 'message', | ||
| content: expect.stringContaining('(project)'), | ||
| }); | ||
| }); | ||
|
|
||
| it('should show scope suffix in main model confirmation', async () => { | ||
| const setValue = vi.fn(); | ||
| const settings = { | ||
| ...createMockSettings(setValue), | ||
| _merged: {}, | ||
| computeMergedSettings: vi.fn(), | ||
| } as unknown as LoadedSettings; | ||
| const mockGenerator = { | ||
| authType: AuthType.USE_OPENAI, | ||
| model: 'qwen-max', | ||
| }; | ||
| const ctx = setupContext(); | ||
| ctx.services.settings = settings; | ||
| const cfg = ctx.services.config as unknown as Partial<Config> & { | ||
| [key: string]: unknown; | ||
| }; | ||
| cfg.getAvailableModelsForAuthType = vi | ||
| .fn() | ||
| .mockReturnValue([ | ||
| { id: 'qwen-max', voiceOnly: false, fastOnly: false }, | ||
| ]); | ||
| cfg.switchModel = vi.fn().mockResolvedValue(mockGenerator); | ||
| const result = await modelCommand.action!(ctx, '--project qwen-max'); | ||
| expect(result).toMatchObject({ | ||
| type: 'message', | ||
| content: expect.stringContaining('(project)'), | ||
| }); | ||
| }); | ||
|
|
||
| it('should not include persistScope when no scope flag is given', async () => { | ||
| const ctx = setupContext(); | ||
| const result = await modelCommand.action!(ctx, ''); | ||
| expect(result).toEqual({ | ||
| type: 'dialog', | ||
| dialog: 'model', | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Two error branches in
Suggested tests: it('should reject --project --global used together', async () => {
const ctx = setupContext();
const result = await modelCommand.action!(ctx, '--project --global');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('Cannot use both'),
});
});
it('should reject inline prompt combined with scope flag', async () => {
// Setup config so qwen-max resolves as a valid model...
const result = await modelCommand.action!(ctx, '--project qwen-max hello world');
expect(result).toMatchObject({
type: 'message',
messageType: 'error',
content: expect.stringContaining('Cannot combine'),
});
});— qwen3.7-max via Qwen Code /review |
||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The only test change in this PR is the description string update. There are no tests for:
parseScopeFlags()functionresolveScope()with scope overridespersistSetting()callspersistScopein dialog return objects--voice/--fast/--visionflag interactionsThe Critical finding (persistScope silently dropped for subcommand dialogs) would have been caught by a single test asserting that
/model --project --fastreturns a dialog action withpersistScope: 'workspace'.— qwen3.7-max via Qwen Code /review