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.
This commit is contained in:
Codeman maintainer
2026-08-24 16:01:02 +02:00
parent 4cda150493
commit 2034719d61
13 changed files with 391 additions and 45 deletions
+3 -1
View File
@@ -31,7 +31,9 @@ in the six external CLIs before it.
| What does a pane run? | `dsh --profile <name>`, 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 `<bin> pane report-agent <id> --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. |