From 2034719d61dd04297b9e0493aee63c5b06fd3115 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Mon, 24 Aug 2026 16:01:02 +0200 Subject: [PATCH] fix(deepseek): close the env-var clamp hole, bound the profile install, make the hook gate per-session 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. --- CLAUDE.md | 2 +- docs/architecture-invariants.md | 6 +- docs/deepseek-integration-plan.md | 4 +- docs/deepseek-integration.md | 33 +++++++- src/session.ts | 14 ++++ src/web/routes/approval-routes.ts | 4 +- src/web/routes/hook-event-routes.ts | 6 +- src/web/routes/readmymind-routes.ts | 7 +- src/web/routes/session-routes.ts | 62 +++++++++++++- src/web/routes/system-routes.ts | 85 ++++++++++++++++--- src/web/server.ts | 7 +- src/web/session-wait-registry.ts | 83 ++++++++++++++++--- test/deepseek-mode.test.ts | 123 +++++++++++++++++++++++++++- 13 files changed, 391 insertions(+), 45 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 240f87dc..32aa8449 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -205,7 +205,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Docker cases**: a case can point at a **container**, with any of the CLI run modes running inside it. Like remote-SSH this is a **LOCATION OVERLAY on cases, never a `SessionMode` of its own**. Exactly one long-lived container **per case**, shared by all its sessions, so killing a session kills only that session's in-container tmux and **never** `docker stop` while siblings remain. The workspace is a real host dir bind-mounted at the **same absolute path**, which is what keeps file-routes/watchers on real host bytes and makes the in-container transcript projHash match the host. Credentials are **seeded** (RO mount, copied into the container once) rather than shared RW, so in-container CLIs never write refreshed tokens back to the host, and bind mounts are excluded from `docker commit` so exports stay secret-free. **NEVER a create-time `-e` for secrets, NEVER `--privileged`, NEVER the docker socket.** Config drift is detected via a label hash and a drifted launch is REFUSED rather than silently launched with stale config. ⚠️ On the loopback-only prod bind a container cannot reach 127.0.0.1, so in-container hooks need `CODEMAN_DOCKER_BRIDGE_HOOKS=1`; otherwise idle detection falls back to output-based. → [architecture-invariants#docker-cases](docs/architecture-invariants.md#docker-cases), `docs/docker-cases.md` (user guide), `docs/docker-cases-plan.md` (design) -**External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token/CLI-info parsing, ❯-prompt readiness); these CLIs render their own TUIs, so readiness is output stabilization instead. All seven **require tmux with no direct PTY fallback**, because secrets are injected via socket-scoped `tmux setenv` and never on the spawn command line. ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope; reading the raw shape silently breaks the run. ⚠️ **Codex sessions use PREDICTIVE WRITE-THROUGH echo, never the buffer overlay** (`_localEchoPolicy` in `_updateLocalEchoState`, terminal-ui.js): codex's composer reacts per keystroke ("/" pops a live-filtering picker, arrows edit server-side state, the composer grows as it wraps), so buffer-until-Enter starved it into issues #218/#219/#220/#222 and stays disabled (`_localEchoEnabled` remains false for codex). Instead, `PredictiveEchoAddon` (separate `vendor/xterm-predictive-echo.js` bundle) paints each keystroke at the predicted cell while the wire path stays BYTE-IDENTICAL: the onData hook (`_predictHookOnData`) is a plain statement with no `return`, so control always falls through into the untouched send path — pinned by vm and E2E byte-identity tests. Predictions reconcile against the parsed buffer and only while the cursor sits on the measured composer row (`isCodexComposerRow`, `/^› /`). Codex also **drops keystrokes that share a PTY read with a bracketed paste**, so flushed text and the paste sequence must go out as separate delayed writes (mirroring the Enter branch's delayed `\r`). Tests: `test/local-echo-codex-gating.test.ts`, `test/codex-predictive-echo.test.ts` (E2E vs real codex), `packages/xterm-zerolag-input/test/codex-replay.test.ts`. ⚠️ **Pi is the opposite kind of CLI and needs the opposite instincts**: it has NO permission prompts and no sandbox, so there is no bypass flag to send and Codeman must not invent one; its privileged knob is the tri-state `approveProjectTrust` (`--approve`/`--no-approve`), which makes pi EXECUTE repo-local `.pi/extensions` TypeScript, so the multi-user clamp puts pi in the **materialize** branch (an absent config still yields `--no-approve` for a non-granted owner) and `--api-key` is never wired. Pi stays OUT of `isAltScreenStripMode()` (main-screen TUI, and its 0.84.0 fullscreen mode is runtime-switchable via `/settings`, where the alt screen is load-bearing), and lands on the `'buffer'` echo policy via the `_updateLocalEchoState` fallthrough. Pi's own tests: `test/pi-mode.test.ts`, `test/routes/external-cli-bypass-clamp.test.ts`; user guide `docs/pi-integration.md`. ⚠️ **Grok is codex-shaped on permissions but opencode-shaped on rendering**: its bypass switch is `alwaysApprove` (`--always-approve`, grok's `bypassPermissions` mode — the Run button sends it `true` like antigravity's, and the clamp's only-if-sent branch strips it for non-granted owners), while its fullscreen alt-screen TUI keeps it OUT of `isAltScreenStripMode()`; the resolver version-probes `grok --version` like pi's (npm squatters exist for the name — `GET /api/grok/status` surfaces path + version), and grok lands on the `'buffer'` echo policy via the fallthrough (UNMEASURED against a live authenticated session; if its composer turns out per-keystroke-reactive like codex, flip it to the `'off'` branch). Grok's own tests: `test/grok-mode.test.ts`, `test/grok-cli-resolver.test.ts`; user guide `docs/grok-integration.md`. ⚠️ **DeepSeek breaks three of this family's assumptions, so do not pattern-match it onto its siblings.** (1) The agent is a **PROFILE, not the binary**: `dsh` is a launcher over `$DSH_HOME/profiles/` and DeepSeek ships only `web`/`headless`/`base`, so the terminal front door is ALWAYS third-party and "installed" ≠ "runnable" — the Run button gates on `isDeepSeekRunnable()` (binary AND a pane-capable profile) while `isDeepSeekAvailable()` gates the "add a profile" affordance; a `web`/`headless` profile is refused at spawn because it cannot drive a pane. (2) The permission switch is the **`DSH_PERMISSION_MODE` env export, not a flag** (`read-only`/`workspace-write`/`danger-full-access`) — the harness has none, and this is the one legitimate exception to the effort-style env-var ban because it is read with `??` as a boot-time default, so it stays soft; absent = `workspace-write`, which asks, hence the only-if-sent clamp branch, clamping to `workspace-write` (never `read-only`, which would break the workspace). (3) It is the **only non-claude mode that passes `hooksAvailableForMode()`**, because the terminal front door reports idle/working/blocked to a supervisor over a generic env-gated contract and `deepseek-status-shim.ts` makes Codeman that supervisor — real `stop`/`blocked` signals, real Approvals Inbox items, plus the `agent_working` event that clears an alert answered in the terminal. ⚠️ The resolver needs the strictest identity probe of the family (`dsh --help` must say `DeepSeek Harness`) because Debian ships an unrelated `dsh` (dancer's shell) that would pass a version probe. Model is NOT a session field (it is a profile composition entry). DeepSeek's own tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`; user guide `docs/deepseek-integration.md`. → [architecture-invariants#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek](docs/architecture-invariants.md#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek) +**External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek)**: `isExternalCliMode()` in `session.ts` gates Claude-specific behavior off (Ralph tracker, BashToolParser, token/CLI-info parsing, ❯-prompt readiness); these CLIs render their own TUIs, so readiness is output stabilization instead. All seven **require tmux with no direct PTY fallback**, because secrets are injected via socket-scoped `tmux setenv` and never on the spawn command line. ⚠️ `run*()` in `session-ui.js` MUST unwrap the `{success,data}` envelope; reading the raw shape silently breaks the run. ⚠️ **Codex sessions use PREDICTIVE WRITE-THROUGH echo, never the buffer overlay** (`_localEchoPolicy` in `_updateLocalEchoState`, terminal-ui.js): codex's composer reacts per keystroke ("/" pops a live-filtering picker, arrows edit server-side state, the composer grows as it wraps), so buffer-until-Enter starved it into issues #218/#219/#220/#222 and stays disabled (`_localEchoEnabled` remains false for codex). Instead, `PredictiveEchoAddon` (separate `vendor/xterm-predictive-echo.js` bundle) paints each keystroke at the predicted cell while the wire path stays BYTE-IDENTICAL: the onData hook (`_predictHookOnData`) is a plain statement with no `return`, so control always falls through into the untouched send path — pinned by vm and E2E byte-identity tests. Predictions reconcile against the parsed buffer and only while the cursor sits on the measured composer row (`isCodexComposerRow`, `/^› /`). Codex also **drops keystrokes that share a PTY read with a bracketed paste**, so flushed text and the paste sequence must go out as separate delayed writes (mirroring the Enter branch's delayed `\r`). Tests: `test/local-echo-codex-gating.test.ts`, `test/codex-predictive-echo.test.ts` (E2E vs real codex), `packages/xterm-zerolag-input/test/codex-replay.test.ts`. ⚠️ **Pi is the opposite kind of CLI and needs the opposite instincts**: it has NO permission prompts and no sandbox, so there is no bypass flag to send and Codeman must not invent one; its privileged knob is the tri-state `approveProjectTrust` (`--approve`/`--no-approve`), which makes pi EXECUTE repo-local `.pi/extensions` TypeScript, so the multi-user clamp puts pi in the **materialize** branch (an absent config still yields `--no-approve` for a non-granted owner) and `--api-key` is never wired. Pi stays OUT of `isAltScreenStripMode()` (main-screen TUI, and its 0.84.0 fullscreen mode is runtime-switchable via `/settings`, where the alt screen is load-bearing), and lands on the `'buffer'` echo policy via the `_updateLocalEchoState` fallthrough. Pi's own tests: `test/pi-mode.test.ts`, `test/routes/external-cli-bypass-clamp.test.ts`; user guide `docs/pi-integration.md`. ⚠️ **Grok is codex-shaped on permissions but opencode-shaped on rendering**: its bypass switch is `alwaysApprove` (`--always-approve`, grok's `bypassPermissions` mode — the Run button sends it `true` like antigravity's, and the clamp's only-if-sent branch strips it for non-granted owners), while its fullscreen alt-screen TUI keeps it OUT of `isAltScreenStripMode()`; the resolver version-probes `grok --version` like pi's (npm squatters exist for the name — `GET /api/grok/status` surfaces path + version), and grok lands on the `'buffer'` echo policy via the fallthrough (UNMEASURED against a live authenticated session; if its composer turns out per-keystroke-reactive like codex, flip it to the `'off'` branch). Grok's own tests: `test/grok-mode.test.ts`, `test/grok-cli-resolver.test.ts`; user guide `docs/grok-integration.md`. ⚠️ **DeepSeek breaks three of this family's assumptions, so do not pattern-match it onto its siblings.** (1) The agent is a **PROFILE, not the binary**: `dsh` is a launcher over `$DSH_HOME/profiles/` and DeepSeek ships only `web`/`headless`/`base`, so the terminal front door is ALWAYS third-party and "installed" ≠ "runnable" — the Run button gates on `isDeepSeekRunnable()` (binary AND a pane-capable profile) while `isDeepSeekAvailable()` gates the "add a profile" affordance; a `web`/`headless` profile is refused at spawn because it cannot drive a pane. (2) The permission switch is the **`DSH_PERMISSION_MODE` env export, not a flag** (`read-only`/`workspace-write`/`danger-full-access`) — the harness has none, and this is the one legitimate exception to the effort-style env-var ban because it is read with `??` as a boot-time default, so it stays soft; absent = `workspace-write`, which asks, hence the only-if-sent clamp branch, clamping to `workspace-write` (never `read-only`, which would break the workspace). ⚠️ **That clamp needs a second half no other CLI needs**, because the switch is an env var and `DSH_*` is an allowlisted `envOverrides` prefix: `applyEnvOverrides()` runs AFTER `_configureDeepSeek()` in tmux-manager, so a non-granted owner sending `DSH_PERMISSION_MODE` on the SAME request would land last and hand back exactly the privilege the config clamp removed. `clampEnvOverridesForOwner()` (session-routes.ts) DROPS `DSH_PERMISSION_MODE` and `DSH_HOME` for a non-granted owner (dropping falls through to what `_configureDeepSeek()` exports, which is the clamped value); `DSH_HOME` is there because it points the launcher at a profile tree whose plugin code runs at BOOT, before any approval row applies. Every OTHER CLI's bypass is a command-line flag reachable only through its config, which is why the config clamp alone is the whole gate for them. (3) It is the **only non-claude mode that passes `hooksAvailableForMode()`**, and for it alone that predicate is a per-SESSION question rather than a per-mode one (`deepSeekConfig.statusReporting: false` disarms the bridge, so every call site passes `sessionHookOptions(session)`; answering from the mode there re-creates the infinite-wait-dressed-as-a-timeout the guard exists to prevent). It passes because the terminal front door reports idle/working/blocked to a supervisor over a generic env-gated contract and `deepseek-status-shim.ts` makes Codeman that supervisor — real `stop`/`blocked` signals, real Approvals Inbox items, plus the `agent_working` event that clears an alert answered in the terminal. ⚠️ The resolver needs the strictest identity probe of the family (`dsh --help` must say `DeepSeek Harness`) because Debian ships an unrelated `dsh` (dancer's shell) that would pass a version probe. Model is NOT a session field (it is a profile composition entry). ⚠️ `hooksAvailableForMode()` is about hook SIGNALS and is not a stand-in for "is this a claude session": Read My Mind and intent capture read Claude's own transcript and compare `mode === 'claude'` directly, because when `deepseek` earned a yes the shared predicate silently widened both to a mode with no transcript to read (pinned by a static check in `test/deepseek-mode.test.ts`). DeepSeek's own tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`; user guide `docs/deepseek-integration.md`. → [architecture-invariants#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek](docs/architecture-invariants.md#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek) **Run launch synchronization**: the Run entrypoint holds an in-flight lock and disables `#runBtn` for the whole launch (≥500ms), so a double click cannot create duplicate sessions with the same `w-` name. `_ensureCreatedSessionVisible()` runs before `selectSession()`, and `_onSessionCreated()` stays an idempotent upsert, so POST-first and SSE-first ordering both produce exactly one rendered tab. ⚠️ **Closing has the mirror-image race and one owner**: `closeSession()` reads `wasActive` BEFORE its `await` and announces the delete via `_closingSessions`, while `_onSessionDeleted` skips the active-session handoff for an id in that set. Both used to read `activeSessionId` after the fact, so the `session_deleted` broadcast for your own delete could null it first and closing the tab you were on landed on the welcome screen instead of the next session, on the same build, depending on timing. The fallback also picks the first order entry that is still in `sessions` (a dead id can linger in `sessionOrder`, same reason Alt+N indexes a live-filtered list). A delete from ANOTHER client still shows the welcome screen, which is the honest answer when what you were looking at was taken away. Tests: `test/session-close-fallback.test.ts`. → [architecture-invariants#run-launch-synchronization](docs/architecture-invariants.md#run-launch-synchronization) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index d81ef2b1..fcebbbcc 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -26,15 +26,15 @@ Implementation detail extracted from `CLAUDE.md` so that file stays small enough ⚠️ **The agent is a PROFILE, not the binary.** `dsh` is a launcher over `$DSH_HOME/profiles/` (an ordered stack of plugin-bundle patch layers), and DeepSeek ships only `web` (browser UI), `headless` (one-shot) and `base` (no app). The interactive terminal front door is ALWAYS third-party. So availability is TWO questions, not one, and `isDeepSeekRunnable()` (binary AND a pane-capable profile) is what the Run button gates on while `isDeepSeekAvailable()` (binary only) gates the "add a profile" affordance and the web-UI shortcut. Reporting only the binary would let Run spawn a pane that dies on arrival, which is this mode's single most confusing failure. `buildDeepSeekCommand()` emits `dsh --profile [--resume [id]]`; an absent profile resolves through `resolveDefaultDeepSeekProfile()`, which prefers a recognized TUI, then an UNRECOGNIZED profile (third-party by construction — a classifier that has not heard of a bundle must not hide it), and refuses `web`/`headless`, which cannot drive a pane. -⚠️ **The permission switch is an ENV VAR, not a flag.** The harness has no `--dangerously-skip-permissions` equivalent; its sandbox/approval rows read `DSH_PERMISSION_MODE` with three presets (`read-only` / `workspace-write` / `danger-full-access`; measured from `dsh --dump-default-config`). It is exported via `tmux setenv` in `_configureDeepSeek()`, never on the command line, and `test/deepseek-mode.test.ts` pins that nothing permission-shaped ever reaches the spawn line. This is the ONE place a Codeman env export is the right mechanism rather than the forbidden one: unlike `CLAUDE_CODE_EFFORT_LEVEL` (which hard-locks in-session `/effort`), the harness reads it with `??` as a boot-time DEFAULT, so it stays soft. Absent = `workspace-write`, which still asks, so the multi-user clamp is the only-if-sent branch (codex/antigravity/grok shape, not pi's materialize) — and it clamps down to `workspace-write`, NOT `read-only`, because the clamp removes privilege without breaking a session's ability to edit its own workspace. +⚠️ **The permission switch is an ENV VAR, not a flag.** The harness has no `--dangerously-skip-permissions` equivalent; its sandbox/approval rows read `DSH_PERMISSION_MODE` with three presets (`read-only` / `workspace-write` / `danger-full-access`; measured from `dsh --dump-default-config`). It is exported via `tmux setenv` in `_configureDeepSeek()`, never on the command line, and `test/deepseek-mode.test.ts` pins that nothing permission-shaped ever reaches the spawn line. This is the ONE place a Codeman env export is the right mechanism rather than the forbidden one: unlike `CLAUDE_CODE_EFFORT_LEVEL` (which hard-locks in-session `/effort`), the harness reads it with `??` as a boot-time DEFAULT, so it stays soft. Absent = `workspace-write`, which still asks, so the multi-user clamp is the only-if-sent branch (codex/antigravity/grok shape, not pi's materialize) — and it clamps down to `workspace-write`, NOT `read-only`, because the clamp removes privilege without breaking a session's ability to edit its own workspace. ⚠️ **Clamping the config is only HALF the gate here, and this is the only CLI where that is true.** Every sibling's bypass is a command-line flag, reachable only through the per-CLI config `clampExternalCliBypassForOwner()` already owns. DeepSeek's is an env var, `DSH_*` is an allowlisted `envOverrides` prefix (it must be — that is also how the harness's ordinary knobs are set), and `applyEnvOverrides()` runs AFTER `_configureDeepSeek()` in tmux-manager, so `envOverrides: {DSH_PERMISSION_MODE: 'danger-full-access'}` sent on the SAME request as a clamped config lands last and wins. `clampEnvOverridesForOwner()` (session-routes.ts, exported as `_clampEnvOverridesForOwner` for tests) DROPS `DSH_PERMISSION_MODE` and `DSH_HOME` for a non-granted owner rather than rewriting them, since dropping falls through to what `_configureDeepSeek()` exports, which is already the clamped value. `DSH_HOME` is on that list because it aims the launcher at a profile tree and a profile's plugin code executes at BOOT, before any approval row can apply — the wider of the two holes. No-op in single-user mode and for a granted owner, like every other clamp. -⚠️ **It is the only non-claude mode that passes `hooksAvailableForMode()`, and it earned that.** The community terminal front door reports its own lifecycle to a supervising process through a generic env-var-gated contract inherited from Herdr: with `HERDR_ENV=1` + `HERDR_BIN_PATH` + `HERDR_PANE_ID` set it shells out ` pane report-agent --state idle|working|blocked …` on every state change and treats exit 0 as delivered. `deepseek-status-shim.ts` GENERATES a small script into the data dir (like `self-update-runner.sh`, so npm installs and git clones behave alike) and points `HERDR_BIN_PATH` at it; it forwards to `POST /api/hook-event` as `idle→stop`, `blocked→permission_prompt`, `working→agent_working`. So a dsh session gets real respawn triggers, real `wait` stop/blocked signals and real Approvals Inbox items instead of output-stabilization guesswork. This is an interface implementation, not an impersonation — no real `herdr` binary is ever executed. A TUI that does not implement the contract simply never calls the shim and falls back to stabilization, so the feature is inert rather than harmful there. +⚠️ **It is the only non-claude mode that passes `hooksAvailableForMode()`, and it earned that.** The community terminal front door reports its own lifecycle to a supervising process through a generic env-var-gated contract inherited from Herdr: with `HERDR_ENV=1` + `HERDR_BIN_PATH` + `HERDR_PANE_ID` set it shells out ` pane report-agent --state idle|working|blocked …` on every state change and treats exit 0 as delivered. `deepseek-status-shim.ts` GENERATES a small script into the data dir (like `self-update-runner.sh`, so npm installs and git clones behave alike) and points `HERDR_BIN_PATH` at it; it forwards to `POST /api/hook-event` as `idle→stop`, `blocked→permission_prompt`, `working→agent_working`. So a dsh session gets real respawn triggers, real `wait` stop/blocked signals and real Approvals Inbox items instead of output-stabilization guesswork. This is an interface implementation, not an impersonation — no real `herdr` binary is ever executed. A TUI that does not implement the contract simply never calls the shim and falls back to stabilization, so the feature is inert rather than harmful there. ⚠️ **For deepseek alone, `hooksAvailableForMode()` is a per-SESSION question**, which is why it takes a `HookCapabilityOptions` second argument and every call site passes `sessionHookOptions(session)`: `deepSeekConfig.statusReporting: false` skips the `HERDR_*` export, and that triple is the only reason a dsh session posts anything, so answering from the mode alone would accept `until=stop` on a session where nothing can ever send one — the infinite-wait-dressed-as-a-timeout the predicate exists to prevent. The option defaults permissive (`!== false`), so a call site that forgets it degrades to the old behaviour instead of 400ing a working session. ⚠️ Profile conformance is the LIMIT of what is knowable at request time: `resolveDefaultDeepSeekProfile()` deliberately treats an unrecognized profile as launchable, so a non-conforming TUI still answers true and still times out on an explicit `stop` — which is why the DEFAULT signal set keeps `idle`/`exit`. ⚠️ **The predicate is not a stand-in for "is this a claude session"**, though it read like one while `claude` was the only true answer: Read My Mind (`POST /api/sessions/:id/readmymind`) and intent capture (`captureIntentPrompt`) read Claude's own transcript and were silently widened to deepseek by this change, so both compare `mode === 'claude'` directly and a static check in `test/deepseek-mode.test.ts` keeps them there. ⚠️ **`agent_working` is a hook event with no Claude Code hook behind it** (157th SSE constant). It exists because a harness turn cannot run while one of its own modal approvals is on screen, so "the agent started working" proves a dialog was answered in the terminal. It joins `APPROVAL_RESOLVING_EVENTS`; without it a dsh session's red alert would survive until the next `stop`, the exact stuck-alert bug the claude path already had to fix once — and the pane-capture staleness sweep that fixed it there is Claude-dialog-shaped and cannot help here. ⚠️ **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. 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). 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/docs/deepseek-integration-plan.md b/docs/deepseek-integration-plan.md index af7f6544..f1c294ae 100644 --- a/docs/deepseek-integration-plan.md +++ b/docs/deepseek-integration-plan.md @@ -31,7 +31,9 @@ in the six external CLIs before it. | What does a pane run? | `dsh --profile `, profile discovered | **The decision that shapes everything else.** DeepSeek ships `web`, `headless` and `base` — no terminal agent. The interactive front door is always a third-party plugin, so Codeman resolves a binary AND a profile inventory, and "available" means both. `resolveDefaultDeepSeekProfile()` prefers a recognized TUI, then an UNRECOGNIZED profile (anyone can publish an app bundle; a classifier that has not heard of one must not hide it), and refuses `web`/`headless`, which cannot occupy a pane. | | Which TUI? | none blessed; default for BOOTSTRAP only | `POST /api/deepseek/install-profile` defaults to `@deepseek-harness-tui/dsh-tui` (~27.5k weekly downloads, ~4x the next, MIT, and it speaks the status contract in §2.3), but accepts any npm name and the resolver never assumes that profile exists. Codeman offers a default; it does not pick a winner. | | Permission bypass | `DSH_PERMISSION_MODE` env export, no flag | The harness has NO command-line permission option; its sandbox/approval rows read one env var with three presets (`read-only` / `workspace-write` / `danger-full-access`, read off `dsh --dump-default-config`). This is the one legitimate exception to the `CLAUDE_CODE_EFFORT_LEVEL` ban: that var hard-locks in-session switching, whereas the harness reads this with `??` as a boot-time DEFAULT, so it stays soft. Exported via `tmux setenv`, never on the command line. The Run button sends `danger-full-access`, matching every sibling Run button. | -| Multi-user clamp branch | only-if-sent, clamped to `workspace-write` | Omitting the export leaves the harness on `workspace-write`, which still ASKS, so an absent config is already safe (the codex/antigravity/grok shape, not pi's materialize). Clamping to `workspace-write` rather than `read-only` is deliberate: the clamp removes privilege, it must not break a session's ability to edit its own workspace. | +| Multi-user clamp branch | only-if-sent, clamped to `workspace-write`, **plus an env-var half** | Omitting the export leaves the harness on `workspace-write`, which still ASKS, so an absent config is already safe (the codex/antigravity/grok shape, not pi's materialize). Clamping to `workspace-write` rather than `read-only` is deliberate: the clamp removes privilege, it must not break a session's ability to edit its own workspace. ⚠️ Unlike every sibling, clamping the CONFIG is only half the gate: the switch is an env var, `DSH_*` is an allowlisted `envOverrides` prefix, and `applyEnvOverrides()` runs AFTER `_configureDeepSeek()`, so `envOverrides: {DSH_PERMISSION_MODE: 'danger-full-access'}` on the same request would land last and win. `clampEnvOverridesForOwner()` drops `DSH_PERMISSION_MODE` and `DSH_HOME` for a non-granted owner (dropping falls through to the clamped export). `DSH_HOME` because it aims the launcher at a profile tree whose plugin code runs at BOOT, before any approval row. | +| `hooksAvailableForMode()` granularity | per SESSION for deepseek, per mode for everything else | `deepSeekConfig.statusReporting: false` disarms the `HERDR_*` export, and the triple is the only reason a dsh session posts anything, so a mode-only answer would accept `until=stop` where nothing can send one — the infinite-wait the predicate exists to prevent. Call sites pass `sessionHookOptions(session)`; the default stays permissive so a forgotten one degrades to the old behaviour. ⚠️ Profile conformance stays unknowable at request time (an unrecognized profile is deliberately launchable), so a non-conforming TUI still times out on an explicit `stop`; the default set keeps `idle`/`exit` for that. ⚠️ The predicate is NOT "is this claude": Read My Mind and intent capture read Claude's transcript and were silently widened by this change, so they compare `mode === 'claude'` directly now. | +| Profile install spawn | own process group, hand-rolled timeout | `dsh plugin add` fans out into package-manager children, and spawn's built-in `timeout` signals only the direct child: survivors keep the inherited stdio pipes open, `close` never fires, and the held-open request leaks with no route-level deadline. `detached: true` + negative-pid SIGTERM→SIGKILL, the same escalation `runGit()` uses for the same reason, plus a last-resort reap for a grandchild that escaped the group. | | Idle detection | **real hook events via a status shim** | The standout decision. The TUI already reports its lifecycle to a supervising process through a generic env-gated contract inherited from Herdr: `HERDR_ENV=1` + `HERDR_BIN_PATH` + `HERDR_PANE_ID` make it run ` pane report-agent --state idle\|working\|blocked …` on every state change, exit 0 = delivered. `deepseek-status-shim.ts` generates a script into the data dir and points `HERDR_BIN_PATH` at it. So deepseek is the only non-claude mode that passes `hooksAvailableForMode()` — earned by emitting definitive signals, not granted. An interface implementation, not an impersonation: no real `herdr` binary is ever executed, and a TUI that ignores the contract simply falls back to output stabilization. | | `agent_working` event | new, 157th SSE constant | The one hook event with no Claude Code hook behind it. A harness turn cannot run while its own modal approval is on screen, so "started working" proves a dialog was answered in the terminal. Without it a dsh red alert would survive until the next `stop` — the exact stuck-alert bug the claude path already fixed once, and its pane-capture staleness sweep is Claude-dialog-shaped and cannot help here. | | Resolver | identity probe THEN version probe | Strictest of the family, and not by preference. `dsh` is not merely a squattable npm name: Debian ships an unrelated `dsh` (dancer's shell, `apt install dsh`) which would answer a version probe convincingly and then be handed a spawn line. `dsh --help` must match `DeepSeek Harness` first. `DEEPSEEK_VERSION_REGEX` keeps the prerelease tail (`0.1.1-rc.2`), since truncating it would report an rc as a release. | diff --git a/docs/deepseek-integration.md b/docs/deepseek-integration.md index 2e941d56..7b97ccc9 100644 --- a/docs/deepseek-integration.md +++ b/docs/deepseek-integration.md @@ -62,7 +62,9 @@ curl -sX POST localhost:3000/api/deepseek/install-profile \ Installing a plugin is arbitrary code execution on the host, so in multi-user mode this endpoint requires the can-bypass-permissions grant (the same bar as a -`shell` session). +`shell` session). The request is held open while the package manager runs and is +bounded at five minutes; the install runs in its own process group, so hitting +that bound kills the whole tree rather than just the launcher. > **`dsh` is also a Debian program.** `apt install dsh` gives you "dancer's > shell", a distributed shell, which would answer `--version` convincingly. @@ -93,6 +95,23 @@ non-granted owner's `danger-full-access` becomes `workspace-write`, not `read-only`: the clamp removes privilege without breaking the session's ability to edit its own workspace. +Because the switch is an env var rather than a flag, that clamp has a second half +no other CLI needs. `DSH_*` is an allowlisted `envOverrides` prefix (it has to be: +that is also how you set the harness's ordinary knobs), and env overrides are +applied *after* the permission export, so in multi-user mode a non-granted owner +sending + +```json +{ "mode": "deepseek", "envOverrides": { "DSH_PERMISSION_MODE": "danger-full-access" } } +``` + +would otherwise hand back the privilege the config clamp just removed. For a +non-granted owner Codeman therefore **drops `DSH_PERMISSION_MODE` and `DSH_HOME` +from `envOverrides`**; dropping them falls through to the clamped config and the +server's own `DSH_HOME`. `DSH_HOME` is in that list because it points the +launcher at a profile tree, and a profile's plugin code runs at boot, before any +approval row can apply. Single-user installs and granted owners are unaffected. + ## 3. Real idle detection (the interesting part) Every other external CLI mode in Codeman is **readiness-guessed**: Codeman @@ -125,6 +144,18 @@ So a DeepSeek session gets Claude-grade signals: `GET /api/sessions/:id/wait` really can block on `stop` and `blocked` for it, and it is the only non-Claude mode for which that is true (`hooksAvailableForMode`). +That is a per-*session* answer, not a per-mode one. Turning the bridge off with +`deepSeekConfig.statusReporting: false` means nothing will ever post a hook event +for that session, so an explicit `until=stop` is refused up front (with a message +naming the setting) rather than blocking for your whole timeout. Omitting `until` +never fails: the hook-only signals are dropped from the default set and you still +get `idle` and `exit`. + +One limit worth knowing: whether the *profile* implements the contract cannot be +known at request time (Codeman deliberately treats an unrecognized profile as +launchable). A dsh session running a non-conforming TUI therefore still accepts +`until=stop` and will time out on it. `idle`/`exit` are the reliable pair there. + This is an interface implementation, not an impersonation — nothing on your machine executes a real `herdr` binary. If you use a terminal profile that does *not* implement the contract, the shim is simply never called and the mode falls diff --git a/src/session.ts b/src/session.ts index 6559a601..2e5e7440 100644 --- a/src/session.ts +++ b/src/session.ts @@ -893,6 +893,20 @@ export class Session extends EventEmitter { return this._remote; } + /** + * `deepSeekConfig.statusReporting` verbatim: `undefined` when the caller sent + * none (i.e. ON), `false` when the user disarmed the status bridge for this + * session. + * + * Exposed because whether a dsh session can deliver `stop`/`blocked` is a + * per-SESSION fact, not a per-mode one, and `hooksAvailableForMode()` is pure + * and holds no `Session` reference by design. Undefined for every other mode, + * where the flag is meaningless. + */ + get deepSeekStatusReporting(): boolean | undefined { + return this._deepSeekConfig?.statusReporting; + } + /** Owning username in multi-user mode, else undefined. */ get owner(): string | undefined { return this._owner; diff --git a/src/web/routes/approval-routes.ts b/src/web/routes/approval-routes.ts index 8ad06f4b..df536057 100644 --- a/src/web/routes/approval-routes.ts +++ b/src/web/routes/approval-routes.ts @@ -23,7 +23,7 @@ import { ApiErrorCode, createErrorResponse } from '../../types.js'; import { ApprovalAnswerSchema } from '../schemas.js'; import { parseBody, getAuthUser, canAccessOwned, findSessionOrFail } from '../route-helpers.js'; import { approvalInbox, type ApprovalItem } from '../approval-inbox.js'; -import { hooksAvailableForMode } from '../session-wait-registry.js'; +import { hooksAvailableForMode, sessionHookOptions } from '../session-wait-registry.js'; import type { SessionPort } from '../ports/index.js'; /** @@ -99,7 +99,7 @@ export function registerApprovalRoutes(app: FastifyInstance, ctx: SessionPort): // Throws 404 (not 403) for sessions the caller does not own, same // no-existence-leak rule as every other session route. const session = findSessionOrFail(ctx, item.sessionId, req); - if (!hooksAvailableForMode(session.mode)) { + if (!hooksAvailableForMode(session.mode, sessionHookOptions(session))) { return createErrorResponse(ApiErrorCode.CONFLICT, 'Session mode cannot have pending approvals'); } diff --git a/src/web/routes/hook-event-routes.ts b/src/web/routes/hook-event-routes.ts index 338e8459..bd754e72 100644 --- a/src/web/routes/hook-event-routes.ts +++ b/src/web/routes/hook-event-routes.ts @@ -13,7 +13,7 @@ import { HookEventSchema, isValidWorkingDir } from '../schemas.js'; import { sanitizeHookData, parseBody } from '../route-helpers.js'; import { persistDockerCaseClaudeSessionId } from '../../docker-hosts.js'; import { getDataDir } from '../../config/instance.js'; -import { sessionWaits, hooksAvailableForMode } from '../session-wait-registry.js'; +import { sessionWaits, hooksAvailableForMode, sessionHookOptions } from '../session-wait-registry.js'; import { approvalInbox, type ApprovalKind } from '../approval-inbox.js'; import type { SessionPort, EventPort, RespawnPort, ConfigPort, InfraPort } from '../ports/index.js'; @@ -59,7 +59,7 @@ export function registerHookEventRoutes( // could never legitimately emit one is now dropped instead of steering another // agent's control flow. const waitSession = ctx.sessions.get(sessionId); - if (waitSession && hooksAvailableForMode(waitSession.mode)) { + if (waitSession && hooksAvailableForMode(waitSession.mode, sessionHookOptions(waitSession))) { if (event === 'stop') { sessionWaits.notifySignal(sessionId, 'stop'); } else if (event === 'permission_prompt' || event === 'elicitation_dialog') { @@ -120,7 +120,7 @@ export function registerHookEventRoutes( // session that can never show one must not create an answerable item). let approvalId: string | undefined; const approvalKind = APPROVAL_KIND_BY_EVENT[event]; - if (session && hooksAvailableForMode(session.mode)) { + if (session && hooksAvailableForMode(session.mode, sessionHookOptions(session))) { if (approvalKind) { const toolInput = safeData.tool_input && typeof safeData.tool_input === 'object' diff --git a/src/web/routes/readmymind-routes.ts b/src/web/routes/readmymind-routes.ts index 25394f71..27a1da60 100644 --- a/src/web/routes/readmymind-routes.ts +++ b/src/web/routes/readmymind-routes.ts @@ -38,7 +38,6 @@ import { IntentGoalsSchema, ReadMyMindPredictSchema } from '../schemas.js'; import { parseBody, findSessionOrFail } from '../route-helpers.js'; import { intentStore } from '../../intent-store.js'; import { approvalInbox } from '../approval-inbox.js'; -import { hooksAvailableForMode } from '../session-wait-registry.js'; import { buildPredictionContext, type PredictionContextInputs } from '../../readmymind-context.js'; import { collectWorkspaceSignals, readTranscriptSignals } from '../../readmymind-collectors.js'; import { readMyMindPredictor } from '../../readmymind-predictor.js'; @@ -72,7 +71,11 @@ export function registerReadMyMindRoutes(app: FastifyInstance, ctx: SessionPort const body = parseBody(ReadMyMindPredictSchema, req.body ?? {}); const session = findSessionOrFail(ctx, id, req); - if (!hooksAvailableForMode(session.mode)) { + // `mode === 'claude'` directly, NOT hooksAvailableForMode(): that predicate + // answers "can this session deliver stop/blocked", and once `deepseek` earned + // a yes it silently widened this gate to a mode whose sessions have no Claude + // transcript for readTranscriptSignals() to read. + if (session.mode !== 'claude') { reply.code(400); return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Read My Mind predicts claude-mode sessions only'); } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index d0e766b2..1c84777a 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -54,6 +54,7 @@ import { TabLayoutValidationError } from '../../tab-layout.js'; import { sessionWaits, resolveWaitSignals, + sessionHookOptions, signalForStatus, WaitCapacityError, type WaitSignal, @@ -391,6 +392,55 @@ async function clampExternalCliBypassForOwner( /** Test hook: the clamp is the multi-user safety gate for the external CLIs' privileged flags. */ export const _clampExternalCliBypassForOwner = clampExternalCliBypassForOwner; +/** + * Env-var keys a non-granted owner must not be able to set, because each one + * hands back privilege the config clamp above just removed. + * + * Both are DeepSeek's, and both are reachable because `DSH_*` is an allowlisted + * `envOverrides` prefix (schemas.ts) — which it has to be, since that is also how + * a user configures the harness's non-privileged knobs. + * + * - `DSH_PERMISSION_MODE` IS the harness's permission switch. Every other CLI's + * bypass is a command-line FLAG, reachable only through the per-CLI config the + * clamp already owns; this one is an env var, so the config clamp alone is + * half a gate. + * - `DSH_HOME` points the launcher at a profile tree, and a profile's plugin code + * executes at BOOT, before any approval row can apply. A user who can write a + * workspace can put a profile in it, so this is the wider of the two. + */ +const OWNER_CLAMPED_ENV_KEYS = ['DSH_PERMISSION_MODE', 'DSH_HOME'] as const; + +/** + * Env-var half of the multi-user bypass clamp. + * + * `clampExternalCliBypassForOwner()` clamps the per-CLI CONFIG, and for every CLI + * but DeepSeek that is the whole story. Here it is not: `applyEnvOverrides()` runs + * AFTER `_configureDeepSeek()` in tmux-manager, so an override sent on the SAME + * request lands last and wins, and a non-granted owner could restore + * `danger-full-access` on the very request the config clamp downgraded. + * + * Keys are DROPPED rather than rewritten: dropping falls through to what + * `_configureDeepSeek()` exports, which is the clamped config and the server's own + * `DSH_HOME`, i.e. exactly the intended state. No-op in single-user mode and for a + * granted owner, like every other clamp here + * (`canUsernameRunPrivilegedCommands()` returns true when `!isMultiUserMode()`), + * and it returns the caller's own object untouched when there is nothing to strip. + */ +async function clampEnvOverridesForOwner( + owner: string | undefined, + envOverrides: Record | undefined +): Promise | undefined> { + if (!envOverrides) return envOverrides; + if (!OWNER_CLAMPED_ENV_KEYS.some((key) => key in envOverrides)) return envOverrides; + if (await canUsernameRunPrivilegedCommands(owner)) return envOverrides; + const clamped = { ...envOverrides }; + for (const key of OWNER_CLAMPED_ENV_KEYS) delete clamped[key]; + return clamped; +} + +/** Test hook: the env-var half of the same multi-user safety gate. */ +export const _clampEnvOverridesForOwner = clampEnvOverridesForOwner; + /** * Why a DeepSeek session cannot start, or null when it can. * @@ -1018,7 +1068,7 @@ export function registerSessionRoutes( grokConfig: mode === 'grok' ? gatedGrokConfig : undefined, deepSeekConfig: mode === 'deepseek' ? gatedDeepSeekConfig : undefined, resumeSessionId: validatedResumeId, - envOverrides: body.envOverrides, + envOverrides: await clampEnvOverridesForOwner(owner, body.envOverrides), effort: body.effort, tmuxHistoryLimit: terminalHistoryConfig.tmuxHistoryLimit, remote, @@ -1344,7 +1394,10 @@ export function registerSessionRoutes( wait === true || (typeof wait === 'string' && wait.trim().length > 0) || (Array.isArray(wait) && wait.length > 0); let until: readonly WaitSignal[] = []; if (wantsWait) { - const resolved = resolveWaitSignals(wait === true ? undefined : wait, { mode: session.mode }); + const resolved = resolveWaitSignals(wait === true ? undefined : wait, { + mode: session.mode, + ...sessionHookOptions(session), + }); if (resolved.error) return createErrorResponse(ApiErrorCode.INVALID_INPUT, resolved.error); until = resolved.until; } @@ -1521,7 +1574,7 @@ export function registerSessionRoutes( // Shared with the `wait` field on POST .../input: unknown token is a 400, // hook-only signals are rejected explicitly but dropped from the default. - const { until, error } = resolveWaitSignals(query.until, { mode: session.mode }); + const { until, error } = resolveWaitSignals(query.until, { mode: session.mode, ...sessionHookOptions(session) }); if (error) return createErrorResponse(ApiErrorCode.INVALID_INPUT, error); // The value actually applied after clamping, echoed below: a caller that asked @@ -3160,6 +3213,7 @@ export function registerSessionRoutes( deepSeekConfig ); const qsTerminalHistoryConfig = await ctx.getTerminalHistoryConfig(); + const qsGatedEnvOverrides = await clampEnvOverridesForOwner(owner, envOverrides); const session = new Session({ workingDir: resolvedCasePath, name: sessionName ? sessionName.slice(0, MAX_SESSION_NAME_LENGTH) : '', @@ -3178,7 +3232,7 @@ export function registerSessionRoutes( piConfig: mode === 'pi' ? qsGatedPiConfig : undefined, grokConfig: mode === 'grok' ? qsGatedGrokConfig : undefined, deepSeekConfig: mode === 'deepseek' ? qsGatedDeepSeekConfig : undefined, - envOverrides, + envOverrides: qsGatedEnvOverrides, effort, remote, docker, diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index ff83a0ea..7f906f36 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -530,7 +530,10 @@ export function registerSystemRoutes( // second command; // - the request is held open with a bounded timeout, mirroring the // synchronous-clone precedent in `POST /api/cases/clone` rather than - // introducing a job store for a once-per-install action. + // introducing a job store for a once-per-install action — and the bound is + // real, because the install runs in its own process GROUP and the timeout + // kills the whole tree (see the spawn below for why the built-in one is + // not enough). app.post('/api/deepseek/install-profile', async (req) => { const body = parseBody(DeepSeekInstallProfileSchema, req.body); if (isMultiUserMode() && !(await canUsernameRunPrivilegedCommands(getAuthUser(req).username))) { @@ -546,29 +549,87 @@ export function registerSystemRoutes( const profile = body.profile || DEEPSEEK_DEFAULT_PROFILE; const pkg = body.package || DEEPSEEK_DEFAULT_TUI_PACKAGE; - const result = await new Promise<{ code: number | null; output: string }>((resolve) => { - const child = spawn(join(dir, 'dsh'), ['plugin', '--profile', profile, 'add', pkg], { - stdio: ['ignore', 'pipe', 'pipe'], - timeout: DEEPSEEK_INSTALL_TIMEOUT_MS, - // dsh bundles its own package manager, so no system pnpm is required — - // but it still needs a HOME to resolve $DSH_HOME against. - env: process.env, - }); + const result = await new Promise<{ code: number | null; output: string; timedOut: boolean }>((resolve) => { + let child: ReturnType; + try { + child = spawn(join(dir, 'dsh'), ['plugin', '--profile', profile, 'add', pkg], { + stdio: ['ignore', 'pipe', 'pipe'], + // Own process group, and the timeout enforced by hand rather than by + // spawn's `timeout` option. A plugin install fans out into + // package-manager resolver/build children, and spawn's own timeout + // signals ONLY the direct child: the survivors keep the inherited stdio + // pipes open, `close` never fires, and this request hangs forever with + // no route-level deadline behind it. Same fan-out, same escalation and + // same negative-pid signal as runGit() in git-clone.ts, which is the + // synchronous-spawn precedent this endpoint is modelled on. + detached: true, + // dsh bundles its own package manager, so no system pnpm is required — + // but it still needs a HOME to resolve $DSH_HOME against. + env: process.env, + }); + } catch (err) { + resolve({ code: null, output: `spawn failed: ${getErrorMessage(err)}`, timedOut: false }); + return; + } + let output = ''; + let timedOut = false; + let settled = false; + let killTimer: NodeJS.Timeout | undefined; + let reapTimer: NodeJS.Timeout | undefined; + const capture = (chunk: Buffer) => { // Bounded: a package manager can emit megabytes of progress. if (output.length < 16_384) output += chunk.toString('utf-8'); }; child.stdout?.on('data', capture); child.stderr?.on('data', capture); - child.on('error', (err) => resolve({ code: null, output: `${output}\n${err.message}` })); - child.on('close', (code) => resolve({ code, output })); + + const killTree = (signal: NodeJS.Signals) => { + try { + if (child.pid) process.kill(-child.pid, signal); + } catch { + try { + child.kill(signal); + } catch { + /* already gone */ + } + } + }; + + const finish = (code: number | null) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (killTimer) clearTimeout(killTimer); + if (reapTimer) clearTimeout(reapTimer); + resolve({ code, output, timedOut }); + }; + + const timer = setTimeout(() => { + timedOut = true; + killTree('SIGTERM'); + killTimer = setTimeout(() => killTree('SIGKILL'), 3_000); + // Last resort: a grandchild that escaped the group (double-fork/setsid) + // can hold the pipes open past SIGKILL, and `close` would still never + // arrive. Answer the caller anyway rather than leaking the request. + reapTimer = setTimeout(() => finish(null), 8_000); + }, DEEPSEEK_INSTALL_TIMEOUT_MS); + + child.on('error', (err) => { + output = `${output}\n${err.message}`; + finish(null); + }); + child.on('close', (code) => finish(code)); }); if (result.code !== 0) { + const detail = result.timedOut + ? `timed out after ${Math.round(DEEPSEEK_INSTALL_TIMEOUT_MS / 1000)}s` + : result.output.slice(-1000).trim() || 'no output'; return createErrorResponse( ApiErrorCode.OPERATION_FAILED, - `Installing ${pkg} into profile "${profile}" failed: ${result.output.slice(-1000).trim() || 'no output'}` + `Installing ${pkg} into profile "${profile}" failed: ${detail}` ); } const { listDeepSeekProfiles, resolveDefaultDeepSeekProfile, isDeepSeekRunnable } = diff --git a/src/web/server.ts b/src/web/server.ts index 98e3bc80..f24fee63 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -91,7 +91,7 @@ import { attachSessionListeners, detachSessionListeners, } from './session-listener-wiring.js'; -import { sessionWaits, hooksAvailableForMode } from './session-wait-registry.js'; +import { sessionWaits } from './session-wait-registry.js'; import { intentStore } from '../intent-store.js'; import { AI_CHECK_MODEL } from '../config/ai-defaults.js'; import { approvalInbox } from './approval-inbox.js'; @@ -1093,7 +1093,10 @@ export class WebServer extends EventEmitter { */ private async captureIntentPrompt(sessionId: string, text: string): Promise { const session = this.sessions.get(sessionId); - if (!session || !hooksAvailableForMode(session.mode)) return; + // `mode === 'claude'` directly: the intent profile is fed from Claude's own + // transcript, so this is a claude question, not a hooks-available one (which + // `deepseek` now answers yes to). + if (!session || session.mode !== 'claude') return; try { const settings = await this.readSettings(); if (settings.readMyMindEnabled !== true) return; diff --git a/src/web/session-wait-registry.ts b/src/web/session-wait-registry.ts index cf7107e0..bf1f6c97 100644 --- a/src/web/session-wait-registry.ts +++ b/src/web/session-wait-registry.ts @@ -170,25 +170,72 @@ export function signalForStatus(status: SessionStatus): WaitSignal | null { /** Signals that arrive only via Claude Code hooks, so only `claude` mode can emit them. */ const HOOK_ONLY_SIGNALS: readonly WaitSignal[] = ['stop', 'blocked']; +/** Per-session facts that can turn a mode's hook capability OFF for one session. */ +export interface HookCapabilityOptions { + /** + * `deepSeekConfig.statusReporting`, verbatim (so `undefined` means "not sent", + * i.e. ON). `false` is the per-session opt-out that stops `_configureDeepSeek()` + * exporting the `HERDR_*` triple, which is the ONLY thing that makes a dsh + * session emit hook events at all. + */ + deepSeekStatusReporting?: boolean; +} + /** - * Whether a session in this mode ever POSTs Codeman hook events, and therefore - * whether `stop` / `blocked` can ever fire for it. + * Whether this session ever POSTs Codeman hook events, and therefore whether + * `stop` / `blocked` can ever fire for it. * - * True for `claude` and nothing else. The tempting predicate is + * `claude` always (Claude Code fires the hooks itself), `deepseek` when its + * status bridge is armed, nothing else. The tempting predicate is * `!isExternalCliMode(mode)`, and it is WRONG: that helper covers only * opencode/codex/gemini/antigravity, so `shell` falls through it — and a shell session * is a plain bash PTY with no Claude Code and no hooks installed. `until=stop` on one * was accepted and then blocked for the caller's whole timeout, which is precisely the * infinite-wait-dressed-as-a-timeout this guard exists to prevent. + * + * ⚠️ `deepseek` is a per-SESSION answer, not a per-mode one, which is why the + * options argument exists: `deepSeekConfig.statusReporting: false` disarms the + * bridge for one session, and answering from the mode alone re-creates the exact + * infinite-wait this guard is for. Every call site therefore passes the session's + * own flag; the default stays permissive so a forgotten one degrades to the old + * behavior rather than 400ing a session that works. + * + * ⚠️ It is also the LIMIT of what can be known at request time. Whether the + * installed profile actually implements the supervisor contract is only + * observable once it reports, and `resolveDefaultDeepSeekProfile()` deliberately + * treats an unrecognized profile as launchable, so a dsh session running a + * non-conforming TUI still answers true here and still times out on an explicit + * `until=stop`. The default signal set keeps `idle`/`exit` for exactly that case. + * + * ⚠️ NOT a stand-in for "is this a claude session". It reads like one and it was + * used as one (Read My Mind, intent capture) until `deepseek` joined and silently + * widened both. Those sites compare `mode === 'claude'` directly now; ask this + * function only about hook SIGNALS. */ -export function hooksAvailableForMode(mode: SessionMode): boolean { +export function hooksAvailableForMode(mode: SessionMode, options: HookCapabilityOptions = {}): boolean { + if (mode === 'claude') return true; // `deepseek` earns this the same way `claude` does — by emitting DEFINITIVE // signals rather than having them inferred. The DeepSeek Harness terminal // front door reports idle/working/blocked to its supervisor, and Codeman is // that supervisor (see deepseek-status-shim.ts), so a dsh session really can - // deliver `stop` and `blocked`. Every other mode is output-stabilization - // guesswork and must keep failing the ask. - return mode === 'claude' || mode === 'deepseek'; + // deliver `stop` and `blocked` — unless the user turned the bridge off, in + // which case nothing on the box will ever post one. Every other mode is + // output-stabilization guesswork and must keep failing the ask. + if (mode === 'deepseek') return options.deepSeekStatusReporting !== false; + return false; +} + +/** + * Lift the per-session hook facts off a live session. + * + * Structurally typed on purpose: this module is pure and deliberately imports no + * `Session` (importing it would drag node-pty and the session layer into every + * consumer). One helper rather than an inline object literal at each of the four + * call sites, so a future per-session fact is added in one place instead of + * being forgotten at three of them. + */ +export function sessionHookOptions(session: { deepSeekStatusReporting?: boolean }): HookCapabilityOptions { + return { deepSeekStatusReporting: session.deepSeekStatusReporting }; } /** Outcome of resolving a caller-supplied wait target against a session's mode. */ @@ -212,10 +259,15 @@ export interface ResolvedWaitSignals { * not drift; the second-guessing that produces is worse than the duplication. * * @param raw - the caller's value (comma string, array, `true` for "the default") - * @param options - `mode` decides whether the hook-only signals are available, and - * names the mode in the error message so the caller can see why + * @param options - `mode` plus the per-session facts `hooksAvailableForMode()` needs + * (a dsh session with its status bridge disarmed emits no hooks even + * though the mode can). The mode also names itself in the error + * message so the caller can see why. */ -export function resolveWaitSignals(raw: unknown, options: { mode: SessionMode }): ResolvedWaitSignals { +export function resolveWaitSignals( + raw: unknown, + options: { mode: SessionMode } & HookCapabilityOptions +): ResolvedWaitSignals { const parsed = parseWaitSignals(raw); if (parsed.invalid.length > 0) { return { @@ -224,7 +276,7 @@ export function resolveWaitSignals(raw: unknown, options: { mode: SessionMode }) }; } - const unsupported = new Set(hooksAvailableForMode(options.mode) ? [] : HOOK_ONLY_SIGNALS); + const unsupported = new Set(hooksAvailableForMode(options.mode, options) ? [] : HOOK_ONLY_SIGNALS); if (parsed.signals.length === 0) { return { until: DEFAULT_WAIT_SIGNALS.filter((signal) => !unsupported.has(signal)), error: null }; @@ -234,7 +286,14 @@ export function resolveWaitSignals(raw: unknown, options: { mode: SessionMode }) if (rejected.length > 0) { return { until: [], - error: `Signal(s) ${rejected.join(', ')} never fire for ${options.mode} sessions (no Claude Code hooks). Use idle or exit.`, + // A dsh session is the one case where the mode is capable and THIS session + // is not, so saying "never fire for deepseek sessions" would send the + // caller looking for a bug that is really a setting they chose. + error: + options.mode === 'deepseek' + ? `Signal(s) ${rejected.join(', ')} never fire for this deepseek session: its status bridge is off ` + + `(deepSeekConfig.statusReporting: false), so nothing posts hook events. Use idle or exit.` + : `Signal(s) ${rejected.join(', ')} never fire for ${options.mode} sessions (no Claude Code hooks). Use idle or exit.`, }; } return { until: parsed.signals, error: null }; diff --git a/test/deepseek-mode.test.ts b/test/deepseek-mode.test.ts index 264129fa..1cdb7b71 100644 --- a/test/deepseek-mode.test.ts +++ b/test/deepseek-mode.test.ts @@ -16,9 +16,11 @@ 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 } from '../src/web/session-wait-registry.js'; -import { _clampExternalCliBypassForOwner } from '../src/web/routes/session-routes.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(); @@ -191,6 +193,62 @@ describe('DeepSeek status bridge', () => { } }); + 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'); @@ -238,3 +296,64 @@ describe('DeepSeek multi-user clamp', () => { 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'); + }); +});