Skip to content

fix(tui): guard model parse and handle variant selection - #48978

Open
marwanvx wants to merge 2 commits into
anomalyco:v2from
marwanvx:fix-tui-model-split
Open

marwanvx wants to merge 2 commits into
anomalyco:v2from
marwanvx:fix-tui-model-split

Conversation

@marwanvx

@marwanvx marwanvx commented Sep 14, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #48957

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

  • Guards parse in util/model.ts against non-string, undefined, and empty inputs to prevent startup crashes (U.split is not a function).
  • Parses #variant syntax in util/model.ts so model identifiers in the format provider/model#variant cleanly separate modelID from variant, ensuring downstream catalog lookups succeed.
  • Validates configured model documents in context/local.tsx so malformed or incomplete config entries return undefined instead of leaking { modelID: undefined }.
  • Preserves model variants from fallbackModel into preferredSelection while keeping in-session user variant switches authoritative.
  • Guards switchLabel against incomplete model descriptors.
  • Adds comprehensive unit tests for parse edge cases and regression tests in local-selection.test.tsx.

How did you verify your code works?

  • bun test test/util/model.test.ts test/context/local-selection.test.tsx from packages/tui: 18 passed (51 assertions).
  • bun test from packages/tui: 1335 passed (0 failed).
  • bun run typecheck from repository root: passed across all 41 packages.
  • oxlint: 0 errors, 0 warnings.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

…8957)

- Harden parse in util/model.ts against non-string, undefined, and empty inputs to avoid crashes (U.split is not a function).
- Parse #variant syntax from model identifiers so modelID remains clean and catalog lookups succeed.
- Validate configured model documents in context/local.tsx so malformed or incomplete entries cleanly return undefined rather than leaking { modelID: undefined }.
- Thread variant through fallbackModel and preferredSelection while keeping in-session user variant switches authoritative.
- Guard switchLabel against incomplete model descriptors.
- Add unit and regression tests for model parsing and local selection.

Closes anomalyco#48957
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@holny holny left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the two test files against head locally (18 pass), and the malformed-config hardening is a real fix — configured.model as a number or null previously threw on property access in configuredModel. Three things worth a look:

  1. CLI #variant loses to the stored per-model variant preference. In preferredSelection the chain is preferences.variant[key] ?? model.variant ?? ..., so with a saved variant for that model, --model provider/second#high is silently ignored. On top of that, the new local.model.variant.set(variant) in app.tsx onMount writes the CLI variant into the preference store, so one launch with #high becomes the default for later launches that pass no flag. Mirroring recents might be intended, but taken together the preferred ?? model.variant ordering is mostly unexercised and there's no test covering stored-preference + CLI-variant together — worth deciding explicitly which one wins.

  2. That variant.set call is a silent no-op unless currentSelection() resolves at mount (it early-returns without a selection, which needs the models list loaded). The variant still reaches the selection later via fallbackModel → preferredSelection, but the preference/draft side effects get skipped depending on timing. Deriving persistence from the loaded selection pipeline instead of an imperative set at mount might be more predictable.

  3. util/model.ts parse now reimplements the provider/model#variant split, but packages/schema already ships the canonical parser (Model.Ref.parse, used by the CLI via parseSessionTargetModel) with stricter rules — it rejects an empty variant and # inside provider/variant, while the TUI parse accepts both (provider/mo#de#l parses to variant de#l). Benign today since preferredSelection normalizes unknown variants away, but two parsers for one syntax will drift; tui already depends on @opencode/schema, so reusing Ref.parse with a soft fallback may end up less code.

Minor: preferredSelection's new parameter type is the same shape as the existing ModelSelection alias.

@marwanvx

Copy link
Copy Markdown
Author

All four addressed.

  1. CLI #variant now wins transiently. preferredSelection checks the CLI memo first, app.tsx no longer calls variant.set, so model.json is never overwritten by a flag. recent history still updates. Added CLI variant wins transiently over stored preference without persisting covering stored-low + CLI-high → selects high, file stays low, then in-session switch to low sticks.
  2. Removed both imperative set calls from onMount. CLI flows through a cliModel memo validated against the loaded catalog, with a recents-only effect after models() + preferences.ready. No timing dependency. Explicit selectModel\ /variant.setsetscliSuperseded` so later user switches beat the launch flag.
  3. util/model.ts delegates to Model.Ref.parse with empty-on-invalid fallback, preserving the non-string crash guard. Strict rules now apply: trailing # and multi-# are invalid. Tests updated.

Minor: preferredSelection takes ModelSelection.

Verified: target files 20 pass, tui typecheck clean, oxlint clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants