diff --git a/CLAUDE.md b/CLAUDE.md index 1a6a9f24..e2ec2a95 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -223,7 +223,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Docker Compose deployment** (`docker/`, contributed): Codeman itself runs in a container and spawns Docker cases as **SIBLING** containers through the mounted host socket (Docker-outside-of-Docker), never nested. That inverts one assumption the bare-host path takes for granted: the daemon no longer shares Codeman's filesystem, so a bind source valid *inside* Codeman means nothing to it. `resolveDockerDaemonMountSource()` translates sources under HOME into the daemon's namespace via `CODEMAN_DOCKER_HOST_HOME`, and `CODEMAN_CASES_PATH` points the cases dir at a host-absolute bind mount so a workspace resolves to the SAME absolute path on both sides (which is what keeps the transcript projHash matching, per Docker cases above). ⚠️ **`CODEMAN_CASES_PATH` must move every consumer or none**: it is resolved once in `config/cases-dir.ts` because `src/cli.ts` resolves case paths too, and when only the server's `CASES_DIR` learned the override, `codeman skill install --case ` reported "Case not found" on exactly the deployment the override exists for. ⚠️ **`.dockerignore` patterns match the WHOLE context-relative path**, so a bare `.env` line excludes only the ROOT file: `docker/.env` (which holds `CODEMAN_PASSWORD` and any provider keys) rode `COPY . .` into the image until `**/.env` was added — verified in both directions with a real build context. ⚠️ A Compose LONG-form bind (`type: bind`) **creates a missing host source directory ROOT-OWNED** rather than refusing. `Start-Codeman.sh` pre-creates both `CODEMAN_APPDATA_PATH` and `CODEMAN_CASES_PATH` on the host before `up`, which is what keeps the daemon from ever having to materialise either as root in the first place; the container ALSO starts as root (`cap_add: [CHOWN, DAC_OVERRIDE, KILL, SETGID, SETUID]` against the base `cap_drop: ALL`; `test/docker-entrypoint.test.ts` pins that list) so `docker/entrypoint.sh` can correct a bind source that turns up root-owned anyway (a restored backup, a cleared directory, plain `docker compose up` run without the script) before dropping to `PUID:PGID` via `setpriv` — a directory owned by neither root nor `PUID:PGID` is never re-owned, since that ownership is not this container's to reassign; it is PROBED for writability as the runtime account (`setpriv ... test -w`, so ACLs, group-writable trees and CIFS/NFS mounts pass) and refused with a message naming path, owner and PUID:PGID if that fails. ⚠️ `KILL` is in that list for tini, not the entrypoint: `init: true` keeps tini as root while the server runs as PUID, and without CAP_KILL its SIGTERM forward fails and the server is SIGKILLed on every `compose down`/`restart` instead of flushing state. ⚠️ `/opt/codeman-cli` (the runtime-owned CLI prefix) is APPENDED to `PATH`, never prepended, and the entrypoint pins its own `PATH` to the system dirs: the root part of the start resolves `setpriv` by bare name, and a prefix ahead of `/usr/bin` let a planted `setpriv` run as uid 0 (measured). `CODEMAN_DOCKER_DISABLE_SWAP_LIMIT=1` drops `--memory-swap` (and filters only that one kernel warning) for hosts without swap accounting; `--memory` still applies. ⚠️ The deployment ALSO self-updates in place (the repo bind mount at `/opt/codeman` + a restart-by-exiting supervisor) — see Self-update below and `docs/docker-self-update.md` before touching `server.Dockerfile`, the compose file or `.env.example`, since each is an input to the updater's environment gate. `docs/docker-compose.md` + `docker/README.md` (user guides) -**CLI registry** (`src/config/cli-registry/`): every run mode is a `CliEntry` — discovery (search dirs, version + identity probes), the launch argv template, env handling, the `capabilities` flags that replace per-CLI branching, and the `overlays` that back the remote/docker pane commands. **No code outside `stock.ts` may branch on a CLI id**; behaviour that genuinely differs is either a capability field or a NAMED PROFILE selected by one (`profiles.ts`), and `test/cli-registry-no-id-branching.test.ts` fails the build if an id check reappears — it matches `===`, `!==`, `case '':` and `[...].includes(mode)`, because an earlier `===`-only version let 36 negated branches survive the conversion (including a seven-mode Ralph chain whose own comment asked the next person to keep it in step with `isExternalCliMode()` by hand). ⚠️ Config contains no shell text: an entry declares typed argv tokens, literals are validated against a safe-word pattern at LOAD time (a bad literal rejects the whole entry — a silently dropped `--no-approve` is not cosmetic), and values resolve through patterns NAMED in code, so a user `clis.json` cannot widen its own validation. ⚠️ `external`, `hooks` and `altScreen` are three INDEPENDENT capabilities on purpose; deriving one from another shipped the `until=stop`-hangs-on-shell bug. ⚠️ Two capability fields carry a REGEX from config (`discovery.version.regex` and `capabilities.workDetect.workingLine`) and both must compile through `compileVersionRegex()`, which caps length and refuses nested quantifiers; `workingLine` is the one that runs on the PTY hot path. ⚠️ **`param` is TWO namespaces.** `launch.params` keys, `env.configSetenv[].fromParam` and `capabilities.privilegedParams[].param` all name a LAUNCH PARAM; the legacy `Config` wire field is a separate namespace, bridged only by `launch.legacyConfigAliases`. Getting `privilegedParams[].param` wrong is SILENT — it is the multi-user bypass clamp's only handle on a CLI's privilege switch, and a wrong name clamps nothing with no load error and no failing test — so `schema.ts` rejects an entry naming a param it never declared. Codex is the entry where the two names differ (`bypassApprovals` vs `dangerouslyBypassApprovals`) and therefore the one that catches a regression. ⚠️ Six fields are DECLARED-FOR-LATER and read by nothing (`shortBadge`, `accent`, `capabilities.echo`/`wheelForward`/`keyboardAccessory`/`maxFrameBytes`): all frontend behaviour, transcribed rather than measured, so re-measure before wiring one up; the list is pinned so it cannot quietly grow. Spawn commands are pinned as literal strings in `test/cli-registry-spawn-golden.test.ts`, remote/docker pane commands in `test/location-overlay-commands.test.ts`. ⚠️ That second golden no longer covers **remote claude or remote omp**: both now have their own arm in `buildRemoteLaunchCommand` (a `--session-id || --resume` pair, and `--continue`, so a respawn continues the same conversation) and never reach `defaultRemoteCommandForMode`, which is what that test asserts. Their real pins are `toContain` substrings in `test/tmux-manager.test.ts` and `test/remote-shared-sessions.test.ts`; changing either arm will NOT fail the golden. ⚠️ Anything reading the registry resolves it AT CALL TIME (`sessionModeSchema()`, `allowedEnvPrefixes()`, `dependencyRegistry()`, the resolvers' `searchDirs` thunks) — a module-level const freezes at first import, so a CLI enabled while the server ran moved the run menu but not that surface. `~/.codeman/clis.json` overrides any entry (read-only in this release; nothing writes it, so importing the registry has no filesystem side effects). → `docs/cli-registry.md` +**CLI registry** (`src/config/cli-registry/`): every run mode is a `CliEntry` — discovery (search dirs, version + identity probes), the launch argv template, env handling, the `capabilities` flags that replace per-CLI branching, and the `overlays` that back the remote/docker pane commands. **No code outside `stock.ts` may branch on a CLI id**; behaviour that genuinely differs is either a capability field or a NAMED PROFILE selected by one (`profiles.ts`), and `test/cli-registry-no-id-branching.test.ts` fails the build if an id check reappears — it matches `===`, `!==`, `case '':` and `[...].includes(mode)`, because an earlier `===`-only version let 36 negated branches survive the conversion (including a seven-mode Ralph chain whose own comment asked the next person to keep it in step with `isExternalCliMode()` by hand). A second CI-gated guard, `test/frontend-cli-no-id-branching.test.ts`, covers the two frontend files the run-menu consolidation (#458) touched, `session-ui.js` and `mobile-overview.js`: its allowlist is keyed by expression rather than line number, with an occurrence count per entry, so a new branch reusing an already-approved expression fails as a count mismatch instead of riding in on the old approval. ⚠️ Config contains no shell text: an entry declares typed argv tokens, literals are validated against a safe-word pattern at LOAD time (a bad literal rejects the whole entry — a silently dropped `--no-approve` is not cosmetic), and values resolve through patterns NAMED in code, so a user `clis.json` cannot widen its own validation. ⚠️ `external`, `hooks` and `altScreen` are three INDEPENDENT capabilities on purpose; deriving one from another shipped the `until=stop`-hangs-on-shell bug. ⚠️ Two capability fields carry a REGEX from config (`discovery.version.regex` and `capabilities.workDetect.workingLine`) and both must compile through `compileVersionRegex()`, which caps length and refuses nested quantifiers; `workingLine` is the one that runs on the PTY hot path. ⚠️ **`param` is TWO namespaces.** `launch.params` keys, `env.configSetenv[].fromParam` and `capabilities.privilegedParams[].param` all name a LAUNCH PARAM; the legacy `Config` wire field is a separate namespace, bridged only by `launch.legacyConfigAliases`. Getting `privilegedParams[].param` wrong is SILENT — it is the multi-user bypass clamp's only handle on a CLI's privilege switch, and a wrong name clamps nothing with no load error and no failing test — so `schema.ts` rejects an entry naming a param it never declared. Codex is the entry where the two names differ (`bypassApprovals` vs `dangerouslyBypassApprovals`) and therefore the one that catches a regression. ⚠️ Six fields are DECLARED-FOR-LATER and read by nothing (`shortBadge`, `accent`, `capabilities.echo`/`wheelForward`/`keyboardAccessory`/`maxFrameBytes`): all frontend behaviour, transcribed rather than measured, so re-measure before wiring one up; the list is pinned so it cannot quietly grow. Spawn commands are pinned as literal strings in `test/cli-registry-spawn-golden.test.ts`, remote/docker pane commands in `test/location-overlay-commands.test.ts`. ⚠️ That second golden no longer covers **remote claude or remote omp**: both now have their own arm in `buildRemoteLaunchCommand` (a `--session-id || --resume` pair, and `--continue`, so a respawn continues the same conversation) and never reach `defaultRemoteCommandForMode`, which is what that test asserts. Their real pins are `toContain` substrings in `test/tmux-manager.test.ts` and `test/remote-shared-sessions.test.ts`; changing either arm will NOT fail the golden. ⚠️ Anything reading the registry resolves it AT CALL TIME (`sessionModeSchema()`, `allowedEnvPrefixes()`, `dependencyRegistry()`, the resolvers' `searchDirs` thunks) — a module-level const freezes at first import, so a CLI enabled while the server ran moved the run menu but not that surface. `~/.codeman/clis.json` overrides any entry (read-only in this release; nothing writes it, so importing the registry has no filesystem side effects). → `docs/cli-registry.md` **External CLI modes (OpenCode, Codex, Gemini, Antigravity, Pi, Grok, DeepSeek, OMP)**: `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. ⚠️ **Work detection is no longer part of that gate**: it is per-CLI `capabilities.workDetect` data (see the ❯ note above), so a CLI that declares its own glyph and working line gets the same screen-probed idle confirmation claude gets, and one that declares neither keeps the output-stabilization behaviour. All eight **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 `_configureCliEnv()` 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`, `DSH_HOME` and `DEEPSEEK_BASE_URL` for a non-granted owner (the last because `_configureCliEnv()` forwards the SERVER's own `DEEPSEEK_API_KEY` into the pane, so a redirected base URL would send it to a foreign host) (dropping falls through to what `_configureCliEnv()` 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`). ⚠️ **It is also the only external CLI whose answers are READ FROM DISK rather than scraped off the pane**: `deepseek-transcript.ts` reads `$DSH_HOME/sessions///session.jsonl.zstd` and backs the `last-response` route for dsh, because the pane segmenter served dsh-TUI's ASCII-art SPLASH as the worker's answer (measured), which anything polling for a first answer reads as an answer. Three traps live in that file: dsh appends **one zstd FRAME per write** and Node's `zlib` zstd decoder stops at the first (a real 56-line transcript decoded as 1 line, so the module walks frame headers itself; a Node older than 22.15 has no zstd and falls back to the pane); every turn also records a **plugin-sourced `user/message`** (the runtime-context snapshot) that must not render as the user's words; and a failed `turn/end` is surfaced as `Turn error: …` rather than as an empty string that reads as "still thinking". ⚠️ Session→transcript pairing is by the header's own `cwd` plus a ±60 s boot window, never by reproducing dsh's directory mangling (which has already changed form once) — and NEVER by newest-mtime alone, which handed a fresh worker its predecessor's answer in the same case dir. DeepSeek's own tests: `test/deepseek-mode.test.ts`, `test/deepseek-cli-resolver.test.ts`, `test/deepseek-transcript.test.ts`; user guide `docs/deepseek-integration.md`. OMP (`omp`) needs no bypass flag (the CLI's own `~/.omp` config governs trust/model routing, defaulting to `tools.approvalMode: yolo`), so its registry entry declares `privilegedParams: []` and a launch spec that only ever passes `--model`/`--resume`/`--continue` — but the multi-user clamp is NOT a no-op for it: `OMP_*` is an allowlisted `envOverrides` prefix, and the two credential-resolution keys it admits, `OMP_AUTH_BROKER_URL`/`OMP_AUTH_BROKER_TOKEN`, are clamped in `clampEnvOverridesForOwner()` for a non-granted owner, the same shape as `DEEPSEEK_BASE_URL`. Separately, `PI_*` is already allowlisted (pi needs it) and omp reads several of its knobs too (`PI_CONFIG_DIR`, `PI_CODING_AGENT_DIR`, `PI_CODING_AGENT_SESSION_DIR`, `PI_SUBPROCESS_CMD`, `PI_SHELL_PREFIX`) — a redirected `PI_CONFIG_DIR` moves the `~/.omp` tree `omp-session-resolver.ts`/`omp-transcript.ts` hardcode, silently breaking pinning/history; this is a known gap shared with pi, not fixed here. → [architecture-invariants#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp](docs/architecture-invariants.md#external-cli-modes-opencode-codex-gemini-antigravity-pi-grok-deepseek-omp) diff --git a/docs/cli-registry.md b/docs/cli-registry.md index c1a9d871..22168cea 100644 --- a/docs/cli-registry.md +++ b/docs/cli-registry.md @@ -98,12 +98,12 @@ DeepSeek is worth reading before assuming an entry looks like its siblings — t `test/cli-registry-no-id-branching.test.ts` fails the build if a CLI id comparison appears outside the stock catalog. It builds its id list from the live catalog, blanks comment lines before scanning (comments legitimately quote the pattern to explain why a branch was removed, and blanking rather than dropping is what keeps reported line numbers pointing at the real file), and keeps an allowlist in which **every entry carries its reason**. -`test/frontend-cli-no-id-branching.test.ts` is the same guard for the two frontend files the CLI registry's Run-menu consolidation touches, `session-ui.js` and `mobile-overview.js` — deliberately not the rest of `src/web/public/`, whose per-CLI rules stay out of scope for now (see "Fields declared for later" below). Its allowlist keys on `::` with no line number, since a single unrelated edit to a contended file would otherwise shift every subsequent line and make every entry go stale at once, and each entry additionally carries the exact number of approved call sites — a bare key would let a brand-new branch reusing an already-approved expression land unreviewed. - It matches four shapes, not one: `mode === ''`, `mode !== ''`, `case '':`, and `['', …].includes(mode)`. The first version matched `===` only, and that gap was not academic — the refactor it guards converted the `===` sites and left the negated ones, so 36 `!==` branches survived it, including a seven-mode chain auto-enabling Ralph under a comment asking the next person to keep it in step with a predicate by hand while the sibling code path already read the capability. A guard that sees half the shapes reports a count measured over the half it happens to catch. The allowlist is not a formality. If a branch is about what a CLI can DO it belongs in `CliCapabilities`; the entries that remain are things that are not CLI-behaviour branches at all — chiefly the legacy per-mode `Config` objects on `POST /api/sessions`, which are a fact about the public HTTP API rather than about any CLI, plus a few documented cases where `mode === 'claude'` is genuinely the right question (Read My Mind reads Claude's _own_ transcript, so a capability there would be actively wrong). +`test/frontend-cli-no-id-branching.test.ts` is the same guard for the two frontend files the CLI registry's Run-menu consolidation touches, `session-ui.js` and `mobile-overview.js` — deliberately not the rest of `src/web/public/`, whose per-CLI rules stay out of scope for now (see "Fields declared for later" below). Its allowlist keys on `::` with no line number, since a single unrelated edit to a contended file would otherwise shift every subsequent line and make every entry go stale at once, and each entry additionally carries the exact number of approved call sites — a bare key would let a brand-new branch reusing an already-approved expression land unreviewed. Its comparison shape differs from the backend guard's in one respect: the left-hand side may be any identifier, not only one named `mode`, `id` or `agentType`, because the review of #458 found `const m = this._runMode; if (m === 'codex')` slipping past the named form while the scanned file already filters with `(m) => m !== 'shell'`. + ## Two namespaces called `param` `launch.params` keys, `env.configSetenv[].fromParam` and `capabilities.privilegedParams[].param` all name a **launch param**. The **legacy wire field** a param arrives as is a separate namespace, and `launch.legacyConfigAliases` is the only bridge between the two. diff --git a/src/web/public/session-ui.js b/src/web/public/session-ui.js index 8bc72e88..5056cf9c 100644 --- a/src/web/public/session-ui.js +++ b/src/web/public/session-ui.js @@ -105,6 +105,11 @@ const RUN_MODE_LAUNCH = { // sibling Run button sends its bypass switch. The harness has no bypass // FLAG, so this rides the `DSH_PERMISSION_MODE` export instead, and the // multi-user clamp forces it back down to `workspace-write` server-side. + // + // `statusReporting` is deliberately LEFT UNSET, i.e. ON: it is what upgrades + // this mode from output-stabilization guessing to definitive idle/blocked + // hook events (the harness reports to Codeman as its supervisor, see + // deepseek-status-shim.ts). Never send `statusReporting: false` from here. buildConfig: () => ({ deepSeekConfig: { permissionMode: 'danger-full-access' } }), }, }; @@ -2157,6 +2162,10 @@ Object.assign(CodemanApp.prototype, { const globalSettings = this.loadAppSettingsFromStorage(); const envOverrides = this.buildEnvOverrides(this.getCaseSettings(caseName), globalSettings); + // No `effort` field for ANY entry in RUN_MODE_LAUNCH: effort is + // Claude-specific (runClaude() alone sends it, and the backend turns it + // into `claude --settings`); none of these CLIs has an /effort. Each of + // the eight bodies this launcher replaced carried that rule as a comment. const firstSessionId = await this._launchQuickStartInstances( caseName, tabCount, diff --git a/src/web/server.ts b/src/web/server.ts index f2f9b419..8d8f9368 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1581,9 +1581,17 @@ export class WebServer extends EventEmitter { // the /session/:id URL path; this global is a belt-and-suspenders fallback. // The id is gated to JSON + <-escaped so it can't break out of the inline // \n`); + html = html.replace('', () => `\n`); } // Gesture-control overlay (Phase 5): dashboard only (not solo popups, which // have no tab strip). `CODEMAN_GESTURE=1` makes the feature *available* on @@ -1655,7 +1663,7 @@ export class WebServer extends EventEmitter { }; html = html.replace( '', - `\n` + () => `\n` ); // Which run modes the Run-menu picker (docs/custom-model-endpoints-plan.md) may // generate an entry for: read generically off the registry's `capabilities` @@ -1672,16 +1680,19 @@ export class WebServer extends EventEmitter { const customModelClisJson = escapeScriptJson(JSON.stringify(customModelClis)); html = html.replace( '', - `\n` + () => `\n` ); } if (!soloSessionId && process.env.CODEMAN_GESTURE === '1') { - html = html.replace('', `\n`); + html = html.replace('', () => `\n`); if (settings.gestureControlEnabled === true) { const v = this.gestureBundleVersion(); // Relative src so the injected `` resolves it under the mount // prefix (a root-absolute `/gesture/...` would escape a sub-path mount). - html = html.replace('', `\n`); + html = html.replace( + '', + () => `\n` + ); } } return html; diff --git a/test/frontend-cli-no-id-branching.test.ts b/test/frontend-cli-no-id-branching.test.ts index 50b4deaa..758d83b7 100644 --- a/test/frontend-cli-no-id-branching.test.ts +++ b/test/frontend-cli-no-id-branching.test.ts @@ -82,6 +82,17 @@ const ALLOWED_BRANCHES: Record = { "session-ui.js::mode === 'deepseek'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, "session-ui.js::mode === 'omp'": { count: 2, reason: 'button-label ternary + runMode setter validity check' }, + // The docker adopt-preflight status line and the docker link/adopt toast + // both list the agent CLIs probed INSIDE the container and leave `shell` + // out of that human-readable "found ..." summary (it is always present and + // is not an agent CLI). Written as `(m) => m !== 'shell'`, the naming the + // original named-variable pattern could not see; the widened pattern + // normalizes the `m` to `mode` (see BRANCH_PATTERN below). + "session-ui.js::mode !== 'shell'": { + count: 2, + reason: 'display filter: the "CLIs found inside the container" summaries omit shell, which is not an agent CLI', + }, + // mobile-overview.js: shell is exempt from the isCliAvailable() gate the // same way the toolbar's #runModeMenu exempts it (shell needs no CLI). "mobile-overview.js::mode !== 'shell'": { @@ -94,11 +105,35 @@ const ALLOWED_BRANCHES: Record = { const IDS = STOCK_CLIS.map((e) => e.id as string); const ID_ALT = IDS.join('|'); -/** Same four shapes as the backend guard — see its own comment for why all four matter. */ +/** + * The backend guard's four shapes (see its own comment for why all four + * matter), with ONE deliberate widening on the first. + * + * The backend pattern accepts a comparison only when its left-hand side is + * literally named `mode`, `id` or `agentType`, so both + * `const m = this._runMode; if (m === 'codex')` and + * `if (this._runMode !== 'gemini')` slip past it, and `session-ui.js` already + * uses exactly that naming (`(m) => m !== 'shell'`, twice). The review of + * PR #458 surfaced that blind spot, so here the left-hand side is ANY + * identifier (`[\w$]+`, the leaf of a member chain), normalized to `mode` in + * the allowlist key by `scan()` so a local rename never churns the entries. + * Measured over both scanned files before widening: every extra hit was a + * genuine mode comparison (the two `m !== 'shell'` filters, allowlisted + * above), so the widening added no false positive; a future one gets an + * allowlist entry with its reason like any other. The backend guard keeps + * its narrower form and is deliberately not changed here. + * + * Still unseen, and worth knowing: a Yoda comparison (`'codex' === mode`), + * and an id list held in a variable (`EXTERNAL.includes(mode)`), since the + * third shape needs the literal list inline. + */ const BRANCH_PATTERN = new RegExp( [ - `\\b(?:mode|id|agentType)\\s*[!=]==\\s*'(?:${ID_ALT})'`, + // === 'codex' / !== 'codex' (any left-hand identifier, see above) + `\\b[\\w$]+\\s*[!=]==\\s*'(?:${ID_ALT})'`, + // case 'codex': `\\bcase\\s+'(?:${ID_ALT})'\\s*:`, + // ['codex', 'gemini'].includes(mode) — the id list IS the branch, wherever `mode` sits `'(?:${ID_ALT})'\\s*(?:,\\s*'(?:${ID_ALT})'\\s*)*\\]\\s*\\.includes\\(`, ].join('|'), 'g' @@ -126,7 +161,10 @@ function scan(): Finding[] { lines.forEach((line, i) => { BRANCH_PATTERN.lastIndex = 0; // shared /g regex — see utils/regex-patterns.ts for (const match of line.matchAll(BRANCH_PATTERN)) { - const expression = match[0].replace(/\s+/g, ' ').replace(/^(?:id|agentType)/, 'mode'); + // Normalize the comparison shape's left-hand identifier (whatever the + // local is called: `id`, `agentType`, `m`, `_runMode`) to `mode`; the + // lookahead leaves the `case`/`.includes(` shapes untouched. + const expression = match[0].replace(/\s+/g, ' ').replace(/^[\w$]+(?=\s*[!=]==)/, 'mode'); findings.push({ file, expression, line: i + 1, key: `${file}::${expression}` }); } }); @@ -162,6 +200,9 @@ describe('no NEW CLI-id branching in session-ui.js / mobile-overview.js (PR B2)' "if (mode !== 'shell' && mode !== 'deepseek') { doSomething(); }", "switch (mode) { case 'gemini': return 1; }", "if (['codex', 'gemini'].includes(mode)) { doSomething(); }", + // The two forms the named-variable pattern was blind to (see BRANCH_PATTERN). + "const m = this._runMode; if (m === 'codex') { doSomething(); }", + "if (this._runMode !== 'gemini') { doSomething(); }", ]; for (const sample of samples) { BRANCH_PATTERN.lastIndex = 0; diff --git a/test/opencode-resize.test.ts b/test/opencode-resize.test.ts index 5d75bc89..ed5540a5 100644 --- a/test/opencode-resize.test.ts +++ b/test/opencode-resize.test.ts @@ -68,23 +68,33 @@ describe('OpenCode session initial resize', () => { ({ context, page } = await freshPage()); await navigateAndWait(page); - const hasPreAssignment = await page.evaluate(() => { + const { selectIdx, assignIdx } = await page.evaluate(() => { const app = (window as unknown as { app: { _runCliMode: { toString: () => string } } }).app; const source = app._runCliMode.toString(); - // Check: the source should NOT have activeSessionId = ... before selectSession - // Find positions of both patterns - const assignIdx = source.indexOf('this.activeSessionId = data.sessionId'); - const selectIdx = source.indexOf('this.selectSession(data.sessionId)'); - - // If assign doesn't exist at all, that's the correct fix - if (assignIdx === -1) return false; - - // If assign comes before select, that's the bug - return assignIdx < selectIdx; + // The launcher hands the FIRST created session to selectSession + // (`_launchQuickStartInstances()` returns `firstSessionId`). An earlier + // version of this check looked for `this.selectSession(data.sessionId)`, + // a string that exists nowhere in session-ui.js, so both lookups came + // back -1 and the assertion could never fail. Hence the anti-vacuity + // check below: the select call itself must be found. + const selectIdx = source.indexOf('this.selectSession(firstSessionId)'); + // ANY assignment to activeSessionId (whatever the right-hand side is + // called), not `==`/`===` comparisons and not the comment that mentions + // pre-setting it without a `this.` prefix. + const assign = /this\.activeSessionId\s*=(?!=)/.exec(source); + return { selectIdx, assignIdx: assign ? assign.index : -1 }; }); - expect(hasPreAssignment).toBe(false); + // Anti-vacuity: if the select call is renamed again, fail here rather + // than pass on two -1s. + expect(selectIdx).toBeGreaterThan(-1); + // Correct: no assignment at all. Bug: an assignment that lands BEFORE + // selectSession runs, which makes selectSession early-return. + expect( + assignIdx === -1 || assignIdx > selectIdx, + `activeSessionId is assigned at ${assignIdx}, before selectSession at ${selectIdx}` + ).toBe(true); }); it('sends resize to server after creating a session via quick-start', async () => { diff --git a/test/render-index-html.test.ts b/test/render-index-html.test.ts index bc3de380..0fb7a321 100644 --- a/test/render-index-html.test.ts +++ b/test/render-index-html.test.ts @@ -23,6 +23,8 @@ import { isDeepSeekAvailable, isDeepSeekRunnable } from '../src/utils/deepseek-c import { isOmpAvailable } from '../src/utils/omp-cli-resolver.js'; import { isCloudflaredAvailable } from '../src/utils/cloudflared-resolver.js'; import { isGitAvailable } from '../src/git-clone.js'; +import { enabledClis } from '../src/config/cli-registry/registry.js'; +import { STOCK_CLIS } from '../src/config/cli-registry/stock.js'; // renderIndexHtml probes the real PATH for every CLI, which would make the // assertions below depend on whatever happens to be installed on the machine @@ -79,6 +81,14 @@ vi.mock('../src/utils/cloudflared-resolver.js', () => ({ vi.mock('../src/git-clone.js', () => ({ isGitAvailable: vi.fn(() => false), })); +// The custom-model list carries `label`, a string a user's own clis.json can set. +// Wrap enabledClis so ONE test below can hand renderIndexHtml a label with `$'` +// in it while every other test still reads the real stock registry through the +// real implementation. +vi.mock('../src/config/cli-registry/registry.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, enabledClis: vi.fn(actual.enabledClis) }; +}); const TEMPLATE = [ '', @@ -222,6 +232,35 @@ describe('WebServer.renderIndexHtml', () => { expect(eval(escaped)[0].label).toBe(''); }); + it("inserts a label containing $' verbatim instead of splicing the document into the script", async () => { + // `String.replace` with a STRING replacement interprets `$'` as "the text + // after the match", so a clis.json label carrying it used to re-inject the + // rest of the document (the whole ) into the inline script, past + // escapeScriptJson, which only neutralizes `<`. Every `` injection + // passes a replacer FUNCTION instead, whose return value is inserted + // verbatim. The other `$` forms ride along so a partial escape cannot pass. + const claude = STOCK_CLIS.find((e) => e.id === 'claude')!; + const label = "Claude $' $& $` $1 $$"; + const real = vi.mocked(enabledClis).getMockImplementation()!; + vi.mocked(enabledClis).mockImplementation(() => [{ ...claude, label }]); + try { + const { server } = makeServer({}); + const html = await render(server); + expect(html.match(//g)).toHaveLength(1); + const clis = JSON.parse(html.match(/window\.__codemanCustomModelClis=(\[.*?\]);/)![1]); + expect(clis).toEqual([{ id: 'claude', label }]); + } finally { + vi.mocked(enabledClis).mockImplementation(real); + } + }); + + it("inserts a solo id containing $' verbatim, under the same replacer rule", async () => { + const { server } = makeServer({}); + const html = await render(server, "sess$'x"); + expect(html.match(//g)).toHaveLength(1); + expect(html).toContain(`window.__CODEMAN_SOLO__="sess$'x"`); + }); + it('still emits the object when nothing at all is installed', async () => { // The all-false case is the one that matters most and the easiest to get // wrong by only injecting when something resolves. diff --git a/test/run-mode-dispatch.test.ts b/test/run-mode-dispatch.test.ts new file mode 100644 index 00000000..1afdf965 --- /dev/null +++ b/test/run-mode-dispatch.test.ts @@ -0,0 +1,147 @@ +/** + * @fileoverview Table-driven pin for `run()`'s dispatch in session-ui.js + * (PR #458). Before the run-menu consolidation, `run()` was an eight-arm + * `if (mode === 'codex') return this.runCodex(); ...` chain and each arm was + * pinned only by the name it called; after it, every non-claude, non-shell + * mode reaches ONE shared launcher, `_runCliMode(mode)`, gated on + * `EXTERNAL_CLI_MODES` (the key set of `RUN_MODE_LAUNCH`). Nothing pinned that + * gate: a mode dropped from the table would fall through to `runClaude()` and + * start a Claude session under a Codex label with no error, while an unknown + * mode reaching `_runCliMode()` would throw on `entry.label` of an undefined + * entry. + * + * The external ids are read off `RUN_MODE_LAUNCH` itself (same JSDOM + * extraction as test/run-mode-launch-table-drift.test.ts, which separately + * pins that key set against stock.ts), so a ninth CLI is covered the day it + * lands in the table. + * + * Port: none. + */ +import { readFileSync } from 'node:fs'; +import { JSDOM } from 'jsdom'; +import { describe, expect, it, vi } from 'vitest'; + +const SESSION_UI_JS = readFileSync(new URL('../src/web/public/session-ui.js', import.meta.url), 'utf-8'); + +interface HarnessApp { + _runMode?: string; + _runInFlight?: boolean; + _runMinLockMs?: number; + run: () => Promise; + runClaude: ReturnType; + runShell: ReturnType; + _runCliMode: ReturnType; +} + +interface Harness { + app: HarnessApp; + runBtn: HTMLButtonElement; + externalIds: string[]; +} + +function loadHarness(): Harness { + const dom = new JSDOM('', { + url: 'http://localhost/', + runScripts: 'dangerously', + }); + const win = dom.window as unknown as { + eval: (s: string) => void; + document: Document; + CodemanApp: new () => HarnessApp; + __TEST_RUN_MODE_LAUNCH: Record; + }; + win.eval('window.CodemanApp = function CodemanApp() {};'); + // The assignment rides in the SAME evaluated string as the module: + // RUN_MODE_LAUNCH is a bare top-level `const`, visible only to this eval call + // (see test/run-mode-launch-table-drift.test.ts for the measurement). + win.eval(`${SESSION_UI_JS}\nwindow.__TEST_RUN_MODE_LAUNCH = RUN_MODE_LAUNCH;`); + const app = new win.CodemanApp(); + app._runMinLockMs = 0; // run() otherwise holds its lock for >= 500ms per call + app.runClaude = vi.fn(async () => 'claude'); + app.runShell = vi.fn(async () => 'shell'); + app._runCliMode = vi.fn(async (mode: string) => `cli:${mode}`); + return { + app, + runBtn: win.document.getElementById('runBtn') as HTMLButtonElement, + externalIds: Object.keys(win.__TEST_RUN_MODE_LAUNCH), + }; +} + +describe('run() dispatch (session-ui.js)', () => { + const { externalIds } = loadHarness(); + + it('reads at least the eight external CLIs off RUN_MODE_LAUNCH (anti-vacuity)', () => { + expect(externalIds.length).toBeGreaterThanOrEqual(8); + expect(externalIds).not.toContain('claude'); + expect(externalIds).not.toContain('shell'); + }); + + it("'claude' reaches runClaude() and nothing else", async () => { + const { app } = loadHarness(); + app._runMode = 'claude'; + await expect(app.run()).resolves.toBe('claude'); + expect(app.runClaude).toHaveBeenCalledTimes(1); + expect(app._runCliMode).not.toHaveBeenCalled(); + expect(app.runShell).not.toHaveBeenCalled(); + }); + + it("'shell' reaches runShell(), which needs no CLI probe at all", async () => { + const { app } = loadHarness(); + app._runMode = 'shell'; + await expect(app.run()).resolves.toBe('shell'); + expect(app.runShell).toHaveBeenCalledTimes(1); + expect(app.runClaude).not.toHaveBeenCalled(); + expect(app._runCliMode).not.toHaveBeenCalled(); + }); + + for (const id of externalIds) { + it(`'${id}' reaches _runCliMode('${id}') and never runClaude()`, async () => { + const { app } = loadHarness(); + app._runMode = id; + await expect(app.run()).resolves.toBe(`cli:${id}`); + expect(app._runCliMode).toHaveBeenCalledTimes(1); + expect(app._runCliMode).toHaveBeenCalledWith(id); + expect(app.runClaude).not.toHaveBeenCalled(); + expect(app.runShell).not.toHaveBeenCalled(); + }); + } + + it('an unknown mode lands on runClaude(), never on the shared launcher', async () => { + // `_runCliMode(mode)` reads `RUN_MODE_LAUNCH[mode].label` unguarded, so an + // unknown id reaching it would throw rather than launch anything. + for (const mode of ['nope', 'CLAUDE', 'code x']) { + const { app } = loadHarness(); + app._runMode = mode; + await expect(app.run()).resolves.toBe('claude'); + expect(app.runClaude).toHaveBeenCalledTimes(1); + expect(app._runCliMode).not.toHaveBeenCalled(); + expect(app.runShell).not.toHaveBeenCalled(); + } + }); + + it('an unset or empty _runMode defaults to claude', async () => { + for (const mode of [undefined, '']) { + const { app } = loadHarness(); + app._runMode = mode; + await expect(app.run()).resolves.toBe('claude'); + expect(app.runClaude).toHaveBeenCalledTimes(1); + expect(app._runCliMode).not.toHaveBeenCalled(); + } + }); + + it('holds the launch lock for the whole launch and releases it afterwards', async () => { + const { app, runBtn } = loadHarness(); + app._runMode = 'codex'; + const pending = app.run(); + expect(app._runInFlight).toBe(true); + expect(runBtn.disabled).toBe(true); + expect(runBtn.getAttribute('aria-busy')).toBe('true'); + // A second click while the first launch is in flight is a no-op. + await expect(app.run()).resolves.toBeUndefined(); + await pending; + expect(app._runCliMode).toHaveBeenCalledTimes(1); + expect(app._runInFlight).toBe(false); + expect(runBtn.disabled).toBe(false); + expect(runBtn.hasAttribute('aria-busy')).toBe(false); + }); +});