Address PR review feedback (#1445)

- applyRemoteSettings now applies the stripped global payload (moved keys can't resurrect)
- resolver error path keeps effective workflowId so builtin fallback survives identity failure
- updateWorkflowSettingValues read-merge-upsert wrapped in transactionImmediate (lost-update race)
- MergeSection directMergeCommitStrategy UI fallback aligned to schema default (always-squash)
- save-split section-routing test asserts the positive case
- setting-values routes 404 on unknown workflow ids (+ tests)

packages/core/src/central-core.ts

@@ -3666,9 +3666,11 @@ export class CentralCore extends EventEmitter<CentralCoreEvents> {
     // count reflects only the keys that survive the strip.
     if (payload.global) {
       // The actual application of global settings is handled by the caller (dashboard route)
-      // since CentralCore doesn't have access to GlobalSettingsStore.
-      // We simply count the number of global settings entries for reporting.
+      // since CentralCore doesn't have access to GlobalSettingsStore. Mutate the payload
+      // in place so the caller applies the stripped version — otherwise moved keys survive
+      // in payload.global and get resurrected cross-node (KTD-8).
       const cleanGlobal = stripMovedSettingsKeys(payload.global as Record<string, unknown>);
+      payload.global = cleanGlobal as typeof payload.global;
       globalCount = Object.keys(cleanGlobal).length;
     }
 

packages/core/src/store.ts

@@ -7204,27 +7204,35 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
       throw new WorkflowSettingRejectionError(result.rejections);
     }
 
-    const current = this.getWorkflowSettingValues(workflowId, projectId);
-    const next: Record<string, unknown> = { ...current };
-    for (const [key, value] of Object.entries(result.accepted)) {
-      if (value === null) {
-        delete next[key];
-      } else {
-        next[key] = value;
+    // Read-merge-upsert must be atomic: two concurrent calls for the same
+    // (workflowId, projectId) could otherwise both merge from the same
+    // pre-update snapshot, and the later upsert would erase the earlier
+    // call's keys (lost update). Serialize the whole cycle under an immediate
+    // 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 };
+      for (const [key, value] of Object.entries(result.accepted)) {
+        if (value === null) {
+          delete next[key];
+        } else {
+          next[key] = value;
+        }
       }
-    }
 
-    const now = new Date().toISOString();
-    this.db
-      .prepare(
-        `INSERT INTO workflow_settings (workflowId, projectId, "values", updatedAt)
-         VALUES (?, ?, ?, ?)
-         ON CONFLICT(workflowId, projectId)
-         DO UPDATE SET "values" = excluded."values", updatedAt = excluded.updatedAt`,
-      )
-      .run(workflowId, projectId, JSON.stringify(next), now);
-    this.db.bumpLastModified();
-    return next;
+      const now = new Date().toISOString();
+      this.db
+        .prepare(
+          `INSERT INTO workflow_settings (workflowId, projectId, "values", updatedAt)
+           VALUES (?, ?, ?, ?)
+           ON CONFLICT(workflowId, projectId)
+           DO UPDATE SET "values" = excluded."values", updatedAt = excluded.updatedAt`,
+        )
+        .run(workflowId, projectId, JSON.stringify(next), now);
+      this.db.bumpLastModified();
+      return next;
+    });
   }
 
   /**

packages/core/src/workflow-settings-resolver.ts

@@ -174,7 +174,8 @@ export async function resolveEffectiveSettingsDetailed(
     projectId = store.getWorkflowSettingsProjectId();
   } catch {
     // Degrade to declaration defaults (empty stored map) on identity failure.
-    return effectiveFrom(store, ir, undefined, "");
+    // Keep the resolved workflowId so builtin graphs still pick up the catalog fallback.
+    return effectiveFrom(store, ir, effectiveWorkflowId, "");
   }
   return effectiveFrom(store, ir, effectiveWorkflowId, projectId);
 }

packages/dashboard/app/__tests__/settings-save-split.test.ts

@@ -177,5 +177,8 @@ describe("splitSettingsSave", () => {
       activeSection: "general",
     });
     expect("githubTrackingDefaultRepo" in onProject.globalPatch).toBe(false);
+    // ...and is instead routed to the project patch on the project-scoped
+    // "general" section, rather than being dropped or erroring.
+    expect(onProject.projectPatch).toMatchObject({ githubTrackingDefaultRepo: "org/repo" });
   });
 });

packages/dashboard/app/components/settings/sections/MergeSection.tsx

@@ -256,7 +256,7 @@ export function MergeSection({
             <select
               id="directMergeCommitStrategy"
               className="select"
-              value={form.directMergeCommitStrategy ?? "auto"}
+              value={form.directMergeCommitStrategy ?? "always-squash"}
               onChange={(e) =>
                 setForm((f) => ({
                   ...f,

packages/dashboard/src/__tests__/workflow-routes.test.ts

@@ -578,6 +578,18 @@ describe("workflow routes (U4)", () => {
       const res = await patch(`/api/workflows/${encodeURIComponent(id)}/setting-values`, { values: [1, 2, 3] });
       expect(res.status).toBe(400);
     });
+
+    it("GET returns 404 for an unknown workflow id (neither built-in nor custom)", async () => {
+      const res = await get("/api/workflows/WF-404/setting-values");
+      expect(res.status).toBe(404);
+    });
+
+    it("PATCH returns 404 for an unknown workflow id (neither built-in nor custom)", async () => {
+      const res = await patch("/api/workflows/WF-404/setting-values", {
+        values: { "timeout-ms": 5000 },
+      });
+      expect(res.status).toBe(404);
+    });
   });
 });
 

packages/dashboard/src/routes/register-workflow-routes.ts

@@ -176,6 +176,21 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
     return declared;
   }
 
+  /**
+   * 404 guard for the setting-values routes (consistent with GET /workflows/:id).
+   * `resolveWorkflowIrById` / the store value methods silently degrade to the
+   * built-in default for an unknown id (so the route would otherwise 200 with
+   * empty/built-in data), and `updateWorkflowSettingValues` likewise resolves
+   * declarations gracefully — neither throws "not found". Mirror the sibling
+   * handlers: an id that is neither a built-in nor an existing custom workflow
+   * must surface as `notFound` rather than a silent success.
+   */
+  async function assertWorkflowExists(store: TaskStore, workflowId: string): Promise<void> {
+    if (isBuiltinWorkflowId(workflowId)) return;
+    const def = await store.getWorkflowDefinition(workflowId);
+    if (!def) throw notFound(`Workflow '${workflowId}' not found`);
+  }
+
   /**
    * Write-time column-agent validation (U6, R11/R13). Delegates to the shared
    * `validateColumnAgentBindings` helper in @fusion/core (the SAME gate the
@@ -397,6 +412,7 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
     try {
       const { store } = await getProjectContext(req);
       const workflowId = req.params.id;
+      await assertWorkflowExists(store, workflowId);
       const projectId = store.getWorkflowSettingsProjectId();
       const declarations = await resolveSettingDeclarations(store, workflowId);
       const stored = store.getWorkflowSettingValues(workflowId, projectId);
@@ -427,6 +443,7 @@ export function registerWorkflowRoutes(ctx: ApiRoutesContext): void {
       if (!values || typeof values !== "object" || Array.isArray(values)) {
         throw badRequest("values is required and must be an object map of setting id → value (null to delete)");
       }
+      await assertWorkflowExists(store, workflowId);
       const projectId = store.getWorkflowSettingsProjectId();
       try {
         const stored = await store.updateWorkflowSettingValues(