feat(model-switch): add commands to save preferred models

.changeset/violet-keys-push.md

@@ -0,0 +1,5 @@
+---
+'@nicknisi/pi-model-switch': minor
+---
+
+Add `/model-switch add` and `/model-switch add-current` to save models to a chosen section without editing JSON. Preserve existing config data and skip duplicates within each section.

packages/model-switch/README.md

@@ -34,6 +34,18 @@ These extension-owned action IDs are ignored by Pi's built-in keybinding manager
 
 ## Configure models
 
+### Add models from Pi
+
+Run `/model-switch add` to search Pi's available model catalog, pick a model, then choose an existing section to save it in. Run `/model-switch add-current` to save the model you're already using without opening the catalog picker. Both subcommands have argument completion.
+
+If the config file is missing, either command asks you to name the first section and creates the file. With an existing config, the section picker includes empty sections too. Add more sections by editing the config below.
+
+Adding a model does not switch the active model. Duplicate references in the chosen section are skipped, but the same model can belong to multiple sections. Escape cancels without writing. Saves preserve the legacy flat format, other sections, and extra config fields. Invalid configs produce a warning and are not overwritten.
+
+The catalog picker requires Pi's terminal UI. `add-current` also works with RPC clients that support Pi's selection and input dialogs. Neither command registers new providers or model definitions. The model must already be known to Pi.
+
+### Edit the config file
+
 Copy the example config:
 
 ```bash
@@ -125,7 +137,7 @@ The config may be missing, empty, malformed, or contain only missing/unauthentic
 - The picker uses a custom search + list component with `ctx.ui.custom()`; native `/model` and Ctrl+L remain available for the full catalog.
 - Terminal support for multi-modifier keys varies; configure simpler non-conflicting keys or use a terminal with the Kitty keyboard protocol when modified keys are not distinguishable.
 - Availability is checked on each interaction, which may resolve provider credentials before switching.
-- Duplicate references within a section are preserved as written; avoid them unless repeated cycle positions are intentional.
+- Duplicate references within a section are preserved as written; avoid them unless repeated cycle positions are intentional. The add commands never insert another copy of an existing reference.
 
 ## Development
 

packages/model-switch/config.test.ts

@@ -1,8 +1,16 @@
-import { afterEach, describe, expect, it } from 'vitest';
-import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import * as fs from 'node:fs';
+import { lstatSync, mkdtempSync, readFileSync, readdirSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
-import { DEFAULT_MODEL_CYCLE_KEYBINDINGS, loadModelSwitchConfig, loadModelSwitchKeybindings } from './config.js';
+import {
+  addModelToSection,
+  DEFAULT_MODEL_CYCLE_KEYBINDINGS,
+  loadModelSwitchConfig,
+  loadModelSwitchKeybindings,
+} from './config.js';
+
+vi.mock('node:fs', async (importOriginal) => ({ ...(await importOriginal<typeof import('node:fs')>()) }));
 
 const tempDirs: string[] = [];
 
@@ -15,6 +23,7 @@ function tempConfig(content?: string): string {
 }
 
 afterEach(() => {
+  vi.restoreAllMocks();
   for (const dir of tempDirs.splice(0)) {
     rmSync(dir, { recursive: true, force: true });
   }
@@ -132,6 +141,8 @@ describe('loadModelSwitchConfig', () => {
     ['missing sections and models', '{}'],
     ['non-object sections', JSON.stringify({ sections: 'nope' })],
     ['empty sections object', JSON.stringify({ sections: {} })],
+    ['array sections', JSON.stringify({ sections: [['provider/model']] })],
+    ['invalid sections with legacy models', JSON.stringify({ sections: null, models: ['provider/model'] })],
     ['non-array section models', JSON.stringify({ sections: { work: 'nope' } })],
     ['non-string model in section', JSON.stringify({ sections: { work: [42] } })],
     ['empty model in section', JSON.stringify({ sections: { work: ['  '] } })],
@@ -144,3 +155,118 @@ describe('loadModelSwitchConfig', () => {
     if (!result.ok) expect(result.error).toContain(path);
   });
 });
+
+describe('addModelToSection', () => {
+  it('appends only to the selected section and preserves other config fields', () => {
+    const config = {
+      sections: { work: ['provider/old'], personal: ['provider/new'] },
+      models: ['legacy/ignored'],
+      note: 'keep me',
+    };
+    const path = tempConfig(JSON.stringify(config));
+
+    expect(addModelToSection('provider/new', 'work', path)).toEqual({ ok: true, added: true });
+    expect(JSON.parse(readFileSync(path, 'utf8'))).toEqual({
+      ...config,
+      sections: { work: ['provider/old', 'provider/new'], personal: ['provider/new'] },
+    });
+  });
+
+  it('does not rewrite a duplicate, including a whitespace-padded reference', () => {
+    const content = '{ "sections": { "work": [" provider/model "] } }';
+    const path = tempConfig(content);
+
+    expect(addModelToSection('provider/model', 'work', path)).toEqual({ ok: true, added: false });
+    expect(readFileSync(path, 'utf8')).toBe(content);
+  });
+
+  it('keeps the legacy flat format', () => {
+    const path = tempConfig(JSON.stringify({ models: ['provider/old'], note: 'keep' }));
+
+    expect(addModelToSection('provider/new', 'models', path)).toEqual({ ok: true, added: true });
+    expect(JSON.parse(readFileSync(path, 'utf8'))).toEqual({
+      models: ['provider/old', 'provider/new'],
+      note: 'keep',
+    });
+  });
+
+  it('creates missing config directories and preserves slashes in model IDs', () => {
+    const path = join(tempConfig(), 'configs', 'model-switch.json');
+
+    expect(addModelToSection('fireworks/accounts/fireworks/models/kimi-k3', 'personal', path)).toEqual({
+      ok: true,
+      added: true,
+    });
+    expect(loadModelSwitchConfig(path)).toEqual({
+      ok: true,
+      config: { sections: [{ name: 'personal', models: ['fireworks/accounts/fireworks/models/kimi-k3'] }] },
+    });
+  });
+
+  it('supports section names that match object prototype properties', () => {
+    const path = tempConfig();
+
+    expect(addModelToSection('provider/model', '__proto__', path)).toEqual({ ok: true, added: true });
+    expect(loadModelSwitchConfig(path)).toEqual({
+      ok: true,
+      config: { sections: [{ name: '__proto__', models: ['provider/model'] }] },
+    });
+  });
+
+  it.each(['{ nope', '{"sections":null,"models":[]}', '{"sections":[[]]}', '{"sections":{"work":[42]}}'])(
+    'does not overwrite malformed config: %s',
+    (content) => {
+      const path = tempConfig(content);
+      const result = addModelToSection('provider/new', 'work', path);
+
+      expect(result.ok).toBe(false);
+      if (!result.ok) expect(result.error).toContain(path);
+      expect(readFileSync(path, 'utf8')).toBe(content);
+    },
+  );
+
+  it('does not recreate a section removed while the picker was open', () => {
+    const content = '{"sections":{"personal":[]}}';
+    const path = tempConfig(content);
+
+    expect(addModelToSection('provider/new', 'work', path)).toEqual({
+      ok: false,
+      error: `Section "work" no longer exists in ${path}`,
+    });
+    expect(readFileSync(path, 'utf8')).toBe(content);
+  });
+
+  it('updates a symlink target without replacing the link', () => {
+    const path = tempConfig('{"sections":{"work":[]}}');
+    const link = `${path}.link`;
+    symlinkSync(path, link);
+
+    expect(addModelToSection('provider/new', 'work', link)).toEqual({ ok: true, added: true });
+    expect(lstatSync(link).isSymbolicLink()).toBe(true);
+    expect(JSON.parse(readFileSync(path, 'utf8'))).toEqual({ sections: { work: ['provider/new'] } });
+  });
+
+  it('does not replace a dangling config symlink', () => {
+    const path = tempConfig();
+    const link = `${path}.link`;
+    symlinkSync(path, link);
+
+    expect(addModelToSection('provider/new', 'work', link).ok).toBe(false);
+    expect(lstatSync(link).isSymbolicLink()).toBe(true);
+  });
+
+  it('preserves the original config and cleans up when replacing the file fails', () => {
+    const content = '{"sections":{"work":[]}}';
+    const path = tempConfig(content);
+    vi.spyOn(fs, 'renameSync').mockImplementation(() => {
+      throw new Error('disk error');
+    });
+
+    expect(addModelToSection('provider/new', 'work', path)).toEqual({
+      ok: false,
+      error: `Could not update model-switch config at ${path}: disk error`,
+    });
+    expect(readFileSync(path, 'utf8')).toBe(content);
+    expect(readdirSync(join(path, '..'))).toEqual(['model-switch.json']);
+  });
+});

packages/model-switch/config.ts

@@ -1,5 +1,16 @@
-import { existsSync, readFileSync } from 'node:fs';
-import { join } from 'node:path';
+import { randomUUID } from 'node:crypto';
+import {
+  existsSync,
+  lstatSync,
+  mkdirSync,
+  readFileSync,
+  realpathSync,
+  renameSync,
+  rmSync,
+  statSync,
+  writeFileSync,
+} from 'node:fs';
+import { dirname, join } from 'node:path';
 import { getAgentDir } from '@earendil-works/pi-coding-agent';
 
 export interface ModelSwitchSection {
@@ -91,14 +102,21 @@ export function loadModelSwitchConfig(path = modelCycleConfigPath()): ConfigLoad
     return { ok: false, error: `Invalid model-switch config at ${path}: ${message}` };
   }
 
-  if (!value || typeof value !== 'object') {
+  return parseModelSwitchConfig(value, path);
+}
+
+function parseModelSwitchConfig(value: unknown, path: string): ConfigLoadResult {
+  if (!value || typeof value !== 'object' || Array.isArray(value)) {
     return { ok: false, error: `Invalid model-switch config at ${path}: expected an object` };
   }
 
   const obj = value as Record<string, unknown>;
 
   // Prefer "sections" if present; fall back to legacy "models" as a single section.
-  if ('sections' in obj && obj.sections && typeof obj.sections === 'object') {
+  if ('sections' in obj) {
+    if (!obj.sections || typeof obj.sections !== 'object' || Array.isArray(obj.sections)) {
+      return { ok: false, error: `Invalid model-switch config at ${path}: expected "sections" to be an object` };
+    }
     const sectionsRaw = obj.sections as Record<string, unknown>;
     const sections: ModelSwitchSection[] = [];
 
@@ -129,3 +147,44 @@ export function loadModelSwitchConfig(path = modelCycleConfigPath()): ConfigLoad
     error: `Invalid model-switch config at ${path}: expected { "sections": { ... } } or { "models": [...] }`,
   };
 }
+
+export function addModelToSection(
+  reference: string,
+  sectionName: string,
+  path = modelCycleConfigPath(),
+): { ok: true; added: boolean } | { ok: false; error: string } {
+  try {
+    // Read again after the dialogs so edits made while they were open are retained.
+    const existing = lstatSync(path, { throwIfNoEntry: false });
+    const target = existing ? realpathSync(path) : path;
+    const value = existing ? JSON.parse(readFileSync(target, 'utf8')) : { sections: { [sectionName]: [] } };
+    const loaded = parseModelSwitchConfig(value, path);
+    if (!loaded.ok) return loaded;
+
+    const section = loaded.config.sections.find((item) => item.name === sectionName);
+    if (!section) {
+      return { ok: false, error: `Section "${sectionName}" no longer exists in ${path}` };
+    }
+    if (section.models.includes(reference)) return { ok: true, added: false };
+
+    const models: string[] = 'sections' in value ? value.sections[sectionName] : value.models;
+    models.push(reference);
+
+    // Replace the file atomically, following config symlinks rather than replacing them.
+    mkdirSync(dirname(target), { recursive: true });
+    const temporaryPath = `${target}.${randomUUID()}.tmp`;
+    try {
+      writeFileSync(temporaryPath, `${JSON.stringify(value, null, 2)}\n`, {
+        flag: 'wx',
+        mode: existing ? statSync(target).mode & 0o777 : 0o600,
+      });
+      renameSync(temporaryPath, target);
+    } finally {
+      rmSync(temporaryPath, { force: true });
+    }
+    return { ok: true, added: true };
+  } catch (error) {
+    const message = error instanceof Error ? error.message : String(error);
+    return { ok: false, error: `Could not update model-switch config at ${path}: ${message}` };
+  }
+}