fix(shared,web,terraform): address CodeRabbit feedback on #672

Must-fix:
- model-defaults.ts: per-field fallback chain (2.1) — defaultModel and
  defaultPlanModel now resolve independently as `DB > env > shared
  constant`. The previous all-or-nothing gate required BOTH fields in
  the CP response; when only one was present, both fields fell back
  together to env/constants, discarding the partial DB value.

Should-fix:
- models-settings.tsx: setDirty only fires on an actual state change
  (2.2). Previously toggling a model off would set dirty even when the
  toggle was blocked by the default-model guard, enabling Save on a
  no-op. The fix tracks a `changed` flag inside the updater and only
  calls setDirty when the underlying Set actually mutated.
- models-settings.tsx: SWR data sync moved from render phase into
  useEffect (2.3). The previous in-render setState would surface a
  React warning under StrictMode and risked an unnecessary re-render.
- terraform/environments/production/workers-{linear,slack}.tf:
  align DEFAULT_PLAN_MODEL to "anthropic/claude-opus-4-6" (2.4) to
  match the format used by workers-control-plane.tf and
  workers-github.tf. The bare "claude-opus-4-6" string was not a
  fully-qualified model ID and would be rejected by isValidModel.

Targeted regression test:
- model-defaults.test.ts: two new cases verifying that when the CP
  response contains only defaultModel (resp. defaultPlanModel), the
  other field independently falls back to env without the present
  field being discarded.

Verification: npm run build -w @open-inspect/shared && npm run lint &&
npm run typecheck && npm test (shared 16/16, control-plane 66/66,
web 32/32 — all green).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: bleleve

packages/shared/src/model-defaults.test.ts

@@ -88,4 +88,46 @@ describe("fetchModelDefaults", () => {
       defaultPlanModel: DEFAULT_PLAN_MODEL,
     });
   });
+
+  it("applies per-field fallback when only defaultModel is in the CP response", async () => {
+    // Regression test for CodeRabbit #672 item 2.1: previously, an
+    // all-or-nothing gate required BOTH fields in the CP response.
+    // Missing one would discard the other and force both fields to fall
+    // back together. Now they resolve independently — DB > env > constant.
+    const fetcher = vi.fn().mockResolvedValue(
+      jsonResponse({
+        enabledModels: ["anthropic/claude-haiku-4-5"],
+        defaultModel: "anthropic/claude-haiku-4-5",
+        // defaultPlanModel omitted
+      })
+    );
+
+    const result = await fetchModelDefaults({
+      CONTROL_PLANE: { fetch: fetcher } as never,
+      DEFAULT_MODEL: "env-should-not-win",
+      DEFAULT_PLAN_MODEL: "env-plan-wins-here",
+    });
+
+    expect(result.defaultModel).toBe("anthropic/claude-haiku-4-5"); // from DB
+    expect(result.defaultPlanModel).toBe("env-plan-wins-here"); // from env, since DB omitted it
+  });
+
+  it("applies per-field fallback when only defaultPlanModel is in the CP response", async () => {
+    const fetcher = vi.fn().mockResolvedValue(
+      jsonResponse({
+        enabledModels: ["anthropic/claude-opus-4-6"],
+        defaultPlanModel: "anthropic/claude-opus-4-6",
+        // defaultModel omitted
+      })
+    );
+
+    const result = await fetchModelDefaults({
+      CONTROL_PLANE: { fetch: fetcher } as never,
+      DEFAULT_MODEL: "env-model-wins-here",
+      DEFAULT_PLAN_MODEL: "env-should-not-win",
+    });
+
+    expect(result.defaultModel).toBe("env-model-wins-here"); // from env, since DB omitted it
+    expect(result.defaultPlanModel).toBe("anthropic/claude-opus-4-6"); // from DB
+  });
 });

packages/shared/src/model-defaults.ts

@@ -41,6 +41,13 @@ interface ModelPreferencesResponse {
  * to the env-var path, so bots stay functional during a CP outage.
  */
 export async function fetchModelDefaults(env: FetchModelDefaultsEnv): Promise<ModelDefaults> {
+  // Per-field fallback chain — each field independently resolves
+  // `DB row > env var > shared constant`. The previous all-or-nothing
+  // gate discarded a present `defaultModel` from the DB whenever
+  // `defaultPlanModel` was null (or vice versa), forcing both fields
+  // to fall back to env/constants together.
+  let dbDefaultModel: string | undefined;
+  let dbDefaultPlanModel: string | undefined;
   try {
     const headers = await buildInternalAuthHeaders(env.INTERNAL_CALLBACK_SECRET);
     const res = await env.CONTROL_PLANE.fetch("https://internal/model-preferences", {
@@ -49,18 +56,14 @@ export async function fetchModelDefaults(env: FetchModelDefaultsEnv): Promise<Mo
     });
     if (res.ok) {
       const data = (await res.json()) as ModelPreferencesResponse;
-      if (data.defaultModel && data.defaultPlanModel) {
-        return {
-          defaultModel: data.defaultModel,
-          defaultPlanModel: data.defaultPlanModel,
-        };
-      }
+      dbDefaultModel = data.defaultModel ?? undefined;
+      dbDefaultPlanModel = data.defaultPlanModel ?? undefined;
     }
   } catch {
     // Service-binding / network failure — fall through to env-or-shared
   }
   return {
-    defaultModel: env.DEFAULT_MODEL || DEFAULT_MODEL,
-    defaultPlanModel: env.DEFAULT_PLAN_MODEL || DEFAULT_PLAN_MODEL,
+    defaultModel: dbDefaultModel || env.DEFAULT_MODEL || DEFAULT_MODEL,
+    defaultPlanModel: dbDefaultPlanModel || env.DEFAULT_PLAN_MODEL || DEFAULT_PLAN_MODEL,
   };
 }

packages/web/src/components/settings/models-settings.tsx

@@ -1,6 +1,6 @@
 "use client";
 
-import { useMemo, useState } from "react";
+import { useEffect, useMemo, useState } from "react";
 import useSWR, { mutate } from "swr";
 import { toast } from "sonner";
 import {
@@ -32,16 +32,19 @@ export function ModelsSettings() {
   const [dirty, setDirty] = useState(false);
   const [toggleError, setToggleError] = useState<string | null>(null);
 
-  // Sync SWR data into local state once on initial load
-  if (data && !initialized) {
+  // Sync SWR data into local state once on initial load. Effect (not render-
+  // phase setState) so React doesn't warn about an in-render update.
+  useEffect(() => {
+    if (!data || initialized) return;
     setEnabledModels(new Set(data.enabledModels));
     if (data.defaultModel) setDefaultModel(data.defaultModel);
     if (data.defaultPlanModel) setDefaultPlanModel(data.defaultPlanModel);
     setInitialized(true);
-  }
+  }, [data, initialized]);
 
   const toggleModel = (modelId: string) => {
     setToggleError(null);
+    let changed = false;
     setEnabledModels((prev) => {
       const next = new Set(prev);
       if (next.has(modelId)) {
@@ -56,25 +59,33 @@ export function ModelsSettings() {
       } else {
         next.add(modelId);
       }
+      changed = true;
       return next;
     });
-    setDirty(true);
+    if (changed) setDirty(true);
   };
 
   const toggleCategory = (category: (typeof MODEL_OPTIONS)[number], enable: boolean) => {
     setToggleError(null);
+    let changed = false;
     setEnabledModels((prev) => {
       const next = new Set(prev);
       let blockedDefault: string | null = null;
       for (const model of category.models) {
         if (enable) {
-          next.add(model.id);
+          if (!next.has(model.id)) {
+            next.add(model.id);
+            changed = true;
+          }
         } else {
           if (model.id === defaultModel || model.id === defaultPlanModel) {
             blockedDefault = model.id;
             continue;
           }
-          next.delete(model.id);
+          if (next.has(model.id)) {
+            next.delete(model.id);
+            changed = true;
+          }
         }
       }
       if (next.size === 0) return prev;
@@ -85,7 +96,7 @@ export function ModelsSettings() {
       }
       return next;
     });
-    setDirty(true);
+    if (changed) setDirty(true);
   };
 
   const handleDefaultChange = (which: "model" | "plan", value: string) => {

terraform/environments/production/workers-linear.tf

@@ -46,7 +46,7 @@ module "linear_bot_worker" {
     { name = "DEPLOYMENT_NAME", value = var.deployment_name },
     { name = "APP_NAME", value = var.app_name },
     { name = "DEFAULT_MODEL", value = "claude-sonnet-4-6" },
-    { name = "DEFAULT_PLAN_MODEL", value = "claude-opus-4-6" },
+    { name = "DEFAULT_PLAN_MODEL", value = "anthropic/claude-opus-4-6" },
     { name = "LINEAR_CLIENT_ID", value = var.linear_client_id },
     { name = "WORKER_URL", value = "https://open-inspect-linear-bot-${local.name_suffix}.${var.cloudflare_worker_subdomain}.workers.dev" },
   ]

terraform/environments/production/workers-slack.tf

@@ -48,7 +48,7 @@ module "slack_bot_worker" {
     { name = "DEPLOYMENT_NAME", value = var.deployment_name },
     { name = "APP_NAME", value = var.app_name },
     { name = "DEFAULT_MODEL", value = "claude-haiku-4-5" },
-    { name = "DEFAULT_PLAN_MODEL", value = "claude-opus-4-6" },
+    { name = "DEFAULT_PLAN_MODEL", value = "anthropic/claude-opus-4-6" },
     { name = "CLASSIFICATION_MODEL", value = "claude-haiku-4-5" },
   ]