From 6acf0dea0f57506871bc6181ef3da626877f3498 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Wed, 2 Sep 2026 08:49:04 +0800 Subject: [PATCH] fix(cron): scope the launch pre-flight to launcher CLIs, not every mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI 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 Claude-Session: https://claude.ai/code/session_01WQkoi1cNegqVwZHgzx5SbJ --- .changeset/cli-registry-core.md | 7 +++++- src/cron/cron-service.ts | 39 +++++++++++++++++++++------------ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/.changeset/cli-registry-core.md b/.changeset/cli-registry-core.md index 2ab839bb..c33a2e62 100644 --- a/.changeset/cli-registry-core.md +++ b/.changeset/cli-registry-core.md @@ -28,7 +28,7 @@ env-prefix validation, the doctor's tool list, and each resolver's search direct re-read the catalog, so a CLI enabled while the server is running moves every surface at once rather than only the run menu. -Three user-visible changes, all small and all deliberate: +Four user-visible changes, all small and all deliberate: - `probeDockerCliVersion()` derives the in-container binary name from the registry rather than assuming it equals the mode name. Only Claude reaches that path today, so nothing was @@ -38,6 +38,11 @@ Three user-visible changes, all small and all deliberate: replaces simply omitted — its own comment said the rule was "every mode except shell", so the two were an oversight from when those CLIs were added, and a remote session in either mode reported no version at all. +- OMP now requires tmux like its seven siblings. `session.ts` carried a hand-written list of + modes with no direct-PTY fallback and omp was missing from it, even though CLAUDE.md's own + text says "all eight require tmux" — so an omp session whose mux creation failed silently + fell back to a direct PTY. `requiresMux` comes off the entry now, so the list cannot drift + from the rule again. - `codeman doctor`'s CLI rows come from the registry, so Claude's install hint is now the documented install command rather than a docs URL, five CLIs gain install hints they never had, and the row order follows the catalog (Claude now sorts below tmux). diff --git a/src/cron/cron-service.ts b/src/cron/cron-service.ts index d6c26282..cce014ee 100644 --- a/src/cron/cron-service.ts +++ b/src/cron/cron-service.ts @@ -414,26 +414,37 @@ export class CronService { let session: Session; try { const mode = job.agentType; - // Refuse a launch the CLI cannot survive, rather than opening a dead pane. The - // launcher CLIs answer with their own specific reason (for dsh: binary missing, no - // pane-capable profile, or the named profile cannot drive a pane); ordinary CLIs - // answer with the resolver's not-found message. Cron sends no per-CLI config, so - // there is no caller-named target to report on. - const cronLaunchError = await resolveCliLaunchError(mode); - if (cronLaunchError) return this.failRun(job, run, cronLaunchError); + // A LAUNCHER CLI's binary is not its agent, so "installed" is not "runnable": without + // this, a job on a box carrying only dsh's stock web/headless profiles spawns a bare + // `dsh` that boots a profile unable to drive a pane, and the prompt is typed into a + // logging server or a dead pane instead of failing the run with an actionable message. + // + // ⚠️ Scoped to `discovery.launcherProfile`, which is byte-identical to the + // `mode === 'deepseek'` check this replaces (dsh is the only launcher today) and + // generalises to the next one. Deliberately NOT every CLI: cron has never pre-flighted + // a merely-missing binary, and doing so replaces tmux-manager's own not-found throw + // ("Session launch failed") with a different message for claude and shell. An earlier + // draft of this line was unscoped and did exactly that — three cron tests caught it. + if (getCli(mode)?.discovery.launcherProfile !== undefined) { + const cronLaunchError = await resolveCliLaunchError(mode); + if (cronLaunchError) return this.failRun(job, run, cronLaunchError); + } const globalNice = await this.deps.getGlobalNiceConfig(); const modelConfig = await this.deps.getModelConfig(); const claudeModeConfig = await this.deps.getClaudeModeConfig(); const effectiveClaudeMode = await resolveClaudeModeForUsername(claudeModeConfig.claudeMode, job.owner); // Cron carries no per-CLI config object, so the only model it can supply is the global - // default — and only to a CLI that takes one that way. `capabilities.model` is the same - // question the HTTP routes ask; the ladder it replaces named `shell` and `deepseek` by - // hand and had to be edited in step with them (deepseek's model is a profile - // composition entry, not a session flag). + // default — and only to a CLI that takes a model at all. + // + // ⚠️ `!== 'none'` is the faithful reading of the `mode !== 'shell' && mode !== 'deepseek'` + // ladder this replaces: those two are exactly the entries declaring `model.source: 'none'` + // (shell has no model; deepseek's is a profile composition entry, not a session flag). + // NOT `=== 'claude-settings-file'`, which is the HTTP route's question — 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 + // means something different here. const model = - getCli(mode)?.capabilities.model.source === 'claude-settings-file' - ? modelConfig?.defaultModel || undefined - : undefined; + getCli(mode)?.capabilities.model.source !== 'none' ? modelConfig?.defaultModel || undefined : undefined; // Section 6.3: materialize the safe default for a non-granted owner (see // clampCronExternalCliConfigs — cron sends no per-CLI config, so the CLI's own // spawn default is what would otherwise apply).