From e8e54bf343e77f7fe73c564221e669fc652d10b5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 22:37:51 -0700 Subject: [PATCH] =?UTF-8?q?feat(engine):=20per-task=20effective=20workflow?= =?UTF-8?q?=20settings=20=E2=80=94=20resolver,=20two-tier=20entry=20merge,?= =?UTF-8?q?=20fallback=20alignment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../workflow-settings-resolver.test.ts | 211 ++++++++++++++++++ .../core/src/builtin-workflow-settings.ts | 20 +- packages/core/src/builtin-workflows.ts | 9 + packages/core/src/index.ts | 8 + packages/core/src/store.ts | 27 ++- .../core/src/workflow-settings-resolver.ts | 180 +++++++++++++++ .../effective-settings-merge.test.ts | 116 ++++++++++ .../effective-settings-model-lane.test.ts | 60 +++++ ...rkflow-settings-fallback-alignment.test.ts | 107 +++++++++ packages/engine/src/effective-settings.ts | 71 ++++++ packages/engine/src/executor.ts | 31 ++- packages/engine/src/merger.ts | 20 +- packages/engine/src/mission-execution-loop.ts | 10 +- packages/engine/src/reviewer.ts | 17 +- packages/engine/src/self-healing.ts | 16 +- packages/engine/src/triage.ts | 16 +- 16 files changed, 880 insertions(+), 39 deletions(-) create mode 100644 packages/core/src/__tests__/workflow-settings-resolver.test.ts create mode 100644 packages/core/src/workflow-settings-resolver.ts create mode 100644 packages/engine/src/__tests__/effective-settings-merge.test.ts create mode 100644 packages/engine/src/__tests__/effective-settings-model-lane.test.ts create mode 100644 packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts create mode 100644 packages/engine/src/effective-settings.ts diff --git a/packages/core/src/__tests__/workflow-settings-resolver.test.ts b/packages/core/src/__tests__/workflow-settings-resolver.test.ts new file mode 100644 index 0000000000..69a4332016 --- /dev/null +++ b/packages/core/src/__tests__/workflow-settings-resolver.test.ts @@ -0,0 +1,211 @@ +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, + resolveEffectiveSettingsById, + type WorkflowSettingsResolverStore, +} from "../workflow-settings-resolver.js"; + +const PROJECT = "proj-1"; + +/** A custom workflow IR with NO settings declarations (declaration-absent path). */ +const CUSTOM_NO_SETTINGS: WorkflowIr = { + version: "v2", + name: "custom-no-settings", + columns: [{ id: "todo", name: "Todo", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { id: "end", kind: "end" }, + ], + edges: [{ from: "start", to: "end" }], +}; + +/** A custom workflow IR declaring a single setting (workflowStepTimeoutMs). */ +const CUSTOM_WITH_SETTING: WorkflowIr = { + ...CUSTOM_NO_SETTINGS, + name: "custom-with-setting", + settings: [ + { id: "workflowStepTimeoutMs", name: "Step timeout", type: "number", default: 99_000 }, + ], +}; + +function makeStore(opts: { + selection?: Record; + selectionThrows?: boolean; + defs?: Record; + values?: Record>; // key: `${workflowId}::${projectId}` + valuesThrows?: boolean; + projectId?: string; + projectIdThrows?: boolean; +}): WorkflowSettingsResolverStore { + return { + getTaskWorkflowSelection: vi.fn((taskId: string) => { + if (opts.selectionThrows) throw new Error("boom"); + return opts.selection?.[taskId]; + }), + getWorkflowDefinition: vi.fn(async (id: string) => opts.defs?.[id]), + getWorkflowSettingValues: vi.fn((workflowId: string, projectId: string) => { + if (opts.valuesThrows) throw new Error("values boom"); + return opts.values?.[`${workflowId}::${projectId}`] ?? {}; + }), + getWorkflowSettingsProjectId: vi.fn(() => { + if (opts.projectIdThrows) throw new Error("identity boom"); + return opts.projectId ?? PROJECT; + }), + }; +} + +describe("resolveEffectiveSettings (per-task)", () => { + it("parity anchor: builtin:coding with no stored values → declaration defaults equal legacy 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; + 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]); + } + } + }); + + it("a stored value for (workflow, project) is returned over the default", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, + values: { "builtin:coding::proj-1": { workflowStepTimeoutMs: 5_000, requirePrApproval: true } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + expect(eff.workflowStepTimeoutMs).toBe(5_000); + expect(eff.requirePrApproval).toBe(true); + // Untouched key falls to the declaration default. + expect(eff.runStepsInNewSessions).toBe(false); + }); + + it("two tasks resolving different workflows each get their own effective values", async () => { + const store = makeStore({ + selection: { + t1: { workflowId: "builtin:coding", stepIds: [] }, + t2: { workflowId: "wf-custom", stepIds: [] }, + }, + defs: { "wf-custom": { ir: CUSTOM_WITH_SETTING } }, + values: { + "builtin:coding::proj-1": { workflowStepTimeoutMs: 5_000 }, + "wf-custom::proj-1": { workflowStepTimeoutMs: 12_000 }, + }, + }); + const a = await resolveEffectiveSettings(store, { id: "t1" }); + const b = await resolveEffectiveSettings(store, { id: "t2" }); + expect(a.workflowStepTimeoutMs).toBe(5_000); + expect(b.workflowStepTimeoutMs).toBe(12_000); + // The custom workflow declares ONLY workflowStepTimeoutMs, so nothing else is in its map. + expect(Object.prototype.hasOwnProperty.call(b, "requirePrApproval")).toBe(false); + }); + + it("custom workflow with empty settings → declaration-absent map (read-site fallback applies)", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "wf-empty", stepIds: [] } }, + defs: { "wf-empty": { ir: CUSTOM_NO_SETTINGS } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + // No declarations → no moved key in the effective map → engine read site keeps + // its `?? ` fallback (= the legacy default; asserted by the alignment test). + expect(Object.keys(eff)).toHaveLength(0); + }); + + it("new custom workflow with empty settings does NOT inherit another workflow's values", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "wf-new", stepIds: [] } }, + defs: { "wf-new": { ir: CUSTOM_NO_SETTINGS } }, + // A different workflow has a customized value; the new one must not see it. + values: { "builtin:coding::proj-1": { workflowStepTimeoutMs: 5_000 } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + expect(Object.prototype.hasOwnProperty.call(eff, "workflowStepTimeoutMs")).toBe(false); + }); + + it("absent-default model lanes are omitted (never undefined) so the merge can't clobber", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + for (const lane of ["executionProvider", "executionModelId", "planningProvider", "validatorProvider"]) { + expect(Object.prototype.hasOwnProperty.call(eff, lane)).toBe(false); + } + }); + + it("a set model lane wins; unset lanes stay absent", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, + values: { "builtin:coding::proj-1": { executionProvider: "anthropic" } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + expect(eff.executionProvider).toBe("anthropic"); + expect(Object.prototype.hasOwnProperty.call(eff, "executionModelId")).toBe(false); + }); + + it("no selection → builtin:coding declaration defaults (never throws)", async () => { + const store = makeStore({ selection: {} }); + const eff = await resolveEffectiveSettings(store, { id: "t-none" }); + expect(eff.workflowStepTimeoutMs).toBe(360_000); + }); + + it("missing custom definition degrades to builtin declarations (never throws)", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "wf-gone", stepIds: [] } }, + defs: { "wf-gone": undefined }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + // Degrades to BUILTIN_CODING_WORKFLOW_IR declarations. + expect(eff.workflowStepTimeoutMs).toBe(360_000); + }); + + it("selection lookup throwing degrades to builtin declarations", async () => { + const store = makeStore({ selectionThrows: true }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + expect(eff.workflowStepTimeoutMs).toBe(360_000); + }); + + it("store value read throwing degrades to declaration defaults", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, + valuesThrows: true, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + expect(eff.workflowStepTimeoutMs).toBe(360_000); + }); + + it("project-id lookup throwing degrades to declaration defaults (empty stored map)", async () => { + const store = makeStore({ + selection: { t1: { workflowId: "builtin:coding", stepIds: [] } }, + projectIdThrows: true, + values: { "builtin:coding::proj-1": { workflowStepTimeoutMs: 5_000 } }, + }); + const eff = await resolveEffectiveSettings(store, { id: "t1" }); + // The stored 5_000 is unreachable because the project key couldn't be resolved. + expect(eff.workflowStepTimeoutMs).toBe(360_000); + }); +}); + +describe("resolveEffectiveSettingsById", () => { + it("resolves declarations + stored values for an explicit (workflowId, projectId)", async () => { + const store = makeStore({ + defs: { "wf-custom": { ir: CUSTOM_WITH_SETTING } }, + values: { "wf-custom::proj-9": { workflowStepTimeoutMs: 7_000 } }, + }); + const eff = await resolveEffectiveSettingsById(store, "wf-custom", "proj-9"); + expect(eff.workflowStepTimeoutMs).toBe(7_000); + }); + + it("builtin id with no stored values → catalog defaults", async () => { + const store = makeStore({}); + const eff = await resolveEffectiveSettingsById(store, "builtin:coding", "proj-9"); + expect(eff.requirePrApproval).toBe(false); + }); +}); diff --git a/packages/core/src/builtin-workflow-settings.ts b/packages/core/src/builtin-workflow-settings.ts index 7f3cce9676..cbef7af4e3 100644 --- a/packages/core/src/builtin-workflow-settings.ts +++ b/packages/core/src/builtin-workflow-settings.ts @@ -161,20 +161,12 @@ export const BUILTIN_WORKFLOW_SETTINGS: WorkflowSettingDefinition[] = [ default: false, description: "Enable periodic reflection passes over completed work.", }, - { - id: "reflectionIntervalMs", - name: "Reflection interval (ms)", - type: "number", - default: 3_600_000, - description: "How often to run a reflection pass when reflection is enabled.", - }, - { - id: "reflectionAfterTask", - name: "Reflect after each task", - type: "boolean", - default: true, - description: "Run a reflection pass after each task completes.", - }, + // NOTE (U3 catalog-shrink, item 5): `reflectionIntervalMs` and + // `reflectionAfterTask` were REMOVED from this catalog — neither has any engine + // read site (verified by grep across packages/engine/src), so per the plan's + // catalog-shrink rule they stay plain project settings and are NOT moved to + // workflow settings. `reflectionEnabled` is kept because executor.ts reads it + // (gate for reflection tools). // ── Per-phase model lanes ────────────────────────────────────────────── // Legacy defaults are all `undefined`; `default` is omitted so resolution diff --git a/packages/core/src/builtin-workflows.ts b/packages/core/src/builtin-workflows.ts index 62e734cf44..8422a8bf43 100644 --- a/packages/core/src/builtin-workflows.ts +++ b/packages/core/src/builtin-workflows.ts @@ -1,4 +1,5 @@ import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "./builtin-stepwise-coding-workflow-ir.js"; +import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import type { WorkflowDefinition } from "./workflow-definition-types.js"; import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; @@ -43,6 +44,14 @@ function linear(spec: BuiltinSpec): WorkflowDefinition { layout[node.id] = { x: 60 + i * 170, y: 160 }; }); const ir = parseWorkflowIr({ version: "v1", name: spec.name, nodes, edges }); + // Attach the moved-key settings catalog (U1/U3, R4) so every built-in workflow + // carries its declarations through the resolver path (resolveWorkflowIrById → + // resolveEffectiveSettings). v1 graphs upgrade to v2 on parse, so the parsed IR + // is v2 and can carry `settings`. Defaults are byte-equal to legacy + // DEFAULT_PROJECT_SETTINGS literals, so this is behavior-inert. + if (ir.version === "v2") { + ir.settings = BUILTIN_WORKFLOW_SETTINGS; + } return { id: spec.id, name: spec.name, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4b371c3d57..085f75e5d8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -260,6 +260,14 @@ export { resolveWorkflowIrById, type WorkflowIrResolverStore, } from "./workflow-ir-resolver.js"; +export { + resolveEffectiveSettings, + resolveEffectiveSettingsDetailed, + resolveEffectiveSettingsById, + type WorkflowSettingsResolverStore, + type EffectiveSettingsResult, + type EffectiveSettingsTaskRef, +} from "./workflow-settings-resolver.js"; // ── Engine wiring (set by @fusion/engine at module load) ──────────── export { diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 330c84c8e3..967a7a3939 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -6986,10 +6986,11 @@ export class TaskStore extends EventEmitter { /** Resolve the setting DECLARATIONS for a workflow id (built-in or custom). The * built-in path mirrors the IR resolver (`resolveWorkflowIrById`): built-in ids * resolve through the same code path so value writes target the same schema the - * engine resolver sees. For built-in ids whose resolved IR does not yet carry - * `settings` (the linear `BUILTIN_WORKFLOWS` graphs predate the settings - * declarations), fall back to the canonical built-in declaration catalog - * (`BUILTIN_WORKFLOW_SETTINGS`) so built-in VALUE writes succeed (R4/KTD-2). + * engine resolver sees. As of U3 every built-in workflow IR embeds + * `BUILTIN_WORKFLOW_SETTINGS` (attached in `builtin-workflows.ts` / + * `builtin-coding-workflow-ir.ts`), so the `declared` branch below now handles + * built-ins too. The built-in catalog fallback is kept as a cheap defensive belt + * in case a future built-in graph is constructed without the embed (R4/KTD-2). * Returns `undefined` when the workflow is missing or declares no settings. */ private async resolveWorkflowSettingDeclarations( workflowId: string, @@ -6997,12 +6998,26 @@ export class TaskStore extends EventEmitter { const ir = await resolveWorkflowIrById(this, workflowId); const declared = ir.version === "v2" ? ir.settings : undefined; if (declared && declared.length > 0) return declared; - // Built-in workflows declare the full moved-key catalog (the migration parity - // anchor); their selectable graphs may not embed it yet. + // Defensive belt: built-in ids always have a declaration catalog even if a + // particular built-in graph somehow lacks the embed. if (isBuiltinWorkflowId(workflowId)) return BUILTIN_WORKFLOW_SETTINGS; return declared; } + /** The stable project id this store scopes `workflow_settings` value rows by + * (U3). A single store instance is bound to one project (its `rootDir`); the + * durable project-identity id is that project's key. Falls back to the store's + * `rootDir` when no identity row exists yet (fresh project pre-identity), which + * is still stable per store instance. The engine's per-task effective-settings + * resolver uses this so reads/writes share one project key. */ + getWorkflowSettingsProjectId(): string { + try { + return this.db.getProjectIdentity()?.id ?? this.rootDir; + } catch { + return this.rootDir; + } + } + /** Read the raw stored setting-value map for `(workflowId, projectId)`. Returns * an empty object when no row exists. Raw (pre drop-on-orphan) — callers that * need engine-effective values run {@link resolveEffectiveSettingValues}. */ diff --git a/packages/core/src/workflow-settings-resolver.ts b/packages/core/src/workflow-settings-resolver.ts new file mode 100644 index 0000000000..1e44312157 --- /dev/null +++ b/packages/core/src/workflow-settings-resolver.ts @@ -0,0 +1,180 @@ +/** + * Per-task EFFECTIVE workflow-settings resolution (U3, R3, KTD-3). + * + * Sibling of `workflow-ir-resolver.ts`. Composes three steps into the flat, + * `Partial`-shaped value map the engine reads at executor entry: + * + * 1. resolve the workflow IR (built-in or custom) → its `settings` declarations; + * 2. read the raw stored `(workflowId, projectId)` value map; + * 3. {@link resolveEffectiveSettingValues} → declaration default ?? stored value, + * dropping orphaned/invalid stored entries (KTD-6). + * + * The moved keys are all current `ProjectSettings` fields, so the returned map is a + * structurally-compatible `Partial` today. The engine MERGES this + * over the project/global settings object so the ~20 flat `settings.` read + * sites keep their exact expressions (KTD-3). + * + * NEVER-THROW contract (mirrors the IR resolver): a missing/corrupt workflow + * degrades to the built-in coding declarations; any store error degrades to an + * empty stored map, so the result falls back to declaration defaults. The caller + * always receives a usable map. + * + * IMPORTANT (parity): for built-in workflows with no stored values the effective + * map carries the declaration defaults, which are byte-equal to the legacy + * `DEFAULT_PROJECT_SETTINGS` literals — so merging it over project settings is a + * no-op when nothing is customized. Keys whose declaration omits a default (the + * per-phase model lanes) are ABSENT from the map (never `undefined`), so the merge + * never clobbers a real project value with `undefined`. + */ + +import { + resolveWorkflowIrById, + resolveWorkflowIrForTask, + type WorkflowIrResolverStore, +} from "./workflow-ir-resolver.js"; +import { resolveEffectiveSettingValues, findOrphanedSettingValues } from "./workflow-settings.js"; +import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; +import type { WorkflowSettingDefinition, WorkflowIr } from "./workflow-ir-types.js"; + +/** + * The effective map PLUS the subset of keys whose value came from an EXPLICIT + * STORED workflow value (not a declaration default). The engine entry merge uses + * `storedKeys` to decide override-vs-fill semantics: + * + * - a STORED key ALWAYS overrides the project/global base (the workflow tuned it); + * - a default-only key (in `effective` but NOT in `storedKeys`) only FILLS the + * base when the base lacks the key. + * + * This is what makes U3 behavior-identical pre-migration: a customized project + * setting (still present in the base before the U4 hard-move) is NOT clobbered by a + * declaration default; only a real stored workflow value overrides it. Post- + * migration the base lacks the moved key, so the declaration default fills it. + */ +export interface EffectiveSettingsResult { + effective: Record; + storedKeys: Set; +} + +/** Minimal store surface the effective-settings resolver needs (public APIs). */ +export interface WorkflowSettingsResolverStore extends WorkflowIrResolverStore { + /** Raw stored `(workflowId, projectId)` value map; `{}` when no row exists. */ + getWorkflowSettingValues(workflowId: string, projectId: string): Record; + /** The stable project id this store scopes `workflow_settings` rows by. A store + * instance is bound to one project, so the resolver derives the project key from + * the store rather than from the task (Task carries no projectId field). */ + getWorkflowSettingsProjectId(): string; +} + +/** The declarations carried by a resolved IR, with the built-in catalog as the + * defensive belt for built-in graphs that predate the embedded `settings` (the + * linear `BUILTIN_WORKFLOWS` carry them now, but keep the belt cheap). */ +function declarationsFromIr( + ir: WorkflowIr, + workflowId: string | undefined, +): WorkflowSettingDefinition[] | undefined { + const declared = ir.version === "v2" ? ir.settings : undefined; + if (declared && declared.length > 0) return declared; + // Built-in workflows declare the full moved-key catalog (the migration parity + // anchor); fall back to it only when the resolved IR didn't embed it. + if (workflowId && workflowId.startsWith("builtin:")) return BUILTIN_WORKFLOW_SETTINGS; + return declared; +} + +/** Compose declarations + raw stored values → effective flat map + the set of keys + * whose value came from an explicit stored workflow value (never throws). */ +function effectiveFrom( + store: WorkflowSettingsResolverStore, + ir: WorkflowIr, + workflowId: string | undefined, + projectId: string, +): EffectiveSettingsResult { + const declarations = declarationsFromIr(ir, workflowId); + let stored: Record = {}; + if (workflowId) { + try { + stored = store.getWorkflowSettingValues(workflowId, projectId) ?? {}; + } catch { + stored = {}; + } + } + const effective = resolveEffectiveSettingValues(declarations, stored); + // A key is "stored" iff it appears in the effective map AND the stored row holds + // a value for it that did NOT orphan (i.e. it was not dropped). Orphaned stored + // entries fall to the declaration default, so they count as default-only. + const orphanedIds = new Set(findOrphanedSettingValues(declarations, stored).map((o) => o.id)); + const storedKeys = new Set(); + for (const id of Object.keys(effective)) { + if (Object.prototype.hasOwnProperty.call(stored, id) && !orphanedIds.has(id)) { + const raw = stored[id]; + if (raw !== null && raw !== undefined) storedKeys.add(id); + } + } + return { effective, storedKeys }; +} + +/** + * Resolve the effective workflow settings for an explicit `(workflowId, + * projectId)`. Used by the migration/export/agent-tool paths that name a + * workflow directly. Never throws. + */ +export async function resolveEffectiveSettingsById( + store: WorkflowSettingsResolverStore, + workflowId: string, + projectId: string, + irCache?: Map, +): Promise> { + const ir = await resolveWorkflowIrById(store, workflowId, irCache); + return effectiveFrom(store, ir, workflowId, projectId).effective; +} + +/** The minimal task identity the per-task resolver reads. Task carries no + * projectId field — the project key comes from the store. */ +export interface EffectiveSettingsTaskRef { + id: string; +} + +/** + * Resolve the effective workflow settings for a TASK (the engine's primary entry). + * Reads the task's workflow selection, resolves its IR, and composes the effective + * value map for `(resolvedWorkflowId, task.projectId)`. + * + * An absent/falsy selection degrades to `builtin:coding` (matching the IR + * resolver), so a selection-less task reads the built-in declaration defaults — + * byte-equal to legacy project-settings defaults. Never throws. + */ +export async function resolveEffectiveSettings( + store: WorkflowSettingsResolverStore, + task: EffectiveSettingsTaskRef, + irCache?: Map, +): Promise> { + return (await resolveEffectiveSettingsDetailed(store, task, irCache)).effective; +} + +/** + * Like {@link resolveEffectiveSettings}, but also returns `storedKeys` (the keys + * whose value came from an explicit stored workflow value vs. a declaration + * default). The engine entry merge uses this to override the base only for stored + * keys and fill-only for default-only keys. Never throws. + */ +export async function resolveEffectiveSettingsDetailed( + store: WorkflowSettingsResolverStore, + task: EffectiveSettingsTaskRef, + irCache?: Map, +): Promise { + let workflowId: string | undefined; + try { + workflowId = store.getTaskWorkflowSelection(task.id)?.workflowId; + } catch { + workflowId = undefined; + } + const effectiveWorkflowId = workflowId || "builtin:coding"; + const ir = await resolveWorkflowIrForTask(store, task.id, irCache); + let projectId: string; + try { + projectId = store.getWorkflowSettingsProjectId(); + } catch { + // Degrade to declaration defaults (empty stored map) on identity failure. + return effectiveFrom(store, ir, undefined, ""); + } + return effectiveFrom(store, ir, effectiveWorkflowId, projectId); +} diff --git a/packages/engine/src/__tests__/effective-settings-merge.test.ts b/packages/engine/src/__tests__/effective-settings-merge.test.ts new file mode 100644 index 0000000000..f3d84e78ad --- /dev/null +++ b/packages/engine/src/__tests__/effective-settings-merge.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, vi } from "vitest"; + +import type { Settings } from "@fusion/core"; +import { mergeEffectiveSettings } from "../effective-settings.js"; + +const PROJECT = "proj-1"; + +/** A base settings object with real project values for the keys under test. */ +function baseSettings(): Settings { + return { + workflowStepTimeoutMs: 360_000, + requirePrApproval: false, + runStepsInNewSessions: false, + // A real project value for an absent-default lane — must NOT be clobbered. + executionProvider: "project-anthropic", + executionModelId: "claude-project", + } as unknown as Settings; +} + +function makeStore(opts: { + workflowId?: string; + values?: Record; +}) { + return { + getTaskWorkflowSelection: vi.fn((_t: string) => + opts.workflowId ? { workflowId: opts.workflowId, stepIds: [] } : undefined, + ), + getWorkflowDefinition: vi.fn(async (_id: string) => undefined), + getWorkflowSettingValues: vi.fn((_w: string, _p: string) => opts.values ?? {}), + getWorkflowSettingsProjectId: vi.fn(() => PROJECT), + }; +} + +describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { + it("parity: builtin:coding, nothing stored → merged equals base for moved keys", async () => { + const store = makeStore({ workflowId: "builtin:coding" }); + const base = baseSettings(); + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, base); + // Declaration defaults are byte-equal to legacy defaults, so these don't change. + expect(merged.workflowStepTimeoutMs).toBe(360_000); + expect(merged.requirePrApproval).toBe(false); + expect(merged.runStepsInNewSessions).toBe(false); + }); + + it("does NOT clobber a real project model-lane value with an undefined effective lane", async () => { + const store = makeStore({ workflowId: "builtin:coding" }); // no stored lane values + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, baseSettings()); + // executionProvider has no declaration default → absent from effective map → + // the real project value survives. + expect(merged.executionProvider).toBe("project-anthropic"); + expect(merged.executionModelId).toBe("claude-project"); + }); + + it("pre-migration parity: a CUSTOMIZED base value is NOT clobbered by a declaration default", async () => { + // The project base still carries a non-default value (pre-U4-migration state); + // no stored workflow value exists. The declaration default must NOT override it. + const store = makeStore({ workflowId: "builtin:coding" }); + const base = { ...baseSettings(), verificationFixRetries: 0, workflowStepTimeoutMs: 12_345 } as unknown as Settings; + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, base); + expect(merged.verificationFixRetries).toBe(0); // not the declaration default (3) + expect(merged.workflowStepTimeoutMs).toBe(12_345); // not the declaration default (360_000) + }); + + it("post-migration fill: a declaration default fills when the base lacks the key", async () => { + const store = makeStore({ workflowId: "builtin:coding" }); + // Base lacks workflowStepTimeoutMs (moved key removed from project settings). + const base = { requirePrApproval: false } as unknown as Settings; + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, base); + expect(merged.workflowStepTimeoutMs).toBe(360_000); // filled from declaration default + }); + + it("a stored value overrides the base", async () => { + const store = makeStore({ + workflowId: "builtin:coding", + values: { workflowStepTimeoutMs: 9_000, requirePrApproval: true, executionProvider: "wf-openai" }, + }); + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, baseSettings()); + expect(merged.workflowStepTimeoutMs).toBe(9_000); + expect(merged.requirePrApproval).toBe(true); + // A stored lane DOES override the project value. + expect(merged.executionProvider).toBe("wf-openai"); + // Untouched lane keeps the project value. + expect(merged.executionModelId).toBe("claude-project"); + }); + + it("returns a NEW object; the base is not mutated", async () => { + const store = makeStore({ workflowId: "builtin:coding", values: { workflowStepTimeoutMs: 1 } }); + const base = baseSettings(); + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, base); + expect(base.workflowStepTimeoutMs).toBe(360_000); + expect(merged).not.toBe(base); + }); + + it("degrades to base on resolver error (never throws)", async () => { + const store = { + getTaskWorkflowSelection: vi.fn(() => { + throw new Error("boom"); + }), + getWorkflowDefinition: vi.fn(async () => { + throw new Error("boom"); + }), + getWorkflowSettingValues: vi.fn(() => { + throw new Error("boom"); + }), + getWorkflowSettingsProjectId: vi.fn(() => { + throw new Error("boom"); + }), + }; + const base = baseSettings(); + const merged = await mergeEffectiveSettings(store as any, { id: "t1" }, base); + // resolveEffectiveSettings degrades to builtin declaration defaults even when the + // selection/project throw; the merge stays behavior-inert for the base values. + expect(merged.workflowStepTimeoutMs).toBe(360_000); + expect(merged.executionProvider).toBe("project-anthropic"); + }); +}); diff --git a/packages/engine/src/__tests__/effective-settings-model-lane.test.ts b/packages/engine/src/__tests__/effective-settings-model-lane.test.ts new file mode 100644 index 0000000000..3c28a19066 --- /dev/null +++ b/packages/engine/src/__tests__/effective-settings-model-lane.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, vi } from "vitest"; + +import { resolveExecutionSettingsModel, type Settings } from "@fusion/core"; +import { mergeEffectiveSettings } from "../effective-settings.js"; + +const PROJECT = "proj-1"; + +function makeStore(values?: Record) { + return { + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "builtin:coding", stepIds: [] })), + getWorkflowDefinition: vi.fn(async () => undefined), + getWorkflowSettingValues: vi.fn(() => values ?? {}), + getWorkflowSettingsProjectId: vi.fn(() => PROJECT), + }; +} + +/** + * KTD-7 model-lane chain, pinned AFTER the entry merge. The chain reads + * `settings.executionProvider` (workflow lane, now from the effective merge) → + * `executionGlobalProvider` (stays global) → project default override → global + * default. The entry merge feeds the workflow lane into that same field, so the + * documented chain is unchanged. + */ +describe("model-lane resolution after effective-settings merge (KTD-7)", () => { + it("workflow lane set → wins over global lane and defaults", async () => { + const base = { + executionGlobalProvider: "global-prov", + executionGlobalModelId: "global-model", + defaultProvider: "def-prov", + defaultModelId: "def-model", + } as unknown as Settings; + const merged = await mergeEffectiveSettings( + makeStore({ executionProvider: "wf-prov", executionModelId: "wf-model" }) as any, + { id: "t1" }, + base, + ); + expect(resolveExecutionSettingsModel(merged)).toEqual({ provider: "wf-prov", modelId: "wf-model" }); + }); + + it("workflow lane empty → falls through to the global lane", async () => { + const base = { + executionGlobalProvider: "global-prov", + executionGlobalModelId: "global-model", + defaultProvider: "def-prov", + defaultModelId: "def-model", + } as unknown as Settings; + // No stored workflow lane; builtin declarations omit lane defaults → lane absent. + const merged = await mergeEffectiveSettings(makeStore() as any, { id: "t1" }, base); + expect(resolveExecutionSettingsModel(merged)).toEqual({ provider: "global-prov", modelId: "global-model" }); + }); + + it("workflow + global lanes empty → falls through to the global default", async () => { + const base = { + defaultProvider: "def-prov", + defaultModelId: "def-model", + } as unknown as Settings; + const merged = await mergeEffectiveSettings(makeStore() as any, { id: "t1" }, base); + expect(resolveExecutionSettingsModel(merged)).toEqual({ provider: "def-prov", modelId: "def-model" }); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts b/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts new file mode 100644 index 0000000000..26ed53d573 --- /dev/null +++ b/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect } from "vitest"; +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +import { BUILTIN_WORKFLOW_SETTINGS, DEFAULT_PROJECT_SETTINGS } from "@fusion/core"; + +/** + * U3 fallback-alignment guard (KTD-3, plan item 4). + * + * Each engine read site of a moved key has a hardcoded `?? ` (or + * default-true `!== false`) fallback that fires when the effective-settings map + * carries no value for that key (a custom workflow that does not declare it). That + * fallback is TODAY's behavior and MUST equal the built-in declaration default — + * otherwise effective resolution returning "absent" would silently change behavior + * for an undeclared key. This test pins: + * + * (a) built-in declaration default === legacy DEFAULT_PROJECT_SETTINGS literal + * (the parity anchor), and + * (b) the literal `?? ` fallbacks actually present in engine source equal the + * declaration default, scanned from source so a future edit that introduces a + * drifting fallback fails here. + * + * The audited read-site table (key → literal) is encoded below; the source scan + * asserts no NEW literal fallback for these keys drifts from the declaration. + */ + +const SRC_DIR = join(__dirname, ".."); + +function readEngineSources(): { file: string; text: string }[] { + const out: { file: string; text: string }[] = []; + for (const name of readdirSync(SRC_DIR)) { + if (!name.endsWith(".ts")) continue; + if (name.endsWith(".d.ts")) continue; + out.push({ file: name, text: readFileSync(join(SRC_DIR, name), "utf-8") }); + } + return out; +} + +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", () => { + 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]); + } + }); + + it("(b) every numeric/string `settings. ?? ` fallback in engine source matches the declaration default", () => { + const sources = readEngineSources(); + const mismatches: string[] = []; + + for (const [id, def] of declDefault) { + if (def === undefined) continue; // absent-default lanes: no `??` literal to check + // Match `settings. ?? ` with numeric (incl. 360_000) or quoted-string literals. + const re = new RegExp( + String.raw`\.${id}\s*\?\?\s*([0-9][0-9_]*|"[^"]*"|'[^']*'|true|false)`, + "g", + ); + for (const { file, text } of sources) { + if (file.endsWith(".test.ts")) continue; + let m: RegExpExecArray | null; + while ((m = re.exec(text)) !== null) { + const raw = m[1]; + let literal: unknown; + if (/^[0-9][0-9_]*$/.test(raw)) literal = Number(raw.replace(/_/g, "")); + else if (raw === "true") literal = true; + else if (raw === "false") literal = false; + else literal = raw.slice(1, -1); // strip quotes + if (literal !== def) { + mismatches.push(`${file}: settings.${id} ?? ${raw} (decl default ${JSON.stringify(def)})`); + } + } + } + } + + expect(mismatches, `misaligned read-site fallbacks:\n${mismatches.join("\n")}`).toEqual([]); + }); + + it("documents the audited read-site fallbacks (key → literal → aligned)", () => { + // This table is the human-readable record from the U3 fallback audit. The + // VALUES here are the audited read-site literals; the assertion ties each to + // the declaration default so the table cannot silently drift. + const audited: Record = { + workflowStepTimeoutMs: 360_000, // executor.ts: ?? 360_000 + workflowStepScopeEnforcement: "block", // executor.ts: ?? "block" + planOnlyScopeLeakEnforcement: "warn", // executor.ts: ?? "warn" + workflowRevisionForkOnScopeMismatch: true, // executor.ts: !== false (default-true) + strictScopeEnforcement: false, // merger.ts: passed truthy/undefined → false + runStepsInNewSessions: false, // executor.ts: truthy check → false + maxParallelSteps: 2, // executor.ts / step-session-executor.ts: ?? 2 + buildRetryCount: 0, // merger.ts: ?? 0 + verificationFixRetries: 3, // executor.ts: ?? 3; merger.ts aligned to ?? 3 (was ?? 2, dead) + maxPostReviewFixes: 1, // self-healing.ts: ?? 1 + requirePrApproval: false, // no engine read; default-false elsewhere + requirePlanApproval: false, // triage.ts: truthy check → false + reviewHandoffPolicy: "disabled", // executor.ts: === "comment-triggered" → "disabled" + maxReviewerContextRetries: 2, // retry-burned-logger.ts: returned directly + maxReviewerFallbackRetries: 2, // retry-burned-logger.ts: returned directly + reflectionEnabled: false, // executor.ts: truthy check → false + }; + for (const [id, lit] of Object.entries(audited)) { + expect(declDefault.get(id)).toStrictEqual(lit); + } + }); +}); diff --git a/packages/engine/src/effective-settings.ts b/packages/engine/src/effective-settings.ts new file mode 100644 index 0000000000..57de2657a3 --- /dev/null +++ b/packages/engine/src/effective-settings.ts @@ -0,0 +1,71 @@ +/** + * Engine-side helper: merge per-task EFFECTIVE workflow settings (U3, KTD-3) over a + * base settings object fetched from the store, so the engine's flat + * `settings.` read sites pick up workflow-setting values with zero changes at + * the read sites. + * + * TWO-TIER MERGE (the parity-preserving rule): + * - a STORED workflow value ALWAYS overrides the base (the workflow tuned it); + * - a declaration-DEFAULT-only key (no stored value) only FILLS the base when the + * base lacks the key. + * + * This is what keeps U3 behavior-identical BEFORE the U4 hard-move: a customized + * project setting still present in the base is NOT clobbered by a declaration + * default; only a real stored workflow value overrides it. After the hard-move the + * base lacks the moved key, so the declaration default fills it. Absent-default + * model lanes contribute nothing, so they never override a real project value. + * + * `resolveEffectiveSettingsDetailed` never throws (degrades to declaration + * defaults), so this helper is a thin store-coupled wrapper that also never throws. + */ + +import { resolveEffectiveSettingsDetailed, type Settings, type TaskStore } from "@fusion/core"; + +/** The minimal task shape the resolver needs. Task carries no projectId field — + * the project key is derived from the store. */ +export interface EffectiveSettingsTask { + id: string; +} + +/** + * Merge `base` with the task's effective workflow settings via the two-tier rule + * (stored overrides; default-only fills only-absent). Returns a NEW object; `base` + * is not mutated. Degrades to returning `base` unchanged on any resolver error. + */ +export async function mergeEffectiveSettings>( + store: Pick< + TaskStore, + | "getTaskWorkflowSelection" + | "getWorkflowDefinition" + | "getWorkflowSettingValues" + | "getWorkflowSettingsProjectId" + >, + task: EffectiveSettingsTask, + base: T, +): Promise { + let effective: Record; + let storedKeys: Set; + try { + const detailed = await resolveEffectiveSettingsDetailed( + store as Parameters[0], + task, + ); + effective = detailed.effective; + storedKeys = detailed.storedKeys; + } catch { + return base; + } + const merged: Record = { ...base }; + for (const key of Object.keys(effective)) { + const value = effective[key]; + if (value === undefined) continue; + if (storedKeys.has(key)) { + // Stored workflow value: always overrides the base. + merged[key] = value; + } else if (merged[key] === undefined) { + // Declaration default: only fills when the base lacks the key (post-migration). + merged[key] = value; + } + } + return merged as T; +} diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3a29414187..bc7295f663 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -10,6 +10,7 @@ import { existsSync, realpathSync } from "node:fs"; import { readFile, rm, writeFile } from "node:fs/promises"; import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, WorkflowStep, MissionStore, Slice, AgentState, AgentCapability, RunMutationContext, AgentHeartbeatConfig, Agent, AgentMemoryInclusionMode, ProjectSettings, MergeResult, WorkflowIrNode } from "@fusion/core"; import { RetryStormError, TaskDeletedError, serializeRetryStormError, isExperimentalFeatureEnabled, resolveWorkflowIrForTask } from "@fusion/core"; +import { mergeEffectiveSettings } from "./effective-settings.js"; import type { TaskStep, WorkflowIr, WorkflowFieldDefinition } from "@fusion/core"; import { buildWorkflowObservationFromTask, @@ -2144,8 +2145,10 @@ export class TaskExecutor { } // After injecting comments, check for review handoff intent - // Only detect handoff in agent-authored comments when policy is enabled - const settings = await this.store.getSettings(); + // Only detect handoff in agent-authored comments when policy is enabled. + // Merge per-task effective workflow settings (U3, KTD-3) so + // reviewHandoffPolicy resolves from the workflow. Behavior-inert by default. + const settings = await mergeEffectiveSettings(this.store, task, await this.store.getSettings()); if (settings.reviewHandoffPolicy === "comment-triggered") { const agentComments = newComments.filter(c => c.author !== "user"); for (const comment of agentComments) { @@ -4273,7 +4276,9 @@ export class TaskExecutor { const worktreePath = active.worktreePath || detail.worktree || this.rootDir; const stepName = detail.steps[stepIndex]?.name ?? `Step ${stepIndex + 1}`; const promptContent = detail.prompt ?? ""; - const settings = await this.store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) so the validator + // model-lane reads below pick up workflow values. Behavior-inert by default. + const settings = await mergeEffectiveSettings(this.store, detail, await this.store.getSettings()); const sem = this.options.semaphore; const invokeReviewer = () => @@ -4730,8 +4735,14 @@ export class TaskExecutor { executorLog.log(`Starting ${task.id}: ${task.title || task.description.slice(0, 60)}`); - // Fetch settings early — needed for worktree naming and later configuration - const settings = await this.store.getSettings(); + // Fetch settings early — needed for worktree naming and later configuration. + // Merge per-task effective workflow settings (U3, KTD-3) OVER the project/global + // base so the ~20 flat `settings.` read sites threaded from here (workflow + // step timeout, scope enforcement, runStepsInNewSessions, model lanes, + // reviewHandoffPolicy, …) pick up workflow values with zero read-site changes. + // Behavior-inert when nothing is customized (declaration defaults === legacy + // defaults; absent-default lanes never override). + const settings = await mergeEffectiveSettings(this.store, task, await this.store.getSettings()); // Keep runtime plugin workflow step templates synchronized into TaskStore. // TaskStore resolves plugin-prefixed workflow IDs from this injected cache @@ -8166,7 +8177,10 @@ export class TaskExecutor { }; } - const settings = await store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) so the + // planOnlyScopeLeakEnforcement read in evaluateTaskDoneScopeLeak picks up + // workflow values. Behavior-inert by default. + const settings = await mergeEffectiveSettings(store, task, await store.getSettings()); const scopeLeakCheck = await this.evaluateTaskDoneScopeLeak(task, worktreePath, promptContent, settings, audit) .catch((error: unknown) => { const errorMessage = error instanceof Error ? error.message : String(error); @@ -8322,7 +8336,10 @@ export class TaskExecutor { } try { - const settings = await store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) so the + // validator model-lane reads below pick up workflow values; this tool + // closure re-fetches independently. Behavior-inert by default. + const settings = await mergeEffectiveSettings(store, detail, await store.getSettings()); // Run the reviewer via semaphore.runNested so its slot accounting // is honest: activeCount transiently bumps to reflect the second // agent session, but the reviewer doesn't enter the wait queue diff --git a/packages/engine/src/merger.ts b/packages/engine/src/merger.ts index a776e7cb86..4a180c63f0 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -3,6 +3,7 @@ import { execSync, exec } from "node:child_process"; import * as childProcess from "node:child_process"; import { promisify } from "node:util"; import { IDENTITY_GUARD_BYPASS_ENV } from "./worktree-hooks.js"; +import { mergeEffectiveSettings } from "./effective-settings.js"; // Internal git plumbing intentionally bypasses sandbox backends. const execAsync = promisify(exec); @@ -7603,7 +7604,11 @@ export async function aiMergeTask( } const projectRootDir = rootDir; - const settings = await store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) over the base so the + // merger's flat reads (strictScopeEnforcement, verificationFixRetries, + // buildRetryCount, titleSummarizer lanes — all threaded from here via + // executeMergeAttempt) pick up workflow values. Behavior-inert by default. + const settings = await mergeEffectiveSettings(store, task, await store.getSettings()); // U7 (R10): resolve the merge trait's policy (strategy / fileScope / rules) // from the task's workflow when the workflowColumns flag is ON, falling back // to the existing settings knobs otherwise. Read-through only — merge @@ -9523,7 +9528,7 @@ export async function aiMergeTask( // Try in-merge fix attempts before propagating if (error.name === "VerificationError") { const verificationErr = error as VerificationError; - const maxFixRetries = Math.min(settings.verificationFixRetries ?? 2, 3); + const maxFixRetries = Math.min(settings.verificationFixRetries ?? 3, 3); // U3: aligned to schema default (3); was ?? 2 (dead today, DEFAULT_PROJECT_SETTINGS re-injects 3) if (maxFixRetries > 0 && (verificationErr.verificationResult.testResult || verificationErr.verificationResult.buildResult)) { mergerLog.log(`${taskId}: deterministic verification failed — attempting in-merge fix (up to ${maxFixRetries} attempts)`); @@ -9682,7 +9687,7 @@ export async function aiMergeTask( // Check if it's a build verification failure if (error.message?.includes("Build verification failed")) { - const maxFixRetries = Math.min(settings.verificationFixRetries ?? 2, 3); + const maxFixRetries = Math.min(settings.verificationFixRetries ?? 3, 3); // U3: aligned to schema default (3); was ?? 2 (dead today, DEFAULT_PROJECT_SETTINGS re-injects 3) // Try in-merge fix before falling back to build retry if (maxFixRetries > 0 && (effectiveTestCommand || effectiveBuildCommand)) { @@ -11585,7 +11590,14 @@ async function runAiAgentForCommit(params: AiAgentParams): Promise<{ success: bo preAttemptHeadSha, } = params; - const settings = await store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) — this worker re-fetches + // settings independently of aiMergeTask, so apply the same merge here (covers the + // titleSummarizer lane reads in resolveSafeCommitBody). Behavior-inert by default. + const settings = await mergeEffectiveSettings( + store, + await store.getTask(taskId), + await store.getSettings(), + ); // Track build failure state let buildFailed = false; diff --git a/packages/engine/src/mission-execution-loop.ts b/packages/engine/src/mission-execution-loop.ts index 438433992b..cfd77f3e7e 100644 --- a/packages/engine/src/mission-execution-loop.ts +++ b/packages/engine/src/mission-execution-loop.ts @@ -28,6 +28,7 @@ import { resolveTaskValidatorModel, } from "@fusion/core"; import { createFnAgent, promptWithFallback, type AgentResult } from "./pi.js"; +import { mergeEffectiveSettings } from "./effective-settings.js"; import { createResolvedAgentSession, extractRuntimeHint, @@ -469,7 +470,14 @@ export class MissionExecutionLoop extends EventEmitter { ? await this.agentStore.getAgent(task.assignedAgentId).catch(() => null) : null; const validationRuntimeHint = extractRuntimeHint(assignedAgent?.runtimeConfig); - const settings = await this.taskStore.getSettings().catch(() => undefined); + // Merge per-task effective workflow settings (U3, KTD-3) so the validator + // model-lane reads pick up workflow values; skip when there is no task in + // scope (mission-level validation has no per-task workflow). Behavior-inert by + // default. + const baseSettings = await this.taskStore.getSettings().catch(() => undefined); + const settings = task && baseSettings + ? await mergeEffectiveSettings(this.taskStore, task, baseSettings) + : baseSettings; const validationSessionModel = this.resolveValidationSessionModel( task, settings, diff --git a/packages/engine/src/reviewer.ts b/packages/engine/src/reviewer.ts index 675b3afd48..6cdadec3f4 100644 --- a/packages/engine/src/reviewer.ts +++ b/packages/engine/src/reviewer.ts @@ -12,6 +12,7 @@ import type { TaskStore, TaskComment, AgentPromptsConfig, Settings } from "@fusion/core"; import { buildReviewerMemoryInstructions, resolveAgentPrompt, resolvePersistAgentThinkingLog, resolveAgentMemoryInclusionMode } from "@fusion/core"; import { recordRetry } from "./retry-burned-logger.js"; +import { mergeEffectiveSettings } from "./effective-settings.js"; import { describeModel, promptWithFallback } from "./pi.js"; import { isContextLimitError } from "./context-limit-detector.js"; import { createResolvedAgentSession, extractRuntimeHint } from "./agent-session-helpers.js"; @@ -676,7 +677,21 @@ export async function reviewStep( }; const hasConfiguredFallback = Boolean(validatorFallbackProvider && validatorFallbackModelId); - const retrySettings = liveSettings ?? options.settings; + // Merge per-task effective workflow settings (U3, KTD-3) over the base so the + // retry-budget reads (maxReviewerContextRetries / maxReviewerFallbackRetries via + // recordRetry) pick up workflow values. `liveSettings`/`options.settings` are the + // base; the merge is behavior-inert when nothing is customized. Resolved once + // here (all recordRetry sites share it). + const retrySettingsBase = liveSettings ?? options.settings; + let retrySettings = retrySettingsBase; + if (options.store && options.taskId && retrySettingsBase) { + try { + const retryTask = await options.store.getTask(options.taskId); + retrySettings = await mergeEffectiveSettings(options.store, retryTask, retrySettingsBase); + } catch { + // Keep the base snapshot on any store/resolve error (never-throw). + } + } const resetReviewerFallbackRetryCount = async (): Promise => { if (!options.store || !options.taskId || typeof options.store.updateTask !== "function") { diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 55c603a4d9..133ec1244d 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -31,6 +31,7 @@ import { isAbsolute, join, relative, resolve } from "node:path"; import { IN_REVIEW_STALL_DEADLOCK_LOG_PREFIX, IN_REVIEW_STALL_LOG_PREFIX, allowsAutoMergeProcessing, countRecentIdenticalStallEntries, detectDependencyCycle, detectSelfDefeatingDependency, getInReviewStalledSignal, getInReviewStallReason, getPrimaryPrInfo, getStalePausedReviewSignal, getStalePausedTodoSignal, getTaskHardMergeBlocker, getTaskMergeBlocker, isEphemeralAgent, isMergeRequestContractShadowEnabled, isWorkflowColumnsEnabled, isSharedBranchGroupMemberIntegration, parseExplicitDuplicateMarker, type AgentStore, type ChatStore, type MessageStore, type TaskStore, type Settings, type Task, type MergeDetails, type TaskPriority, type MergeResult } from "@fusion/core"; import type { MeshLeaseManager } from "./mesh-lease-manager.js"; import { createLogger, schedulerLog } from "./logger.js"; +import { mergeEffectiveSettings } from "./effective-settings.js"; import { RemovalReason, classifyTaskWorktree, getRegisteredWorktreeBranchMap, getRegisteredWorktreePaths, isUsableTaskWorktree, removeWorktree, resolveWorktreeBackend, scanIdleWorktrees, scanOrphanedBranches } from "./worktree-pool.js"; import { classifyMissingWorktreeSessionStartFailure, @@ -5008,12 +5009,20 @@ export class SelfHealingManager { try { const settings = await this.store.getSettings(); if (settings.globalPause || settings.enginePaused) return 0; - const maxFixes = settings.maxPostReviewFixes ?? 1; - if (!Number.isFinite(maxFixes) || maxFixes <= 0) return 0; const tasks = await this.store.listTasks({ column: "in-review", slim: true }); const executingIds = this.options.getExecutingTaskIds?.() ?? new Set(); + // Resolve the per-task effective `maxPostReviewFixes` (U3, KTD-3) — this is a + // cross-task recovery sweep, so the budget is resolved per task rather than + // from a single global read. Behavior-inert when nothing is customized. + const maxFixesByTask = new Map(); + for (const task of tasks) { + const eff = await mergeEffectiveSettings(this.store, task, settings); + maxFixesByTask.set(task.id, eff.maxPostReviewFixes ?? 1); + } + const maxFixesFor = (taskId: string): number => maxFixesByTask.get(taskId) ?? 1; + const candidates = tasks.filter((task) => { if (task.column !== "in-review") return false; if (!allowsAutoMergeProcessing(task, settings)) return false; @@ -5022,6 +5031,8 @@ export class SelfHealingManager { // merging, etc.). Only revive tasks that are otherwise idle. if (task.status) return false; if (executingIds.has(task.id)) return false; + const maxFixes = maxFixesFor(task.id); + if (!Number.isFinite(maxFixes) || maxFixes <= 0) return false; if ((task.postReviewFixCount ?? 0) >= maxFixes) return false; // Must have at least one failed pre-merge workflow step result. @@ -5051,6 +5062,7 @@ export class SelfHealingManager { let recovered = 0; for (const task of candidates) { const nextCount = (task.postReviewFixCount ?? 0) + 1; + const maxFixes = maxFixesFor(task.id); try { // Increment the counter BEFORE delegating so that even if the // executor path crashes or races, the budget is still consumed and diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 07a13fc8f2..67da4a1e96 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -35,6 +35,7 @@ import { resolvePlanningSessionModel, } from "./agent-session-helpers.js"; import { reviewStep, type ReviewVerdict } from "./reviewer.js"; +import { mergeEffectiveSettings } from "./effective-settings.js"; import { detectDanglingTaskDocReferences, formatDanglingDiagnostic } from "./spec-validation/task-document-references.js"; import { detectExternalIntegrationEvidenceGaps, @@ -911,7 +912,9 @@ export class TriageProcessor { return false; } - const settings = await this.store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) so requirePlanApproval + // resolves from the workflow. Behavior-inert when nothing is customized. + const settings = await mergeEffectiveSettings(this.store, task, await this.store.getSettings()); const promptPath = join(this.rootDir, ".fusion", "tasks", task.id, "PROMPT.md"); const written = await readFile(promptPath, "utf-8").catch((err: unknown) => { const msg = err instanceof Error ? err.message : String(err); @@ -1079,7 +1082,10 @@ export class TriageProcessor { try { const detail = await this.store.getTask(task.id); - const settings = await this.store.getSettings(); + // Merge per-task effective workflow settings (U3, KTD-3) over the base so the + // planning-phase reads (requirePlanApproval, planning/validator model lanes) + // pick up workflow values. Behavior-inert when nothing is customized. + const settings = await mergeEffectiveSettings(this.store, task, await this.store.getSettings()); const promptPath = `.fusion/tasks/${task.id}/PROMPT.md`; const isFast = task.executionMode === "fast"; @@ -2200,8 +2206,10 @@ export class TriageProcessor { } // Re-read settings at review time so long-lived triage sessions pick up - // model changes made after the session started. - const currentSettings = await store.getSettings(); + // model changes made after the session started. Merge per-task effective + // workflow settings (U3, KTD-3) so the validator model-lane reads below + // pick up workflow values. Behavior-inert when nothing is customized. + const currentSettings = await mergeEffectiveSettings(store, currentDetail, await store.getSettings()); // Spec reviewer runs via semaphore.runNested so it transiently // bumps activeCount for honest observability while bypassing the