fix(custom-model): unset injected env on clear, resume on restart, select the model for pi/omp/grok

Custom Model Endpoint Profiles (#393) let a session point its CLI at a custom OpenAI-compatible endpoint by injecting env vars or a config file and restarting the CLI in place. Review of the apply path found four things, two of them destructive. This lands all four plus the smaller items from the same review. 1. Clearing a selection did not clear it. The injected vars reach the CLI via `tmux setenv`, which persists at the tmux-session level and is inherited by `respawn-pane` (measured: `setenv FOO bar` survived two successive `respawn-pane -k`), so deleting the keys from the session's envOverrides relaunched the CLI still pointed at the old endpoint, and for the configDir kinds at a HOME/CODEX_HOME/GROK_HOME that had just been deleted. `Session.setCustomModel()` now reports the removed keys, queues them (`_pendingEnvUnsets`), and `RespawnPaneOptions.unsetEnvKeys` carries them into `applyEnvOverrides()`, which `setenv -u`s them before re-applying the live overrides, on the same path that already unsets the legacy CLAUDE_CODE_EFFORT_LEVEL. Verified on a private tmux socket that `setenv -u HOME` hands the next respawn the global HOME back. 2. Applying a model to a local claude session killed the pane. The relaunch was `claude --session-id <id>` and Claude refuses an id that already has a transcript, and unlike the dead-pane respawn this one kills a working pane first. `restartCli()` now pins the live conversation id as the resume id for that respawn when the CLI's launch declares a `fallback` chain, which renders the same `--resume <id> || --session-id <id>` shape the docker and remote pane commands use. Gated on the registry shape, not the CLI id: an entry whose resume id is minted by the CLI itself never declares that chain. 3. pi, omp and grok wrote their config file and then launched without the `--model` that selects it, so the file was ignored. The registry entry now declares `customModelInjection.launchModel` (`custom/{modelId}` for pi and omp, grok's `[model.codeman-custom]` block name), the builder renders it, and `_withCustomModelLaunchModel()` applies it onto the respawn options through `legacyConfigField`, leaving the stored <Mode>Config untouched so a clear falls back to the user's own model. A model id the CLI's `model` token pattern cannot carry is refused with a 400 rather than silently dropped by the argv engine. 4. Remote (SSH) and Docker sessions reported `restarted: true` and changed nothing: their `restartCli()` reattaches the durable tmux rather than relaunching the agent, and the env lands on the local pane. Both are refused with a 400 until those paths are plumbed. Smaller items from the same review: - The selection survives a Codeman restart as the disk-only `__customModel` bookkeeping (endpoint, model, injected key NAMES, config dir, launch model; never the values, which carry the API key). Recovery re-derives the values from the endpoint store through the same apply path the route uses and keeps the bookkeeping even when the endpoint is gone, so a later clear still has keys to unset. - Discovery goes through `webviewFetch()`, so the RESOLVED address is judged by the same egress guard the web-tab proxy uses, and `baseUrl` reuses `webviewUrlSchema` (http(s) only, no embedded credentials, link-local and cloud-metadata addresses refused). undici's `fetch failed` wrapper is unwrapped so the user sees the ECONNREFUSED underneath. - `custom-model-hosts.json` is written 0600 via tmp+rename, the per-session config dir 0700/0600 (pi and omp embed the key literally), and that dir is removed with the session. - `PR.md` is gone from the repo root and the design doc moved to `docs/custom-model-endpoints-plan.md` with the LAN address and the personal name scrubbed; every reference follows. The guide's `authStyle` text matches the shipped schema (`bearer | api-key`, default `bearer`) and says that `customModelEndpointsEnabled` is read by nothing until the picker lands. - `config/tsconfig.scripts.json` typechecks `scripts/test-local-llm-harnesses.ts` (four real type errors fixed). It is not yet wired into `npm run typecheck` because that line differs on master; adding `&& tsc -p config/tsconfig.scripts.json` there is the one-line follow-up. Tests: `test/session-custom-model-restart.test.ts` drives a real Session and fails on the unfixed code for items 1 to 3; the route suite covers item 4 and the pattern refusal; `test/tmux-manager.test.ts` pins that the unsets run before the overrides and that a shell-metachar key never reaches tmux. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-10-05 15:09:42 +02:00 · 2026-09-14 23:46:28 +02:00
parent 1e42cb4e2d
commit 942bf37e48
31 changed files with 988 additions and 457 deletions
@@ -1,16 +1,17 @@
 /**
 * @fileoverview Custom Model Endpoint Profiles CRUD + discovery
- * (deployment_plan.md). Endpoints are machine-level infra, like remote/docker
- * hosts, so writes are admin-only in multi-user mode
+ * (docs/custom-model-endpoints-plan.md). Endpoints are machine-level infra,
+ * like remote/docker hosts, so writes are admin-only in multi-user mode
 * (`case-routes.ts`'s `/api/remote-hosts` is the pattern this mirrors).
 *
- * Discovery (`POST /:id/discover-models`) fetches `${baseUrl}/v1/models`.
- * `isBlockedWebviewUrl()` is the same synchronous hostname/link-local/cloud-
- * metadata check `webview-egress-policy.ts` uses for saved dashboard URLs —
- * reused here as a save-time and discover-time guard. It does NOT re-check
- * the DNS-RESOLVED address the way `webviewFetch()`'s undici lookup hook
- * does; wiring that dispatcher-level guard here is a followup, not done in
- * this pass, since this route is already admin-only in multi-user mode.
+ * Discovery (`POST /:id/discover-models`) fetches `${baseUrl}/v1/models`
+ * through `webviewFetch()` (`webview-egress.ts`), the same guarded dispatcher
+ * the web-tab proxy uses: `baseUrl` is refused at save time by the schema's
+ * hostname check (link-local / cloud-metadata literals and names), and the
+ * undici lookup hook refuses a name that RESOLVES into one of those ranges at
+ * 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.
 */

 import type { FastifyInstance, FastifyRequest } from 'fastify';
@@ -19,6 +20,7 @@ import { isAdmin, parseBody } from '../route-helpers.js';
 import { isMultiUserMode } from '../../config/multiuser.js';
 import { getDataDir } from '../../config/instance.js';
 import { isBlockedWebviewUrl } from '../webview-egress-policy.js';
+import { egressBlockedReason, webviewFetch } from '../webview-egress.js';
 import { CustomModelHostSchema } from '../schemas.js';
 import { readCustomModelHosts, writeCustomModelHosts, type CustomModelHost } from '../../custom-model-hosts.js';

@@ -40,7 +42,7 @@ async function discoverModels(host: Pick<CustomModelHost, 'baseUrl' | 'apiKey' |
  if (apiKey && style === 'bearer') headers.Authorization = `Bearer ${apiKey}`;
  if (apiKey && style === 'api-key') headers['api-key'] = apiKey;

-  const res = await fetch(`${host.baseUrl.replace(/\/+$/, '')}/v1/models`, {
+  const res = await webviewFetch(new URL(`${host.baseUrl.replace(/\/+$/, '')}/v1/models`), {
    headers,
    signal: AbortSignal.timeout(DISCOVER_TIMEOUT_MS),
  });
@@ -49,6 +51,21 @@ async function discoverModels(host: Pick<CustomModelHost, 'baseUrl' | 'apiKey' |
  return (body.data ?? []).map((m) => m.id).filter((id): id is string => typeof id === 'string' && id.length > 0);
 }

+/**
+ * undici reports every network failure as `TypeError('fetch failed', { cause })`, with the
+ * useful part (`connect ECONNREFUSED 127.0.0.1:8080`) one level down; surface the deepest
+ * message so the user sees the refused connection, not the wrapper.
+ */
+function describeFetchError(err: unknown): string {
+  let message = err instanceof Error ? err.message : String(err);
+  let current: unknown = err;
+  for (let depth = 0; depth < 5 && current instanceof Error && current.cause !== undefined; depth++) {
+    current = current.cause;
+    if (current instanceof Error && current.message) message = current.message;
+  }
+  return message;
+}
+
 export function registerCustomModelRoutes(app: FastifyInstance): void {
  app.get('/api/model-endpoints', async (req) =>
    isMultiUserMode() && !isAdmin(req) ? [] : readCustomModelHosts(CODEMAN_CONFIG_DIR)
@@ -118,9 +135,10 @@ export function registerCustomModelRoutes(app: FastifyInstance): void {
        await writeCustomModelHosts(CODEMAN_CONFIG_DIR, next);
        return { success: true, data: { models } };
      } catch (err) {
+        const blocked = egressBlockedReason(err);
        return createErrorResponse(
          ApiErrorCode.OPERATION_FAILED,
-          `Could not reach endpoint: ${err instanceof Error ? err.message : String(err)}`
+          blocked ? `Endpoint refused: ${blocked}` : `Could not reach endpoint: ${describeFetchError(err)}`
        );
      }
    }
@@ -54,8 +54,8 @@ import {
  CustomModelSelectionSchema,
 } from '../schemas.js';
 import { readCustomModelHosts } from '../../custom-model-hosts.js';
-import { buildCustomModelInjection } from '../../custom-model-injection.js';
-import { applyConfigDirInjection, removeConfigDir } from '../../custom-model-injection-apply.js';
+import { applyCustomModelInjection, removeConfigDir } from '../../custom-model-injection-apply.js';
+import { matchesPattern } from '../../config/cli-registry/patterns.js';
 import { ownerLayoutKey } from '../../tab-layout-persistence.js';
 import { TabLayoutValidationError } from '../../tab-layout.js';
 import {
@@ -1156,7 +1156,7 @@ export function registerSessionRoutes(
    return { color: session.color };
  });

-  // ========== Custom Model Endpoint Profiles (deployment_plan.md) ==========
+  // ========== Custom Model Endpoint Profiles (docs/custom-model-endpoints-plan.md) ==========
  //
  // Applies (or clears) a session's custom OpenAI-compatible endpoint selection and
  // RESTARTS the pane's CLI process — these harnesses read endpoint config at process
@@ -1165,17 +1165,30 @@ export function registerSessionRoutes(
  // (chunk 3's CRUD routes), never raw client-supplied env — that's what keeps this
  // route safe to let any session owner call for their own session, unlike the
  // generic envOverrides field the privilegedEnvKeys clamp exists to guard.
+  //
+  // ⚠️ Local sessions only for now. A remote session's `restartCli()` renders
+  // `ssh ... tmux new-session -A`, which reattaches the durable remote tmux rather than
+  // restarting the agent, and the env lands on the LOCAL pane running ssh, which
+  // forwards nothing; docker is the same attach-or-create shape. Both used to answer
+  // `restarted: true` and change nothing, so they are refused until those paths are
+  // plumbed (the env would have to ride the remote/in-container launch command).
  app.post('/api/sessions/:id/custom-model', async (req) => {
    const { id } = req.params as { id: string };
    const body = parseBody(CustomModelSelectionSchema, req.body, 'Invalid request body');
    const session = findSessionOrFail(ctx, id, req);

+    if (session.remote || session.docker) {
+      return createErrorResponse(
+        ApiErrorCode.INVALID_INPUT,
+        'Custom model endpoints are not supported for remote (SSH) or Docker sessions yet'
+      );
+    }
    if (session.isBusy()) {
      return createErrorResponse(ApiErrorCode.SESSION_BUSY, 'Session is busy');
    }

    if ('clear' in body) {
-      const previousConfigDir = session.setCustomModel(undefined);
+      const { previousConfigDir } = session.setCustomModel(undefined);
      removeConfigDir(previousConfigDir);
      const restarted = await session.restartCli();
      persistAndBroadcastSession(ctx, session);
@@ -1196,32 +1209,41 @@ export function registerSessionRoutes(
      return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Model endpoint not found');
    }

-    const injection = buildCustomModelInjection(entry, endpoint, body.modelId);
-
-    let envOverrides: Record<string, string>;
-    let envKeys: string[];
-    let configDir: string | undefined;
-
-    if (injection.kind === 'env') {
-      envOverrides = injection.envOverrides;
-      envKeys = Object.keys(injection.envOverrides);
-    } else if (injection.kind === 'configDir') {
-      // Isolated per-session dir — never the user's real CLI config path.
-      configDir = join(dataPath('custom-model-configs'), session.id);
-      envOverrides = applyConfigDirInjection(configDir, injection);
-      envKeys = Object.keys(envOverrides);
-    } else {
-      // 'unsupported' is already handled above; this keeps the switch exhaustive.
+    // A CLI whose config alone cannot select the model also gets its `model` launch param
+    // forced (pi/omp `custom/<id>`, grok's block name). The argv engine DROPS a token that
+    // fails its pattern rather than quoting it, which would silently launch the CLI on its
+    // own default provider again, so refuse an id the pattern cannot carry up front.
+    const modelSpec = entry.launch.params.model;
+    const applied = applyCustomModelInjection(entry, endpoint, body.modelId, session.id);
+    if (!applied) {
      return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `${session.mode} has no known custom-model mechanism`);
    }
+    if (
+      applied.launchModel !== undefined &&
+      modelSpec?.type === 'token' &&
+      !matchesPattern(modelSpec.pattern, applied.launchModel)
+    ) {
+      removeConfigDir(applied.configDir);
+      return createErrorResponse(
+        ApiErrorCode.INVALID_INPUT,
+        `Model id ${JSON.stringify(body.modelId)} cannot be passed to ${session.mode} on its command line`
+      );
+    }

-    const previousConfigDir = session.setCustomModel(
-      { endpointId: endpoint.id, modelId: body.modelId, label: endpoint.label, envKeys, configDir },
-      envOverrides
+    const { previousConfigDir } = session.setCustomModel(
+      {
+        endpointId: endpoint.id,
+        modelId: body.modelId,
+        label: endpoint.label,
+        envKeys: applied.envKeys,
+        configDir: applied.configDir,
+        launchModel: applied.launchModel,
+      },
+      applied.envOverrides
    );
    // Clean up the OLD config dir on disk, unless the new one happens to reuse the same
    // path (same session, configDir kind again) — never delete the dir we just wrote.
-    if (previousConfigDir && previousConfigDir !== configDir) {
+    if (previousConfigDir && previousConfigDir !== applied.configDir) {
      removeConfigDir(previousConfigDir);
    }

@@ -739,29 +739,6 @@ export const RemoteHostSchema = z.object({
  commands: RemoteCommandOverridesSchema,
 });

-// Custom Model Endpoint Profiles (deployment_plan.md) — a user-configured custom
-// OpenAI-compatible endpoint, local (llama.cpp) or cloud (Azure AI Foundry, etc.).
-export const CustomModelHostSchema = z.object({
-  id: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid endpoint id'),
-  label: z.string().min(1).max(100),
-  baseUrl: z.string().url().max(2048),
-  apiKey: z.string().max(4096).optional(),
-  // No 'both': live-tested against a real server, sending both auth header
-  // conventions on one request reliably HANGS it — see custom-model-hosts.ts.
-  authStyle: z.enum(['bearer', 'api-key']).optional(),
-  models: z.array(z.string().max(200)).max(200).optional(),
-  lastDiscoveredAt: z.string().max(64).optional(),
-});
-
-/** POST /api/sessions/:id/custom-model — apply or clear a session's custom-model selection. */
-export const CustomModelSelectionSchema = z.union([
-  z.object({
-    endpointId: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid endpoint id'),
-    modelId: z.string().min(1).max(200),
-  }),
-  z.object({ clear: z.literal(true) }),
-]);
-
 export const RemoteCaseLinkSchema = z.object({
  name: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid case name format'),
  hostId: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid remote host id'),
@@ -1261,7 +1238,7 @@ export const SettingsUpdateSchema = z
     */
    readMyMindEnabled: z.boolean().optional(),
    /**
-     * Custom Model Endpoint Profiles (deployment_plan.md): the toolbar picker that lets a
+     * Custom Model Endpoint Profiles (docs/custom-model-endpoints-plan.md): the toolbar picker that lets a
     * session point at a user-configured custom OpenAI-compatible endpoint (local or
     * cloud) instead of its native cloud backend. SYNCED, default OFF — endpoint entry,
     * discovery, and the extra toolbar surface are all opt-in.
@@ -1918,3 +1895,29 @@ export const WebviewUpdateSchema = WebviewBaseSchema.partial();

 /** POST /api/webviews/probe: reachability + framing check for the editor's Test button. */
 export const WebviewProbeSchema = z.object({ url: webviewUrlSchema });
+
+// Custom Model Endpoint Profiles (docs/custom-model-endpoints-plan.md) — a
+// user-configured custom OpenAI-compatible endpoint, local (llama.cpp) or cloud
+// (Azure AI Foundry, etc.). Lives below `webviewUrlSchema` because `baseUrl` IS that
+// schema: http(s) only, a real hostname, no embedded credentials, and the link-local /
+// cloud-metadata refusal, the same bar a saved dashboard URL has to clear.
+export const CustomModelHostSchema = z.object({
+  id: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid endpoint id'),
+  label: z.string().min(1).max(100),
+  baseUrl: webviewUrlSchema,
+  apiKey: z.string().max(4096).optional(),
+  // No 'both': live-tested against a real server, sending both auth header
+  // conventions on one request reliably HANGS it — see custom-model-hosts.ts.
+  authStyle: z.enum(['bearer', 'api-key']).optional(),
+  models: z.array(z.string().max(200)).max(200).optional(),
+  lastDiscoveredAt: z.string().max(64).optional(),
+});
+
+/** POST /api/sessions/:id/custom-model — apply or clear a session's custom-model selection. */
+export const CustomModelSelectionSchema = z.union([
+  z.object({
+    endpointId: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid endpoint id'),
+    modelId: z.string().min(1).max(200),
+  }),
+  z.object({ clear: z.literal(true) }),
+]);
@@ -67,6 +67,10 @@ import {
 import { imageWatcher } from '../image-watcher.js';
 import { workflowRunWatcher, summarizeRun } from '../workflow-run-watcher.js';
 import { attachmentRegistry, buildFileThumbnailRoute, registerExternalAttachment } from '../attachment-registry.js';
+import { getCli } from '../config/cli-registry/registry.js';
+import { readCustomModelHosts } from '../custom-model-hosts.js';
+import { applyCustomModelInjection, customModelConfigDir, removeConfigDir } from '../custom-model-injection-apply.js';
+import type { CustomModelBookkeeping } from '../types/session.js';
 import { registerGeneratedArtifactAttachment } from '../generated-artifact-attachments.js';
 import {
  buildDetectedAttachmentHistoryItem,
@@ -1181,6 +1185,34 @@ export class WebServer extends EventEmitter {
    });
  }

+  /**
+   * Recovery half of Custom Model Endpoint Profiles: the env values a selection injects
+   * are never persisted (they carry the API key), so they are computed again from the
+   * endpoint store, through the SAME apply path the route uses. Undefined when the
+   * endpoint is gone or the CLI is unregistered: the bookkeeping is still restored so
+   * the selection can be cleared, and the pane keeps running on tmux's retained env.
+   */
+  private async _rebuildCustomModelEnv(
+    session: Session,
+    saved: CustomModelBookkeeping
+  ): Promise<Record<string, string> | undefined> {
+    const entry = getCli(session.mode);
+    if (!entry) return undefined;
+    const endpoint = (await readCustomModelHosts(getDataDir())).find((h) => h.id === saved.endpointId);
+    if (!endpoint) {
+      console.warn(
+        `[WebServer] custom-model endpoint ${saved.endpointId} no longer exists; selection kept for clearing`
+      );
+      return undefined;
+    }
+    try {
+      return applyCustomModelInjection(entry, endpoint, saved.modelId, session.id)?.envOverrides;
+    } catch (err) {
+      console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err);
+      return undefined;
+    }
+  }
+
  /** Persists full session state including respawn config to state.json */
  private _persistSessionStateNow(session: Session): void {
    // See session-manager.updateSessionState: __envOverrides is an internal disk-only
@@ -1190,10 +1222,14 @@ export class WebServer extends EventEmitter {
    // __attachmentHistory keeps the private (externalPath-bearing) history on disk,
    // separate from the sanitized public attachmentHistory in toState().
    const attachmentHistory = session.getAttachmentHistoryForPersist();
+    // __customModel keeps the selection's bookkeeping (injected env KEYS, config dir,
+    // launch model; never the values) so recovery can restore and later clear it.
+    const customModel = session.getCustomModelForPersist();
    const state = {
      ...base,
      ...(envOverrides ? { __envOverrides: envOverrides } : {}),
      ...(attachmentHistory ? { __attachmentHistory: attachmentHistory } : {}),
+      ...(customModel ? { __customModel: customModel } : {}),
    } as SessionState;
    const controller = this.respawnControllers.get(session.id);
    if (controller) {
@@ -1396,6 +1432,9 @@ export class WebServer extends EventEmitter {
      // come back to a loader whose file we deleted.
      if (killMux) {
        void removeAgentSessionPreamble(sessionId);
+        // The per-session custom-model config dir carries the endpoint's API key (pi and
+        // omp embed it literally); it must not outlive the session it was written for.
+        removeConfigDir(customModelConfigDir(sessionId));
      }
      await session.stop(killMux);
      this.sessions.delete(sessionId);
@@ -2853,6 +2892,7 @@ export class WebServer extends EventEmitter {
            // Note: a legacy CLAUDE_CODE_EFFORT_LEVEL entry is auto-migrated to `effort`
            // by the Session constructor (env var would hard-lock /effort switching).
            const savedEnvOverrides = (savedState as { __envOverrides?: Record<string, string> })?.__envOverrides;
+            const savedCustomModel = (savedState as { __customModel?: CustomModelBookkeeping })?.__customModel;
            // Prefer the private (externalPath-bearing) history; fall back to the
            // sanitized public copy for sessions persisted before that split.
            const savedAttachmentHistory =
@@ -2915,6 +2955,16 @@ export class WebServer extends EventEmitter {
              parentSessionId: savedState?.parentSessionId,
            });

+            // Custom-model selection survives the restart. The tmux session still carries
+            // the injected `setenv`s (that is what kept the pane on the endpoint across the
+            // restart), but `_envOverrides` is rebuilt from a persist that deliberately
+            // excludes them, so re-derive the values from the endpoint store and re-write
+            // the isolated config dir; an endpoint that has since been deleted still gets
+            // the bookkeeping restored, which is what a later clear needs to unset.
+            if (savedCustomModel) {
+              session.setCustomModel(savedCustomModel, await this._rebuildCustomModelEnv(session, savedCustomModel));
+            }
+
            // Update session name if it was a "Restored:" placeholder or doesn't match saved name
            if (savedState?.name && muxSession.name !== savedState.name) {
              this.mux.updateSessionName(muxSession.sessionId, savedState.name);