From 1464332e91821c7da22b4762163a54c1bbad7939 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Mon, 2 Nov 2026 14:06:56 +0800 Subject: [PATCH] fix(custom-model): create missing dsh home dirs before linking, rebuild restore bookkeeping, dedicated custom key var, schema guards for the real-home recipe - linkFromRealHomeDirs marks directory entries: missing dirs are created (0700) instead of leaving a dangling link that failed every prompt with ENOENT; missing file targets are skipped - _restoreCustomModel rebuilds the bookkeeping from the CURRENT recipe (configDir + env-key union) so a selection saved under the old env recipe can still be cleared after upgrade; drop the dead session-home recovery lookup - deepseek's patch now references CODEMAN_CUSTOM_MODEL_API_KEY, so an in-session /model switch back to the official route cannot carry the custom key to api.deepseek.com (privilegedEnvKeys updated) - schema: realHomeFallback must be one relative segment; linkFromRealHome requires it and must not collide with fileName - docs: maintainer-verified real-dsh run replaces the outstanding-verification wording; shares-vs-isolates note; plan table row refreshed --- docs/custom-model-endpoints-plan.md | 8 +- docs/custom-model-endpoints.md | 24 +++-- src/config/cli-registry/schema.ts | 36 +++++++- src/config/cli-registry/stock.ts | 11 ++- src/config/cli-registry/types.ts | 11 +++ src/custom-model-injection-apply.ts | 17 +++- src/custom-model-injection.ts | 21 +++-- src/web/server.ts | 60 ++++++++++--- test/cli-registry-schema.test.ts | 41 +++++++++ test/custom-model-injection-apply.test.ts | 2 +- test/custom-model-injection-contract.test.ts | 4 +- test/custom-model-injection.test.ts | 9 +- test/custom-model-recovery.test.ts | 94 +++++++++++++------- test/deepseek-custom-endpoint.test.ts | 72 +++++++++++++-- 14 files changed, 335 insertions(+), 75 deletions(-) diff --git a/docs/custom-model-endpoints-plan.md b/docs/custom-model-endpoints-plan.md index 5078f17b..a72b1406 100644 --- a/docs/custom-model-endpoints-plan.md +++ b/docs/custom-model-endpoints-plan.md @@ -8,8 +8,10 @@ against a **cloud** OpenAI-compatible endpoint too (e.g. Azure AI Foundry's OpenAI-compatible inference endpoint, OpenRouter, a self-hosted gateway). Right now every Codeman session mode defaults to its native cloud backend with no way to redirect a session at any other endpoint from the UI — the -closest existing precedent is DeepSeek's server-env-sourced -`DEEPSEEK_BASE_URL`, which isn't user-facing. +closest existing precedent was DeepSeek's server-env-sourced +`DEEPSEEK_BASE_URL`, which wasn't user-facing (that env redirect has since +been superseded by deepseek's per-session patch recipe — see the table +below). **Scope note**: this plan originally said "local LLM." It now covers any OpenAI-compatible endpoint the user configures — local (llama.cpp, Ollama, @@ -112,7 +114,7 @@ declared capability, never an `if (mode === 'claude')` branch. | `gemini` | Env vars `GOOGLE_GEMINI_BASE_URL` + `GEMINI_API_KEY` + `GEMINI_MODEL`; CLI needs a restart to pick them up | **Confirmed BROKEN against llama.cpp/llama-swap, unresolved after real investigation.** Setting `GOOGLE_GEMINI_BASE_URL` makes gemini-cli internally select an `AuthType.GATEWAY` auth path (undocumented — inferred from behaviour) with validation requirements distinct from every normal auth mode; a real run against llama-swap fails with `Invalid auth method selected` regardless of what key/format is supplied. Tried and all failed: a Google-format dummy API key, `GOOGLE_GENAI_USE_VERTEXAI=false`, a `GEMINI_DEFAULT_AUTH_TYPE` override, and hand-writing `settings.json` directly. `--skip-trust` was a real, separate fix (without it a trust-folder check silently overrides `--approval-mode yolo` back to `default`) but does not touch this auth failure. Documented as an open gap, not shipped as working — the registry entry and injection code exist and are exercised by the test script, but end-to-end gemini support needs upstream investigation of `GATEWAY` AuthType before it can be called done | | `pi` | Config file `~/.pi/agent/models.json` with a custom provider whose `models` is an **array** of `{id}` objects (not an object keyed by id) plus `authHeader: true`. Redirected via the child process's own `HOME` env var, isolated per test/session — **not** `PI_CONFIG_DIR`, which does nothing for pi (grepped pi's entire bundled JS source: the string appears nowhere) | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back. Two real bugs found and fixed before this worked: (1) `PI_CONFIG_DIR` is not read by pi at all — pi hardcodes `~/.pi/agent/models.json` with no dedicated override, so the actual redirect has to be the child process's `HOME`; (2) `models` must be an array of `{id}` objects per pi's own bundled `docs/models.md`, not an object keyed by model id (silently loaded zero models). Also requires an explicit `--model custom/` on invocation — without it pi falls back to its own default provider and fails with "No API key found for the selected model" | | `grok` | TOML `config.toml`: a fixed `[model.codeman-custom]` block (`base_url`, `env_key` naming an env var the key rides in, never a literal TOML field) written to an isolated dir via `GROK_HOME`. Invoked with `-m codeman-custom` | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back. The ORIGINAL recipe in this table (env vars `GROK_BASE_URL`/`XAI_API_KEY`/`GROK_MODEL`) was flat-out **wrong**, not just unverified: it produced "Not signed in" against a real binary. Grok's real mechanism, confirmed against xAI's own docs and a live binary, is a `config.toml` with a `[model.]` block, redirected via `GROK_HOME`; the key still rides as an env var (`XAI_API_KEY` via `env_key`), just referenced from the TOML rather than read directly | -| `deepseek` | Reuse the **existing** `DEEPSEEK_BASE_URL` + `DEEPSEEK_API_KEY` keys (already declared in `stock.ts`), now with `appendV1Suffix: true` (see confidence). Only `DEEPSEEK_BASE_URL` is in `privilegedEnvKeys` — `DEEPSEEK_API_KEY` deliberately stays clamp-exempt, since a non-granted owner supplying their OWN key removes privilege rather than granting it (adding it to the clamp list was a real regression, caught by `test/deepseek-mode.test.ts` and fixed before merge). No model-selection var — dsh model is a profile composition entry, not a flag/env var | **Root cause of the original `HTTP_404` found and fixed, by reading dsh's own bundled source — the same bar pi/grok's fixes were held to.** Installed `@deepseek-ai/dsh` (all its real published dependencies) into a scratch directory purely to read `@deepseek-ai/dsh-llm-deepseek/lib/index.js`: it builds its request as `fetch(\`${connection.baseURL}/chat/completions\`, ...)`with`baseURL`read straight from`DEEPSEEK_BASE_URL`(or defaulting to DeepSeek's real public API root,`https://api.deepseek.com`, which also carries no `/v1`) — no `/v1` insertion of dsh's own, unlike the OpenAI-SDK convention this recipe originally assumed. llama-swap/llama.cpp only ever serves the OpenAI-conventional `/v1/chat/completions`. Confirmed live: `POST /chat/completions` → `404`, `POST /v1/chat/completions` → `200`, on the exact same endpoint — and dsh's own error-message template, `DeepSeek API error (HTTP ${status})`, reproduces the originally reported `dsh: HTTP_404: DeepSeek API error (HTTP 404)` precisely. Fixed by adding `appendV1Suffix` (env kind only, deepseek's entry alone — claude/gemini must NOT get it, since claude was already confirmed working against the unmodified `baseUrl`), which runs `endpoint.baseUrl` through the same `withV1Suffix()` helper `configDir`-kind CLIs already use. ⚠️ Not yet re-run end-to-end with a real `dsh` binary — no install available in this environment (no npm-installed CLI binary in `PATH`, and the `codeman-test-picker` container doesn't bundle it either); the fix is source-confirmed and live-verified at the HTTP level, but a genuine "hello world" reply through `dsh` itself is the remaining step before promoting this to **verified** alongside claude/opencode/pi/grok/omp | +| `deepseek` | Per-session **`DSH_HOME` configDir recipe** (supersedes the original `DEEPSEEK_BASE_URL` + `appendV1Suffix` env redirect): an isolated dir holding only a generated `cordis.patch.yml` — a `codeman-custom` `openai-completions` provider (the base URL gets the `/v1` suffix from the shared `renderConfigFile` step), plus home-level `dsh-tui`/`agent-default-model` rows that select the exact model; the key is referenced by name via `apiKeyEnv: CODEMAN_CUSTOM_MODEL_API_KEY` — a dedicated var, because dsh's own `llm-deepseek`/`web-search-deepseek` patch rows read `DEEPSEEK_API_KEY`, and an in-session `/model` switch back to the official route must not carry the custom key to api.deepseek.com. `linkFromRealHome` + `linkFromRealHomeDirs` symlink `profiles`/`sessions` (directories, created if the real home never ran a turn) and `settings.yaml` back out of the real dsh home. `DSH_HOME` and `CODEMAN_CUSTOM_MODEL_API_KEY` are in `privilegedEnvKeys`; the operator's own `DEEPSEEK_API_KEY` handling (clamp-exempt, server-key forwarding) is unchanged | **Verified end-to-end against a real dsh install by the maintainer** (dsh 0.1.1-rc.2, dsh-TUI 0.10.0-beta.1): `--dump-config` shows the three patched rows, the TUI boots on the chosen model, and a prompt reaches the endpoint as `POST /v1/chat/completions` with the right model, the bearer key and `max_tokens: 8192` — the reply renders and the context window shows in the footer. The original `HTTP_404` root cause stays documented in docs/custom-model-endpoints.md: dsh concatenates `/chat/completions` onto the raw base URL, which is why the env recipe needed `/v1` | | `omp` | Config file `~/.omp/agent/models.yml` with the same array-shaped `models` + `authHeader: true` fix as pi. Redirected via `HOME`, same reasoning as pi (`PI_CONFIG_DIR` does not relocate omp's config either, despite an earlier CLAUDE.md note claiming it does) | **Verified end-to-end** against a real llama-swap server — real "hello world" reply came back, after applying the same two fixes as pi (array-shaped `models`, `HOME`-redirect instead of `PI_CONFIG_DIR`) plus an explicit `--model custom/` on invocation. Unverified against omp's own official docs (none are bundled in the install), but empirically confirmed working live | | `antigravity` | No CLI/env/config mechanism found — Antigravity's docs describe only a GUI settings panel, and explicitly say a custom endpoint "cannot currently" become the core reasoning model. **Not implemented**; toolbar entry stays disabled for this mode with an explanatory tooltip | No known mechanism | diff --git a/docs/custom-model-endpoints.md b/docs/custom-model-endpoints.md index 74130e31..e7790e4c 100644 --- a/docs/custom-model-endpoints.md +++ b/docs/custom-model-endpoints.md @@ -549,11 +549,25 @@ for '' not found. Defaulting to fallback metadata...` on launch — through), so the installed profile keeps supplying the transport; the recipe's `linkFromRealHome` field symlinks `profiles`, `sessions` and `settings.yaml` back out of the real dsh home so installed bundles, - transcripts and user settings all survive the redirect. Not yet re-run - end-to-end with a real `dsh` binary — the fix - is source-confirmed and live-verified at the HTTP level, but a real - "hello world" reply through `dsh` itself is still outstanding before - calling this fully verified like the harnesses above. + transcripts and user settings all survive the redirect (missing + directory entries are created first — a home that has never run a + turn would otherwise get a dangling link whose every write through + it failed with ENOENT). **Verified end-to-end against a real dsh + install by the maintainer** (dsh 0.1.1-rc.2, dsh-TUI 0.10.0-beta.1): + `--dump-config` shows the three patched rows, the TUI boots on the + chosen model, and a prompt reaches the endpoint as + `POST /v1/chat/completions` with the right model, the bearer key and + `max_tokens: 8192`; the reply renders and the context window shows in + the footer. What such a session SHARES with the real home is exactly + the linked entries — `profiles/`, `sessions/`, `settings.yaml`. + Everything else in the generated `DSH_HOME` stays isolated: + `storages/` (dsh-TUI's persisted `/model` choice and workspace + state), `.agent-presets` and `.env`. Whether those dotted ones should + be shared too is an open design question left to the maintainer — + linking them additionally needs `homeSegmentName` to allow a leading + dot. Note too that the generated home-level patch row REPLACES the + `dsh-tui` row's whole `config` for the session, so a profile-level + `effort`/`preset` does not apply. - **Antigravity** — no known custom-endpoint mechanism at all; unsupported. See the confidence table in `custom-model-endpoints-plan.md` for the full detail behind diff --git a/src/config/cli-registry/schema.ts b/src/config/cli-registry/schema.ts index dfa7e3f3..184bc569 100644 --- a/src/config/cli-registry/schema.ts +++ b/src/config/cli-registry/schema.ts @@ -595,9 +595,41 @@ const capabilitiesSchema = z // segments only — the consumer joins them, so a traversal here would escape the // real home. See the field's comment in cli-registry/types.ts. linkFromRealHome: z.array(homeSegmentName).max(8).optional(), - realHomeFallback: z.string().min(1).max(80).optional(), + // Which of `linkFromRealHome` are directories (missing ones are created before + // linking; missing files are skipped). Same segment pattern, see types.ts. + linkFromRealHomeDirs: z.array(homeSegmentName).max(8).optional(), + // The real home's fallback location, relative to the user's home: ONE segment — + // a leading dot is legitimate (`.dsh` is what ships) but separators and `..` + // are not; the consumer joins it under the user's home, and a traversal here + // escapes that home. + realHomeFallback: z + .string() + .min(1) + .max(80) + .regex(/^(?!\.\.\/?$)[^/\\]+$/) + .optional(), }) - .strict(), + .strict() + .superRefine((recipe, ctx) => { + // Both rules were comments before; the schema is the security boundary, so + // enforce them at load time (see the header of this file). + if (recipe.linkFromRealHome?.length && !recipe.realHomeFallback) { + ctx.addIssue({ + code: 'custom', + message: 'realHomeFallback is required whenever linkFromRealHome is declared', + path: ['realHomeFallback'], + }); + } + for (const name of recipe.linkFromRealHome ?? []) { + if (name === recipe.fileName) { + ctx.addIssue({ + code: 'custom', + message: `linkFromRealHome must never include fileName (${JSON.stringify(name)}): the generated file owns that name in the isolated directory, a link there would write THROUGH into the user's real file`, + path: ['linkFromRealHome'], + }); + } + } + }), z.object({ kind: z.literal('unsupported') }).strict(), ]), }) diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index 5ce80be8..8b00ab64 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1545,8 +1545,10 @@ const DEEPSEEK: CliEntry = { // SERVER's own key into every dsh pane, so DEEPSEEK_BASE_URL is the exfiltration // vector, not the key itself — a non-granted owner supplying THEIR OWN key removes // privilege rather than granting it, and clamping it here was a real regression - // (test/deepseek-mode.test.ts) fixed before this shipped. - privilegedEnvKeys: ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL'], + // (test/deepseek-mode.test.ts) fixed before this shipped. CODEMAN_CUSTOM_MODEL_API_KEY + // joins it (like codex's entry): the dedicated custom-endpoint key rides the same + // allowlisted-envOverrides path as the redirect itself. + privilegedEnvKeys: ['DSH_PERMISSION_MODE', 'DSH_HOME', 'DEEPSEEK_BASE_URL', 'CODEMAN_CUSTOM_MODEL_API_KEY'], // The installed profile supplies the transport; a per-session patch selects its exact model. customModelInjection: { kind: 'configDir', @@ -1558,7 +1560,12 @@ const DEEPSEEK: CliEntry = { // close), `profiles` are the installed bundles the patch composes over, and // `settings.yaml` must keep applying. The generated home-level `cordis.patch.yml` // deliberately replaces the user's own for the session rather than linking it. + // `sessions` and `profiles` are directories: dsh creates them itself on a first + // turn, and a fresh/relocated $DSH_HOME that never ran one would otherwise get a + // dangling link whose every write fails ENOENT — the consumer creates missing + // directory targets and skips missing file targets like `settings.yaml`. linkFromRealHome: ['profiles', 'sessions', 'settings.yaml'], + linkFromRealHomeDirs: ['profiles', 'sessions'], realHomeFallback: '.dsh', }, }, diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index f5ca60f7..91f91457 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -896,6 +896,17 @@ export interface CliCapabilities { * Consumed generically by custom-model-injection-apply.ts. */ linkFromRealHome?: string[]; + /** + * Which of `linkFromRealHome` are DIRECTORIES (`sessions`, `profiles`) rather than + * files (`settings.yaml`). A missing directory target is created (mode 0700, what + * dsh does itself on a first turn) before linking — linking a nonexistent directory + * leaves a dangling symlink the CLI's own `mkdir` under it fails on with ENOENT on + * EVERY turn. A missing FILE target is skipped instead of linked (dangling-file + * links serve no purpose the CLI could not do itself). Must be a subset of + * `linkFromRealHome`; entries absent from it are treated as files. + * Consumed generically by custom-model-injection-apply.ts. + */ + linkFromRealHomeDirs?: string[]; /** * Fallback location of the CLI's real home, relative to the user's home directory * (deepseek's `.dsh`), used only when neither the session's own `dirEnvVar` override diff --git a/src/custom-model-injection-apply.ts b/src/custom-model-injection-apply.ts index 3ca067b0..627307df 100644 --- a/src/custom-model-injection-apply.ts +++ b/src/custom-model-injection-apply.ts @@ -81,12 +81,13 @@ function linkRealHomeEntries(baseDir: string, injection: ConfigDirInjection, ses process.env[injection.dirEnvVar]?.trim() || (injection.realHomeFallback ? join(homedir(), injection.realHomeFallback) : undefined); if (!realHome) return; + const dirs = new Set(injection.linkFromRealHomeDirs ?? []); for (const name of names) { - linkRealHomeEntry(join(realHome, name), join(baseDir, name)); + linkRealHomeEntry(join(realHome, name), join(baseDir, name), dirs.has(name)); } } -function linkRealHomeEntry(target: string, link: string): void { +function linkRealHomeEntry(target: string, link: string, isDir: boolean): void { // lstat, never existsSync: existsSync is false for a DANGLING link, and recovery // re-applies on every restart, so the second pass would try to recreate the link and // throw EEXIST. lstat answers for real entries, good links and dangling links alike. @@ -95,6 +96,18 @@ function linkRealHomeEntry(target: string, link: string): void { } catch { // ENOENT — nothing is here yet, the only state in which a link gets created } + if (!existsSync(target)) { + if (!isDir) return; // missing FILE target: skip, never plant a dangling file link + // Missing DIRECTORY target: a dangling directory link is not inert — the CLI's own + // recursive mkdir THROUGH it fails ENOENT on every turn (dsh writing under + // `$DSH_HOME/sessions` on a fresh/relocated home). Create it first, 0700, exactly + // what dsh itself does on a first turn. + try { + mkdirSync(target, { recursive: true, mode: 0o700 }); + } catch { + return; // e.g. the target's parent refuses writes — no link rather than a dangling one + } + } try { symlinkSync(target, link, platform() === 'win32' ? 'junction' : 'dir'); } catch { diff --git a/src/custom-model-injection.ts b/src/custom-model-injection.ts index 24aface2..02b189a7 100644 --- a/src/custom-model-injection.ts +++ b/src/custom-model-injection.ts @@ -89,6 +89,8 @@ export interface ConfigDirInjection { launchModel?: string; /** Carried from the registry recipe — see `cli-registry/types.ts` `linkFromRealHome`. */ linkFromRealHome?: string[]; + /** Carried from the registry recipe — see `cli-registry/types.ts` `linkFromRealHomeDirs`. */ + linkFromRealHomeDirs?: string[]; /** Carried from the registry recipe — see `cli-registry/types.ts` `realHomeFallback`. */ realHomeFallback?: string; } @@ -153,6 +155,7 @@ export function buildCustomModelInjection( files: [{ relPath: cap.fileName, content }], extraEnv, ...(cap.linkFromRealHome ? { linkFromRealHome: cap.linkFromRealHome } : {}), + ...(cap.linkFromRealHomeDirs ? { linkFromRealHomeDirs: cap.linkFromRealHomeDirs } : {}), ...(cap.realHomeFallback ? { realHomeFallback: cap.realHomeFallback } : {}), }, cap.launchModel, @@ -196,7 +199,14 @@ function renderConfigContent( } } -const CODEX_API_KEY_ENV_VAR = 'CODEMAN_CUSTOM_MODEL_API_KEY'; +/** The DEDICATED env var carrying a custom endpoint's API key for config-file recipes that + * reference keys by name (codex's `env_key`, deepseek's `apiKeyEnv`). Never a CLI-native + * name: dsh's own `llm-deepseek`/`web-search-deepseek` rows read `DEEPSEEK_API_KEY` too, + * so an in-session `/model` switch back to `deepseek-official` (or a profile with web + * search on) would otherwise send the custom endpoint's key to api.deepseek.com. Both + * entries list it in `privilegedEnvKeys` (stock.ts) like each other's redirect vars. + */ +const CUSTOM_MODEL_API_KEY_ENV_VAR = 'CODEMAN_CUSTOM_MODEL_API_KEY'; /** The `[model.]` block name grok's config.toml uses for the injected model — also * what `-m ` in the standalone script's ONE_SHOT argv must reference to select it. */ @@ -222,7 +232,8 @@ function renderConfigFile( config: { providers: { 'codeman-custom': { - apiKeyEnv: 'DEEPSEEK_API_KEY', + // By NAME only (never the literal key — see CUSTOM_MODEL_API_KEY_ENV_VAR). + apiKeyEnv: CUSTOM_MODEL_API_KEY_ENV_VAR, api: 'openai-completions', baseURL: baseUrl, models: [ @@ -247,7 +258,7 @@ function renderConfigFile( null, 2 ), - extraEnv: { DEEPSEEK_API_KEY: apiKey }, + extraEnv: { [CUSTOM_MODEL_API_KEY_ENV_VAR]: apiKey }, }; } case 'codex-toml': { @@ -272,11 +283,11 @@ function renderConfigFile( '[model_providers.custom]', `name = "Custom Endpoint"`, `base_url = ${quoted(baseUrl)}`, - `env_key = ${quoted(CODEX_API_KEY_ENV_VAR)}`, + `env_key = ${quoted(CUSTOM_MODEL_API_KEY_ENV_VAR)}`, `wire_api = "responses"`, '', ].join('\n'); - return { content, extraEnv: { [CODEX_API_KEY_ENV_VAR]: apiKey } }; + return { content, extraEnv: { [CUSTOM_MODEL_API_KEY_ENV_VAR]: apiKey } }; } case 'pi-models-json': // Verified against pi's OWN bundled docs (models.md): `models` is an ARRAY of diff --git a/src/web/server.ts b/src/web/server.ts index b26b864e..61f1df98 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -81,7 +81,12 @@ import { folderGrants } from '../folder-listing.js'; import { getCli, enabledClis, listClis } from '../config/cli-registry/registry.js'; import { isCliEntryInstalled, probeStockCliAvailability } from '../utils/cli-installed-probes.js'; import { readCustomModelHosts } from '../custom-model-hosts.js'; -import { applyCustomModelInjection, customModelConfigDir, removeConfigDir } from '../custom-model-injection-apply.js'; +import { + applyCustomModelInjection, + customModelConfigDir, + removeConfigDir, + type AppliedCustomModel, +} from '../custom-model-injection-apply.js'; import type { CustomModelBookkeeping } from '../types/session.js'; import { registerGeneratedArtifactAttachment } from '../generated-artifact-attachments.js'; import { @@ -1454,7 +1459,7 @@ export class WebServer extends EventEmitter { private async _rebuildCustomModelEnv( session: Session, saved: CustomModelBookkeeping - ): Promise | undefined> { + ): Promise { const entry = getCli(session.mode); if (!entry) return undefined; const endpoint = (await readCustomModelHosts(getDataDir())).find((h) => h.id === saved.endpointId); @@ -1465,25 +1470,56 @@ export class WebServer extends EventEmitter { return undefined; } try { + // Boot recovery omits both `workingDir` (no reason to re-answer a trust dialog that + // already fired once) and the session-env real-home lookup: the injected VALUES are + // never persisted, so after a real restart the session can no longer carry its + // allowlisted `DSH_HOME` either (`getEnvOverridesForPersist()` keeps only + // `CLAUDE_CODE_*`/`CLAUDE_CONFIG_DIR`) — the lookup could only ever hit on the first + // apply in-process. Real-home resolution therefore falls back to the SERVER's + // `DSH_HOME` or the recipe's `realHomeFallback` (~/.dsh). return applyCustomModelInjection( entry, endpoint, saved.modelId, session.id, - endpoint.modelContextLengths?.[saved.modelId], - // Boot recovery deliberately omits `workingDir` (no reason to re-answer a trust - // dialog that already fired once), so the session-env lookup rides in after it: - // a session with its own DSH_HOME override must link its config dir back to - // THAT home, not the server's, exactly like the routes do. - undefined, - (envVar) => session.envOverrideValue(envVar) - )?.envOverrides; + endpoint.modelContextLengths?.[saved.modelId] + ); } catch (err) { console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err); return undefined; } } + /** + * Restore one persisted custom-model selection (both boot-recovery paths). The rebuilt + * bookkeeping must come from the CURRENT recipe, not from the saved record: this is the + * first feature where a shipped recipe changed KIND (deepseek: env redirect -> per-session + * patch home), so a selection saved by an old build restores with e.g. no `configDir` + * while the new recipe writes one — and clearing it later could then never delete the + * generated dir. `envKeys` is the UNION of saved and applied keys: the tmux session may + * still carry `setenv`s from both, and a clear has to unset every one. + */ + private async _restoreCustomModel(session: Session, saved: CustomModelBookkeeping): Promise { + const applied = await this._rebuildCustomModelEnv(session, saved); + if (!applied) { + // Endpoint gone or CLI unregistered: restore the bookkeeping exactly as saved, which + // is what a later clear needs. + session.setCustomModel(saved); + return; + } + session.setCustomModel( + { + ...saved, + // Prefer the current recipe's dir; keep the saved one when the recipe writes none, + // so a clear can still delete a directory an older recipe created. + configDir: applied.configDir ?? saved.configDir, + launchModel: applied.launchModel, + envKeys: [...new Set([...saved.envKeys, ...applied.envKeys])], + }, + applied.envOverrides + ); + } + /** 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 @@ -3610,7 +3646,7 @@ export class WebServer extends EventEmitter { // bookkeeping survives a restart and the values are re-derived here. const savedCustomModel = (saved as { __customModel?: CustomModelBookkeeping }).__customModel; if (savedCustomModel) { - session.setCustomModel(savedCustomModel, await this._rebuildCustomModelEnv(session, savedCustomModel)); + await this._restoreCustomModel(session, savedCustomModel); } if (saved.niceEnabled !== undefined || saved.niceValue !== undefined) { session.setNice({ enabled: saved.niceEnabled, niceValue: saved.niceValue }); @@ -3912,7 +3948,7 @@ export class WebServer extends EventEmitter { // 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)); + await this._restoreCustomModel(session, savedCustomModel); } // Update session name if it was a "Restored:" placeholder or doesn't match saved name diff --git a/test/cli-registry-schema.test.ts b/test/cli-registry-schema.test.ts index d0fa5f5b..1191c966 100644 --- a/test/cli-registry-schema.test.ts +++ b/test/cli-registry-schema.test.ts @@ -55,6 +55,47 @@ describe('customModelInjection.launchModel', () => { }); }); +/** Mutates the shipped deepseek configDir recipe (the one real-home-linking entry) and expects rejection. */ +function expectRejectedOnDeepSeek(mutate: (recipe: Record) => void, because: string): void { + const entry = baseEntry('deepseek'); + const recipe = (entry.capabilities as Record).customModelInjection as Record; + mutate(recipe); + const result = CliEntrySchema.safeParse(entry); + expect(result.success, `expected rejection: ${because}`).toBe(false); +} + +describe('configDir real-home linking (linkFromRealHome / realHomeFallback)', () => { + it('rejects a realHomeFallback that is not a single relative path segment', () => { + // The consumer joins it under the user's home, so any of these escapes that home. + for (const bad of ['../x', '.dsh/evil', '/.dsh', '..']) { + expectRejectedOnDeepSeek((r) => { + r.realHomeFallback = bad; + }, `realHomeFallback ${JSON.stringify(bad)}`); + } + }); + + it('accepts a shipped-style dot-prefixed segment', () => { + const entry = baseEntry('deepseek'); + expect(CliEntrySchema.safeParse(entry).success).toBe(true); // ships as `.dsh` + const recipe = (entry.capabilities as Record).customModelInjection as Record; + recipe.realHomeFallback = '.dsh-relocated'; + expect(CliEntrySchema.safeParse(entry).success).toBe(true); + }); + + it('requires realHomeFallback whenever linkFromRealHome is declared', () => { + expectRejectedOnDeepSeek((r) => { + delete r.realHomeFallback; + }, 'without it the consumer would have to hardcode a per-CLI home default'); + }); + + it('rejects a linked entry that collides with fileName', () => { + // A symlink at the generated file's own name would write THROUGH into the user's real config. + expectRejectedOnDeepSeek((r) => { + r.linkFromRealHome = ['cordis.patch.yml', 'profiles']; + }, 'linkFromRealHome containing fileName'); + }); +}); + describe('the shipped catalog', () => { it('validates every stock entry exactly as shipped', () => { // If this fails, the catalog cannot load at all — every other test here is downstream. diff --git a/test/custom-model-injection-apply.test.ts b/test/custom-model-injection-apply.test.ts index 4f4a887c..6aca903e 100644 --- a/test/custom-model-injection-apply.test.ts +++ b/test/custom-model-injection-apply.test.ts @@ -57,7 +57,7 @@ describe('applyCustomModelInjection: context length', () => { const sessionId = 'sess-deepseek-context'; sessionsToClean.push(sessionId); const applied = applyCustomModelInjection(entryOrThrow('deepseek'), endpoint, 'qwen3', sessionId, 32768); - expect(Object.keys(applied?.envOverrides ?? {}).sort()).toEqual(['DEEPSEEK_API_KEY', 'DSH_HOME']); + expect(Object.keys(applied?.envOverrides ?? {}).sort()).toEqual(['CODEMAN_CUSTOM_MODEL_API_KEY', 'DSH_HOME']); expect(readFileSync(join(applied!.configDir!, 'cordis.patch.yml'), 'utf8')).toContain('"contextWindow": 32768'); }); }); diff --git a/test/custom-model-injection-contract.test.ts b/test/custom-model-injection-contract.test.ts index ceb6450a..940e1ba0 100644 --- a/test/custom-model-injection-contract.test.ts +++ b/test/custom-model-injection-contract.test.ts @@ -194,12 +194,12 @@ describe('custom-model-injection contract (mock server)', () => { const injection = buildCustomModelInjection(entryOrThrow('deepseek'), endpointFor(mock), 'qwen3'); if (injection.kind !== 'configDir') throw new Error('unreachable'); expect(injection.dirEnvVar).toBe('DSH_HOME'); - expect(Object.keys(injection.extraEnv ?? {})).toEqual(['DEEPSEEK_API_KEY']); + expect(Object.keys(injection.extraEnv ?? {})).toEqual(['CODEMAN_CUSTOM_MODEL_API_KEY']); const provider = JSON.parse(injection.files[0].content)[0].config.providers['codeman-custom']; expect(provider.baseURL).toBe(`${mock.baseUrl}/v1`); expect(provider.models[0].id).toBe('qwen3'); - await callOpenAiCompat(provider.baseURL, injection.extraEnv!.DEEPSEEK_API_KEY, 'qwen3'); + await callOpenAiCompat(provider.baseURL, injection.extraEnv!.CODEMAN_CUSTOM_MODEL_API_KEY, 'qwen3'); expect(mock.requests[0].path).toBe('/v1/chat/completions'); expect(mock.requests[0].headers.authorization).toBe('Bearer contract-test-key'); diff --git a/test/custom-model-injection.test.ts b/test/custom-model-injection.test.ts index ab1e51a2..1649abb6 100644 --- a/test/custom-model-injection.test.ts +++ b/test/custom-model-injection.test.ts @@ -183,9 +183,16 @@ describe('buildCustomModelInjection', () => { const result = buildCustomModelInjection(entryOrThrow('deepseek'), endpoint, 'qwen3'); if (result.kind !== 'configDir') throw new Error('Expected isolated DeepSeek config'); expect(result.dirEnvVar).toBe('DSH_HOME'); - expect(result.extraEnv).toEqual({ DEEPSEEK_API_KEY: 'my-key' }); + expect(result.extraEnv).toEqual({ CODEMAN_CUSTOM_MODEL_API_KEY: 'my-key' }); expect(result.files[0].content).not.toContain('my-key'); const patch = JSON.parse(result.files[0].content); + // The patch references the DEDICATED var by name, never dsh's own DEEPSEEK_API_KEY — + // dsh's llm-deepseek/web-search rows read that too and would leak this key to + // api.deepseek.com on an in-session /model switch back to the official route. + expect(patch.find((row: { id: string }) => row.id === 'llm-pi-ai').config.providers['codeman-custom'].apiKeyEnv).toBe( + 'CODEMAN_CUSTOM_MODEL_API_KEY' + ); + expect(result.files[0].content).not.toContain('DEEPSEEK_API_KEY'); expect(patch.find((row: { id: string }) => row.id === 'dsh-tui').config.model).toBe('qwen3'); }); diff --git a/test/custom-model-recovery.test.ts b/test/custom-model-recovery.test.ts index cb9c54e1..75f1b69b 100644 --- a/test/custom-model-recovery.test.ts +++ b/test/custom-model-recovery.test.ts @@ -1,16 +1,24 @@ /** * @fileoverview Boot recovery re-derives a saved custom-model selection through - * `_rebuildCustomModelEnv`, and the `linkFromRealHome` recipe must resolve the CLI's - * real home from the SESSION's own env overrides first: the user who relocated dsh - * (allowlisted `DSH_HOME`) runs their CLI against THAT home, and recovery must link the - * regenerated config dir back to it, not to whatever the server's environment says. + * `_rebuildCustomModelEnv` and rebuilds the BOOKKEEPING from what the CURRENT recipe + * applied — not from the saved record. A selection persisted while deepseek still used + * the old env redirect comes back env-shaped (envKeys `DEEPSEEK_BASE_URL`/ + * `DEEPSEEK_API_KEY`, no `configDir`) although the recipe now writes a per-session patch + * home: restoring the saved record unchanged left `DSH_HOME` on a generated config dir + * that clearing could never delete. * - * `_rebuildCustomModelEnv` reads no `this` state (it re-derives everything from the endpoint - * store and the Session it is handed), so it is driven straight off the prototype with an - * empty receiver — `restoreMuxSessions()` itself cannot be reached under vitest, the same - * constraint recorded in test/session-model-recovery.test.ts. + * Real-home resolution is NOT recovered from the session's env overrides any more: + * `getEnvOverridesForPersist()` keeps only `CLAUDE_CODE_*`/`CLAUDE_CONFIG_DIR`, so a + * session's own `DSH_HOME` never survives a real restart and the lookup only ever hit on + * the first apply in-process. Recovery falls back to the server's `DSH_HOME` or the + * recipe's `realHomeFallback` (~/.dsh) — see `_rebuildCustomModelEnv`'s comment. + * + * `_restoreCustomModel` reads no `this` state beyond the rebuild helper (it re-derives + * everything from the endpoint store and the Session it is handed), so it is driven off + * the prototype with a minimal receiver — `restoreMuxSessions()` itself cannot be + * reached under vitest, the same constraint recorded in test/session-model-recovery.test.ts. */ -import { mkdirSync, realpathSync, rmSync } from 'node:fs'; +import { mkdirSync, rmSync } from 'node:fs'; import { homedir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -21,13 +29,14 @@ import { Session } from '../src/session.js'; import { WebServer } from '../src/web/server.js'; import type { CustomModelBookkeeping } from '../src/types/session.js'; -type RebuildFn = (session: Session, saved: CustomModelBookkeeping) => Promise | undefined>; - -/** The real method, with an empty receiver — see the file header for why that is sound. */ -function rebuildCustomModelEnv(session: Session, saved: CustomModelBookkeeping) { - const fn = (WebServer.prototype as unknown as { _rebuildCustomModelEnv: RebuildFn })._rebuildCustomModelEnv; - expect(typeof fn, '_rebuildCustomModelEnv vanished from WebServer.prototype').toBe('function'); - return fn.call({}, session, saved); +/** The real pair, on a receiver that only carries the rebuild helper — see the header. */ +async function restoreCustomModel(session: Session, saved: CustomModelBookkeeping): Promise { + const proto = WebServer.prototype as unknown as { + _rebuildCustomModelEnv: (session: Session, saved: CustomModelBookkeeping) => Promise; + _restoreCustomModel: (session: Session, saved: CustomModelBookkeeping) => Promise; + }; + expect(typeof proto._restoreCustomModel, '_restoreCustomModel vanished from WebServer.prototype').toBe('function'); + await proto._restoreCustomModel.call({ _rebuildCustomModelEnv: proto._rebuildCustomModelEnv }, session, saved); } function endpoint(): CustomModelHost { @@ -40,7 +49,7 @@ function endpoint(): CustomModelHost { }; } -describe('_rebuildCustomModelEnv: real-home resolution for linkFromRealHome', () => { +describe('_restoreCustomModel: bookkeeping rebuilt from the current recipe', () => { const configs: string[] = []; afterEach(() => { @@ -48,23 +57,46 @@ describe('_rebuildCustomModelEnv: real-home resolution for linkFromRealHome', () vi.unstubAllEnvs(); }); - it('links an isolated deepseek home from the SESSION’s own DSH_HOME, not the server’s', async () => { - // The user who relocated dsh runs their CLI against THAT home; recovery must re-link - // the session's generated config dir back to it even though the value never persists - // into the server's environment. The server's DSH_HOME is emptied deliberately: with - // the session lookup working, the fallback home is never consulted, and with it - // broken this test gets a link into ~/.dsh rather than a pass. - const sessionHome = join(homedir(), 'codeman-recovery-dsh-home'); - mkdirSync(join(sessionHome, 'profiles'), { recursive: true }); - vi.stubEnv('DSH_HOME', ''); + it('carries the configDir and env-key union for a selection saved by the old env recipe', async () => { + // Saved exactly as 1.31.0-1.47.0 persisted a deepseek selection: the env recipe's two + // keys, no configDir. The rebuilt record must carry the per-session config dir the + // CURRENT recipe wrote (so clearing deletes it) while KEEPING the saved keys, which + // the tmux session may still carry `setenv`s for (the union is what a clear unsets). + const dshHome = join(homedir(), 'codeman-recovery-dsh-home'); + mkdirSync(join(dshHome, 'profiles'), { recursive: true }); + vi.stubEnv('DSH_HOME', dshHome); await writeCustomModelHosts(getDataDir(), [endpoint()]); - const session = new Session({ workingDir: '/tmp', mode: 'deepseek', envOverrides: { DSH_HOME: sessionHome } }); + const session = new Session({ workingDir: '/tmp', mode: 'deepseek' }); configs.push(session.id); - const env = await rebuildCustomModelEnv(session, { endpointId: 'ep', modelId: 'qwen3', envKeys: [] }); + await restoreCustomModel(session, { + endpointId: 'ep', + modelId: 'qwen3', + label: 'Ep', + envKeys: ['DEEPSEEK_BASE_URL', 'DEEPSEEK_API_KEY'], + }); - expect(env?.DSH_HOME).toBe(customModelConfigDir(session.id)); - expect(realpathSync(join(env!.DSH_HOME, 'profiles'))).toBe(join(sessionHome, 'profiles')); - rmSync(sessionHome, { recursive: true, force: true }); + const persisted = session.getCustomModelForPersist(); + expect(persisted?.configDir).toBe(customModelConfigDir(session.id)); + expect(persisted?.launchModel).toBeUndefined(); + expect(new Set(persisted?.envKeys)).toEqual( + new Set(['DEEPSEEK_BASE_URL', 'DEEPSEEK_API_KEY', 'DSH_HOME', 'CODEMAN_CUSTOM_MODEL_API_KEY']) + ); + rmSync(dshHome, { recursive: true, force: true }); + }); + + it('restores an env-shaped record unchanged when the endpoint is gone, so a clear still works', async () => { + await writeCustomModelHosts(getDataDir(), []); + const session = new Session({ workingDir: '/tmp', mode: 'deepseek' }); + const saved: CustomModelBookkeeping = { + endpointId: 'gone', + modelId: 'qwen3', + label: 'Gone', + envKeys: ['DEEPSEEK_BASE_URL', 'DEEPSEEK_API_KEY'], + }; + + await restoreCustomModel(session, saved); + + expect(session.getCustomModelForPersist()).toEqual(saved); }); }); diff --git a/test/deepseek-custom-endpoint.test.ts b/test/deepseek-custom-endpoint.test.ts index 705e263e..f1a48e48 100644 --- a/test/deepseek-custom-endpoint.test.ts +++ b/test/deepseek-custom-endpoint.test.ts @@ -1,5 +1,5 @@ -import { mkdirSync, readFileSync, readlinkSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; -import { homedir } from 'node:os'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, realpathSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { getCli } from '../src/config/cli-registry/index.js'; @@ -32,7 +32,11 @@ describe('DeepSeek custom endpoint isolation', () => { if (!applied?.configDir) throw new Error('Missing isolated profile'); const content = readFileSync(join(applied.configDir, 'cordis.patch.yml'), 'utf8'); expect(content).not.toContain(endpoint.apiKey); - expect(applied.envOverrides.DEEPSEEK_API_KEY).toBe(endpoint.apiKey); + // The dedicated key var, never DEEPSEEK_API_KEY: dsh's own llm-deepseek/web-search + // rows read that too, and an in-session /model switch back to the official route must + // not take the custom endpoint's key to api.deepseek.com with it. + expect(applied.envOverrides.CODEMAN_CUSTOM_MODEL_API_KEY).toBe(endpoint.apiKey); + expect(applied.envOverrides.DEEPSEEK_API_KEY).toBeUndefined(); const patch = JSON.parse(content); const provider = patch.find((row: { id: string }) => row.id === 'llm-pi-ai').config.providers['codeman-custom']; expect(provider.api).toBe('openai-completions'); @@ -50,15 +54,65 @@ describe('DeepSeek custom endpoint isolation', () => { // must keep applying through the redirect. expect(realpathSync(join(applied.configDir, 'sessions'))).toBe(join(realHome, 'sessions')); expect(realpathSync(join(applied.configDir, 'settings.yaml'))).toBe(join(realHome, 'settings.yaml')); - // Re-apply onto the same config dir (boot recovery re-runs this every restart) with one - // link dangling: the lstat guard must skip every existing link, broken ones included, - // which the old existsSync check missed — recreating them would raise EEXIST. - rmSync(join(realHome, 'sessions'), { recursive: true, force: true }); + // Re-apply onto the same config dir (boot recovery re-runs this every restart): the + // lstat guard must skip every existing link, which the old existsSync check missed — + // recreating them would raise EEXIST. const reapplied = applyCustomModelInjection(entry, endpoint, 'chosen-model', 'deepseek-probe', 131072); expect(reapplied?.envOverrides.DSH_HOME).toBe(applied.configDir); - expect(readlinkSync(join(applied.configDir, 'sessions'))).toBe(join(realHome, 'sessions')); - expect(readFileSync(original, 'utf8')).toBe('original profile'); removeConfigDir(applied.configDir); expect(readFileSync(original, 'utf8')).toBe('original profile'); }); + + it('creates missing directory targets before linking, skips a missing file target, stays fine on re-apply', () => { + // A fresh install (or relocated DSH_HOME) has never run a turn, so the real home + // carries none of the linked entries. Linking a missing DIRECTORY left a dangling + // symlink whose every write dsh attempts THROUGH it failed with ENOENT on EVERY turn, + // and the lstat guard kept that dangling link forever. Directories are now created + // (0700, what dsh itself does on a first turn) before linking; a missing FILE + // (`settings.yaml`) is simply skipped. A stubbed HOME and CODEMAN_DATA_DIR keep both + // the fake real home and the generated dir off the developer's own dotfiles. + const sandbox = mkdtempSync(join(tmpdir(), 'dsh-fresh-home-')); + const realHome = join(sandbox, '.dsh'); + mkdirSync(realHome, { recursive: true }); // exists, but no sessions/, no profiles/, no settings.yaml + vi.stubEnv('HOME', sandbox); + vi.stubEnv('CODEMAN_DATA_DIR', join(sandbox, 'data')); + vi.stubEnv('DSH_HOME', ''); // force resolution down to the recipe's `realHomeFallback` + try { + const entry = getCli('deepseek'); + if (!entry) throw new Error('Missing DeepSeek entry'); + const endpoint = { id: 'local', label: 'Local', baseUrl: 'http://box:8080', apiKey: 'private-probe-key' }; + const applied = applyCustomModelInjection(entry, endpoint, 'chosen-model', 'deepseek-probe', 131072); + if (!applied?.configDir) throw new Error('Missing isolated profile'); + + expect(realpathSync(join(applied.configDir, 'sessions'))).toBe(join(realHome, 'sessions')); + expect(realpathSync(join(applied.configDir, 'profiles'))).toBe(join(realHome, 'profiles')); + expect(statSync(join(realHome, 'sessions')).mode & 0o777).toBe(0o700); + // The missing FILE target is skipped outright: not a dangling link either — lstat + // must find nothing, so a later apply (once the file exists for real) links it. + expect(existsSync(join(realHome, 'settings.yaml'))).toBe(false); + expect(readlinkSafe(join(applied.configDir, 'settings.yaml'))).toBeUndefined(); + + // Re-apply stays fine (boot recovery re-runs it every restart): idempotent... + const reapplied = applyCustomModelInjection(entry, endpoint, 'chosen-model', 'deepseek-probe', 131072); + expect(reapplied?.envOverrides.DSH_HOME).toBe(applied.configDir); + // ...and a target deleted AFTER linking keeps the existing link untouched (the + // lstat guard), rather than recreating it and throwing EEXIST. + rmSync(join(realHome, 'sessions'), { recursive: true, force: true }); + expect( + applyCustomModelInjection(entry, endpoint, 'chosen-model', 'deepseek-probe', 131072)?.envOverrides.DSH_HOME + ).toBe(applied.configDir); + expect(readlinkSync(join(applied.configDir, 'sessions'))).toBe(join(realHome, 'sessions')); + } finally { + rmSync(sandbox, { recursive: true, force: true }); + } + }); }); + +/** readlinkSync without the throw an absent link causes (ENOENT -> undefined). */ +function readlinkSafe(path: string): string | undefined { + try { + return readlinkSync(path); + } catch { + return undefined; + } +}