fix(runner): fail loud on an unresolvable requested model (F-007) - #5240
Conversation
A per-request model override on the sandbox-agent path was strict only for the Claude managed-connection case. Every other path (all Pi runs) silently fell back to the harness default when the requested id was rejected, so a user who picked a model could get a different, often pricier one with no error. Make strict resolution the default on every harness path: applyModel now tries the requested id, then resolves it against the harness's own selectable ids (so a bare "gpt-5.5" reaches Pi's "openai-codex/gpt-5.5"), and only then either returns the resolved id or throws ModelNotSettableError naming the requested id and the valid options. A run that requests no model still keeps the harness default. The legacy warn-and-fallback stays available behind AGENTA_AGENT_MODEL_STRICT=false. Also fixes allowedModels to read the pi-acp choice `value` (it read `id` and silently returned []). Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughModel selection now resolves harness-supported ids, throws structured errors in strict mode, and falls back only when explicitly non-strict. Environment acquisition derives strictness independently from Claude connection setup. ChangesModel resolution behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant applyModel
participant Harness
Runner->>applyModel: apply requested model with strictness
applyModel->>Harness: set requested model
Harness-->>applyModel: reject or accept model
applyModel->>Harness: retry resolved selectable model
applyModel-->>Runner: return model or throw ModelNotSettableError
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:M
| options: { strict?: boolean } = {}, | ||
| ): Promise<string | undefined> { | ||
| if (!wanted) return undefined; | ||
| const strict = options.strict ?? true; |
There was a problem hiding this comment.
Decision: strict defaults to true on every harness path (F-007). This flips the model-config proposal's staged default (Part 2b): a requested model that cannot be set fails the run instead of silently substituting the harness default. No model requested is unaffected (early return above).
| // pi-acp builds each choice as `{ value: model.modelId, name, description }` and sandbox-agent | ||
| // reads `entry.value`; older shapes used `id`. Read `value` first so this returns the real | ||
| // selectable ids (reading only `id` silently returned [] for pi-acp). | ||
| return choices.map((c: any) => c.value ?? c.id).filter(Boolean); |
There was a problem hiding this comment.
Bug fix: pi-acp builds each choice as { value: model.modelId, ... }, so mapping c.id returned []. Reading value first surfaces the real selectable set, so the ModelNotSettableError message can name valid options (the 'valid options source').
| * no model is unaffected either way — it keeps the harness default. | ||
| */ | ||
| function modelResolutionStrict(): boolean { | ||
| return process.env.AGENTA_AGENT_MODEL_STRICT !== "false"; |
There was a problem hiding this comment.
Decision: the fallback is kept behind an explicit env flag (default strict). AGENTA_AGENT_MODEL_STRICT=false restores the legacy warn-and-fallback. Env flag (not a wire field) keeps the /run contract and goldens unchanged; strictModel is now computed here instead of piggybacking on the Claude-only applyClaudeConnectionEnv return value.
| } | ||
| } | ||
| if (strict) { | ||
| throw new ModelNotSettableError(wanted, fallbackAllowed, (err as Error).message); |
There was a problem hiding this comment.
This throw is reached only AFTER the suffix-resolution retry above fails. That ordering is the Pi selection fix: a valid bare id (gpt-5.5) resolves to the harness id and never reaches here; only a genuinely unresolvable id does. The message names the requested id and the valid options, so the failure is actionable instead of a silent fallback.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/runner/src/engines/sandbox_agent/model.ts (1)
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider setting
Error.causefor proper error chaining.The
causeparameter is only interpolated into the message string but not passed tosuper()as{ cause }. Setting the standardcauseoption preserves the original error for stack traces and programmatic inspection by downstream handlers.♻️ Proposed improvement
super( `model '${requested}' is not available on this run (${cause}). ` + `Valid models for this harness: ${options}.`, - ); + , { cause });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 308fabce-1fe5-400e-bffe-2891141f84cd
📒 Files selected for processing (3)
services/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/model.tsservices/runner/tests/unit/sandbox-agent-model.test.ts
Adds a third pickModel tier that strips a trailing context-window hint so a bare alias (sonnet, fable) resolves to the harness's reported id (sonnet[1m], fable[1m]) when that is the only variant on offer. Only widens to an equal-or-larger context; never downgrades a [1m] request. This is what makes the sonnet/fable aliases actually settle instead of failing loud under strict resolution. Also folds in a prettier reformat of daytona.ts (no logic change). Claude-Session: https://claude.ai/code/session_0127AM79khCdvD2b8BG2joZL
dosubot
Bot
added
size:L
1 participant
Context
A user picks a model in the agent playground and silently gets a different one, often the pricier default. On the sandbox-agent (ACP) path, a per-request model override was strict only for the Claude managed-connection case. Every other path (all Pi runs, and Claude without a custom base URL) took the lenient branch in
applyModel: when the harness rejected the requested id, the runner logged one line and kept the harness default. The run always succeeded, so the drop was invisible. For Claude that default is Sonnet, so a rejected id billed the expensive model. This is finding F-007 (docs/design/agent-workflows/projects/qa/findings.md).Two symptoms:
What changed
Strict resolution is now the default on every harness path. A run that requests a model either runs that model or fails loudly. This is Part 2 of the model-config proposal (
docs/design/agent-workflows/projects/model-config/proposal.md), aligned with the harness-filtered picker (agent-model-picker, PR #4815 lineage): the picker only offers harness-valid ids, and the runner is the fail-loud backstop.applyModel(services/runner/src/engines/sandbox_agent/model.ts): tries the requested id, then resolves it against the harness's own selectable ids viapickModel(so a baregpt-5.5reaches Pi'sopenai-codex/gpt-5.5), and only then either returns the resolved id or throwsModelNotSettableErrornaming the requested id and the valid options. Suffix resolution now runs before the failure, which is what makes Pi selection work.strictdefaults totrue.allowedModels(same file): read the pi-acp choicevalue(it readidand silently returned[]), so the error message and any pre-validation see the real selectable set.sandbox_agent.ts:strictModelis decoupled from the Claude-onlyapplyClaudeConnectionEnvand computed bymodelResolutionStrict(), strict by default.applyClaudeConnectionEnvkeeps its Claude env side-effect and returnsvoid.Before / after
model: "claude-haiku-4-5-20251001"on a Claude run -> rejected -> silent fallback to Sonnet, billed silently.model: "gpt-5.5"on Pi -> dropped, ran the default.model: "gpt-5.5"on Pi -> resolves to the harness id and runs. An unresolvable id -> the run fails withmodel '<id>' is not available on this run (...). Valid models for this harness: <list>.Scope / risk
protocol.tschange.AGENTA_AGENT_MODEL_STRICT=falsefor operators who need it.How to QA
Prerequisites: the runner unit suite (
services/runner, Node 24), and optionally a live EE dev stack with a Pi harness.Unit:
cd services/runner && pnpm exec vitest run tests/unit/sandbox-agent-model.test.tsModelNotSettableErrornaming the id and options;strict: false-> warn-and-fallback;allowedModelsreadsvalue.Live (Pi run):
Edge cases:
AGENTA_AGENT_MODEL_STRICT=falserestores the old fallback; a Claude alias (haiku) still resolves; a full Claude model id that no alias matches now fails loudly instead of billing Sonnet.Links finding F-007 (
docs/design/agent-workflows/projects/qa/findings.md,findings-status.md).https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj