diff --git a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts index 6e253b7a58..5b715f2fa4 100644 --- a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts +++ b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts @@ -70,12 +70,42 @@ interface MockTask { column: string; } +// `requirePrApproval` MOVED to workflow settings (U4): the CLI now resolves the +// task's EFFECTIVE workflow settings and overlays them onto the project base. So a +// mock store must expose `requirePrApproval` (and any moved key) through the +// effective-settings resolver store surface (`getWorkflowSettingValues` etc.), not +// through `getSettings()`. These stubs make `resolveEffectiveSettings` degrade to +// `builtin:coding` and read the moved value from the stored workflow values. +const MOVED_TEST_KEYS = new Set(["requirePrApproval"]); + +function splitMovedSettings(settings: Record) { + const projectSettings: Record = {}; + const workflowValues: Record = {}; + for (const [key, value] of Object.entries(settings)) { + if (MOVED_TEST_KEYS.has(key)) workflowValues[key] = value; + else projectSettings[key] = value; + } + return { projectSettings, workflowValues }; +} + +function workflowSettingsResolverStubs(workflowValues: Record) { + return { + // No selection → resolver degrades to builtin:coding, whose declarations carry + // the moved-key catalog; the stored values below override the declaration default. + getTaskWorkflowSelection: vi.fn().mockReturnValue(undefined), + getWorkflowDefinition: vi.fn().mockResolvedValue(undefined), + getWorkflowSettingValues: vi.fn().mockReturnValue(workflowValues), + getWorkflowSettingsProjectId: vi.fn().mockReturnValue("test-project"), + }; +} + function makeStore(task: MockTask, settings: Record = {}) { const emitter = new EventEmitter(); const updates: Array<{ id: string; patch: Record }> = []; + const { projectSettings, workflowValues } = splitMovedSettings(settings); return Object.assign(emitter, { getTask: vi.fn().mockResolvedValue(task), - getSettings: vi.fn().mockResolvedValue({ requirePrApproval: false, ...settings }), + getSettings: vi.fn().mockResolvedValue({ ...projectSettings }), updateTask: vi.fn(async (id: string, patch: Record) => { updates.push({ id, patch }); }), @@ -86,6 +116,7 @@ function makeStore(task: MockTask, settings: Record = {}) { getBranchGroup: vi.fn().mockReturnValue(null), updateBranchGroup: vi.fn(), listTasksByBranchGroup: vi.fn().mockResolvedValue([]), + ...workflowSettingsResolverStubs(workflowValues), _updates: updates, }); } @@ -93,9 +124,11 @@ function makeStore(task: MockTask, settings: Record = {}) { function makeStatefulStore(task: MockTask, settings: Record = {}) { const emitter = new EventEmitter(); let state = structuredClone(task); + const { projectSettings, workflowValues } = splitMovedSettings(settings); return Object.assign(emitter, { getTask: vi.fn(async () => structuredClone(state)), - getSettings: vi.fn().mockResolvedValue({ requirePrApproval: false, ...settings }), + getSettings: vi.fn().mockResolvedValue({ ...projectSettings }), + ...workflowSettingsResolverStubs(workflowValues), updateTask: vi.fn(async (_id: string, patch: Record) => { state = { ...state, ...patch }; }), diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index be0004c70e..bb84e44f3f 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -24,7 +24,7 @@ const execAsync = promisify(exec); const execFileAsync: (file: string, args: string[], opts?: import("node:child_process").ExecFileOptions) => Promise<{ stdout: string; stderr: string }> = (file, args, opts) => (promisify(childProcess.execFile) as (f: string, a: string[], o?: object) => Promise<{ stdout: string; stderr: string }>)(file, args, opts); import type { TaskStore } from "@fusion/core"; -import { resolveTaskMergeTarget, getCurrentRepo, isBranchGroupMemberLanded } from "@fusion/core"; +import { resolveTaskMergeTarget, getCurrentRepo, isBranchGroupMemberLanded, resolveEffectiveSettings } from "@fusion/core"; import type { Settings, TaskDetail, PrInfo, MergeResult, BranchGroup, BranchGroupPrState, Task } from "@fusion/core"; import { activeSessionRegistry, resolveIntegrationBranch } from "@fusion/engine"; import type { CreateGroupPrFn, SyncGroupPrFn, WorktreePool } from "@fusion/engine"; @@ -448,6 +448,17 @@ export async function processPullRequestMergeTask( const branch = getTaskBranchName(task.id); const settings = await store.getSettings(); + // `requirePrApproval` MOVED to workflow settings (U4): resolve the task's + // effective workflow settings and overlay them onto the project/global base so + // the approval-gate reads the per-(workflow, project) value post-migration. The + // resolver never throws — a missing workflow degrades to built-in declaration + // defaults (requirePrApproval=false), matching the pre-move default. + try { + const effective = await resolveEffectiveSettings(store, { id: task.id }); + Object.assign(settings as Record, effective); + } catch { + // Defensive: keep the base settings if effective resolution fails entirely. + } const resolvedIntegrationBranch = await resolveIntegrationBranch(cwd, settings); const projectDefaultBranch = resolvedIntegrationBranch; diff --git a/packages/core/src/__tests__/settings-migration.test.ts b/packages/core/src/__tests__/settings-migration.test.ts new file mode 100644 index 0000000000..61ddd0838f --- /dev/null +++ b/packages/core/src/__tests__/settings-migration.test.ts @@ -0,0 +1,362 @@ +/** + * U4 — One-time hard-move migration of MOVED_SETTINGS_KEYS into workflow setting + * values (R6, R8, KTD-5). The load-bearing gate is the default re-injection + * regression: post-migration, saving an unrelated setting must NOT re-materialize + * any moved key in raw storage. + * + * Strategy: the migration runs at store init. To exercise a *pre-migration + * customized project* deterministically, we (a) init a store, (b) seed the RAW + * `config.settings` row + global settings file with customized moved keys and + * clear the `__meta` marker (simulating a project written by an older binary), + * then (c) invoke the migration directly and assert the end state. This mirrors + * the real flow (a fresh `init()` on a legacy DB) without depending on a binary + * downgrade. + */ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync, readFileSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { TaskStore } from "../store.js"; +import { + MOVED_SETTINGS_KEYS, + SETTINGS_MIGRATION_VERSION, + SETTINGS_MIGRATION_MARKER_KEY, +} from "../moved-settings.js"; +import { resolveEffectiveSettingsById, type WorkflowSettingsResolverStore } from "../workflow-settings-resolver.js"; +import { PROJECT_SETTINGS_KEYS } from "../settings-schema.js"; + +// ── Test harness ──────────────────────────────────────────────────────────── + +interface Env { + tempDir: string; + fusionDir: string; + globalSettingsDir: string; +} + +function createEnv(): Env { + const tempDir = mkdtempSync(join(tmpdir(), "fn-settings-migration-")); + const fusionDir = join(tempDir, ".fusion"); + const tasksDir = join(fusionDir, "tasks"); + const globalSettingsDir = join(tempDir, "global-settings"); + mkdirSync(tasksDir, { recursive: true }); + mkdirSync(globalSettingsDir, { recursive: true }); + writeFileSync(join(globalSettingsDir, "settings.json"), JSON.stringify({})); + return { tempDir, fusionDir, globalSettingsDir }; +} + +async function openStore(env: Env): Promise { + const { TaskStore } = await import("../store.js"); + // Disk-backed DB so the global readRaw + config row paths are realistic and the + // raw settings survive across the seeding/migration steps. + const store = new TaskStore(env.tempDir, env.globalSettingsDir, { inMemoryDb: false }); + await store.init(); + return store; +} + +/** Low-level raw db handle (tests routinely reach for `store["db"]`). */ +function rawDb(store: TaskStore): { + prepare: (sql: string) => { run: (...a: unknown[]) => unknown; get: (...a: unknown[]) => unknown; all: (...a: unknown[]) => unknown }; +} { + return (store as unknown as { db: ReturnType }).db; +} + +/** Overwrite the RAW persisted project `config.settings` JSON with `settings`. */ +function seedRawProjectSettings(store: TaskStore, settings: Record): void { + const db = rawDb(store); + const now = new Date().toISOString(); + // Ensure a config row exists, then set its settings JSON directly. + db.prepare( + `INSERT INTO config (id, nextWorkflowStepId, settings, workflowSteps, updatedAt) + VALUES (1, 1, ?, '[]', ?) + ON CONFLICT(id) DO UPDATE SET settings = excluded.settings, updatedAt = excluded.updatedAt`, + ).run(JSON.stringify(settings), now); +} + +/** Read the RAW persisted project settings JSON back. */ +function readRawProjectSettings(store: TaskStore): Record { + const row = rawDb(store).prepare("SELECT settings FROM config WHERE id = 1").get() as + | { settings: string } + | undefined; + if (!row) return {}; + return JSON.parse(row.settings) as Record; +} + +/** Clear the migration marker so the next migration run executes. */ +function clearMarker(store: TaskStore): void { + rawDb(store).prepare("DELETE FROM __meta WHERE key = ?").run(SETTINGS_MIGRATION_MARKER_KEY); +} + +function readMarker(store: TaskStore): number | undefined { + const row = rawDb(store).prepare("SELECT value FROM __meta WHERE key = ?").get(SETTINGS_MIGRATION_MARKER_KEY) as + | { value: string } + | undefined; + return row ? Number(row.value) : undefined; +} + +/** Insert a `task_workflow_selection` row directly (deterministic; no flag deps). */ +function seedSelection(store: TaskStore, taskId: string, workflowId: string): void { + rawDb(store) + .prepare( + `INSERT INTO task_workflow_selection (taskId, workflowId, stepIds, updatedAt) + VALUES (?, ?, '[]', ?) + ON CONFLICT(taskId) DO UPDATE SET workflowId = excluded.workflowId`, + ) + .run(taskId, workflowId, new Date().toISOString()); +} + +/** Run the (private) migration directly. */ +async function runMigration(store: TaskStore): Promise { + await (store as unknown as { migrateMovedSettingsToWorkflowValuesOnce(): Promise }).migrateMovedSettingsToWorkflowValuesOnce(); +} + +const resolverStore = (store: TaskStore) => store as unknown as WorkflowSettingsResolverStore; + +// ── Tests ──────────────────────────────────────────────────────────────────── + +describe("settings hard-move migration (U4)", () => { + let env: Env; + let store: TaskStore; + + beforeEach(async () => { + env = createEnv(); + store = await openStore(env); + }); + + afterEach(async () => { + try { + await store.close(); + } catch { + /* ignore */ + } + try { + rmSync(env.tempDir, { recursive: true, force: true }); + } catch { + /* ignore */ + } + }); + + it("MOVED_SETTINGS_KEYS excludes buildTimeoutMs and the reflection interval/after keys", () => { + expect(MOVED_SETTINGS_KEYS).not.toContain("buildTimeoutMs"); + expect(MOVED_SETTINGS_KEYS).not.toContain("reflectionIntervalMs"); + expect(MOVED_SETTINGS_KEYS).not.toContain("reflectionAfterTask"); + expect(MOVED_SETTINGS_KEYS).not.toContain("completionDocumentationMode"); + expect(MOVED_SETTINGS_KEYS).toContain("workflowStepTimeoutMs"); + expect(MOVED_SETTINGS_KEYS).toContain("requirePrApproval"); + expect(MOVED_SETTINGS_KEYS).toContain("executionProvider"); + // 30 keys after removing buildTimeoutMs from the catalog. + expect(MOVED_SETTINGS_KEYS.length).toBe(30); + }); + + it("fresh project post-init: marker set, effective values equal declaration defaults, no moved key in PROJECT_SETTINGS_KEYS", async () => { + // The store's own init() already ran the migration on a fresh DB. + expect(readMarker(store)).toBe(SETTINGS_MIGRATION_VERSION); + for (const key of MOVED_SETTINGS_KEYS) { + expect((PROJECT_SETTINGS_KEYS as readonly string[]).includes(key)).toBe(false); + } + const effective = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", store.getWorkflowSettingsProjectId()); + // Declaration defaults: workflowStepTimeoutMs=360000, requirePrApproval=false. + expect(effective.workflowStepTimeoutMs).toBe(360_000); + expect(effective.requirePrApproval).toBe(false); + }); + + it("customized project: moved values land under the in-use (workflowId, projectId); raw settings lose the keys; effective values identical pre/post", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + + // Capture the PRE-migration effective values (the migration hasn't run on the + // seeded state yet). We resolve them from the legacy raw values by simulating + // them as builtin:coding effective inputs: pre-move these lived in project + // settings, so the "effective" engine value WAS the customized value. + const customized = { + // unrelated, non-moved project key — must survive untouched + maxConcurrent: 3, + // moved keys, customized: + workflowStepTimeoutMs: 120_000, + requirePrApproval: true, + executionProvider: "anthropic", + }; + seedRawProjectSettings(store, customized); + clearMarker(store); + + await runMigration(store); + + // Marker set. + expect(readMarker(store)).toBe(SETTINGS_MIGRATION_VERSION); + + // Raw project settings no longer contain the moved keys; the unrelated key stays. + const raw = readRawProjectSettings(store); + expect(raw.workflowStepTimeoutMs).toBeUndefined(); + expect(raw.requirePrApproval).toBeUndefined(); + expect(raw.executionProvider).toBeUndefined(); + expect(raw.maxConcurrent).toBe(3); + + // Values land on the resolved default (builtin:coding) for this project. + const effective = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + expect(effective.workflowStepTimeoutMs).toBe(120_000); + expect(effective.requirePrApproval).toBe(true); + expect(effective.executionProvider).toBe("anthropic"); + }); + + it("mixed-pinning: one builtin task + one custom-pinned task, defaultWorkflowId unset → both read identical customized effective values", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + // A custom workflow declaring the moved keys (so values validate against it). + const custom = await store.createWorkflowDefinition({ + name: "Custom WF", + ir: { + version: "v2", + name: "custom-wf", + columns: [{ id: "todo", name: "Todo", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { id: "end", kind: "end" }, + ], + edges: [{ from: "start", to: "end" }], + settings: [ + { id: "workflowStepTimeoutMs", name: "Step timeout", type: "number", default: 360_000 }, + { id: "requirePrApproval", name: "Require PR approval", type: "boolean", default: false }, + ], + }, + }); + + seedSelection(store, "FN-1", custom.id); // task pinned to custom + // FN-2 has NO selection row → resolves builtin:coding. + seedRawProjectSettings(store, { + workflowStepTimeoutMs: 200_000, + requirePrApproval: true, + }); + clearMarker(store); + + await runMigration(store); + + const builtinEffective = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + const customEffective = await resolveEffectiveSettingsById(resolverStore(store), custom.id, projectId); + + expect(builtinEffective.workflowStepTimeoutMs).toBe(200_000); + expect(builtinEffective.requirePrApproval).toBe(true); + expect(customEffective.workflowStepTimeoutMs).toBe(200_000); + expect(customEffective.requirePrApproval).toBe(true); + }); + + it("defaultWorkflowId unset, no selections → snapshot lands on (builtin:coding, projectId)", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + seedRawProjectSettings(store, { workflowStepTimeoutMs: 90_000 }); + clearMarker(store); + + await runMigration(store); + + const effective = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + expect(effective.workflowStepTimeoutMs).toBe(90_000); + }); + + it("migration runs twice → second run is a no-op (idempotent via marker)", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + seedRawProjectSettings(store, { workflowStepTimeoutMs: 111_000 }); + clearMarker(store); + + await runMigration(store); + const valuesAfterFirst = store.getWorkflowSettingValues("builtin:coding", projectId); + + // Second run: marker is set, so it no-ops. Mutating raw settings afterward must + // not be re-snapshotted. + await runMigration(store); + const valuesAfterSecond = store.getWorkflowSettingValues("builtin:coding", projectId); + expect(valuesAfterSecond).toEqual(valuesAfterFirst); + expect(valuesAfterSecond.workflowStepTimeoutMs).toBe(111_000); + }); + + it("crash simulation: value-writes then full re-run converges (write-then-null re-runnable)", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + seedRawProjectSettings(store, { workflowStepTimeoutMs: 150_000, requirePrApproval: true }); + clearMarker(store); + + // First (completing) run. + await runMigration(store); + const first = store.getWorkflowSettingValues("builtin:coding", projectId); + + // Simulate a crash that left the marker UNSET but values written: clear marker, + // restore the raw keys (as if the null-out had not committed), re-run. + clearMarker(store); + seedRawProjectSettings(store, { workflowStepTimeoutMs: 150_000, requirePrApproval: true }); + await runMigration(store); + + const second = store.getWorkflowSettingValues("builtin:coding", projectId); + expect(second.workflowStepTimeoutMs).toBe(first.workflowStepTimeoutMs); + expect(second.requirePrApproval).toBe(first.requirePrApproval); + expect(readRawProjectSettings(store).workflowStepTimeoutMs).toBeUndefined(); + expect(readMarker(store)).toBe(SETTINGS_MIGRATION_VERSION); + }); + + it("LOAD-BEARING: post-migration save of an unrelated setting does NOT re-materialize any moved key; effective values unchanged", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + seedRawProjectSettings(store, { workflowStepTimeoutMs: 130_000, requirePrApproval: true, maxConcurrent: 2 }); + clearMarker(store); + await runMigration(store); + + const before = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + + // Save an UNRELATED project setting through the normal API. + await store.updateSettings({ maxConcurrent: 7 }); + + // No moved key re-materialized in raw storage (the default re-injection trap). + const raw = readRawProjectSettings(store); + for (const key of MOVED_SETTINGS_KEYS) { + expect(raw[key]).toBeUndefined(); + } + expect(raw.maxConcurrent).toBe(7); + + // Effective values unchanged. + const after = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + expect(after.workflowStepTimeoutMs).toBe(before.workflowStepTimeoutMs); + expect(after.requirePrApproval).toBe(before.requirePrApproval); + }); + + it("defaultWorkflowId points at a deleted/missing workflow → values land on builtin:coding", async () => { + const projectId = store.getWorkflowSettingsProjectId(); + // Seed a default pointing at a non-existent workflow + the customized value. + seedRawProjectSettings(store, { + defaultWorkflowId: "missing-workflow-id", + workflowStepTimeoutMs: 175_000, + }); + clearMarker(store); + + await runMigration(store); + + const effective = await resolveEffectiveSettingsById(resolverStore(store), "builtin:coding", projectId); + expect(effective.workflowStepTimeoutMs).toBe(175_000); + // The missing workflow id received nothing. + const missingValues = store.getWorkflowSettingValues("missing-workflow-id", projectId); + expect(missingValues.workflowStepTimeoutMs).toBeUndefined(); + }); + + it("stale writer: updateSettings patch containing a moved key post-migration is dropped, not persisted", async () => { + clearMarker(store); + await runMigration(store); + + await store.updateSettings({ + // unrelated key + maxConcurrent: 5, + // stale moved key — must be dropped + workflowStepTimeoutMs: 999_999, + } as unknown as Parameters[0]); + + const raw = readRawProjectSettings(store); + expect(raw.maxConcurrent).toBe(5); + expect(raw.workflowStepTimeoutMs).toBeUndefined(); + }); + + it("global settings file moved keys are nulled out by the migration (defensive belt)", async () => { + // Seed a moved key into the global settings file (legacy/defensive case). + const globalPath = join(env.globalSettingsDir, "settings.json"); + writeFileSync(globalPath, JSON.stringify({ requirePrApproval: true, themeMode: "dark" })); + // Also seed the project raw with the same key (project wins). + seedRawProjectSettings(store, { requirePrApproval: true }); + clearMarker(store); + + await runMigration(store); + + const globalRaw = existsSync(globalPath) + ? (JSON.parse(readFileSync(globalPath, "utf-8")) as Record) + : {}; + expect(globalRaw.requirePrApproval).toBeUndefined(); + expect(globalRaw.themeMode).toBe("dark"); + }); +}); diff --git a/packages/core/src/__tests__/settings-parity.test.ts b/packages/core/src/__tests__/settings-parity.test.ts index a2e13b78b9..eae4ca2f72 100644 --- a/packages/core/src/__tests__/settings-parity.test.ts +++ b/packages/core/src/__tests__/settings-parity.test.ts @@ -182,7 +182,56 @@ describe("settings key parity", () => { it("keeps task stuck timeout active by default without coupling to workflow step timeout", () => { expect(DEFAULT_PROJECT_SETTINGS.taskStuckTimeoutMs).toBe(600_000); expect(DEFAULT_PROJECT_SETTINGS.runtimeStopDrainMs).toBe(2_000); - expect(DEFAULT_PROJECT_SETTINGS.workflowStepTimeoutMs).toBe(360_000); + // workflowStepTimeoutMs MOVED to workflow settings (U4) — no longer a project key. + expect(isProjectSettingsKey("workflowStepTimeoutMs")).toBe(false); + expect(PROJECT_SETTINGS_KEYS).not.toContain("workflowStepTimeoutMs"); + }); + + it("removes the moved settings keys (U4 hard-move) from the project scope", () => { + const movedKeys = [ + "workflowStepTimeoutMs", + "workflowStepScopeEnforcement", + "planOnlyScopeLeakEnforcement", + "workflowRevisionForkOnScopeMismatch", + "strictScopeEnforcement", + "runStepsInNewSessions", + "maxParallelSteps", + "buildRetryCount", + "verificationFixRetries", + "maxPostReviewFixes", + "requirePrApproval", + "requirePlanApproval", + "reviewHandoffPolicy", + "maxReviewerContextRetries", + "maxReviewerFallbackRetries", + "reflectionEnabled", + "executionProvider", + "executionModelId", + "planningProvider", + "planningModelId", + "planningFallbackProvider", + "planningFallbackModelId", + "validatorProvider", + "validatorModelId", + "validatorFallbackProvider", + "validatorFallbackModelId", + "titleSummarizerProvider", + "titleSummarizerModelId", + "titleSummarizerFallbackProvider", + "titleSummarizerFallbackModelId", + ]; + for (const key of movedKeys) { + expect(isProjectSettingsKey(key)).toBe(false); + expect(PROJECT_SETTINGS_KEYS).not.toContain(key); + expect(isGlobalSettingsKey(key)).toBe(false); + } + }); + + it("keeps buildTimeoutMs / reflectionIntervalMs / reflectionAfterTask project-scoped (NOT moved)", () => { + expect(isProjectSettingsKey("buildTimeoutMs")).toBe(true); + expect(isProjectSettingsKey("reflectionIntervalMs")).toBe(true); + expect(isProjectSettingsKey("reflectionAfterTask")).toBe(true); + expect(DEFAULT_PROJECT_SETTINGS.buildTimeoutMs).toBe(300_000); }); it("defaults engine activation grace and leaves engine active clock undefined", () => { @@ -367,27 +416,33 @@ describe("eval settings parity regression (FN-3393)", () => { }); describe("model lane key parity regression (FN-1729)", () => { - // All model lane provider/modelId pairs that should exist + // All model lane provider/modelId pairs that should exist. + // + // U4 hard-move: the per-PHASE project lanes (execution/planning/validator/ + // titleSummarizer provider+model, plus their fallbacks) MOVED to workflow + // settings and are no longer in either scope key list ("workflow" scope). The + // GLOBAL baseline lanes (`*GlobalProvider`) and the default/fallback baseline + // stay global. const allModelLanePairs = [ // Default baseline (global only) { provider: "defaultProvider", modelId: "defaultModelId", expectedScope: "global" }, // Fallback baseline (global only) { provider: "fallbackProvider", modelId: "fallbackModelId", expectedScope: "global" }, // Execution lane - { provider: "executionProvider", modelId: "executionModelId", expectedScope: "project" }, + { provider: "executionProvider", modelId: "executionModelId", expectedScope: "workflow" }, { provider: "executionGlobalProvider", modelId: "executionGlobalModelId", expectedScope: "global" }, // Planning lane - { provider: "planningProvider", modelId: "planningModelId", expectedScope: "project" }, + { provider: "planningProvider", modelId: "planningModelId", expectedScope: "workflow" }, { provider: "planningGlobalProvider", modelId: "planningGlobalModelId", expectedScope: "global" }, - { provider: "planningFallbackProvider", modelId: "planningFallbackModelId", expectedScope: "project" }, + { provider: "planningFallbackProvider", modelId: "planningFallbackModelId", expectedScope: "workflow" }, // Validator lane - { provider: "validatorProvider", modelId: "validatorModelId", expectedScope: "project" }, + { provider: "validatorProvider", modelId: "validatorModelId", expectedScope: "workflow" }, { provider: "validatorGlobalProvider", modelId: "validatorGlobalModelId", expectedScope: "global" }, - { provider: "validatorFallbackProvider", modelId: "validatorFallbackModelId", expectedScope: "project" }, + { provider: "validatorFallbackProvider", modelId: "validatorFallbackModelId", expectedScope: "workflow" }, // Summarizer lane - { provider: "titleSummarizerProvider", modelId: "titleSummarizerModelId", expectedScope: "project" }, + { provider: "titleSummarizerProvider", modelId: "titleSummarizerModelId", expectedScope: "workflow" }, { provider: "titleSummarizerGlobalProvider", modelId: "titleSummarizerGlobalModelId", expectedScope: "global" }, - { provider: "titleSummarizerFallbackProvider", modelId: "titleSummarizerFallbackModelId", expectedScope: "project" }, + { provider: "titleSummarizerFallbackProvider", modelId: "titleSummarizerFallbackModelId", expectedScope: "workflow" }, ] as const; it.each(allModelLanePairs)( @@ -398,6 +453,12 @@ describe("model lane key parity regression (FN-1729)", () => { expect(isGlobalSettingsKey(modelId)).toBe(true); expect(isProjectSettingsKey(provider)).toBe(false); expect(isProjectSettingsKey(modelId)).toBe(false); + } else if (expectedScope === "workflow") { + // Moved to workflow settings — absent from BOTH scope key lists. + expect(isGlobalSettingsKey(provider)).toBe(false); + expect(isGlobalSettingsKey(modelId)).toBe(false); + expect(isProjectSettingsKey(provider)).toBe(false); + expect(isProjectSettingsKey(modelId)).toBe(false); } else { expect(isProjectSettingsKey(provider)).toBe(true); expect(isProjectSettingsKey(modelId)).toBe(true); @@ -407,15 +468,19 @@ describe("model lane key parity regression (FN-1729)", () => { }, ); - it("model lane keys appear in exactly one scope key list", () => { + it("scoped (non-workflow) model lane keys appear in exactly one scope key list", () => { const globalKeys = new Set(GLOBAL_SETTINGS_KEYS as readonly string[]); const projectKeys = new Set(PROJECT_SETTINGS_KEYS as readonly string[]); - for (const { provider, modelId } of allModelLanePairs) { + for (const { provider, modelId, expectedScope } of allModelLanePairs) { + if (expectedScope === "workflow") { + // Workflow-scoped lanes are in neither list. + expect(globalKeys.has(provider) || projectKeys.has(provider)).toBe(false); + expect(globalKeys.has(modelId) || projectKeys.has(modelId)).toBe(false); + continue; + } const inGlobal = globalKeys.has(provider) && globalKeys.has(modelId); const inProject = projectKeys.has(provider) && projectKeys.has(modelId); - - // Each pair must appear in exactly one scope expect(inGlobal || inProject).toBe(true); expect(inGlobal && inProject).toBe(false); } @@ -433,15 +498,14 @@ describe("model lane key parity regression (FN-1729)", () => { } }); - it("all project model lane keys are in PROJECT_SETTINGS_KEYS", () => { - const projectKeys = new Set(PROJECT_SETTINGS_KEYS as readonly string[]); - - const projectLanes = allModelLanePairs - .filter((p) => p.expectedScope === "project") + it("moved (workflow) model lane keys are in NEITHER scope key list", () => { + const allKeys = new Set([...GLOBAL_SETTINGS_KEYS, ...PROJECT_SETTINGS_KEYS] as readonly string[]); + const workflowLanes = allModelLanePairs + .filter((p) => p.expectedScope === "workflow") .flatMap((p) => [p.provider, p.modelId]); - for (const key of projectLanes) { - expect(projectKeys.has(key)).toBe(true); + for (const key of workflowLanes) { + expect(allKeys.has(key)).toBe(false); } }); diff --git a/packages/core/src/__tests__/store-settings.test.ts b/packages/core/src/__tests__/store-settings.test.ts index 95fcf07a7f..a84b0fbc9f 100644 --- a/packages/core/src/__tests__/store-settings.test.ts +++ b/packages/core/src/__tests__/store-settings.test.ts @@ -124,150 +124,71 @@ describe("TaskStore", () => { // ── Planning/Validator Model Settings ──────────────────────────── - describe("planning/validator model settings", () => { - it("saves and restores planning model settings via updateSettings", async () => { + // U4 hard-move: planning/validator (and execution/titleSummarizer) PROJECT model + // lanes MOVED to workflow settings. `updateSettings` now DROPS them (R8); their + // persistence/precedence is covered by the workflow-settings + settings-migration + // suites. This block asserts the new drop behavior at the project-settings layer. + describe("planning/validator model settings (moved to workflow settings)", () => { + it("drops planning model settings from project settings (not persisted)", async () => { await harness.store().updateSettings({ planningProvider: "anthropic", planningModelId: "claude-sonnet-4-5", }); const settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - }); - - it("saves and restores validator model settings via updateSettings", async () => { - await harness.store().updateSettings({ - validatorProvider: "openai", - validatorModelId: "gpt-4o", - }); - const settings = await harness.store().getSettings(); - expect(settings.validatorProvider).toBe("openai"); - expect(settings.validatorModelId).toBe("gpt-4o"); - }); - - it("saves and restores both planning and validator model settings via updateSettings", async () => { - await harness.store().updateSettings({ - planningProvider: "anthropic", - planningModelId: "claude-sonnet-4-5", - validatorProvider: "openai", - validatorModelId: "gpt-4o", - }); - const settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - expect(settings.validatorProvider).toBe("openai"); - expect(settings.validatorModelId).toBe("gpt-4o"); - }); - - it("clears planning model settings when set to undefined", async () => { - await harness.store().updateSettings({ - planningProvider: "anthropic", - planningModelId: "claude-sonnet-4-5", - }); - await harness.store().updateSettings({ - planningProvider: undefined, - planningModelId: undefined, - }); - const settings = await harness.store().getSettings(); expect(settings.planningProvider).toBeUndefined(); expect(settings.planningModelId).toBeUndefined(); + + const config = JSON.parse(await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8")); + expect((config.settings as any).planningProvider).toBeUndefined(); + expect((config.settings as any).planningModelId).toBeUndefined(); }); - it("clears validator model settings when set to undefined", async () => { + it("drops validator model settings from project settings (not persisted)", async () => { await harness.store().updateSettings({ validatorProvider: "openai", validatorModelId: "gpt-4o", }); - await harness.store().updateSettings({ - validatorProvider: undefined, - validatorModelId: undefined, - }); const settings = await harness.store().getSettings(); expect(settings.validatorProvider).toBeUndefined(); expect(settings.validatorModelId).toBeUndefined(); }); - it("persists planning/validator settings in project config", async () => { + it("drops both planning and validator model settings together", async () => { await harness.store().updateSettings({ planningProvider: "anthropic", - planningModelId: "claude-opus-4", + planningModelId: "claude-sonnet-4-5", validatorProvider: "openai", - validatorModelId: "gpt-4-turbo", + validatorModelId: "gpt-4o", }); - - // Verify the settings are in the project config file - const configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - const config = JSON.parse(configRaw); - expect(config.settings.planningProvider).toBe("anthropic"); - expect(config.settings.planningModelId).toBe("claude-opus-4"); - expect(config.settings.validatorProvider).toBe("openai"); - expect(config.settings.validatorModelId).toBe("gpt-4-turbo"); + const settings = await harness.store().getSettings(); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); }); }); // ── Dual-Scope Lane Model Settings (FN-1710) ───────────────────── describe("dual-scope lane model settings", () => { - // Legacy backward compatibility tests - it("legacy: project config with only planningProvider/planningModelId round-trips unchanged", async () => { + // U4 hard-move: the per-phase PROJECT lanes are dropped by updateSettings. + it("moved project lanes are dropped, not round-tripped through project config", async () => { await harness.store().updateSettings({ planningProvider: "anthropic", planningModelId: "claude-sonnet-4-5", - }); - - const settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - - // Verify it's persisted correctly - const configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - const config = JSON.parse(configRaw); - expect(config.settings.planningProvider).toBe("anthropic"); - expect(config.settings.planningModelId).toBe("claude-sonnet-4-5"); - }); - - it("legacy: project config with only validatorProvider/validatorModelId round-trips unchanged", async () => { - await harness.store().updateSettings({ validatorProvider: "openai", validatorModelId: "gpt-4o", - }); - - const settings = await harness.store().getSettings(); - expect(settings.validatorProvider).toBe("openai"); - expect(settings.validatorModelId).toBe("gpt-4o"); - - const configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - const config = JSON.parse(configRaw); - expect(config.settings.validatorProvider).toBe("openai"); - expect(config.settings.validatorModelId).toBe("gpt-4o"); - }); - - it("legacy: project config with only titleSummarizerProvider/titleSummarizerModelId round-trips unchanged", async () => { - await harness.store().updateSettings({ titleSummarizerProvider: "google", titleSummarizerModelId: "gemini-2.5-pro", }); const settings = await harness.store().getSettings(); - expect(settings.titleSummarizerProvider).toBe("google"); - expect(settings.titleSummarizerModelId).toBe("gemini-2.5-pro"); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.titleSummarizerProvider).toBeUndefined(); - const configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - const config = JSON.parse(configRaw); - expect(config.settings.titleSummarizerProvider).toBe("google"); - expect(config.settings.titleSummarizerModelId).toBe("gemini-2.5-pro"); - }); - - it("legacy: partial provider without modelId behaves correctly", async () => { - // Set provider only without modelId (partial legacy pair) - await harness.store().updateSettings({ - planningProvider: "anthropic", - // No planningModelId - }); - - const settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBeUndefined(); + const config = JSON.parse(await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8")); + expect((config.settings as any).planningProvider).toBeUndefined(); + expect((config.settings as any).validatorProvider).toBeUndefined(); + expect((config.settings as any).titleSummarizerProvider).toBeUndefined(); }); // New default override fields @@ -300,26 +221,26 @@ describe("TaskStore", () => { }); // New execution lane fields - it("persists executionProvider/executionModelId via updateSettings", async () => { + it("executionProvider/executionModelId are DROPPED from project settings (moved)", async () => { await harness.store().updateSettings({ executionProvider: "anthropic", executionModelId: "claude-opus-4", }); const settings = await harness.store().getSettings(); - expect(settings.executionProvider).toBe("anthropic"); - expect(settings.executionModelId).toBe("claude-opus-4"); + expect(settings.executionProvider).toBeUndefined(); + expect(settings.executionModelId).toBeUndefined(); }); - it("executionProvider/executionModelId appear in project scope", async () => { + it("executionProvider/executionModelId never appear in project scope (moved)", async () => { await harness.store().updateSettings({ executionProvider: "openai", executionModelId: "gpt-4-turbo", }); const { project } = await harness.store().getSettingsByScope(); - expect(project.executionProvider).toBe("openai"); - expect(project.executionModelId).toBe("gpt-4-turbo"); + expect((project as any).executionProvider).toBeUndefined(); + expect((project as any).executionModelId).toBeUndefined(); }); it("executionProvider/executionModelId default to undefined", async () => { @@ -399,12 +320,12 @@ describe("TaskStore", () => { planningModelId: "gpt-4o", }); - // Both should be readable with no crashes + // Global lane stays; project lane is MOVED → dropped. const settings = await harness.store().getSettings(); expect(settings.planningGlobalProvider).toBe("anthropic"); expect(settings.planningGlobalModelId).toBe("claude-sonnet-4-5"); - expect(settings.planningProvider).toBe("openai"); - expect(settings.planningModelId).toBe("gpt-4o"); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningModelId).toBeUndefined(); }); it("mixed shape: project validatorProvider + global validatorGlobalProvider is stable", async () => { @@ -421,8 +342,8 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); expect(settings.validatorGlobalProvider).toBe("google"); expect(settings.validatorGlobalModelId).toBe("gemini-2.5-pro"); - expect(settings.validatorProvider).toBe("anthropic"); - expect(settings.validatorModelId).toBe("claude-opus-4"); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.validatorModelId).toBeUndefined(); }); it("mixed shape: project titleSummarizerProvider + global titleSummarizerGlobalProvider is stable", async () => { @@ -439,8 +360,8 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); expect(settings.titleSummarizerGlobalProvider).toBe("openai"); expect(settings.titleSummarizerGlobalModelId).toBe("gpt-4o-mini"); - expect(settings.titleSummarizerProvider).toBe("anthropic"); - expect(settings.titleSummarizerModelId).toBe("claude-haiku"); + expect(settings.titleSummarizerProvider).toBeUndefined(); + expect(settings.titleSummarizerModelId).toBeUndefined(); }); // Global-only key filtering tests @@ -496,22 +417,46 @@ describe("TaskStore", () => { describe("model lane persistence regression", () => { // Table-driven test matrix: verifies all model lane fields persist correctly // Fields are split by their correct scope (global or project) + // U4 hard-move: the per-PHASE project lanes (execution/planning/validator/ + // titleSummarizer + fallbacks) MOVED to workflow settings and no longer + // persist through `updateSettings` (the stale-writer guard drops them). They + // are covered by the workflow-settings store + settings-migration suites. + // Only `defaultProviderOverride`/`defaultModelIdOverride` remain project-scoped. const projectModelLanePairs = [ - // Execution lane (project override) - { provider: "executionProvider", modelId: "executionModelId" }, - // Planning lane (project override + fallback) - { provider: "planningProvider", modelId: "planningModelId" }, - { provider: "planningFallbackProvider", modelId: "planningFallbackModelId" }, - // Validator lane (project override + fallback) - { provider: "validatorProvider", modelId: "validatorModelId" }, - { provider: "validatorFallbackProvider", modelId: "validatorFallbackModelId" }, - // Summarizer lane (project override + fallback) - { provider: "titleSummarizerProvider", modelId: "titleSummarizerModelId" }, - { provider: "titleSummarizerFallbackProvider", modelId: "titleSummarizerFallbackModelId" }, - // Default override (project-level override of global defaults) + // Default override (project-level override of global defaults) — NOT moved. { provider: "defaultProviderOverride", modelId: "defaultModelIdOverride" }, ] as const; + // The moved lanes, asserted to be DROPPED from project settings (R8). + const movedProjectModelLanePairs = [ + { provider: "executionProvider", modelId: "executionModelId" }, + { provider: "planningProvider", modelId: "planningModelId" }, + { provider: "planningFallbackProvider", modelId: "planningFallbackModelId" }, + { provider: "validatorProvider", modelId: "validatorModelId" }, + { provider: "validatorFallbackProvider", modelId: "validatorFallbackModelId" }, + { provider: "titleSummarizerProvider", modelId: "titleSummarizerModelId" }, + { provider: "titleSummarizerFallbackProvider", modelId: "titleSummarizerFallbackModelId" }, + ] as const; + + it.each(movedProjectModelLanePairs)( + "moved lane $provider/$modelId is DROPPED from project settings (U4 hard-move)", + async ({ provider, modelId }) => { + const patch: Record = {}; + patch[provider] = "anthropic"; + patch[modelId] = "claude-opus-4"; + await harness.store().updateSettings(patch); + + const settings = await harness.store().getSettings(); + expect((settings as any)[provider]).toBeUndefined(); + expect((settings as any)[modelId]).toBeUndefined(); + + const configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); + const config = JSON.parse(configRaw); + expect((config.settings as any)[provider]).toBeUndefined(); + expect((config.settings as any)[modelId]).toBeUndefined(); + }, + ); + const globalModelLanePairs = [ // Default baseline { provider: "defaultProvider", modelId: "defaultModelId" }, @@ -740,13 +685,11 @@ describe("TaskStore", () => { planningGlobalModelId: "claude-sonnet-4-5", }); + // U4 hard-move: the per-phase project lanes are dropped; use a remaining + // project-scoped key (defaultProviderOverride) for the project-scope side. await harness.store().updateSettings({ - planningProvider: "anthropic", - planningModelId: "claude-opus-4", - planningFallbackProvider: "openai", - planningFallbackModelId: "gpt-4o-mini", - executionProvider: "google", - executionModelId: "gemini-2.5-pro", + defaultProviderOverride: "anthropic", + defaultModelIdOverride: "claude-opus-4", }); const { global, project } = await harness.store().getSettingsByScope(); @@ -759,20 +702,15 @@ describe("TaskStore", () => { expect(global.planningGlobalProvider).toBe("anthropic"); expect(global.planningGlobalModelId).toBe("claude-sonnet-4-5"); - // Project scope - expect(project.planningProvider).toBe("anthropic"); - expect(project.planningModelId).toBe("claude-opus-4"); - expect(project.planningFallbackProvider).toBe("openai"); - expect(project.planningFallbackModelId).toBe("gpt-4o-mini"); - expect(project.executionProvider).toBe("google"); - expect(project.executionModelId).toBe("gemini-2.5-pro"); + // Project scope (remaining, non-moved keys) + expect(project.defaultProviderOverride).toBe("anthropic"); + expect(project.defaultModelIdOverride).toBe("claude-opus-4"); - // Verify no cross-contamination - expect((global as any).planningProvider).toBeUndefined(); - expect((global as any).planningFallbackProvider).toBeUndefined(); - expect((global as any).executionProvider).toBeUndefined(); + // Verify no cross-contamination + moved lanes never resurface in project scope expect((project as any).planningGlobalProvider).toBeUndefined(); expect((project as any).defaultProvider).toBeUndefined(); + expect((project as any).planningProvider).toBeUndefined(); + expect((project as any).executionProvider).toBeUndefined(); }); }); @@ -893,24 +831,25 @@ describe("TaskStore", () => { expect(settings.fallbackModelId).toBe("gpt-4o"); expect(settings.planningGlobalProvider).toBe("google"); expect(settings.planningGlobalModelId).toBe("gemini-2.5-pro"); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - expect(settings.planningFallbackProvider).toBe("openai"); - expect(settings.planningFallbackModelId).toBe("gpt-4o-mini"); expect(settings.executionGlobalProvider).toBe("anthropic"); expect(settings.executionGlobalModelId).toBe("claude-opus-4"); - expect(settings.executionProvider).toBe("google"); - expect(settings.executionModelId).toBe("gemini-2.5-pro"); - expect(settings.validatorProvider).toBe("anthropic"); - expect(settings.validatorModelId).toBe("claude-opus-4"); - expect(settings.validatorFallbackProvider).toBe("openai"); - expect(settings.validatorFallbackModelId).toBe("gpt-4o"); - expect(settings.titleSummarizerProvider).toBe("google"); - expect(settings.titleSummarizerModelId).toBe("gemini-2.5-pro"); expect(settings.titleSummarizerGlobalProvider).toBe("anthropic"); expect(settings.titleSummarizerGlobalModelId).toBe("claude-haiku"); - expect(settings.titleSummarizerFallbackProvider).toBe("anthropic"); - expect(settings.titleSummarizerFallbackModelId).toBe("claude-haiku"); + // U4 hard-move: the per-phase PROJECT lanes are dropped by updateSettings. + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningModelId).toBeUndefined(); + expect(settings.planningFallbackProvider).toBeUndefined(); + expect(settings.planningFallbackModelId).toBeUndefined(); + expect(settings.executionProvider).toBeUndefined(); + expect(settings.executionModelId).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.validatorModelId).toBeUndefined(); + expect(settings.validatorFallbackProvider).toBeUndefined(); + expect(settings.validatorFallbackModelId).toBeUndefined(); + expect(settings.titleSummarizerProvider).toBeUndefined(); + expect(settings.titleSummarizerModelId).toBeUndefined(); + expect(settings.titleSummarizerFallbackProvider).toBeUndefined(); + expect(settings.titleSummarizerFallbackModelId).toBeUndefined(); }); }); @@ -932,9 +871,11 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); - // Project pair should win - expect(settings.planningProvider).toBe("openai"); - expect(settings.planningModelId).toBe("gpt-4o"); + // U4 hard-move: project lane no longer persists in project settings; the + // project-vs-global precedence now resolves through workflow effective + // settings (covered by the workflow-settings/migration suites). + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningModelId).toBeUndefined(); // Global should still be readable expect(settings.planningGlobalProvider).toBe("anthropic"); @@ -996,9 +937,9 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); - // Project override should win - expect(settings.executionProvider).toBe("openai"); - expect(settings.executionModelId).toBe("gpt-4o"); + // U4 hard-move: execution project lane dropped from project settings. + expect(settings.executionProvider).toBeUndefined(); + expect(settings.executionModelId).toBeUndefined(); // Global should still be accessible expect(settings.executionGlobalProvider).toBe("google"); @@ -1050,31 +991,23 @@ describe("TaskStore", () => { expect(settings.fallbackProvider).toBe("openai"); expect(settings.fallbackModelId).toBe("gpt-4o"); - expect(settings.executionProvider).toBe("openai"); - expect(settings.executionModelId).toBe("gpt-4o-mini"); + // Global lanes stay; U4 hard-move drops every per-phase PROJECT lane. expect(settings.executionGlobalProvider).toBe("google"); expect(settings.executionGlobalModelId).toBe("gemini-2.5-pro"); - - expect(settings.planningProvider).toBe("google"); - expect(settings.planningModelId).toBe("gemini-2.5-flash"); expect(settings.planningGlobalProvider).toBe("anthropic"); expect(settings.planningGlobalModelId).toBe("claude-opus-4"); - expect(settings.planningFallbackProvider).toBe("anthropic"); - expect(settings.planningFallbackModelId).toBe("claude-sonnet-4-5"); - - expect(settings.validatorProvider).toBe("google"); - expect(settings.validatorModelId).toBe("gemini-2.5-pro"); expect(settings.validatorGlobalProvider).toBe("openai"); expect(settings.validatorGlobalModelId).toBe("gpt-4-turbo"); - expect(settings.validatorFallbackProvider).toBe("anthropic"); - expect(settings.validatorFallbackModelId).toBe("claude-opus-4"); - - expect(settings.titleSummarizerProvider).toBe("openai"); - expect(settings.titleSummarizerModelId).toBe("gpt-4o"); expect(settings.titleSummarizerGlobalProvider).toBe("anthropic"); expect(settings.titleSummarizerGlobalModelId).toBe("claude-haiku"); - expect(settings.titleSummarizerFallbackProvider).toBe("google"); - expect(settings.titleSummarizerFallbackModelId).toBe("gemini-2.5-flash"); + + expect(settings.executionProvider).toBeUndefined(); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningFallbackProvider).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.validatorFallbackProvider).toBeUndefined(); + expect(settings.titleSummarizerProvider).toBeUndefined(); + expect(settings.titleSummarizerFallbackProvider).toBeUndefined(); }); }); @@ -1096,11 +1029,11 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); - // Both should coexist + // Global lane stays; U4 drops the project lane. expect(settings.executionGlobalProvider).toBe("anthropic"); expect(settings.executionGlobalModelId).toBe("claude-sonnet-4-5"); - expect(settings.planningProvider).toBe("openai"); - expect(settings.planningModelId).toBe("gpt-4o"); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningModelId).toBeUndefined(); }); it("mixed legacy canonical shapes resolve deterministically", async () => { @@ -1132,17 +1065,12 @@ describe("TaskStore", () => { const settings = await harness.store().getSettings(); - // Legacy shapes preserved - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - expect(settings.validatorProvider).toBe("openai"); - expect(settings.validatorModelId).toBe("gpt-4o"); - expect(settings.titleSummarizerProvider).toBe("google"); - expect(settings.titleSummarizerModelId).toBe("gemini-2.5-pro"); - - // Canonical shapes preserved - expect(settings.executionProvider).toBe("anthropic"); - expect(settings.executionModelId).toBe("claude-opus-4"); + // U4 hard-move: all per-phase PROJECT lanes are dropped from project settings. + expect(settings.planningProvider).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.titleSummarizerProvider).toBeUndefined(); + expect(settings.executionProvider).toBeUndefined(); + expect(settings.executionModelId).toBeUndefined(); // Global canonical shapes preserved expect(settings.planningGlobalProvider).toBe("anthropic"); @@ -1151,66 +1079,43 @@ describe("TaskStore", () => { expect(settings.validatorGlobalModelId).toBe("gpt-4o-mini"); }); - it("legacy format: planningProvider without planningModelId is valid partial pair", async () => { - await harness.store().updateSettings({ - planningProvider: "anthropic", - // planningModelId intentionally omitted - }); - + // U4 hard-move: partial/full PROJECT lane writes are dropped — they no longer + // persist in project settings. (Workflow-setting partial-pair semantics are + // covered by the workflow-settings suite.) + it("moved project lane: planningProvider without planningModelId is dropped", async () => { + await harness.store().updateSettings({ planningProvider: "anthropic" }); const settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); + expect(settings.planningProvider).toBeUndefined(); expect(settings.planningModelId).toBeUndefined(); }); - it("legacy format: validatorProvider without validatorModelId is valid partial pair", async () => { - await harness.store().updateSettings({ - validatorProvider: "openai", - // validatorModelId intentionally omitted - }); - + it("moved project lane: validatorProvider without validatorModelId is dropped", async () => { + await harness.store().updateSettings({ validatorProvider: "openai" }); const settings = await harness.store().getSettings(); - expect(settings.validatorProvider).toBe("openai"); + expect(settings.validatorProvider).toBeUndefined(); expect(settings.validatorModelId).toBeUndefined(); }); - it("canonical format: executionProvider without executionModelId is valid partial pair", async () => { - await harness.store().updateSettings({ - executionProvider: "google", - // executionModelId intentionally omitted - }); - + it("moved project lane: executionProvider without executionModelId is dropped", async () => { + await harness.store().updateSettings({ executionProvider: "google" }); const settings = await harness.store().getSettings(); - expect(settings.executionProvider).toBe("google"); + expect(settings.executionProvider).toBeUndefined(); expect(settings.executionModelId).toBeUndefined(); }); - it("mixed: full pair + partial pair coexist in same lane", async () => { - // Set full planning pair + it("moved project lanes: full + partial writes all drop from project settings", async () => { await harness.store().updateSettings({ planningProvider: "anthropic", planningModelId: "claude-sonnet-4-5", - }); - - // Set partial validator pair (only provider) - await harness.store().updateSettings({ validatorProvider: "openai", - // validatorModelId intentionally omitted - }); - - // Set full execution pair - await harness.store().updateSettings({ executionProvider: "google", executionModelId: "gemini-2.5-pro", }); const settings = await harness.store().getSettings(); - - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); - expect(settings.validatorProvider).toBe("openai"); - expect(settings.validatorModelId).toBeUndefined(); - expect(settings.executionProvider).toBe("google"); - expect(settings.executionModelId).toBe("gemini-2.5-pro"); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.validatorProvider).toBeUndefined(); + expect(settings.executionProvider).toBeUndefined(); }); }); @@ -1222,9 +1127,11 @@ describe("TaskStore", () => { planningModelId: "claude-sonnet-4-5", }); + // U4 hard-move: the project lane never persists (dropped on write), so it is + // already undefined; a subsequent null-clear is a harmless no-op. let settings = await harness.store().getSettings(); - expect(settings.planningProvider).toBe("anthropic"); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); + expect(settings.planningProvider).toBeUndefined(); + expect(settings.planningModelId).toBeUndefined(); // Clear with null // @ts-expect-error - null is intentionally used to clear field (null-as-delete) @@ -1392,8 +1299,10 @@ describe("TaskStore", () => { await harness.store().updateSettings({ planningProvider: null }); const settings = await harness.store().getSettings(); + // U4 hard-move: both moved-lane fields are dropped on the initial write, so + // neither persists in project settings. expect(settings.planningProvider).toBeUndefined(); - expect(settings.planningModelId).toBe("claude-sonnet-4-5"); // Preserved + expect(settings.planningModelId).toBeUndefined(); }); it("cleared model settings fall back to undefined (not default values)", async () => { @@ -1426,26 +1335,23 @@ describe("TaskStore", () => { expect(settings.defaultModelId).toBe("claude-sonnet-4-5"); }); - it("cleared model settings removed from persisted config", async () => { + it("moved model settings are never persisted to config (dropped on write)", async () => { await harness.store().updateSettings({ planningProvider: "anthropic", planningModelId: "claude-sonnet-4-5", }); - // Verify persisted - let configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - let config = JSON.parse(configRaw); - expect((config.settings as any).planningProvider).toBe("anthropic"); + // U4 hard-move: never persisted to project config in the first place. + let config = JSON.parse(await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8")); + expect((config.settings as any).planningProvider).toBeUndefined(); - // Clear with null + // Null-clear is a harmless no-op; still absent. // @ts-expect-error - null is intentionally used to clear field (null-as-delete) await harness.store().updateSettings({ planningProvider: null }); // @ts-expect-error - null is intentionally used to clear field (null-as-delete) await harness.store().updateSettings({ planningModelId: null }); - // Verify removed from persisted config - configRaw = await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8"); - config = JSON.parse(configRaw); + config = JSON.parse(await readFile(join(harness.rootDir(), ".fusion", "config.json"), "utf-8")); expect((config.settings as any).planningProvider).toBeUndefined(); expect((config.settings as any).planningModelId).toBeUndefined(); }); diff --git a/packages/core/src/__tests__/task-creation-hook.test.ts b/packages/core/src/__tests__/task-creation-hook.test.ts index 4f0668a325..0558893eff 100644 --- a/packages/core/src/__tests__/task-creation-hook.test.ts +++ b/packages/core/src/__tests__/task-creation-hook.test.ts @@ -247,8 +247,11 @@ describe("task creation hook", () => { summarizeTitleMock.mockResolvedValue("Auto Generated Title"); setTaskCreatedHook(hook); - await store.updateSettings({ - autoSummarizeTitles: true, + // autoSummarizeTitles stays a project setting; the summarizer model lanes + // MOVED to workflow settings (U4/KTD-7), so write them to the project's + // default workflow (builtin:coding) value store. + await store.updateSettings({ autoSummarizeTitles: true }); + await store.updateWorkflowSettingValues("builtin:coding", store.getWorkflowSettingsProjectId(), { titleSummarizerProvider: "openai", titleSummarizerModelId: "gpt-5-mini", }); diff --git a/packages/core/src/__tests__/workflow-ir-settings.test.ts b/packages/core/src/__tests__/workflow-ir-settings.test.ts index a3ce1c3692..64db696f6b 100644 --- a/packages/core/src/__tests__/workflow-ir-settings.test.ts +++ b/packages/core/src/__tests__/workflow-ir-settings.test.ts @@ -216,14 +216,49 @@ describe("built-in workflow settings parity anchor (U1, R4)", () => { expect(builtin.settings).toEqual(BUILTIN_WORKFLOW_SETTINGS); }); - it("each declaration default strictly equals the legacy DEFAULT_PROJECT_SETTINGS literal", () => { + it("the moved-key catalog has left DEFAULT_PROJECT_SETTINGS (U4 hard-move) and pins its legacy defaults", () => { const legacy = DEFAULT_PROJECT_SETTINGS as Record; + // Post-U4 hard-move: every catalog key has been REMOVED from + // DEFAULT_PROJECT_SETTINGS (the type-vs-schema split keeps the type field but + // drops the default literal), so the legacy object no longer carries them. for (const setting of BUILTIN_WORKFLOW_SETTINGS) { - // Catalog keys must exist as a known project-settings key. - expect(Object.prototype.hasOwnProperty.call(legacy, setting.id)).toBe(true); - // A declared default must byte-equal the legacy literal; an omitted - // default corresponds to a legacy `undefined` literal. - expect(setting.default).toStrictEqual(legacy[setting.id]); + expect(Object.prototype.hasOwnProperty.call(legacy, setting.id)).toBe(false); + } + // The declaration defaults are now the single source of truth; pin the legacy + // values explicitly so they can never silently drift from what they were when + // they lived in DEFAULT_PROJECT_SETTINGS. + const expectedDefaults: Record = { + workflowStepTimeoutMs: 360_000, + workflowStepScopeEnforcement: "block", + planOnlyScopeLeakEnforcement: "warn", + workflowRevisionForkOnScopeMismatch: true, + strictScopeEnforcement: false, + runStepsInNewSessions: false, + maxParallelSteps: 2, + buildRetryCount: 0, + verificationFixRetries: 3, + maxPostReviewFixes: 1, + requirePrApproval: false, + requirePlanApproval: false, + reviewHandoffPolicy: "disabled", + maxReviewerContextRetries: 2, + maxReviewerFallbackRetries: 2, + reflectionEnabled: false, + // Per-phase model lanes have undefined legacy defaults → declaration omits default. + }; + for (const setting of BUILTIN_WORKFLOW_SETTINGS) { + if (Object.prototype.hasOwnProperty.call(expectedDefaults, setting.id)) { + expect(setting.default).toStrictEqual(expectedDefaults[setting.id]); + } else { + // Model-lane keys: no default. + expect(setting.default).toBeUndefined(); + } } }); + + it("buildTimeoutMs is NOT in the catalog and stays a plain project setting", () => { + const declaredIds = new Set(BUILTIN_WORKFLOW_SETTINGS.map((s) => s.id)); + expect(declaredIds.has("buildTimeoutMs")).toBe(false); + expect((DEFAULT_PROJECT_SETTINGS as Record).buildTimeoutMs).toBe(300_000); + }); }); diff --git a/packages/core/src/__tests__/workflow-settings-resolver.test.ts b/packages/core/src/__tests__/workflow-settings-resolver.test.ts index 69a4332016..3841b8b5ef 100644 --- a/packages/core/src/__tests__/workflow-settings-resolver.test.ts +++ b/packages/core/src/__tests__/workflow-settings-resolver.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import { BUILTIN_WORKFLOW_SETTINGS } from "../builtin-workflow-settings.js"; -import { DEFAULT_PROJECT_SETTINGS } from "../types.js"; import type { WorkflowIr } from "../workflow-ir-types.js"; import { resolveEffectiveSettings, @@ -59,19 +58,21 @@ function makeStore(opts: { } describe("resolveEffectiveSettings (per-task)", () => { - it("parity anchor: builtin:coding with no stored values → declaration defaults equal legacy defaults", async () => { + it("parity anchor: builtin:coding with no stored values → effective equals declaration defaults", async () => { const store = makeStore({ selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, }); const eff = await resolveEffectiveSettings(store, { id: "t1" }); - // Every catalog key with a default equals the legacy DEFAULT_PROJECT_SETTINGS literal. - const legacy = DEFAULT_PROJECT_SETTINGS as Record; + // Every catalog key with a default contributes its declaration default to the + // effective map. (Post-U4 hard-move the legacy DEFAULT_PROJECT_SETTINGS literals + // for these keys are GONE — the declaration default is now the single source of + // truth, byte-equal to what the legacy literal used to be.) for (const s of BUILTIN_WORKFLOW_SETTINGS) { if (s.default === undefined) { // Absent-default lanes contribute nothing to the effective map. expect(Object.prototype.hasOwnProperty.call(eff, s.id)).toBe(false); } else { - expect(eff[s.id]).toStrictEqual(legacy[s.id]); + expect(eff[s.id]).toStrictEqual(s.default); } } }); diff --git a/packages/core/src/builtin-workflow-settings.ts b/packages/core/src/builtin-workflow-settings.ts index cbef7af4e3..423a6554d7 100644 --- a/packages/core/src/builtin-workflow-settings.ts +++ b/packages/core/src/builtin-workflow-settings.ts @@ -91,13 +91,11 @@ export const BUILTIN_WORKFLOW_SETTINGS: WorkflowSettingDefinition[] = [ default: 0, description: "Number of times to retry a failing build before giving up.", }, - { - id: "buildTimeoutMs", - name: "Build timeout (ms)", - type: "number", - default: 300_000, - description: "Maximum time a build command may run before it is timed out.", - }, + // NOTE (U4 catalog-shrink): `buildTimeoutMs` was REMOVED from this catalog — + // it has NO reader anywhere in the engine, so per the per-task-reader rule + // (KTD-5) it stays a plain project setting and is NOT moved to workflow + // settings. It is therefore absent from `MOVED_SETTINGS_KEYS` and remains in + // `DEFAULT_PROJECT_SETTINGS`. { id: "verificationFixRetries", name: "Verification fix retries", diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 085f75e5d8..2a36abb1f9 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -81,6 +81,14 @@ export type { export { BUILTIN_CODING_WORKFLOW_IR } from "./builtin-coding-workflow-ir.js"; export { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "./builtin-stepwise-coding-workflow-ir.js"; export { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; +export { + MOVED_SETTINGS_KEYS, + SETTINGS_MIGRATION_VERSION, + SETTINGS_MIGRATION_MARKER_KEY, + isMovedSettingsKey, + stripMovedSettingsKeys, + patchContainsMovedKey, +} from "./moved-settings.js"; // ── Trait model (U2) ───────────────────────────────────────────────── export type { diff --git a/packages/core/src/moved-settings.ts b/packages/core/src/moved-settings.ts new file mode 100644 index 0000000000..d0f7de9aed --- /dev/null +++ b/packages/core/src/moved-settings.ts @@ -0,0 +1,89 @@ +/** + * Tombstone allowlist for the U4 hard-move (KTD-5). + * + * `MOVED_SETTINGS_KEYS` is the single, authoritative record of the settings keys + * that left `DEFAULT_PROJECT_SETTINGS` and now live exclusively as **workflow + * setting values** per `(workflowId, projectId)`. It is derived directly from the + * built-in workflow declaration catalog (`BUILTIN_WORKFLOW_SETTINGS`) so the move + * has exactly one source of truth — a key is "moved" iff a built-in workflow + * declares it. Adding/removing a key from the catalog automatically reflows the + * tombstone list, the migration write target, and the stale-writer guard. + * + * What the tombstone shields (KTD-5, R8): + * - the project/global settings WRITE paths (`updateSettings` / + * `updateGlobalSettings`) — incoming moved keys from stale writers are silently + * dropped, never persisted (they would otherwise re-materialize in raw + * storage and, via the default re-injection trap, silently override the + * migrated workflow value); + * - the migration's raw-key null-out (it nulls exactly these keys from the + * persisted project + global stores); + * - (in U5) settings export v2 / cross-node sync diff / v1 import. + * + * ── TYPE-vs-SCHEMA SPLIT (deliberate, documented per the U4 plan) ────────────── + * The moved keys are REMOVED from `DEFAULT_PROJECT_SETTINGS` (so they vanish from + * `PROJECT_SETTINGS_KEYS` / `isProjectSettingsKey` / the save-split), but the + * corresponding fields are RETAINED on the `ProjectSettings` / `Settings` + * TypeScript interfaces. This is intentional: the engine still types its ~20 flat + * `settings.` read sites and the U3 effective-settings merge off + * `Partial`, so dropping the fields from the type would break those + * call sites. The schema MEMBERSHIP (key lists / predicates / persistence + * filters) is the thing that must not include moved keys — not the type shape. + * + * NOTE on `buildTimeoutMs`: it has NO reader anywhere in the engine, so it fails + * the per-task-reader rule (KTD-5 / catalog-shrink) and was removed from + * `BUILTIN_WORKFLOW_SETTINGS` entirely. It therefore stays a plain project + * setting and is intentionally ABSENT from this list. + */ + +import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; + +/** + * The version of the per-project settings hard-move migration. Persisted per + * project as a `__meta` marker (`settingsMigrationVersion`). A project whose + * marker is `>= SETTINGS_MIGRATION_VERSION` has already migrated and the runner + * no-ops. Bump only if a future migration must re-run on already-migrated DBs. + */ +export const SETTINGS_MIGRATION_VERSION = 1; + +/** The `__meta` key under which the migration marker is persisted (per project DB). */ +export const SETTINGS_MIGRATION_MARKER_KEY = "settingsMigrationVersion"; + +/** + * The definitive moved-key catalog — derived from the built-in workflow + * declarations so it cannot drift from them. Frozen so callers cannot mutate it. + */ +export const MOVED_SETTINGS_KEYS: readonly string[] = Object.freeze( + BUILTIN_WORKFLOW_SETTINGS.map((s) => s.id), +); + +/** Set form for O(1) membership checks on the hot write path. */ +const MOVED_SETTINGS_KEY_SET: ReadonlySet = new Set(MOVED_SETTINGS_KEYS); + +/** Whether `key` is a moved (tombstoned) settings key. */ +export function isMovedSettingsKey(key: string): boolean { + return MOVED_SETTINGS_KEY_SET.has(key); +} + +/** + * Return a shallow copy of `patch` with every moved (tombstoned) key removed. + * Used by the project/global settings write paths to silently drop moved keys + * arriving from stale writers (R8) — they must never be persisted back into the + * raw settings store. Non-moved keys pass through untouched. + */ +export function stripMovedSettingsKeys>(patch: T): Partial { + const out: Record = {}; + for (const [key, value] of Object.entries(patch)) { + if (!MOVED_SETTINGS_KEY_SET.has(key)) { + out[key] = value; + } + } + return out as Partial; +} + +/** Whether `patch` carries at least one moved key (for debug-logging the drop). */ +export function patchContainsMovedKey(patch: Record): boolean { + for (const key of Object.keys(patch)) { + if (MOVED_SETTINGS_KEY_SET.has(key)) return true; + } + return false; +} diff --git a/packages/core/src/settings-schema.ts b/packages/core/src/settings-schema.ts index c955e7c5ce..3c35ef9a30 100644 --- a/packages/core/src/settings-schema.ts +++ b/packages/core/src/settings-schema.ts @@ -6,6 +6,50 @@ export interface MergeRequestContractShadowSettingsSource { type CompleteSettings = { [K in keyof Required]: Required[K] | undefined }; +/** + * The settings keys hard-MOVED to workflow settings in U4 (see + * `moved-settings.ts`). They are REMOVED from `DEFAULT_PROJECT_SETTINGS` (so they + * leave `PROJECT_SETTINGS_KEYS` / the save-split), but their FIELDS are retained + * on the `ProjectSettings` type for the engine's flat `settings.` reads and + * the U3 effective-settings merge. `DEFAULT_PROJECT_SETTINGS` is therefore + * type-checked against `ProjectSettings` MINUS these keys — the type-vs-schema + * split documented in `moved-settings.ts`. This union MUST stay in lockstep with + * `MOVED_SETTINGS_KEYS` (the parity/consistency tests enforce coherence). + */ +type MovedProjectSettingsKey = + | "workflowStepTimeoutMs" + | "workflowStepScopeEnforcement" + | "planOnlyScopeLeakEnforcement" + | "workflowRevisionForkOnScopeMismatch" + | "strictScopeEnforcement" + | "runStepsInNewSessions" + | "maxParallelSteps" + | "buildRetryCount" + | "verificationFixRetries" + | "maxPostReviewFixes" + | "requirePrApproval" + | "requirePlanApproval" + | "reviewHandoffPolicy" + | "maxReviewerContextRetries" + | "maxReviewerFallbackRetries" + | "reflectionEnabled" + | "executionProvider" + | "executionModelId" + | "planningProvider" + | "planningModelId" + | "planningFallbackProvider" + | "planningFallbackModelId" + | "validatorProvider" + | "validatorModelId" + | "validatorFallbackProvider" + | "validatorFallbackModelId" + | "titleSummarizerProvider" + | "titleSummarizerModelId" + | "titleSummarizerFallbackProvider" + | "titleSummarizerFallbackModelId"; + +type ProjectSettingsSchema = Omit; + /** * Settings schema source of truth. * @@ -209,7 +253,7 @@ export const DEFAULT_PROJECT_SETTINGS = { mergeIntegrationWorktree: "reuse-task-worktree", mergeAdvanceAutoSync: "stash-and-ff", integrationBranch: undefined, - requirePrApproval: false, + // `requirePrApproval` MOVED to workflow settings (U4) — see MOVED_SETTINGS_KEYS. pushAfterMerge: false, pushRemote: "origin", unavailableNodePolicy: "block", @@ -236,19 +280,12 @@ export const DEFAULT_PROJECT_SETTINGS = { commitAuthorEnabled: true, commitAuthorName: "Fusion", commitAuthorEmail: "noreply@runfusion.ai", - planningProvider: undefined, - planningModelId: undefined, - planningFallbackProvider: undefined, - planningFallbackModelId: undefined, - // Project-level default override and execution lane + // Per-phase model lanes (planning/execution/validator) MOVED to workflow + // settings (U4) — see MOVED_SETTINGS_KEYS. The GLOBAL baseline lanes + // (executionGlobalProvider etc.) stay global; project default overrides stay. + // Project-level default override (NOT moved — stays project-scoped) defaultProviderOverride: undefined, defaultModelIdOverride: undefined, - executionProvider: undefined, - executionModelId: undefined, - validatorProvider: undefined, - validatorModelId: undefined, - validatorFallbackProvider: undefined, - validatorFallbackModelId: undefined, modelPresets: [], autoSelectModelPreset: false, completionDocumentationMode: "off", @@ -283,15 +320,13 @@ export const DEFAULT_PROJECT_SETTINGS = { maxRetries: 3, }, reliabilityStatsResetAt: undefined, - workflowStepTimeoutMs: 360_000, - workflowStepScopeEnforcement: "block", - planOnlyScopeLeakEnforcement: "warn", - workflowRevisionForkOnScopeMismatch: true, - strictScopeEnforcement: false, - buildRetryCount: 0, - verificationFixRetries: 3, + // Step-execution knobs (workflowStepTimeoutMs, workflowStepScopeEnforcement, + // planOnlyScopeLeakEnforcement, workflowRevisionForkOnScopeMismatch, + // strictScopeEnforcement, buildRetryCount, verificationFixRetries, + // requirePlanApproval) MOVED to workflow settings (U4) — see + // MOVED_SETTINGS_KEYS. `buildTimeoutMs` is NOT moved (no engine reader) and + // stays a plain project setting: buildTimeoutMs: 300_000, - requirePlanApproval: false, ephemeralAgentsEnabled: true, agentProvisioning: {}, sandboxProvisioning: {}, @@ -335,11 +370,11 @@ export const DEFAULT_PROJECT_SETTINGS = { autoUnpauseMaxDelayMs: 3_600_000, maxStuckKills: 6, maxBranchConflictRecoveries: 5, - maxReviewerContextRetries: 2, - maxReviewerFallbackRetries: 2, + // maxReviewerContextRetries / maxReviewerFallbackRetries MOVED to workflow + // settings (U4) — see MOVED_SETTINGS_KEYS. maxTotalRetriesBeforeFail: 25, preserveProgressOnStuckRequeue: true, - maxPostReviewFixes: 1, + // maxPostReviewFixes MOVED to workflow settings (U4). maxSpawnedAgentsPerParent: 5, maxSpawnedAgentsGlobal: 20, // Run maintenance (including WAL checkpointing) every 5 minutes by default. @@ -368,10 +403,8 @@ export const DEFAULT_PROJECT_SETTINGS = { memoryBackupScope: "all" as const, autoSummarizeTitles: false, useAiMergeCommitSummary: true, - titleSummarizerProvider: undefined, - titleSummarizerModelId: undefined, - titleSummarizerFallbackProvider: undefined, - titleSummarizerFallbackModelId: undefined, + // Title-summarizer model lanes MOVED to workflow settings (U4) — + // see MOVED_SETTINGS_KEYS. scripts: undefined, setupScript: undefined, insightExtractionEnabled: false, @@ -392,17 +425,19 @@ export const DEFAULT_PROJECT_SETTINGS = { memoryDreamsSchedule: "0 4 * * *", tokenCap: undefined, taskTokenBudget: undefined, - runStepsInNewSessions: false, - maxParallelSteps: 2, + // runStepsInNewSessions / maxParallelSteps MOVED to workflow settings (U4) — + // see MOVED_SETTINGS_KEYS. missionStaleThresholdMs: 600_000, missionMaxTaskRetries: 3, missionHealthCheckIntervalMs: 300_000, agentPrompts: undefined, promptOverrides: undefined, - reflectionEnabled: false, + // reflectionEnabled MOVED to workflow settings (U4). reflectionIntervalMs / + // reflectionAfterTask have no engine reader, so they STAY plain project + // settings (catalog-shrink rule) and are NOT in MOVED_SETTINGS_KEYS. reflectionIntervalMs: 3_600_000, reflectionAfterTask: true, - reviewHandoffPolicy: "disabled", + // reviewHandoffPolicy MOVED to workflow settings (U4) — see MOVED_SETTINGS_KEYS. showQuickChatFAB: false, chatAutoCleanupDays: 0, mailAutoCleanupDays: 0, @@ -451,7 +486,7 @@ export const DEFAULT_PROJECT_SETTINGS = { researchDefaultTimeout: 300000, researchMaxSourcesPerRun: 20, researchMaxSynthesisRounds: 2, -} satisfies CompleteSettings; +} satisfies CompleteSettings; /** * Merged default settings (backward compatible). diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 967a7a3939..4a126a7486 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -7,6 +7,13 @@ import type { Task, TaskDetail, TaskCreateInput, TaskAttachment, AgentLogEntry, import { createActivityLogSnapshot, createRunAuditSnapshot, createTaskMetadataSnapshot, toTaskMetadataRecord, validateSnapshotEnvelope, type ActivityLogSnapshot, type RunAuditSnapshot, type TaskMetadataSnapshot } from "./shared-mesh-state.js"; import { VALID_TRANSITIONS, COLUMNS, DEFAULT_SETTINGS, isGlobalOnlySettingsKey, WORKFLOW_STEP_TEMPLATES, validateDocumentKey } from "./types.js"; import { DEFAULT_PROJECT_SETTINGS } from "./settings-schema.js"; +import { + MOVED_SETTINGS_KEYS, + SETTINGS_MIGRATION_VERSION, + SETTINGS_MIGRATION_MARKER_KEY, + stripMovedSettingsKeys, + patchContainsMovedKey, +} from "./moved-settings.js"; import { parseWorkflowIr, serializeWorkflowIr, downgradeIrToV1IfPure } from "./workflow-ir.js"; import { isWorkflowColumnsEnabled } from "./workflow-columns-settings.js"; import { resolveAllowedColumns, workflowHasColumn } from "./workflow-transitions.js"; @@ -134,6 +141,7 @@ import { validateNodeOverrideChange } from "./node-override-guard.js"; import { sanitizeTitle, summarizeTitle } from "./ai-summarize.js"; import { extractTaskIdTokens, normalizeTitleForTaskId } from "./task-title-id-drift.js"; import { resolveTitleSummarizerSettingsModel } from "./model-resolution.js"; +import { resolveEffectiveSettingsById } from "./workflow-settings-resolver.js"; import { getErrorMessage } from "./error-message.js"; import { getTaskCreatedHook } from "./task-creation-hooks.js"; import { @@ -1579,6 +1587,16 @@ export class TaskStore extends EventEmitter { await this.migrateActiveArchivedTasksToArchiveDb(); await this.migrateAgentLogEntriesToFilesOnce(); await this.cleanupNoOpTaskMovedActivityRowsOnce(); + // U4: one-time per-project hard-move of MOVED_SETTINGS_KEYS into workflow + // setting values (marker-gated, idempotent, never blocks startup). + try { + await this.migrateMovedSettingsToWorkflowValuesOnce(); + } catch (err) { + storeLog.warn("Settings hard-move migration failed during init (non-fatal)", { + phase: "init:settings-hard-move", + error: err instanceof Error ? err.message : String(err), + }); + } // Re-run init when migrations are pending, or when the deferred // agentLogEntries drop still needs to fire: migration 102 skips the // destructive drop until migrateAgentLogEntriesToFilesOnce() above writes @@ -3347,9 +3365,24 @@ export class TaskStore extends EventEmitter { * to the project config. Use `updateGlobalSettings()` for global fields. */ async updateSettings(patch: Partial): Promise { + // Stale-writer guard (U4, R8): moved keys no longer live in project settings — + // they belong to workflow setting values. Drop any moved key arriving from a + // stale writer/import so it is never persisted back into raw storage (where the + // default re-injection trap would silently override the migrated value). + const guardedPatch = + patchContainsMovedKey(patch as Record) + ? (() => { + storeLog.warn("Dropped moved settings keys from project updateSettings patch", { + phase: "updateSettings:moved-key-guard", + dropped: Object.keys(patch).filter((k) => (MOVED_SETTINGS_KEYS as readonly string[]).includes(k)), + }); + return stripMovedSettingsKeys(patch as Record) as Partial; + })() + : patch; + // Filter out global-only fields — they should go through updateGlobalSettings() const projectPatch: Partial = {}; - for (const [key, value] of Object.entries(patch)) { + for (const [key, value] of Object.entries(guardedPatch)) { if (!isGlobalOnlySettingsKey(key)) { (projectPatch as Record)[key] = value; } @@ -3461,7 +3494,12 @@ export class TaskStore extends EventEmitter { const config = this.readConfigFast(); const previous: Settings = { ...DEFAULT_SETTINGS, ...previousGlobal, ...config.settings } as Settings; - const globalPatch: Partial = { ...patch }; + // Stale-writer guard (U4, R8): moved keys are all project-scoped, but null + // them defensively out of the global write path too so a stale writer cannot + // resurrect them in the global store. + const globalPatch: Partial = patchContainsMovedKey(patch as Record) + ? (stripMovedSettingsKeys(patch as Record) as Partial) + : { ...patch }; delete globalPatch.secretsSyncPassphraseConfigured; // Handle deep merge + targeted null clear semantics for remoteAccess @@ -3946,7 +3984,25 @@ export class TaskStore extends EventEmitter { let onSummarize = options?.onSummarize; if (!onSummarize && resolvedSettings?.autoSummarizeTitles === true) { - const summarizerModel = resolveTitleSummarizerSettingsModel(resolvedSettings); + // The title-summarizer model lanes MOVED to workflow settings (U4/KTD-7). + // At task-creation time there is no task/workflow yet, so resolve the + // project DEFAULT workflow's effective settings (unset default normalizes to + // builtin:coding) and overlay them so the moved lane reads from its new home; + // the global `titleSummarizerGlobal*` lane in `resolvedSettings` remains the + // fallback below. + let summarizerSettings: Partial = resolvedSettings ?? {}; + try { + const defaultWorkflowId = (await this.getDefaultWorkflowId()) ?? "builtin:coding"; + const effective = await resolveEffectiveSettingsById( + this, + defaultWorkflowId, + this.getWorkflowSettingsProjectId(), + ); + summarizerSettings = { ...summarizerSettings, ...(effective as Partial) }; + } catch { + // Never-throw: fall back to the base settings (global lane only). + } + const summarizerModel = resolveTitleSummarizerSettingsModel(summarizerSettings); if (summarizerModel.provider && summarizerModel.modelId) { onSummarize = async (description: string) => { try { @@ -11692,6 +11748,228 @@ export class TaskStore extends EventEmitter { }); } + /** + * U4 (R6/R8, KTD-5): one-time, idempotent, per-project hard-move of the + * `MOVED_SETTINGS_KEYS` catalog out of project/global settings and into + * `workflow_settings` values, keyed per `(workflowId, projectId)`. + * + * Gated by the `settingsMigrationVersion` `__meta` marker so it runs exactly + * once per project DB. The sequence (matching the plan's HTD diagram): + * + * 1. Read the RAW persisted project + global settings (the typed read can no + * longer see moved keys post-schema-removal, so read the JSON directly); + * snapshot ONLY the moved keys the user actually CUSTOMIZED (present in raw + * storage) — defaults are not snapshotted (they re-derive from declarations). + * 2. Compute the write target = distinct `task_workflow_selection.workflowId` + * for this project ∪ the resolved project default, where an unset/empty + * `defaultWorkflowId` normalizes to `builtin:coding` (the id every + * selection-less task resolves to). A default pointing at a deleted/missing + * workflow also degrades to `builtin:coding`. + * 3. Validate the snapshot against EACH target workflow's declarations (the + * values came from validated project settings, so this normally passes); a + * value that fails the new validation is DROPPED and logged — never aborts. + * 4. In ONE SQLite transaction: upsert the accepted snapshot into each + * `(workflowId, projectId)` value row, null the moved keys out of the raw + * project `config.settings`, and set the marker. (The async validation / + * declaration resolution happens BEFORE the transaction — the transaction + * body is pure synchronous SQLite, so the persisted writes commit atomically.) + * 5. Defensively null the moved keys out of the global store (outside the txn; + * all moved keys are project-scoped, so this is belt-and-suspenders). + * + * Idempotent / crash-safe: value upserts overwrite identically, the raw null-out + * is re-runnable, and the marker is set LAST inside the transaction. A crash + * between the value-write and the null-out re-runs the whole thing and converges. + */ + private async migrateMovedSettingsToWorkflowValuesOnce(): Promise { + const markerKey = SETTINGS_MIGRATION_MARKER_KEY; + const markerRow = this.db.prepare("SELECT value FROM __meta WHERE key = ?").get(markerKey) as + | { value: string } + | undefined; + if (markerRow && Number(markerRow.value) >= SETTINGS_MIGRATION_VERSION) { + return; + } + + const movedKeys = MOVED_SETTINGS_KEYS as readonly string[]; + const projectId = this.getWorkflowSettingsProjectId(); + + // (1) Snapshot CUSTOMIZED moved keys from RAW persisted project + global stores. + const rawProjectSettings = this.readRawProjectSettings(); + let rawGlobalSettings: Record = {}; + try { + rawGlobalSettings = await this.globalSettingsStore.readRaw(); + } catch { + rawGlobalSettings = {}; + } + const snapshot: Record = {}; + for (const key of movedKeys) { + // Project storage wins over global (moved keys are project-scoped); only + // snapshot keys the user actually customized (present in raw storage). + if (Object.prototype.hasOwnProperty.call(rawProjectSettings, key)) { + snapshot[key] = rawProjectSettings[key]; + } else if (Object.prototype.hasOwnProperty.call(rawGlobalSettings, key)) { + snapshot[key] = rawGlobalSettings[key]; + } + } + + // (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); + + // (3) Validate the snapshot per target workflow (async declaration resolution + // done HERE, before the synchronous transaction). Drop-and-log invalid + // values; never abort. Empty accepted maps are fine (nothing to write). + const acceptedByWorkflow = new Map>(); + if (Object.keys(snapshot).length > 0) { + for (const workflowId of targetWorkflowIds) { + let declarations: WorkflowSettingDefinition[] | undefined; + try { + declarations = await this.resolveWorkflowSettingDeclarations(workflowId); + } catch { + declarations = undefined; + } + const result = validateSettingValuePatch(declarations, snapshot); + if (result.rejections.length > 0) { + storeLog.warn("Dropped invalid moved-setting values during hard-move migration", { + phase: "migrateMovedSettings:validate", + workflowId, + projectId, + rejected: result.rejections.map((r) => `${r.settingId}:${r.code}`), + }); + } + acceptedByWorkflow.set(workflowId, result.accepted); + } + } + + // (4) ONE SQLite transaction: value upserts + raw project null-out + marker. + const now = new Date().toISOString(); + this.db.transactionImmediate(() => { + for (const [workflowId, accepted] of acceptedByWorkflow) { + if (Object.keys(accepted).length === 0) continue; + const current = this.getWorkflowSettingValues(workflowId, projectId); + const next: Record = { ...current }; + for (const [k, v] of Object.entries(accepted)) { + if (v === null || v === undefined) { + delete next[k]; + } else { + next[k] = v; + } + } + this.db + .prepare( + `INSERT INTO workflow_settings (workflowId, projectId, "values", updatedAt) + VALUES (?, ?, ?, ?) + ON CONFLICT(workflowId, projectId) + DO UPDATE SET "values" = excluded."values", updatedAt = excluded.updatedAt`, + ) + .run(workflowId, projectId, JSON.stringify(next), now); + } + + // Null the moved keys out of the raw project config.settings. + const configRow = this.db.prepare("SELECT settings FROM config WHERE id = 1").get() as + | { settings: string } + | undefined; + if (configRow) { + let parsed: Record = {}; + try { + parsed = (JSON.parse(configRow.settings) as Record) ?? {}; + } catch { + parsed = {}; + } + let changed = false; + for (const key of movedKeys) { + if (Object.prototype.hasOwnProperty.call(parsed, key)) { + delete parsed[key]; + changed = true; + } + } + if (changed) { + this.db + .prepare("UPDATE config SET settings = ?, updatedAt = ? WHERE id = 1") + .run(JSON.stringify(parsed), now); + } + } + + this.db.prepare(` + INSERT INTO __meta (key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value + `).run(markerKey, String(SETTINGS_MIGRATION_VERSION)); + this.db.bumpLastModified(); + }); + + // (5) Defensive: null the moved keys out of the global store (outside the txn). + const globalMovedPatch: Record = {}; + for (const key of movedKeys) { + if (Object.prototype.hasOwnProperty.call(rawGlobalSettings, key)) { + globalMovedPatch[key] = null; // null-as-delete + } + } + if (Object.keys(globalMovedPatch).length > 0) { + try { + await this.globalSettingsStore.updateSettings(globalMovedPatch as Partial); + } catch (err) { + storeLog.warn("Global moved-key null-out failed during hard-move migration (non-fatal)", { + phase: "migrateMovedSettings:global-nullout", + error: err instanceof Error ? err.message : String(err), + }); + } + } + + // Invalidate cached config so subsequent reads reflect the removed keys. + this.invalidateConfigCacheAfterMigration(); + } + + /** Read the RAW persisted project settings JSON (the `config.settings` row), + * WITHOUT applying `DEFAULT_SETTINGS`. The migration needs this because the + * typed read merges defaults (which no longer contain moved keys), so it could + * not distinguish a customized moved value from an absent one. Returns `{}` on + * any read/parse failure. */ + private readRawProjectSettings(): Record { + try { + const row = this.db.prepare("SELECT settings FROM config WHERE id = 1").get() as + | { settings: string } + | undefined; + if (!row) return {}; + const parsed = JSON.parse(row.settings) as unknown; + return parsed && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record) + : {}; + } catch { + return {}; + } + } + + /** Drop any in-memory config cache after the migration mutates the raw + * `config.settings` row directly (bypassing `writeConfig`). No-op if the store + * has no such cache field. */ + private invalidateConfigCacheAfterMigration(): void { + // The project config is read fresh from SQLite each call (readConfigFast), + // so there is no project-settings cache to invalidate. The global store does + // cache; updateSettings() above already refreshed it. This hook exists as a + // documented seam in case a config cache is added later. + } + // ── Archive Cleanup Methods ───────────────────────────────────────── /** diff --git a/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts b/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts index 26ed53d573..fa4fd4547e 100644 --- a/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts +++ b/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts @@ -39,11 +39,13 @@ function readEngineSources(): { file: string; text: string }[] { const declDefault = new Map(BUILTIN_WORKFLOW_SETTINGS.map((s) => [s.id, s.default])); describe("workflow-settings fallback alignment (KTD-3, item 4)", () => { - it("(a) every built-in declaration default equals the legacy DEFAULT_PROJECT_SETTINGS literal", () => { + it("(a) the moved-key catalog has left DEFAULT_PROJECT_SETTINGS (U4 hard-move)", () => { + // Post-U4 hard-move: the moved keys were REMOVED from DEFAULT_PROJECT_SETTINGS + // (the parity literal is gone — the declaration default is now the single + // source of truth). The legacy object must no longer carry any catalog key. const legacy = DEFAULT_PROJECT_SETTINGS as Record; for (const s of BUILTIN_WORKFLOW_SETTINGS) { - expect(Object.prototype.hasOwnProperty.call(legacy, s.id)).toBe(true); - expect(s.default).toStrictEqual(legacy[s.id]); + expect(Object.prototype.hasOwnProperty.call(legacy, s.id)).toBe(false); } });