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) <noreply@anthropic.com>
This commit is contained in:
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 })`.
|
||||||
@@ -374,4 +374,87 @@ describe("TaskStore.getModelLaneDrift", () => {
|
|||||||
|
|
||||||
expect(store.getModelLaneDrift(BUILTIN_CODING, before, after)).toEqual([]);
|
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);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -8798,6 +8798,22 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
|
|||||||
projectId: string,
|
projectId: string,
|
||||||
patch: Record<string, unknown>,
|
patch: Record<string, unknown>,
|
||||||
): Promise<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 declarations = await this.resolveWorkflowSettingDeclarations(workflowId);
|
||||||
const result = validateSettingValuePatch(declarations, patch);
|
const result = validateSettingValuePatch(declarations, patch);
|
||||||
if (result.rejections.length > 0) {
|
if (result.rejections.length > 0) {
|
||||||
@@ -8812,8 +8828,8 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
|
|||||||
// write transaction. Validation/declaration resolution above stays outside
|
// write transaction. Validation/declaration resolution above stays outside
|
||||||
// since it's async and doesn't read the row being mutated.
|
// since it's async and doesn't read the row being mutated.
|
||||||
return this.db.transactionImmediate(() => {
|
return this.db.transactionImmediate(() => {
|
||||||
const current = this.getWorkflowSettingValues(workflowId, projectId);
|
const previous = this.getWorkflowSettingValues(workflowId, projectId);
|
||||||
const next: Record<string, unknown> = { ...current };
|
const next: Record<string, unknown> = { ...previous };
|
||||||
for (const [key, value] of Object.entries(result.accepted)) {
|
for (const [key, value] of Object.entries(result.accepted)) {
|
||||||
if (value === null) {
|
if (value === null) {
|
||||||
delete next[key];
|
delete next[key];
|
||||||
@@ -8832,7 +8848,9 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
|
|||||||
)
|
)
|
||||||
.run(workflowId, projectId, JSON.stringify(next), now);
|
.run(workflowId, projectId, JSON.stringify(next), now);
|
||||||
this.db.bumpLastModified();
|
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
|
* so changing a workflow's default silently orphans already-created tasks
|
||||||
* unless this drift is surfaced. Read-only — never mutates `tasks`; pair
|
* unless this drift is surfaced. Read-only — never mutates `tasks`; pair
|
||||||
* with `POST /tasks/batch-update-models` to actually re-pin flagged ids.
|
* 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(
|
getModelLaneDrift(
|
||||||
workflowId: string,
|
workflowId: string,
|
||||||
before: Record<string, unknown>,
|
before: Record<string, unknown>,
|
||||||
after: Record<string, unknown>,
|
after: Record<string, unknown>,
|
||||||
|
options?: { includeNullSelection?: boolean },
|
||||||
): Array<{
|
): Array<{
|
||||||
lane: string;
|
lane: string;
|
||||||
from: { provider: string | null; modelId: string | null };
|
from: { provider: string | null; modelId: string | null };
|
||||||
@@ -15835,7 +15861,8 @@ ${stepsSection}`;
|
|||||||
taskIds: string[];
|
taskIds: string[];
|
||||||
}> {
|
}> {
|
||||||
const asStringOrNull = (v: unknown): string | null => (typeof v === "string" ? v : null);
|
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<{
|
const drift: Array<{
|
||||||
lane: string;
|
lane: string;
|
||||||
from: { provider: string | null; modelId: string | null };
|
from: { provider: string | null; modelId: string | null };
|
||||||
|
|||||||
@@ -481,8 +481,11 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
|
|||||||
await assertWorkflowExists(store, workflowId);
|
await assertWorkflowExists(store, workflowId);
|
||||||
const projectId = store.getWorkflowSettingsProjectId();
|
const projectId = store.getWorkflowSettingsProjectId();
|
||||||
try {
|
try {
|
||||||
const before = store.getWorkflowSettingValues(workflowId, projectId);
|
// FNXC:ModelLaneDrift 2026-07-08-07:24:
|
||||||
const stored = await store.updateWorkflowSettingValues(
|
// 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,
|
workflowId,
|
||||||
projectId,
|
projectId,
|
||||||
values as Record<string, unknown>,
|
values as Record<string, unknown>,
|
||||||
@@ -491,7 +494,17 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
|
|||||||
// Model-lane drift: tasks already pinned to the value being replaced
|
// Model-lane drift: tasks already pinned to the value being replaced
|
||||||
// are never auto-resynced (see getModelLaneDrift), so surface them
|
// are never auto-resynced (see getModelLaneDrift), so surface them
|
||||||
// here rather than let the change silently orphan those tasks.
|
// 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({
|
res.json({
|
||||||
stored,
|
stored,
|
||||||
effective: resolveEffectiveSettingValues(declarations, stored),
|
effective: resolveEffectiveSettingValues(declarations, stored),
|
||||||
|
|||||||
Reference in New Issue
Block a user