fix: surface model-lane drift when a workflow's default model changes (#1958)
## Problem A task's model fields (`modelProvider`/`modelId` and the planning/validator equivalents) are snapshotted once at task-creation time from the workflow's model-lane default in `workflow_settings`. Nothing re-syncs, flags, or surfaces drift when that default is later changed. Concretely: the `builtin:coding` workflow's execution default was `claude-sonnet-4-6` until it was corrected on 2026-07-05. Every task created before that correction stayed permanently, invisibly pinned to the stale model id — 52 tasks were found silently stuck on it. ## Fix - `TaskStore.getModelLaneDrift(workflowId, before, after)` (`packages/core/src/store.ts`): read-only diff over the three model lanes (execution/planning/validator). For any lane whose provider+modelId actually changed, it lists the non-terminal (`column` not `archived`/`done`, not soft-deleted) tasks on that workflow still pinned to the old value. Never mutates `tasks`. - Wired into `PATCH /workflows/:id/setting-values` (`packages/dashboard/src/routes/register-workflow-routes.ts`): captures a `before` snapshot, runs the existing `updateWorkflowSettingValues` unchanged, then attaches an optional `modelDrift` field to the response when a lane change orphans existing tasks. Backward compatible — the field is only present when non-empty. - Operators can act on the surfaced drift via the existing `POST /tasks/batch-update-models` endpoint; this change intentionally does not auto-rewrite any task (avoids touching tasks mid-execution). ## Testing - New tests in `packages/core/src/__tests__/workflow-settings.test.ts` (`TaskStore.getModelLaneDrift`): verifies a task pinned to a changed lane's old value is surfaced, a task already on the new value and a `done`-column task are excluded, and an unrelated/unchanged lane produces no drift entry. - `packages/core`: `npx vitest run src/__tests__/workflow-settings.test.ts` — 24/24 pass. - `npx tsc --noEmit` clean in both `packages/core` and `packages/dashboard`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Workflow setting updates can now return a lane-based model drift summary (execution, planning, validator) showing task IDs still pinned to the previous model configuration. * The PATCH workflow setting response conditionally includes `modelDrift` when impacted tasks are found. * **Bug Fixes** * Drift detection now compares a consistent “before” snapshot with the updated values to avoid stale pairing. * “No workflow selection” tasks are handled correctly based on the default-workflow behavior. * **Tests** * Added coverage for lane drift across model changes and null-selection inclusion rules. * **Documentation** * Added a release note entry for the change. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
gsxdsm
7
.changeset/fix-model-lane-drift-visibility.md
Normal file
7
.changeset/fix-model-lane-drift-visibility.md
Normal file
@@ -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 })`.
|
||||
@@ -308,3 +308,153 @@ describe("TaskStore.updateWorkflowSettingValues", () => {
|
||||
expect(effective.workflowStepTimeoutMs).toBe(900_000);
|
||||
});
|
||||
});
|
||||
|
||||
describe("TaskStore.getModelLaneDrift", () => {
|
||||
const harness = createSharedTaskStoreTestHarness();
|
||||
|
||||
beforeAll(harness.beforeAll);
|
||||
afterAll(harness.afterAll);
|
||||
beforeEach(harness.beforeEach);
|
||||
afterEach(harness.afterEach);
|
||||
|
||||
it("flags non-terminal tasks still pinned to a lane's old value, and excludes done/unrelated tasks", 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);
|
||||
|
||||
const pinned = await store.createTask({
|
||||
description: "pinned to old model",
|
||||
workflowId: BUILTIN_CODING,
|
||||
modelProvider: "anthropic",
|
||||
modelId: "claude-sonnet-4-6",
|
||||
});
|
||||
const alreadyCurrent = await store.createTask({
|
||||
description: "already on the new model",
|
||||
workflowId: BUILTIN_CODING,
|
||||
modelProvider: "anthropic",
|
||||
modelId: "claude-sonnet-5",
|
||||
});
|
||||
const doneTask = await store.createTask({
|
||||
description: "terminal task, excluded even though pinned to the old model",
|
||||
workflowId: BUILTIN_CODING,
|
||||
modelProvider: "anthropic",
|
||||
modelId: "claude-sonnet-4-6",
|
||||
column: "done",
|
||||
});
|
||||
|
||||
await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, {
|
||||
executionModelId: "claude-sonnet-5",
|
||||
});
|
||||
const after = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT);
|
||||
|
||||
const drift = store.getModelLaneDrift(BUILTIN_CODING, before, after);
|
||||
expect(drift).toHaveLength(1);
|
||||
const execution = drift[0];
|
||||
expect(execution.lane).toBe("execution");
|
||||
expect(execution.from).toEqual({ provider: "anthropic", modelId: "claude-sonnet-4-6" });
|
||||
expect(execution.to).toEqual({ provider: "anthropic", modelId: "claude-sonnet-5" });
|
||||
expect(execution.taskIds).toEqual([pinned.id]);
|
||||
expect(execution.taskIds).not.toContain(alreadyCurrent.id);
|
||||
expect(execution.taskIds).not.toContain(doneTask.id);
|
||||
});
|
||||
|
||||
it("reports no drift when the lane value is unchanged", async () => {
|
||||
const store = harness.store();
|
||||
await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, {
|
||||
executionProvider: "anthropic",
|
||||
executionModelId: "claude-sonnet-5",
|
||||
});
|
||||
const before = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT);
|
||||
await store.updateWorkflowSettingValues(BUILTIN_CODING, PROJECT, { requirePrApproval: true });
|
||||
const after = store.getWorkflowSettingValues(BUILTIN_CODING, PROJECT);
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8847,6 +8847,22 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
|
||||
projectId: string,
|
||||
patch: Record<string, unknown>,
|
||||
): Promise<Record<string, unknown>> {
|
||||
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<string, unknown>,
|
||||
): Promise<{ previous: Record<string, unknown>; stored: Record<string, unknown> }> {
|
||||
const declarations = await this.resolveWorkflowSettingDeclarations(workflowId);
|
||||
const result = validateSettingValuePatch(declarations, patch);
|
||||
if (result.rejections.length > 0) {
|
||||
@@ -8861,8 +8877,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<string, unknown> = { ...current };
|
||||
const previous = this.getWorkflowSettingValues(workflowId, projectId);
|
||||
const next: Record<string, unknown> = { ...previous };
|
||||
for (const [key, value] of Object.entries(result.accepted)) {
|
||||
if (value === null) {
|
||||
delete next[key];
|
||||
@@ -8881,7 +8897,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 };
|
||||
});
|
||||
}
|
||||
|
||||
@@ -15855,6 +15873,87 @@ ${stepsSection}`;
|
||||
return ids;
|
||||
}
|
||||
|
||||
/** Model-lane setting keys (workflow_settings) paired with the task columns
|
||||
* they seed at task-creation time (a permanent point-in-time snapshot,
|
||||
* never re-synced when the workflow default later changes). */
|
||||
private static readonly MODEL_LANES = [
|
||||
{ lane: "execution", providerKey: "executionProvider", modelIdKey: "executionModelId", providerCol: "modelProvider", modelIdCol: "modelId" },
|
||||
{ lane: "planning", providerKey: "planningProvider", modelIdKey: "planningModelId", providerCol: "planningModelProvider", modelIdCol: "planningModelId" },
|
||||
{ lane: "validator", providerKey: "validatorProvider", modelIdKey: "validatorModelId", providerCol: "validatorModelProvider", modelIdCol: "validatorModelId" },
|
||||
] as const;
|
||||
|
||||
/**
|
||||
* Diff `before`/`after` workflow setting values for the three model lanes
|
||||
* and, for each lane whose provider+modelId pair changed, list the
|
||||
* non-terminal tasks on this workflow still pinned to the OLD pair. A
|
||||
* task's model columns are captured once at creation and never re-synced,
|
||||
* 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<string, unknown>,
|
||||
after: Record<string, unknown>,
|
||||
options?: { includeNullSelection?: boolean },
|
||||
): Array<{
|
||||
lane: string;
|
||||
from: { provider: string | null; modelId: string | null };
|
||||
to: { provider: string | null; modelId: string | null };
|
||||
taskIds: string[];
|
||||
}> {
|
||||
const asStringOrNull = (v: unknown): string | null => (typeof v === "string" ? v : null);
|
||||
const includeNullSelection =
|
||||
options?.includeNullSelection ?? workflowId === TaskStore.DEFAULT_WORKFLOW_POOL_ID;
|
||||
const drift: Array<{
|
||||
lane: string;
|
||||
from: { provider: string | null; modelId: string | null };
|
||||
to: { provider: string | null; modelId: string | null };
|
||||
taskIds: string[];
|
||||
}> = [];
|
||||
|
||||
for (const l of TaskStore.MODEL_LANES) {
|
||||
const fromProvider = asStringOrNull(before[l.providerKey]);
|
||||
const fromModelId = asStringOrNull(before[l.modelIdKey]);
|
||||
const toProvider = asStringOrNull(after[l.providerKey]);
|
||||
const toModelId = asStringOrNull(after[l.modelIdKey]);
|
||||
if (fromProvider === toProvider && fromModelId === toModelId) continue;
|
||||
if (fromProvider === null && fromModelId === null) continue;
|
||||
|
||||
const occupants = this.listWorkflowOccupantTaskIds(workflowId, includeNullSelection);
|
||||
let taskIds: string[] = [];
|
||||
if (occupants.length > 0) {
|
||||
const placeholders = occupants.map(() => "?").join(",");
|
||||
const rows = this.db
|
||||
.prepare(
|
||||
`SELECT id FROM tasks
|
||||
WHERE id IN (${placeholders})
|
||||
AND "deletedAt" IS NULL
|
||||
AND "column" != 'archived'
|
||||
AND "column" != 'done'
|
||||
AND "${l.providerCol}" IS ?
|
||||
AND "${l.modelIdCol}" IS ?`,
|
||||
)
|
||||
.all(...occupants, fromProvider, fromModelId) as Array<{ id: string }>;
|
||||
taskIds = rows.map((r) => r.id);
|
||||
}
|
||||
drift.push({
|
||||
lane: l.lane,
|
||||
from: { provider: fromProvider, modelId: fromModelId },
|
||||
to: { provider: toProvider, modelId: toModelId },
|
||||
taskIds,
|
||||
});
|
||||
}
|
||||
return drift;
|
||||
}
|
||||
|
||||
/** Map column id → occupant count for the tasks selecting `workflowId`
|
||||
* (plus null-selection tasks when `includeNullSelection`). */
|
||||
private occupantsByColumnForWorkflow(
|
||||
|
||||
@@ -481,16 +481,35 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
|
||||
await assertWorkflowExists(store, workflowId);
|
||||
const projectId = store.getWorkflowSettingsProjectId();
|
||||
try {
|
||||
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<string, unknown>,
|
||||
);
|
||||
const declarations = await resolveSettingDeclarations(store, workflowId);
|
||||
// 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.
|
||||
// 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),
|
||||
orphaned: findOrphanedSettingValues(declarations, stored),
|
||||
...(modelDrift.length > 0 ? { modelDrift } : {}),
|
||||
});
|
||||
} catch (writeErr: unknown) {
|
||||
// Typed rejection → 400 with the structured rejections so the client can
|
||||
|
||||
Reference in New Issue
Block a user