From 62c58402ff289c554a3c88ed920916472a60bfe5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 1 Jul 2026 08:11:18 -0700 Subject: [PATCH] fix(workflow): raise step review timeout default --- .changeset/workflow-step-timeout-default.md | 7 +++++++ docs/settings-reference.md | 1 + .../effective-settings-overlay.test.ts | 2 +- .../src/__tests__/settings-migration.test.ts | 6 +++--- .../__tests__/workflow-ir-settings.test.ts | 14 +++++++------ .../__tests__/workflow-settings-e2e.test.ts | 2 +- .../workflow-settings-resolver.test.ts | 10 +++++----- .../src/__tests__/workflow-settings.test.ts | 10 +++++----- .../core/src/builtin-workflow-settings.ts | 20 +++++++++++-------- packages/core/src/types.ts | 2 +- .../task-effective-settings-route.test.ts | 2 +- .../effective-settings-merge.test.ts | 14 ++++++------- ...rkflow-settings-fallback-alignment.test.ts | 2 +- packages/engine/src/executor.ts | 2 +- 14 files changed, 54 insertions(+), 40 deletions(-) create mode 100644 .changeset/workflow-step-timeout-default.md diff --git a/.changeset/workflow-step-timeout-default.md b/.changeset/workflow-step-timeout-default.md new file mode 100644 index 0000000000..38a3cadc7e --- /dev/null +++ b/.changeset/workflow-step-timeout-default.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Give workflow review steps a longer default timeout. +category: fix +dev: Raises the built-in `workflowStepTimeoutMs` default and engine fallback from 6 minutes to 15 minutes. diff --git a/docs/settings-reference.md b/docs/settings-reference.md index 85d5435504..721589fdc2 100644 --- a/docs/settings-reference.md +++ b/docs/settings-reference.md @@ -505,6 +505,7 @@ Default notes: | `validatorModelId` | `string` | `undefined` | Model ID for plan/code reviewers. | | `validatorFallbackProvider` | `string` | `undefined` | Fallback provider for reviewers; also used by reviewer UNAVAILABLE/error recovery retry before returning terminal UNAVAILABLE. | | `validatorFallbackModelId` | `string` | `undefined` | Fallback model ID for reviewers; paired with `validatorFallbackProvider` for reviewer recovery retry. | +| `workflowStepTimeoutMs` | `number` | `900000` | Maximum time in milliseconds a single workflow step may run before it is timed out. | | `modelPresets` | `ModelPreset[]` | `[]` | Reusable executor/reviewer model presets. | | `autoSelectModelPreset` | `boolean` | `false` | Auto-select presets by task size. | | `defaultPresetBySize` | `{ S?: string; M?: string; L?: string }` | `{}` | Mapping for `S`/`M`/`L` → preset ID. | diff --git a/packages/core/src/__tests__/effective-settings-overlay.test.ts b/packages/core/src/__tests__/effective-settings-overlay.test.ts index 86b38526b1..9bbb71d7ff 100644 --- a/packages/core/src/__tests__/effective-settings-overlay.test.ts +++ b/packages/core/src/__tests__/effective-settings-overlay.test.ts @@ -25,7 +25,7 @@ describe("applyWorkflowSettingsOverlay", () => { planningFallbackModelId: "workflow-planner-fallback-model", validatorFallbackProvider: "workflow-validator-fallback", validatorFallbackModelId: "workflow-validator-fallback-model", - workflowStepTimeoutMs: 360_000, + workflowStepTimeoutMs: 900_000, runStepsInNewSessions: false, maxParallelSteps: undefined, }, diff --git a/packages/core/src/__tests__/settings-migration.test.ts b/packages/core/src/__tests__/settings-migration.test.ts index 6f4e465f9c..57b1926019 100644 --- a/packages/core/src/__tests__/settings-migration.test.ts +++ b/packages/core/src/__tests__/settings-migration.test.ts @@ -176,8 +176,8 @@ describe("settings hard-move migration (U4)", () => { 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); + // Declaration defaults: workflowStepTimeoutMs=900000, requirePrApproval=false. + expect(effective.workflowStepTimeoutMs).toBe(900_000); expect(effective.requirePrApproval).toBe(false); }); @@ -233,7 +233,7 @@ describe("settings hard-move migration (U4)", () => { ], edges: [{ from: "start", to: "end" }], settings: [ - { id: "workflowStepTimeoutMs", name: "Step timeout", type: "number", default: 360_000 }, + { id: "workflowStepTimeoutMs", name: "Step timeout", type: "number", default: 900_000 }, { id: "requirePrApproval", name: "Require PR approval", type: "boolean", default: false }, ], }, diff --git a/packages/core/src/__tests__/workflow-ir-settings.test.ts b/packages/core/src/__tests__/workflow-ir-settings.test.ts index 9f2982acc6..7ed2ebf953 100644 --- a/packages/core/src/__tests__/workflow-ir-settings.test.ts +++ b/packages/core/src/__tests__/workflow-ir-settings.test.ts @@ -6,6 +6,7 @@ import { WorkflowIrError, } from "../workflow-ir.js"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR } from "../builtin-stepwise-final-review-coding-workflow-ir.js"; import { getBuiltinWorkflow } from "../builtin-workflows.js"; import { BUILTIN_MOVED_WORKFLOW_SETTINGS, @@ -218,11 +219,12 @@ describe("built-in workflow settings parity anchor (U1, R4)", () => { expect(declaredIds.has(setting.id)).toBe(true); } expect(builtin.settings).toEqual(BUILTIN_WORKFLOW_SETTINGS); - expect(getBuiltinWorkflow("builtin:coding")!.ir).toBe(BUILTIN_CODING_WORKFLOW_IR); + expect(getBuiltinWorkflow("builtin:coding")!.ir).toBe(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR); + expect(getBuiltinWorkflow("builtin:legacy-coding")!.ir).toBe(BUILTIN_CODING_WORKFLOW_IR); expect((getBuiltinWorkflow("builtin:coding")!.ir as WorkflowIrV2).settings).toEqual(BUILTIN_WORKFLOW_SETTINGS); }); - it("the moved-key catalog has left DEFAULT_PROJECT_SETTINGS (U4 hard-move) and pins its legacy defaults", () => { + it("the moved-key catalog has left DEFAULT_PROJECT_SETTINGS (U4 hard-move) and pins current built-in 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 @@ -230,11 +232,11 @@ describe("built-in workflow settings parity anchor (U1, R4)", () => { for (const setting of BUILTIN_MOVED_WORKFLOW_SETTINGS) { 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. + // The declaration defaults are now the single source of truth; pin current + // built-in values explicitly so they can never silently drift from engine + // read-site fallbacks or workflow editor display. const expectedDefaults: Record = { - workflowStepTimeoutMs: 360_000, + workflowStepTimeoutMs: 900_000, workflowStepScopeEnforcement: "block", planOnlyScopeLeakEnforcement: "warn", workflowRevisionForkOnScopeMismatch: true, diff --git a/packages/core/src/__tests__/workflow-settings-e2e.test.ts b/packages/core/src/__tests__/workflow-settings-e2e.test.ts index 614dd217c1..18d3694798 100644 --- a/packages/core/src/__tests__/workflow-settings-e2e.test.ts +++ b/packages/core/src/__tests__/workflow-settings-e2e.test.ts @@ -252,7 +252,7 @@ describe("workflow-settings end-to-end journey (U10)", () => { // The fresh project has declaration defaults (NOT the source project's values). const freshBefore = await resolveEffectiveSettingsById(resolverStore(store2), "builtin:coding", projectId2); - expect(freshBefore.workflowStepTimeoutMs).toBe(360_000); // legacy/declaration default + expect(freshBefore.workflowStepTimeoutMs).toBe(900_000); // declaration default expect(freshBefore.requirePrApproval).toBe(false); // ── Import the v2 export → effective values match the exported project, diff --git a/packages/core/src/__tests__/workflow-settings-resolver.test.ts b/packages/core/src/__tests__/workflow-settings-resolver.test.ts index 02d159c959..bc6f7ef1c6 100644 --- a/packages/core/src/__tests__/workflow-settings-resolver.test.ts +++ b/packages/core/src/__tests__/workflow-settings-resolver.test.ts @@ -227,7 +227,7 @@ describe("resolveEffectiveSettings (per-task)", () => { 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); + expect(eff.workflowStepTimeoutMs).toBe(900_000); }); it("missing custom definition degrades to builtin declarations (never throws)", async () => { @@ -237,13 +237,13 @@ describe("resolveEffectiveSettings (per-task)", () => { }); const eff = await resolveEffectiveSettings(store, { id: "t1" }); // Degrades to BUILTIN_CODING_WORKFLOW_IR declarations. - expect(eff.workflowStepTimeoutMs).toBe(360_000); + expect(eff.workflowStepTimeoutMs).toBe(900_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); + expect(eff.workflowStepTimeoutMs).toBe(900_000); }); it("store value read throwing degrades to declaration defaults", async () => { @@ -252,7 +252,7 @@ describe("resolveEffectiveSettings (per-task)", () => { valuesThrows: true, }); const eff = await resolveEffectiveSettings(store, { id: "t1" }); - expect(eff.workflowStepTimeoutMs).toBe(360_000); + expect(eff.workflowStepTimeoutMs).toBe(900_000); }); it("project-id lookup throwing degrades to declaration defaults (empty stored map)", async () => { @@ -263,7 +263,7 @@ describe("resolveEffectiveSettings (per-task)", () => { }); 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); + expect(eff.workflowStepTimeoutMs).toBe(900_000); }); }); diff --git a/packages/core/src/__tests__/workflow-settings.test.ts b/packages/core/src/__tests__/workflow-settings.test.ts index 1f677e4e87..894eeb4872 100644 --- a/packages/core/src/__tests__/workflow-settings.test.ts +++ b/packages/core/src/__tests__/workflow-settings.test.ts @@ -33,7 +33,7 @@ const TIMEOUT_DECL: WorkflowSettingDefinition = { id: "workflowStepTimeoutMs", name: "Step timeout (ms)", type: "number", - default: 360_000, + default: 900_000, }; const FLAG_DECL: WorkflowSettingDefinition = { id: "runStepsInNewSessions", @@ -131,7 +131,7 @@ describe("resolveEffectiveSettingValues", () => { it("falls to the declaration default when unset", () => { const eff = resolveEffectiveSettingValues([TIMEOUT_DECL], {}); - expect(eff).toEqual({ workflowStepTimeoutMs: 360_000 }); + expect(eff).toEqual({ workflowStepTimeoutMs: 900_000 }); }); it("drops a stored value that no longer validates (enum→number retype) and uses the default", () => { @@ -143,7 +143,7 @@ describe("resolveEffectiveSettingValues", () => { it("drops stored values for ids with no current declaration", () => { const eff = resolveEffectiveSettingValues([TIMEOUT_DECL], { removedSetting: 7 }); - expect(eff).toEqual({ workflowStepTimeoutMs: 360_000 }); + expect(eff).toEqual({ workflowStepTimeoutMs: 900_000 }); }); it("omits a setting with neither a valid value nor a default", () => { @@ -248,7 +248,7 @@ describe("TaskStore.updateWorkflowSettingValues", () => { const def = await store.getWorkflowDefinition(wfId); const decls = def!.ir.version === "v2" ? def!.ir.settings : undefined; - expect(resolveEffectiveSettingValues(decls, stored)).toEqual({ workflowStepTimeoutMs: 360_000 }); + expect(resolveEffectiveSettingValues(decls, stored)).toEqual({ workflowStepTimeoutMs: 900_000 }); }); it("retype enum→number with a stale stored string: effective resolution drops it, returns default, stored row untouched", async () => { @@ -305,6 +305,6 @@ describe("TaskStore.updateWorkflowSettingValues", () => { ); expect(effective.requirePrApproval).toBe(true); // Untouched built-in keys resolve to their declaration defaults. - expect(effective.workflowStepTimeoutMs).toBe(360_000); + expect(effective.workflowStepTimeoutMs).toBe(900_000); }); }); diff --git a/packages/core/src/builtin-workflow-settings.ts b/packages/core/src/builtin-workflow-settings.ts index 24979deef0..4646ea0681 100644 --- a/packages/core/src/builtin-workflow-settings.ts +++ b/packages/core/src/builtin-workflow-settings.ts @@ -6,7 +6,8 @@ import type { WorkflowSettingDefinition } from "./workflow-ir-types.js"; * * `BUILTIN_MOVED_WORKFLOW_SETTINGS` is the U4 moved-key catalog: keys that * formerly lived in `DEFAULT_PROJECT_SETTINGS` and are tombstoned by - * `MOVED_SETTINGS_KEYS`. Keep those defaults byte-equal to the legacy literals. + * `MOVED_SETTINGS_KEYS`. Defaults should stay aligned with engine fallback + * literals; intentional product-default changes must update both together. * * `BUILTIN_TRIAGE_POLICY_SETTINGS` is workflow-native triage/spec policy. These * keys never lived in `DEFAULT_PROJECT_SETTINGS`, are NOT part of the U4 @@ -29,12 +30,11 @@ import type { WorkflowSettingDefinition } from "./workflow-ir-types.js"; * (`builtin-coding-workflow-ir.ts`, `builtin-stepwise-coding-workflow-ir.ts`) so * the catalog has exactly one definition. * - * Each `default` here MUST be byte-equal to the corresponding literal in - * `DEFAULT_PROJECT_SETTINGS` (`settings-schema.ts`) — this is the parity anchor - * for the U4 hard-move migration. The U1 test - * (`workflow-ir-settings.test.ts`) asserts strict equality against the legacy - * literals. Keys with `undefined` legacy defaults (the per-phase model lanes) - * omit `default` entirely, which round-trips to the same effective value. + * Each `default` here must stay aligned with the corresponding engine read-site + * fallback literal so projects without stored workflow values execute with the + * same policy the Workflow Editor displays. Keys with `undefined` legacy + * defaults (the per-phase model lanes) omit `default` entirely, which + * round-trips to the same effective value. * * NOTE: these declarations are inert in U1 — nothing reads them until the * effective-settings resolver and engine integration land (U3). Adding them does @@ -51,7 +51,11 @@ export const BUILTIN_MOVED_WORKFLOW_SETTINGS: WorkflowSettingDefinition[] = [ id: "workflowStepTimeoutMs", name: "Step timeout (ms)", type: "number", - default: 360_000, + /* + * FNXC:WorkflowReview 2026-07-01-08:09: + * Code Review steps can exceed the old six-minute default on ordinary dashboard changes. Use a fifteen-minute workflow-step default so every project without an override gets enough reviewer time while keeping runaway sessions bounded. + */ + default: 900_000, description: "Maximum time a single workflow step may run before it is timed out.", }, { diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 6c2e6feb27..d62e0716c3 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -3945,7 +3945,7 @@ export interface ProjectSettings { /** Wall-clock timeout (ms) for a single pre-merge workflow step's AI call. * When a step exceeds this, the session is aborted and the executor is * given one shot to retry with the configured fallback model before the - * step is reported as failed. Default: 360_000 (6 minutes). */ + * step is reported as failed. Default: 900_000 (15 minutes). */ workflowStepTimeoutMs?: number; /** How pre-merge prompt workflow steps enforce declared File Scope at step end. * - "block" (default): mark the step failed/revision-requested on off-scope writes diff --git a/packages/dashboard/src/__tests__/task-effective-settings-route.test.ts b/packages/dashboard/src/__tests__/task-effective-settings-route.test.ts index 8c988dbe64..3516eb080a 100644 --- a/packages/dashboard/src/__tests__/task-effective-settings-route.test.ts +++ b/packages/dashboard/src/__tests__/task-effective-settings-route.test.ts @@ -17,7 +17,7 @@ class MockStore extends EventEmitter { getSettingsFast = vi.fn(async (): Promise => ({ defaultProvider: "base-default-provider", defaultModelId: "base-default-model", - workflowStepTimeoutMs: 360_000, + workflowStepTimeoutMs: 900_000, runStepsInNewSessions: false, } as Settings)); getTaskWorkflowSelection = vi.fn((taskId: string) => this.workflowSelections.get(taskId)); diff --git a/packages/engine/src/__tests__/effective-settings-merge.test.ts b/packages/engine/src/__tests__/effective-settings-merge.test.ts index f3d84e78ad..f63391da1a 100644 --- a/packages/engine/src/__tests__/effective-settings-merge.test.ts +++ b/packages/engine/src/__tests__/effective-settings-merge.test.ts @@ -8,7 +8,7 @@ const PROJECT = "proj-1"; /** A base settings object with real project values for the keys under test. */ function baseSettings(): Settings { return { - workflowStepTimeoutMs: 360_000, + workflowStepTimeoutMs: 900_000, requirePrApproval: false, runStepsInNewSessions: false, // A real project value for an absent-default lane — must NOT be clobbered. @@ -36,8 +36,8 @@ describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { 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); + // Declaration defaults match the engine read-site fallback, so these don't change. + expect(merged.workflowStepTimeoutMs).toBe(900_000); expect(merged.requirePrApproval).toBe(false); expect(merged.runStepsInNewSessions).toBe(false); }); @@ -58,7 +58,7 @@ describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { 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) + expect(merged.workflowStepTimeoutMs).toBe(12_345); // not the declaration default (900_000) }); it("post-migration fill: a declaration default fills when the base lacks the key", async () => { @@ -66,7 +66,7 @@ describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { // 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 + expect(merged.workflowStepTimeoutMs).toBe(900_000); // filled from declaration default }); it("a stored value overrides the base", async () => { @@ -87,7 +87,7 @@ describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { 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(base.workflowStepTimeoutMs).toBe(900_000); expect(merged).not.toBe(base); }); @@ -110,7 +110,7 @@ describe("mergeEffectiveSettings (engine entry merge, U3/KTD-3)", () => { 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.workflowStepTimeoutMs).toBe(900_000); expect(merged.executionProvider).toBe("project-anthropic"); }); }); 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 666f1502ea..391f0fe42c 100644 --- a/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts +++ b/packages/engine/src/__tests__/workflow-settings-fallback-alignment.test.ts @@ -85,7 +85,7 @@ describe("workflow-settings fallback alignment (KTD-3, item 4)", () => { // 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 + workflowStepTimeoutMs: 900_000, // executor.ts: ?? 900_000 workflowStepScopeEnforcement: "block", // executor.ts: ?? "block" planOnlyScopeLeakEnforcement: "warn", // executor.ts: ?? "warn" workflowRevisionForkOnScopeMismatch: true, // executor.ts: !== false (default-true) diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 4c7621d0c4..615ca36c12 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -14351,7 +14351,7 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB (c) => c.provider && c.modelId && (c.provider !== primaryProvider || c.modelId !== primaryModelId), ); - const timeoutMs = Math.max(60_000, settings.workflowStepTimeoutMs ?? 360_000); + const timeoutMs = Math.max(60_000, settings.workflowStepTimeoutMs ?? 900_000); const runOnce = async ( provider: string | undefined,