mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-05 06:59:42 +02:00
fix(sessions): keep a session's model through recovery, refuse it off claude
SessionState now carries the model a session launched with, and both recovery constructors (mux recovery and reboot restore) pass it back, so a recovered session relaunches on the same --model rather than the account default. A top-level `model` sent with any other CLI is refused, since those take their model in their own config object, and an empty string means no per-session model, as it does for modelOverride. CLAUDE.md now describes both routes for a Claude model. The tests pin which of `model` and `modelOverride` reaches the launch and which the case file, and that a model opening with a dash renders as --model's value. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
4123d229f4
commit
3df113fc54
@@ -137,7 +137,7 @@ 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 `<case>/.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 <configDir>/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 <level>` 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`
|
||||
- **Model choice flows via `settings.local.json`, NOT `--model` or 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 `<case>/.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
|
||||
- **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 `<case>/.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 <id>`, 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`).
|
||||
- **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`.
|
||||
|
||||
@@ -388,14 +388,14 @@ a directory that is not a case (body takes `workingDir`, `mode`, `name`, `effort
|
||||
differences that break copied code:
|
||||
|
||||
- The id is at **`.data.session.id`**, not quick-start's `.data.sessionId`
|
||||
(`session-routes.ts:878` returns `{ session: lightState }`).
|
||||
(the `POST /api/sessions` handler in `session-routes.ts` returns `{ session: lightState }`).
|
||||
- **It spawns no PTY.** The session exists with `pid:null` and nothing running, so
|
||||
`wait?until=exit` answers `exit` immediately. Follow it with
|
||||
`POST /api/v1/sessions/:id/interactive` (claude and the other agent CLIs) or
|
||||
`POST /api/v1/sessions/:id/shell` (shell mode) to actually start the worker.
|
||||
- Its capacity failure is **`OPERATION_FAILED` (422)**, not quick-start's
|
||||
`SESSION_BUSY` (409), from the same global-50 / per-user-25 caps
|
||||
(`session-routes.ts:648`).
|
||||
(`sessionCapacityMessage()` in `route-helpers.ts`).
|
||||
|
||||
⚠️ `POST .../interactive` accepts `{"clearBreaker":true}`, which resets the **PTY-exit
|
||||
circuit breaker**. That breaker exists to stop a session that keeps crashing on spawn
|
||||
|
||||
@@ -388,14 +388,14 @@ a directory that is not a case (body takes `workingDir`, `mode`, `name`, `effort
|
||||
differences that break copied code:
|
||||
|
||||
- The id is at **`.data.session.id`**, not quick-start's `.data.sessionId`
|
||||
(`session-routes.ts:878` returns `{ session: lightState }`).
|
||||
(the `POST /api/sessions` handler in `session-routes.ts` returns `{ session: lightState }`).
|
||||
- **It spawns no PTY.** The session exists with `pid:null` and nothing running, so
|
||||
`wait?until=exit` answers `exit` immediately. Follow it with
|
||||
`POST /api/v1/sessions/:id/interactive` (claude and the other agent CLIs) or
|
||||
`POST /api/v1/sessions/:id/shell` (shell mode) to actually start the worker.
|
||||
- Its capacity failure is **`OPERATION_FAILED` (422)**, not quick-start's
|
||||
`SESSION_BUSY` (409), from the same global-50 / per-user-25 caps
|
||||
(`session-routes.ts:648`).
|
||||
(`sessionCapacityMessage()` in `route-helpers.ts`).
|
||||
|
||||
⚠️ `POST .../interactive` accepts `{"clearBreaker":true}`, which resets the **PTY-exit
|
||||
circuit breaker**. That breaker exists to stop a session that keeps crashing on spawn
|
||||
|
||||
@@ -1827,6 +1827,7 @@ export class Session extends EventEmitter {
|
||||
ompConfig: this._ompConfig,
|
||||
resumeSessionId: this._resumeSessionId,
|
||||
effort: this._effort,
|
||||
model: this._model,
|
||||
customModel: this.customModel,
|
||||
// COD-118: runtime-only — surfaced so the frontend can require explicit user
|
||||
// intent before restarting a crash-looped session. Deliberately NOT restored
|
||||
|
||||
@@ -795,6 +795,13 @@ export interface SessionState {
|
||||
resumeSessionId?: string;
|
||||
/** Claude CLI effort level (soft default via --settings, switchable in-session via /effort) */
|
||||
effort?: EffortLevel;
|
||||
/**
|
||||
* 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.
|
||||
*/
|
||||
model?: string;
|
||||
/**
|
||||
* Custom Model Endpoint Profiles (docs/custom-model-endpoints-plan.md): the custom
|
||||
* OpenAI-compatible endpoint (local or cloud) this session's CLI is currently pointed
|
||||
|
||||
@@ -195,6 +195,7 @@ export function registerRebootRestoreRoutes(app: FastifyInstance, ctx: RebootRes
|
||||
(saved as { __envOverrides?: Record<string, string> }).__envOverrides
|
||||
),
|
||||
effort: saved.effort,
|
||||
model: saved.model,
|
||||
attachmentHistory:
|
||||
(saved as { __attachmentHistory?: SessionAttachmentHistoryItem[] }).__attachmentHistory ??
|
||||
saved.attachmentHistory,
|
||||
|
||||
@@ -888,6 +888,15 @@ export function registerSessionRoutes(
|
||||
if (capMsg) return createErrorResponse(ApiErrorCode.OPERATION_FAILED, capMsg);
|
||||
|
||||
const body = parseBody(CreateSessionSchema, req.body);
|
||||
// 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') {
|
||||
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'
|
||||
);
|
||||
}
|
||||
let workingDir = body.workingDir || process.cwd();
|
||||
let remote = undefined;
|
||||
|
||||
|
||||
+4
-1
@@ -529,12 +529,15 @@ export const CreateSessionSchema = z.object({
|
||||
/**
|
||||
* Claude model for THIS session only, passed as `claude --model <id>`; 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.
|
||||
* `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.
|
||||
*/
|
||||
model: z
|
||||
.string()
|
||||
.max(100)
|
||||
.regex(/^[a-zA-Z0-9._\-[\]]+$/)
|
||||
.or(z.literal(''))
|
||||
.optional(),
|
||||
openCodeConfig: OpenCodeConfigSchema,
|
||||
codexConfig: CodexConfigSchema,
|
||||
|
||||
@@ -3489,6 +3489,7 @@ export class WebServer extends EventEmitter {
|
||||
ompConfig: muxSession.mode === 'omp' ? savedState?.ompConfig : undefined,
|
||||
envOverrides: savedEnvOverrides,
|
||||
effort: savedState?.effort,
|
||||
model: savedState?.model,
|
||||
attachmentHistory: savedAttachmentHistory,
|
||||
// The pane's last Enter. Without it the response viewer would show
|
||||
// the launch conversation until the user types again, even though
|
||||
|
||||
@@ -54,6 +54,19 @@ 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.
|
||||
expect(claude({ model: 'claude-fable-5-1' })).toBe(
|
||||
'claude --dangerously-skip-permissions --session-id "0f9c2b14-1111-2222-3333-444455556666" --model "claude-fable-5-1"'
|
||||
);
|
||||
expect(claude({ model: '--dangerously-skip-permissions' })).toBe(
|
||||
'claude --dangerously-skip-permissions --session-id "0f9c2b14-1111-2222-3333-444455556666" ' +
|
||||
'--model "--dangerously-skip-permissions"'
|
||||
);
|
||||
});
|
||||
|
||||
it('resumes through a shell fallback to a fresh session', () => {
|
||||
// The ` || ` is emitted by the ENGINE, not by config — no registry field can hold shell
|
||||
// text. This pin is what proves the fallback chain still renders as one command line.
|
||||
|
||||
@@ -75,12 +75,30 @@ describe('POST /api/sessions model', () => {
|
||||
expect(await launchedModel({ mode: 'claude' })).toBe('sonnet');
|
||||
});
|
||||
|
||||
it('writes no model into the case directory', async () => {
|
||||
// The create still installs Codeman's workspace hooks into settings.local.json, so the
|
||||
// file exists; what must not be in it is a model that would outlive this session.
|
||||
await launchedModel({ mode: 'claude', model: 'opus' });
|
||||
const settings = await readFile(join(workingDir, '.claude', 'settings.local.json'), 'utf8').catch(() => '{}');
|
||||
expect(JSON.parse(settings)).not.toHaveProperty('model');
|
||||
it('launches on `model` while `modelOverride` alone reaches the case file', async () => {
|
||||
// Sent together, each lands where it belongs: the persistent default in the case's
|
||||
// settings.local.json, and this session's model on its launch line. A route that wrote
|
||||
// `model` to disk would put 'opus' in the file; one that ignored it would launch 'sonnet'.
|
||||
expect(await launchedModel({ mode: 'claude', model: 'opus', modelOverride: 'sonnet' })).toBe('opus');
|
||||
const settings = JSON.parse(await readFile(join(workingDir, '.claude', 'settings.local.json'), 'utf8'));
|
||||
expect(settings.model).toBe('sonnet');
|
||||
});
|
||||
|
||||
it('reads an empty model as no model, as modelOverride does', async () => {
|
||||
harness.ctx.getModelConfig.mockResolvedValue({ defaultModel: 'sonnet' });
|
||||
expect(await launchedModel({ mode: 'claude', model: '' })).toBe('sonnet');
|
||||
});
|
||||
|
||||
it('refuses a model for a CLI that takes its model in its own config object', async () => {
|
||||
const res = await harness.app.inject({
|
||||
method: 'POST',
|
||||
url: '/api/sessions',
|
||||
payload: { workingDir, mode: 'codex', model: 'gpt-5' },
|
||||
});
|
||||
const parsed = JSON.parse(res.body);
|
||||
expect(parsed.success).toBe(false);
|
||||
expect(parsed.errorCode).toBe('INVALID_INPUT');
|
||||
expect(harness.ctx.sessions.size).toBe(1); // only the session the mock context starts with
|
||||
});
|
||||
|
||||
it('rejects a model with characters the launch pattern refuses', async () => {
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
/**
|
||||
* @fileoverview A session's launch model survives recovery.
|
||||
*
|
||||
* `Session._model` is what becomes `claude --model <id>`: the caller's per-session `model`
|
||||
* from POST /api/sessions, or the app-wide default. It lives in memory, so it reaches a
|
||||
* relaunch after a Codeman restart or a reboot restore only if `toState()` persists it and
|
||||
* both recovery constructors hand it back. Without that, a recovered session silently
|
||||
* relaunches on the account default.
|
||||
*
|
||||
* `restoreMuxSessions()` (server.ts) cannot be reached under vitest, where
|
||||
* `reconcileSessions()` reports every pane alive, and the reboot-restore route rejects every
|
||||
* workspace before building a Session in its route tests. The two constructors are therefore
|
||||
* pinned by a source check, the same way `test/remote-wake.test.ts` pins its wiring, and the
|
||||
* round trip itself is driven through a real `Session` against the in-memory tmux layer.
|
||||
*/
|
||||
import { mkdirSync, readFileSync, rmSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import { Session } from '../src/session.js';
|
||||
import { TmuxManager } from '../src/tmux-manager.js';
|
||||
|
||||
const SRC = fileURLToPath(new URL('../src', import.meta.url));
|
||||
|
||||
describe('the launch model survives recovery', () => {
|
||||
const workingDir = join(homedir(), 'codeman-cases', 'session-model-recovery');
|
||||
const sessions: Session[] = [];
|
||||
|
||||
afterEach(() => {
|
||||
for (const s of sessions.splice(0)) s.stop();
|
||||
rmSync(workingDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('persists the model in the session state', () => {
|
||||
const session = new Session({ workingDir: '/tmp', mode: 'claude', model: 'claude-fable-5-1' });
|
||||
sessions.push(session);
|
||||
expect(session.toState().model).toBe('claude-fable-5-1');
|
||||
});
|
||||
|
||||
it('relaunches a session rebuilt from that state on the same model', async () => {
|
||||
mkdirSync(workingDir, { recursive: true });
|
||||
const original = new Session({ workingDir, mode: 'claude', model: 'claude-fable-5-1' });
|
||||
sessions.push(original);
|
||||
const state = original.toState();
|
||||
|
||||
// Rebuilt the way both recovery paths build one, from the persisted record.
|
||||
const mux = new TmuxManager();
|
||||
const createSession = vi.spyOn(mux, 'createSession');
|
||||
const rebuilt = new Session({
|
||||
id: state.id,
|
||||
workingDir,
|
||||
mode: state.mode,
|
||||
mux,
|
||||
useMux: true,
|
||||
model: state.model,
|
||||
});
|
||||
sessions.push(rebuilt);
|
||||
await rebuilt.startInteractive();
|
||||
|
||||
expect(createSession).toHaveBeenCalledWith(expect.objectContaining({ model: 'claude-fable-5-1' }));
|
||||
});
|
||||
|
||||
it('is handed back by both recovery constructors', () => {
|
||||
const server = readFileSync(join(SRC, 'web', 'server.ts'), 'utf-8');
|
||||
const reboot = readFileSync(join(SRC, 'web', 'routes', 'reboot-restore-routes.ts'), 'utf-8');
|
||||
expect(server).toMatch(/model:\s*savedState\?\.model,/);
|
||||
expect(reboot).toMatch(/model:\s*saved\.model,/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user