diff --git a/src/shared/migration/config-migration.test.ts b/src/shared/migration/config-migration.test.ts new file mode 100644 index 000000000..88f288e5b --- /dev/null +++ b/src/shared/migration/config-migration.test.ts @@ -0,0 +1,121 @@ +/// + +import { afterEach, describe, expect, test } from "bun:test" +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "fs" +import { tmpdir } from "os" +import { join } from "path" +import { migrateConfigFile } from "./config-migration" +import { getSidecarPath } from "./migrations-sidecar" + +const createdDirectories: string[] = [] +const MIGRATION_KEY = "model-version:anthropic/claude-opus-4-5->anthropic/claude-opus-4-6" + +function createWorkdir(): string { + const workdir = mkdtempSync(join(tmpdir(), "omo-config-migration-")) + createdDirectories.push(workdir) + return workdir +} + +function createLegacyConfig(): Record { + return { + agents: { + prometheus: { model: "anthropic/claude-opus-4-5" }, + }, + } +} + +afterEach(() => { + for (const directory of createdDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }) + } +}) + +describe("migrateConfigFile sidecar write ordering", () => { + test("writes the migrated config before recording the sidecar when both writes succeed", () => { + // given + const workdir = createWorkdir() + const configPath = join(workdir, "oh-my-opencode.json") + const rawConfig = createLegacyConfig() + + writeFileSync(configPath, JSON.stringify(rawConfig, null, 2) + "\n") + + // when + const needsWrite = migrateConfigFile(configPath, rawConfig) + + // then + expect(needsWrite).toBe(true) + expect(rawConfig._migrations).toBeUndefined() + expect((rawConfig.agents as Record>).prometheus.model).toBe( + "anthropic/claude-opus-4-6", + ) + + const persistedConfig = JSON.parse(readFileSync(configPath, "utf-8")) as Record + expect(persistedConfig._migrations).toBeUndefined() + expect((persistedConfig.agents as Record>).prometheus.model).toBe( + "anthropic/claude-opus-4-6", + ) + + const sidecar = JSON.parse(readFileSync(getSidecarPath(configPath), "utf-8")) as { + appliedMigrations: string[] + } + expect(sidecar.appliedMigrations).toEqual([MIGRATION_KEY]) + }) + + test("skips the sidecar when the config write fails so the migration retries on next startup", () => { + // given + const workdir = createWorkdir() + const configPath = join(workdir, "missing-parent", "oh-my-opencode.json") + const firstAttemptConfig = createLegacyConfig() + + // when + const firstAttemptNeedsWrite = migrateConfigFile(configPath, firstAttemptConfig) + + // then + expect(firstAttemptNeedsWrite).toBe(true) + expect(existsSync(getSidecarPath(configPath))).toBe(false) + expect(firstAttemptConfig._migrations).toEqual([MIGRATION_KEY]) + + // given + mkdirSync(join(workdir, "missing-parent"), { recursive: true }) + writeFileSync(configPath, JSON.stringify(createLegacyConfig(), null, 2) + "\n") + const retriedConfig = createLegacyConfig() + + // when + const retriedNeedsWrite = migrateConfigFile(configPath, retriedConfig) + + // then + expect(retriedNeedsWrite).toBe(true) + expect(retriedConfig._migrations).toBeUndefined() + expect((retriedConfig.agents as Record>).prometheus.model).toBe( + "anthropic/claude-opus-4-6", + ) + expect(existsSync(getSidecarPath(configPath))).toBe(true) + }) + + test("preserves _migrations in the config when the sidecar write fails after the config write succeeds", () => { + // given + const workdir = createWorkdir() + const configPath = join(workdir, "oh-my-opencode.json") + const rawConfig = createLegacyConfig() + + writeFileSync(configPath, JSON.stringify(rawConfig, null, 2) + "\n") + mkdirSync(getSidecarPath(configPath)) + + // when + const needsWrite = migrateConfigFile(configPath, rawConfig) + + // then + expect(needsWrite).toBe(true) + expect(rawConfig._migrations).toEqual([MIGRATION_KEY]) + expect((rawConfig.agents as Record>).prometheus.model).toBe( + "anthropic/claude-opus-4-6", + ) + + const persistedConfig = JSON.parse(readFileSync(configPath, "utf-8")) as Record + expect(persistedConfig._migrations).toEqual([MIGRATION_KEY]) + expect((persistedConfig.agents as Record>).prometheus.model).toBe( + "anthropic/claude-opus-4-6", + ) + expect(statSync(getSidecarPath(configPath)).isDirectory()).toBe(true) + }) +}) diff --git a/src/shared/migration/config-migration.ts b/src/shared/migration/config-migration.ts index 3174720bb..11b34156f 100644 --- a/src/shared/migration/config-migration.ts +++ b/src/shared/migration/config-migration.ts @@ -1,4 +1,4 @@ -import * as fs from "fs" +import * as fs from "node:fs" import { log } from "../logger" import { writeFileAtomically } from "../write-file-atomically" import { AGENT_NAME_MAP, migrateAgentNames } from "./agent-names" @@ -10,7 +10,7 @@ export function migrateConfigFile( configPath: string, rawConfig: Record ): boolean { - const copy = structuredClone(rawConfig) + const copy = JSON.parse(JSON.stringify(rawConfig)) as Record let needsWrite = false // Load previously applied migrations from BOTH the legacy in-config @@ -74,14 +74,11 @@ export function migrateConfigFile( // the first place. The in-memory `rawConfig` never re-exposes // `_migrations` to downstream schema validation. const newMigrationsToRecord = allNewMigrations.filter(mKey => !existingMigrations.has(mKey)) - let sidecarWriteSucceeded = false const fullMigrationSet = new Set([ ...existingMigrations, ...newMigrationsToRecord, ]) - if (newMigrationsToRecord.length > 0 || hadLegacyInConfigMigrations) { - sidecarWriteSucceeded = writeAppliedMigrations(configPath, fullMigrationSet) - } + const shouldWriteSidecar = newMigrationsToRecord.length > 0 || hadLegacyInConfigMigrations if (newMigrationsToRecord.length > 0) { needsWrite = true } @@ -89,10 +86,9 @@ export function migrateConfigFile( // Migrating state out of the config body is itself a config write. needsWrite = true } - if (sidecarWriteSucceeded && "_migrations" in copy) { - delete copy._migrations - } else if (!sidecarWriteSucceeded && newMigrationsToRecord.length > 0) { - // Sidecar write failed — persist migration tracking in-config as fallback + if (shouldWriteSidecar) { + // Keep `_migrations` in the first config write so a later sidecar failure + // does not strand the config with migrated state missing from disk. ;(copy as Record)._migrations = Array.from(fullMigrationSet) needsWrite = true } @@ -158,17 +154,32 @@ export function migrateConfigFile( } let writeSucceeded = false + let finalConfig = JSON.parse(JSON.stringify(copy)) as Record try { - writeFileAtomically(configPath, JSON.stringify(copy, null, 2) + "\n") + writeFileAtomically(configPath, JSON.stringify(finalConfig, null, 2) + "\n") writeSucceeded = true } catch (err) { log(`Failed to write migrated config to ${configPath}:`, err) } + if (writeSucceeded && shouldWriteSidecar) { + const sidecarWriteSucceeded = writeAppliedMigrations(configPath, fullMigrationSet) + if (sidecarWriteSucceeded && "_migrations" in finalConfig) { + const configWithoutLegacyMigrations = JSON.parse(JSON.stringify(finalConfig)) as Record + delete configWithoutLegacyMigrations._migrations + try { + writeFileAtomically(configPath, JSON.stringify(configWithoutLegacyMigrations, null, 2) + "\n") + finalConfig = configWithoutLegacyMigrations + } catch (err) { + log(`Failed to remove legacy _migrations fallback from ${configPath}:`, err) + } + } + } + for (const key of Object.keys(rawConfig)) { delete rawConfig[key] } - Object.assign(rawConfig, copy) + Object.assign(rawConfig, finalConfig) if (writeSucceeded) { const backupMessage = backupSucceeded ? ` (backup: ${backupPath})` : ""