fix(ai-roles): validate chatModel + guard driver-enum drift (#52)

chatModel was a free string accepted with empty/garbage values, failing only at
runtime as a provider 503; tighten it (trim + non-empty + max 200). Driver was
already @IsIn(AI_DRIVERS). Collapse the client driver list to one AI_DRIVER_VALUES
source and add a contract test that reads the server AI_DRIVERS and fails on
client/server drift.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

apps/client/src/features/workspace/components/settings/components/ai-agent-role-form.drivers.test.ts

@@ -0,0 +1,53 @@
+import { readFileSync } from "node:fs";
+import { fileURLToPath } from "node:url";
+import path from "node:path";
+import { describe, expect, it } from "vitest";
+import {
+  AI_DRIVER_VALUES,
+  DRIVER_OPTIONS,
+} from "./ai-agent-role-form";
+
+/**
+ * Drift guard: the client's hardcoded driver list must stay in sync with the
+ * server `AI_DRIVERS`. Client and server are separate build targets and Vite
+ * refuses to import a module from outside the client root, so instead of an
+ * `import` we read the server `ai.types.ts` source and parse out the AI_DRIVERS
+ * literal. This contract test fails loudly if the two lists ever diverge
+ * (order-independent).
+ */
+function readServerAiDrivers(): string[] {
+  const here = path.dirname(fileURLToPath(import.meta.url));
+  // apps/client/src/.../components -> repo apps/server/src/integrations/ai
+  const serverTypesPath = path.resolve(
+    here,
+    "../../../../../../../server/src/integrations/ai/ai.types.ts",
+  );
+  const source = readFileSync(serverTypesPath, "utf8");
+  const match = source.match(/AI_DRIVERS\s*:\s*AiDriver\[\]\s*=\s*\[([^\]]*)\]/);
+  if (!match) {
+    throw new Error(
+      `Could not locate the AI_DRIVERS literal in ${serverTypesPath}`,
+    );
+  }
+  return match[1]
+    .split(",")
+    .map((s) => s.trim().replace(/^['"]|['"]$/g, ""))
+    .filter((s) => s.length > 0);
+}
+
+describe("ai-agent-role-form driver drift guard", () => {
+  it("mirrors the server AI_DRIVERS list exactly", () => {
+    const serverDrivers = readServerAiDrivers();
+    expect([...AI_DRIVER_VALUES].sort()).toEqual([...serverDrivers].sort());
+  });
+
+  it("exposes one Select option per server driver plus a workspace-default", () => {
+    const serverDrivers = readServerAiDrivers();
+    const driverOptionValues = DRIVER_OPTIONS.map((o) => o.value).filter(
+      (v) => v !== "",
+    );
+    expect(driverOptionValues.sort()).toEqual([...serverDrivers].sort());
+    // Exactly one empty-value option for the "Workspace default" choice.
+    expect(DRIVER_OPTIONS.filter((o) => o.value === "")).toHaveLength(1);
+  });
+});

apps/client/src/features/workspace/components/settings/components/ai-agent-role-form.tsx

@@ -23,13 +23,25 @@ import {
   IAiRoleUpdate,
 } from "@/features/ai-chat/types/ai-chat.types.ts";
 
-// Supported drivers for the optional model override (mirrors server AI_DRIVERS).
-// "" => use the workspace default driver/model.
-const DRIVER_OPTIONS = [
+// Source of truth: the server `AI_DRIVERS` list in
+// apps/server/src/integrations/ai/ai.types.ts. The client cannot import that
+// constant at build time (separate build target), so it is mirrored here and a
+// drift contract test (ai-agent-role-form.drivers.test.ts) fails if the two
+// lists diverge. Keep this in sync when adding/removing a server driver.
+export const AI_DRIVER_VALUES = ["openai", "gemini", "ollama"] as const;
+export type AiDriverValue = (typeof AI_DRIVER_VALUES)[number];
+
+const DRIVER_LABELS: Record<AiDriverValue, string> = {
+  openai: "OpenAI",
+  gemini: "Gemini",
+  ollama: "Ollama",
+};
+
+// Select options for the optional model override. "" => use the workspace
+// default driver/model.
+export const DRIVER_OPTIONS = [
   { value: "", label: "Workspace default" },
-  { value: "openai", label: "OpenAI" },
-  { value: "gemini", label: "Gemini" },
-  { value: "ollama", label: "Ollama" },
+  ...AI_DRIVER_VALUES.map((value) => ({ value, label: DRIVER_LABELS[value] })),
 ];
 
 const formSchema = z.object({
@@ -38,7 +50,7 @@ const formSchema = z.object({
   description: z.string(),
   instructions: z.string().min(1),
   // "" => no driver override (use the workspace driver).
-  driver: z.enum(["", "openai", "gemini", "ollama"]),
+  driver: z.enum(["", ...AI_DRIVER_VALUES]),
   chatModel: z.string(),
   enabled: z.boolean(),
 });

apps/server/src/core/ai-chat/roles/dto/agent-role.dto.spec.ts

@@ -0,0 +1,81 @@
+import 'reflect-metadata';
+import { plainToInstance } from 'class-transformer';
+import { validateSync } from 'class-validator';
+import { CreateAgentRoleDto, RoleModelConfigDto } from './agent-role.dto';
+
+/**
+ * API-boundary validation for the role model override. The key invariants:
+ *  - `driver`, when present, must be a supported server driver (AI_DRIVERS);
+ *  - `chatModel`, when present, must be a non-empty, trimmed, bounded string —
+ *    empty/whitespace-only garbage is rejected here, not at provider runtime.
+ */
+describe('RoleModelConfigDto validation', () => {
+  function validateConfig(config: unknown) {
+    const dto = plainToInstance(RoleModelConfigDto, config);
+    return validateSync(dto as object);
+  }
+
+  it('accepts a supported driver + non-empty chatModel', () => {
+    expect(validateConfig({ driver: 'openai', chatModel: 'gpt-4o' })).toHaveLength(
+      0,
+    );
+  });
+
+  it('accepts an empty object (omitted override => workspace default)', () => {
+    expect(validateConfig({})).toHaveLength(0);
+  });
+
+  it('rejects an unknown driver', () => {
+    const errors = validateConfig({ driver: 'anthropic', chatModel: 'x' });
+    expect(errors.some((e) => e.property === 'driver')).toBe(true);
+  });
+
+  it('rejects an empty chatModel string', () => {
+    const errors = validateConfig({ chatModel: '' });
+    expect(errors.some((e) => e.property === 'chatModel')).toBe(true);
+  });
+
+  it('rejects a whitespace-only chatModel (trimmed to empty)', () => {
+    const errors = validateConfig({ chatModel: '   ' });
+    expect(errors.some((e) => e.property === 'chatModel')).toBe(true);
+  });
+
+  it('trims surrounding whitespace from chatModel', () => {
+    const dto = plainToInstance(RoleModelConfigDto, {
+      chatModel: '  gpt-4o-mini  ',
+    });
+    expect(validateSync(dto as object)).toHaveLength(0);
+    expect(dto.chatModel).toBe('gpt-4o-mini');
+  });
+
+  it('rejects a chatModel longer than 200 chars', () => {
+    const errors = validateConfig({ chatModel: 'a'.repeat(201) });
+    expect(errors.some((e) => e.property === 'chatModel')).toBe(true);
+  });
+});
+
+describe('CreateAgentRoleDto with nested modelConfig', () => {
+  function validateCreate(payload: unknown) {
+    const dto = plainToInstance(CreateAgentRoleDto, payload);
+    return validateSync(dto as object);
+  }
+
+  const base = { name: 'Researcher', instructions: 'Do research.' };
+
+  it('accepts a valid create payload with a model override', () => {
+    expect(
+      validateCreate({
+        ...base,
+        modelConfig: { driver: 'gemini', chatModel: 'gemini-2.0-flash' },
+      }),
+    ).toHaveLength(0);
+  });
+
+  it('rejects a create payload whose nested chatModel is blank', () => {
+    const errors = validateCreate({
+      ...base,
+      modelConfig: { chatModel: '   ' },
+    });
+    expect(errors.length).toBeGreaterThan(0);
+  });
+});

apps/server/src/core/ai-chat/roles/dto/agent-role.dto.ts

@@ -5,9 +5,10 @@ import {
   IsOptional,
   IsString,
   MaxLength,
+  MinLength,
   ValidateNested,
 } from 'class-validator';
-import { Type } from 'class-transformer';
+import { Transform, TransformFnParams, Type } from 'class-transformer';
 import { AI_DRIVERS, AiDriver } from '../../../../integrations/ai/ai.types';
 
 /**
@@ -20,8 +21,16 @@ export class RoleModelConfigDto {
   @IsIn(AI_DRIVERS)
   driver?: AiDriver;
 
+  // Free-form provider model id (providers add models constantly, so we don't
+  // pin an allow-list). We still reject empty/whitespace-only garbage at the API
+  // boundary: trim first, then require a non-empty, bounded string. An invalid
+  // model still surfaces as a clear provider 503 at resolve time, not here.
   @IsOptional()
+  @Transform(({ value }: TransformFnParams) =>
+    typeof value === 'string' ? value.trim() : value,
+  )
   @IsString()
+  @MinLength(1)
   @MaxLength(200)
   chatModel?: string;
 }