mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 20:49:41 +02:00
Three review findings on the DeepSeek Harness mode, plus one the third exposed. 1. The multi-user clamp was bypassable by a sibling field on the same request. clampExternalCliBypassForOwner() clamps deepSeekConfig.permissionMode, but DSH_* is an allowlisted envOverrides prefix and applyEnvOverrides() runs AFTER _configureDeepSeek(), so a non-granted owner sending envOverrides.DSH_PERMISSION_MODE landed last and won. Measured on an isolated instance: a session created with permissionMode "read-only" and that override ran with DSH_PERMISSION_MODE=danger-full-access in its pane. Every other CLI's bypass is a command-line flag reachable only through the per-CLI config, which is why the config clamp alone is the whole gate for them. clampEnvOverridesForOwner() adds the env-var half: for a non-granted owner it DROPS DSH_PERMISSION_MODE and DSH_HOME (dropping falls through to what _configureDeepSeek() exports, i.e. the clamped value). DSH_HOME is on that list because it aims the launcher at a profile tree whose plugin code runs at boot, before any approval row can apply. Verified end to end in real multi-user mode: a non-granted user sending both now gets workspace-write and no DSH_HOME, while an unrelated DSH_TELEMETRY_MODE passes through untouched. 2. POST /api/deepseek/install-profile could hang forever. spawn's own `timeout` signals only the direct child, and a plugin install fans out into package-manager children that keep the inherited stdio pipes open, so `close` never fires and the held-open request leaks with no route-level deadline. Reproduced: with a 1.5s built-in timeout the promise was still unsettled after 6s and both fan-out children were alive. Now detached: true plus negative-pid SIGTERM/SIGKILL, the same escalation runGit() uses for the same reason, with a last-resort reap for a grandchild that escaped the group. Same probe after the change: close fires, direct child and both grandchildren dead. 3. hooksAvailableForMode() promised more than a dsh session can deliver. deepSeekConfig.statusReporting: false disarms the HERDR_* export, and that triple is the only reason a dsh session posts hook events, so `until=stop` was accepted and then blocked for the caller's whole timeout: the exact infinite-wait-dressed-as-a-timeout the predicate exists to prevent. It now takes HookCapabilityOptions and every call site passes sessionHookOptions(), with the deepseek arm reading `!== false` so a forgotten one degrades to the old behaviour. The refusal names the setting rather than saying "no Claude Code hooks", which would send the caller hunting a bug that is really a setting they chose. Profile conformance stays unknowable at request time and is documented as such. The stale "True for `claude` and nothing else" docblock is corrected. 4. Exposed by (3): hooksAvailableForMode() was doing double duty as "is this a claude session". Read My Mind (POST /api/sessions/:id/readmymind) and intent capture read Claude's own transcript, and adding deepseek silently widened both to a mode that has none. They compare mode === 'claude' directly now, and a static check pins them there. Verified: full CI gate green (6132 passed), typecheck/lint/format clean, and the wait-signal gating exercised against a live server with a real dsh 0.1.1-rc.2 -- bridge off plus explicit until=stop is a 400 naming the setting, bridge off with no `until` still 200s on idle/exit, bridge on accepts stop.
360 lines
16 KiB
TypeScript
360 lines
16 KiB
TypeScript
/**
|
|
* DeepSeek Harness (`dsh`) run mode.
|
|
*
|
|
* The interesting assertions here are the ones that differ from every sibling
|
|
* CLI, because dsh is shaped differently in two ways:
|
|
*
|
|
* 1. the agent is a PROFILE, not the binary, so the spawn line carries
|
|
* `--profile <name>` and a profile name has to be treated as a path segment;
|
|
* 2. the permission switch is an ENV VAR (`DSH_PERMISSION_MODE`), not a flag,
|
|
* so the thing to pin is that nothing permission-shaped ever reaches the
|
|
* command line.
|
|
*/
|
|
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest';
|
|
import { CreateSessionSchema, QuickStartSchema, HookEventSchema } from '../src/web/schemas.js';
|
|
import { buildSpawnCommand } from '../src/tmux-manager.js';
|
|
import { defaultDockerCommandForMode } from '../src/docker-hosts.js';
|
|
import { defaultRemoteCommandForMode } from '../src/remote-hosts.js';
|
|
import { isExternalCliMode, isAltScreenStripMode } from '../src/session.js';
|
|
import { hooksAvailableForMode, resolveWaitSignals } from '../src/web/session-wait-registry.js';
|
|
import { _clampExternalCliBypassForOwner, _clampEnvOverridesForOwner } from '../src/web/routes/session-routes.js';
|
|
import { DEEPSEEK_STATE_TO_HOOK_EVENT } from '../src/deepseek-status-shim.js';
|
|
import { readFileSync } from 'node:fs';
|
|
import { join } from 'node:path';
|
|
|
|
vi.mock('../src/utils/deepseek-cli-resolver.js', async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import('../src/utils/deepseek-cli-resolver.js')>();
|
|
return { ...actual, resolveDefaultDeepSeekProfile: vi.fn(() => 'dsh-tui') };
|
|
});
|
|
|
|
describe('DeepSeek mode schemas', () => {
|
|
it('accepts DeepSeek session creation config', () => {
|
|
const parsed = CreateSessionSchema.parse({
|
|
workingDir: '/tmp',
|
|
mode: 'deepseek',
|
|
deepSeekConfig: { profile: 'dsh-tui', permissionMode: 'danger-full-access' },
|
|
});
|
|
|
|
expect(parsed.mode).toBe('deepseek');
|
|
expect(parsed.deepSeekConfig).toEqual({ profile: 'dsh-tui', permissionMode: 'danger-full-access' });
|
|
});
|
|
|
|
it('accepts DeepSeek quick-start config', () => {
|
|
const parsed = QuickStartSchema.parse({
|
|
caseName: 'dsh-case',
|
|
mode: 'deepseek',
|
|
deepSeekConfig: { resumeSessionId: 'sess_01H9', statusReporting: false },
|
|
});
|
|
|
|
expect(parsed.mode).toBe('deepseek');
|
|
expect(parsed.deepSeekConfig?.resumeSessionId).toBe('sess_01H9');
|
|
expect(parsed.deepSeekConfig?.statusReporting).toBe(false);
|
|
});
|
|
|
|
it('rejects a profile name that is not a single path segment', () => {
|
|
// A profile is BOTH interpolated into a `bash -c "…"` line and joined into a
|
|
// filesystem path under $DSH_HOME/profiles, so separators and traversal have
|
|
// to die at the schema boundary.
|
|
for (const profile of ['../../etc/passwd', 'a/b', './x', '-rf', 'has space', 'semi;colon']) {
|
|
expect(() =>
|
|
CreateSessionSchema.parse({ workingDir: '/tmp', mode: 'deepseek', deepSeekConfig: { profile } })
|
|
).toThrow();
|
|
}
|
|
});
|
|
|
|
it('rejects an unknown permission preset', () => {
|
|
// The three presets are the harness's own; anything else would be exported
|
|
// verbatim as DSH_PERMISSION_MODE and silently fall back to its default.
|
|
expect(() =>
|
|
CreateSessionSchema.parse({
|
|
workingDir: '/tmp',
|
|
mode: 'deepseek',
|
|
deepSeekConfig: { permissionMode: 'yolo' },
|
|
})
|
|
).toThrow();
|
|
});
|
|
|
|
it('rejects unsafe resumeSessionId values', () => {
|
|
expect(() =>
|
|
CreateSessionSchema.parse({
|
|
workingDir: '/tmp',
|
|
mode: 'deepseek',
|
|
deepSeekConfig: { resumeSessionId: '../../etc/passwd' },
|
|
})
|
|
).toThrow();
|
|
});
|
|
|
|
it('allows DSH_* and DEEPSEEK_* env overrides but not a foreign provider key', () => {
|
|
const ok = CreateSessionSchema.parse({
|
|
workingDir: '/tmp',
|
|
mode: 'deepseek',
|
|
envOverrides: { DSH_HOME: '/tmp/dsh', DEEPSEEK_API_KEY: 'sk-test' },
|
|
});
|
|
expect(ok.envOverrides).toEqual({ DSH_HOME: '/tmp/dsh', DEEPSEEK_API_KEY: 'sk-test' });
|
|
|
|
// A dsh settings.yaml can name ANY env var as a provider credential
|
|
// (apiKeyEnv), which is pi's 34-provider-key problem in a new shape. The
|
|
// allowlist is global, so admitting them would widen every mode at once.
|
|
expect(() =>
|
|
CreateSessionSchema.parse({
|
|
workingDir: '/tmp',
|
|
mode: 'deepseek',
|
|
envOverrides: { QWEN5090_API_KEY: 'sk-test' },
|
|
})
|
|
).toThrow();
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek spawn command', () => {
|
|
it('boots the requested profile', () => {
|
|
const cmd = buildSpawnCommand({
|
|
mode: 'deepseek',
|
|
sessionId: 's1',
|
|
deepSeekConfig: { profile: 'dsh-tui' },
|
|
});
|
|
expect(cmd).toBe('dsh --profile dsh-tui');
|
|
});
|
|
|
|
it('falls back to the resolved default profile when none was requested', () => {
|
|
const cmd = buildSpawnCommand({ mode: 'deepseek', sessionId: 's1' });
|
|
expect(cmd).toBe('dsh --profile dsh-tui');
|
|
});
|
|
|
|
it('never puts anything permission-shaped on the command line', () => {
|
|
// The harness has NO permission flag: the switch is the DSH_PERMISSION_MODE
|
|
// env export, applied via `tmux setenv`. If this ever starts failing, someone
|
|
// has invented a flag that does not exist.
|
|
const cmd = buildSpawnCommand({
|
|
mode: 'deepseek',
|
|
sessionId: 's1',
|
|
deepSeekConfig: { profile: 'dsh-tui', permissionMode: 'danger-full-access' },
|
|
});
|
|
expect(cmd).toBe('dsh --profile dsh-tui');
|
|
expect(cmd).not.toMatch(/danger|approve|permission|yolo|dangerously/i);
|
|
});
|
|
|
|
it('prefers an explicit resume id over the most-recent form', () => {
|
|
const cmd = buildSpawnCommand({
|
|
mode: 'deepseek',
|
|
sessionId: 's1',
|
|
deepSeekConfig: { profile: 'p', resumeSession: true, resumeSessionId: 'sess_42' },
|
|
});
|
|
expect(cmd).toBe('dsh --profile p --resume sess_42');
|
|
});
|
|
|
|
it('resumes the most recent session when only the flag is set', () => {
|
|
const cmd = buildSpawnCommand({
|
|
mode: 'deepseek',
|
|
sessionId: 's1',
|
|
deepSeekConfig: { profile: 'p', resumeSession: true },
|
|
});
|
|
expect(cmd).toBe('dsh --profile p --resume');
|
|
});
|
|
|
|
it('drops an unsafe profile rather than interpolating it', () => {
|
|
// Defense in depth behind the schema: builders must not trust their callers,
|
|
// because this string is interpolated into a `bash -c "…"` argument.
|
|
const cmd = buildSpawnCommand({
|
|
mode: 'deepseek',
|
|
sessionId: 's1',
|
|
deepSeekConfig: { profile: 'evil; rm -rf /' },
|
|
});
|
|
expect(cmd).not.toContain('rm -rf');
|
|
expect(cmd).toBe('dsh --profile dsh-tui');
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek mode wiring', () => {
|
|
it('is an external CLI mode', () => {
|
|
expect(isExternalCliMode('deepseek')).toBe(true);
|
|
});
|
|
|
|
it('is NOT an alt-screen strip mode', () => {
|
|
// The strip is for Ink-style repaint TUIs (claude/codex/gemini). A dsh
|
|
// terminal profile is a third-party fullscreen TUI, i.e. the opencode case.
|
|
expect(isAltScreenStripMode('deepseek')).toBe(false);
|
|
});
|
|
|
|
it('has default remote and docker commands', () => {
|
|
expect(defaultRemoteCommandForMode('deepseek')).toContain('dsh');
|
|
expect(defaultDockerCommandForMode('deepseek')).toBe('exec dsh');
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek status bridge', () => {
|
|
it('is the only non-claude mode allowed to deliver hook signals', () => {
|
|
// Earned, not granted: the harness terminal front door REPORTS its state to
|
|
// a supervisor, so `stop` and `blocked` for a dsh session are definitive
|
|
// rather than inferred. Every other external CLI must keep failing this.
|
|
expect(hooksAvailableForMode('deepseek')).toBe(true);
|
|
expect(hooksAvailableForMode('claude')).toBe(true);
|
|
for (const mode of ['shell', 'opencode', 'codex', 'gemini', 'antigravity', 'pi', 'grok'] as const) {
|
|
expect(hooksAvailableForMode(mode)).toBe(false);
|
|
}
|
|
});
|
|
|
|
it('is a per-SESSION answer for deepseek: a disarmed status bridge emits nothing', () => {
|
|
// `statusReporting: false` is what stops _configureDeepSeek() exporting the
|
|
// HERDR_* triple, and the triple is the ONLY reason a dsh session posts hook
|
|
// events. Answering from the mode alone would accept `until=stop` on a
|
|
// session where nothing can ever send one, which is the exact
|
|
// infinite-wait-dressed-as-a-timeout this predicate exists to prevent.
|
|
expect(hooksAvailableForMode('deepseek', { deepSeekStatusReporting: false })).toBe(false);
|
|
expect(hooksAvailableForMode('deepseek', { deepSeekStatusReporting: true })).toBe(true);
|
|
// Not sent = ON, so an ordinary session is unaffected.
|
|
expect(hooksAvailableForMode('deepseek', {})).toBe(true);
|
|
expect(hooksAvailableForMode('deepseek', { deepSeekStatusReporting: undefined })).toBe(true);
|
|
// The flag is meaningless for every other mode and must not move them.
|
|
expect(hooksAvailableForMode('claude', { deepSeekStatusReporting: false })).toBe(true);
|
|
expect(hooksAvailableForMode('codex', { deepSeekStatusReporting: true })).toBe(false);
|
|
});
|
|
|
|
it('refuses an explicit stop/blocked on a dsh session whose bridge is off, and says why', () => {
|
|
const off = { mode: 'deepseek' as const, deepSeekStatusReporting: false };
|
|
const on = { mode: 'deepseek' as const };
|
|
|
|
expect(resolveWaitSignals('stop', on)).toEqual({ until: ['stop'], error: null });
|
|
|
|
const rejected = resolveWaitSignals('stop', off);
|
|
expect(rejected.until).toEqual([]);
|
|
// The generic "no Claude Code hooks" wording would send the caller hunting a
|
|
// bug that is really a setting they chose, so this arm names the setting.
|
|
expect(rejected.error).toContain('statusReporting');
|
|
expect(rejected.error).not.toContain('no Claude Code hooks');
|
|
|
|
// An OMITTED `until` must never 400: the hook-only signals are dropped from
|
|
// the default set instead, leaving the two that still work.
|
|
expect(resolveWaitSignals(undefined, off)).toEqual({ until: ['idle', 'exit'], error: null });
|
|
expect(resolveWaitSignals(undefined, on).until).toContain('stop');
|
|
});
|
|
|
|
it('keeps the hook predicate out of the two gates that mean "is this claude"', () => {
|
|
// Read My Mind and intent capture read Claude's own transcript, so they mean
|
|
// mode === 'claude'. They used to ask hooksAvailableForMode(), which was the
|
|
// same question until `deepseek` earned a yes and silently widened both to a
|
|
// mode with no transcript to read. Static, because the alternative is
|
|
// standing up a predictor and a transcript watcher to observe one `if`.
|
|
const rmm = readFileSync(join(process.cwd(), 'src/web/routes/readmymind-routes.ts'), 'utf-8');
|
|
expect(rmm).toContain("session.mode !== 'claude'");
|
|
// Comment lines dropped first: the comment above that `if` names the
|
|
// predicate in order to explain why it is NOT the one being called there.
|
|
const uncommented = (src: string) =>
|
|
src
|
|
.split('\n')
|
|
.filter((line) => !/^\s*(\/\/|\*|\/\*)/.test(line))
|
|
.join('\n');
|
|
expect(uncommented(rmm)).not.toMatch(/hooksAvailableForMode\(/);
|
|
|
|
const server = readFileSync(join(process.cwd(), 'src/web/server.ts'), 'utf-8');
|
|
expect(server).toContain("if (!session || session.mode !== 'claude') return;");
|
|
});
|
|
|
|
it('maps the harness lifecycle states onto real hook events', () => {
|
|
expect(DEEPSEEK_STATE_TO_HOOK_EVENT.idle).toBe('stop');
|
|
expect(DEEPSEEK_STATE_TO_HOOK_EVENT.blocked).toBe('permission_prompt');
|
|
expect(DEEPSEEK_STATE_TO_HOOK_EVENT.working).toBe('agent_working');
|
|
// Every mapped event must be one the hook endpoint actually accepts, or the
|
|
// bridge would post reports the schema silently rejects.
|
|
for (const event of Object.values(DEEPSEEK_STATE_TO_HOOK_EVENT)) {
|
|
expect(() => HookEventSchema.parse({ event, sessionId: 's1' })).not.toThrow();
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek multi-user clamp', () => {
|
|
const ORIGINAL = process.env.CODEMAN_MULTIUSER;
|
|
beforeEach(() => {
|
|
process.env.CODEMAN_MULTIUSER = '1';
|
|
});
|
|
afterEach(() => {
|
|
if (ORIGINAL === undefined) delete process.env.CODEMAN_MULTIUSER;
|
|
else process.env.CODEMAN_MULTIUSER = ORIGINAL;
|
|
});
|
|
|
|
it('clamps a sent danger-full-access down to workspace-write, not read-only', () => {
|
|
// The clamp removes PRIVILEGE; it must not also break the session's ability
|
|
// to edit its own workspace, which read-only would.
|
|
return _clampExternalCliBypassForOwner('nobody', undefined, undefined, undefined, undefined, undefined, {
|
|
permissionMode: 'danger-full-access',
|
|
}).then((out) => {
|
|
expect(out.deepSeekConfig?.permissionMode).toBe('workspace-write');
|
|
});
|
|
});
|
|
|
|
it('leaves an ABSENT config absent (the only-if-sent branch)', async () => {
|
|
// Omitting DSH_PERMISSION_MODE leaves the harness on its own workspace-write
|
|
// preset, which still asks — so there is nothing to materialize, unlike pi.
|
|
const out = await _clampExternalCliBypassForOwner(
|
|
'nobody',
|
|
undefined,
|
|
undefined,
|
|
undefined,
|
|
undefined,
|
|
undefined,
|
|
undefined
|
|
);
|
|
expect(out.deepSeekConfig).toBeUndefined();
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek multi-user clamp: the env-var half', () => {
|
|
const ORIGINAL = process.env.CODEMAN_MULTIUSER;
|
|
beforeEach(() => {
|
|
process.env.CODEMAN_MULTIUSER = '1';
|
|
});
|
|
afterEach(() => {
|
|
if (ORIGINAL === undefined) delete process.env.CODEMAN_MULTIUSER;
|
|
else process.env.CODEMAN_MULTIUSER = ORIGINAL;
|
|
});
|
|
|
|
it('strips DSH_PERMISSION_MODE, which would otherwise undo the config clamp on the same request', async () => {
|
|
// applyEnvOverrides() runs AFTER _configureDeepSeek() in tmux-manager, so an
|
|
// override sent alongside the config lands last and WINS. Clamping the config
|
|
// alone is therefore half a gate: this is the other half.
|
|
const out = await _clampEnvOverridesForOwner('nobody', {
|
|
DSH_PERMISSION_MODE: 'danger-full-access',
|
|
DSH_TELEMETRY_MODE: 'off',
|
|
});
|
|
expect(out).toEqual({ DSH_TELEMETRY_MODE: 'off' });
|
|
});
|
|
|
|
it('strips DSH_HOME, which points the launcher at a profile tree that executes at boot', async () => {
|
|
const out = await _clampEnvOverridesForOwner('nobody', { DSH_HOME: '/home/attacker/evil-dsh' });
|
|
expect(out).toEqual({});
|
|
});
|
|
|
|
it('leaves unrelated overrides alone, and returns the same object when there is nothing to strip', async () => {
|
|
const input = { DEEPSEEK_API_KEY: 'sk-test', CODEX_HOME: '/tmp/cx' };
|
|
const out = await _clampEnvOverridesForOwner('nobody', input);
|
|
expect(out).toBe(input);
|
|
expect(await _clampEnvOverridesForOwner('nobody', undefined)).toBeUndefined();
|
|
});
|
|
|
|
it('is a no-op in single-user mode', async () => {
|
|
delete process.env.CODEMAN_MULTIUSER;
|
|
const input = { DSH_PERMISSION_MODE: 'danger-full-access', DSH_HOME: '/opt/dsh' };
|
|
// canUsernameRunPrivilegedCommands() returns true when !isMultiUserMode(), so
|
|
// the single-user behaviour has to be byte-identical to before this clamp.
|
|
expect(await _clampEnvOverridesForOwner(undefined, input)).toBe(input);
|
|
});
|
|
});
|
|
|
|
describe('DeepSeek profile install is bounded for real', () => {
|
|
it('runs in its own process group and escalates the kill to the whole tree', () => {
|
|
// `dsh plugin add` fans out into package-manager resolver/build children, and
|
|
// spawn's own `timeout` signals only the direct child: survivors hold the
|
|
// inherited stdio pipes open, `close` never fires, and the held-open request
|
|
// leaks forever. Same failure and same fix as runGit() in git-clone.ts.
|
|
// Static, because reproducing it needs a real package manager that hangs.
|
|
const src = readFileSync(join(process.cwd(), 'src/web/routes/system-routes.ts'), 'utf-8');
|
|
const handler = src.slice(src.indexOf("app.post('/api/deepseek/install-profile'"));
|
|
const body = handler.slice(0, handler.indexOf('app.post(', 1) + 1 || handler.length);
|
|
expect(body).toContain('detached: true');
|
|
expect(body).toContain('process.kill(-child.pid, signal)');
|
|
expect(body).toContain("killTree('SIGTERM')");
|
|
expect(body).toContain("killTree('SIGKILL')");
|
|
// The built-in option is the thing that did NOT work here; it must not come back.
|
|
expect(body).not.toContain('timeout: DEEPSEEK_INSTALL_TIMEOUT_MS');
|
|
});
|
|
});
|