From 8c317d84c80cefe5d0a14fb618b9550c7ff05884 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Wed, 4 Nov 2026 16:55:44 +0100 Subject: [PATCH] fix: apply the review items for #755 at merge Say that settings.yaml is shared only until the session's first settings write (dsh saves by rename, which replaces the link), warn when a session's own DSH_HOME does not answer and the server's home is linked instead, test guardSessionConfigDirHome, and document the linkFromRealHome exception and what a custom-model dsh session does not carry over. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- docs/custom-model-endpoints.md | 7 +- docs/wiki/Custom-Model-Endpoints.md | 2 +- src/config/cli-registry/stock.ts | 4 +- src/config/cli-registry/types.ts | 2 + src/web/routes/session-routes.ts | 3 +- test/custom-model-session-home-guard.test.ts | 68 ++++++++++++++++++++ 7 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 test/custom-model-session-home-guard.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index e47929e9..6fac0d31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -183,7 +183,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Case rename** (`POST /api/cases/:name/rename`, `src/case-rename.ts`; `docs/wiki/Core-Concepts.md#case`): a case under the cases dir is its folder name and always moves; a linked case renames its alias and moves its real folder only with `renameFolder`; remote and Docker cases are refused. ⚠️ A folder that sessions have open is the hazard: live sessions at or under it make the route answer `409 requiresStop` with their names, and only `stopSessions: true` ends them (via `ctx.cleanupSession`: closed like a tab close, uploads included, nothing is restarted); the free-target pre-check runs before that and refuses a dangling symlink too (`lstat`, as `moveCaseFolder` does), so no session is ended for a move that cannot happen. `withCaseLock` holds the old and new paths for the whole operation (`isCasePathLocked`; the `Session` constructor throws for a working directory inside a held path, so nothing is constructed mid-move). ⚠️ A create awaits the owner's layout lock and a settings read between `new Session` and the server's map, where the rename looks for open sessions, so the constructor also records the session (`noteCaseSessionStarting`) until `registerSessionWithLayout()` or `stop()` settles it (30 s TTL for one that never registers), and a folder move refuses `409` with `renameCode: "BUSY"` while one is recorded at or under the folder (`isCaseSessionStarting`, checked in the same tick that takes the lock); an alias-only rename is not refused. A create whose path was validated before the move but whose `Session` is constructed after it is not covered (the plain check-then-use race of any create path), `moveCaseFolder` refuses an existing target itself (POSIX `rename` would silently replace an EMPTY directory) and retries `EBUSY`/`EPERM`/`EACCES` before answering `423 LOCKED`. If the registries (`linked-cases.json`, `caseOrder`, `lastUsedCase`) fail after the move, `settings.json` and the folder are put back (a failed move-back says the folder is now at the new path), and an error after `stopSessions` carries `stoppedSessions`. Claude Code keys history by path (`claudeProjectDirName`: every non-alphanumeric becomes `-`), so `/projects/` is renamed too, or merged entry by entry, never overwriting, into one already at the new key (case delete never removes history, so an earlier case of the new name leaves one); other CLIs' history, Docker bind mounts, `git worktree` links, cron jobs' `workingDir`, Claude history of subfolder sessions and Claude Code's per-project entry in `~/.claude.json` (local-scope `mcpServers`, `allowedTools`, trust; writing another CLI's config is the opt-in MCP sync's job) are NOT migrated. The history scanner prefers the decoded project key over a transcript's `cwd` that no longer exists, so Resume follows a rename. Only an admin's rename touches the server-wide `settings.json` (`caseOrder`, `lastUsedCase`); the owner's default session group is carried across with `TabLayoutService`. `settings.json` is written before `linked-cases.json`, and both are restored if the registry write fails. -**Custom Model Endpoint Profiles** (opt-in, `customModelEndpointsEnabled`, SYNCED, default OFF; `docs/custom-model-endpoints.md`): points a session at a user-configured OpenAI-compatible endpoint (store `custom-model-hosts.ts`, `~/.codeman/custom-model-hosts.json`, 0600). `authStyle` is `bearer` or `api-key`, never both headers (hangs the server). The per-CLI redirect is registry data, `capabilities.customModelInjection` (`env` / `configContentEnv` / `configDir` / `unsupported`), computed by the pure `custom-model-injection.ts`; ⚠️ `configDir` writes an isolated per-session config, NEVER the user's real `~/.codex`/`~/.pi`/`~/.omp`/grok config. ⚠️ Claude applies via `Session.restartCli()` (a relaunch in the existing pane, so the conversation is pinned as `resumeSessionId`), the rest one-shot at launch; retired env keys must also be `setenv -u`'d (`_pendingEnvUnsets`), since tmux env survives `respawn-pane`. ⚠️ Remote/Docker sessions are refused (400). ⚠️ Persist only KEYS (`__customModel`), never values (they carry the API key). ⚠️ Every redirectable var must be in that CLI's `privilegedEnvKeys`, and `ANTHROPIC_*` stays out of claude's `allowedPrefixes`. ⚠️ The Run-menu picker builds entries from `window.__codemanCustomModelClis` (escaped via `escapeScriptJson()`), never a hardcoded CLI id list, and launches through `run()` via a temporary `_runMode` swap, never `setRunMode()`. → [architecture-invariants#custom-model-endpoint-profiles](docs/architecture-invariants.md#custom-model-endpoint-profiles) +**Custom Model Endpoint Profiles** (opt-in, `customModelEndpointsEnabled`, SYNCED, default OFF; `docs/custom-model-endpoints.md`): points a session at a user-configured OpenAI-compatible endpoint (store `custom-model-hosts.ts`, `~/.codeman/custom-model-hosts.json`, 0600). `authStyle` is `bearer` or `api-key`, never both headers (hangs the server). The per-CLI redirect is registry data, `capabilities.customModelInjection` (`env` / `configContentEnv` / `configDir` / `unsupported`), computed by the pure `custom-model-injection.ts`; ⚠️ `configDir` writes an isolated per-session config, NEVER the user's real `~/.codex`/`~/.pi`/`~/.omp`/grok config; the one exception is a recipe's `linkFromRealHome` (deepseek's `profiles`, `sessions`, `settings.yaml`), which symlinks those named entries back out of the real home so they survive the redirect. ⚠️ Claude applies via `Session.restartCli()` (a relaunch in the existing pane, so the conversation is pinned as `resumeSessionId`), the rest one-shot at launch; retired env keys must also be `setenv -u`'d (`_pendingEnvUnsets`), since tmux env survives `respawn-pane`. ⚠️ Remote/Docker sessions are refused (400). ⚠️ Persist only KEYS (`__customModel`), never values (they carry the API key). ⚠️ Every redirectable var must be in that CLI's `privilegedEnvKeys`, and `ANTHROPIC_*` stays out of claude's `allowedPrefixes`. ⚠️ The Run-menu picker builds entries from `window.__codemanCustomModelClis` (escaped via `escapeScriptJson()`), never a hardcoded CLI id list, and launches through `run()` via a temporary `_runMode` swap, never `setRunMode()`. → [architecture-invariants#custom-model-endpoint-profiles](docs/architecture-invariants.md#custom-model-endpoint-profiles) ⚠️ **llama-swap endpoints** (one model at a time): the apply routes check `GET /running` and return `requiresConfirmation` before evicting a model another live session uses; `confirmedSwap` and `confirmedContext` are SEPARATE flags and must stay so. Claude alone gets a context floor (`CLAUDE_MIN_SAFE_CONTEXT_TOKENS`); context is parsed from `/running`'s `cmd`, never trusted from `/props`. The floor also divides that context by the model's parallel slot count (`modelSlotCounts`: `--parallel`/`-np` from `cmd`, else `/props` `total_slots`; `perSlotContext()`), and `/props` only ever RAISES that warning: it supplies the context only when `cmd` states none and the count only when `cmd` has no `--parallel`, never a larger pool. `/props` is probed once per launch command (it refreshes llama-swap's last-use time, so a `ttl` would never expire), and a `/props` context beside an explicit `--parallel N` is already per slot, so it is not divided again. Backend log lines come from llama-swap's `/api/events` `upstream` source, never `/logs`. → [architecture-invariants#custom-model-endpoint-profiles](docs/architecture-invariants.md#custom-model-endpoint-profiles) diff --git a/docs/custom-model-endpoints.md b/docs/custom-model-endpoints.md index 48084b94..4ba60bec 100644 --- a/docs/custom-model-endpoints.md +++ b/docs/custom-model-endpoints.md @@ -620,7 +620,12 @@ for '' not found. Defaulting to fallback metadata...` on launch — `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` is + `settings.yaml`. The last is shared only until the session writes + it: dsh saves settings with a temp file and a rename + (`writeFileAtomic` in `@deepseek-ai/dsh-atomic-write`), which replaces + the symlink, so the first `/settings` change inside such a session + never reaches `~/.dsh/settings.yaml` and is deleted with the session's + directory on clear or close. Everything else in the generated `DSH_HOME` is isolated: `storages/` (dsh-TUI's persisted `/model` choice and workspace state), `.agent-presets` and `.env`. Two consequences of the generated home-level file follow: the user's own home-level diff --git a/docs/wiki/Custom-Model-Endpoints.md b/docs/wiki/Custom-Model-Endpoints.md index 548b28f1..89b51068 100644 --- a/docs/wiki/Custom-Model-Endpoints.md +++ b/docs/wiki/Custom-Model-Endpoints.md @@ -197,7 +197,7 @@ read is treated as one slot, never guessed. | **Claude Code, opencode, Pi, Grok, OMP** | Verified end-to-end against a real local server. | | **Codex** | Config is correct, and plain chat can work against a server that speaks the Responses API — but a real tool-call attempt comes back as inert text instead of running, so it's still not usable for real coding work. | | **Gemini** | Fails with an auth error gemini-cli raises once redirected. Unresolved; don't rely on it yet. | -| **DeepSeek** | Verified end-to-end against a real `dsh` install (dsh 0.1.1-rc.2, dsh-TUI 0.10.0-beta.1): the original 404 is root-caused and fixed, and the redirect is now a generated per-session patch file whose prompt reaches the endpoint as `POST /v1/chat/completions`. | +| **DeepSeek** | Verified end-to-end against a real `dsh` install (dsh 0.1.1-rc.2, dsh-TUI 0.10.0-beta.1): the original 404 is root-caused and fixed, and the redirect is now a generated per-session patch file whose prompt reaches the endpoint as `POST /v1/chat/completions`. Such a session does not carry over your home-level `cordis.patch.yml`, your own `llm-pi-ai` providers, or a profile-level `effort`/`preset`. | | **Antigravity** | No known custom-endpoint mechanism at all. Not offered. | Which harnesses show up in the Run-menu picker is read live off Codeman's own CLI registry, diff --git a/src/config/cli-registry/stock.ts b/src/config/cli-registry/stock.ts index ecaf7c34..7e95d997 100644 --- a/src/config/cli-registry/stock.ts +++ b/src/config/cli-registry/stock.ts @@ -1562,7 +1562,9 @@ const DEEPSEEK: CliEntry = { // dsh keeps state in its home that the redirect must NOT swallow: `sessions` is the // session-log root (deleting it would take the user's transcripts with it on clear/ // close), `profiles` are the installed bundles the patch composes over, and - // `settings.yaml` must keep applying. The generated home-level `cordis.patch.yml` + // `settings.yaml` must keep applying (read through the link only until the session's + // first settings write: dsh saves by rename, which replaces the link with a local file + // that goes away with the session). 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 diff --git a/src/config/cli-registry/types.ts b/src/config/cli-registry/types.ts index 01475d49..e558d37a 100644 --- a/src/config/cli-registry/types.ts +++ b/src/config/cli-registry/types.ts @@ -893,6 +893,8 @@ export interface CliCapabilities { * home carries state that must survive the redirect (deepseek's `sessions` transcript * root and `profiles` bundles, plus `settings.yaml`, which would otherwise stop * applying and — for `sessions` — get deleted by `removeConfigDir` on clear/close). + * A CLI that saves a linked file by rename replaces the link: dsh does, so the first + * in-session settings write turns `settings.yaml` into a local file. * Never include `fileName` itself: the generated file owns that name in the isolated * directory, and a link there would write THROUGH into the user's real file. * Consumed generically by custom-model-injection-apply.ts. diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 7545a01e..a189f003 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -246,7 +246,7 @@ function landedInGroup(layout: TabLayout | undefined, sessionId: string): boolea * one env var, so linking falls through to the server's own environment and the unresponsive * path is never touched. Any other entry's lookup passes through untouched. */ -async function guardSessionConfigDirHome( +export async function guardSessionConfigDirHome( entry: Pick, baseLookup: (envVar: string) => string | undefined ): Promise<(envVar: string) => string | undefined> { @@ -256,6 +256,7 @@ async function guardSessionConfigDirHome( const supplied = baseLookup(dirEnvVar)?.trim(); if (!supplied || !isAbsolute(supplied)) return baseLookup; if ((await probePathKind(supplied, { pastCap: true })) === 'unknown') { + console.warn(`[custom-model] ${dirEnvVar} (${supplied}) did not answer; linking from the server's own home instead`); return (envVar: string) => (envVar === dirEnvVar ? undefined : baseLookup(envVar)); } return baseLookup; diff --git a/test/custom-model-session-home-guard.test.ts b/test/custom-model-session-home-guard.test.ts new file mode 100644 index 00000000..08f36fc7 --- /dev/null +++ b/test/custom-model-session-home-guard.test.ts @@ -0,0 +1,68 @@ +/** + * @fileoverview `guardSessionConfigDirHome` probes a session-supplied configDir home before + * the apply runs synchronous fs calls on it (the #516 rule): a home that does not answer is + * hidden from the apply, so it links from the server's own home instead and says so. + * + * The probe is mocked: a mount that never answers is the case under test, and a real one + * cannot be made here. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../src/utils/bounded-path-probe.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, probePathKind: vi.fn() }; +}); + +import { getCli } from '../src/config/cli-registry/registry.js'; +import { probePathKind } from '../src/utils/bounded-path-probe.js'; +import { guardSessionConfigDirHome } from '../src/web/routes/session-routes.js'; + +const probe = vi.mocked(probePathKind); +const deepseek = () => getCli('deepseek')!; +const env = + (values: Record) => + (name: string): string | undefined => + values[name]; + +describe('guardSessionConfigDirHome', () => { + afterEach(() => { + probe.mockReset(); + vi.restoreAllMocks(); + }); + + it('hides only the home when it does not answer, and warns that the server home is used', async () => { + probe.mockResolvedValue('unknown'); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + const lookup = await guardSessionConfigDirHome(deepseek(), env({ DSH_HOME: '/mnt/dead/dsh', OTHER: 'kept' })); + + expect(lookup('DSH_HOME')).toBeUndefined(); + expect(lookup('OTHER')).toBe('kept'); + expect(probe).toHaveBeenCalledWith('/mnt/dead/dsh', { pastCap: true }); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('/mnt/dead/dsh'); + }); + + it('passes a home that answers straight through', async () => { + probe.mockResolvedValue('directory'); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const base = env({ DSH_HOME: '/home/u/.dsh' }); + + expect(await guardSessionConfigDirHome(deepseek(), base)).toBe(base); + expect(warn).not.toHaveBeenCalled(); + }); + + it('never probes a relative home: the apply refuses to join one', async () => { + const base = env({ DSH_HOME: 'relative/dsh' }); + + expect(await guardSessionConfigDirHome(deepseek(), base)).toBe(base); + expect(probe).not.toHaveBeenCalled(); + }); + + it('never probes for a CLI with no linkFromRealHome recipe', async () => { + const base = env({ CODEX_HOME: '/mnt/dead/codex' }); + + expect(await guardSessionConfigDirHome(getCli('codex')!, base)).toBe(base); + expect(probe).not.toHaveBeenCalled(); + }); +});