PR A: CLI registry core as a pure internal refactor - #347
Conversation
opticon454
commented
Aug 26, 2026
As per #343 |
Ark0N
commented
Aug 27, 2026
@opticon454 This is the PR I was hoping for. Every item from the PR A scope came back done, and a few came back better than I asked: the literal-string golden pins captured before the builders were deleted, the identity probe as a general concept rather than a dsh special case, I read the whole diff and then went looking for seams independently rather than trusting the write-up. What I verifiedFull CI gate on your branch, locally: For the "byte-identical spawn command" claim I did not want to rely on the golden pins alone, since those are the same ~30 cases you chose. So I imported master's So the parity claim holds under a much wider net than the pins cover, including the drop-do-not-quote behaviour and claude's Two things before I merge1. Rebase. Your base is 2. This is the one finding I would not merge without. legacyConfigAliases: {bypassApprovals: 'dangerouslyBypassApprovals'},privilegedParams: [{param: 'dangerouslyBypassApprovals',clampTo: false}],
DeepSeek's chain works today only because one name happens to coincide in both namespaces: the clamp writes Follow-ups, happy for these to ride with PR B3. The no-id-branching guard only matches
Not regressions, they are all pre-existing, but it does mean the "~123 to 32, each allowlisted with its reason" figure is measured only over the shape the regex happens to catch. Widening the pattern is a couple of lines; the branches it surfaces are mostly one-liners. 4. 5. A custom CLI is now API-acceptable but not survivable downstream. Smaller
Where this leaves usRebase, fix 2, and I will merge it. Items 3 to 5 and the smaller list can come with PR B, or here if you would rather have them in one place; either is fine by me, just tell me which so I know when to look again. Separately: yes please to the Really good work. |
opticon454
commented
Aug 27, 2026
I'm off on a holiday today for the next 5 days so I'll fix and rebase on whatever version it is next week when I'm back 👍 |
opticon454
commented
Sep 1, 2026
@Ark0N I'm updating right now but unsure how that will fit between our timezones |
…anching Every run mode is now a `CliEntry` in `src/config/cli-registry/` — 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. Code that used to ask "which CLI is this?" reads the entry instead. Behaviour is unchanged. `test/cli-registry-spawn-golden.test.ts` pins every spawn command as a literal string, captured from the hand-written builders before they were deleted, and `test/location-overlay-commands.test.ts` does the same for all 20 remote and in-container pane commands. Config can never contain 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. `~/.codeman/clis.json` overrides any entry, read-only in this release. OMP is included as a registry entry rather than a tenth hand-written builder, so `buildOmpCommand()`, the omp availability pre-flight, the omp arm of `buildPathExport()` and the omp entries in the truecolor/NO_COLOR, alt-screen and doctor ladders all drop out. Guard rails: - `test/cli-registry-no-id-branching.test.ts` fails the build if per-CLI-id branching reappears outside `stock.ts`, in any of its four shapes (`===`, `!==`, `switch`/`case`, `includes`) — an `===`-only version would miss the negated forms, which is how 36 of them survived an earlier pass. Every allowlisted branch carries its reason. - `external`, `hooks` and `altScreen` stay three INDEPENDENT capabilities; deriving one from another shipped the `until=stop`-hangs-on-shell bug. - `param` is two namespaces. `launch.params` keys, `configSetenv.fromParam` and `privilegedParams[].param` all name a LAUNCH param; the legacy `<Mode>Config` wire field is separate, bridged only by `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 error and no failing test — so `schema.ts` rejects an entry naming a param it never declared. - Registry data resolves 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. - Six fields are annotated DECLARED-FOR-LATER and read by nothing (`shortBadge`, `accent`, `capabilities.echo`/`wheelForward`/ `keyboardAccessory`/`maxFrameBytes`): all frontend behaviour, transcribed rather than measured. A test pins the list so it cannot quietly grow. Three user-visible changes, all deliberate and named: - `probeDockerCliVersion()` derives the in-container binary from the registry rather than assuming it equals the mode name (`antigravity` runs `agy`). - The remote CLI version probe now covers grok and deepseek, which the hardcoded map it replaces omitted while its own comment said the rule was "every mode except shell". - `codeman doctor`'s CLI rows are generated from the entries, so Claude's install hint is the install command rather than a docs URL, five CLIs gain hints they never had, and the row order follows the catalog. Also hardened along the way: `sessionModeSchema()` is bounded at 24 chars (matching the `cliId` pattern) before its failure message quotes the value back, and `deepMerge` skips `__proto__`/`constructor`/`prototype` when reading the hand-editable `clis.json`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQkoi1cNegqVwZHgzx5SbJ
f43e72a to
4830e66CompareCI caught three cron-service failures. Both are mine, from converting cron's per-mode ladders to capability reads without checking what each ladder's scope actually was. **The pre-flight.** cron only ever pre-flighted `deepseek` — dsh is a profile LAUNCHER, so "installed" is not "runnable" and a bare `dsh` can boot a profile that cannot drive a pane. I replaced that with an unscoped `resolveCliLaunchError(mode)`, which pre-flights EVERY mode, so a claude cron job on a box with no claude binary now failed with "Claude CLI not found" instead of reaching tmux-manager's own throw. Three tests assert the latter. It is now gated on `discovery.launcherProfile !== undefined`, which is byte-identical to the `mode === 'deepseek'` check it replaces and generalises to the next launcher. The equivalent HTTP-route conversion was already scoped (to `capabilities.external`, matching what that route has always pre-flighted); I simply failed to carry the same reasoning across. **The model.** cron's ladder was `mode !== 'shell' && mode !== 'deepseek'`, and I read it as `capabilities.model.source === 'claude-settings-file'` — which is the HTTP route's question, not cron's. There, every external CLI reads its model from its own config object earlier in the chain, so only claude reaches the global default; cron has no such config, so the same expression silently narrowed the default model from eight modes to one. Now `!== 'none'`, which is exactly the two entries the ladder excluded. Not caught by a test — found by re-deriving each ladder's scope after the first failure. Also names a fourth deliberate behaviour change in the changeset, found while tracing these: `session.ts` carried a hand-written list of modes with no direct-PTY fallback and OMP was missing from it, though CLAUDE.md's own text says "all eight require tmux". `requiresMux` comes off the entry now, so an omp session whose mux creation fails refuses instead of silently starting outside tmux. Verified by diffing failing tests BY NAME against an upstream/master baseline, rather than by file as before — which is how the regression slipped through: the three new failures landed inside a file already failing for unrelated Windows-path reasons, and the aggregate count happened to collide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQkoi1cNegqVwZHgzx5SbJ
Review item 4 named THREE live tables duplicating registry data. Two are now read from the entry (`defaultRemoteCommandForMode`, `defaultDockerCommandForMode`); the third, `resolveDockerCredentialArtifacts`, is not — and it was left neither wired nor annotated, which is the state that item explicitly rules out. It is not wired because the shape cannot express the live table: `credStore` is ONE store per CLI, and `CRED_STORES` needs two for gemini (`.gemini` for the CLI's own auth plus `.config/gcloud` for Vertex), while deepseek's entry declares none at all even though `.dsh` is seeded. Wiring it means making the field an array and correcting those two entries — a change to credential seeding, which is at once the worst thing in that file to get wrong and the least covered by tests, since every docker IO path is no-op'd under vitest. It belongs in its own change, measured against a real container. So it is annotated instead, at the field, in the type's declared-for-later header, in docs/cli-registry.md, and in the pinned DECLARED_FOR_LATER list — the last of which means wiring it later makes a test fail rather than leaving a stale comment behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQkoi1cNegqVwZHgzx5SbJ
opticon454
commented
Sep 2, 2026
Back, rebased, and everything from your review is in this PR rather than a PR B — so there's nothing left to look at elsewhere. Answering your question first, then what changed since you read it. Checks are green (both jobs). Base is now 1 — RebaseMaster had moved further than 1.23.2 by the time I got back, so this is rebased onto 1.24.4. The I brought it in as a registry entry rather than re-adding the shape this PR removes, so 2 — |
Uh oh!
There was an error while loading. Please reload this page.
Ark0N
commented
Sep 4, 2026
Merged, thank you. Before pressing the button I re-ran everything: the full gate locally (typecheck, lint, format, 331 test files / 6468 tests green), the same on a trial merge with current master, and the spawn-command diff against master's builders across 11,602 option combinations for all ten modes, shell and an unregistered id included: 0 diffs. Three separate audits over the multi-user clamps, the guard test plus overlays, and runtime drift in session/tmux/cron/schemas/resolvers all came back with no semantic difference for the ten stock modes. Everything from the first review is in. What is left is follow-up grade, none of it blocking, roughly in order of interest:
Ships in the next release, going out today. |
opticon454
commented
Sep 4, 2026
Thankyou for making an awesome app and integrating everyone's ideas :) |
…iner feat(docker): attach a case to an already-running container Conflicts came from work that landed after the PR was opened, and each is resolved onto the newer abstraction rather than by keeping the older code: - `defaultDockerCommandForMode` is registry-driven since #347, so the PR's `runsAsRoot` arm became `overlays.docker.rootCommand` (claude only). Claude Code still refuses `--dangerously-skip-permissions` as root in 2.1.261 and the refusal is visible only inside the container, so an adopted root container otherwise just shows a dead pane. Which flag to drop is a per-CLI fact, and `test/cli-registry-no-id-branching.test.ts` forbids expressing it as a branch. - The probe's mode list and its mode -> binary table both duplicated the registry. They now read `enabledCliIds()` / `discovery.binaries[0]`, which is also what fixes the merge's silent regression: the hand-written list predates `omp`, and the run menu gates every docker case on this probe, so owned containers would have lost that mode. `shell` needs no arm — it declares no binary, so it is dropped from the lookup and reported available regardless. - The per-mode `mode === 'claude' && !cliDir` chain in `tmux-manager.ts` is one `missingCliMessage(mode)` gate since #347; the PR's docker exemption moved onto it. Its test now pins the single gate instead of counting seven arms. - The create arm keeps #349's swap-limit warning filter, which the adopted arm never reaches; the run-mode list gains `omp` from #353. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TecFD9hvPYJ1mkkMtBQbT1
Thanks for the detailed read-through — it made this a much easier thing to scope. This is PR A from your list: the registry core as a pure internal refactor, rebased on current master (1.23.0), opened as a draft as you suggested so you can look at the DeepSeek extension design before I go further.
Behaviour is unchanged.No new endpoints, no new settings keys, no dependency changes, and the spawn command every CLI receives is byte-identical to what the hand-written builders produced.What this does
Every run mode —
claude,shell,opencode,codex,gemini,antigravity,pi,grok,deepseek— is aCliEntryinsrc/config/cli-registry/. Code that branched on a CLI's name now reads capability flags off that entry.Per-CLI-id branch sites: ~123 → 32. The 32 that remain are allowlisted individually, each with its reason, by the guard test (details below).
src/tmux-manager.tsalone sheds ~570 lines.What the registry owns: binary discovery (search dirs, version and identity probes), the launch argv template, environment handling (exports,
tmux setenvkeys, the env-override allowlist), the multi-user privileged-parameter clamps, and the behavioural capabilities the rest of the app reads (isExternalCliMode,isAltScreenStripMode,hooksAvailableForMode, alt-screen strip class, echo policy, transcript format, model source, and friends).codeman doctor's per-CLI rows are generated from the same entries.New files
config/cli-registry/types.tsconfig/cli-registry/patterns.tsconfig/cli-registry/profiles.tsconfig/cli-registry/schema.ts.strict()validationconfig/cli-registry/argv.tsconfig/cli-registry/stock.tsconfig/cli-registry/registry.tssession-cli-registry-bridge.ts<Mode>Configwire shape → registry paramsutils/cli-resolver.tsutils/cli-launcher.ts1. DeepSeek, and the four assumptions it breaks
This is the part I'd most like your eyes on.
DSH_PERMISSION_MODEenv var, not a flagcapabilities.privilegedEnvKeyshooksAvailableForMode('deepseek')is a per-session questioncapabilities.hookswidens to'none' | 'always' | 'supervised'dshis a profile launcher — installed ≠ runnablediscovery.launcherProfile(+launcherTargetParam)capabilities.transcriptgains'deepseek-zstd'The env-var privileged param
You flagged that
capabilities.privilegedParamscan only clamp argv params, so the registry as designed could not expressclampEnvOverridesForOwner()— and that merging as-is would make a real multi-user control silently disappear. That is now a separate, deliberately distinct field:It is not a variant of
privilegedParamsbecause the two reach the CLI by different paths — one becomes an argv flag, the other ridestmux setenv, which no argv clamp can see.ownerClampedEnvKeys()insession-routes.tsnow derives its list from every enabled entry'sprivilegedEnvKeys, and I verified at runtime that it resolves to exactly master's list:The reasons for each key are recorded on the field's doc comment, including the one that is easy to lose:
applyEnvOverrides()runs after the per-CLI env configure step, so without this a non-granted owner sendingDSH_PERMISSION_MODEon the same request would land last and hand back exactly the privilege the config clamp removed.DSH_PERMISSION_MODEitself is exported through a newenv.configSetenvmapping rather than a bespoke configure step, which is what lets the ordinaryprivilegedParamsclamp reach it: the clamp rewrites the param, and whatever the param ends up as is what gets exported. Values are re-validated against the declaredParamSpecbefore export — the wire shape is already Zod-checked, but this one reachestmux setenvas a permission level, and a builder should not trust its caller there.hooksas a tri-state'supervised'means the CLI reports its own state to a supervisor and Codeman is that supervisor — definitive signals rather than inferred ones — but the session can disarm the bridge and a docker/remote session cannot reach it at all.hooksAvailableForMode(mode, options)keeps its exact signature andsessionHookOptions()is untouched, so every existing call site behaves as before.A boolean is explicitly rejected by the schema, with a test, because
truewould have to mean'always'— which is wrong for a supervised CLI and would promise astopthat never arrives.Launcher profiles
discovery.launcherProfilenames an entry in a small map (utils/cli-launcher.ts) answering two questions the binary alone cannot: is it runnable (stricter than "is the binary on disk") and what is the default target.discovery.launcherTargetParamnames the param carrying a caller-requested target, soresolveCliLaunchError()keeps DeepSeek's three distinct actionable messages (binary missing / no pane-capable profile / the profile you named cannot drive a pane) rather than collapsing them to "not installed".Identity probes
discovery.identityis new and general: proof that the binary found is the program meant, checked before the version probe.requireVersionMatchcatches output with the wrong shape; this catches output with the right shape naming the wrong program — Debian'sdsh(dancer's shell) answers--versionperfectly happily.Both named tests pass:
test/deepseek-mode.test.ts(including its static source scans and the env-half clamp cases) andtest/routes/external-cli-bypass-clamp.test.ts(the materialize-vs-only-if-sent split, unchanged).2. Parse-time resolution
SESSION_MODE_IDS/ALLOWED_ENV_PREFIXES/ALLOWED_ENV_KEYSare gone as module-load constants.sessionModeSchema()is now a refinement that readsenabledClis()when the request is validated, andisAllowedEnvKey()reads the registry per call.BLOCKED_ENV_KEYSis deliberately not registry-driven and is still checked first, so a pathologicalallowedPrefixesentry cannot unblockPATH— there is a test for exactly that.Pinned by a test that disables a CLI, calls
reloadCliRegistry(), and assertsPOST /api/sessionsstarts rejecting that mode with no restart, plus the same for an env prefix:One consequence worth naming:
test/agent-skill-mode-lists.test.tsderived its expected mode set by unwrapping the Zod optional to reach.options, which a parse-time refinement no longer has. Rather than restate the list there,schemas.tsnow exportssessionModeIds()and the test reads that — it still derives from the runtime source of truth. Its "both endpoints agree" assertion would have become tautological (both now share one validator), so it parses a sample through each schema instead, which still catches the two drifting apart.3. No Copilot
Removed entirely — no entry, no code, and no stale comments (three referenced it in the grok entry; they are gone). Happy to bring it back as pure registry data once github/copilot-cli#4180 and #4223 close; agreed it would be a good proof of the registry.
4. No write API, no auto-install
No
/api/clisroutes, nocli-installer.ts.discovery.install.commandis display text only — it feedscodeman doctor's install hints and the "CLI not found" message, and is never spawned. Its doc comment states the invariant explicitly rather than pointing at an executor.~/.codeman/clis.jsonis read on load (deep-merge,.strict()validation, pristine-stock fallback for a bad override, drop-with-warning for a bad custom entry, quarantine-not-overwrite for malformed JSON, group/world-writable refused) but nothing writes it. TheseededStockIdsratchet is deferred to PR C along with the write API that needs it — which also means importing the registry, and thereforeschemas.ts, performs no filesystem writes. A file written by a later version still loads cleanly here.5. Your assorted findings
GET /api/grok/status— kept. Verified live on a deployed container alongside the other seven; all three distinct response shapes preserved ({available,path},{available,path,version}, and DeepSeek's seven-field shape). No route registrations added or removed anywhere in this PR.install.sh/ docker — untouched, along withconfig/andscripts/. Noclis.stock.jsonhere; the bash-3.2 fix and theenabledfilter belong to PR B.test/cli-registry-no-id-branching.test.tsnow exists. It builds its id list from the live catalog, strips comments before scanning (comments legitimately quote the banned pattern to explain why a branch was removed), has an anti-vacuity check and a sanity check on the scanned file count, and fails on a stale allowlist entry. I confirmed it fails on a real violation by introducing one. The leftoverif (this.mode === 'grok')is gone.test/cli-registry-spawn-golden.test.ts, captured from the hand-written builders before those builders were deleted, so the pins are the surviving record of what they emitted. Grok and DeepSeek are both covered — grok had no parity coverage at all previously, and the drop-don't-escape behaviour for rejected values is pinned too.cli-executable-resolver.tswas layered on rather than replaced. The impostor-rejection tests, the version-regex contracts and the vitest hermeticity pins were never at risk.test/dependency-checker.test.tsnow checks the pi/grok/dsh version rules by source rather than object identity, since the doctor compiles the entry's serialized pattern throughcompileVersionRegex().codeman doctorgrok row — present. The rows are generated from the registry, so that class of omission is now structurally impossible; there is also a test asserting a row exists for every CLI with a binary to probe. Live output shows all ten, Grok and DeepSeek included.test/**— dropped. Three test files are touched, all with real edits.mobile-overview.js— deliberately not addressed, and I want to flag it rather than have you notice. The phone picker can only diverge from the desktop menu once enable/disable exists, and PR A adds neither. It belongs with the settings UI in PR C. Say the word if you'd rather have it now.Two fixes the registry enabled
probeDockerCliVersion()derived the in-container binary from the mode name.antigravityrunsagy, so that assumption was wrong — though onlyclaudereaches that path today (it is the one CLI with a version gate), so nothing was actually broken. It now readsdiscovery.binaries[0]. This is the one intentional behaviour change in the PR and I did not want to bury it.compileVersionRegex()in the doctor — the dependency table compiles the entry's version pattern through the same ReDoS guard the argv engine uses, rather thannew RegExp().Security model, unchanged
Config still contains no shell text. Four independent layers, all preserved:
command: "..."field anywhere in the schema;argv.tsowns every separator, including the||between fallback variants.--no-approveis not cosmetic).clis.jsoncannot supply its own regex for a shell token and cannot widen its own validation.renderToken()re-checks before emitting bare.The only config-supplied regexes are
discovery.version.regexanddiscovery.identity.regex; both run against truncated command output, never a shell token, and both go throughcompileVersionRegex()'s length cap and nested-quantifier rejection.Named profiles are the escape hatch for behaviour that genuinely needs to run code. Their names live in
profiles.ts, which is kept import-free soschema.tscan validate a name at load time — an entry naming a profile this build does not implement fails loudly instead of failing closed later.Verification
Full CI gate, run inside a Linux container built from this branch:
Plus live behaviour checks on a running instance: one session per installed mode spawned through
quick-start, with the real pane commands captured and compared against the golden pins —— all eight per-mode status routes, and the full
codeman doctortable.Notes
CliEntry, add a golden spawn pin, add a row to the capability-predicate table. If you find yourself wanting anif, the guard test says so.test/setup.tsscrubsCODEMAN_PASSWORD/CODEMAN_USERNAME/CODEMAN_GESTUREfrom the environment so a dev box cannot influence results, but notCODEMAN_INSTANCE/CODEMAN_DATA_DIR/CODEMAN_TMUX_SOCKET. Anyone running the suite with those set gets 7 spurioustmux-managerfailures on socket names. One-line fix, deliberately kept out of this PR — happy to send it separately.🤖 Generated with Claude Code