fix(custom-model): serialize every write of the endpoint store

The four endpoint routes and the 5-minute discovery sweep each read
custom-model-hosts.json, changed their copy and wrote it back through one
shared temp file name. Two of them at once could tear the file or drop each
other's record: ten creates sent together did not all land.

Every write now goes through mutateCustomModelHosts() (one promise chain, a
random temp name, mode 0600), with each check inside the same step as its
write. A file that does not parse is refused with 409 and never replaced.
A delete of an unknown id no longer rewrites the file. A guard test fails
any other writer in src/.

The id `local-ollama` is reserved for the model Codeman runs on this Mac: a
new endpoint may not take it, and while the record is the managed one it is
not edited, deleted or re-discovered from these routes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

src/custom-model-hosts-writer.ts

@@ -2,9 +2,6 @@
  * @fileoverview The serialized writer of `custom-model-hosts.json`, the store of the saved
  * custom model endpoints (src/custom-model-hosts.ts).
  *
- * CONTRACT STAGE: the function below has its final signature and a body that throws; the body
- * is written next, and every writer of the file is moved onto it then.
- *
  * Why it exists: `writeCustomModelHosts()` writes through ONE temp file name per process and
  * has no queue, and `readCustomModelHosts()` reads a file that does not parse as an empty
  * list. Two overlapping writers (an endpoint route and the 5-minute discovery sweep) can
@@ -28,13 +25,12 @@
  * direct writer left is config-backup restore, which must work on a file that does not parse.
  */
 
-import type { CustomModelHost } from './custom-model-hosts.js';
-import type { ApiResponse } from './types.js';
-
-/** Marks a body the contract stage left unwritten. */
-function notImplemented(what: string, ..._args: unknown[]): never {
-  throw new Error(`${what} is not implemented yet`);
-}
+import { randomUUID } from 'node:crypto';
+import fs from 'node:fs/promises';
+import { dirname } from 'node:path';
+import { getDataDir } from './config/instance.js';
+import { customModelHostsPath, type CustomModelHost } from './custom-model-hosts.js';
+import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from './types.js';
 
 /** A write refused because the existing `custom-model-hosts.json` must not be overwritten. The message is user-facing. */
 export class CustomModelHostsWriteRefusedError extends Error {
@@ -44,6 +40,49 @@ export class CustomModelHostsWriteRefusedError extends Error {
   }
 }
 
+/** The list a write starts from. Only a missing or empty file is an empty list; anything else that is not an array is refused. */
+async function readHostsForWrite(path: string): Promise<CustomModelHost[]> {
+  let raw: string;
+  try {
+    raw = await fs.readFile(path, 'utf-8');
+  } catch (err) {
+    if ((err as NodeJS.ErrnoException).code === 'ENOENT') return [];
+    throw new CustomModelHostsWriteRefusedError(
+      `Cannot read ${path} (${(err as Error).message}), so this change was not saved.`
+    );
+  }
+  // An empty file holds no endpoint to lose.
+  if (raw.trim() === '') return [];
+  let parsed: unknown;
+  try {
+    parsed = JSON.parse(raw);
+  } catch {
+    parsed = undefined;
+  }
+  if (!Array.isArray(parsed)) {
+    throw new CustomModelHostsWriteRefusedError(
+      `${path} is not a valid JSON list, so this change was not saved (saving would have replaced every other model endpoint in it, with its API key). Fix or delete the file, or restore it from a config backup, then try again.`
+    );
+  }
+  return parsed as CustomModelHost[];
+}
+
+async function writeHostsFile(path: string, hosts: CustomModelHost[]): Promise<void> {
+  // Through a symlink: the rename lands on the real file.
+  const target = await fs.realpath(path).catch(() => path);
+  await fs.mkdir(dirname(target), { recursive: true });
+  const tmp = `${target}.${process.pid}.${randomUUID()}.tmp`;
+  try {
+    await fs.writeFile(tmp, JSON.stringify(hosts, null, 2), { mode: 0o600 });
+    await fs.rename(tmp, target);
+  } catch (err) {
+    await fs.rm(tmp, { force: true }).catch(() => {});
+    throw err;
+  }
+}
+
+let mutationChain: Promise<unknown> = Promise.resolve();
+
 /** What a mutation returns: the list to write (absent: write nothing) and what the caller gets back. */
 export interface CustomModelHostsMutation<T> {
   /** The whole list to store. Absent: the file is left as it is. */
@@ -63,10 +102,23 @@ export function mutateCustomModelHosts<T>(
   fn: (hosts: CustomModelHost[]) => CustomModelHostsMutation<T>,
   configDir?: string
 ): Promise<T> {
-  return notImplemented('mutateCustomModelHosts', fn, configDir);
+  const run = mutationChain.then(async () => {
+    // Resolved inside the chain: the data dir is read when the mutation runs, like the file.
+    const path = customModelHostsPath(configDir ?? getDataDir());
+    const current = await readHostsForWrite(path).catch((err: unknown) => {
+      console.warn(`[custom-model] write refused: ${getErrorMessage(err)}`);
+      throw err;
+    });
+    const mutation = fn(current);
+    if (mutation.hosts) await writeHostsFile(path, mutation.hosts);
+    return mutation.result;
+  });
+  mutationChain = run.catch(() => {});
+  return run;
 }
 
 /** A refused write answers 409 CONFLICT with the fix; any other failure stays OPERATION_FAILED. Like `settingsWriteErrorResponse()`. */
 export function customModelHostsWriteErrorResponse(err: unknown): ApiResponse<never> {
-  return notImplemented('customModelHostsWriteErrorResponse', err);
+  if (err instanceof CustomModelHostsWriteRefusedError) return createErrorResponse(ApiErrorCode.CONFLICT, err.message);
+  return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getErrorMessage(err));
 }

src/web/routes/custom-model-routes.ts

@@ -12,6 +12,21 @@
  * connect time, redirects included — a save-time hostname check alone would
  * let `models.example` resolve to 169.254.169.254 later. The endpoint is
  * admin-configured, so this is defence in depth rather than the only gate.
+ *
+ * Every write of `custom-model-hosts.json` goes through `mutateCustomModelHosts()`
+ * (src/custom-model-hosts-writer.ts): the four writing routes and the re-discovery sweep run
+ * one at a time, each check ("the id is free", "the record still exists") inside the same
+ * serialized step as its write, and a file that does not parse is refused with 409 rather than
+ * overwritten. They used to read, change and write on their own, through one shared temp file
+ * name: a route and the sweep could tear the file, and the next writer then saved a list that
+ * held only its own record.
+ *
+ * The local model install (src/local-model/) keeps ONE record here under a reserved id
+ * (`LOCAL_MODEL_ENDPOINT_ID`). A new endpoint may not take that id. While the record is the
+ * managed one (`isManagedLocalEndpoint()`: the local model state file records it as ours) it
+ * is not edited, deleted or re-discovered from here: its `models` must stay the one model the
+ * install checked, and its `baseUrl` the port of the runtime Codeman supervises. It is changed
+ * and removed through the local model routes.
  */
 
 import type { FastifyInstance, FastifyRequest } from 'fastify';
@@ -22,7 +37,9 @@ import { getDataDir } from '../../config/instance.js';
 import { isBlockedWebviewUrl } from '../webview-egress-policy.js';
 import { egressBlockedReason, webviewFetch } from '../webview-egress.js';
 import { CustomModelHostSchema, MAX_DISCOVERED_CONTEXT_TOKENS } from '../schemas.js';
-import { readCustomModelHosts, writeCustomModelHosts, type CustomModelHost } from '../../custom-model-hosts.js';
+import { readCustomModelHosts, type CustomModelHost } from '../../custom-model-hosts.js';
+import { customModelHostsWriteErrorResponse, mutateCustomModelHosts } from '../../custom-model-hosts-writer.js';
+import { isLocalModelEndpointId, isManagedLocalEndpoint } from '../../local-model/connect.js';
 import type { CliEntry } from '../../config/cli-registry/types.js';
 
 const CODEMAN_CONFIG_DIR = getDataDir();
@@ -795,20 +812,30 @@ export async function refreshAllCustomModelHosts(): Promise<void> {
   const hosts = await readCustomModelHosts(dataDir);
   for (const host of hosts) {
     if (isBlockedWebviewUrl(host.baseUrl)) continue;
+    // The local model's own record keeps the one model its install checked: whatever else the
+    // runtime lists (a model somebody pulled through its port) must not become launchable.
+    if (await isManagedLocalEndpoint(host.id)) continue;
     let result: DiscoveryResult;
     try {
       result = await discoverModels(host);
     } catch {
       continue; // unreachable this cycle — try again next tick, not fatal to the sweep
     }
-    // Re-read + splice by id rather than reusing the array captured above: an
-    // admin editing or deleting an endpoint via the API mid-sweep must win,
-    // not be silently overwritten by a refresh that started before their change.
-    const current = await readCustomModelHosts(dataDir);
-    const index = current.findIndex((item) => item.id === host.id);
-    if (index === -1) continue; // deleted mid-sweep
-    current[index] = applyDiscoveredModels(current[index], result);
-    await writeCustomModelHosts(dataDir, current);
+    // Spliced by id inside the serialized writer, onto the list as it is THEN, rather than
+    // the array captured above: an admin editing or deleting an endpoint via the API mid-sweep
+    // must win, not be silently overwritten by a refresh that started before their change.
+    try {
+      await mutateCustomModelHosts<void>((current) => {
+        const index = current.findIndex((item) => item.id === host.id);
+        if (index === -1) return { result: undefined }; // deleted mid-sweep
+        current[index] = applyDiscoveredModels(current[index], result);
+        return { hosts: current, result: undefined };
+      }, dataDir);
+    } catch {
+      // A file that must not be overwritten (it does not parse) refuses every write of this
+      // sweep the same way. The writer logged why; nothing more can be saved this cycle.
+      return;
+    }
   }
 }
 
@@ -897,6 +924,21 @@ export async function detectCustomModelSwapDisplacements(
   return displacements;
 }
 
+/** A write the serialized writer refused (the file does not parse) is 409 with the fix; any other failure stays in the envelope. */
+function refusedHostsWrite(err: unknown, reply: { code: (n: number) => unknown }): ApiResponse<never> {
+  const response = customModelHostsWriteErrorResponse(err);
+  if (!response.success && response.errorCode === ApiErrorCode.CONFLICT) reply.code(409);
+  return response;
+}
+
+/** The answer to an edit or a delete of the record the local model install manages. */
+function managedEndpointRefusal(): ApiResponse<never> {
+  return createErrorResponse(
+    ApiErrorCode.INVALID_INPUT,
+    'This endpoint belongs to the model Codeman runs on this Mac. Change or remove it in the local model window.'
+  );
+}
+
 export function registerCustomModelRoutes(app: FastifyInstance): void {
   app.get('/api/model-endpoints', async (req): Promise<RedactedHost[]> => {
     if (isMultiUserMode() && !isAdmin(req)) return [];
@@ -913,12 +955,22 @@ export function registerCustomModelRoutes(app: FastifyInstance): void {
     }
     const badDefault = invalidDefaultModel(host);
     if (badDefault) return badDefault;
-    const hosts = await readCustomModelHosts(CODEMAN_CONFIG_DIR);
-    if (hosts.some((item) => item.id === host.id)) {
-      return createErrorResponse(ApiErrorCode.ALREADY_EXISTS, 'Model endpoint already exists');
+    if (isLocalModelEndpointId(host.id)) {
+      return createErrorResponse(
+        ApiErrorCode.INVALID_INPUT,
+        `The endpoint id "${host.id}" is reserved for the model Codeman runs on this Mac. Pick another id.`
+      );
+    }
+    try {
+      return await mutateCustomModelHosts<ApiResponse<{ host: RedactedHost }>>((hosts) => {
+        if (hosts.some((item) => item.id === host.id)) {
+          return { result: createErrorResponse(ApiErrorCode.ALREADY_EXISTS, 'Model endpoint already exists') };
+        }
+        return { hosts: [...hosts, host], result: { success: true, data: { host: redactApiKey(host) } } };
+      }, CODEMAN_CONFIG_DIR);
+    } catch (err) {
+      return refusedHostsWrite(err, reply);
     }
-    await writeCustomModelHosts(CODEMAN_CONFIG_DIR, [...hosts, host]);
-    return { success: true, data: { host: redactApiKey(host) } };
   });
 
   app.put('/api/model-endpoints/:id', async (req, reply): Promise<ApiResponse<{ host: RedactedHost }>> => {
@@ -931,26 +983,37 @@ export function registerCustomModelRoutes(app: FastifyInstance): void {
     }
     const badDefault = invalidDefaultModel(incoming);
     if (badDefault) return badDefault;
-    const hosts = await readCustomModelHosts(CODEMAN_CONFIG_DIR);
-    const index = hosts.findIndex((item) => item.id === id);
-    if (index === -1) return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Model endpoint not found');
-    const host = applyDiscoveredFields(applyStoredApiKey(incoming, hosts[index]), hosts[index]);
-    const next = [...hosts];
-    next[index] = host;
-    await writeCustomModelHosts(CODEMAN_CONFIG_DIR, next);
-    return { success: true, data: { host: redactApiKey(host) } };
+    if (await isManagedLocalEndpoint(id)) return managedEndpointRefusal();
+    try {
+      return await mutateCustomModelHosts<ApiResponse<{ host: RedactedHost }>>((hosts) => {
+        const index = hosts.findIndex((item) => item.id === id);
+        if (index === -1) return { result: createErrorResponse(ApiErrorCode.NOT_FOUND, 'Model endpoint not found') };
+        const host = applyDiscoveredFields(applyStoredApiKey(incoming, hosts[index]), hosts[index]);
+        const next = [...hosts];
+        next[index] = host;
+        return { hosts: next, result: { success: true, data: { host: redactApiKey(host) } } };
+      }, CODEMAN_CONFIG_DIR);
+    } catch (err) {
+      return refusedHostsWrite(err, reply);
+    }
   });
 
   app.delete('/api/model-endpoints/:id', async (req, reply): Promise<ApiResponse<{ id: string }>> => {
     const denied = adminOnly(req, reply);
     if (denied) return denied;
     const { id } = req.params as { id: string };
-    const hosts = await readCustomModelHosts(CODEMAN_CONFIG_DIR);
-    await writeCustomModelHosts(
-      CODEMAN_CONFIG_DIR,
-      hosts.filter((item) => item.id !== id)
-    );
-    return { success: true, data: { id } };
+    if (await isManagedLocalEndpoint(id)) return managedEndpointRefusal();
+    try {
+      return await mutateCustomModelHosts<ApiResponse<{ id: string }>>((hosts) => {
+        const next = hosts.filter((item) => item.id !== id);
+        // Nothing to take out: the file is left as it is, and the answer is the same.
+        return next.length === hosts.length
+          ? { result: { success: true, data: { id } } }
+          : { hosts: next, result: { success: true, data: { id } } };
+      }, CODEMAN_CONFIG_DIR);
+    } catch (err) {
+      return refusedHostsWrite(err, reply);
+    }
   });
 
   app.post(
@@ -966,12 +1029,12 @@ export function registerCustomModelRoutes(app: FastifyInstance): void {
       if (isBlockedWebviewUrl(host.baseUrl)) {
         return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Endpoint base URL is not allowed');
       }
+      // The local model's own record is not probed: it lists the one model its install checked,
+      // and that list is the answer.
+      if (await isManagedLocalEndpoint(id)) return { success: true, data: { models: host.models ?? [] } };
+      let result: DiscoveryResult;
       try {
-        const result = await discoverModels(host);
-        const next = [...hosts];
-        next[index] = applyDiscoveredModels(host, result);
-        await writeCustomModelHosts(CODEMAN_CONFIG_DIR, next);
-        return { success: true, data: { models: result.models } };
+        result = await discoverModels(host);
       } catch (err) {
         const blocked = egressBlockedReason(err);
         return createErrorResponse(
@@ -979,6 +1042,18 @@ export function registerCustomModelRoutes(app: FastifyInstance): void {
           blocked ? `Endpoint refused: ${blocked}` : `Could not reach endpoint: ${describeFetchError(err)}`
         );
       }
+      try {
+        // Onto the record as it is NOW: the probe above took a moment, and an edit made
+        // meanwhile must not be replaced by the copy read before it.
+        return await mutateCustomModelHosts<ApiResponse<{ models: string[] }>>((current) => {
+          const at = current.findIndex((item) => item.id === id);
+          if (at === -1) return { result: createErrorResponse(ApiErrorCode.NOT_FOUND, 'Model endpoint not found') };
+          current[at] = applyDiscoveredModels(current[at], result);
+          return { hosts: current, result: { success: true, data: { models: result.models } } };
+        }, CODEMAN_CONFIG_DIR);
+      } catch (err) {
+        return refusedHostsWrite(err, reply);
+      }
     }
   );