refactor(run-menu): one table-driven launcher for every external CLI (PR B2)

[opticon454]

PR B2 — the frontend half of the CLI registry (window.__codemanCliCatalog, session-ui.js, mobile-overview.js)

Status: implemented, fully tested, opened for review.
Base: followups @ cd64b0a3 (v1.31.0), which already includes the merged
feature/run-menu-custom-model-picker work.
All verification below ran on a dedicated tmux-capable Debian VM (this sandbox has
no tmux, and WebServer's constructor hard-requires one), not this environment.

Context

PR #380 (PR B, merged) drove install.sh and the Docker agent image from the CLI
catalogue, and explicitly held back the frontend half:

"The frontend half — injecting window.__codemanCliCatalog and making
mobile-overview.js / session-ui.js catalogue-driven — is held back as PR B2"

Reasons given: six of the thirteen PRs open at the time touched those files, so
merging conflict-free mattered more than doing everything in one PR.

Sequencing note: before starting, I found feature/run-menu-custom-model-picker
(the Run-menu picker for Custom Model Endpoint Profiles) was active, unmerged WIP
that directly rewrote the same 8 launch functions this PR also touches — 734 lines
of session-ui.js changes overlapping mine hunk-for-hunk. I waited for it to merge
before starting rather than fight it in parallel; it has since landed (the
window.__codemanCliAvailable/window.__codemanCustomModelClis globals and the
custom-model launch folding in each run<Mode>() you'll see below are its work, not
mine).

What changed

1. window.__codemanCliCatalog (src/web/server.ts)

A general-purpose run-menu catalogue, injected in renderIndexHtml() beside the
existing window.__codemanCliAvailable (booleans) and window.__codemanCustomModelClis
(narrowed to custom-model-capable CLIs) globals, following their exact pattern —
enabledClis() read generically, escapeScriptJson()-guarded, resolved per-request
never at import.

Carries {id, label, shortBadge, order, kind} per enabled CLI — no filter beyond
enabled, unlike the custom-model global. Deliberately excludes launch/env/
capabilities/overlays/discovery, mirroring the same rule
scripts/generate-cli-catalog.mts already follows for config/clis.stock.json.

Reading shortBadge here is what made it a genuinely-read field — I removed it from
CliEntry's DECLARED_FOR_LATER list in types.ts and updated the header comment,
which test/cli-registry-no-id-branching.test.ts's own pinned-list guard caught and
required.

Current status: built, tested, has no consumer yet. See "What I decided not to
do" below for why — kept anyway as documented forward-looking infrastructure, the
same pattern this codebase already uses for accent/capabilities.echo/etc.

2. session-ui.js: the 8 near-duplicate launch functions

runOpenCode/runCodex/runGemini/runAntigravity/runPi/runOmp/runGrok/
runDeepSeek were each an independent ~45-line copy of the same probe → build-config
→ launch → select skeleton, with only a handful of lines actually differing per CLI.
Consolidated into:

• RUN_MODE_LAUNCH, a local table (label, install hint, per-CLI wire-config builder,
  custom-model eligibility) — deliberately not sourced from
  window.__codemanCliCatalog (see below for why)
• _runCliMode(mode), the one shared implementation
• The 8 original method names kept as thin one-line wrappers, because index.html's
  welcome-screen buttons call them by name (app.runOpenCode() etc.) and several
  tests assert on the name directly

run()'s 8-branch if-chain collapsed to: shell gets its own path, claude (or
anything not in the launch table) falls through to runClaude(), everything else
goes through _runCliMode(mode).

Also collapsed a duplicated 8-way session.mode === '<id>' || ... OR-chain that
appeared twice inside the same function (openSessionOptions, for isAltMode
and isExternalCli — literally the same expression, copy-pasted) into one
EXTERNAL_CLI_MODES.has(session.mode) check, backed by the same Set the launch table
derives from.

Net diff: -446/+153 lines in session-ui.js.

3. mobile-overview.js: investigated, no changes

The original plan assumed ~16 scattered hardcoded CLI occurrences here, based on a
rough grep before reading the file. On actual investigation there's exactly one
data structure (MOBILE_OVERVIEW_RUN_MODES, a single literal array), already gated
by isCliAvailable(), already cross-checked against index.html's menu (10/10 modes
match, no drift), and its own test explicitly requires it to stay a literal array
(same anti-drift-guard-via-pinned-source-text pattern found in session-ui.js).
Making no change here was the correct call, not a shortfall — see below.

4. New guard: test/frontend-cli-no-id-branching.test.ts

Mirrors test/cli-registry-no-id-branching.test.ts (same four-shape
BRANCH_PATTERN, same STOCK_CLIS-derived id list, same allowlist-with-reasons
design), but scoped only to session-ui.js/mobile-overview.js — not widened
onto the whole src/web/public/ directory, which would force fixing or allowlisting
dozens of branches in files CLAUDE.md explicitly keeps out of scope (app.js,
terminal-ui.js, settings-ui.js, …).

21 branches remain across both files post-consolidation, all reviewed and allowlisted
with reasons (claude/shell dispatch splits, the documented restart-vs-one-shot
custom-model mechanism, the Respawn/Ralph claude-only gate, the button-label ternary,
the runMode setter's validity check, mobile-overview.js's shell-exempt
availability gate).

Verified for real, not just written: injected a genuine unrelated branch on the test
VM, confirmed the guard failed (both the unapproved-branch and stale-allowlist
checks fired), reverted, confirmed green again.

What I decided not to do, and why

Three items were in the original plan and turned out to be unsafe or unnecessary on
actual investigation — each verified against real pinned tests, not assumed:

1. The button-label ternary (mode === 'opencode' ? 'Run OC' : ...). Catalogue
   field is shortBadge; the ternary's actual OMP text is 'Run OMP' (3 chars)
   while shortBadge for omp is 'OM' (2 chars). test/run-mode-ui.test.ts pins
   the exact 'Run OMP' text. Deriving from the catalogue would silently change
   displayed text and fail that test.

2. _refreshRunModeAvailability's mode-iteration array. A pinned test literally
   scans this function's source text for quoted mode strings — its own comment: "Catches
   a sixth run mode being added to index.html without being gated." It's an
   intentional anti-drift guard, not an anti-pattern.

3. window.__codemanCliCatalog itself wasn't consumed by either of the above,
   because several tests exercise these functions inside a bare vm.createContext()
   with no window global at all — referencing it unguarded there throws
   ReferenceError, not undefined. Local static constants (RUN_MODE_LAUNCH,
   EXTERNAL_CLI_MODES) sidestep this while achieving the same actual goal: one
   source of truth instead of duplicated logic.

I'm flagging rather than hiding these, same as the maintainer's own review style —
each is a place the plan's first-pass assumptions didn't survive contact with the
real code and its test suite.

Verification

All four phases run on a tmux-capable Debian VM I set up for this
(codeman-devbox), since this sandbox can't run WebServer at all. Full CI gate run
after each phase, not just at the end:

Phase	Files	Full gate result
1 (__codemanCliCatalog)	server.ts + tests	404 files / 7712 tests / 0 failures
2 (session-ui.js consolidation)	session-ui.js	404 files / 7712 tests / 0 failures (no change — same count, no regressions)
3 (mobile-overview.js)	none (investigation only)	not re-run, nothing changed
4 (frontend guard)	new test file	405 files / 7717 tests / 0 failures

Also:

• npm run typecheck clean at every phase
• npm run test:browser -- test/opencode-resize.test.ts (real Chromium): the one
  test directly inspecting runOpenCode.toString() for the historical
  activeSessionId-bypass bug passes; the other 5 tests in that file fail identically
  with my changes stashed out (need an authenticated claude CLI this fresh VM
  doesn't have — pre-existing gap, confirmed, not a regression)
• 94 targeted tests across test/run-mode-ui.test.ts,
  test/custom-model-run-menu-ui.test.ts, test/custom-model-one-shot-launch.test.ts
  — exact wire-body shape per CLI (codexConfig/geminiConfig/antigravityConfig/
  grokConfig/deepSeekConfig, pi's deliberate absence of piConfig) — all pass unmodified
• Local working tree confirmed byte-identical to what was tested throughout
  (checksum comparison against the devbox after every sync)

5. stock.ts's accent field doesn't match reality for at least 4 of 9 CLIs

Not part of this PR's diff, but worth flagging: I checked whether the "declared but
not yet read" accent field (docs/cli-registry.md lists it alongside echo/
wheelForward/keyboardAccessory) was safe to wire up as a stretch goal. It isn't
— its real consumer would be styles.css's hand-authored per-CLI gradients, a file
outside this PR's scope — but while checking, I compared the registered values
against the actual rendered gradients:

CLI	stock.ts accent	Actual button gradient
claude	#d97757 (orange — Claude's real brand color)	blue (#1a3358 → #2563eb)
opencode	#f59e0b (amber)	green (#0a2e2a → #0d4a40, emerald text)
antigravity	#8b5cf6 (purple)	cyan (#0b2b33 → #0891b2)
pi	#10b981 (green)	pink/rose (#33121f → #be185d)
deepseek	#4d6bfe	matches exactly (#4d6bfe appears literally in the gradient)

Confirms docs/cli-registry.md's own "transcribed, not authoritative" warning
empirically. Not fixing it here since it's a color-correctness question unrelated to
B2's actual goal, but you'll probably want it fixed before anyone wires accent up.

Open questions for you

1. OMP's 'Run OMP' vs. shortBadge: 'OM' mismatch (found during Phase 2) — pre-existing,
   harmless today, but blocks making the label ternary catalogue-driven. Worth fixing
   stock.ts's omp entry, or leave the ternary hardcoded permanently?
2. window.__codemanCliCatalog has no consumer. Kept it anyway (cheap, tested,
   matches this codebase's own "declared but not yet read" precedent) rather than
   reverting Phase 1's work — agree, or would you rather it came out until something
   actually needs it?
3. Sequencing/splitting: ship Phases 1+2+4 as one PR (this one), or split further?
   feat(cli-registry): drive install.sh and the Docker agent image from the CLI catalogue #380's own rationale for holding B2 back was avoiding conflicts in a crowded area —
   worth checking how many PRs currently open still touch these files before merging.
4. The accent mismatches above — worth its own small fix PR before or after this one?

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01GuHtuPiHXdykq9T6rKQJ9n

[Ark0N]

Thanks for this, and for the write-up. The open questions and the "what I decided not to do" section made the review a lot faster than it would otherwise have been.

To summarise for anyone else reading: this collapses the eight near-identical run<CLI>() launch functions in session-ui.js into one shared _runCliMode() plus a local launch table, replaces two copy-pasted 8-way mode chains in openSessionOptions with a Set, injects a new window.__codemanCliCatalog global from renderIndexHtml, and adds a static guard against new CLI-id branching in session-ui.js / mobile-overview.js.

The refactor itself is faithful. I compared every launch body against master and the wire shapes are unchanged, including antigravity's deliberate absence of customModel, pi's absence of piConfig, omp sending no config object, and deepseek's two-part available-plus-runnable check. Full gate green here as well: 405 files, 7717 tests, 0 failures, plus typecheck, lint, format:check and check:frontend-syntax.

Two things I would like fixed before this lands.

1. test/frontend-cli-no-id-branching.test.ts:123 and every key in ALLOWED_BRANCHES at :34: the allowlist keys embed line numbers. I inserted one comment line at the top of session-ui.js and both assertions failed at once, all 21 entries went stale and the same 21 branches were then reported as "new CLI-id branch(es)". session-ui.js is one of the most contended files in the repo (your own description counts six concurrent PRs in it), so as written this goes red on essentially every unrelated change to that file, with a message that sends the next person after the wrong problem. The backend guard you mirrored keys on <relative path>::<expression> with no line number for exactly this reason. Dropping ::${i + 1} from the key and collapsing the list to one entry per file plus expression gives 11 entries; I tried that version locally and it is green both on the current tree and after a line shift.

2. test/run-mode-ui.test.ts:170, the "routes every run mode through the ownership helpers, never the terminal directly" guard, is now blind to the consolidated path. Its header regex is ^ {2}async (run[A-Za-z]*)\(\) \{$, which matches the eight one-line wrappers but not _runCliMode(mode), where the body now lives. I added this.terminal.clear() and this.terminal.writeln() to _runCliMode and the file stayed 32/32 green, so the exact bug that guard was written for would now ship unseen for all eight external CLIs at once. Changing the regex to /^ {2}async (_?run[A-Za-z]*)\(\w*\) \{$/gm and adding '_runCliMode' to the arrayContaining sanity list fixes it: green on your tree, red on the injected bug. Same class, lower priority: test/opencode-resize.test.ts:69 now inspects runOpenCode.toString() on a wrapper.

Smaller things I am happy to take at merge rather than have you respin:

• CLAUDE.md:226 and docs/cli-registry.md:113 still list shortBadge among the six "declared but not yet read" fields. Your new comment at src/config/cli-registry/types.ts:637 points at docs/cli-registry.md as the record of the change, and that file currently says the opposite.
• src/web/server.ts:1694: String.replace with a string replacement interprets $&, $' and friends, so a label containing $' re-injects the rest of the document, unescaped </script> included, straight past escapeScriptJson. The only source is the server owner's own ~/.codeman/clis.json, and __codemanCustomModelClis at :1673 has had the same shape for a while, so this is not urgent. A replacer function (.replace('</head>', () => ...)) closes both while you are in there.
• src/web/public/session-ui.js:115 refers to _isAltCliMode(), which does not exist in the tree.

On your open questions:

2 is the one I want settled before merging. Nothing reads window.__codemanCliCatalog yet, and the eight labels it would have supplied are re-stated in RUN_MODE_LAUNCH (they match CliEntry.label exactly today), so the title promises something the diff does not deliver yet. A declared-for-later registry field costs nothing; a script tag on every index render with no consumer is a slightly different trade. Either answer works for me: keep it and fix the two docs above, or drop the server side and land the session-ui.js consolidation plus the guard, which is the part carrying the value. Tell me which you prefer and I will go with it.

1: leave the label ternary hardcoded. omp's shortBadge is worth a separate look and it is my mistake, not yours.

3: current head is clean against master, nothing else open conflicts with these files, so no further splitting needed on that account.

4: yes please, a small separate PR for the accent values, and thank you for actually measuring them against the rendered gradients.

Also checked and confirming your reasoning: most of the vm.createContext() harnesses in test/run-mode-ui.test.ts really do run without a window global, so the local table rather than a catalogue read was the right call for now.

Once the two guard fixes are in and you have picked an answer for question 2, this goes in.

[opticon454]

Thanks for the fast, thorough turnaround — and for reproducing both bugs rather than just describing them, that made both fixes unambiguous.

Both required fixes are in, verified the same way you found them: I reproduced each bug live (inserted the same comment line at the top of session-ui.js; added this.terminal.clear() to _runCliMode), confirmed the old test went red exactly as you described, applied the fix, confirmed both scenarios now pass and the suite stays green.

1 — Allowlist keyed on line numbers

Fixed as you suggested: dropped ::${i + 1} from the key, now <file>::<expression>, matching the backend guard exactly. 21 line-keyed entries collapsed to 11 — several now cover more than one physical call site sharing the same expression in the same file (e.g. session-ui.js::mode === 'claude' now has one combined reason covering run()'s dispatch, runCustomModelEntry()'s restart-vs-one-shot mechanism, the Respawn/Ralph gate, and the runMode setter, instead of four separately-line-numbered entries). Reproduced your exact repro (one inserted comment line) against the old version, confirmed it went red the same way, applied the fix, confirmed green.

2 — _runCliMode blind spot

Applied your regex exactly (^ {2}async (_?run[A-Za-z]*)\(\w*\) \{$) plus '_runCliMode' in the sanity list. Reproduced your exact repro (this.terminal.clear() added to _runCliMode) against the old regex — stayed green, confirming the blind spot — then against the fix, which caught it immediately.

Also fixed the same-class issue in test/opencode-resize.test.ts: it now inspects app._runCliMode.toString() instead of app.runOpenCode.toString(), since the wrapper never contained the pattern being checked for either. Ran it on a real Chromium against a live server on my test VM — the fixed assertion passes; the other 5 tests in that file fail identically with my changes stashed out (they create real sessions via POST /api/sessions, which needs an authenticated claude CLI this VM doesn't have — pre-existing gap, confirmed, not something either of us introduced).

Smaller things

• session-ui.js's EXTERNAL_CLI_MODES comment referencing _isAltCliMode() — fixed, it now describes the actual EXTERNAL_CLI_MODES.has(...) inline check.
• The doc staleness and String.replace escaping points are moot now — see below — but thank you for catching the escaping one regardless; I'll flag it if I touch that code again in the accent follow-up.

Open questions

2 (keep or drop the catalogue): dropping it. Nothing reads window.__codemanCliCatalog, and your framing settled it for me — a registry field that costs nothing until read is a different trade than a script tag re-serialized on every page render with no consumer. Reverted Phase 1 cleanly: server.ts's injection removed, shortBadge back in types.ts's DECLARED_FOR_LATER list and the pinned test, the three new/changed assertions in render-index-html.test.ts/server-index-title.test.ts removed. This also means the CLAUDE.md/docs/cli-registry.md staleness you flagged is moot — those files never changed, only my (now-reverted) comment in types.ts disagreed with them.

1 (OMP label): left hardcoded, per your call.

3 (splitting): noted, no action needed.

4 (accent PR): will follow up separately once this lands.

Full gate: 405 files, 7717 tests, 0 failures (net unchanged — removed one test with the catalogue, restored one with shortBadge). Typecheck, lint, and format:check all clean. @Ark0N Ready whenever you are.

[Ark0N]

Thanks for this, and for turning both guard fixes around so fast with the repros attached. To restate what lands: this collapses the eight near-identical run<CLI>() launch functions in session-ui.js into one shared _runCliMode() plus a local launch table, replaces the two copy-pasted 8-way mode chains in openSessionOptions with a Set, and adds a static guard against new CLI-id branching in session-ui.js and mobile-overview.js.

I re-verified the refactor rather than reading it. A JSDOM harness drove all eight launch functions on your head and on master across 80 combinations (local, remote, docker, with and without envOverrides, with and without a pending custom-model pick, CLI unavailable, dsh installed but not runnable, codex with both app settings on, two-instance launch), capturing the status-probe URL, the banner text, every /api/quick-start body, every error string and the selected session id. The two dumps are byte identical, including antigravity's absence of customModel, pi's absence of piConfig, omp sending no config object, and deepseek's two-part check. Full gate here: 405 files, 7717 tests, 0 failures, plus typecheck, lint, format:check, check:frontend-syntax and check:public-assets.

Both round-one fixes reproduce as fixes. The allowlist survives an inserted line at the top of session-ui.js, and this.terminal.clear() added to _runCliMode now goes red under the new regex. test/opencode-resize.test.ts behaves exactly as you said: the assertion you changed passes on real Chromium, and I ran the same file with your changes swapped out for the merge-base versions and got the identical five failures, so that is the environment rather than the PR.

Four things I will take at merge rather than send back:

1. test/frontend-cli-no-id-branching.test.ts:40. Because the key is <file>::<expression> and every stock id is already allowlisted for session-ui.js in the mode === '<id>' form, a new mode === '<id>' branch anywhere in that file now passes unnoticed. I injected if (this.mode === 'codex') into runOpenCode() and the guard stayed 5/5 green; switch (this.mode) { case 'codex': } is caught correctly. The keying itself is right and I do not want it reverted, it is what fixed the line-shift problem and it is what the backend guard does. The close is to carry an expected occurrence count in each allowlist value and assert equality, so the count goes stale instead of the key.

2. src/web/public/session-ui.js:32. RUN_MODE_LAUNCH now restates four things stock.ts already owns: labels, install commands, supportsCustomModel, and the external-mode key set. I checked all four and they agree today, install one-liners included. The one that fails silently when it drifts is supportsCustomModel: the Run menu builds its "CLI (endpoint)" rows from the server-injected window.__codemanCustomModelClis at :652, so a CLI that gains a customModelInjection recipe later gets offered in the menu while _runCliMode drops the field at :2059, and the session launches on the vendor cloud with the UI claiming the local endpoint. One JSDOM test that evals the file, reads RUN_MODE_LAUNCH and compares it to STOCK_CLIS covers all four at once.

3. test/frontend-cli-no-id-branching.test.ts:43 and :58 point at "Open Question 7 in PR-B2.md", and there is no PR-B2.md in the tree. I will inline the reason.

4. docs/cli-registry.md:97 still names only the backend guard. This PR adds a second CI-gated one, so it gets a sentence there.

Separately, the title and the body still describe window.__codemanCliCatalog, the server.ts injection and the types.ts change, and the merged diff is four files with no server-side change at all. Your follow-up comment explains it, but the title becomes the squash subject, so I will retitle to name the consolidation instead of the catalogue.

Merging after those, which are all mine to apply. Thank you for measuring the accent values against the rendered gradients rather than assuming them; send that as its own PR whenever suits you, and omp's shortBadge is my mistake to fix.

[opticon454]

Thank you for re-verifying with the 80-combination JSDOM harness rather than trusting the diff — that's a level of rigor I appreciate, especially on the byte-identity claims.

I went ahead and applied three of your four "will take at merge" items myself instead, since they were straightforward once I had the reproductions:

1 — count-based allowlist. You're right, and I reproduced it exactly as you described (if (this.mode === 'codex') in runOpenCode(), stayed green). Each ALLOWED_BRANCHES entry now carries { count, reason }, and a new assertion checks actual-vs-declared count per key. Re-ran your exact repro against the fix: expected 2, found 3, caught immediately with a clear diagnostic pointing at which key and by how much.

2 — the RUN_MODE_LAUNCH/stock.ts drift. Added test/run-mode-launch-table-drift.test.ts, driving the real session-ui.js via JSDOM and comparing RUN_MODE_LAUNCH against STOCK_CLIS on all four axes you named (id-set coverage, label, install command, supportsCustomModel). You're right that supportsCustomModel is the one that matters — I traced the actual consequence through to confirm: window.__codemanCustomModelClis and _runCliMode's supportsCustomModel flag would disagree silently, offering a CLI in the picker while dropping its customModel field at launch. Worth having a real test on, not just a comment.

3 — the dead PR-B2.md reference. Inlined; it now points at this review thread instead of a file that was never in the tree.

4 — the docs/cli-registry.md sentence. Added, describing the new guard's key design (no line number, count-carrying) alongside the backend one.

Left your title-change and the $&/$' escaping fix for you, as you described — both are yours to take and I don't want to step on either.

Full gate: 406 files, 7721 tests, 0 failures (net +4 from the new drift test), typecheck/lint/format:check/check:frontend-syntax all clean.

One process note, unrelated to the code: partway through this round my local checkout got switched to an unrelated branch by another session sharing the same working directory (a documented hazard in this repo's own CLAUDE.md). Caught it before committing anything, via git worktree add rather than disturbing that session's state — mentioning it only because if you see a d9e6ebb2 commit that looks like it came from a clean followups, it did; nothing from the other branch leaked in, and I checked.

[Ark0N]

Merged into master for 1.32.0, with the merge-time items applied on the way in (0f95532): the opencode-resize guard now targets the real launcher code and fails when a pre-assignment is introduced, the two dropped invariants are back as comments, the docs paragraph sits below its backend antecedents, the frontend guard now catches a comparison on any variable name, a table-driven test pins the run() dispatch, CLAUDE.md names the second guard, and every injection in server.ts uses a replacer function so a label carrying $' cannot splice the document. I also retitled the PR to name the consolidation. Thank you for the byte-identity proof and for the fast rounds.