diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index fcebbbcc..6fbb5dbe 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -34,7 +34,7 @@ Implementation detail extracted from `CLAUDE.md` so that file stays small enough ⚠️ **The resolver needs the strictest identity probe of any CLI**, because `dsh` is not merely a squattable npm name: Debian ships an unrelated `dsh` (dancer's shell, `apt install dsh`) that would answer a version probe convincingly. `probeDeepSeekVersion()` therefore checks `dsh --help` against `DEEPSEEK_IDENTITY_REGEX` (`DeepSeek Harness`) FIRST and only then reads a version, and `test/deepseek-cli-resolver.test.ts` pins both the rejection and the VITEST hermeticity gate with a real executable fixture. `DEEPSEEK_VERSION_REGEX` keeps the prerelease tail (`0.1.1-rc.2`), since truncating it would report an rc as a release; it is shared with the `dsh` dependency-registry entry so doctor and run mode agree about the version even though the resolver is stricter about identity. -Model is NOT a session field: it is a composition entry in the profile's config tree (`agent-default-model`), configured in `~/.dsh/settings.yaml` + `cordis.patch.yml`, so both create paths deliberately resolve no model for this mode. Env allowlist: `DSH_*` + `DEEPSEEK_*`; provider keys named by a settings-file `apiKeyEnv` stay OUT, which is pi's 34-provider-key problem in a new shape and gets the same answer. Docker seeds `~/.dsh` per-file (`.env`, `settings.yaml`, `cordis.patch.yml`) and the image installs its OWN profile, because `profiles/` is a per-profile `node_modules` tree — host-arch-specific and far too large to copy per container start. Stays OUT of `isAltScreenStripMode()` (third-party fullscreen TUI — the opencode case). Availability via `GET /api/deepseek/status`, the widest per-CLI status shape (`available`/`runnable`/`path`/`version`/`dshHome`/`defaultProfile`/`profiles`); `POST /api/deepseek/install-profile` bootstraps a profile and is the only endpoint in Codeman that installs third-party code — regex-confined specifier, argv-array spawn, privileged grant required in multi-user mode, and the held-open request is bounded by a HAND-ROLLED timeout over a `detached: true` process group (negative-pid SIGTERM→SIGKILL, as `runGit()` does in git-clone.ts). ⚠️ Node's own `spawn` `timeout` is NOT enough: a plugin install fans out into package-manager children, the built-in timeout signals only the direct child, and the survivors hold the inherited stdio pipes open so `close` never fires and the request leaks forever. User guide: `docs/deepseek-integration.md`. Tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`. +Model is NOT a session field: it is a composition entry in the profile's config tree (`agent-default-model`), configured in `~/.dsh/settings.yaml` + `cordis.patch.yml`, so both create paths deliberately resolve no model for this mode. Env allowlist: `DSH_*` + `DEEPSEEK_*`; provider keys named by a settings-file `apiKeyEnv` stay OUT, which is pi's 34-provider-key problem in a new shape and gets the same answer. Docker seeds `~/.dsh` per-file (`.env`, `settings.yaml`, `cordis.patch.yml`) and the image installs its OWN profile, because `profiles/` is a per-profile `node_modules` tree — host-arch-specific and far too large to copy per container start. Stays OUT of `isAltScreenStripMode()` (third-party fullscreen TUI — the opencode case). ⚠️ `classifyProfile()` reads the profile's BUNDLES, and "unknown means launchable" is deliberate (anyone can publish an app bundle), but it has one knowably-wrong case: `readProfile()` returns an empty bundle list for a `package.json` with no `dsh.profile.bundles`, which made the SHIPPED `web`/`headless` profiles look third-party and launchable. The directory name is therefore consulted as a LAST resort (`STOCK_NON_INTERACTIVE_PROFILES`), after the bundle patterns, so real bundle evidence always wins over a name the user chose. The loose `tui` arm carries word boundaries for the same reason: it decides which profile boots by default, and matching the middle of `intuition` is not a rule anyone could predict. ⚠️ The generated shim is written **temp + rename**, not in place: the TUI can be exec'ing that exact path while an upgraded Codeman refreshes it, and a half-written file is a syntax error the caller then retries four times per state change forever. Bump `SHIM_VERSION` whenever `SHIM_SOURCE` changes, or an existing shim keeps matching the embedded marker and is never refreshed. Availability via `GET /api/deepseek/status`, the widest per-CLI status shape (`available`/`runnable`/`path`/`version`/`dshHome`/`defaultProfile`/`profiles`); `POST /api/deepseek/install-profile` bootstraps a profile and is the only endpoint in Codeman that installs third-party code — regex-confined specifier, argv-array spawn, privileged grant required in multi-user mode, and the held-open request is bounded by a HAND-ROLLED timeout over a `detached: true` process group (negative-pid SIGTERM→SIGKILL, as `runGit()` does in git-clone.ts). ⚠️ Node's own `spawn` `timeout` is NOT enough: a plugin install fans out into package-manager children, the built-in timeout signals only the direct child, and the survivors hold the inherited stdio pipes open so `close` never fires and the request leaks forever. User guide: `docs/deepseek-integration.md`. Tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`. **Pi specifics** (#206, `docs/pi-integration.md`): command built by `buildPiCommand()` (`--model` — the only builder whose model regex admits `:` and `/`, for `sonnet:high` and `openai/gpt-4o` — plus `--provider`, `--thinking`, `--session ` / `-c`, and the TRI-STATE `--approve`/`--no-approve`). ⚠️ **Pi has no permission prompts and no sandbox**, so there is no `--dangerously-skip-permissions` analog and Codeman must not invent one; the privilege-shaped knob is `approveProjectTrust`, which makes pi LOAD AND EXECUTE repo-local `.pi/extensions` TypeScript and npm-install missing project packages. It therefore joins `clampExternalCliBypassForOwner()`'s **materialize** branch (gemini's, not codex/antigravity's only-if-sent one): an absent config still yields `--no-approve` for a non-granted owner, because pi's own default is an interactive prompt the session user could answer themselves. ⚠️ `--api-key` is NEVER wired — it would put a provider secret on the spawn command line. ⚠️ Pi stays **out** of `isAltScreenStripMode()`: its default TUI renders into the main screen with terminal-owned scrollback (nothing to strip), and since 0.84.0 the user can flip to a fullscreen TUI at runtime via `/settings`, where the alt screen is load-bearing — being out of the list is exactly what makes that switch safe. ⚠️ Only the `PI_*` env prefix was added; pi's ~34 provider keys share no prefix and `ALLOWED_ENV_PREFIXES` is a single GLOBAL list with no mode context, so admitting them would widen the allowlist for every mode at once (a mode-aware allowlist is the tracked follow-up). ⚠️ `pi` is a short, GENERIC binary name, so unlike the sibling resolvers `pi-cli-resolver.ts` sanity-probes `pi --version` (cached, vitest-skipped) and requires semver-shaped output; `GET /api/pi/status` carries `version` on top of the sibling `{available, path}` shape so a misresolution is diagnosable. Local echo: pi lands on the `'buffer'` overlay via the fallthrough in `_updateLocalEchoState` (pinned in `test/local-echo-codex-gating.test.ts`); if pi's live composer turns out to fight it the way codex's did, the fallback is one `'off'` branch. Tests: `test/pi-mode.test.ts`, `test/routes/external-cli-bypass-clamp.test.ts` (first-ever coverage of the clamp). diff --git a/src/deepseek-status-shim.ts b/src/deepseek-status-shim.ts index 19dbfe9a..fa3b385e 100644 --- a/src/deepseek-status-shim.ts +++ b/src/deepseek-status-shim.ts @@ -44,7 +44,7 @@ * @module deepseek-status-shim */ -import { chmodSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; import { dataPath } from './config/instance.js'; @@ -54,7 +54,7 @@ import { dataPath } from './config/instance.js'; * by an older Codeman and rewrite only when needed (rather than rewriting on * every session create, or — worse — leaving a stale one in place forever). */ -const SHIM_VERSION = 1; +const SHIM_VERSION = 2; const SHIM_MARKER = `codeman-dsh-status-shim v${SHIM_VERSION}`; /** @@ -125,8 +125,13 @@ const event = STATE_TO_EVENT[String(flag('--state') ?? '')] if (!event) process.exit(0) // The pane id we hand the TUI IS the Codeman session id, but prefer the ambient -// env: it is set by the same code that set HERDR_PANE_ID and cannot be spoofed -// by an argument the agent itself could influence. +// env: it is set by the same code that set HERDR_PANE_ID, so a TUI that mangles, +// truncates or re-uses the pane argument still reports against the right session. +// NOT a security boundary, and do not read it as one: the agent runs IN this pane +// and can invoke the shim with CODEMAN_SESSION_ID unset and any argv it likes. +// That buys it nothing it did not already have, since the hook-secret file is +// readable from the same pane and any process there can POST /api/hook-event +// directly. Attribution here is about accidents, not adversaries. const sessionId = process.env.CODEMAN_SESSION_ID || argv[2] const apiUrl = process.env.CODEMAN_API_URL if (!sessionId || !apiUrl) process.exit(1) @@ -213,7 +218,26 @@ export function ensureDeepSeekStatusShim(): string | null { } if (!current.includes(SHIM_MARKER)) { mkdirSync(dirname(path), { recursive: true }); - writeFileSync(path, SHIM_SOURCE, { mode: 0o700 }); + // Temp + rename, not a plain write: the TUI can be executing this exact + // path at the moment an upgraded Codeman refreshes it (every state change + // runs it, and session create is when the rewrite happens), and a reader + // that catches a half-written file gets a syntax error, exits non-zero, + // and is retried four times per state change for a file that will never + // parse. rename(2) is atomic within the directory, so a concurrent exec + // sees either the old shim or the new one, never a truncated one. + // Same reasoning as the state-store writes; pid-suffixed so two instances + // sharing a data dir cannot collide on the temp name. + const tempPath = `${path}.${process.pid}.tmp`; + try { + writeFileSync(tempPath, SHIM_SOURCE, { mode: 0o700 }); + // The mode argument only applies when writeFileSync CREATES the file, so + // a leftover temp from a crashed run would keep its old permissions. + chmodSync(tempPath, 0o700); + renameSync(tempPath, path); + } catch (err) { + rmSync(tempPath, { force: true }); + throw err; + } } // Re-assert the mode even when the content matched: a shim that lost its // executable bit (a restored backup, a copied data dir) would make every diff --git a/src/utils/deepseek-cli-resolver.ts b/src/utils/deepseek-cli-resolver.ts index c30604b3..9868d01f 100644 --- a/src/utils/deepseek-cli-resolver.ts +++ b/src/utils/deepseek-cli-resolver.ts @@ -120,8 +120,36 @@ const HEADLESS_BUNDLE_PATTERN = /dsh-headless/i; * scoped `dsh-tui` packages from a dozen different authors compete. Anything * matching is a TUI; anything unmatched is `unknown`, which still counts as * launchable. + * + * `tui` carries word boundaries so the loose arm stays a TOKEN match: `-` and + * `/` are non-word characters, so `@someone/tui-app` and `dsh-tui` both match + * while `intuition` and `gratuitous` do not. Being wrong here is cheap (an + * unmatched profile is `unknown`, which is launchable too) but it decides which + * profile a session boots by DEFAULT, and "the one whose name happens to contain + * t-u-i" is not a rule anyone could predict. */ -const TUI_BUNDLE_PATTERN = /dsh-tui|dsh-terminal-app|tui/i; +const TUI_BUNDLE_PATTERN = /dsh-tui|dsh-terminal-app|\btui\b/i; + +/** + * The profile names DeepSeek itself ships for its non-interactive surfaces. + * + * Consulted only AFTER the bundle patterns have found nothing, and only against + * the directory name. `readProfile()` yields an empty bundle list for any + * `package.json` without a `dsh.profile.bundles` array — a hand-edited file, an + * older layout, a profile mid-install — and with no bundles to read, the stock + * `web` and `headless` profiles look exactly like an unrecognized third-party + * one and inherit its launchable-by-default treatment. That is the single + * "unknown" that is knowably wrong, and it produces precisely the + * pane-dies-on-arrival failure the two-part availability gate exists to prevent. + * + * Deliberately a fallback rather than a first check: a third-party profile that + * legitimately composes a terminal app is identified by its BUNDLES, and its + * directory name (which the user chose) must never override that evidence. + */ +const STOCK_NON_INTERACTIVE_PROFILES = new Map([ + ['web', 'web'], + ['headless', 'headless'], +]); /** Profile directory names that are not profiles. */ const NON_PROFILE_DIRS = new Set(['node_modules', '.bin', '.pnpm']); @@ -134,7 +162,7 @@ function classifyProfile(name: string, bundles: string[]): DeepSeekProfileKind { if (WEB_BUNDLE_PATTERN.test(haystack)) return 'web'; if (HEADLESS_BUNDLE_PATTERN.test(haystack)) return 'headless'; if (TUI_BUNDLE_PATTERN.test(haystack)) return 'interactive'; - return 'unknown'; + return STOCK_NON_INTERACTIVE_PROFILES.get(name.toLowerCase()) ?? 'unknown'; } /** diff --git a/test/deepseek-cli-resolver.test.ts b/test/deepseek-cli-resolver.test.ts index c458110d..83397c7c 100644 --- a/test/deepseek-cli-resolver.test.ts +++ b/test/deepseek-cli-resolver.test.ts @@ -224,6 +224,51 @@ describe('DeepSeek profile inventory', () => { expect(resolveDefaultDeepSeekProfile()).toBe('custom'); }); + it('does not treat a bundle-less stock profile as launchable', () => { + // readProfile() yields an empty bundle list for any package.json without a + // `dsh.profile.bundles` array (hand-edited, older layout, mid-install), and + // with no bundles to read the shipped web/headless profiles used to look + // exactly like an unrecognized third-party one — inheriting its + // launchable-by-default treatment and producing the pane-dies-on-arrival + // failure the two-part availability gate exists to prevent. + const bare = (name: string) => { + const dir = join(home, 'profiles', name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'package.json'), JSON.stringify({ name: `dsh-profile-${name}` })); + }; + bare('web'); + bare('headless'); + + const profiles = listDeepSeekProfiles(); + expect(profiles.map((p) => `${p.name}:${p.kind}`).sort()).toEqual(['headless:headless', 'web:web']); + expect(profiles.every((p) => !isLaunchableProfile(p))).toBe(true); + expect(resolveDefaultDeepSeekProfile()).toBeNull(); + }); + + it('lets bundle evidence beat the name fallback', () => { + // The name check is a LAST resort, so a profile the user happened to call + // `web` that really composes a terminal app is still interactive. Otherwise + // a directory name would override what the profile actually contains. + writeProfile('web', ['@deepseek-ai/dsh-base', '@someone/dsh-tui']); + const profile = listDeepSeekProfiles().find((p) => p.name === 'web')!; + expect(profile.kind).toBe('interactive'); + expect(resolveDefaultDeepSeekProfile()).toBe('web'); + }); + + it('does not read `tui` out of the middle of an unrelated word', () => { + // The loose arm is a TOKEN match: `@someone/tui-app` is a TUI, `intuition` + // is a word. Being wrong is cheap (unknown is launchable too) but it decides + // which profile boots by DEFAULT, and "its name contains t-u-i" is not a + // rule anyone could predict. + writeProfile('intuition', ['@someone/gratuitous-surface']); + expect(listDeepSeekProfiles().find((p) => p.name === 'intuition')!.kind).toBe('unknown'); + + writeProfile('mine', ['@someone/tui-app']); + expect(listDeepSeekProfiles().find((p) => p.name === 'mine')!.kind).toBe('interactive'); + // Preferred over the merely-unknown one, which is the whole point of ranking. + expect(resolveDefaultDeepSeekProfile()).toBe('mine'); + }); + it('survives a stray directory under profiles/', () => { mkdirSync(join(home, 'profiles', 'not-a-profile'), { recursive: true }); writeProfile('dsh-tui', ['@deepseek-harness-tui/dsh-tui']); diff --git a/test/deepseek-status-shim.test.ts b/test/deepseek-status-shim.test.ts new file mode 100644 index 00000000..15b51c81 --- /dev/null +++ b/test/deepseek-status-shim.test.ts @@ -0,0 +1,207 @@ +/** + * The generated DeepSeek Harness status shim. + * + * This file is the one piece of DeepSeek's wiring that is neither TypeScript we + * typecheck nor a route we can `inject()` into: it is a script emitted as a + * string, dropped in the data dir, and executed by a third-party TUI as a + * SUBPROCESS. So the assertions here run it the way the harness does — a real + * `node` process, real argv, real env, against a real listener — rather than + * inspecting the source text. + * + * The exit codes are the contract's load-bearing half: the caller retries with + * backoff on any non-zero, so "cannot ever succeed" (unknown verb, unmapped + * state) must exit 0 or one typo becomes four HTTP requests per state change, + * forever. + */ +import { describe, expect, it, beforeEach, beforeAll, afterAll } from 'vitest'; +import { execFileSync, spawn } from 'node:child_process'; +import { createServer, type Server } from 'node:http'; +import { existsSync, readdirSync, readFileSync, statSync, writeFileSync, chmodSync } from 'node:fs'; +import { dirname } from 'node:path'; +import { + ensureDeepSeekStatusShim, + deepSeekStatusShimPath, + resetDeepSeekStatusShimForTest, + DEEPSEEK_STATE_TO_HOOK_EVENT, +} from '../src/deepseek-status-shim.js'; + +const PORT = 3251; + +describe('DeepSeek status shim: provisioning', () => { + beforeEach(() => { + resetDeepSeekStatusShimForTest(); + }); + + it('writes an executable shim that node can actually parse', () => { + const path = ensureDeepSeekStatusShim(); + expect(path).toBeTruthy(); + expect(existsSync(path!)).toBe(true); + // 0700: the TUI execs it directly, so a lost exec bit means every report + // fails and is retried four times per state change. + expect(statSync(path!).mode & 0o777).toBe(0o700); + // `node --check` on the real file, because a template-literal typo in + // SHIM_SOURCE is invisible to tsc — the shim is a STRING as far as the + // compiler is concerned. + expect(() => execFileSync(process.execPath, ['--check', path!], { stdio: 'pipe' })).not.toThrow(); + }); + + it('refreshes a shim written by an older Codeman, and leaves no temp file behind', () => { + const path = deepSeekStatusShimPath(); + ensureDeepSeekStatusShim(); + const current = readFileSync(path, 'utf-8'); + + // A v1 shim from an older install: right path, stale content. + writeFileSync(path, '#!/usr/bin/env node\n// codeman-dsh-status-shim v1\nprocess.exit(0)\n', { mode: 0o700 }); + resetDeepSeekStatusShimForTest(); + ensureDeepSeekStatusShim(); + + expect(readFileSync(path, 'utf-8')).toBe(current); + // The rewrite goes through a temp + rename so a TUI exec'ing this path mid + // refresh can never read a half-written file. The temp must not survive it. + const strays = readdirSync(dirname(path)).filter((f) => f.startsWith('dsh-status-shim') && f.endsWith('.tmp')); + expect(strays).toEqual([]); + }); + + it('re-asserts the exec bit even when the content already matches', () => { + const path = ensureDeepSeekStatusShim()!; + chmodSync(path, 0o600); // a restored backup / copied data dir + resetDeepSeekStatusShimForTest(); + ensureDeepSeekStatusShim(); + expect(statSync(path).mode & 0o777).toBe(0o700); + }); +}); + +describe('DeepSeek status shim: the supervisor contract', () => { + let server: Server | undefined; + const received: Array<{ body: unknown; secret: string | undefined }> = []; + let status = 200; + + const listen = () => + new Promise((resolve) => { + server = createServer((req, res) => { + let raw = ''; + req.on('data', (c) => (raw += c)); + req.on('end', () => { + received.push({ + body: (() => { + try { + return JSON.parse(raw); + } catch { + return raw; + } + })(), + secret: req.headers['x-codeman-hook-secret'] as string | undefined, + }); + res.writeHead(status, { 'Content-Type': 'application/json' }); + res.end('{}'); + }); + }); + server.listen(PORT, '127.0.0.1', resolve); + }); + + beforeAll(() => listen()); + + afterAll(() => { + server?.close(); + }); + + /** + * Run the shim the way the TUI does, and ASYNCHRONOUSLY. + * + * Never spawnSync here: the listener above lives in this same process, so a + * synchronous spawn blocks the event loop that has to accept the connection. + * The shim then waits out its own 1500ms socket timeout and exits 1, which + * reads exactly like a broken shim (measured: `Socket._onTimeout` in its exit + * trace, and the server logging nothing). + */ + const run = (args: string[], env: Record = {}) => + new Promise<{ status: number | null; stderr: string }>((resolve) => { + const path = ensureDeepSeekStatusShim()!; + const child = spawn(process.execPath, [path, ...args], { + env: { + ...process.env, + CODEMAN_API_URL: `http://127.0.0.1:${PORT}`, + CODEMAN_SESSION_ID: 'sess-from-env', + ...env, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stderr = ''; + child.stderr.on('data', (c: Buffer) => (stderr += c.toString('utf-8'))); + child.on('close', (status) => resolve({ status, stderr })); + }); + + // The exact command line the harness TUI runs, from the Herdr contract. + const report = (state: string, extra: string[] = []) => [ + 'pane', + 'report-agent', + 'pane-arg-id', + '--source', + 'custom:dsh-tui', + '--agent', + 'dsh-tui', + '--state', + state, + ...extra, + '--seq', + '7', + ]; + + it('forwards each harness state as its mapped hook event, and exits 0 on delivery', async () => { + resetDeepSeekStatusShimForTest(); + for (const [state, event] of Object.entries(DEEPSEEK_STATE_TO_HOOK_EVENT)) { + received.length = 0; + const out = await run(report(state, ['--message', 'needs a decision'])); + expect(out.status, `${state}: ${out.stderr}`).toBe(0); + expect(received).toHaveLength(1); + const body = received[0].body as { event: string; sessionId: string; data: Record }; + expect(body.event).toBe(event); + // The ambient env wins over the pane argument: same code set both, and the + // argument is whatever the TUI chose to pass. + expect(body.sessionId).toBe('sess-from-env'); + expect(body.data.agent).toBe('dsh-tui'); + expect(body.data.message).toBe('needs a decision'); + } + }); + + it('sends the hook secret read at EXECUTION time, so rotation needs no respawn', async () => { + resetDeepSeekStatusShimForTest(); + const secretFile = `${deepSeekStatusShimPath()}.secret-fixture`; + writeFileSync(secretFile, 'rotated-secret\n', { mode: 0o600 }); + received.length = 0; + const out = await run(report('idle'), { CODEMAN_HOOK_SECRET_FILE: secretFile }); + expect(out.status).toBe(0); + expect(received[0].secret).toBe('rotated-secret'); + }); + + it('exits 0 without posting for anything a retry could never fix', async () => { + resetDeepSeekStatusShimForTest(); + for (const args of [ + ['pane', 'list'], // unknown verb + ['something-else', 'report-agent', 'id', '--state', 'idle'], // unknown noun + [...report('rebooting')], // a state this bridge does not map + ['pane', 'report-agent', 'id'], // no --state at all + ]) { + received.length = 0; + const out = await run(args); + expect(out.status, `args ${args.join(' ')}`).toBe(0); + expect(received).toEqual([]); + } + }); + + it('exits non-zero when the post genuinely fails, so the caller retries', async () => { + resetDeepSeekStatusShimForTest(); + + // A rejecting server: transport worked, Codeman said no. + status = 500; + received.length = 0; + expect((await run(report('idle'))).status).not.toBe(0); + expect(received).toHaveLength(1); + status = 200; + + // Nothing listening at all. + expect((await run(report('idle'), { CODEMAN_API_URL: 'http://127.0.0.1:1' })).status).not.toBe(0); + // No API url to post to. + expect((await run(report('idle'), { CODEMAN_API_URL: '' })).status).not.toBe(0); + }); +});