Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 6
feat: add model roles for small, implementer, and advisor#56
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
4d48da1a86a9edc1d75e8b2c092a1e1559e4243f03File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@pythoughts/pythinker-code": minor | ||
| --- | ||
| Add model roles: lock a model alias to the small, implementer, or advisor slot with `/model <role>`, list assignments with `/model roles`, and reference roles as `@small`, `@implementer`, or `@advisor` wherever a subagent model can be set; an assigned implementer role becomes the default model for subagents. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -41,7 +41,11 @@ import { | ||
| openFileInExternalEditor, | ||
| resolveEditorCommand, | ||
| } from '#/utils/process/external-editor'; | ||
| import { LLM_NOT_SET_MESSAGE, NO_ACTIVE_SESSION_MESSAGE } from '../constant/pythinker-tui'; | ||
| import { | ||
| BUILT_IN_MODEL_ROLES, | ||
| LLM_NOT_SET_MESSAGE, | ||
| NO_ACTIVE_SESSION_MESSAGE, | ||
| } from '#/tui/constant/pythinker-tui'; | ||
| import { formatErrorMessage } from '../utils/event-payload'; | ||
| import { showUsage } from './info'; | ||
| import { setExperimentalFeatures } from './experimental-flags'; | ||
| @@ -480,6 +484,41 @@ function resolveWorkspaceConfigPath(input: string, workDir: string): string { | ||
| export async function handleModelCommand(host: SlashCommandHost, args: string): Promise<void> { | ||
| const requestedAlias = args.trim(); | ||
| const tokens = requestedAlias.split(/\s+/u).filter(Boolean); | ||
| const config = await host.harness.getConfig({ reload: true }); | ||
| const roles = [...new Set([...BUILT_IN_MODEL_ROLES, ...Object.keys(config.modelRoles ?? {})])] | ||
| .filter((role) => role.length > 0 && role !== 'default'); | ||
| if (tokens.length === 1 && tokens[0] === 'roles') { | ||
| host.showNotice( | ||
| 'Model roles', | ||
| roles | ||
| .map((role) => `${role}: ${config.modelRoles?.[role]?.trim() || '(not set)'}`) | ||
| .join('\n'), | ||
| ); | ||
| return; | ||
| } | ||
| const role = tokens[0]; | ||
| if (role !== undefined && roles.includes(role)) { | ||
| if (tokens.length === 2 && (tokens[1] === 'clear' || tokens[1] === 'none')) { | ||
| await host.harness.setConfig({ modelRoles: { [role]: '' } }); | ||
| host.showStatus(`Cleared the ${role} model role.`, 'success'); | ||
| return; | ||
| } | ||
| if (tokens.length === 1) { | ||
| const picker = showModelPicker(host, config.modelRoles?.[role], undefined, { | ||
| assignToRole: role, | ||
| }); | ||
| if (picker !== undefined) { | ||
| void refreshModelsForOpenPicker(host, picker, config.modelRoles?.[role], { | ||
| assignToRole: role, | ||
| }); | ||
| } | ||
| return; | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| } | ||
| const normalized = normalizeModelChoices(host.state.appState.availableModels); | ||
| const selectedValue = | ||
| requestedAlias.length === 0 | ||
| @@ -524,6 +563,7 @@ async function refreshModelsForOpenPicker( | ||
| host: SlashCommandHost, | ||
| picker: TabbedModelSelectorComponent, | ||
| selectedValue: string | undefined, | ||
| options?: { assignToRole?: string }, | ||
| ): Promise<void> { | ||
| const availableModels = host.state.appState.availableModels; | ||
| const normalized = normalizeModelChoices(availableModels); | ||
| @@ -574,7 +614,7 @@ async function refreshModelsForOpenPicker( | ||
| } | ||
| } | ||
| showModelPicker(host, refreshedSelected, activeTabId); | ||
| showModelPicker(host, refreshedSelected, activeTabId, options); | ||
| } | ||
| async function applyEditorChoice(host: SlashCommandHost, value: string): Promise<void> { | ||
| @@ -615,6 +655,7 @@ export function showModelPicker( | ||
| host: SlashCommandHost, | ||
| selectedValue?: string, | ||
| initialTabId?: string, | ||
| options?: { assignToRole?: string }, | ||
| ): TabbedModelSelectorComponent | undefined { | ||
| const normalized = normalizeModelChoices(host.state.appState.availableModels); | ||
| const entries = Object.entries(normalized.models); | ||
| @@ -646,6 +687,10 @@ export function showModelPicker( | ||
| initialTabId, | ||
| onSelect: ({ alias, effort }) => { | ||
| host.restoreEditor(); | ||
| if (options?.assignToRole !== undefined) { | ||
| void assignModelRole(host, options.assignToRole, alias); | ||
| return; | ||
| } | ||
| void performModelSwitch(host, alias, effort); | ||
| }, | ||
| onCancel: () => { | ||
| @@ -656,6 +701,17 @@ export function showModelPicker( | ||
| return picker; | ||
| } | ||
| async function assignModelRole(host: SlashCommandHost, role: string, alias: string): Promise<void> { | ||
| // Model roles store aliases only; thinking effort stays with the active model. | ||
| try { | ||
| await host.harness.setConfig({ modelRoles: { [role]: alias } }); | ||
| } catch (error) { | ||
| host.showError(`Failed to lock the ${role} model: ${formatErrorMessage(error)}`); | ||
| return; | ||
| } | ||
| host.showStatus(`Locked ${alias} as the ${role} model.`, 'success'); | ||
| } | ||
| async function performModelSwitch(host: SlashCommandHost, alias: string, effort: string): Promise<void> { | ||
| if (host.state.appState.streamingPhase !== 'idle') { | ||
| host.showError('Cannot switch models while streaming — press Esc or Ctrl-C first.'); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { handleModelCommand } from '#/tui/commands/index'; | ||
| import type { SlashCommandHost } from '#/tui/commands/dispatch'; | ||
| const ENTER = '\r'; | ||
| interface TestPicker { | ||
| handleInput(data: string): void; | ||
| } | ||
| function model(name: string) { | ||
| return { | ||
| provider: 'test', | ||
| model: name, | ||
| maxContextSize: 200_000, | ||
| displayName: name, | ||
| capabilities: [], | ||
| }; | ||
| } | ||
| function makeHost(options: { | ||
| currentModel?: string; | ||
| availableModels?: Record<string, ReturnType<typeof model>>; | ||
| modelRoles?: Record<string, string>; | ||
| setConfig?: ReturnType<typeof vi.fn>; | ||
| } = {}) { | ||
| const session = { | ||
| setModel: vi.fn(async () => {}), | ||
| setThinking: vi.fn(async () => {}), | ||
| }; | ||
| const getConfig = vi.fn(async () => ({ | ||
| providers: {}, | ||
| modelRoles: options.modelRoles, | ||
| })); | ||
| const setConfig = options.setConfig ?? vi.fn(async () => {}); | ||
| const host = { | ||
| state: { | ||
| appState: { | ||
| model: options.currentModel ?? 'worker', | ||
| thinkingLevel: 'off', | ||
| streamingPhase: 'idle', | ||
| availableModels: options.availableModels ?? { worker: model('worker') }, | ||
| }, | ||
| editorContainer: { children: [] }, | ||
| }, | ||
| session, | ||
| harness: { getConfig, setConfig }, | ||
| authFlow: { | ||
| refreshProviderModels: vi.fn(async () => ({ failed: [] })), | ||
| }, | ||
| setAppState: vi.fn((patch: Record<string, unknown>) => Object.assign(host.state.appState, patch)), | ||
| showError: vi.fn(), | ||
| showStatus: vi.fn(), | ||
| showNotice: vi.fn(), | ||
| mountEditorReplacement: vi.fn(), | ||
| restoreEditor: vi.fn(), | ||
| track: vi.fn(), | ||
| } as unknown as SlashCommandHost; | ||
| return { host, session, setConfig }; | ||
| } | ||
| function mountedPicker(host: SlashCommandHost, index = 0): TestPicker { | ||
| const mount = host.mountEditorReplacement as ReturnType<typeof vi.fn>; | ||
| return mount.mock.calls[index]?.[0] as TestPicker; | ||
| } | ||
| describe('/model roles', () => { | ||
| it('lists every built-in role as not set when no assignments exist', async () => { | ||
| const { host } = makeHost(); | ||
| await handleModelCommand(host, 'roles'); | ||
| expect(host.showNotice).toHaveBeenCalledWith( | ||
| 'Model roles', | ||
| 'small: (not set)\nimplementer: (not set)\nadvisor: (not set)', | ||
| ); | ||
| }); | ||
| it('locks a selected alias to a role without switching the session model', async () => { | ||
| const { host, session, setConfig } = makeHost(); | ||
| await handleModelCommand(host, 'small'); | ||
| expect(host.authFlow.refreshProviderModels).toHaveBeenCalledOnce(); | ||
| mountedPicker(host).handleInput(ENTER); | ||
| await vi.waitFor(() => { | ||
| expect(setConfig).toHaveBeenCalledWith({ modelRoles: { small: 'worker' } }); | ||
| }); | ||
| expect(session.setModel).not.toHaveBeenCalled(); | ||
| }); | ||
| it('keeps role assignment active after the picker refreshes', async () => { | ||
| const { host, session, setConfig } = makeHost({ | ||
| currentModel: 'parent', | ||
| availableModels: { | ||
| parent: model('parent'), | ||
| worker: model('worker'), | ||
| }, | ||
| modelRoles: { small: 'worker' }, | ||
| }); | ||
| vi.mocked(host.mountEditorReplacement).mockImplementation((picker) => { | ||
| host.state.editorContainer.children[0] = picker; | ||
| }); | ||
| vi.mocked(host.authFlow.refreshProviderModels).mockImplementation(async () => { | ||
| host.state.appState.availableModels['reviewer'] = model('reviewer'); | ||
| return { changed: [], unchanged: [], failed: [] }; | ||
| }); | ||
| await handleModelCommand(host, 'small'); | ||
| await vi.waitFor(() => { | ||
| expect(host.mountEditorReplacement).toHaveBeenCalledTimes(2); | ||
| }); | ||
| mountedPicker(host, 1).handleInput(ENTER); | ||
| await vi.waitFor(() => { | ||
| expect(setConfig).toHaveBeenCalledWith({ modelRoles: { small: 'worker' } }); | ||
| }); | ||
| expect(session.setModel).not.toHaveBeenCalled(); | ||
| }); | ||
| it('reports a role persistence failure without showing success', async () => { | ||
| const setConfig = vi.fn(async () => { | ||
| throw new Error('disk full'); | ||
| }); | ||
| const { host } = makeHost({ setConfig }); | ||
| await handleModelCommand(host, 'small'); | ||
| mountedPicker(host).handleInput(ENTER); | ||
| await vi.waitFor(() => { | ||
| expect(host.showError).toHaveBeenCalledWith(expect.stringContaining('disk full')); | ||
| }); | ||
| expect(host.showStatus).not.toHaveBeenCalled(); | ||
| }); | ||
| it('clears a role with an empty-string tombstone', async () => { | ||
| const { host, setConfig } = makeHost({ modelRoles: { small: 'worker' } }); | ||
| await handleModelCommand(host, 'small clear'); | ||
| expect(setConfig).toHaveBeenCalledWith({ modelRoles: { small: '' } }); | ||
| }); | ||
| it('keeps an existing model alias on the default switch path', async () => { | ||
| const { host, session } = makeHost({ | ||
| currentModel: 'parent', | ||
| availableModels: { | ||
| parent: model('parent'), | ||
| worker: model('worker'), | ||
| }, | ||
| }); | ||
| await handleModelCommand(host, 'worker'); | ||
| mountedPicker(host).handleInput(ENTER); | ||
| await vi.waitFor(() => { | ||
| expect(session.setModel).toHaveBeenCalledWith('worker'); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** Built-in model roles a user can lock a model alias to. */ | ||
| export const BUILT_IN_MODEL_ROLES = ['small', 'implementer', 'advisor'] as const; | ||
| export type BuiltInModelRole = (typeof BUILT_IN_MODEL_ROLES)[number]; | ||
| interface ModelRoleSource { | ||
| modelRoles?: Record<string, string>; | ||
| defaultModel?: string; | ||
| } | ||
| /** Resolve a role name to its locked model alias. Empty string means cleared. */ | ||
| export function resolveModelRoleAlias( | ||
| config: ModelRoleSource | undefined, | ||
| role: string, | ||
| ): string | undefined { | ||
| if (role === 'default') return config?.defaultModel; | ||
| const alias = config?.modelRoles?.[role]?.trim(); | ||
| return alias === '' ? undefined : alias; | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| /** Expand a "@role" model reference; non-@ strings pass through unchanged. */ | ||
| export function expandModelRef( | ||
| config: ModelRoleSource | undefined, | ||
| ref: string, | ||
| ): string | undefined { | ||
| return ref.startsWith('@') ? resolveModelRoleAlias(config, ref.slice(1)) : ref; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.