mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-05 23:19:43 +02:00
fix(sessions): scope the launch model to claude and pin it with the advisor (#514, #515, #530 landing)
Maintainer merge-time fixes for the three PRs that landed together on the session create / launch / persistence path. #514 findings (bot verdict merge-with-fixes): - minor, fixed: SessionState.model was published and persisted for every mode, so a codex/opencode cron session reported the app-wide Claude default it never ran on. toState() now emits it only where the new cliTakesSessionModel() holds (registry capability model.source === 'claude-settings-file', no CLI id branch). POST /api/sessions uses the same helper for its non-claude refusal, so refusal and publication cannot drift. Recovery then hands back undefined for other modes on its own. - nit, fixed: the `model` schema admitted a leading dash (and '.', '['). The first character must now be a letter or digit; still a subset of the registry's model-claude pattern, so nothing accepted is refused at launch. - nit, fixed (reject, the consistent choice): `model` with attachRemoteSession was silently dropped. Now a 400 INVALID_INPUT, as #514 does for non-claude CLIs and quick-start does for remote cases. advisorModel (#530) gets the same refusal there. effort and envOverrides keep their older silent ignore on that branch so no existing caller breaks. #515 finding (bot verdict merge, one nit): - nit, fixed: the types/session.ts @fileoverview described CodexConfig as (model, resumeSessionId); it now lists reasoningEffort, bypass, animations and renderMode too. Audit of the merged combination (not reviewed before): - The conflict resolutions in session.ts (toState), types/session.ts, reboot-restore-routes.ts, server.ts (restoreMuxSessions), CLAUDE.md and skills/codeman/reference/endpoints.md (+ plugin mirror) keep both sides correctly; nothing was lost or doubled. - A claude session with both `model` and `advisorModel` launches with `--model <id>` and ONE merged `--settings` JSON (ultracode + advisorModel, or advisorModel beside `--effort <level>`), on the tmux template (including the resume || new variant and with the statusLine exporter) and on the direct-PTY fallback. Both values (and effort) survive restoreMuxSessions onto a dead pane, a reboot restore into a fresh pane, and restartCli/dead-pane respawn via _buildRespawnPaneOptions. - quick-start and ralph-loop take no per-session `model` (matching #514's scope, POST /api/sessions only) and launch on the app-wide default, which toState now persists for claude, so recovery stays consistent. - No defect found in the combination beyond the findings above. Noted, not changed: advisorModel is still published for any mode a caller sends it with (launch-inert there; the UI and skill send it for claude only). Tests: test/session-model-recovery.test.ts pins the pair through both recovery shapes for effort ultracode/high/none, the recovery constructors' fields, the tmux-manager builder hop, and the codex/opencode/shell non-publication; test/advisor-model.test.ts pins the launch lines and a real direct-PTY Session's pty.spawn argv; the route test covers flag-shaped models, attach refusals and the published fields. Docs: SessionState.model docstring, the reboot-restore-registry header, the golden test comment and the CLAUDE.md model/advisor bullets. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
+108
-1
@@ -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<typeof import('node-pty')>();
|
||||
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 <id>` 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' });
|
||||
|
||||
@@ -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"'
|
||||
);
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<RespawnPaneOptions, 'sessionId' | 'model' | 'effort' | 'advisorModel'>): 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},`));
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user