feat(model): configurable default + per-spawn model override#81
feat(model): configurable default + per-spawn model override#81dcetlin wants to merge 3 commits into
Conversation
Model was hardcoded in two places (bridge-dispatch SPAWN_MODEL for spawns, lifecycle.ts for the byte). This makes it selectable at two levels, with precedence: `--model` (per-spawn) > HYDRA_MODEL (config default) > built-in. - daemon/bridge-dispatch.ts: SPAWN_MODEL = HYDRA_MODEL ?? DEFAULT_MODEL - cli/lifecycle.ts: byte reads HYDRA_MODEL - daemon/session-lifecycle.ts + sessions.ts: SpawnOpts.model overrides per spawn - cli/hydra.ts + daemon/cli-handler.ts: `hydra spawn --model <id>` - .env.example: document HYDRA_MODEL The daemon self-loads the state .env (config.ts), so HYDRA_MODEL reaches spawns without touching buildDaemonEnvs. Do nothing → identical to today. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sf8193
left a comment
There was a problem hiding this comment.
PR #81 Review — configurable default + per-spawn model override
Clean, well-scoped change. The fallback chain (--model > HYDRA_MODEL > DEFAULT_MODEL) is sound. Two should-fix items and a few nits below.
Items not on diff lines
Should-fix: bridge-server.ts fallback capabilities still use SPAWN_MODEL — When a session reconnects without stored capabilities, the fallback model field uses the global SPAWN_MODEL constant, not the model the session was actually spawned with. After this PR, a session spawned with --model sonnet would have its capabilities lie on reconnect. Worth fixing the fallback path or ensuring stored capabilities are always restored.
Nit: spawn_session bridge tool has no model passthrough — Only CLI-initiated spawns get the per-spawn override. Byte-initiated spawns via the spawn_session bridge tool always use SPAWN_MODEL. If this is intentional, a comment noting it would help. If byte should also be able to pick models, the tool schema needs a model field.
| } | ||
|
|
||
| const byteCwd = process.env.BYTE_CWD ?? cfg.spawnCwd | ||
| // Canonical default is DEFAULT_MODEL in daemon/bridge-dispatch.ts; keep in sync. |
There was a problem hiding this comment.
Should-fix (flagged by both reviewers): duplicate default constant. The hardcoded 'claude-opus-4-6[1m]' here duplicates DEFAULT_MODEL in bridge-dispatch.ts. The comment says "keep in sync" — that's how things get out of sync. Import DEFAULT_MODEL instead, or extract to a shared constants.ts. The CLI already cross-imports types from daemon/sessions.ts, so this is established practice.
There was a problem hiding this comment.
Fixed — imported DEFAULT_MODEL from bridge-dispatch.ts and removed the hardcoded string + "keep in sync" comment.
| @@ -361,14 +361,16 @@ export async function doSpawnSession(topic: string, chatId?: string, messageId?: | |||
| prompt = buildSpawnPrompt(promptParams) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Should-fix: empty-string model bypasses fallback. If --model "" is passed, the ?? operator doesn't catch it (empty string is not nullish), so shq('') produces '' and the spawned claude --model '' will fail opaquely. Use opts?.model || SPAWN_MODEL (falsy check), or validate at the CLI parsing layer in hydra.ts.
There was a problem hiding this comment.
Fixed — switched to || (falsy check) so empty string falls through to SPAWN_MODEL. Also applied the same fix to the SPAWN_MODEL env-read itself in bridge-dispatch.ts:58 which had the same latent issue.
| export const SPAWN_MODEL = 'claude-opus-4-6[1m]' | ||
| // Canonical default. HYDRA_MODEL overrides it for byte + all spawns; | ||
| // `hydra spawn --model <id>` overrides a single spawn. | ||
| export const DEFAULT_MODEL = 'claude-opus-4-6[1m]' |
There was a problem hiding this comment.
Nit: import-time-frozen env read. process.env.HYDRA_MODEL is read once when this module is first imported. For a long-running daemon, changing the env var requires a full daemon restart. This is likely fine in practice (daemon restarts are the norm), but worth either a brief comment noting it's intentionally frozen, or converting to a getSpawnModel() function if hot-reload is ever desired.
There was a problem hiding this comment.
Leaving as-is — daemon restarts are the norm for config changes, so import-time freeze is intentional. The constant name + declaration position make this self-evident.
| if (filtered[i] === '--idempotency-key' && i + 1 < filtered.length) { | ||
| idempotencyKey = filtered[++i] | ||
| } else if (filtered[i] === '--initiator' && i + 1 < filtered.length) { | ||
| initiator = filtered[++i] |
There was a problem hiding this comment.
Nit: --model as last arg silently swallowed into prompt. If --model is the final argument (no value follows), the i + 1 < filtered.length guard fails and "--model" gets pushed into promptParts. This matches the existing pattern for --initiator/--idempotency-key so it's consistent — but all three share the same latent issue. Not a blocker for this PR.
There was a problem hiding this comment.
Agreed — pre-existing pattern shared by all flag args. Out of scope for this PR but noted for a future arg-parsing cleanup pass.
…ion model passthrough - Import DEFAULT_MODEL in lifecycle.ts instead of duplicating the string - Use || (falsy) instead of ?? (nullish) so empty-string model falls back correctly - Add model field to spawn_session bridge tool so byte can select models for spawns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Same empty-string bypass that was fixed for opts.model also existed at the env-read level — HYDRA_MODEL="" would resolve to "" instead of DEFAULT_MODEL. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Re: bridge-server.ts fallback capabilities — Investigated. Capabilities are always set during spawn and persisted to disk, so the fallback only fires for sessions predating the capabilities field (migration edge case). Re: spawn_session model passthrough — Fixed. Added Bonus fix: the |
| compileCheck, killOrphanBytes, hasOrphanBytes, appendLog, shq, | ||
| waitForSocket, buildDaemonEnvs, | ||
| } from './helpers.js' | ||
| import { DEFAULT_MODEL } from '../daemon/bridge-dispatch.js' |
There was a problem hiding this comment.
Should-fix (flagged by both reviewers): Cross-boundary import — CLI layer imports from daemon/bridge-dispatch.js. This creates a transitive dependency on the daemon module graph at CLI load time. If bridge-dispatch.ts gains daemon-only side effects or imports, this breaks the CLI entrypoint.
Suggested fix: extract DEFAULT_MODEL into a shared constants module (e.g. shared/constants.ts) that both CLI and daemon import.
| // Canonical default. HYDRA_MODEL overrides it for byte + all spawns; | ||
| // `hydra spawn --model <id>` overrides a single spawn. | ||
| export const DEFAULT_MODEL = 'claude-opus-4-6[1m]' | ||
| export const SPAWN_MODEL = process.env.HYDRA_MODEL || DEFAULT_MODEL |
There was a problem hiding this comment.
Nit (flagged by both reviewers): SPAWN_MODEL is evaluated once at module load time. Changes to HYDRA_MODEL after import are invisible for the lifetime of the daemon process. This is fine if restart-to-reconfigure is the contract — worth a brief doc comment if so.
Also: the fallback || DEFAULT_MODEL is duplicated in cli/lifecycle.ts:69. Centralizing in a shared module (per the lifecycle.ts comment) would eliminate this.
| prompt = buildSpawnPrompt(promptParams) | ||
| } | ||
|
|
||
| const model = opts?.model || SPAWN_MODEL |
There was a problem hiding this comment.
Should-fix (flagged by both reviewers): Use ?? instead of || here. With ||, an empty string model: '' (possible from the bridge tool JSON path) silently falls back to SPAWN_MODEL. With ??, only undefined/null trigger the fallback — an empty string would propagate and fail visibly at the claude CLI, which is the correct behavior for a bad input.
const model = opts?.model ?? SPAWN_MODEL| const worktree = args.worktree as string | undefined | ||
| const topic = worktree ? `worktree:${worktree} ${args.topic}` : args.topic as string | ||
| const result = await doSpawnSession(topic, args.chat_id as string | undefined, args.message_id as string | undefined) | ||
| const model = args.model as string | undefined |
There was a problem hiding this comment.
Should-fix: No guard against empty-string model from LLM callers. The bridge receives arbitrary JSON from Claude — { "model": "" } passes the model ? { model } : undefined ternary (empty string is falsy, so this specific line is OK), but consider normalizing: const model = (args.model as string | undefined)?.trim() || undefined to be defensive.
Also: no validation or logging of the model override. A misbehaving session could request a non-existent model ID, causing a silent tmux failure. At minimum, logging the override would aid debugging.
| --idempotency-key <key> Prevent duplicate spawns | ||
| Spawn options: | ||
| --initiator <name> Who triggered this spawn (required) | ||
| --idempotency-key <key> Prevent duplicate spawns (required) |
There was a problem hiding this comment.
Nit: Users reading CLI help won't know what HYDRA_MODEL resolves to when unset. Consider: (default: $HYDRA_MODEL or claude-opus-4-6[1m]).
What
Make the model selectable instead of hardcoded in two places (
bridge-dispatch.SPAWN_MODELfor spawns,cli/lifecycle.tsfor the byte).Precedence:
--model(per-spawn) >HYDRA_MODEL(config default) > built-inclaude-opus-4-6[1m].Changes
daemon/bridge-dispatch.ts—DEFAULT_MODELconstant;SPAWN_MODEL = process.env.HYDRA_MODEL ?? DEFAULT_MODELcli/lifecycle.ts— byte launch readsHYDRA_MODELdaemon/sessions.ts—SpawnOpts.model?daemon/session-lifecycle.ts—opts.model ?? SPAWN_MODELacross the 3 spawn/fork/resume branches + the registrymodelfielddaemon/cli-handler.ts+cli/hydra.ts—hydra spawn <prompt> --model <id>.env.example— documentHYDRA_MODELWhy no
buildDaemonEnvschangedaemon/config.tsself-loads both the repo.envand the state-dir.envat import, soHYDRA_MODELset in.envreachesSPAWN_MODELin the daemon automatically.Verification
bun buildofcli/hydra.ts,daemon.ts,bridge.tsall pass.claude --model …string is byte-for-byte unchanged.HYDRA_MODEL=X→ byte and spawns launch with--model X.hydra spawn … --model Y→ that session launches with--model Yregardless ofHYDRA_MODEL.🤖 Generated with Claude Code