From db5222c4fa277d8116bb6046195e77b924891e5f Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 6 Nov 2026 02:39:11 +0100 Subject: [PATCH] fix: keep a home value that is not a path off disk, and re-apply from the own home (release review) getCustomModelForPersist() trusted homeEnvVar to name a path, but the registry types it as any env name, so a clis.json override could write a session's own credential to state.json. Only an absolute path is written now. A re-apply resolved the CLI's real home from what the session's map held, which under a selection is the injected directory, and recovery resolved none at all. An entry the own home lacks (settings.yaml) was then linked from the server's home on an endpoint switch and on recovery. The route now reads the new Session.ownEnvValue(), and recovery reads the clamped record through the same bounded guard, under the bulk cap since it walks every saved session. Also corrects the comment that said only a removed directory gets its links again. Co-Authored-By: Claude Opus 5.5 --- src/session.ts | 19 ++++++++-- src/types/session.ts | 5 ++- src/web/routes/session-routes.ts | 12 +++--- src/web/server.ts | 27 +++++++++----- test/custom-model-recovery.test.ts | 39 +++++++++++++++++--- test/custom-model-session-home-guard.test.ts | 22 +++++++++++ test/mocks/mock-session.ts | 2 + test/routes/session-custom-model.test.ts | 23 +++++++++++- test/session-custom-model-restart.test.ts | 27 +++++++++++++- 9 files changed, 149 insertions(+), 27 deletions(-) diff --git a/src/session.ts b/src/session.ts index 7b1f1ff5..11327f5b 100644 --- a/src/session.ts +++ b/src/session.ts @@ -29,7 +29,7 @@ */ import { EventEmitter } from 'node:events'; -import { basename } from 'node:path'; +import { basename, isAbsolute } from 'node:path'; import { realpath } from 'node:fs/promises'; import { execSync, execFileSync } from 'node:child_process'; import { v4 as uuidv4 } from 'uuid'; @@ -1374,6 +1374,17 @@ export class Session extends EventEmitter { return value && value.trim() ? value.trim() : undefined; } + /** + * What the SESSION itself set for `key`, same single-key rule. `envOverrideValue()` answers + * with whatever the map holds, which under a selection is the injected value: a re-apply + * resolving the CLI's real home must read the session's own home, or none. + */ + ownEnvValue(key: string): string | undefined { + const selection = this._customModel; + const value = selection?.envKeys.includes(key) ? selection.ownEnv?.[key] : this._envOverrides?.[key]; + return value?.trim() || undefined; + } + /** Owning username in multi-user mode, else undefined. */ get owner(): string | undefined { return this._owner; @@ -2003,7 +2014,9 @@ export class Session extends EventEmitter { * The one value it does carry is the session's own home directory (`ownEnv`, narrowed to * `homeEnvVar`), a path like `configDir` beside it: the selection overwrote the tmux * `setenv` that held it, so without this a clear after a restart would lose it. Any other - * entry of `ownEnv` may be a credential and stays in memory. + * entry of `ownEnv` may be a credential and stays in memory, and so does a `homeEnvVar` + * value that is not an absolute path: the registry types that name as any env var, so a + * `clis.json` override can point it at a credential. */ getCustomModelForPersist(): CustomModelBookkeeping | undefined { if (!this._customModel) return undefined; @@ -2013,7 +2026,7 @@ export class Session extends EventEmitter { return { ...selection, envKeys: [...selection.envKeys], - ...(homeEnvVar && ownHome ? { ownEnv: { [homeEnvVar]: ownHome } } : {}), + ...(homeEnvVar && ownHome && isAbsolute(ownHome) ? { ownEnv: { [homeEnvVar]: ownHome } } : {}), }; } diff --git a/src/types/session.ts b/src/types/session.ts index 26e7a8c4..6fea1fbf 100644 --- a/src/types/session.ts +++ b/src/types/session.ts @@ -711,8 +711,9 @@ export interface CustomModelBookkeeping extends CustomModelSelection { /** * What the session itself had set under keys the selection replaced, put back when the * selection goes. In memory this can hold a credential (a session's own `GEMINI_API_KEY`), - * so `Session.getCustomModelForPersist()` writes `homeEnvVar`'s entry and nothing else: - * tmux keeps only the injected value, so state.json is the one copy left after a restart. + * so `Session.getCustomModelForPersist()` writes `homeEnvVar`'s entry, when it is an absolute + * path, and nothing else: tmux keeps only the injected value, so state.json is the one copy + * left after a restart. */ ownEnv?: Record; } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 0ef420a1..3f1d96a3 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -202,7 +202,7 @@ import { } from '../../docker-hosts.js'; import { resolveCaseRepository, resolveManagedWorktree } from '../../git-worktrees.js'; import { LRUMap } from '../../utils/lru-map.js'; -import { describeUnknownPath, probePathKind } from '../../utils/index.js'; +import { describeUnknownPath, probePathKind, type PathProbeOptions } from '../../utils/index.js'; import { findLatestOmpSessionId } from '../../utils/omp-session-resolver.js'; import { scanOmpSessionsHistory } from '../../omp-transcript.js'; import { scanCodexSessionsHistory, codexThreadBySessionId } from '../../codex-transcript.js'; @@ -244,18 +244,20 @@ function landedInGroup(layout: TabLayout | undefined, sessionId: string): boolea * Probe the session's own home with `probePathKind(..., { pastCap: true })` first: on * `'unknown'` (a dead mount that never answered) hand the apply a lookup that hides that * 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. + * path is never touched. Any other entry's lookup passes through untouched. Boot recovery + * passes `probeOptions` of `{}`: it walks every saved session, so it keeps the bulk cap. */ export async function guardSessionConfigDirHome( entry: Pick, - baseLookup: (envVar: string) => string | undefined + baseLookup: (envVar: string) => string | undefined, + probeOptions: PathProbeOptions = { pastCap: true } ): Promise<(envVar: string) => string | undefined> { const recipe = entry.capabilities.customModelInjection; if (recipe?.kind !== 'configDir' || !recipe.linkFromRealHome?.length) return baseLookup; const dirEnvVar = recipe.dirEnvVar; const supplied = baseLookup(dirEnvVar)?.trim(); if (!supplied || !isAbsolute(supplied)) return baseLookup; - if ((await probePathKind(supplied, { pastCap: true })) === 'unknown') { + if ((await probePathKind(supplied, probeOptions)) === 'unknown') { console.warn( `[custom-model] ${dirEnvVar} (${supplied}) did not answer; linking from the server's own home instead` ); @@ -1675,7 +1677,7 @@ export function registerSessionRoutes( session.id, contextLength, session.workingDir, - await guardSessionConfigDirHome(entry, (envVar) => session.envOverrideValue(envVar)) + await guardSessionConfigDirHome(entry, (envVar) => session.ownEnvValue(envVar)) ); if (!applied) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `${session.mode} has no known custom-model mechanism`); diff --git a/src/web/server.ts b/src/web/server.ts index 67001f87..408c682c 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -136,6 +136,7 @@ import { fetchWorkspaceOnce } from '../git-fetch.js'; import { GitFetchScheduler } from '../git-fetch-scheduler.js'; import { probePathKind } from '../utils/bounded-path-probe.js'; import { defaultDockerWorkspaces } from './routes/git-status-routes.js'; +import { guardSessionConfigDirHome } from './routes/session-routes.js'; import { composerIsEmpty, plainFrame } from '../session-submit-verifier.js'; import { detectUsageLimitPause } from '../usage-limit-patterns.js'; import { EditHistorySync } from './edit-history-sync.js'; @@ -1520,7 +1521,8 @@ export class WebServer extends EventEmitter { */ private async _rebuildCustomModelEnv( session: Session, - saved: CustomModelBookkeeping + saved: CustomModelBookkeeping, + ownEnv: Record = {} ): Promise { const entry = getCli(session.mode); if (!entry) return undefined; @@ -1532,17 +1534,21 @@ 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 first apply already - // linked this directory from the session's own home, and `linkRealHomeEntry()` keeps - // what is there. Only a directory someone removed gets its links again, from the - // SERVER's `DSH_HOME` or the recipe's `realHomeFallback` (~/.dsh). + // Boot recovery omits `workingDir` (no reason to re-answer a trust dialog that already + // fired once). The real-home lookup reads the record's own values, since the session's + // overrides hold what the previous run injected. `linkRealHomeEntry()` keeps what is + // there, so this only decides an entry that is missing, which an own home skips when it + // does not hold one. With no own home, or one that does not answer (probed under the + // bulk cap, this walks every saved session), a missing entry links from the SERVER's + // `DSH_HOME` or the recipe's `realHomeFallback` (~/.dsh). return applyCustomModelInjection( entry, endpoint, saved.modelId, session.id, - resolveCustomModelContextLength(endpoint, saved.modelId) + resolveCustomModelContextLength(endpoint, saved.modelId), + undefined, + await guardSessionConfigDirHome(entry, (envVar) => ownEnv[envVar], {}) ); } catch (err) { console.warn('[WebServer] Failed to rebuild custom-model env on recovery:', err); @@ -1565,7 +1571,7 @@ export class WebServer extends EventEmitter { */ private async _restoreCustomModel(session: Session, saved: CustomModelBookkeeping): Promise { const ownEnv = (await clampEnvOverridesForOwner(session.owner, saved.ownEnv)) ?? {}; - const applied = await this._rebuildCustomModelEnv(session, saved); + const applied = await this._rebuildCustomModelEnv(session, saved, ownEnv); if (!applied) { // Endpoint gone or CLI unregistered: restore the bookkeeping exactly as saved, which // is what a later clear needs. @@ -1596,8 +1602,9 @@ 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. + // __customModel keeps the selection's bookkeeping (injected env KEYS, config dir, launch + // model, the session's own home path; never an injected value or any other own value) + // so recovery can restore and later clear it. const customModel = session.getCustomModelForPersist(); const state = { ...base, diff --git a/test/custom-model-recovery.test.ts b/test/custom-model-recovery.test.ts index 0fc64910..636f6496 100644 --- a/test/custom-model-recovery.test.ts +++ b/test/custom-model-recovery.test.ts @@ -7,17 +7,18 @@ * home: restoring the saved record unchanged left `DSH_HOME` on a generated config dir * that clearing could never delete. * - * Real-home resolution takes no session lookup on this path: the first apply's links are - * still in the generated directory, and one someone removed links from the server's - * `DSH_HOME` or the recipe's `realHomeFallback` (~/.dsh), see `_rebuildCustomModelEnv`'s - * comment. A session's own home comes back on clear from `ownEnv`, the last block here. + * Real-home resolution reads the record's `ownEnv` on this path, never the session's + * overrides: the first apply's links are still in the generated directory, and an entry + * that is missing links from the own home, or with none from the server's `DSH_HOME` or + * the recipe's `realHomeFallback` (~/.dsh), see `_rebuildCustomModelEnv`'s comment. The + * own home itself comes back on clear from `ownEnv`, the last block here. * * `_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, rmSync } from 'node:fs'; +import { existsSync, mkdirSync, readlinkSync, rmSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -137,6 +138,34 @@ describe('_restoreCustomModel: bookkeeping rebuilt from the current recipe', () expect(session.envOverrideValue('DSH_HOME')).toBe(OWN_HOME); }); + it('never fills an entry the own home lacks from the server home', async () => { + // The first apply skipped `settings.yaml`, since the own home has none. Recovery finds + // that gap and, with no lookup, used to fill it with the server's settings. + const serverHome = join(homedir(), 'codeman-recovery-server-dsh'); + const ownHome = join(homedir(), 'codeman-recovery-own-dsh'); + mkdirSync(join(ownHome, 'profiles'), { recursive: true }); + mkdirSync(serverHome, { recursive: true }); + writeFileSync(join(serverHome, 'settings.yaml'), 'server: true\n'); + vi.stubEnv('DSH_HOME', serverHome); + await writeCustomModelHosts(getDataDir(), [endpoint()]); + const session = new Session({ workingDir: '/tmp', mode: 'deepseek' }); + configs.push(session.id); + + await restoreCustomModel(session, { + endpointId: 'ep', + modelId: 'qwen3', + envKeys: ['DSH_HOME', 'CODEMAN_CUSTOM_MODEL_API_KEY'], + homeEnvVar: 'DSH_HOME', + ownEnv: { DSH_HOME: ownHome }, + }); + + const configDir = customModelConfigDir(session.id); + expect(readlinkSync(join(configDir, 'profiles'))).toBe(join(ownHome, 'profiles')); + expect(existsSync(join(configDir, 'settings.yaml'))).toBe(false); + rmSync(ownHome, { recursive: true, force: true }); + rmSync(serverHome, { recursive: true, force: true }); + }); + it('keeps it when the endpoint is gone', async () => { await writeCustomModelHosts(getDataDir(), []); const session = new Session({ workingDir: '/tmp', mode: 'deepseek' }); diff --git a/test/custom-model-session-home-guard.test.ts b/test/custom-model-session-home-guard.test.ts index 08f36fc7..18460bbd 100644 --- a/test/custom-model-session-home-guard.test.ts +++ b/test/custom-model-session-home-guard.test.ts @@ -14,8 +14,14 @@ vi.mock('../src/utils/bounded-path-probe.js', async (importOriginal) => { }); import { getCli } from '../src/config/cli-registry/registry.js'; +import { getDataDir } from '../src/config/instance.js'; +import { writeCustomModelHosts } from '../src/custom-model-hosts.js'; +import { customModelConfigDir, removeConfigDir } from '../src/custom-model-injection-apply.js'; +import { Session } from '../src/session.js'; import { probePathKind } from '../src/utils/bounded-path-probe.js'; import { guardSessionConfigDirHome } from '../src/web/routes/session-routes.js'; +import { WebServer } from '../src/web/server.js'; +import type { CustomModelBookkeeping } from '../src/types/session.js'; const probe = vi.mocked(probePathKind); const deepseek = () => getCli('deepseek')!; @@ -52,6 +58,22 @@ describe('guardSessionConfigDirHome', () => { expect(warn).not.toHaveBeenCalled(); }); + it('boot recovery probes the own home under the stall cap: it is a bulk pass, not one request', async () => { + probe.mockResolvedValue('unknown'); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + await writeCustomModelHosts(getDataDir(), [{ id: 'ep', label: 'Ep', baseUrl: 'http://box:8080' }]); + const session = new Session({ workingDir: '/tmp', mode: 'deepseek' }); + const saved: CustomModelBookkeeping = { endpointId: 'ep', modelId: 'q', envKeys: [] }; + const proto = WebServer.prototype as unknown as { + _rebuildCustomModelEnv: (session: Session, saved: CustomModelBookkeeping, ownEnv: object) => Promise; + }; + + await proto._rebuildCustomModelEnv(session, saved, { DSH_HOME: '/mnt/dead/dsh' }); + + expect(probe).toHaveBeenCalledWith('/mnt/dead/dsh', {}); + removeConfigDir(customModelConfigDir(session.id)); + }); + it('never probes a relative home: the apply refuses to join one', async () => { const base = env({ DSH_HOME: 'relative/dsh' }); diff --git a/test/mocks/mock-session.ts b/test/mocks/mock-session.ts index cd04cebf..fa59db6c 100644 --- a/test/mocks/mock-session.ts +++ b/test/mocks/mock-session.ts @@ -464,6 +464,8 @@ export class MockSession extends EventEmitter { /** Mirrors Session.envOverrideValue; no override map here, so the custom-model * real-home lookup defaults to "session says nothing" (overridable per test). */ envOverrideValue = vi.fn((_key: string): string | undefined => undefined); + /** Mirrors Session.ownEnvValue, same default. */ + ownEnvValue = vi.fn((_key: string): string | undefined => undefined); getCustomModelForPersist = vi.fn(() => this._mockCustomModel); /** Mirrors Session.noteReportedModel: the statusline route records the CLI's model. */ reportedModel: { model: string; source: 'statusline' | 'screen' } | null = null; diff --git a/test/routes/session-custom-model.test.ts b/test/routes/session-custom-model.test.ts index d28277f0..964c1ae5 100644 --- a/test/routes/session-custom-model.test.ts +++ b/test/routes/session-custom-model.test.ts @@ -9,7 +9,7 @@ import { createRouteTestHarness } from './_route-test-utils.js'; import { createMockSession } from '../mocks/index.js'; import { getDataDir } from '../../src/config/instance.js'; import { writeCustomModelHosts, type CustomModelHost } from '../../src/custom-model-hosts.js'; -import { existsSync, readFileSync, rmSync, statSync } from 'node:fs'; +import { existsSync, readFileSync, readlinkSync, rmSync, statSync } from 'node:fs'; import { join } from 'node:path'; import { webviewFetch } from '../../src/web/webview-egress.js'; @@ -97,6 +97,27 @@ describe('POST /api/sessions/:id/custom-model', () => { expect(trustFile.customApiKeyResponses.approved).toEqual(['k']); }); + it("links a switch from the session's own home, never the injected one the map now holds", async () => { + const { app, ctx } = await setup(); + const session = ctx.sessions.get('test-session-1')!; + session.mode = 'deepseek'; + const isolatedDir = join(getDataDir(), 'custom-model-configs', 'test-session-1'); + const ownHome = join(getDataDir(), 'own-dsh-home'); + session.envOverrideValue.mockImplementation(() => isolatedDir); + session.ownEnvValue.mockImplementation((key: string) => (key === 'DSH_HOME' ? ownHome : undefined)); + + const res = await app.inject({ + method: 'POST', + url: '/api/sessions/test-session-1/custom-model', + payload: { endpointId: 'ep1', modelId: 'qwen3' }, + }); + + expect(res.statusCode).toBe(200); + expect(readlinkSync(join(isolatedDir, 'sessions'))).toBe(join(ownHome, 'sessions')); + rmSync(isolatedDir, { recursive: true, force: true }); + rmSync(ownHome, { recursive: true, force: true }); + }); + it('clears back to the native default', async () => { const { app, ctx } = await setup(); const session = ctx.sessions.get('test-session-1')!; diff --git a/test/session-custom-model-restart.test.ts b/test/session-custom-model-restart.test.ts index 581b6dbb..9828cbb6 100644 --- a/test/session-custom-model-restart.test.ts +++ b/test/session-custom-model-restart.test.ts @@ -1,7 +1,7 @@ /** * @fileoverview Custom Model Endpoint Profiles, the Session half of applying and * clearing a selection against a live pane (`Session.setCustomModel()` + - * `Session.restartCli()`), pinned against the three ways the first cut broke a + * `Session.restartCli()`), pinned against the four ways the first cut broke a * working session: * * 1. Clearing did not clear. The injected vars reach the CLI via `tmux setenv`, @@ -347,6 +347,31 @@ describe('a value the session set itself comes back with the selection gone', () expect(session.envOverrideValue('GEMINI_API_KEY')).toBe('own-secret'); }); + it('keeps a home variable holding anything but an absolute path off disk', () => { + // The registry types `configDirVar`/`dirEnvVar` as plain env names, so a clis.json + // override can name a credential as the home variable. Only a path may reach disk. + const { session } = liveSession('gemini', { envOverrides: { GEMINI_API_KEY: 'own-secret' } }); + session.setCustomModel( + { endpointId: 'ep', modelId: 'q', envKeys: ['GEMINI_API_KEY'], homeEnvVar: 'GEMINI_API_KEY' }, + { GEMINI_API_KEY: 'endpoint-key' } + ); + + expect(JSON.stringify(session.getCustomModelForPersist())).not.toContain('own-secret'); + session.setCustomModel(undefined); + expect(session.envOverrideValue('GEMINI_API_KEY')).toBe('own-secret'); + }); + + it('reads the own value while a selection holds the key, never the injected one', () => { + const { session } = liveSession('deepseek', { envOverrides: { DSH_HOME: ' /home/u/my-dsh ' } }); + expect(session.ownEnvValue('DSH_HOME')).toBe('/home/u/my-dsh'); + + session.setCustomModel(DSH_SELECTION, DSH_ENV); + expect(session.ownEnvValue('DSH_HOME')).toBe('/home/u/my-dsh'); + // An injected key the session never set has no own value, whatever the map holds now. + expect(session.envOverrideValue('CODEMAN_CUSTOM_MODEL_API_KEY')).toBe('k'); + expect(session.ownEnvValue('CODEMAN_CUSTOM_MODEL_API_KEY')).toBeUndefined(); + }); + it('persists nothing extra for a recipe with no home variable', () => { const { session } = liveSession('gemini', { envOverrides: { GEMINI_API_KEY: 'own-secret' } }); session.setCustomModel(