From cb9a3f1cc52ca92a691d35caf6276b45ea1a37c1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 23:41:30 -0700 Subject: [PATCH] =?UTF-8?q?feat(settings):=20surface=20sweep=20for=20the?= =?UTF-8?q?=20hard-move=20=E2=80=94=20export=20v2=20with=20workflowSetting?= =?UTF-8?q?s,=20sync=20moved-key=20filtering,=20CLI=20redirect=20hints,=20?= =?UTF-8?q?consistency=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/commands/__tests__/settings.test.ts | 44 +-- packages/cli/src/commands/settings.ts | 32 +-- .../__tests__/settings-consistency.test.ts | 104 +++++++ .../src/__tests__/settings-export.test.ts | 218 ++++++++++++++- packages/core/src/central-core.ts | 19 +- packages/core/src/index.ts | 2 + packages/core/src/settings-export.ts | 256 ++++++++++++++++-- packages/core/src/store.ts | 88 ++++-- .../app/components/SettingsModal.css | 7 + .../app/components/SettingsModal.tsx | 8 + .../src/__tests__/routes-nodes-sync.test.ts | 83 ++++++ .../register-settings-sync-inbound-routes.ts | 11 +- .../routes/register-settings-sync-routes.ts | 8 +- 13 files changed, 785 insertions(+), 95 deletions(-) create mode 100644 packages/core/src/__tests__/settings-consistency.test.ts diff --git a/packages/cli/src/commands/__tests__/settings.test.ts b/packages/cli/src/commands/__tests__/settings.test.ts index b1b82659d6..de9772e093 100644 --- a/packages/cli/src/commands/__tests__/settings.test.ts +++ b/packages/cli/src/commands/__tests__/settings.test.ts @@ -85,6 +85,10 @@ describe("settings commands", () => { expect(VALID_SETTINGS).toContain("worktrunk.enabled"); expect(VALID_SETTINGS).toContain("worktrunk.binaryPath"); expect(VALID_SETTINGS).toContain("worktrunk.onFailure"); + // Moved keys are NOT settable via the CLI (they live in workflow settings). + expect(VALID_SETTINGS).not.toContain("runStepsInNewSessions"); + expect(VALID_SETTINGS).not.toContain("maxParallelSteps"); + expect(VALID_SETTINGS).not.toContain("requirePlanApproval"); expect(parseValue("ntfyEnabled", "yes")).toBe(true); expect(parseValue("maxConcurrent", "4")).toBe(4); expect(parseValue("worktreeNaming", "task-id")).toBe("task-id"); @@ -212,20 +216,21 @@ describe("settings commands", () => { expect(resolveProject).not.toHaveBeenCalled(); }); - it("runSettingsSet with project updates runStepsInNewSessions", async () => { - const updateSettings = vi.fn().mockResolvedValue(makeSettings({ runStepsInNewSessions: true })); - const getSettings = vi.fn().mockResolvedValue(makeSettings({ runStepsInNewSessions: true })); + it("rejects setting a moved key (runStepsInNewSessions) and prints the workflow-settings redirect hint", async () => { + const updateSettings = vi.fn(); vi.mocked(resolveProject).mockResolvedValue({ projectId: "proj-1", projectName: "demo-project", projectPath: "/projects/demo", isRegistered: true, - store: { updateSettings, getSettings } as any, + store: { updateSettings, getSettings: vi.fn() } as any, }); - await runSettingsSet("runStepsInNewSessions", "true", "demo-project"); + await expect(runSettingsSet("runStepsInNewSessions", "true", "demo-project")).rejects.toThrow("process.exit:1"); - expect(updateSettings).toHaveBeenCalledWith({ runStepsInNewSessions: true }); + expect(updateSettings).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalledWith('Error: Unknown setting "runStepsInNewSessions"'); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("workflow settings")); }); it("runSettingsSet with project updates worktreesDir", async () => { @@ -244,19 +249,20 @@ describe("settings commands", () => { expect(updateSettings).toHaveBeenCalledWith({ worktreesDir: "~/.fn-worktrees/{repo}" }); }); - it("runSettingsSet with project updates maxParallelSteps", async () => { const updateSettings = vi.fn().mockResolvedValue(makeSettings({ maxParallelSteps: 3 })); - const getSettings = vi.fn().mockResolvedValue(makeSettings({ maxParallelSteps: 3 })); + it("rejects setting a moved key (maxParallelSteps) — it lives in workflow settings now", async () => { + const updateSettings = vi.fn(); vi.mocked(resolveProject).mockResolvedValue({ projectId: "proj-1", projectName: "demo-project", projectPath: "/projects/demo", isRegistered: true, - store: { updateSettings, getSettings } as any, + store: { updateSettings, getSettings: vi.fn() } as any, }); - await runSettingsSet("maxParallelSteps", "3", "demo-project"); + await expect(runSettingsSet("maxParallelSteps", "3", "demo-project")).rejects.toThrow("process.exit:1"); - expect(updateSettings).toHaveBeenCalledWith({ maxParallelSteps: 3 }); + expect(updateSettings).not.toHaveBeenCalled(); + expect(errorSpy).toHaveBeenCalledWith('Error: Unknown setting "maxParallelSteps"'); }); it("runSettingsSet updates defaultNodeId and unavailableNodePolicy", async () => { @@ -277,7 +283,7 @@ describe("settings commands", () => { expect(updateSettings).toHaveBeenNthCalledWith(2, { unavailableNodePolicy: "fallback-local" }); }); - it("rejects maxParallelSteps values outside range", async () => { + it("rejects values outside range for a still-valid numeric setting (maxWorktrees)", async () => { vi.mocked(resolveProject).mockResolvedValue({ projectId: "proj-1", projectName: "demo-project", @@ -286,11 +292,11 @@ describe("settings commands", () => { store: { updateSettings: vi.fn(), getSettings: vi.fn() } as any, }); - await expect(runSettingsSet("maxParallelSteps", "5", "demo-project")).rejects.toThrow("process.exit:1"); - expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Value out of range for maxParallelSteps")); + await expect(runSettingsSet("maxWorktrees", "99", "demo-project")).rejects.toThrow("process.exit:1"); + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Value out of range for maxWorktrees")); }); - it("runSettingsShow displays Execution section with step-session settings", async () => { + it("runSettingsShow prints the workflow-settings redirect hint and no longer lists moved step settings", async () => { const getSettings = vi.fn().mockResolvedValue(makeSettings({ runStepsInNewSessions: true, maxParallelSteps: 3, @@ -306,9 +312,11 @@ describe("settings commands", () => { await runSettingsShow("demo-project"); const output = logSpy.mock.calls.map((args) => args.join(" ")).join("\n"); - expect(output).toContain("Execution"); - expect(output).toContain("Run Steps In New Sessions"); - expect(output).toContain("Max Parallel Steps"); + // Moved step settings are no longer listed; the redirect hint points users + // to workflow settings. + expect(output).not.toContain("Run Steps In New Sessions"); + expect(output).not.toContain("Max Parallel Steps"); + expect(output).toContain("workflow settings"); }); it("rejects enabling worktrunk when binary is not verified", async () => { diff --git a/packages/cli/src/commands/settings.ts b/packages/cli/src/commands/settings.ts index 84fea3e418..4a1c421cb1 100644 --- a/packages/cli/src/commands/settings.ts +++ b/packages/cli/src/commands/settings.ts @@ -9,7 +9,12 @@ import { import { probeWorktrunk, resolveWorktrunkBinary } from "@fusion/engine"; import { resolveProject } from "../project-context.js"; -// Settings that can be updated via CLI +// Settings that can be updated via CLI. +// +// NOTE: the step/review/model-lane policy keys (`runStepsInNewSessions`, +// `maxParallelSteps`, `requirePlanApproval`, etc.) were MOVED to workflow settings +// (U4/KTD-5) and are intentionally ABSENT here — they live as per-workflow values, +// not project/global settings. See WORKFLOW_SETTINGS_REDIRECT_HINT below. export const VALID_SETTINGS = [ "maxConcurrent", "maxWorktrees", @@ -19,11 +24,8 @@ export const VALID_SETTINGS = [ "ntfyTopic", "autoResolveConflicts", "smartConflictResolution", - "requirePlanApproval", "ntfyEnabled", "defaultModel", - "runStepsInNewSessions", - "maxParallelSteps", "defaultNodeId", "unavailableNodePolicy", "worktrunk.enabled", @@ -32,6 +34,11 @@ export const VALID_SETTINGS = [ "language", ] as const; +// One-line redirect surfaced wherever the CLI lists/validates settings keys, so +// users who reach for a moved key learn where it lives now (U5/KTD-8). +export const WORKFLOW_SETTINGS_REDIRECT_HINT = + "Note: step, review, and model-lane policy now live in workflow settings — edit them in the workflow editor or via fn_workflow_settings."; + const GLOBAL_ONLY_SETTINGS = ["ntfyEnabled", "ntfyTopic", "defaultModel", "language"] as const; const PROJECT_ONLY_SETTINGS = [ "maxConcurrent", @@ -41,9 +48,6 @@ const PROJECT_ONLY_SETTINGS = [ "taskPrefix", "autoResolveConflicts", "smartConflictResolution", - "requirePlanApproval", - "runStepsInNewSessions", - "maxParallelSteps", "defaultNodeId", "unavailableNodePolicy", ] as const; @@ -54,13 +58,11 @@ type ValidSettingKey = (typeof VALID_SETTINGS)[number]; const BOOLEAN_SETTINGS: readonly string[] = [ "autoResolveConflicts", "smartConflictResolution", - "requirePlanApproval", "ntfyEnabled", - "runStepsInNewSessions", "worktrunk.enabled", ]; -const NUMBER_SETTINGS: readonly string[] = ["maxConcurrent", "maxWorktrees", "maxParallelSteps"]; +const NUMBER_SETTINGS: readonly string[] = ["maxConcurrent", "maxWorktrees"]; const ENUM_SETTINGS: Record = { worktreeNaming: ["random", "task-id", "task-title"], @@ -83,7 +85,6 @@ const STRING_SETTINGS: readonly string[] = [ const NUMBER_RANGES: Record = { maxConcurrent: { min: 1, max: 10 }, maxWorktrees: { min: 1, max: 20 }, - maxParallelSteps: { min: 1, max: 4 }, }; async function getGlobalSettingsStore(): Promise { @@ -256,10 +257,6 @@ export async function runSettingsShow(projectName?: string): Promise { title: "Engine", keys: ["maxConcurrent", "maxWorktrees", "autoResolveConflicts", "smartConflictResolution"], }, - { - title: "Execution", - keys: ["runStepsInNewSessions", "maxParallelSteps"], - }, { title: "Worktrees", keys: ["worktreeNaming", "worktreesDir", "recycleWorktrees"], @@ -270,7 +267,7 @@ export async function runSettingsShow(projectName?: string): Promise { }, { title: "Tasks", - keys: ["taskPrefix", "requirePlanApproval", "includeTaskIdInCommit"], + keys: ["taskPrefix", "includeTaskIdInCommit"], }, { title: "Node Routing", @@ -302,6 +299,8 @@ export async function runSettingsShow(projectName?: string): Promise { } console.log(); + console.log(` ${WORKFLOW_SETTINGS_REDIRECT_HINT}`); + console.log(); } /** @@ -315,6 +314,7 @@ export async function runSettingsSet(key: string, value: string, projectName?: s if (!VALID_SETTINGS.includes(key as ValidSettingKey)) { console.error(`Error: Unknown setting "${key}"`); console.error(`Valid settings: ${VALID_SETTINGS.join(", ")}`); + console.error(WORKFLOW_SETTINGS_REDIRECT_HINT); process.exit(1); return; } diff --git a/packages/core/src/__tests__/settings-consistency.test.ts b/packages/core/src/__tests__/settings-consistency.test.ts new file mode 100644 index 0000000000..7c32d2196b --- /dev/null +++ b/packages/core/src/__tests__/settings-consistency.test.ts @@ -0,0 +1,104 @@ +/** + * U5 — Permanent settings-regime consistency guard (registration-drift lesson). + * + * Every settings key must live in EXACTLY ONE regime: either a project/global + * SCHEMA key, or a MOVED (tombstoned) workflow-setting key. This test fails fast + * if the schema key lists, the tombstone list, and the built-in workflow setting + * declarations ever drift apart — the exact class of bug the U4/U5 work exists to + * prevent (a moved key re-materializing in project settings, or a tombstone with + * no backing declaration). + */ +import { describe, it, expect } from "vitest"; +import { MOVED_SETTINGS_KEYS } from "../moved-settings.js"; +import { BUILTIN_WORKFLOW_SETTINGS } from "../builtin-workflow-settings.js"; +import { + DEFAULT_GLOBAL_SETTINGS, + DEFAULT_PROJECT_SETTINGS, + GLOBAL_SETTINGS_KEYS, + PROJECT_SETTINGS_KEYS, + isGlobalSettingsKey, + isProjectSettingsKey, +} from "../settings-schema.js"; +import { + SETTINGS_EXPORT_VERSION, + exportSettings, +} from "../settings-export.js"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const movedKeys = MOVED_SETTINGS_KEYS as readonly string[]; + +describe("settings consistency (U5)", () => { + it("(a) no moved key is also a DEFAULT_PROJECT_SETTINGS or DEFAULT_GLOBAL_SETTINGS key", () => { + const projectDefaultKeys = Object.keys(DEFAULT_PROJECT_SETTINGS); + const globalDefaultKeys = Object.keys(DEFAULT_GLOBAL_SETTINGS); + for (const key of movedKeys) { + expect(projectDefaultKeys, `moved key '${key}' must not be in DEFAULT_PROJECT_SETTINGS`).not.toContain(key); + expect(globalDefaultKeys, `moved key '${key}' must not be in DEFAULT_GLOBAL_SETTINGS`).not.toContain(key); + } + }); + + it("(b) MOVED_SETTINGS_KEYS and BUILTIN_WORKFLOW_SETTINGS declaration ids are exactly equal sets", () => { + const declIds = new Set(BUILTIN_WORKFLOW_SETTINGS.map((s) => s.id)); + const moved = new Set(movedKeys); + // Every moved key has a declaration. + for (const key of moved) { + expect(declIds.has(key), `moved key '${key}' has no BUILTIN_WORKFLOW_SETTINGS declaration`).toBe(true); + } + // Every declaration is a moved key. + for (const id of declIds) { + expect(moved.has(id), `declaration '${id}' is missing from MOVED_SETTINGS_KEYS`).toBe(true); + } + expect(moved.size).toBe(declIds.size); + }); + + it("(c) every moved key is absent from GLOBAL_SETTINGS_KEYS / PROJECT_SETTINGS_KEYS and their predicates", () => { + const globalKeys = GLOBAL_SETTINGS_KEYS as readonly string[]; + const projectKeys = PROJECT_SETTINGS_KEYS as readonly string[]; + for (const key of movedKeys) { + expect(globalKeys, `moved key '${key}' must not be in GLOBAL_SETTINGS_KEYS`).not.toContain(key); + expect(projectKeys, `moved key '${key}' must not be in PROJECT_SETTINGS_KEYS`).not.toContain(key); + expect(isGlobalSettingsKey(key), `isGlobalSettingsKey('${key}') must be false`).toBe(false); + expect(isProjectSettingsKey(key), `isProjectSettingsKey('${key}') must be false`).toBe(false); + } + }); + + it("(d) settings-export v2 global/project section keys never overlap moved keys", async () => { + expect(SETTINGS_EXPORT_VERSION).toBe(2); + + const tempDir = mkdtempSync(join(tmpdir(), "fn-settings-consistency-")); + const fusionDir = join(tempDir, ".fusion"); + const globalSettingsDir = join(tempDir, "global-settings"); + mkdirSync(join(fusionDir, "tasks"), { recursive: true }); + mkdirSync(globalSettingsDir, { recursive: true }); + writeFileSync(join(fusionDir, "config.json"), JSON.stringify({ nextId: 1, settings: {} })); + writeFileSync(join(globalSettingsDir, "settings.json"), JSON.stringify({})); + + const { TaskStore } = await import("../store.js"); + const store = new TaskStore(tempDir, globalSettingsDir, { inMemoryDb: true }); + await store.init(); + try { + // Even with a moved key written as a workflow value, it must surface ONLY in + // the workflowSettings section, never under global/project. + await store.updateWorkflowSettingValues( + "builtin:coding", + store.getWorkflowSettingsProjectId(), + { requirePrApproval: true }, + ); + const exported = await exportSettings(store, { scope: "both" }); + + const globalSectionKeys = Object.keys(exported.global ?? {}); + const projectSectionKeys = Object.keys(exported.project ?? {}); + for (const key of movedKeys) { + expect(globalSectionKeys, `moved key '${key}' must not appear in export global section`).not.toContain(key); + expect(projectSectionKeys, `moved key '${key}' must not appear in export project section`).not.toContain(key); + } + // It IS present in the workflowSettings section. + expect(exported.workflowSettings?.["builtin:coding"]?.requirePrApproval).toBe(true); + } finally { + store.close(); + rmSync(tempDir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/core/src/__tests__/settings-export.test.ts b/packages/core/src/__tests__/settings-export.test.ts index 67825b37bb..365227ccac 100644 --- a/packages/core/src/__tests__/settings-export.test.ts +++ b/packages/core/src/__tests__/settings-export.test.ts @@ -139,14 +139,23 @@ describe("settings-export", () => { ]); }); - it("should return error for wrong version", () => { + it("should accept v2 data", () => { const data = { version: 2, exportedAt: new Date().toISOString(), global: {}, }; + expect(validateImportData(data)).toEqual([]); + }); + + it("should return error for wrong version", () => { + const data = { + version: 3, + exportedAt: new Date().toISOString(), + global: {}, + }; expect(validateImportData(data)).toContain( - "Unsupported export version: 2. Expected: 1" + "Unsupported export version: 3. Expected: 1 or 2" ); }); @@ -166,7 +175,7 @@ describe("settings-export", () => { exportedAt: new Date().toISOString(), }; expect(validateImportData(data)).toContain( - "Export data must contain at least one of 'global' or 'project' settings" + "Export data must contain at least one of 'global', 'project', or 'workflowSettings' settings" ); }); @@ -201,7 +210,7 @@ describe("settings-export", () => { const result = await exportSettings(store); - expect(result.version).toBe(1); + expect(result.version).toBe(2); expect(result.exportedAt).toBeDefined(); expect(result.global).toBeDefined(); expect(result.global?.themeMode).toBe("dark"); @@ -365,7 +374,7 @@ describe("settings-export", () => { it("should fail with validation errors for invalid data", async () => { const importData = { - version: 2, + version: 3, exportedAt: new Date().toISOString(), global: {}, } as unknown as SettingsExportData; @@ -373,7 +382,7 @@ describe("settings-export", () => { const result = await importSettings(store, importData); expect(result.success).toBe(false); - expect(result.error).toContain("Unsupported export version: 2"); + expect(result.error).toContain("Unsupported export version: 3"); }); it("should handle import errors gracefully", async () => { @@ -513,6 +522,203 @@ describe("settings-export", () => { }); }); + // ── U5: workflow settings (v2) export/import + v1 upgrade (KTD-8) ────────── + describe("workflow settings export/import (U5/KTD-8)", () => { + function rawDb(s: TaskStore): { + prepare: (sql: string) => { run: (...a: unknown[]) => unknown }; + } { + return (s as unknown as { db: { prepare: (sql: string) => { run: (...a: unknown[]) => unknown } } }).db; + } + + it("export post-migration carries workflow setting values; no moved key under project", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + // A normal unrelated project key + a workflow setting value on builtin:coding. + await store.updateSettings({ maxConcurrent: 3 }); + await store.updateWorkflowSettingValues("builtin:coding", projectId, { + workflowStepTimeoutMs: 120_000, + requirePrApproval: true, + }); + + const result = await exportSettings(store, { scope: "project" }); + + expect(result.version).toBe(2); + // Project section: the unrelated key survives, NO moved key present. + expect(result.project?.maxConcurrent).toBe(3); + expect((result.project as Record)?.workflowStepTimeoutMs).toBeUndefined(); + expect((result.project as Record)?.requirePrApproval).toBeUndefined(); + // workflowSettings section carries the value-table row. + expect(result.workflowSettings?.["builtin:coding"]).toEqual({ + workflowStepTimeoutMs: 120_000, + requirePrApproval: true, + }); + }); + + it("import v1 payload containing workflowStepTimeoutMs → value lands per target rule, not project settings", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + const importData = { + version: 1 as const, + exportedAt: new Date().toISOString(), + project: { + // unrelated key — imports normally + maxConcurrent: 5, + // moved key — must be UPGRADED into workflow setting values + workflowStepTimeoutMs: 90_000, + } as Record, + }; + + const result = await importSettings(store, importData as unknown as SettingsExportData, { + scope: "project", + merge: true, + }); + + expect(result.success).toBe(true); + expect(result.projectCount).toBe(1); // only maxConcurrent + expect(result.workflowSettingsCount).toBeGreaterThanOrEqual(1); + + // Project settings: moved key never written into raw project settings. + const settings = await store.getSettings(); + expect(settings.maxConcurrent).toBe(5); + const db = (store as unknown as { db: { prepare: (s: string) => { get: (...a: unknown[]) => unknown } } }).db; + const rawProject = JSON.parse( + (db.prepare("SELECT settings FROM config WHERE id = 1").get() as { settings: string }).settings, + ) as Record; + expect(rawProject.workflowStepTimeoutMs).toBeUndefined(); + + // Value landed on the resolved default workflow (builtin:coding, unset default). + expect(store.getWorkflowSettingValues("builtin:coding", projectId).workflowStepTimeoutMs).toBe(90_000); + }); + + it("import v1 upgrade targets every in-use selection workflow ∪ default", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + // Seed an in-use selection on a builtin workflow distinct from the default. + rawDb(store) + .prepare( + `INSERT INTO task_workflow_selection (taskId, workflowId, stepIds, updatedAt) + VALUES (?, ?, '[]', ?) + ON CONFLICT(taskId) DO UPDATE SET workflowId = excluded.workflowId`, + ) + .run("task-1", "builtin:quick-fix", new Date().toISOString()); + + const importData = { + version: 1 as const, + exportedAt: new Date().toISOString(), + project: { requirePrApproval: true } as Record, + }; + + await importSettings(store, importData as unknown as SettingsExportData, { scope: "project" }); + + // Both the in-use selection workflow and the default lane received the value. + expect(store.getWorkflowSettingValues("builtin:quick-fix", projectId).requirePrApproval).toBe(true); + expect(store.getWorkflowSettingValues("builtin:coding", projectId).requirePrApproval).toBe(true); + }); + + it("import v2 round-trips workflow setting values", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + const importData: SettingsExportData = { + version: 2, + exportedAt: new Date().toISOString(), + workflowSettings: { + "builtin:coding": { workflowStepTimeoutMs: 45_000, requirePrApproval: true }, + }, + }; + + const result = await importSettings(store, importData, { scope: "project", merge: true }); + + expect(result.success).toBe(true); + expect(result.workflowSettingsCount).toBe(2); + expect(store.getWorkflowSettingValues("builtin:coding", projectId)).toEqual({ + workflowStepTimeoutMs: 45_000, + requirePrApproval: true, + }); + }); + + it("import v2 drops-and-logs invalid values without aborting", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + const importData: SettingsExportData = { + version: 2, + exportedAt: new Date().toISOString(), + workflowSettings: { + // workflowStepTimeoutMs expects a number; the bad string is dropped, the + // valid requirePrApproval still lands. + "builtin:coding": { + workflowStepTimeoutMs: "not-a-number" as unknown as number, + requirePrApproval: true, + }, + }, + }; + + const result = await importSettings(store, importData, { scope: "project", merge: true }); + + expect(result.success).toBe(true); + const stored = store.getWorkflowSettingValues("builtin:coding", projectId); + expect(stored.workflowStepTimeoutMs).toBeUndefined(); + expect(stored.requirePrApproval).toBe(true); + }); + + it("merge mode merges into existing rows; replace mode replaces the workflow's row", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + await store.updateWorkflowSettingValues("builtin:coding", projectId, { + workflowStepTimeoutMs: 10_000, + requirePrApproval: true, + }); + + // merge: only requirePrApproval changes; the timeout survives. + await importSettings( + store, + { + version: 2, + exportedAt: new Date().toISOString(), + workflowSettings: { "builtin:coding": { requirePrApproval: false } }, + }, + { scope: "project", merge: true }, + ); + expect(store.getWorkflowSettingValues("builtin:coding", projectId)).toEqual({ + workflowStepTimeoutMs: 10_000, + requirePrApproval: false, + }); + + // replace: the row becomes exactly the imported values (timeout dropped). + await importSettings( + store, + { + version: 2, + exportedAt: new Date().toISOString(), + workflowSettings: { "builtin:coding": { requirePrApproval: true } }, + }, + { scope: "project", merge: false }, + ); + expect(store.getWorkflowSettingValues("builtin:coding", projectId)).toEqual({ + requirePrApproval: true, + }); + }); + + it("export → import round-trips the full payload", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + await store.updateSettings({ maxConcurrent: 4 }); + await store.updateWorkflowSettingValues("builtin:coding", projectId, { + workflowStepTimeoutMs: 77_000, + }); + + const exported = await exportSettings(store, { scope: "project" }); + + // Fresh store, import the exported payload. + const env2 = createTestEnv(); + const { TaskStore: TS } = await import("../store.js"); + const store2 = new TS(env2.tempDir, env2.globalSettingsDir, { inMemoryDb: true }); + await store2.init(); + try { + const r = await importSettings(store2, exported, { scope: "project", merge: true }); + expect(r.success).toBe(true); + const settings2 = await store2.getSettings(); + expect(settings2.maxConcurrent).toBe(4); + expect(store2.getWorkflowSettingValues("builtin:coding", store2.getWorkflowSettingsProjectId()).workflowStepTimeoutMs).toBe(77_000); + } finally { + store2.close(); + cleanupTestEnv(env2.tempDir); + } + }); + }); + describe("readExportFile", () => { it("should read and parse valid export file", async () => { const filePath = join(env.tempDir, "test-export.json"); diff --git a/packages/core/src/central-core.ts b/packages/core/src/central-core.ts index 3eb8391230..866862a0dc 100644 --- a/packages/core/src/central-core.ts +++ b/packages/core/src/central-core.ts @@ -83,6 +83,7 @@ import { getAppVersion, parseSemver } from "./app-version.js"; import { validateDockerNodeConfig } from "./types.js"; import { CentralDatabase, toJson, toJsonNullable, fromJson } from "./central-db.js"; import { resolveGlobalDir } from "./global-settings.js"; +import { stripMovedSettingsKeys } from "./moved-settings.js"; import { NodeConnection } from "./node-connection.js"; import { NodeDiscovery } from "./node-discovery.js"; import { collectSystemMetrics } from "./system-metrics.js"; @@ -3659,12 +3660,16 @@ export class CentralCore extends EventEmitter { let projectCount = 0; const authCount = payload.providerAuth ? Object.keys(payload.providerAuth).length : 0; - // Apply global settings (shallow merge, local-wins) + // Apply global settings (shallow merge, local-wins). + // Moved (tombstoned) keys are dropped here as a second line of defense — a + // mid-migration peer must never resurrect a moved key cross-node (KTD-8). The + // count reflects only the keys that survive the strip. if (payload.global) { // The actual application of global settings is handled by the caller (dashboard route) // since CentralCore doesn't have access to GlobalSettingsStore. // We simply count the number of global settings entries for reporting. - globalCount = Object.keys(payload.global).length; + const cleanGlobal = stripMovedSettingsKeys(payload.global as Record); + globalCount = Object.keys(cleanGlobal).length; } // Apply project settings (match by name, local-wins merge) @@ -3675,11 +3680,17 @@ export class CentralCore extends EventEmitter { for (const [projectName, remoteSettings] of Object.entries(payload.projects)) { const localProject = projectsByName.get(projectName); if (localProject) { + // Strip moved keys from the inbound remote settings before merging — + // defense beyond the store guard so they can never be persisted into a + // project's raw config via the cross-node path (KTD-8). + const cleanRemote = stripMovedSettingsKeys( + (remoteSettings ?? {}) as unknown as Record, + ) as Partial; // Merge settings: local values take precedence const mergedSettings: ProjectSettings = { - ...remoteSettings, + ...cleanRemote, ...localProject.settings, - }; + } as ProjectSettings; await this.updateProject(localProject.id, { settings: mergedSettings }); projectCount++; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2a36abb1f9..93a6a7e6a1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -843,12 +843,14 @@ export { generateExportFilename, readExportFile, writeExportFile, + SETTINGS_EXPORT_VERSION, } from "./settings-export.js"; export type { SettingsExportData, ExportSettingsOptions, ImportSettingsOptions, ImportResult, + WorkflowSettingsExportSection, } from "./settings-export.js"; // ── AI Summarization ───────────────────────────────────────────────────── diff --git a/packages/core/src/settings-export.ts b/packages/core/src/settings-export.ts index 5e205b5946..9ac9eebdd0 100644 --- a/packages/core/src/settings-export.ts +++ b/packages/core/src/settings-export.ts @@ -4,19 +4,44 @@ * This module provides utilities for exporting and importing fn settings, * supporting both global (~/.fusion/settings.json) and project-level (.fusion/config.json) * settings for backup, migration, and sharing. + * + * ── Export format versions ──────────────────────────────────────────────────── + * - v1: `{ version: 1, global?, project? }` — the legacy shape. Project settings + * could carry the (now-moved) workflow/step/model-lane keys flat under + * `project`. Still importable: any moved key found in a v1 `project` section is + * UPGRADED into workflow setting VALUES (KTD-8) using the same write-target + * rule as the U4 migration, instead of dead-writing it back into project + * settings (the store guard would strip it anyway). + * - v2: adds a `workflowSettings` section carrying the per-project value table + * (`workflowId → { key: value }`). Moved keys never appear under `project` in a + * v2 export. Import round-trips the section via `updateWorkflowSettingValues`, + * dropping-and-logging invalid values without aborting. */ import { writeFile, readFile, rename } from "node:fs/promises"; import type { Settings, GlobalSettings, ProjectSettings } from "./types.js"; import { TaskStore } from "./store.js"; +import { + MOVED_SETTINGS_KEYS, + stripMovedSettingsKeys, +} from "./moved-settings.js"; + +/** Current export format version emitted by {@link exportSettings}. */ +export const SETTINGS_EXPORT_VERSION = 2; + +/** + * Per-project workflow setting VALUE table carried by a v2 export: + * `workflowId → { settingKey: value }`. + */ +export type WorkflowSettingsExportSection = Record>; /** * Structure for exported settings JSON. * Contains metadata about the export and the actual settings data. */ export interface SettingsExportData { - /** Export format version for future compatibility */ - version: 1; + /** Export format version. 2 is current; 1 remains importable. */ + version: 1 | 2; /** Timestamp when the export was created */ exportedAt: string; /** Source identifier (e.g., hostname, project path) */ @@ -25,6 +50,11 @@ export interface SettingsExportData { global?: GlobalSettings; /** Project settings (project-level, .fusion/config.json) */ project?: Partial; + /** + * Workflow setting VALUES for the exporting project (v2+). Keyed + * `workflowId → { settingKey: value }`. Absent in v1 payloads. + */ + workflowSettings?: WorkflowSettingsExportSection; } /** @@ -57,6 +87,8 @@ export interface ImportResult { globalCount: number; /** Number of project settings imported */ projectCount: number; + /** Number of workflow setting VALUES imported (across all workflows). */ + workflowSettingsCount: number; /** Error message if import failed */ error?: string; } @@ -64,6 +96,7 @@ export interface ImportResult { /** * Validate that data conforms to the SettingsExportData structure. * Returns validation errors as an array of strings, or empty array if valid. + * Both v1 and v2 are accepted. */ export function validateImportData(data: unknown): string[] { const errors: string[] = []; @@ -75,9 +108,9 @@ export function validateImportData(data: unknown): string[] { const obj = data as Record; - // Check version - if (obj.version !== 1) { - errors.push(`Unsupported export version: ${obj.version}. Expected: 1`); + // Check version (v1 and v2 are both supported) + if (obj.version !== 1 && obj.version !== 2) { + errors.push(`Unsupported export version: ${obj.version}. Expected: 1 or 2`); } // Check exportedAt @@ -99,9 +132,26 @@ export function validateImportData(data: unknown): string[] { } } - // At least one of global or project must be present - if (obj.global === undefined && obj.project === undefined) { - errors.push("Export data must contain at least one of 'global' or 'project' settings"); + // Validate workflowSettings section if present (v2) + if (obj.workflowSettings !== undefined) { + if ( + typeof obj.workflowSettings !== "object" + || obj.workflowSettings === null + || Array.isArray(obj.workflowSettings) + ) { + errors.push("'workflowSettings' field must be an object if provided"); + } else { + for (const [workflowId, values] of Object.entries(obj.workflowSettings as Record)) { + if (typeof values !== "object" || values === null || Array.isArray(values)) { + errors.push(`'workflowSettings.${workflowId}' must be an object of setting values`); + } + } + } + } + + // At least one of global, project, or workflowSettings must be present + if (obj.global === undefined && obj.project === undefined && obj.workflowSettings === undefined) { + errors.push("Export data must contain at least one of 'global', 'project', or 'workflowSettings' settings"); } return errors; @@ -124,7 +174,9 @@ export function generateExportFilename(date: Date = new Date()): string { /** * Export settings from the current project. * - * Reads both global and project settings and returns them in an exportable structure. + * Reads both global and project settings and returns them in an exportable + * structure. When project scope is requested, the per-project workflow setting + * value table is carried under `workflowSettings` (v2). * * @param store - The TaskStore instance for accessing project settings * @param options - Export options including scope selection @@ -137,7 +189,7 @@ export async function exportSettings( const { scope = "both", source } = options; const result: SettingsExportData = { - version: 1, + version: SETTINGS_EXPORT_VERSION, exportedAt: new Date().toISOString(), source, }; @@ -152,15 +204,159 @@ export async function exportSettings( if (scope === "project" || scope === "both") { const scopes = await store.getSettingsByScope(); result.project = scopes.project; + + // Carry the per-project workflow setting value table (v2). Defensively strip + // any moved key that somehow lingered in the project section (post-migration + // it never should) so the two regimes can never both claim the same key. + if (result.project) { + result.project = stripMovedSettingsKeys( + result.project as Record, + ) as Partial; + } + + const workflowSettings = store.listWorkflowSettingValuesForProject(); + // Only attach non-empty rows; an empty table omits the section entirely. + const nonEmpty: WorkflowSettingsExportSection = {}; + for (const [workflowId, values] of Object.entries(workflowSettings)) { + if (values && Object.keys(values).length > 0) { + nonEmpty[workflowId] = values; + } + } + if (Object.keys(nonEmpty).length > 0) { + result.workflowSettings = nonEmpty; + } } return result; } +/** + * Apply the `workflowSettings` value section (v2) into the store. + * + * Each `(workflowId, values)` pair is written via `store.updateWorkflowSettingValues`. + * Invalid values are dropped-and-logged per-key (the write never aborts the whole + * import): we pre-validate by attempting the write and, on rejection, retry with + * the offending keys removed. Returns the number of values successfully applied. + * + * Merge semantics: + * - merge=true → per-key merge into the existing row (store's default upsert). + * - merge=false → replace the exported workflow's row: delete keys present in the + * current row but absent from the import, then write the import values. + */ +async function applyWorkflowSettingsSection( + store: TaskStore, + section: WorkflowSettingsExportSection, + merge: boolean, +): Promise { + const projectId = store.getWorkflowSettingsProjectId(); + let applied = 0; + + for (const [workflowId, rawValues] of Object.entries(section)) { + if (!rawValues || typeof rawValues !== "object" || Array.isArray(rawValues)) continue; + let patch: Record = { ...(rawValues as Record) }; + + if (!merge) { + // Replace mode: null out keys present in the current row but absent here so + // the row ends up matching the imported workflow exactly. + const current = store.getWorkflowSettingValues(workflowId, projectId); + for (const key of Object.keys(current)) { + if (!(key in patch)) { + patch[key] = null; // null-as-delete + } + } + } + + // Attempt the write; on a validation rejection, drop the offending keys and + // retry so one bad value never blocks the rest. Never abort the import. + // Retry at most until the patch is empty. + while (Object.keys(patch).length > 0) { + try { + await store.updateWorkflowSettingValues(workflowId, projectId, patch); + // Count only the non-null (set) keys as applied values. + applied += Object.values(patch).filter((v) => v !== null).length; + break; + } catch (err) { + const rejectedIds = extractRejectedSettingIds(err); + if (rejectedIds.length === 0) { + // Unknown error (not a value-rejection) — log and skip this workflow. + // eslint-disable-next-line no-console + console.warn( + `[settings-import] skipped workflow setting values for '${workflowId}': ${ + err instanceof Error ? err.message : String(err) + }`, + ); + break; + } + for (const id of rejectedIds) { + delete patch[id]; + // eslint-disable-next-line no-console + console.warn( + `[settings-import] dropped invalid workflow setting value '${id}' for workflow '${workflowId}'`, + ); + } + } + } + } + + return applied; +} + +/** + * Extract rejected setting ids from a {@link WorkflowSettingRejectionError}-shaped + * error without importing the class (avoids a hard dependency cycle). Returns an + * empty array for errors that don't carry per-key rejections. + */ +function extractRejectedSettingIds(err: unknown): string[] { + if (!err || typeof err !== "object") return []; + const rejections = (err as { rejections?: unknown }).rejections; + if (!Array.isArray(rejections)) return []; + const ids: string[] = []; + for (const r of rejections) { + if (r && typeof r === "object" && typeof (r as { settingId?: unknown }).settingId === "string") { + ids.push((r as { settingId: string }).settingId); + } + } + return ids; +} + +/** + * Upgrade moved keys found in a v1 payload's `project` section into workflow + * setting VALUES (KTD-8). The moved keys are written to every target workflow + * (in-use selection workflows ∪ resolved default, unset → `builtin:coding`), + * mirroring the U4 migration. Invalid values are dropped-and-logged. Returns the + * total count of values applied across all target workflows. + */ +async function upgradeMovedKeysFromV1Project( + store: TaskStore, + projectSection: Record, +): Promise { + const movedSnapshot: Record = {}; + for (const key of MOVED_SETTINGS_KEYS) { + if ( + Object.prototype.hasOwnProperty.call(projectSection, key) + && projectSection[key] !== undefined + ) { + movedSnapshot[key] = projectSection[key]; + } + } + if (Object.keys(movedSnapshot).length === 0) return 0; + + const targets = await store.computeMovedSettingsTargetWorkflowIds(); + const section: WorkflowSettingsExportSection = {}; + for (const workflowId of targets) { + section[workflowId] = { ...movedSnapshot }; + } + // Always merge moved-key upgrades into existing rows (never replace) — they are + // an overlay onto whatever the workflow already has. + return applyWorkflowSettingsSection(store, section, true); +} + /** * Import settings into the current project. * - * Validates the import data and applies it to global and/or project settings. + * Validates the import data and applies it to global, project, and (v2) workflow + * setting values. v1 payloads whose `project` section carries moved keys upgrade + * those keys into workflow setting values instead of dead-writing them. * * @param store - The TaskStore instance for writing settings * @param data - The settings data to import @@ -181,20 +377,22 @@ export async function importSettings( success: false, globalCount: 0, projectCount: 0, + workflowSettingsCount: 0, error: validationErrors.join("; "), }; } let globalCount = 0; let projectCount = 0; + let workflowSettingsCount = 0; try { - // Import global settings if present and requested + // Import global settings if present and requested. + // (The store guard strips any moved key arriving here, so global is safe.) if ((scope === "global" || scope === "both") && data.global) { const globalSettings = data.global as GlobalSettings; if (merge) { - // Merge mode: only import defined fields, keeping existing values for undefined ones const definedEntries = Object.entries(globalSettings).filter( ([, value]) => value !== undefined ); @@ -204,9 +402,6 @@ export async function importSettings( globalCount = definedEntries.length; } } else { - // Replace mode: get current settings, then update with imported values - // For global settings, we still preserve values not in the import data - // because a full "clear" of settings isn't practical const patch = data.global as Partial; await store.updateGlobalSettings(patch); globalCount = Object.entries(globalSettings).filter( @@ -215,12 +410,20 @@ export async function importSettings( } } - // Import project settings if present and requested + // Import project settings if present and requested. if ((scope === "project" || scope === "both") && data.project) { - const projectSettings = data.project as Partial; + const projectSection = data.project as Record; + + // KTD-8: a v1 payload may carry moved keys flat under `project`. Upgrade + // them into workflow setting values (the project write would strip them + // anyway). v2 payloads carry no moved keys here, so this is a no-op for v2. + workflowSettingsCount += await upgradeMovedKeysFromV1Project(store, projectSection); + + // Non-moved project keys import as before. Strip moved keys defensively so + // the count reflects only what actually lands in project settings. + const projectSettings = stripMovedSettingsKeys(projectSection) as Partial; if (merge) { - // Merge mode: only import defined fields const definedEntries = Object.entries(projectSettings).filter( ([, value]) => value !== undefined ); @@ -230,8 +433,6 @@ export async function importSettings( projectCount = definedEntries.length; } } else { - // Replace mode: We need to explicitly handle this by updating all project settings - // The store's updateSettings merges, so we need to be explicit about clearing const patch = projectSettings as Partial; await store.updateSettings(patch); projectCount = Object.entries(projectSettings).filter( @@ -240,16 +441,29 @@ export async function importSettings( } } + // Import workflow setting values (v2). Only meaningful when project scope is + // in play (these values are project-scoped). Round-trips through the store's + // validated write path; invalid values drop-and-log without aborting. + if ((scope === "project" || scope === "both") && data.workflowSettings) { + workflowSettingsCount += await applyWorkflowSettingsSection( + store, + data.workflowSettings, + merge, + ); + } + return { success: true, globalCount, projectCount, + workflowSettingsCount, }; } catch (err) { return { success: false, globalCount, projectCount, + workflowSettingsCount, error: (err as Error).message, }; } diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 4a126a7486..a764f30bc5 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -7074,6 +7074,66 @@ export class TaskStore extends EventEmitter { } } + /** + * Enumerate every stored `workflow_settings` value row for THIS project + * (`getWorkflowSettingsProjectId()`), returned as `workflowId → values map`. + * Used by settings export v2 to carry the value table. Rows whose JSON is + * corrupt or non-object are skipped; rows with an empty values map are + * included as `{}` only if the row physically exists (callers that want to + * drop empties filter on their side). + */ + listWorkflowSettingValuesForProject(): Record> { + const projectId = this.getWorkflowSettingsProjectId(); + const rows = this.db + .prepare('SELECT workflowId, "values" FROM workflow_settings WHERE projectId = ?') + .all(projectId) as Array<{ workflowId: string; values: string }>; + const out: Record> = {}; + for (const row of rows) { + try { + const parsed = JSON.parse(row.values) as unknown; + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + out[row.workflowId] = parsed as Record; + } + } catch { + // Skip corrupt row. + } + } + return out; + } + + /** + * Compute the write-target workflow ids for moved-setting values in THIS + * project: every distinct `task_workflow_selection.workflowId` in use ∪ the + * resolved project default, where an unset/empty/missing default normalizes to + * `builtin:coding`. Shared by the U4 hard-move migration and the U5 settings + * export v1→v2 upgrade so both write to exactly the same lanes. + */ + async computeMovedSettingsTargetWorkflowIds(): Promise> { + const targetWorkflowIds = new Set(); + try { + const rows = this.db + .prepare("SELECT DISTINCT workflowId FROM task_workflow_selection WHERE workflowId IS NOT NULL AND workflowId != ''") + .all() as Array<{ workflowId: string }>; + for (const row of rows) { + if (row.workflowId && row.workflowId.trim()) targetWorkflowIds.add(row.workflowId); + } + } catch { + // No selections / table issue — fall through to the default below. + } + let defaultWorkflowId = "builtin:coding"; + try { + const resolved = await this.getDefaultWorkflowId(); + if (resolved && resolved.trim()) { + const exists = isBuiltinWorkflowId(resolved) || (await this.getWorkflowDefinition(resolved)); + defaultWorkflowId = exists ? resolved : "builtin:coding"; + } + } catch { + defaultWorkflowId = "builtin:coding"; + } + targetWorkflowIds.add(defaultWorkflowId); + return targetWorkflowIds; + } + /** Read the raw stored setting-value map for `(workflowId, projectId)`. Returns * an empty object when no row exists. Raw (pre drop-on-orphan) — callers that * need engine-effective values run {@link resolveEffectiveSettingValues}. */ @@ -11811,31 +11871,9 @@ export class TaskStore extends EventEmitter { } } - // (2) Compute the write-target workflow ids. - const targetWorkflowIds = new Set(); - try { - const rows = this.db - .prepare("SELECT DISTINCT workflowId FROM task_workflow_selection WHERE workflowId IS NOT NULL AND workflowId != ''") - .all() as Array<{ workflowId: string }>; - for (const row of rows) { - if (row.workflowId && row.workflowId.trim()) targetWorkflowIds.add(row.workflowId); - } - } catch { - // No selections / table issue — fall through to the default below. - } - // Resolve the project default, normalizing unset/empty/missing → builtin:coding. - let defaultWorkflowId = "builtin:coding"; - try { - const resolved = await this.getDefaultWorkflowId(); - if (resolved && resolved.trim()) { - // A default pointing at a deleted/missing workflow degrades to builtin:coding. - const exists = isBuiltinWorkflowId(resolved) || (await this.getWorkflowDefinition(resolved)); - defaultWorkflowId = exists ? resolved : "builtin:coding"; - } - } catch { - defaultWorkflowId = "builtin:coding"; - } - targetWorkflowIds.add(defaultWorkflowId); + // (2) Compute the write-target workflow ids (shared with the U5 v1→v2 + // import upgrade so both write to identical lanes). + const targetWorkflowIds = await this.computeMovedSettingsTargetWorkflowIds(); // (3) Validate the snapshot per target workflow (async declaration resolution // done HERE, before the synchronous transaction). Drop-and-log invalid diff --git a/packages/dashboard/app/components/SettingsModal.css b/packages/dashboard/app/components/SettingsModal.css index d622d5f21d..b726bd574b 100644 --- a/packages/dashboard/app/components/SettingsModal.css +++ b/packages/dashboard/app/components/SettingsModal.css @@ -2110,6 +2110,13 @@ margin-right: var(--space-xs); } +/* KTD-8: informational note at the bottom of the Node Sync section. */ +.settings-sync-workflow-note { + margin-top: var(--space-md); + font-size: var(--font-size-sm); + color: var(--text-muted); +} + @media (max-width: 768px) { .auth-custom-provider-item { flex-direction: column; diff --git a/packages/dashboard/app/components/SettingsModal.tsx b/packages/dashboard/app/components/SettingsModal.tsx index f268a491eb..a2d865997c 100644 --- a/packages/dashboard/app/components/SettingsModal.tsx +++ b/packages/dashboard/app/components/SettingsModal.tsx @@ -6678,6 +6678,14 @@ export function SettingsModal({ )} + {/* KTD-8: workflow settings are not yet part of the cross-node sync + channel. Non-dismissible, informational only, no action affordance. */} +

+ {t( + "settings.nodeSync.workflowSettingsNotSynced", + "Workflow settings are not synced across nodes yet.", + )} +

); case "remote": { diff --git a/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts b/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts index df3065a83e..3b782f32de 100644 --- a/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts +++ b/packages/dashboard/src/__tests__/routes-nodes-sync.test.ts @@ -4,6 +4,7 @@ import { request, get } from "../test-request.js"; import { createServer } from "../server.js"; import { resetRuntimeLogSink, setRuntimeLogSink, type RuntimeLogContext } from "../runtime-logger.js"; import { MISSING_REMOTE_NODE_API_KEY_MESSAGE } from "../routes/register-settings-sync-helpers.js"; +import { MOVED_SETTINGS_KEYS } from "@fusion/core"; // Mock node:fs for auth.json reading vi.mock("node:fs", () => ({ @@ -532,6 +533,50 @@ describe("Node settings sync routes", () => { expect(mockApplyRemoteSettings).not.toHaveBeenCalled(); }); + it("manual diff EXCLUDES moved keys even when a mid-migration remote peer still carries them (KTD-8)", async () => { + const movedProjectKey = MOVED_SETTINGS_KEYS[0]; // e.g. workflowStepTimeoutMs + const movedGlobalKey = MOVED_SETTINGS_KEYS.find((k) => k === "executionProvider") ?? MOVED_SETTINGS_KEYS[1]; + const remoteNode = createMockRemoteNode(); + mockGetNode.mockResolvedValue(remoteNode); + // REAL post-broadcast shape: an unmigrated peer's /settings/scopes still lists + // moved keys flat under global/project, with values that DIFFER from local. + mockFetch.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ + global: { defaultProvider: "openai", [movedGlobalKey]: "anthropic" }, + project: { maxConcurrent: 3, [movedProjectKey]: 999_999 }, + }), + }); + // Migrated local node: no moved keys present at all. + vi.spyOn(store, "getSettingsByScope").mockResolvedValue({ + global: {}, + project: { maxConcurrent: 1 }, + }); + vi.spyOn(store, "getGlobalSettingsStore").mockReturnValue({ + getSettings: vi.fn().mockResolvedValue({ defaultProvider: "anthropic" }), + } as ReturnType); + + const res = await request( + app, + "POST", + "/api/nodes/node-remote-001/settings/pull", + JSON.stringify({ conflictResolution: "manual" }), + { "content-type": "application/json" }, + ); + + expect(res.status).toBe(200); + // Non-moved differences still surface. + expect(res.body.diff.global).toContain("defaultProvider"); + expect(res.body.diff.project).toContain("maxConcurrent"); + // Moved keys NEVER appear in the diff, despite differing values. + expect(res.body.diff.global).not.toContain(movedGlobalKey); + expect(res.body.diff.project).not.toContain(movedProjectKey); + for (const k of MOVED_SETTINGS_KEYS) { + expect(res.body.diff.global).not.toContain(k); + expect(res.body.diff.project).not.toContain(k); + } + }); + it("returns 400 for local node", async () => { const localNode = createMockLocalNode(); mockGetNode.mockResolvedValue(localNode); @@ -1063,6 +1108,44 @@ describe("Node settings sync routes", () => { expect(mockStoreUpdateGlobalSettings).toHaveBeenCalledWith({ dashboardCurrentNodeId: "node-remote-001" }); }); + it("drops moved keys from an inbound push: never applied to global settings, never in appliedFields (KTD-8)", async () => { + const localNode = createMockLocalNode(); + mockListNodes.mockResolvedValue([localNode]); + mockApplyRemoteSettings.mockResolvedValue({ + success: true, + globalCount: 1, + projectCount: 0, + authCount: 0, + }); + const movedGlobalKey = MOVED_SETTINGS_KEYS.find((k) => k === "executionProvider") ?? MOVED_SETTINGS_KEYS[0]; + + // REAL post-broadcast payload from a mid-migration peer: a moved key rides + // along under `global` next to a legitimate new key. + const res = await request( + app, + "POST", + "/api/settings/sync-receive", + JSON.stringify({ + sourceNodeId: "node-remote-001", + exportedAt: "2026-04-14T10:00:00.000Z", + checksum: "abc123", + version: 1, + global: { newLegitKey: "value-x", [movedGlobalKey]: "anthropic" }, + }), + { "content-type": "application/json", "Authorization": `Bearer ${localNode.apiKey}` }, + ); + + expect(res.status).toBe(200); + // The legit key is applied; the moved key is NOT in the patch. + const patches = mockStoreUpdateGlobalSettings.mock.calls.map((c) => c[0] as Record); + const applied = Object.assign({}, ...patches) as Record; + expect(applied.newLegitKey).toBe("value-x"); + expect(applied[movedGlobalKey]).toBeUndefined(); + // appliedFields reported back also excludes the moved key. + expect(res.body.appliedFields).toContain("newLegitKey"); + expect(res.body.appliedFields).not.toContain(movedGlobalKey); + }); + it("returns 401 when auth header is missing", async () => { const res = await request( app, diff --git a/packages/dashboard/src/routes/register-settings-sync-inbound-routes.ts b/packages/dashboard/src/routes/register-settings-sync-inbound-routes.ts index ff81f040ca..5dc9046b2f 100644 --- a/packages/dashboard/src/routes/register-settings-sync-inbound-routes.ts +++ b/packages/dashboard/src/routes/register-settings-sync-inbound-routes.ts @@ -1,3 +1,4 @@ +import { isMovedSettingsKey } from "@fusion/core"; import { ApiError, badRequest } from "../api-error.js"; import { invalidateAllGlobalSettingsCaches } from "../project-store-resolver.js"; import { getFusionAuthPath } from "../auth-paths.js"; @@ -71,7 +72,9 @@ export const registerSettingsSyncInboundRoutes: ApiRouteRegistrar = (ctx) => { const localGlobal = await store.getGlobalSettingsStore().getSettings() as Record; const globalPatch = Object.fromEntries( Object.entries(payload.global as Record) - .filter(([key, value]) => value !== undefined && localGlobal[key] === undefined), + // Drop moved (tombstoned) keys here too — defense beyond the store + // guard so an inbound push can never resurrect a moved key (KTD-8). + .filter(([key, value]) => value !== undefined && localGlobal[key] === undefined && !isMovedSettingsKey(key)), ); if (Object.keys(globalPatch).length > 0) { await store.updateGlobalSettings(globalPatch); @@ -79,11 +82,13 @@ export const registerSettingsSyncInboundRoutes: ApiRouteRegistrar = (ctx) => { } } - // Build applied/skipped field lists + // Build applied/skipped field lists. Moved keys are excluded so the reported + // applied set matches what actually persisted (the store + applyRemoteSettings + // both drop them). const appliedFields = [ ...Object.keys(payload.global || {}), ...Object.keys(payload.projects || {}), - ]; + ].filter((key) => !isMovedSettingsKey(key)); const skippedFields = result.error ? appliedFields : []; await central.close(); diff --git a/packages/dashboard/src/routes/register-settings-sync-routes.ts b/packages/dashboard/src/routes/register-settings-sync-routes.ts index f691aa1a9c..95eb66bb9d 100644 --- a/packages/dashboard/src/routes/register-settings-sync-routes.ts +++ b/packages/dashboard/src/routes/register-settings-sync-routes.ts @@ -1,4 +1,5 @@ import type { ProjectSettings } from "@fusion/core"; +import { isMovedSettingsKey } from "@fusion/core"; import { basename } from "node:path"; import { ApiError, badRequest, notFound } from "../api-error.js"; import { getFusionAuthPath } from "../auth-paths.js"; @@ -17,14 +18,17 @@ function computeSettingsDiff( localGlobalSettings: Record, localProjectSettings: Record, ): { global: string[]; project: string[] } { + // Moved (tombstoned) keys are excluded from the diff entirely (KTD-8): workflow + // settings are not synced across nodes yet, so they must never appear in a + // diff/push/pull field list — even if a mid-migration peer still carries them. const globalKeys = Array.from(new Set([ ...Object.keys(remoteSettings.global ?? {}), ...Object.keys(localGlobalSettings ?? {}), - ])); + ])).filter((key) => !isMovedSettingsKey(key)); const projectKeys = Array.from(new Set([ ...Object.keys(remoteSettings.project ?? {}), ...Object.keys(localProjectSettings ?? {}), - ])); + ])).filter((key) => !isMovedSettingsKey(key)); return { global: globalKeys.filter((key) => JSON.stringify(remoteSettings.global?.[key]) !== JSON.stringify(localGlobalSettings[key])),