From 474d660137d0652f11f6561f0101c5eb46b23a79 Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Thu, 9 Jul 2026 17:29:13 +0530 Subject: [PATCH] fix(infra): converge legacy state migrations on archive collisions --- extensions/codex/doctor-contract-api.test.ts | 62 +++++ .../src/migration/session-binding-sidecars.ts | 41 +++- ...r-config-preflight.state-migration.test.ts | 25 ++ src/commands/doctor-config-preflight.ts | 16 +- src/flows/doctor-health-contributions.test.ts | 25 ++ src/flows/doctor-health-contributions.ts | 4 + src/infra/state-migrations.debug-proxy.ts | 68 ++++-- src/infra/state-migrations.state-dir.test.ts | 86 +++++++ src/infra/state-migrations.ts | 228 ++++++++++++------ src/plugins/doctor-contract-registry.ts | 4 +- src/plugins/doctor-state-migration-fs.test.ts | 71 ++++++ src/plugins/doctor-state-migration-fs.ts | 35 ++- 12 files changed, 559 insertions(+), 106 deletions(-) create mode 100644 src/plugins/doctor-state-migration-fs.test.ts diff --git a/extensions/codex/doctor-contract-api.test.ts b/extensions/codex/doctor-contract-api.test.ts index 0bbd41bc850..a09fabfdd97 100644 --- a/extensions/codex/doctor-contract-api.test.ts +++ b/extensions/codex/doctor-contract-api.test.ts @@ -231,6 +231,68 @@ describe("codex doctor contract", () => { await fs.rm(stateDir, { recursive: true, force: true }); }); + it("reports unresolved-owner binding sidecars as notices after importing conversation binding", async () => { + const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-")); + const env = { ...process.env, OPENCLAW_STATE_DIR: stateDir }; + const sessionsDir = path.join(stateDir, "agents", "main", "sessions"); + const transcriptPath = path.join(sessionsDir, "orphan.jsonl"); + const sidecarPath = `${transcriptPath}.codex-app-server.json`; + await fs.mkdir(sessionsDir, { recursive: true }); + await fs.writeFile( + sidecarPath, + JSON.stringify({ + schemaVersion: 2, + threadId: "thread-orphan", + sessionFile: transcriptPath, + updatedAt: "2026-01-01T00:00:00.000Z", + pluginAppPolicyContext: { + fingerprint: "policy-1", + apps: {}, + pluginAppIds: {}, + }, + }), + "utf8", + ); + const params = { + config: {}, + env, + stateDir, + oauthDir: path.join(stateDir, "oauth"), + context: createDoctorContext(env), + }; + const migration = stateMigrations[0]; + if (!migration) { + throw new Error("missing Codex binding migration"); + } + + const result = await migration.migrateLegacyState(params); + const canonicalSidecarPath = await fs.realpath(sidecarPath); + + expect(result.warnings).toStrictEqual([]); + expect(result.notices).toStrictEqual([ + `Left Codex binding sidecar in place after importing its conversation binding because its session owner could not be resolved: ${canonicalSidecarPath}`, + ]); + expect(result.changes).toContain( + "Migrated 1 safe Codex app-server binding row(s) to plugin state; retained legacy sidecars needing review", + ); + await expect(fs.access(sidecarPath)).resolves.toBeUndefined(); + const store = createDoctorContext(env).openPluginStateKeyedStore({ + namespace: CODEX_APP_SERVER_BINDING_NAMESPACE, + maxEntries: CODEX_APP_SERVER_BINDING_MAX_ENTRIES, + overflowPolicy: "reject-new", + }); + await expect( + store.lookup( + bindingStoreKey({ + kind: "conversation", + bindingId: legacyCodexConversationBindingId(transcriptPath), + }), + ), + ).resolves.toMatchObject({ state: "active", binding: { threadId: "thread-orphan" } }); + + await fs.rm(stateDir, { recursive: true, force: true }); + }); + it("does not scan above stateDir when a session store sits at its parent", async () => { const outerDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-codex-doctor-outer-")); const stateDir = path.join(outerDir, "state"); diff --git a/extensions/codex/src/migration/session-binding-sidecars.ts b/extensions/codex/src/migration/session-binding-sidecars.ts index 62c23e015fd..f241d5a5d74 100644 --- a/extensions/codex/src/migration/session-binding-sidecars.ts +++ b/extensions/codex/src/migration/session-binding-sidecars.ts @@ -57,6 +57,7 @@ type SourceMigrationResult = { archived: boolean; importedKeys: number; warning?: string; + notice?: string; }; // Keep the doctor contract graph independent from the full Codex runtime. @@ -430,7 +431,7 @@ async function migrateSource( return { archived: false, importedKeys, - warning: `Left Codex binding sidecar in place after importing its conversation binding because its session owner could not be resolved: ${source.sidecarPath}`, + notice: `Left Codex binding sidecar in place after importing its conversation binding because its session owner could not be resolved: ${source.sidecarPath}`, }; } const ownershipWarning = await recordSessionOwner(owner); @@ -442,11 +443,7 @@ async function migrateSource( return retain(`canonical plugin state changed at ${entry.key}`); } } - const archivePath = `${source.sidecarPath}.migrated`; - if (await pathExists(archivePath)) { - return retain(`its archive already exists at ${archivePath}`); - } - await fs.rename(source.sidecarPath, archivePath); + await archiveBindingSidecar(source.sidecarPath); return { archived: true, importedKeys }; }); } catch (error) { @@ -527,6 +524,32 @@ async function pathExists(filePath: string): Promise { } } +async function firstFreeArchivePath(sourcePath: string): Promise { + for (let index = 2; ; index++) { + const candidate = `${sourcePath}.migrated.${index}`; + if (!(await pathExists(candidate))) { + return candidate; + } + } +} + +async function archiveBindingSidecar(sourcePath: string): Promise { + const archivePath = `${sourcePath}.migrated`; + if (await pathExists(archivePath)) { + const [sourceBytes, archiveBytes] = await Promise.all([ + fs.readFile(sourcePath), + fs.readFile(archivePath), + ]); + if (sourceBytes.equals(archiveBytes)) { + await fs.rm(sourcePath, { force: true }); + return; + } + await fs.rename(sourcePath, await firstFreeArchivePath(sourcePath)); + return; + } + await fs.rename(sourcePath, archivePath); +} + export const stateMigrations: PluginDoctorStateMigration[] = [ { id: "codex-app-server-sidecars-to-plugin-state", @@ -544,6 +567,7 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ async migrateLegacyState(params) { const changes: string[] = []; const warnings: string[] = []; + const notices: string[] = []; const { sources, surfaces } = await collectLegacyBindingSources(params); if (sources.length === 0) { return { changes, warnings }; @@ -563,6 +587,9 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ if (result.warning) { warnings.push(result.warning); } + if (result.notice) { + notices.push(result.notice); + } if (result.archived) { migrated++; } else { @@ -579,7 +606,7 @@ export const stateMigrations: PluginDoctorStateMigration[] = [ `Migrated ${partialImports} safe Codex app-server binding row(s) to plugin state; retained legacy sidecars needing review`, ); } - return { changes, warnings }; + return notices.length > 0 ? { changes, warnings, notices } : { changes, warnings }; }, }, ]; diff --git a/src/commands/doctor-config-preflight.state-migration.test.ts b/src/commands/doctor-config-preflight.state-migration.test.ts index ae5c2da363f..dbcdb7d0178 100644 --- a/src/commands/doctor-config-preflight.state-migration.test.ts +++ b/src/commands/doctor-config-preflight.state-migration.test.ts @@ -6,6 +6,7 @@ type StateMigrationResult = { skipped: boolean; changes: string[]; warnings: string[]; + notices?: string[]; }; type StartupConvergenceWarning = { @@ -294,6 +295,30 @@ describe("runDoctorConfigPreflight state migration", () => { expect(startupMigrationLeaseRelease).toHaveBeenCalledOnce(); }); + it("records the startup migration checkpoint when state migrations only leave notices", async () => { + needsStartupMigrationCheckpoint.mockReturnValue(true); + autoMigrateLegacyStateDir.mockResolvedValueOnce({ + migrated: true, + skipped: false, + changes: [], + warnings: [], + notices: ["Left reviewed residue in place."], + }); + + await runDoctorConfigPreflight({ + migrateLegacyConfig: false, + invalidConfigNote: false, + requireStartupMigrationCheckpoint: true, + }); + + expect(recordSuccessfulStartupMigrations).toHaveBeenCalledWith({ + env: process.env, + lease: startupMigrationLease, + }); + expect(note).toHaveBeenCalledWith("- Left reviewed residue in place.", "Doctor notices"); + expect(startupMigrationLeaseRelease).toHaveBeenCalledOnce(); + }); + it("does not acquire the startup migration lease when the checkpoint is current", async () => { await runDoctorConfigPreflight({ migrateLegacyConfig: false, diff --git a/src/commands/doctor-config-preflight.ts b/src/commands/doctor-config-preflight.ts index 5720bf5930b..76d242e373b 100644 --- a/src/commands/doctor-config-preflight.ts +++ b/src/commands/doctor-config-preflight.ts @@ -100,10 +100,18 @@ export function shouldSkipPluginValidationForDoctorConfigPreflight( return isTruthyEnvValue(env.OPENCLAW_UPDATE_IN_PROGRESS); } -function noteStateMigrationResult(result: { changes: string[]; warnings: string[] }): void { +function noteStateMigrationResult(result: { + changes: string[]; + warnings: string[]; + notices?: string[]; +}): void { if (result.changes.length > 0) { note(result.changes.map((entry) => `- ${entry}`).join("\n"), "Doctor changes"); } + const notices = result.notices ?? []; + if (notices.length > 0) { + note(notices.map((entry) => `- ${entry}`).join("\n"), "Doctor notices"); + } if (result.warnings.length > 0) { note(result.warnings.map((entry) => `- ${entry}`).join("\n"), "Doctor warnings"); } @@ -178,7 +186,11 @@ export async function runDoctorConfigPreflight( let startupMigrationHeartbeat: ReturnType | undefined; let startupMigrationHeartbeatError: unknown; const startupMigrationWarnings: string[] = []; - const noteStartupStateMigrationResult = (result: { changes: string[]; warnings: string[] }) => { + const noteStartupStateMigrationResult = (result: { + changes: string[]; + warnings: string[]; + notices?: string[]; + }) => { startupMigrationWarnings.push(...result.warnings); noteStateMigrationResult(result); }; diff --git a/src/flows/doctor-health-contributions.test.ts b/src/flows/doctor-health-contributions.test.ts index 140e2c0806c..ba6c997355f 100644 --- a/src/flows/doctor-health-contributions.test.ts +++ b/src/flows/doctor-health-contributions.test.ts @@ -1453,6 +1453,31 @@ describe("doctor health contributions", () => { }); }); + it("prints legacy state migration notices during manual doctor runs", async () => { + const contribution = requireDoctorContribution("doctor:legacy-state"); + const detected = { preview: ["legacy sessions"], warnings: [] }; + mocks.detectLegacyStateMigrations.mockResolvedValue(detected); + mocks.runLegacyStateMigrations.mockResolvedValue({ + changes: [], + warnings: [], + notices: ["Left reviewed legacy residue in place."], + }); + const ctx = { + cfg: {}, + sourceConfigValid: true, + prompter: buildDoctorPrompter(true), + runtime: { log: vi.fn(), error: vi.fn(), exit: vi.fn() }, + options: { nonInteractive: true }, + } as unknown as Parameters<(typeof contribution)["run"]>[0]; + + await contribution.run(ctx); + + expect(mocks.note).toHaveBeenCalledWith( + "Left reviewed legacy residue in place.", + "Doctor notices", + ); + }); + it("skips Gateway health probes for exec SecretRefs unless allow-exec is set", async () => { const contribution = requireDoctorContribution("doctor:gateway-health"); mocks.gatewaySecretInputPathCanWin.mockImplementation( diff --git a/src/flows/doctor-health-contributions.ts b/src/flows/doctor-health-contributions.ts index 25cf6914221..62db51dd2e3 100644 --- a/src/flows/doctor-health-contributions.ts +++ b/src/flows/doctor-health-contributions.ts @@ -544,6 +544,10 @@ async function runLegacyStateHealth(ctx: DoctorHealthFlowContext): Promise if (migrated.changes.length > 0) { note(migrated.changes.join("\n"), "Doctor changes"); } + const notices = migrated.notices ?? []; + if (notices.length > 0) { + note(notices.join("\n"), "Doctor notices"); + } if (migrated.warnings.length > 0) { note(migrated.warnings.join("\n"), "Doctor warnings"); } diff --git a/src/infra/state-migrations.debug-proxy.ts b/src/infra/state-migrations.debug-proxy.ts index a6aa9bbe998..7864c56fbb9 100644 --- a/src/infra/state-migrations.debug-proxy.ts +++ b/src/infra/state-migrations.debug-proxy.ts @@ -320,18 +320,27 @@ function archiveLegacyDebugProxySqlite(params: { if (existingSources.length === 0) { return; } - const existingArchives = existingSources - .map((sourcePath) => `${sourcePath}.migrated`) - .filter(fileExists); - if (existingArchives.length > 0) { - params.warnings.push( - `Left migrated debug proxy capture sidecar in place because archive already exists: ${existingArchives[0]}`, - ); - return; - } + const resolutions: Array<{ sourcePath: string; targetPath: string; removed: boolean }> = []; for (const sourcePath of existingSources) { + const archivedPath = `${sourcePath}.migrated`; try { - fs.renameSync(sourcePath, `${sourcePath}.migrated`); + if (fileExists(archivedPath)) { + if (fs.readFileSync(sourcePath).equals(fs.readFileSync(archivedPath))) { + fs.rmSync(sourcePath, { force: true }); + resolutions.push({ sourcePath, targetPath: archivedPath, removed: true }); + continue; + } + let index = 2; + while (fs.existsSync(`${sourcePath}.migrated.${index}`)) { + index++; + } + const nextArchivePath = `${sourcePath}.migrated.${index}`; + fs.renameSync(sourcePath, nextArchivePath); + resolutions.push({ sourcePath, targetPath: nextArchivePath, removed: false }); + continue; + } + fs.renameSync(sourcePath, archivedPath); + resolutions.push({ sourcePath, targetPath: archivedPath, removed: false }); } catch (err) { params.warnings.push( `Failed archiving debug proxy capture sidecar ${sourcePath}: ${String(err)}`, @@ -339,9 +348,24 @@ function archiveLegacyDebugProxySqlite(params: { return; } } - params.changes.push( - `Archived debug proxy capture sidecar legacy source → ${params.sourcePath}.migrated`, - ); + if ( + resolutions.every( + (resolution) => + !resolution.removed && resolution.targetPath === `${resolution.sourcePath}.migrated`, + ) + ) { + params.changes.push( + `Archived debug proxy capture sidecar legacy source → ${params.sourcePath}.migrated`, + ); + return; + } + for (const resolution of resolutions) { + params.changes.push( + resolution.removed + ? `Removed already-archived debug proxy capture sidecar legacy source ${resolution.sourcePath}` + : `Archived debug proxy capture sidecar legacy source → ${resolution.targetPath}`, + ); + } } function archiveLegacyDebugProxyBlobs(params: { @@ -353,15 +377,17 @@ function archiveLegacyDebugProxyBlobs(params: { return; } const archivePath = `${params.blobDir}.migrated`; - if (dirExists(archivePath)) { - params.warnings.push( - `Left migrated debug proxy capture blobs in place because archive already exists: ${archivePath}`, - ); - return; - } try { - fs.renameSync(params.blobDir, archivePath); - params.changes.push(`Archived debug proxy capture blobs → ${archivePath}`); + let targetPath = archivePath; + if (dirExists(archivePath)) { + let index = 2; + while (fs.existsSync(`${params.blobDir}.migrated.${index}`)) { + index++; + } + targetPath = `${params.blobDir}.migrated.${index}`; + } + fs.renameSync(params.blobDir, targetPath); + params.changes.push(`Archived debug proxy capture blobs → ${targetPath}`); } catch (err) { params.warnings.push( `Failed archiving debug proxy capture blobs ${params.blobDir}: ${String(err)}`, diff --git a/src/infra/state-migrations.state-dir.test.ts b/src/infra/state-migrations.state-dir.test.ts index e72dc6b1329..71638c0b47b 100644 --- a/src/infra/state-migrations.state-dir.test.ts +++ b/src/infra/state-migrations.state-dir.test.ts @@ -105,6 +105,92 @@ describe("legacy state dir auto-migration", () => { }); }); + it("removes legacy plugin install index source when the existing archive has identical bytes", async () => { + await withStateDirFixture(async (root) => { + const stateDir = path.join(root, "custom-state"); + const sourcePath = path.join(stateDir, "plugins", "installs.json"); + const archivePath = `${sourcePath}.migrated`; + const legacyJson = JSON.stringify({ + records: { + demo: { + source: "npm", + spec: "demo@1.0.0", + }, + }, + }); + fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); + fs.writeFileSync(sourcePath, legacyJson, "utf8"); + fs.writeFileSync(archivePath, legacyJson, "utf8"); + + const first = await autoMigrateLegacyStateDir({ + env: { OPENCLAW_STATE_DIR: stateDir } as NodeJS.ProcessEnv, + homedir: () => root, + }); + + expect(first.warnings).toStrictEqual([]); + expect(first.changes).toContain( + `Removed already-archived plugin install index legacy source ${sourcePath}`, + ); + expect(fs.existsSync(sourcePath)).toBe(false); + expect(fs.readFileSync(archivePath, "utf8")).toBe(legacyJson); + await expect(readPersistedInstalledPluginIndex({ stateDir })).resolves.toMatchObject({ + installRecords: { demo: { source: "npm", spec: "demo@1.0.0" } }, + }); + + resetAutoMigrateLegacyStateDirForTest(); + const second = await autoMigrateLegacyStateDir({ + env: { OPENCLAW_STATE_DIR: stateDir } as NodeJS.ProcessEnv, + homedir: () => root, + }); + expect(second.changes).toStrictEqual([]); + expect(second.warnings).toStrictEqual([]); + }); + }); + + it("renames legacy plugin install index source to the next archive when existing archive differs", async () => { + await withStateDirFixture(async (root) => { + const stateDir = path.join(root, "custom-state"); + const sourcePath = path.join(stateDir, "plugins", "installs.json"); + const archivePath = `${sourcePath}.migrated`; + const nextArchivePath = `${sourcePath}.migrated.2`; + const legacyJson = JSON.stringify({ + records: { + demo: { + source: "npm", + spec: "demo@1.0.0", + }, + }, + }); + fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); + fs.writeFileSync(sourcePath, legacyJson, "utf8"); + fs.writeFileSync(archivePath, "older archive", "utf8"); + + const first = await autoMigrateLegacyStateDir({ + env: { OPENCLAW_STATE_DIR: stateDir } as NodeJS.ProcessEnv, + homedir: () => root, + }); + + expect(first.warnings).toStrictEqual([]); + expect(first.changes).toContain( + `Archived plugin install index legacy source → ${nextArchivePath}`, + ); + expect(fs.existsSync(sourcePath)).toBe(false); + expect(fs.readFileSync(archivePath, "utf8")).toBe("older archive"); + expect(fs.readFileSync(nextArchivePath, "utf8")).toBe(legacyJson); + await expect(readPersistedInstalledPluginIndex({ stateDir })).resolves.toMatchObject({ + installRecords: { demo: { source: "npm", spec: "demo@1.0.0" } }, + }); + + resetAutoMigrateLegacyStateDirForTest(); + const second = await autoMigrateLegacyStateDir({ + env: { OPENCLAW_STATE_DIR: stateDir } as NodeJS.ProcessEnv, + homedir: () => root, + }); + expect(second.changes).toStrictEqual([]); + expect(second.warnings).toStrictEqual([]); + }); + }); + it("only runs once per process until reset", async () => { await withStateDirFixture(async (root) => { const legacyDir = path.join(root, ".clawdbot"); diff --git a/src/infra/state-migrations.ts b/src/infra/state-migrations.ts index 5d969d7a5d4..b8ffc76b6a1 100644 --- a/src/infra/state-migrations.ts +++ b/src/infra/state-migrations.ts @@ -242,6 +242,12 @@ type DetectedPluginDoctorStateMigrationPlan = { preview: string[]; }; +type MigrationMessages = { + changes: string[]; + warnings: string[]; + notices?: string[]; +}; + // Move the canonical database first so a partial archive never leaves a // readable database separated from committed WAL rows. Pending sidecars are // detected and archived without reopening the migrated database. @@ -400,6 +406,61 @@ function hasPendingSqliteSidecarArchive(sourcePath: string, suffixes: readonly s ); } +type LegacyArchiveResolution = { + sourcePath: string; + targetPath: string; + action: "archived" | "removed"; +}; + +function firstFreeArchivePath(sourcePath: string): string { + for (let index = 2; ; index++) { + const candidate = `${sourcePath}.migrated.${index}`; + if (!fs.existsSync(candidate)) { + return candidate; + } + } +} + +function archiveLegacyFileSource(params: { + sourcePath: string; + label: string; + warnings: string[]; +}): LegacyArchiveResolution | null { + const archivedPath = `${params.sourcePath}.migrated`; + try { + if (fileExists(archivedPath)) { + // Import has already committed before archival. Identical archive bytes + // preserve the same snapshot, so the leftover source can be removed. + if (fs.readFileSync(params.sourcePath).equals(fs.readFileSync(archivedPath))) { + fs.rmSync(params.sourcePath, { force: true }); + return { sourcePath: params.sourcePath, targetPath: archivedPath, action: "removed" }; + } + const nextArchivePath = firstFreeArchivePath(params.sourcePath); + fs.renameSync(params.sourcePath, nextArchivePath); + return { sourcePath: params.sourcePath, targetPath: nextArchivePath, action: "archived" }; + } + fs.renameSync(params.sourcePath, archivedPath); + return { sourcePath: params.sourcePath, targetPath: archivedPath, action: "archived" }; + } catch (err) { + params.warnings.push(`Failed archiving ${params.label} ${params.sourcePath}: ${String(err)}`); + return null; + } +} + +function recordArchiveCollisionResolutions( + changes: string[], + label: string, + resolutions: readonly LegacyArchiveResolution[], +): void { + for (const resolution of resolutions) { + changes.push( + resolution.action === "removed" + ? `Removed already-archived ${label} legacy source ${resolution.sourcePath}` + : `Archived ${label} legacy source → ${resolution.targetPath}`, + ); + } +} + function archiveLegacyPluginStateSidecar(params: { sourcePath: string; changes: string[]; @@ -411,28 +472,32 @@ function archiveLegacyPluginStateSidecar(params: { if (existingSources.length === 0) { return; } - const existingArchives = existingSources - .map((sourcePath) => `${sourcePath}.migrated`) - .filter(fileExists); - if (existingArchives.length > 0) { - params.warnings.push( - `Left migrated plugin-state sidecar in place because archive already exists: ${existingArchives[0]}`, - ); - return; - } + const resolutions: LegacyArchiveResolution[] = []; for (const sourcePath of existingSources) { - const archivedPath = `${sourcePath}.migrated`; - try { - fs.renameSync(sourcePath, archivedPath); - } catch (err) { - params.warnings.push(`Failed archiving plugin-state sidecar ${sourcePath}: ${String(err)}`); + const resolution = archiveLegacyFileSource({ + sourcePath, + label: "plugin-state sidecar", + warnings: params.warnings, + }); + if (!resolution) { return; } + resolutions.push(resolution); + } + if ( + resolutions.every( + (resolution) => + resolution.action === "archived" && + resolution.targetPath === `${resolution.sourcePath}.migrated`, + ) + ) { + params.changes.push( + `Archived plugin-state sidecar legacy source → ${params.sourcePath}.migrated`, + ); + } else { + recordArchiveCollisionResolutions(params.changes, "plugin-state sidecar", resolutions); } - params.changes.push( - `Archived plugin-state sidecar legacy source → ${params.sourcePath}.migrated`, - ); } function readLegacyInstalledPluginIndex(sourcePath: string): InstalledPluginIndex | null { @@ -639,21 +704,19 @@ function archiveLegacyInstalledPluginIndex(params: { changes: string[]; warnings: string[]; }): void { - const archivedPath = `${params.sourcePath}.migrated`; - if (fileExists(archivedPath)) { - params.warnings.push( - `Left migrated plugin install index in place because archive already exists: ${archivedPath}`, - ); + const resolution = archiveLegacyFileSource({ + sourcePath: params.sourcePath, + label: "plugin install index", + warnings: params.warnings, + }); + if (!resolution) { return; } - try { - fs.renameSync(params.sourcePath, archivedPath); - params.changes.push(`Archived plugin install index legacy source → ${archivedPath}`); - } catch (err) { - params.warnings.push( - `Failed archiving plugin install index ${params.sourcePath}: ${String(err)}`, - ); - } + params.changes.push( + resolution.action === "removed" + ? `Removed already-archived plugin install index legacy source ${params.sourcePath}` + : `Archived plugin install index legacy source → ${resolution.targetPath}`, + ); } function archiveLegacyTaskStateSidecar(params: { @@ -668,28 +731,31 @@ function archiveLegacyTaskStateSidecar(params: { if (existingSources.length === 0) { return; } - const existingArchives = existingSources - .map((sourcePath) => `${sourcePath}.migrated`) - .filter(fileExists); - if (existingArchives.length > 0) { - params.warnings.push( - `Left migrated ${params.label} sidecar in place because archive already exists: ${existingArchives[0]}`, - ); - return; - } + const resolutions: LegacyArchiveResolution[] = []; for (const sourcePath of existingSources) { - try { - fs.renameSync(sourcePath, `${sourcePath}.migrated`); - } catch (err) { - params.warnings.push( - `Failed archiving ${params.label} sidecar ${sourcePath}: ${String(err)}`, - ); + const resolution = archiveLegacyFileSource({ + sourcePath, + label: `${params.label} sidecar`, + warnings: params.warnings, + }); + if (!resolution) { return; } + resolutions.push(resolution); + } + if ( + resolutions.every( + (resolution) => + resolution.action === "archived" && + resolution.targetPath === `${resolution.sourcePath}.migrated`, + ) + ) { + params.changes.push( + `Archived ${params.label} sidecar legacy source → ${params.sourcePath}.migrated`, + ); + } else { + recordArchiveCollisionResolutions(params.changes, `${params.label} sidecar`, resolutions); } - params.changes.push( - `Archived ${params.label} sidecar legacy source → ${params.sourcePath}.migrated`, - ); } function hardenLegacyImportSource(params: { @@ -712,29 +778,31 @@ function archiveLegacyImportSource(params: { changes: string[]; warnings: string[]; }): void { - const archivedPath = `${params.sourcePath}.migrated`; - if (fileExists(archivedPath)) { - params.warnings.push( - `Left migrated ${params.label} source in place because ${archivedPath} already exists`, - ); - return; - } if (!hardenLegacyImportSource(params)) { return; } - try { - fs.renameSync(params.sourcePath, archivedPath); + const resolution = archiveLegacyFileSource({ + sourcePath: params.sourcePath, + label: `${params.label} legacy source`, + warnings: params.warnings, + }); + if (!resolution) { + return; + } + if (resolution.action === "archived") { try { - fs.chmodSync(archivedPath, 0o600); + fs.chmodSync(resolution.targetPath, 0o600); } catch (err) { params.warnings.push( `Failed securing archived ${params.label} legacy source: ${String(err)}`, ); } - params.changes.push(`Archived ${params.label} legacy source → ${archivedPath}`); - } catch (err) { - params.warnings.push(`Failed archiving ${params.label} legacy source: ${String(err)}`); } + params.changes.push( + resolution.action === "removed" + ? `Removed already-archived ${params.label} legacy source ${params.sourcePath}` + : `Archived ${params.label} legacy source → ${resolution.targetPath}`, + ); } function listSqliteColumns(db: DatabaseSync, table: string): Set { @@ -3728,6 +3796,7 @@ type StateDirMigrationResult = { skipped: boolean; changes: string[]; warnings: string[]; + notices?: string[]; }; function resolveSymlinkTarget(linkPath: string): string | null { @@ -3980,6 +4049,7 @@ export async function autoMigrateLegacyTaskStateSidecars(params: { skipped: boolean; changes: string[]; warnings: string[]; + notices?: string[]; }> { if (autoMigrateTaskStateSidecarsChecked) { return { migrated: false, skipped: true, changes: [], warnings: [] }; @@ -4725,9 +4795,10 @@ async function runPluginDoctorStateMigrationPlans(params: { detected: LegacyStateDetection; config: OpenClawConfig; env: NodeJS.ProcessEnv; -}): Promise<{ changes: string[]; warnings: string[] }> { +}): Promise { const changes: string[] = []; const warnings: string[] = []; + const notices: string[] = []; const refreshedPlans = await collectPluginDoctorStateMigrationPlans({ cfg: params.config, env: params.env, @@ -4753,11 +4824,12 @@ async function runPluginDoctorStateMigrationPlans(params: { }); changes.push(...result.changes); warnings.push(...result.warnings); + notices.push(...(result.notices ?? [])); } catch (err) { warnings.push(`Failed migrating ${plan.migration.label}: ${String(err)}`); } } - return { changes, warnings }; + return notices.length > 0 ? { changes, warnings, notices } : { changes, warnings }; } export async function autoMigrateLegacyPluginDoctorState(params: { @@ -4770,6 +4842,7 @@ export async function autoMigrateLegacyPluginDoctorState(params: { skipped: boolean; changes: string[]; warnings: string[]; + notices?: string[]; }> { const env = params.env ?? process.env; const stateDirResult = await autoMigrateLegacyStateDir({ @@ -4784,12 +4857,14 @@ export async function autoMigrateLegacyPluginDoctorState(params: { }); const changes = [...stateDirResult.changes, ...stateSchema.changes]; const warnings = [...stateDirResult.warnings, ...stateSchema.warnings]; + const notices = [...(stateDirResult.notices ?? [])]; if (stateSchema.warnings.length > 0) { return { migrated: stateDirResult.migrated || stateSchema.changes.length > 0, skipped: false, changes, warnings, + ...(notices.length > 0 ? { notices } : {}), }; } const plans = await collectPluginDoctorStateMigrationPlans({ @@ -4810,6 +4885,7 @@ export async function autoMigrateLegacyPluginDoctorState(params: { }); changes.push(...result.changes); warnings.push(...result.warnings); + notices.push(...(result.notices ?? [])); } catch (err) { warnings.push(`Failed migrating ${plan.migration.label}: ${String(err)}`); } @@ -4819,6 +4895,7 @@ export async function autoMigrateLegacyPluginDoctorState(params: { skipped: false, changes, warnings, + ...(notices.length > 0 ? { notices } : {}), }; } @@ -5026,7 +5103,7 @@ export async function runLegacyStateMigrations(params: { env?: NodeJS.ProcessEnv; now?: () => number; recoverCorruptTargetStore?: boolean; -}): Promise<{ changes: string[]; warnings: string[] }> { +}): Promise { const now = params.now ?? (() => Date.now()); const detected = params.detected; const env = params.env ?? process.env; @@ -5135,6 +5212,9 @@ export async function runLegacyStateMigrations(params: { ...agentDir.warnings, ...channelPlans.warnings, ], + ...(pluginPlans.notices && pluginPlans.notices.length > 0 + ? { notices: [...pluginPlans.notices] } + : {}), }; } @@ -5759,6 +5839,7 @@ export async function autoMigrateLegacyState(params: { skipped: boolean; changes: string[]; warnings: string[]; + notices?: string[]; }> { if (autoMigrateChecked) { return { migrated: false, skipped: true, changes: [], warnings: [] }; @@ -5805,7 +5886,7 @@ export async function autoMigrateLegacyState(params: { pluginSessionStoreAgentIds, }); - const logMigrationResults = (changes: string[], warnings: string[]) => { + const logMigrationResults = (changes: string[], warnings: string[], notices: string[]) => { const logger = params.log ?? createSubsystemLogger("state-migrations"); if (changes.length > 0) { logger.info( @@ -5817,6 +5898,11 @@ export async function autoMigrateLegacyState(params: { `Legacy state migration warnings:\n${warnings.map((entry) => `- ${entry}`).join("\n")}`, ); } + if (notices.length > 0) { + logger.info( + `Legacy state migration notes:\n${notices.map((entry) => `- ${entry}`).join("\n")}`, + ); + } }; const detected = await detectLegacyStateMigrations({ @@ -5913,7 +5999,8 @@ export async function autoMigrateLegacyState(params: { ...preSessionChannelPlans.warnings, ...pluginPlans.warnings, ]; - logMigrationResults(changes, warnings); + const notices = [...(stateDirResult.notices ?? []), ...(pluginPlans.notices ?? [])]; + logMigrationResults(changes, warnings, notices); return { migrated: stateDirResult.migrated || @@ -5936,6 +6023,7 @@ export async function autoMigrateLegacyState(params: { skipped: true, changes, warnings, + ...(notices.length > 0 ? { notices } : {}), }; } if ( @@ -5969,7 +6057,8 @@ export async function autoMigrateLegacyState(params: { ...orphanKeys.warnings, ...acpSessionMetadata.warnings, ]; - logMigrationResults(changes, warnings); + const notices = [...(stateDirResult.notices ?? [])]; + logMigrationResults(changes, warnings, notices); return { migrated: stateDirResult.migrated || @@ -5979,6 +6068,7 @@ export async function autoMigrateLegacyState(params: { skipped: false, changes, warnings, + ...(notices.length > 0 ? { notices } : {}), }; } @@ -6088,13 +6178,15 @@ export async function autoMigrateLegacyState(params: { ...agentDir.warnings, ...channelPlans.warnings, ]; + const notices = [...(stateDirResult.notices ?? []), ...(pluginPlans.notices ?? [])]; - logMigrationResults(changes, warnings); + logMigrationResults(changes, warnings, notices); return { migrated: changes.length > 0, skipped: false, changes, warnings, + ...(notices.length > 0 ? { notices } : {}), }; } diff --git a/src/plugins/doctor-contract-registry.ts b/src/plugins/doctor-contract-registry.ts index a72f2e7e9de..bc13c790310 100644 --- a/src/plugins/doctor-contract-registry.ts +++ b/src/plugins/doctor-contract-registry.ts @@ -84,8 +84,8 @@ export type PluginDoctorStateMigration = { oauthDir: string; context: PluginDoctorStateMigrationContext; }) => - | Promise<{ changes: string[]; warnings: string[] }> - | { changes: string[]; warnings: string[] }; + | Promise<{ changes: string[]; warnings: string[]; notices?: string[] }> + | { changes: string[]; warnings: string[]; notices?: string[] }; }; export type PluginDoctorStateMigrationEntry = { diff --git a/src/plugins/doctor-state-migration-fs.test.ts b/src/plugins/doctor-state-migration-fs.test.ts new file mode 100644 index 00000000000..f1255833776 --- /dev/null +++ b/src/plugins/doctor-state-migration-fs.test.ts @@ -0,0 +1,71 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { archiveLegacyStateSource } from "./doctor-state-migration-fs.js"; + +describe("archiveLegacyStateSource", () => { + let dir: string; + + beforeEach(async () => { + dir = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-doctor-fs-"))); + }); + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + it("archives a source without an existing archive", async () => { + const filePath = path.join(dir, "state.json"); + await fs.writeFile(filePath, "{}"); + const changes: string[] = []; + const warnings: string[] = []; + + await archiveLegacyStateSource({ filePath, label: "test state", changes, warnings }); + + expect(warnings).toEqual([]); + expect(changes).toEqual([`Archived test state legacy source -> ${filePath}.migrated`]); + await expect(fs.readFile(`${filePath}.migrated`, "utf8")).resolves.toBe("{}"); + }); + + it("removes the source when an identical archive already exists", async () => { + const filePath = path.join(dir, "state.json"); + await fs.writeFile(filePath, "{}"); + await fs.writeFile(`${filePath}.migrated`, "{}"); + const changes: string[] = []; + const warnings: string[] = []; + + await archiveLegacyStateSource({ filePath, label: "test state", changes, warnings }); + + expect(warnings).toEqual([]); + expect(changes).toEqual([`Removed already-archived test state legacy source ${filePath}`]); + await expect(fs.stat(filePath)).rejects.toThrow(); + }); + + it("archives under a free suffix when a differing archive already exists", async () => { + const filePath = path.join(dir, "state.json"); + await fs.writeFile(filePath, `{"newer":true}`); + await fs.writeFile(`${filePath}.migrated`, "{}"); + const changes: string[] = []; + const warnings: string[] = []; + + await archiveLegacyStateSource({ filePath, label: "test state", changes, warnings }); + + expect(warnings).toEqual([]); + expect(changes).toEqual([`Archived test state legacy source -> ${filePath}.migrated.2`]); + await expect(fs.readFile(`${filePath}.migrated.2`, "utf8")).resolves.toBe(`{"newer":true}`); + await expect(fs.readFile(`${filePath}.migrated`, "utf8")).resolves.toBe("{}"); + }); + + it("keeps a failed archive as a warning", async () => { + const filePath = path.join(dir, "missing.json"); + const changes: string[] = []; + const warnings: string[] = []; + + await archiveLegacyStateSource({ filePath, label: "test state", changes, warnings }); + + expect(changes).toEqual([]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("Failed archiving test state legacy source"); + }); +}); diff --git a/src/plugins/doctor-state-migration-fs.ts b/src/plugins/doctor-state-migration-fs.ts index c25b7b620cc..c35cb7463e5 100644 --- a/src/plugins/doctor-state-migration-fs.ts +++ b/src/plugins/doctor-state-migration-fs.ts @@ -23,16 +23,39 @@ export async function archiveLegacyStateSource(params: { warnings: string[]; }): Promise { const archivedPath = `${params.filePath}.migrated`; - if (await legacyStateFileExists(archivedPath)) { - params.warnings.push( - `Left migrated ${params.label} source in place because ${archivedPath} already exists`, - ); - return; - } try { + if (await legacyStateFileExists(archivedPath)) { + // Import commits before archival, so an existing archive must converge + // instead of re-warning every startup (#102749): identical bytes already + // preserve the snapshot; differing bytes archive under a free suffix. + const [sourceBytes, archiveBytes] = await Promise.all([ + fs.readFile(params.filePath), + fs.readFile(archivedPath), + ]); + if (sourceBytes.equals(archiveBytes)) { + await fs.rm(params.filePath, { force: true }); + params.changes.push( + `Removed already-archived ${params.label} legacy source ${params.filePath}`, + ); + return; + } + const nextArchivePath = await firstFreeArchivePath(params.filePath); + await fs.rename(params.filePath, nextArchivePath); + params.changes.push(`Archived ${params.label} legacy source -> ${nextArchivePath}`); + return; + } await fs.rename(params.filePath, archivedPath); params.changes.push(`Archived ${params.label} legacy source -> ${archivedPath}`); } catch (err) { params.warnings.push(`Failed archiving ${params.label} legacy source: ${String(err)}`); } } + +async function firstFreeArchivePath(sourcePath: string): Promise { + for (let index = 2; ; index++) { + const candidate = `${sourcePath}.migrated.${index}`; + if (!(await legacyStateFileExists(candidate))) { + return candidate; + } + } +}