From 176222918a93ba3c418b6664aa64a3932d955d63 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 8 Jul 2026 07:26:26 -0700 Subject: [PATCH] fix(dashboard): address model-lane drift review feedback - getModelLaneDrift now takes an explicit includeNullSelection option; the setting-values route passes it when patching the project default workflow so no-workflow-selection tasks (which resolve through the default) are counted instead of silently dropped (Greptile P1). - Add updateWorkflowSettingValuesWithPrevious so the drift baseline is captured inside the settings write transaction, removing the stale-read race against a concurrent patch of the same row (Greptile P2). - Broaden getModelLaneDrift tests to cover planning and validator lanes and the null-selection/default-workflow case (FN-5893 invariant across surfaces). - Add changeset. CodeRabbit's effective-values suggestion is intentionally skipped: model-lane declarations carry no declaration-level default (KTD-7), so effective == raw for these keys and comparing effective values is a no-op. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fix-model-lane-drift-visibility.md | 7 ++ .../src/__tests__/workflow-settings.test.ts | 83 +++++++++++++++++++ packages/core/src/store.ts | 35 +++++++- .../src/routes/register-workflow-routes.ts | 19 ++++- 4 files changed, 137 insertions(+), 7 deletions(-) create mode 100644 .changeset/fix-model-lane-drift-visibility.md diff --git a/.changeset/fix-model-lane-drift-visibility.md b/.changeset/fix-model-lane-drift-visibility.md new file mode 100644 index 0000000000..21b3870e87 --- /dev/null +++ b/.changeset/fix-model-lane-drift-visibility.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Warn when changing a workflow's model surfaces tasks still pinned to the old model, including default-workflow tasks. +category: fix +dev: PATCH /workflows/:id/setting-values now returns `modelDrift` for execution/planning/validator lanes. Drift baseline is captured inside the settings write transaction (no stale-read race), and default-workflow patches pass `includeNullSelection` so no-workflow-selection tasks are counted. New `TaskStore.updateWorkflowSettingValuesWithPrevious` and `getModelLaneDrift(..., { includeNullSelection })`. diff --git a/packages/core/src/__tests__/workflow-settings.test.ts b/packages/core/src/__tests__/workflow-settings.test.ts index 6eb7bb2628..75f2a161b0 100644 --- a/packages/core/src/__tests__/workflow-settings.test.ts +++ b/packages/core/src/__tests__/workflow-settings.test.ts @@ -374,4 +374,87 @@ describe("TaskStore.getModelLaneDrift", () => { expect(store.getModelLaneDrift(BUILTIN_CODING, before, after)).toEqual([]); }); + + // FN-5893: the invariant holds across ALL model lanes, not only `execution`. + it("flags the planning lane's pinned tasks when the planning model changes", async () => { + const store = harness.store(); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + planningProvider: "anthropic", + planningModelId: "claude-opus-4-6", + }); + const before = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + const pinned = await store.createTask({ + description: "pinned to old planning model", + workflowId: BUILTIN_CODING, + planningModelProvider: "anthropic", + planningModelId: "claude-opus-4-6", + }); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + planningModelId: "claude-opus-4-8", + }); + const after = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + + const drift = store.getModelLaneDrift(BUILTIN_CODING, before, after); + expect(drift).toHaveLength(1); + expect(drift[0].lane).toBe("planning"); + expect(drift[0].taskIds).toEqual([pinned.id]); + }); + + it("flags the validator lane's pinned tasks when the validator model changes", async () => { + const store = harness.store(); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + validatorProvider: "anthropic", + validatorModelId: "claude-haiku-4-5", + }); + const before = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + const pinned = await store.createTask({ + description: "pinned to old validator model", + workflowId: BUILTIN_CODING, + validatorModelProvider: "anthropic", + validatorModelId: "claude-haiku-4-5", + }); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + validatorModelId: "claude-haiku-5", + }); + const after = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + + const drift = store.getModelLaneDrift(BUILTIN_CODING, before, after); + expect(drift).toHaveLength(1); + expect(drift[0].lane).toBe("validator"); + expect(drift[0].taskIds).toEqual([pinned.id]); + }); + + // Greptile P1: when the default workflow is diffed, no-selection tasks resolve + // through it and are pinned to its lane values, so they must be counted — but + // only when the caller opts in via `includeNullSelection`. + it("includes no-workflow-selection tasks only when includeNullSelection is set", async () => { + const store = harness.store(); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + executionProvider: "anthropic", + executionModelId: "claude-sonnet-4-6", + }); + const before = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + // No workflowId → no task_workflow_selection row → resolves to the default. + const nullSelected = await store.createTask({ + description: "no workflow selection, pinned to old model", + modelProvider: "anthropic", + modelId: "claude-sonnet-4-6", + }); + await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { + executionModelId: "claude-sonnet-5", + }); + const after = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT); + + // Default excludes null-selection tasks: the route passes a concrete id. + const withoutNull = store.getModelLaneDrift(BUILTIN_CODING, before, after); + expect(withoutNull).toHaveLength(1); + expect(withoutNull[0].taskIds).not.toContain(nullSelected.id); + + // Opt in (the route does this when patching the default workflow). + const withNull = store.getModelLaneDrift(BUILTIN_CODING, before, after, { + includeNullSelection: true, + }); + expect(withNull).toHaveLength(1); + expect(withNull[0].taskIds).toContain(nullSelected.id); + }); }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 94af3cc741..7f026e5e60 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -8798,6 +8798,22 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} projectId: string, patch: Record, ): Promise> { + return (await this.updateWorkflowSettingValuesWithPrevious(workflowId, projectId, patch)).stored; + } + + /** + * Like {@link updateWorkflowSettingValues} but also returns the row's value + * snapshot as it was read INSIDE the same serialized transaction, immediately + * before the merge. Callers that diff before→after (e.g. model-lane drift on + * `PATCH /workflows/:id/setting-values`) must use this — reading `before` + * outside the write transaction races a concurrent patch of the same row and + * can pair a stale `previous` with another writer's `stored` (Greptile P2). + */ + async updateWorkflowSettingValuesWithPrevious( + workflowId: string, + projectId: string, + patch: Record, + ): Promise<{ previous: Record; stored: Record }> { const declarations = await this.resolveWorkflowSettingDeclarations(workflowId); const result = validateSettingValuePatch(declarations, patch); if (result.rejections.length > 0) { @@ -8812,8 +8828,8 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} // write transaction. Validation/declaration resolution above stays outside // since it's async and doesn't read the row being mutated. return this.db.transactionImmediate(() => { - const current = this.getWorkflowSettingValues(workflowId, projectId); - const next: Record = { ...current }; + const previous = this.getWorkflowSettingValues(workflowId, projectId); + const next: Record = { ...previous }; for (const [key, value] of Object.entries(result.accepted)) { if (value === null) { delete next[key]; @@ -8832,7 +8848,9 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS} ) .run(workflowId, projectId, JSON.stringify(next), now); this.db.bumpLastModified(); - return next; + // `previous` is captured before the merge; it and `next` are a consistent + // before/after pair from the same transaction snapshot. + return { previous, stored: next }; }); } @@ -15823,11 +15841,19 @@ ${stepsSection}`; * so changing a workflow's default silently orphans already-created tasks * unless this drift is surfaced. Read-only — never mutates `tasks`; pair * with `POST /tasks/batch-update-models` to actually re-pin flagged ids. + * + * When the diffed workflow is the project default, no-selection tasks resolve + * THROUGH it and are pinned to its lane values, so they must be counted as + * occupants. Callers pass `includeNullSelection: true` in that case (the + * route never hands us the internal `DEFAULT_WORKFLOW_POOL_ID` sentinel — it + * has the concrete default id — so we cannot infer this from `workflowId` + * alone; Greptile P1). When omitted, it falls back to the sentinel check. */ getModelLaneDrift( workflowId: string, before: Record, after: Record, + options?: { includeNullSelection?: boolean }, ): Array<{ lane: string; from: { provider: string | null; modelId: string | null }; @@ -15835,7 +15861,8 @@ ${stepsSection}`; taskIds: string[]; }> { const asStringOrNull = (v: unknown): string | null => (typeof v === "string" ? v : null); - const includeNullSelection = workflowId === TaskStore.DEFAULT_WORKFLOW_POOL_ID; + const includeNullSelection = + options?.includeNullSelection ?? workflowId === TaskStore.DEFAULT_WORKFLOW_POOL_ID; const drift: Array<{ lane: string; from: { provider: string | null; modelId: string | null }; diff --git a/packages/dashboard/src/routes/register-workflow-routes.ts b/packages/dashboard/src/routes/register-workflow-routes.ts index 91f3880a54..43b936203f 100644 --- a/packages/dashboard/src/routes/register-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-workflow-routes.ts @@ -481,8 +481,11 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void { await assertWorkflowExists(store, workflowId); const projectId = store.getWorkflowSettingsProjectId(); try { - const before = store.getWorkflowSettingValues(workflowId, projectId); - const stored = await store.updateWorkflowSettingValues( + // FNXC:ModelLaneDrift 2026-07-08-07:24: + // Capture `before` INSIDE the write transaction (paired with `stored`) + // so a concurrent patch of the same row cannot pair a stale baseline + // with another writer's result (Greptile P2). + const { previous: before, stored } = await store.updateWorkflowSettingValuesWithPrevious( workflowId, projectId, values as Record, @@ -491,7 +494,17 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void { // Model-lane drift: tasks already pinned to the value being replaced // are never auto-resynced (see getModelLaneDrift), so surface them // here rather than let the change silently orphan those tasks. - const modelDrift = store.getModelLaneDrift(workflowId, before, stored).filter((d) => d.taskIds.length > 0); + // FNXC:ModelLaneDrift 2026-07-08-07:24: + // When the patched workflow is the project default, no-selection tasks + // resolve through it and must be counted, so flag includeNullSelection + // then (Greptile P1) — the route holds the concrete default id, not the + // internal DEFAULT_WORKFLOW_POOL_ID sentinel getModelLaneDrift checks. + const defaultWorkflowId = (await store.getDefaultWorkflowId()) ?? "builtin:coding"; + const modelDrift = store + .getModelLaneDrift(workflowId, before, stored, { + includeNullSelection: workflowId === defaultWorkflowId, + }) + .filter((d) => d.taskIds.length > 0); res.json({ stored, effective: resolveEffectiveSettingValues(declarations, stored),