diff --git a/scripts/migrations/337-claude-sonnet-5-additive.js b/scripts/migrations/337-claude-sonnet-5-additive.js index d02b6d03af..92306e3d1c 100644 --- a/scripts/migrations/337-claude-sonnet-5-additive.js +++ b/scripts/migrations/337-claude-sonnet-5-additive.js @@ -10,9 +10,10 @@ * that record: the reviewer/task model pickers reading it offer the retired * sonnet and cannot offer the current one at all. * - * ADDITIVE, deliberately — the opposite policy from 153/206 and from - * `makeSeededProviderTierMigration`, because this one runs against lists the - * user curated: + * Built on `makeAdditiveProviderInsertMigration` (`_lib.js` family 7b) — + * ADDITIVE, deliberately the opposite policy from 153/206 and from + * `makeSeededProviderTierMigration` (family 7), because this one runs against + * lists the user curated: * * - `claude-sonnet-5` is INSERTED right after `claude-sonnet-4-6`, and the * retired id is KEPT. `claude-sonnet-4-6` still resolves for the CLI, so @@ -27,56 +28,16 @@ * or a fresh `data.reference` seed already put it there) and on a second run. */ -import { readProvidersDoc, writeJsonAtomic } from './_lib.js'; - -const PROVIDERS_REL_PATH = 'data/providers.json'; +import { makeAdditiveProviderInsertMigration } from './_lib.js'; // The four seeded Claude records and the sonnet id each one spells. The Bedrock // pair uses the region-qualified form its own environment resolves — inserting a // bare `claude-sonnet-5` there would offer an id that record cannot run. -const TARGETS = [ +export const TARGETS = [ { id: 'claude-code', retired: 'claude-sonnet-4-6', current: 'claude-sonnet-5' }, { id: 'claude-code-tui', retired: 'claude-sonnet-4-6', current: 'claude-sonnet-5' }, { id: 'claude-code-bedrock', retired: 'us.anthropic.claude-sonnet-4-6', current: 'us.anthropic.claude-sonnet-5' }, { id: 'claude-code-tui-bedrock', retired: 'us.anthropic.claude-sonnet-4-6', current: 'us.anthropic.claude-sonnet-5' }, ]; -export default { - async up({ rootDir }) { - const doc = await readProvidersDoc({ rootDir }); - if (!doc.ok) { - if (doc.reason === 'no-file') console.log(`📄 ${PROVIDERS_REL_PATH} not present — skipping (fresh install seeds claude-sonnet-5 from data.reference)`); - else if (doc.reason === 'unreadable') console.log(`⚠️ ${PROVIDERS_REL_PATH}: invalid JSON, skipping (${doc.err.message})`); - else console.log(`⚠️ ${PROVIDERS_REL_PATH}: unexpected shape, skipping`); - return { ok: false, reason: doc.reason, updated: 0 }; - } - - const { config, providers, path: providersPath } = doc; - const touched = []; - - for (const { id, retired, current } of TARGETS) { - const provider = providers[id]; - if (!provider || !Array.isArray(provider.models)) continue; - const at = provider.models.indexOf(retired); - // Nothing to repair unless the retired id is listed AND the current one - // isn't: an already-current record (seeded, or bumped by 153) is a no-op, - // and a record that never listed the retired tier is not this bug. - if (at === -1 || provider.models.includes(current)) continue; - provider.models = [ - ...provider.models.slice(0, at + 1), - current, - ...provider.models.slice(at + 1), - ]; - touched.push(id); - } - - if (touched.length === 0) { - console.log(`✅ ${PROVIDERS_REL_PATH}: Claude sonnet tier already current — no change`); - return { ok: true, reason: 'already-current', updated: 0 }; - } - - await writeJsonAtomic(providersPath, config); - console.log(`📝 ${PROVIDERS_REL_PATH}: offered claude-sonnet-5 on ${touched.join(', ')}`); - return { ok: true, reason: 'updated', updated: touched.length }; - }, -}; +export default makeAdditiveProviderInsertMigration({ targets: TARGETS, label: 'claude-sonnet-5' }); diff --git a/scripts/migrations/337-claude-sonnet-5-additive.test.js b/scripts/migrations/337-claude-sonnet-5-additive.test.js index 47316977fe..f9503aabe0 100644 --- a/scripts/migrations/337-claude-sonnet-5-additive.test.js +++ b/scripts/migrations/337-claude-sonnet-5-additive.test.js @@ -1,125 +1,14 @@ /** * Test for migration 337 — offer `claude-sonnet-5` on a Claude CLI/TUI record - * that still lists only the retired `claude-sonnet-4-6` tier. + * that still lists only the retired `claude-sonnet-4-6` tier. Built on + * `makeAdditiveProviderInsertMigration` (`_lib.js` family 7b); the shared + * contract lives in `runAdditiveProviderInsertMigrationTests` + * (`_testHelpers.js`) and is exercised here against 337's own `targets`. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync, readFileSync, mkdirSync } from 'fs'; -import { tmpdir } from 'os'; -import { join } from 'path'; - -import migration from './337-claude-sonnet-5-additive.js'; - -const writeJson = (path, value) => writeFileSync(path, JSON.stringify(value, null, 2) + '\n'); -const readJson = (path) => JSON.parse(readFileSync(path, 'utf-8')); +import { describe } from 'vitest'; +import migration, { TARGETS } from './337-claude-sonnet-5-additive.js'; +import { runAdditiveProviderInsertMigrationTests } from './_testHelpers.js'; describe('migration 337 — claude-sonnet-5 additive repair', () => { - let rootDir; - let providersPath; - - beforeEach(() => { - rootDir = mkdtempSync(join(tmpdir(), 'portos-337-')); - mkdirSync(join(rootDir, 'data')); - providersPath = join(rootDir, 'data', 'providers.json'); - }); - - afterEach(() => rmSync(rootDir, { recursive: true, force: true })); - - const seed = (providers) => writeJson(providersPath, { activeProvider: 'claude-code', providers }); - - it('inserts claude-sonnet-5 after the retired tier on a CURATED list 153 skipped', async () => { - seed({ - 'claude-code': { - models: ['claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-opus-5', 'claude-fable-5'], - defaultModel: 'claude-opus-5', - mediumModel: 'claude-sonnet-4-6', - }, - }); - - const result = await migration.up({ rootDir }); - - expect(result).toMatchObject({ ok: true, reason: 'updated', updated: 1 }); - const after = readJson(providersPath).providers['claude-code']; - expect(after.models).toEqual([ - 'claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-sonnet-5', 'claude-opus-5', 'claude-fable-5', - ]); - // Additive: the retired id and every tier pointer survive untouched. - expect(after.mediumModel).toBe('claude-sonnet-4-6'); - expect(after.defaultModel).toBe('claude-opus-5'); - }); - - it('uses each Bedrock record\'s own region-qualified sonnet spelling', async () => { - seed({ - 'claude-code-bedrock': { - models: ['us.anthropic.claude-haiku-4-5', 'us.anthropic.claude-sonnet-4-6', 'global.anthropic.claude-opus-5'], - }, - 'claude-code-tui-bedrock': { - models: ['us.anthropic.claude-sonnet-4-6'], - }, - }); - - const result = await migration.up({ rootDir }); - - expect(result.updated).toBe(2); - const { providers } = readJson(providersPath); - expect(providers['claude-code-bedrock'].models).toEqual([ - 'us.anthropic.claude-haiku-4-5', - 'us.anthropic.claude-sonnet-4-6', - 'us.anthropic.claude-sonnet-5', - 'global.anthropic.claude-opus-5', - ]); - expect(providers['claude-code-tui-bedrock'].models).toEqual([ - 'us.anthropic.claude-sonnet-4-6', - 'us.anthropic.claude-sonnet-5', - ]); - // The bare id must never leak into a Bedrock record — its environment - // resolves only the region-qualified form. - expect(providers['claude-code-bedrock'].models).not.toContain('claude-sonnet-5'); - }); - - it('is a no-op on an already-current record and on a second run', async () => { - seed({ - 'claude-code': { models: ['claude-haiku-4-5', 'claude-sonnet-5', 'claude-opus-5'] }, - 'claude-code-tui': { models: ['claude-haiku-4-5', 'claude-sonnet-4-6', 'claude-sonnet-5'] }, - }); - - const first = await migration.up({ rootDir }); - expect(first).toMatchObject({ ok: true, reason: 'already-current', updated: 0 }); - - // And a record it DID repair stays repaired rather than gaining a duplicate. - seed({ 'claude-code': { models: ['claude-sonnet-4-6'] } }); - expect((await migration.up({ rootDir })).updated).toBe(1); - const second = await migration.up({ rootDir }); - expect(second.updated).toBe(0); - expect(readJson(providersPath).providers['claude-code'].models) - .toEqual(['claude-sonnet-4-6', 'claude-sonnet-5']); - }); - - it('leaves records outside the four seeded Claude ids alone', async () => { - seed({ - 'claude-ollama': { models: ['claude-sonnet-4-6'] }, - 'antigravity-cli': { models: ['claude-sonnet-4-6'] }, - }); - - expect((await migration.up({ rootDir })).updated).toBe(0); - const { providers } = readJson(providersPath); - expect(providers['claude-ollama'].models).toEqual(['claude-sonnet-4-6']); - expect(providers['antigravity-cli'].models).toEqual(['claude-sonnet-4-6']); - }); - - it('skips a missing or malformed providers file without throwing', async () => { - expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'no-file' }); - - writeFileSync(providersPath, '{not json'); - expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'unreadable' }); - - writeJson(providersPath, { activeProvider: 'claude-code' }); - expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'bad-shape' }); - }); - - it('skips a record whose models field is not an array', async () => { - seed({ 'claude-code': { models: 'claude-sonnet-4-6', defaultModel: 'claude-sonnet-4-6' } }); - - expect((await migration.up({ rootDir })).updated).toBe(0); - expect(readJson(providersPath).providers['claude-code'].models).toBe('claude-sonnet-4-6'); - }); + runAdditiveProviderInsertMigrationTests({ migration, targets: TARGETS, prefix: 'migration-337-' }); }); diff --git a/scripts/migrations/_lib.js b/scripts/migrations/_lib.js index 1405d5c5a0..a0df499dbf 100644 --- a/scripts/migrations/_lib.js +++ b/scripts/migrations/_lib.js @@ -22,11 +22,22 @@ * 032 / 058 / 153 / 206 each hand-copied. Those four stay frozen; the * factory is for the next RETIREMENT bump. It expresses id→id retirement * only: `idMap` maps 1:1 over `oldModels`, and `swapTierPointers` moves - * EVERY pointer off a retired id. An ADDITIVE change — appending a model - * while keeping the old one listed, or re-pointing some tiers but not all - * (292, 294) — does not fit, and hand-rolls on `readProvidersDoc` instead. + * EVERY pointer off a retired id. This is a **pristine-list rewrite** — + * only an EXACT match of the prior seeded `models` array qualifies, so a + * curated list is left alone rather than reset. + * 7b. Additive provider-model inserts — `makeAdditiveProviderInsertMigration` + * (337 is the first and, so far, only member) is the opposite policy for + * the SAME `data/providers.json` targets: it runs against a list the user + * may have curated, so it never rewrites or drops anything — it only + * splices `current` in immediately after `retired` when `retired` is + * listed and `current` is not, regardless of what else the list holds. + * Tier pointers are left alone (a curated list is exactly where + * re-pointing would override a deliberate choice). Reach for family 7 + * when retiring an id from the shipped seed; reach for 7b when a later + * tier needs to be OFFERED without disturbing whatever the user already + * has listed. * - * Families 5 and 7 both target `data/providers.json` and share its + * Families 5, 7, and 7b all target `data/providers.json` and share its * read → parse → shape-guard preamble via `readProvidersDoc`; each still owns * its own log copy and result shape. * @@ -1080,6 +1091,84 @@ export function makeSeededProviderTierMigration({ targets, tierLabel }) { return { up }; } +// ---- additive provider-model insert migration family (7b) ---- +// +// The opposite policy from family 7, for the same `data/providers.json` +// targets: family 7 rewrites a PRISTINE seeded list wholesale; this one splices +// a new id into a list the user may have curated, touching nothing else. +// 337 is the first member and the shell every future "offer a model without +// disturbing what's already listed" migration should reach for instead of +// hand-copying it a fourth time. + +/** + * Build an additive provider-model insert migration's `up()`. Returns `{ up }`, + * so a migration collapses to + * `export default makeAdditiveProviderInsertMigration({ targets, label })` + * over a small data table. + * + * - `targets` — `[{ id, retired, current }, …]`, one entry per provider + * record this migration can touch. `retired` and `current` are per-target + * (not shared across the table) so sibling records that spell the same + * tier differently — e.g. a Bedrock record's region-qualified id versus the + * bare CLI id — each get their own exact strings; inserting one target's + * `current` onto another target's record would offer an id that record's + * environment cannot resolve. + * - `label` — the human name of the tier being offered, used in the log + * copy (`"offered ${label} on …"`, `"${label} already current"`). + * + * For each target: if `retired` is listed in the record's `models` and + * `current` is not, `current` is spliced in immediately after `retired` — + * `retired` itself, every other entry, and every tier pointer are left + * exactly as found. A record already listing `current`, or never listing + * `retired`, is untouched — which makes a second run, and a fresh install + * already seeded with `current`, both no-ops. + * + * Resolves to `{ ok, reason: 'no-file' | 'unreadable' | 'bad-shape' | + * 'already-current' | 'updated', updated }`. + */ +export function makeAdditiveProviderInsertMigration({ targets, label }) { + async function up({ rootDir }) { + const doc = await readProvidersDoc({ rootDir }); + if (!doc.ok) { + if (doc.reason === 'no-file') console.log(`📄 ${PROVIDERS_REL_PATH} not present — skipping (fresh install seeds ${label} from data.reference)`); + else if (doc.reason === 'unreadable') console.log(`⚠️ ${PROVIDERS_REL_PATH}: invalid JSON, skipping (${doc.err.message})`); + else console.log(`⚠️ ${PROVIDERS_REL_PATH}: unexpected shape, skipping`); + return { ok: false, reason: doc.reason, updated: 0 }; + } + + const { config, providers, path: providersPath } = doc; + const touched = []; + + for (const { id, retired, current } of targets) { + const provider = providers[id]; + if (!provider || !Array.isArray(provider.models)) continue; + const at = provider.models.indexOf(retired); + // Nothing to repair unless the retired id is listed AND the current one + // isn't: an already-current record (seeded, or bumped by a prior run) is + // a no-op, and a record that never listed the retired tier is not this + // migration's concern. + if (at === -1 || provider.models.includes(current)) continue; + provider.models = [ + ...provider.models.slice(0, at + 1), + current, + ...provider.models.slice(at + 1), + ]; + touched.push(id); + } + + if (touched.length === 0) { + console.log(`✅ ${PROVIDERS_REL_PATH}: ${label} already current — no change`); + return { ok: true, reason: 'already-current', updated: 0 }; + } + + await writeJsonAtomic(providersPath, config); + console.log(`📝 ${PROVIDERS_REL_PATH}: offered ${label} on ${touched.join(', ')}`); + return { ok: true, reason: 'updated', updated: touched.length }; + } + + return { up }; +} + /** * Build the per-subdir prompt-drift tables that `scripts/setup-data.js` uses * for its "pending migration" warning by sweeping every numbered migration's diff --git a/scripts/migrations/_testHelpers.js b/scripts/migrations/_testHelpers.js index 8fd1dbff98..cc75d1fcec 100644 --- a/scripts/migrations/_testHelpers.js +++ b/scripts/migrations/_testHelpers.js @@ -10,6 +10,10 @@ * with `makeSeededProviderTierMigration`. Every fixture is derived from the * migration's own `targets` table, so a bump's test is a `describe` + one * call and still asserts the full conservative contract. + * - `runAdditiveProviderInsertMigrationTests` — additive provider-model + * inserts built with `makeAdditiveProviderInsertMigration` (family 7b). + * Same shape: a `describe` + one call, with fixtures derived from the + * migration's own `targets` array. */ import { it, expect, beforeEach, afterEach } from 'vitest'; @@ -378,3 +382,128 @@ export function runSeededProviderTierMigrationTests({ migration, targets, prefix } }); } + +// ---- additive provider-model insert migrations (family 7b) ---- + +/** + * Standard suite for a migration built with `makeAdditiveProviderInsertMigration`. + * A migration's `*.test.js` collapses to a `describe` + a single + * `runAdditiveProviderInsertMigrationTests({ migration, targets, prefix })` call. + * + * - `migration` — the migration's default export (`{ up }`). + * - `targets` — the SAME `targets` array passed to the factory + * (`[{ id, retired, current }, …]`). Every fixture is derived from it, so + * the suite covers each target the migration actually ships. + * - `prefix` — `mkdtempSync` directory name (`'migration-337-'`), so a + * debugger leaves a recognizable sandbox in the temp dir. + */ +export function runAdditiveProviderInsertMigrationTests({ migration, targets, prefix }) { + let rootDir; + let providersPath; + + beforeEach(() => { + rootDir = mkdtempSync(join(tmpdir(), prefix)); + mkdirSync(join(rootDir, 'data'), { recursive: true }); + providersPath = join(rootDir, 'data', 'providers.json'); + }); + + afterEach(() => { + rmSync(rootDir, { recursive: true, force: true }); + }); + + const write = (providers) => writeFileSync(providersPath, JSON.stringify({ providers }, null, 2) + '\n'); + const read = () => JSON.parse(readFileSync(providersPath, 'utf-8')).providers; + const raw = () => readFileSync(providersPath, 'utf-8'); + + it('declares at least one target', () => { + expect(targets.length).toBeGreaterThan(0); + for (const { retired, current } of targets) { + expect(retired).not.toBe(current); + } + }); + + it('inserts each target\'s current id right after its retired id on a curated list, leaving pointers alone', async () => { + write(Object.fromEntries(targets.map(({ id, retired }) => [ + id, + { models: ['unrelated-tier', retired, 'sibling-tier'], defaultModel: 'unrelated-tier', mediumModel: retired }, + ]))); + + const result = await migration.up({ rootDir }); + + expect(result).toMatchObject({ ok: true, reason: 'updated', updated: targets.length }); + const out = read(); + for (const { id, retired, current } of targets) { + expect(out[id].models).toEqual(['unrelated-tier', retired, current, 'sibling-tier']); + // Additive: the retired id and every tier pointer survive untouched. + expect(out[id].defaultModel).toBe('unrelated-tier'); + expect(out[id].mediumModel).toBe(retired); + } + }); + + it('never offers one target\'s current id on a different target\'s record', async () => { + const distinct = targets.filter(({ current }, i) => targets.findIndex((t) => t.current === current) === i); + if (distinct.length < 2) return; // nothing to cross-check when every target shares one id + write(Object.fromEntries(targets.map(({ id, retired }) => [id, { models: [retired] }]))); + + await migration.up({ rootDir }); + + const out = read(); + for (const { id, current } of targets) { + for (const other of distinct) { + if (other.current === current) continue; + expect(out[id].models).not.toContain(other.current); + } + } + }); + + it('is a no-op on an already-current record and on a second run', async () => { + const [first, ...rest] = targets; + write(Object.fromEntries([ + [first.id, { models: [first.retired, first.current] }], + ...rest.map(({ id, retired }) => [id, { models: [retired] }]), + ])); + + const before = raw(); + const firstRun = await migration.up({ rootDir }); + expect(firstRun.updated).toBe(rest.length); + if (rest.length > 0) expect(raw()).not.toBe(before); // the repairable targets did get written once + + // A record repaired on the first run is untouched by a second — no + // duplicate insert — and the already-current record was never touched. + const secondRun = await migration.up({ rootDir }); + expect(secondRun).toMatchObject({ ok: true, reason: 'already-current', updated: 0 }); + for (const { id, retired, current } of targets) { + expect(read()[id].models).toEqual([retired, current]); + } + }); + + it('leaves providers outside the target set untouched', async () => { + const unrelated = { id: 'unrelated-provider', models: ['some-configured-default'], defaultModel: 'some-configured-default' }; + write({ + ...Object.fromEntries(targets.map(({ id, retired }) => [id, { models: [retired] }])), + 'unrelated-provider': unrelated, + }); + + await migration.up({ rootDir }); + + expect(read()['unrelated-provider']).toEqual(unrelated); + }); + + it('skips a missing or malformed providers file without throwing', async () => { + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'no-file' }); + + writeFileSync(providersPath, '{not json'); + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'unreadable' }); + + writeFileSync(providersPath, JSON.stringify({ activeProvider: 'x' }, null, 2) + '\n'); + expect(await migration.up({ rootDir })).toMatchObject({ ok: false, reason: 'bad-shape' }); + }); + + it('skips a record whose models field is not an array', async () => { + const [first] = targets; + write({ [first.id]: { models: first.retired, defaultModel: first.retired } }); + + expect((await migration.up({ rootDir })).updated).toBe(0); + expect(read()[first.id].models).toBe(first.retired); + }); +}