diff --git a/CLAUDE.md b/CLAUDE.md index d42a87be..c17edcb9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -137,8 +137,8 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph - **Global regex `lastIndex`** — Shared `g`-flag patterns in loops must reset `lastIndex = 0` first, or use the `execPattern()` helper in `utils/regex-patterns.ts` (resets automatically) - **`envOverrides` flow `CLAUDE_CODE_*` / `OPENCODE_*` / `CODEX_*` / `GEMINI_*` / `GOOGLE_*` / `ANTIGRAVITY_*` / `PI_*` / `GROK_*` / `XAI_*` / `DSH_*` / `DEEPSEEK_*` env vars, plus exact-key `CLAUDE_CONFIG_DIR`** — Set via `POST /api/sessions { envOverrides }`, stored on `Session._envOverrides`, exported by `tmux-manager.buildEnvExports()` at spawn time, persisted in `SessionState.envOverrides`. **Do NOT** write these to `/.claude/settings.local.json` — that's the old path and creates UI/disk drift. (`GOOGLE_*` is the deliberately-broad Vertex-AI namespace for Gemini — see Multi-CLI prefix discipline.) `CLAUDE_CONFIG_DIR` (#255, exact match via `ALLOWED_ENV_KEYS` in `schemas.ts`) points a session at a separate Claude account/config dir for per-client subscriptions; it persists to state.json (a path, not a secret; losing it on restart would silently switch accounts). ⚠️ A relocated config dir writes transcripts outside `~/.claude/projects`, so the response viewer, subagent windows, ultracode panel and Read My Mind capture go blind for that session unless the user symlinks `projects` back into the shared tree (`ln -s ~/.claude/projects /projects`). ⚠️ It is also one of claude's `privilegedEnvKeys` (Custom Model Endpoint Profiles, since it can redirect a session's traffic same as any other injected var), so in multi-user mode setting it via `envOverrides` is admin-only, and a non-granted owner's already-persisted `CLAUDE_CONFIG_DIR` is stripped on reboot-restore — silently returning that session to the default Claude account rather than the one it was pointed at (see `session-env-clamp.ts`). → [architecture-invariants#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir](docs/architecture-invariants.md#per-session-env-overrides-exact-key-allowlist-and-claude_config_dir) - **Effort is NOT an env var** — never carry effort as `CLAUDE_CODE_EFFORT_LEVEL`: the env var hard-locks effort and blocks in-session `/effort` switching (incl. ultracode). It flows as the dedicated `effort` payload field → `Session._effort` → `claude --effort ` for regular levels incl. `max` (the settings `effortLevel` key is `enum(["low","medium","high","xhigh"]).catch(undefined)` — `max` gets SILENTLY dropped there), or `claude --settings '{"ultracode":true}'` for ultracode (rejected by `--effort`). Both are soft defaults the user can override anytime. Legacy env-var entries are auto-migrated by the Session constructor and unset from tmux sessions in `applyEnvOverrides()`. See `buildEffortCliArgs()` in `session-cli-builder.ts`, tests in `test/effort-injection.test.ts` -- **The advisor rides `--settings`, NEVER the `--advisor` flag**: Claude Code's advisor tool (a stronger model consulted at decision points, code.claude.com/docs/en/advisor) flows as the `advisorModel` payload field → `Session._advisorModel` (persisted, so respawn and reboot restore keep it) → the `advisorModel` key in the launch's ONE `--settings` JSON, merged with ultracode and the statusLine exporter by `buildAdvisorSettings()` (`session-cli-builder.ts`). ⚠️ The flag EXITS at launch on any pairing the CLI refuses (`claude --advisor haiku` exits 1, so does Fable before its usage-credit consent), which would leave a dead pane on every respawn; the settings key degrades to "no advisor" instead. ⚠️ `isAdvisorModel()` (fable/opus/sonnet aliases or full ids, no haiku) is also the injection guard for the single-quoted argument. Soft default: `/advisor` still switches it in-session. App Settings key `claudeAdvisorModel` (SYNCED, `''` = leave it to the CLI). Remote/docker quick-start refuses it, like `effort`. Tests: `test/advisor-model.test.ts` -- **Model choice: a persistent default in `settings.local.json`, a per-session `--model`, never env** — the App Settings **Claude Model** picker (`claudeModel` in `settings.json`) is read by `session-ui.js` at session create (wins over the legacy 1M-Opus toggles `opusContext1m`/`opusContext1mEnabled`), sent as the `modelOverride` payload field, and `updateCaseModel()` (`hooks-config.ts`) writes/deletes the `model` key in `/.claude/settings.local.json`. This is the intended exception to the envOverrides rule above: model legitimately lives in `settings.local.json` (a soft default — in-session `/model` still works); env vars do not. A caller that wants one session on a model without touching the case sends `model` on `POST /api/sessions` instead: it goes out as `claude --model `, writes nothing, wins over the app-wide default, and is persisted as `SessionState.model` so both recovery paths relaunch on it (`test/routes/session-routes-claude-model.test.ts`, `test/session-model-recovery.test.ts`). +- **The advisor rides `--settings`, NEVER the `--advisor` flag**: Claude Code's advisor tool (a stronger model consulted at decision points, code.claude.com/docs/en/advisor) flows as the `advisorModel` payload field → `Session._advisorModel` (persisted, so respawn and reboot restore keep it) → the `advisorModel` key in the launch's ONE `--settings` JSON, merged with ultracode and the statusLine exporter by `buildAdvisorSettings()` (`session-cli-builder.ts`). ⚠️ The flag EXITS at launch on any pairing the CLI refuses (`claude --advisor haiku` exits 1, so does Fable before its usage-credit consent), which would leave a dead pane on every respawn; the settings key degrades to "no advisor" instead. ⚠️ `isAdvisorModel()` (fable/opus/sonnet aliases or full ids, no haiku) is also the injection guard for the single-quoted argument. Soft default: `/advisor` still switches it in-session. App Settings key `claudeAdvisorModel` (SYNCED, `''` = leave it to the CLI). Remote/docker quick-start refuses it, like `effort`, and so does a remote attach on `POST /api/sessions`. Tests: `test/advisor-model.test.ts` +- **Model choice: a persistent default in `settings.local.json`, a per-session `--model`, never env** — the App Settings **Claude Model** picker (`claudeModel` in `settings.json`) is read by `session-ui.js` at session create (wins over the legacy 1M-Opus toggles `opusContext1m`/`opusContext1mEnabled`), sent as the `modelOverride` payload field, and `updateCaseModel()` (`hooks-config.ts`) writes/deletes the `model` key in `/.claude/settings.local.json`. This is the intended exception to the envOverrides rule above: model legitimately lives in `settings.local.json` (a soft default — in-session `/model` still works); env vars do not. A caller that wants one session on a model without touching the case sends `model` on `POST /api/sessions` instead: it goes out as `claude --model `, writes nothing, wins over the app-wide default, and is persisted as `SessionState.model` so both recovery paths relaunch on it (`test/routes/session-routes-claude-model.test.ts`, `test/session-model-recovery.test.ts`). ⚠️ Claude only, via the `model.source` capability (`cliTakesSessionModel()`), never a mode check: refused for other CLIs and on a remote attach, and published by `toState()` for claude alone (cron hands its Claude default to every CLI with a model). - **Multi-CLI prefix discipline** — env-var prefix is CLI-specific (`CLAUDE_CODE_*` vs `OPENCODE_*` vs `CODEX_*` vs `GEMINI_*` vs `ANTIGRAVITY_*` vs `PI_*` vs `GROK_*` vs `DSH_*`) and the `ALLOWED_ENV_PREFIXES` allowlist in `schemas.ts` enforces this; non-prefix exceptions are exact keys in `ALLOWED_ENV_KEYS` (currently only `CLAUDE_CONFIG_DIR`), never a widened prefix. Gemini additionally allowlists the **broad `GOOGLE_*`** namespace (intentional: Vertex AI auth needs `GOOGLE_CLOUD_PROJECT`/`GOOGLE_APPLICATION_CREDENTIALS`/`GOOGLE_GENAI_USE_VERTEXAI`; it is the loosest allowlist entry, affecting only the user's own spawned CLI), and Grok allowlists **`XAI_*`** for the same vendor-namespace reason (`XAI_API_KEY` is grok's documented auth var). When adding a setting, decide which CLI(s) it applies to and gate the env export accordingly. Never blanket-forward all prefixes. ⚠️ Pi is the case that proves the rule: its ~34 provider keys (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `HF_TOKEN`, …) share NO prefix, and the allowlist is one GLOBAL list applied by a refine with no mode context, so admitting them for pi would widen it for every mode at once — they stay out, and pi users authenticate via `/login` or the server process's own env. ⚠️ DeepSeek repeats pi's lesson exactly: a dsh `settings.yaml` can nominate ANY env var as a provider credential (`apiKeyEnv`), so only the vendor namespaces `DSH_*` (launcher inputs incl. `DSH_PERMISSION_MODE`) and `DEEPSEEK_*` (`DEEPSEEK_API_KEY`/`DEEPSEEK_BASE_URL`) are admitted; foreign provider keys authenticate from dsh's own files or the server env. Resolver design pattern: `docs/opencode-integration.md`, `docs/pi-integration.md`, `docs/grok-integration.md`, `docs/deepseek-integration.md` - **Zod `.optional()` rejects `null`** — accepts `undefined` only. When the frontend builds a request body with `JSON.stringify`, an explicit `null` field is preserved on the wire and fails validation with `INVALID_INPUT`. Convert `null` → `undefined` before stringifying (e.g. `field: value ?? undefined`), or declare the schema `.nullish()`. This has caused real shipped bugs twice - **Local-echo overlay stays on screen**: the overlay lays its wrapped lines out DOWNWARD from the prompt row, and the text has not reached the PTY yet, so the CLI never learns the prompt is long and nothing scrolls to make room. With the keyboard up only a handful of rows are visible, so a long prompt used to run off the bottom and the user typed blind. The block now grows UPWARD once it would pass the last visible row (optional `totalRows` in `RenderParams`; the line divs are opaque, so they cover transcript above), and a prompt taller than the viewport keeps its TAIL. ⚠️ Separately, `_shrinkPaddingToFit()` (mobile-handlers.js) must never shrink `main`'s padding-bottom below the MEASURED height of the fixed bars: on phones the toolbar and accessory bar are `position: fixed`, so that padding is the only thing reserving room for them, and taking it pulled the terminal's bottom row behind them. Tests: `packages/xterm-zerolag-input/test/overlay-renderer.test.ts`, `test/mobile-keyboard-bottom-padding.test.ts`. diff --git a/src/session.ts b/src/session.ts index 5835b1ce..7e92df81 100644 --- a/src/session.ts +++ b/src/session.ts @@ -213,6 +213,21 @@ export function isExternalCliMode(mode: SessionMode): boolean { return getCli(mode)?.capabilities.external ?? true; } +/** + * Does this CLI take the top-level session `model` (claude's per-session `--model`)? + * + * Read off the registry's model-source capability: only a `claude-settings-file` CLI + * (claude) launches on that field. Every other CLI takes its model in its own config object + * (`codexConfig.model` and so on), so for them the field is inert, and cron hands the + * app-wide default (always a Claude id) to any CLI that has a model at all. `toState()` + * publishes and persists the field only where this holds, so a codex cron session never + * reports a Claude model it did not run on, and `POST /api/sessions` refuses a `model` for + * any CLI where it does not. + */ +export function cliTakesSessionModel(mode: SessionMode): boolean { + return getCli(mode)?.capabilities.model.source === 'claude-settings-file'; +} + /** Display name for a run mode. Falls back to the raw id for an unregistered one. */ function getModeLabel(mode: SessionMode): string { return getCli(mode)?.label ?? mode; @@ -1838,7 +1853,9 @@ export class Session extends EventEmitter { ompConfig: this._ompConfig, resumeSessionId: this._resumeSessionId, effort: this._effort, - model: this._model, + // Claude only: for any other CLI `_model` is inert (its model lives in its own config + // object) and may be the app-wide Claude default cron handed it. + model: cliTakesSessionModel(this.mode) ? this._model : undefined, advisorModel: this._advisorModel, customModel: this.customModel, // COD-118: runtime-only — surfaced so the frontend can require explicit user diff --git a/src/types/session.ts b/src/types/session.ts index c4570950..08dbf4be 100644 --- a/src/types/session.ts +++ b/src/types/session.ts @@ -12,7 +12,7 @@ * - ClaudeMode — CLI permission mode ('dangerously-skip-permissions' | 'auto' | 'normal' | 'allowedTools') * - SessionColor — visual differentiation color * - OpenCodeConfig — OpenCode-specific settings (model, autoAllowTools, continueSession) - * - CodexConfig — Codex (OpenAI CLI)-specific settings (model, resumeSessionId) + * - CodexConfig — Codex (OpenAI CLI)-specific settings (model, reasoningEffort, resumeSessionId, bypass, animations, renderMode) * - GeminiConfig — Gemini CLI-specific settings (model, approvalMode, resumeSession) * - AntigravityConfig — Antigravity CLI (agy) settings (model, dangerouslySkipPermissions, resumeConversationId) * - PiConfig — Pi CLI (pi.dev) settings (model, provider, thinking, resume/continue, project trust) @@ -833,7 +833,9 @@ export interface SessionState { * The model the session was LAUNCHED with (`--model`): the caller's per-session `model`, or * the app-wide default when there was none. Persisted so a recovered session relaunches on * the same model rather than whatever the default is by then. Not `cliModel`, which is what - * the CLI's banner reports. + * the CLI's banner reports. Claude sessions only (`cliTakesSessionModel()`): every other CLI + * keeps its model in its own config object (`codexConfig.model` and so on), and this is + * absent for them. */ model?: string; /** diff --git a/src/web/reboot-restore-registry.ts b/src/web/reboot-restore-registry.ts index 93b7822d..bc38d833 100644 --- a/src/web/reboot-restore-registry.ts +++ b/src/web/reboot-restore-registry.ts @@ -18,7 +18,8 @@ * session record involved. A dropped plan therefore returns the user to * resuming by hand, one at a time, which is where they are without this * feature. What the plan held that a transcript does not is the owner, the - * name, the env overrides, the effort, the advisor model and the lineage. + * name, the env overrides, the effort, the model, the advisor model and the + * lineage. * - Module-level singleton in the style of `web/approval-inbox.ts`: no `Session` * import and no IO, which keeps it unit-testable and cycle-free. * - Spending is take-then-build: `take()` removes entries synchronously, before diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 9de70f37..bb4bdbe0 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -30,7 +30,13 @@ import { type OmpConfig, type RemoteHost, } from '../../types.js'; -import { Session, isAltScreenStripMode, isExternalCliMode, isMuxAltScreenOnlyStripMode } from '../../session.js'; +import { + Session, + cliTakesSessionModel, + isAltScreenStripMode, + isExternalCliMode, + isMuxAltScreenOnlyStripMode, +} from '../../session.js'; import type { PaneCaptureOptions } from '../../mux-interface.js'; import { SseEvent } from '../sse-events.js'; import { webviewCapabilities } from '../../webview-capabilities.js'; @@ -892,12 +898,22 @@ export function registerSessionRoutes( // The top-level `model` is Claude's per-session `--model`. Every other CLI takes its model // in its own config object (`codexConfig.model` and so on), so a `model` here would be // dropped without a word; refuse it before anything is written for the session. - if (body.model && getCli(body.mode ?? 'claude')?.capabilities.model.source !== 'claude-settings-file') { + if (body.model && !cliTakesSessionModel(body.mode ?? 'claude')) { return createErrorResponse( ApiErrorCode.INVALID_INPUT, 'model applies to claude sessions only; other CLIs take their model in their own config object, such as codexConfig.model' ); } + // An attach launches nothing (the remote agent is already running), so a launch model + // or advisor would be dropped the same way, so both are refused, as they have been since + // they were added. The older launch fields (effort, envOverrides) predate this and keep + // their silent ignore here, since refusing them now would break existing callers. + if (body.attachRemoteSession && (body.model || body.advisorModel)) { + return createErrorResponse( + ApiErrorCode.INVALID_INPUT, + 'model and advisorModel shape a new launch, and attachRemoteSession launches nothing; leave them out when attaching' + ); + } let workingDir = body.workingDir || process.cwd(); let remote = undefined; diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 99c934a8..48845fd0 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -548,15 +548,17 @@ export const CreateSessionSchema = z.object({ modelOverride: z.string().max(50).optional(), /** * Claude model for THIS session only, passed as `claude --model `; nothing is written to - * disk. Wins over the app-wide default model. Same character set as the registry's - * `model-claude` pattern, so a value accepted here is never rejected at launch. An empty - * string means no per-session model, as it does for `modelOverride`. Claude only: the route - * refuses it for any other CLI. + * disk. Wins over the app-wide default model. A subset of the registry's `model-claude` + * pattern, so a value accepted here is never rejected at launch. The first character must be + * a letter or digit: the value lands in argv, and no model id opens with `-`, so a + * flag-shaped value is refused here rather than left to the launch quoting. An empty string + * means no per-session model, as it does for `modelOverride`. Claude only: the route refuses + * it for any other CLI and on a remote attach. */ model: z .string() .max(100) - .regex(/^[a-zA-Z0-9._\-[\]]+$/) + .regex(/^[a-zA-Z0-9][a-zA-Z0-9._\-[\]]*$/) .or(z.literal('')) .optional(), openCodeConfig: OpenCodeConfigSchema, diff --git a/test/advisor-model.test.ts b/test/advisor-model.test.ts index 88a4b8af..39f94b05 100644 --- a/test/advisor-model.test.ts +++ b/test/advisor-model.test.ts @@ -12,7 +12,7 @@ * assertions see exactly what a spawned pane would. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; import { execFileSync } from 'node:child_process'; import { buildAdvisorSettings, buildInteractiveArgs } from '../src/session-cli-builder.js'; import { buildSpawnCommand } from '../src/tmux-manager.js'; @@ -27,6 +27,23 @@ import { const EXPORTER_CMD = 'curl -sfk -X POST "$CODEMAN_API_URL/api/status-telemetry" --data @- 2>/dev/null || true'; +/** + * What the direct-PTY fallback hands `pty.spawn`. The file and argv are recorded, then a + * harmless stand-in runs instead: the real `claude` must never start from a test, and the + * stand-in is a real process so `Session.stop()` has a real pid to signal. + */ +const ptySpawns = vi.hoisted(() => [] as Array<{ file: string; args: string[] }>); +vi.mock('node-pty', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + spawn: (file: string, args: string[] | string, options: import('node-pty').IPtyForkOptions) => { + ptySpawns.push({ file, args: Array.isArray(args) ? args : [args] }); + return real.spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], options); + }, + }; +}); + function extractSettingsJson(cmd: string): unknown { const idx = cmd.indexOf('--settings '); expect(idx).toBeGreaterThan(-1); @@ -164,6 +181,96 @@ describe('buildInteractiveArgs advisorModel (direct-PTY fallback)', () => { }); }); +/** + * #514 (per-session `model` → `--model`) and #530 landed together. A claude session carrying + * both must launch with `--model ` AND the advisor folded into the one `--settings` JSON, + * whatever the effort, on the tmux template and on the direct-PTY fallback alike. Recovery of + * the same pair is pinned in test/session-model-recovery.test.ts. + */ +describe('a per-session model together with an advisor', () => { + const MODEL = 'claude-fable-5-1'; + const cases = [ + ['ultracode', { ultracode: true, advisorModel: 'opus' }], + ['high', { advisorModel: 'opus' }], + [undefined, { advisorModel: 'opus' }], + ] as const; + + it.each(cases)('tmux template carries --model and the merged --settings, effort %s', (effort, settings) => { + for (const resumeSessionId of [undefined, '11111111-2222-3333-4444-555555555555']) { + const cmd = buildSpawnCommand({ + mode: 'claude', + sessionId: 'sid-1', + model: MODEL, + effort, + advisorModel: 'opus', + resumeSessionId, + claudeCliVersion: null, + }); + // The resume variant renders `resume || new`, so the model appears once per branch. + expect(cmd).toContain(`--model "${MODEL}"`); + expect(cmd).not.toContain('--advisor '); + const settingsFlags = cmd.match(/--settings /g) ?? []; + expect(settingsFlags.length).toBe(resumeSessionId ? 2 : 1); + expect(extractSettingsJson(cmd)).toEqual(settings); + if (effort === 'high') expect(cmd).toContain("--effort 'high'"); + } + }); + + it('keeps the statusLine exporter in the same object beside the model', () => { + const cmd = buildSpawnCommand({ + mode: 'claude', + sessionId: 'sid-1', + model: MODEL, + effort: 'ultracode', + advisorModel: 'fable', + statusLineCommand: EXPORTER_CMD, + claudeCliVersion: null, + }); + expect(cmd).toContain(`--model "${MODEL}"`); + expect(cmd.match(/--settings /g)).toHaveLength(1); + expect(extractSettingsJson(cmd)).toEqual({ + ultracode: true, + advisorModel: 'fable', + statusLine: { type: 'command', command: EXPORTER_CMD }, + }); + }); + + it.each(cases)('direct-PTY args carry --model and the merged --settings, effort %s', (effort, settings) => { + const args = buildInteractiveArgs('sid', 'normal', MODEL, undefined, effort, undefined, null, 'opus'); + expect(args[args.indexOf('--model') + 1]).toBe(MODEL); + expect(args.filter((a) => a === '--settings')).toHaveLength(1); + expect(JSON.parse(args[args.indexOf('--settings') + 1])).toEqual(settings); + if (effort === 'high') expect(args).toEqual(expect.arrayContaining(['--effort', 'high'])); + }); + + describe('a real Session on the direct-PTY fallback', () => { + const live: Session[] = []; + afterEach(async () => { + for (const s of live.splice(0)) await s.stop(); + ptySpawns.length = 0; + }); + + it.each(cases)('hands pty.spawn both, effort %s', async (effort, settings) => { + const session = new Session({ + workingDir: '/tmp', + mode: 'claude', + useMux: false, + model: MODEL, + advisorModel: 'opus', + effort, + }); + live.push(session); + await session.startInteractive(); + + expect(ptySpawns).toHaveLength(1); + const { args } = ptySpawns[0]; + expect(args[args.indexOf('--model') + 1]).toBe(MODEL); + expect(args.filter((a) => a === '--settings')).toHaveLength(1); + expect(JSON.parse(args[args.indexOf('--settings') + 1])).toEqual(settings); + }); + }); +}); + describe('Session advisorModel', () => { it('stores a valid advisor and persists it through toState()', () => { const session = new Session({ workingDir: '/tmp', advisorModel: 'opus' }); diff --git a/test/cli-registry-spawn-golden.test.ts b/test/cli-registry-spawn-golden.test.ts index 28960b68..77a8cf41 100644 --- a/test/cli-registry-spawn-golden.test.ts +++ b/test/cli-registry-spawn-golden.test.ts @@ -56,9 +56,10 @@ describe('claude', () => { }); it('renders a model as the quoted value of --model, even one that opens with a dash', () => { - // POST /api/sessions admits a leading '-' in `model`. It still lands as the option's - // value: quoted here, and Claude's option parser takes the word after `--model` as its - // value whatever it starts with, so it can never become a flag of its own. + // POST /api/sessions refuses a leading '-' in `model`, but the registry's `model-claude` + // pattern still admits one, so the builder must stay safe on its own: the value lands + // quoted, and Claude's option parser takes the word after `--model` as its value whatever + // it starts with, so it can never become a flag of its own. expect(claude({ model: 'claude-fable-5-1' })).toBe( 'claude --dangerously-skip-permissions --session-id "0f9c2b14-1111-2222-3333-444455556666" --model "claude-fable-5-1"' ); diff --git a/test/routes/session-routes-claude-model.test.ts b/test/routes/session-routes-claude-model.test.ts index 658b8692..d825f35c 100644 --- a/test/routes/session-routes-claude-model.test.ts +++ b/test/routes/session-routes-claude-model.test.ts @@ -109,4 +109,54 @@ describe('POST /api/sessions model', () => { }); expect(res.statusCode).toBe(400); }); + + it('rejects a flag-shaped model, since the value lands in argv', async () => { + for (const model of ['--dangerously-skip-permissions', '-p', '.hidden', '[1m]']) { + const res = await harness.app.inject({ + method: 'POST', + url: '/api/sessions', + payload: { workingDir, mode: 'claude', model }, + }); + expect(res.statusCode, model).toBe(400); + } + expect(harness.ctx.sessions.size).toBe(1); // only the session the mock context starts with + }); + + it('still accepts real model ids, aliases and the [1m] suffix', async () => { + for (const model of ['claude-fable-5-1', 'opus', 'opus[1m]', 'claude-opus-5-5[1m]']) { + expect(await launchedModel({ mode: 'claude', model })).toBe(model); + } + }); + + it.each([ + ['model', { model: 'opus' }], + ['advisorModel', { advisorModel: 'opus' }], + ])('refuses %s on a remote attach, which launches nothing', async (_field, extra) => { + // Refused before the host is looked up, so no remote host needs to exist. + const res = await harness.app.inject({ + method: 'POST', + url: '/api/sessions', + payload: { + mode: 'claude', + attachRemoteSession: { hostId: 'h1', remoteSessionName: 'codeman-ssh-abc123' }, + ...extra, + }, + }); + const parsed = JSON.parse(res.body); + expect(parsed.success).toBe(false); + expect(parsed.errorCode).toBe('INVALID_INPUT'); + expect(harness.ctx.sessions.size).toBe(1); + }); + + it('publishes the launch model on the created claude session', async () => { + const res = await harness.app.inject({ + method: 'POST', + url: '/api/sessions', + payload: { workingDir, mode: 'claude', model: 'claude-fable-5-1', advisorModel: 'opus' }, + }); + const parsed = JSON.parse(res.body); + const session = parsed.data?.session ?? parsed.session; + expect(session.model).toBe('claude-fable-5-1'); + expect(session.advisorModel).toBe('opus'); + }); }); diff --git a/test/session-model-recovery.test.ts b/test/session-model-recovery.test.ts index 54a09d70..6c90dc3e 100644 --- a/test/session-model-recovery.test.ts +++ b/test/session-model-recovery.test.ts @@ -19,8 +19,11 @@ import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; import { Session } from '../src/session.js'; -import { TmuxManager } from '../src/tmux-manager.js'; +import { TmuxManager, buildSpawnCommand } from '../src/tmux-manager.js'; +import type { MuxSession, RespawnPaneOptions, TerminalMultiplexer } from '../src/mux-interface.js'; +import type { EffortLevel } from '../src/types.js'; const SRC = fileURLToPath(new URL('../src', import.meta.url)); @@ -68,4 +71,152 @@ describe('the launch model survives recovery', () => { expect(server).toMatch(/model:\s*savedState\?\.model,/); expect(reboot).toMatch(/model:\s*saved\.model,/); }); + + it('is neither published nor persisted for a CLI that keeps its model in its own config', () => { + // Cron hands the app-wide default (a Claude id) to every CLI that has a model at all. + // codex never launches on the top-level field, so publishing it would report a model the + // session never ran on, and recovery would carry that wrong value forward. + for (const mode of ['codex', 'opencode', 'shell'] as const) { + const session = new Session({ workingDir: '/tmp', mode, model: 'claude-fable-5-1' }); + sessions.push(session); + expect(session.toState().model, mode).toBeUndefined(); + } + }); +}); + +/** + * #514 (per-session `model` → `--model`) and #530 (`advisorModel` → the launch's ONE + * `--settings` JSON) landed together and touch the same launch and recovery code. A claude + * session carrying both, with or without ultracode (whose blob shares that `--settings` + * object), must relaunch with both on every recovery path. + */ +describe('a launch model and an advisor survive recovery together', () => { + const MODEL = 'claude-fable-5-1'; + const ADVISOR = 'opus'; + const sessions: Session[] = []; + + afterEach(async () => { + for (const s of sessions.splice(0)) await s.stop(); + }); + + /** The settings JSON exactly as a shell would hand it to claude. */ + function settingsOf(cmd: string): unknown { + expect(cmd.match(/--settings /g)).toHaveLength(1); + const tail = cmd.slice(cmd.indexOf('--settings ')); + return JSON.parse(execFileSync('bash', ['-c', `set -- ${tail}; printf '%s' "$2"`]).toString()); + } + + /** Render what tmux-manager hands buildSpawnCommand for these options. */ + function launchLine(options: Pick): string { + return buildSpawnCommand({ + mode: 'claude', + sessionId: options.sessionId, + model: options.model, + effort: options.effort, + advisorModel: options.advisorModel, + claudeCliVersion: null, + }); + } + + function expectBoth(cmd: string, effort: EffortLevel | undefined): void { + expect(cmd).toContain(`--model "${MODEL}"`); + expect(cmd).not.toContain('--advisor '); + expect(settingsOf(cmd)).toEqual( + effort === 'ultracode' ? { ultracode: true, advisorModel: ADVISOR } : { advisorModel: ADVISOR } + ); + if (effort && effort !== 'ultracode') expect(cmd).toContain(`--effort '${effort}'`); + } + + function persistedRecord(effort: EffortLevel | undefined) { + const original = new Session({ workingDir: '/tmp', mode: 'claude', model: MODEL, advisorModel: ADVISOR, effort }); + sessions.push(original); + const state = original.toState(); + expect(state.model).toBe(MODEL); + expect(state.advisorModel).toBe(ADVISOR); + return state; + } + + it.each([['ultracode'], ['high'], [undefined]] as const)( + 'reboot restore (fresh pane) relaunches on both, effort %s', + async (effort) => { + const state = persistedRecord(effort); + // The reboot-restore constructor: no muxSession, so startInteractive() creates a pane. + const mux = new TmuxManager(); + const createSession = vi.spyOn(mux, 'createSession'); + const rebuilt = new Session({ + id: state.id, + workingDir: '/tmp', + mode: state.mode, + mux, + useMux: true, + effort: state.effort, + model: state.model, + advisorModel: state.advisorModel, + }); + sessions.push(rebuilt); + await rebuilt.startInteractive(); + + expect(createSession).toHaveBeenCalledTimes(1); + const options = createSession.mock.calls[0][0]; + expect(options).toEqual(expect.objectContaining({ model: MODEL, advisorModel: ADVISOR, effort })); + expectBoth(launchLine(options), effort); + } + ); + + it.each([['ultracode'], ['high'], [undefined]] as const)( + 'restoreMuxSessions onto a dead pane respawns on both, effort %s', + async (effort) => { + const state = persistedRecord(effort); + const respawns: RespawnPaneOptions[] = []; + const mux = { + isAvailable: () => true, + muxSessionExists: () => true, + isPaneDead: () => true, + setAttached: () => {}, + respawnPane: async (options: RespawnPaneOptions) => { + respawns.push(options); + return 4242; + }, + } as unknown as TerminalMultiplexer; + // The restoreMuxSessions() constructor: an existing muxSession, whose pane is dead, so + // startInteractive() takes the dead-pane respawn. + const rebuilt = new Session({ + id: state.id, + workingDir: '/tmp', + mode: state.mode, + mux, + useMux: true, + muxSession: { muxName: 'codeman-aaaa', sessionId: state.id } as unknown as MuxSession, + effort: state.effort, + model: state.model, + advisorModel: state.advisorModel, + }); + sessions.push(rebuilt); + await rebuilt.startInteractive(); + + expect(respawns).toHaveLength(1); + expect(respawns[0]).toEqual(expect.objectContaining({ model: MODEL, advisorModel: ADVISOR, effort })); + expectBoth(launchLine(respawns[0]), effort); + } + ); + + it('both recovery constructors hand back model, advisorModel and effort', () => { + const server = readFileSync(join(SRC, 'web', 'server.ts'), 'utf-8'); + const reboot = readFileSync(join(SRC, 'web', 'routes', 'reboot-restore-routes.ts'), 'utf-8'); + for (const field of ['effort', 'model', 'advisorModel']) { + expect(server).toMatch(new RegExp(`\\b${field}:\\s*savedState\\?\\.${field},`)); + expect(reboot).toMatch(new RegExp(`\\b${field}:\\s*saved\\.${field},`)); + } + }); + + it('tmux-manager forwards model, effort and advisorModel to both launch builders', () => { + // createSession() and respawnPane() each build the pane command. The tests above stop at + // the options a Session hands them, so this pins the last hop to the builder. + const tmux = readFileSync(join(SRC, 'tmux-manager.ts'), 'utf-8'); + const calls = [...tmux.matchAll(/buildSpawnCommand\(\{([^}]*)\}\)/g)].map((m) => m[1]); + expect(calls).toHaveLength(2); + for (const call of calls) { + for (const field of ['model', 'effort', 'advisorModel']) expect(call).toMatch(new RegExp(`\\b${field},`)); + } + }); });