mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 22:49:41 +02:00
fix(remote): address review feedback on omp/claude respawn continuity
- Remote omp command now renders through buildSpawnCommandFromRegistry (the mode-agnostic engine local/docker spawns use) instead of the buildOmpCommand() the CLI-registry refactor deleted. - Session._pinOmpRespawnId()/_maybeCaptureOmpSessionId() now skip host-local ~/.omp resolution entirely for a remote session and fall back to --continue: that resolver only ever reads THIS host's filesystem, which is meaningless (and could wrongly alias an unrelated local conversation) for a conversation that lives on the remote host. - Remote-claude launch now honors an explicit resumeSessionId distinct from sessionId (mirrors claudeDockerPaneCommand's shape), and validates sessionId the same way that sibling does before interpolating it into the remote shell command. - Add the still-missing header-cwd half of the trailing-slash test, and document respawn/reattach continuation + auto-reconnect-vs- clean-exit in docs/remote-sessions.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
88243e9ffa
commit
797f0d387c
@@ -25,7 +25,7 @@ import { join } from 'node:path';
|
||||
import { afterEach, describe, expect, it } from 'vitest';
|
||||
import { Session } from '../src/session.js';
|
||||
import { TmuxManager } from '../src/tmux-manager.js';
|
||||
import type { MuxSession } from '../src/types.js';
|
||||
import type { MuxSession, SessionRemote } from '../src/types.js';
|
||||
|
||||
describe('OMP: fresh session vs. reattach must not share resumeSessionId resolution', () => {
|
||||
const workingDir = join(homedir(), 'codeman-cases', 'resume-test');
|
||||
@@ -126,4 +126,48 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut
|
||||
expect(session.toState().ompConfig?.resumeSessionId).toBe('real-omp-uuid');
|
||||
expect(session.claudeSessionId).toBe('real-omp-uuid');
|
||||
});
|
||||
|
||||
it("a remote session never resolves --resume from this host's local ~/.omp, even when a same-named local session file exists", () => {
|
||||
// Seed a LOCAL session file whose directory mangle happens to match this
|
||||
// remote session's remotePath. If _pinOmpRespawnId() ever fell through to
|
||||
// resolveAndClaimOmpSessionId() for a remote session, it would wrongly
|
||||
// claim/pin this unrelated local conversation's id onto the remote respawn.
|
||||
seedOmpSessionFile('wrong-local-conversation-id');
|
||||
|
||||
const remote: SessionRemote = {
|
||||
hostId: 'remote-box',
|
||||
label: 'remote-box',
|
||||
host: 'remote-box',
|
||||
username: 'someone',
|
||||
remotePath: workingDir,
|
||||
owned: true,
|
||||
};
|
||||
|
||||
const muxSession: MuxSession = {
|
||||
sessionId: 'placeholder',
|
||||
muxName: 'codeman-deadbeef',
|
||||
pid: 1,
|
||||
createdAt: Date.now(),
|
||||
workingDir,
|
||||
mode: 'omp',
|
||||
attached: false,
|
||||
};
|
||||
|
||||
const session = new Session({
|
||||
workingDir,
|
||||
mode: 'omp',
|
||||
mux: new TmuxManager(),
|
||||
useMux: true,
|
||||
muxSession,
|
||||
remote,
|
||||
});
|
||||
sessions.push(session);
|
||||
|
||||
(session as unknown as { _pinOmpRespawnId(): void })._pinOmpRespawnId();
|
||||
|
||||
const state = session.toState();
|
||||
expect(state.ompConfig?.resumeSessionId).toBeUndefined();
|
||||
expect(state.ompConfig?.continueSession).toBe(true);
|
||||
expect(session.claudeSessionId).toBe(session.id);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -20,7 +20,11 @@ import { mkdirSync, rmSync, utimesSync, writeFileSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { afterEach, describe, expect, it } from 'vitest';
|
||||
import { findLatestOmpSessionId, mangleOmpWorkingDir } from '../src/utils/omp-session-resolver.js';
|
||||
import {
|
||||
findLatestOmpSessionId,
|
||||
mangleOmpWorkingDir,
|
||||
resolveAndClaimOmpSessionId,
|
||||
} from '../src/utils/omp-session-resolver.js';
|
||||
import { resolveOmpConfigForCreate } from '../src/web/routes/session-routes.js';
|
||||
|
||||
describe('mangleOmpWorkingDir', () => {
|
||||
@@ -78,6 +82,33 @@ describe('findLatestOmpSessionId', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveAndClaimOmpSessionId: header-cwd trailing-slash normalization', () => {
|
||||
// Sibling of the directory-mangle trailing-slash regression above, but for
|
||||
// the OTHER half of the same fix: resolveAndClaimOmpSessionId additionally
|
||||
// verifies each candidate file's own header `cwd` against workingDir (the
|
||||
// mangle is lossy, so the filename-derived id alone isn't enough — see the
|
||||
// function's doc comment). A remote case's workingDir carries a trailing
|
||||
// slash (e.g. `/home/user/dotfiles/`) but omp's header `cwd` never does;
|
||||
// without stripTrailingSlash() on BOTH sides of that comparison, a real
|
||||
// on-disk session would be found by directory but rejected by the cwd
|
||||
// check, silently degrading pinning to the ambiguous `--continue`.
|
||||
const workingDirNoSlash = join(homedir(), 'dotfiles');
|
||||
const workingDirWithSlash = `${workingDirNoSlash}/`;
|
||||
const sessionDir = join(homedir(), '.omp', 'agent', 'sessions', '-dotfiles');
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(join(homedir(), '.omp'), { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('matches a header cwd with no trailing slash against a workingDir that has one', () => {
|
||||
mkdirSync(sessionDir, { recursive: true });
|
||||
const header = `${JSON.stringify({ type: 'session', id: 'remote-dotfiles-uuid', cwd: workingDirNoSlash })}\n`;
|
||||
writeFileSync(join(sessionDir, '2026-08-29T00-00-00-000Z_remote-dotfiles-uuid.jsonl'), header);
|
||||
|
||||
expect(resolveAndClaimOmpSessionId(workingDirWithSlash)).toBe('remote-dotfiles-uuid');
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveOmpConfigForCreate', () => {
|
||||
// The exact pipeline "resume this OMP row from the history list" drives:
|
||||
// POST /api/sessions with mode:'omp' + ompConfig:{continueSession:true}
|
||||
|
||||
@@ -187,6 +187,28 @@ describe('TmuxManager (unit)', () => {
|
||||
expect(command).toContain('claude --dangerously-skip-permissions --session-id abc123def456');
|
||||
expect(command).toContain('claude --dangerously-skip-permissions --resume abc123def456');
|
||||
});
|
||||
|
||||
it('resumes an explicit resumeSessionId distinct from sessionId (mirrors claudeDockerPaneCommand)', () => {
|
||||
// The docker-claude builder (claudeDockerPaneCommand) has always handled a
|
||||
// resumeId that differs from sessionId — e.g. a resume-from-history launch —
|
||||
// by leading with `--resume <rid> || --session-id <sessionId>`. The remote
|
||||
// claude branch used to only mirror the SAME-id fallback shape and silently
|
||||
// dropped a distinct resumeSessionId, so a remote resume-from-history claude
|
||||
// launch created a brand-new conversation instead of resuming the named one.
|
||||
const command = buildRemoteLaunchCommand({
|
||||
mode: 'claude',
|
||||
remote: { hostId: 'gpu-box', label: 'GPU Box', host: '10.0.0.42', username: 'ubuntu', remotePath: '/w' },
|
||||
sessionId: 'abc123def456',
|
||||
resumeSessionId: 'old-conversation-uuid',
|
||||
});
|
||||
expect(command).toContain('claude --dangerously-skip-permissions --resume old-conversation-uuid');
|
||||
expect(command).toContain('claude --dangerously-skip-permissions --session-id abc123def456');
|
||||
// The resume attempt must lead — session-id is the fallback here, reversed
|
||||
// from the same-id case.
|
||||
const resumeIdx = command.indexOf('--resume old-conversation-uuid');
|
||||
const sessionIdIdx = command.indexOf('--session-id abc123def456');
|
||||
expect(resumeIdx).toBeLessThan(sessionIdIdx);
|
||||
});
|
||||
});
|
||||
|
||||
describe('remote kill command builder', () => {
|
||||
|
||||
Reference in New Issue
Block a user