fix(agents): resolve model aliases in sessions_spawn#59681
Conversation
Greptile SummaryThis PR fixes a real bug where passing a user-defined model alias (e.g. Key observations:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 522
Comment:
**Dead fallback `?? raw` is unreachable**
`raw` is produced by a `??`-chain whose final branch is the always-non-empty string `\`${runtimeDefault.provider}/${runtimeDefault.model}\``. Because `raw` can never be falsy, `resolveModelThroughAliases` will never hit its `if (!value) return undefined` guard, so the `?? raw` at the end of the return statement can never be evaluated.
This is harmless today, but it creates a misleading implication that the function could return `undefined` for a non-empty `raw`, which may confuse future maintainers. Consider adjusting the helper's return type to `string` when `value` is guaranteed non-empty — or, at minimum, add a comment documenting why the fallback exists (purely as a type-safety net, not reachable at runtime).
```suggestion
return resolveModelThroughAliases(raw, params.cfg) ?? /* unreachable; raw is always non-empty */ raw;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/model-selection.ts
Line: 493-496
Comment:
**Alias index rebuilt on every call**
`buildModelAliasIndex` iterates over `cfg.agents?.defaults?.models` each time it is called. `resolveSubagentSpawnModelSelection` already calls `resolveDefaultModelForAgent` → `resolveConfiguredModelRef` → `buildModelAliasIndex`, so the index is built at least twice per spawn call (once in `resolveDefaultModelForAgent`, once here).
For a hot spawn path this is wasted work. Consider accepting a pre-built `ModelAliasIndex` as an optional parameter (matching the pattern already used in `resolveModelRefFromString` and `resolveAllowedModelRef`), or building the index once in `resolveSubagentSpawnModelSelection` and passing it down.
```ts
// Example signature change
function resolveModelThroughAliases(
value: string | undefined,
cfg: OpenClawConfig,
aliasIndex?: ModelAliasIndex, // accept pre-built index
): string | undefined {
// ...
const index = aliasIndex ?? buildModelAliasIndex({ cfg, defaultProvider: DEFAULT_PROVIDER });
// ...
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): resolve model aliases in se..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
martingarramon
left a comment
martingarramon
left a comment
There was a problem hiding this comment.
Clean fix — the alias resolution logic is straightforward and the tests cover the main paths well.
One question: what's the expected behavior when modelOverride is a bare string that isn't a configured alias? Currently resolveModelThroughAliases returns it unchanged, so the gateway would get something like "typo-model" and presumably fail. Is that the right failure mode, or should this surface an error earlier (e.g., during spawn validation)?
Passing unknown strings through unchanged is intentional — it matches how resolveConfiguredModelRef and resolveAllowedModelRef already handle unrecognized values elsewhere in the codebase. The gateway's model routing layer is the right place to surface "unknown model" errors because it has the full provider catalog context to produce a useful diagnostic (e.g., "did you mean X?"). Adding validation at the spawn layer would duplicate that logic and create a second error surface to maintain. TL;DR: fail late at the routing layer where the error message seems better, not early at spawn where we'd just say "unknown model." |
7d65a5d to
054243d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 054243d0e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Makes sense — matches the pattern. LGTM. |
@martingarramon - Thanks! I've addressed the Codex P1/P2 feedback in a follow-up PR:
Seems ready to merge. |
normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes #57532 Refs #50736
- Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot]
054243d to
da9c613
Compare
|
ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical. Source PR: #59681 |
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
* fix(agents): resolve model aliases in sessions_spawn normalizeModelSelection() only trims the input — it never resolves aliases through the model alias index. When a user passes an alias like 'opus' to sessions_spawn, the child session gets patched with the raw string, which the gateway cannot match to any provider. Add resolveModelThroughAliases() to check bare strings against the configured alias map before returning from resolveSubagentSpawnModelSelection(). Fixes openclaw#57532 Refs openclaw#50736 * refactor: address review feedback on alias resolution - Accept pre-built ModelAliasIndex instead of rebuilding per call - Narrow helper signature to (string, ModelAliasIndex) → string - Remove unreachable ?? raw fallback Co-Authored-By: greptile-apps[bot] * fix(agents): resolve sessions_spawn model aliases --------- Co-authored-by: HowdyDooToYou <[email protected]> Co-authored-by: vincentkoc <[email protected]>
3 participants
Summary:
resolveSubagentSpawnModelSelection() uses normalizeModelSelection() to process the model override — but that function only trims the string. It never checks the configured model alias index. When a user passes an alias like "opus" or "gpt" to sessions_spawn, the child session gets patched with the raw alias string, which the gateway cannot resolve to any provider. The child session dies immediately.
Changes:
Added resolveModelThroughAliases() helper in src/agents/model-selection.ts that checks bare strings against buildModelAliasIndex. Already-qualified provider/model refs and unknown strings pass through unchanged.
Modified resolveSubagentSpawnModelSelection() to pass its result through resolveModelThroughAliases() before returning.
Test plan:
4 new tests in model-selection.test.ts:
Alias override resolves to full provider/model ref
Configured subagent model alias resolves
Already-qualified refs pass through unchanged
Falls back to runtime default when no override
All 9 existing spawn model tests pass
pnpm tsgo — clean
Pre-commit hooks pass (lint, typecheck, conflict markers)
Fixes #57532
Refs #50736