Skip to content

fix(session): tolerate pre-#24512 model JSON shape in fromRow#26572

Closed
kitlangton wants to merge 1 commit into
devfrom
kit/fix-26435-legacy-session-model
Closed

fix(session): tolerate pre-#24512 model JSON shape in fromRow#26572
kitlangton wants to merge 1 commit into
devfrom
kit/fix-26435-legacy-session-model

Conversation

@kitlangton
Copy link
Copy Markdown
Contributor

Closes #26435.

What broke

Sessions store their selected model as a JSON-typed SQLite column. On 2026-05-02, commit a3bc5d35b (PR #24512 "Refactor v2 session events as schemas" by @thdxr) renamed the JSON shape:

Before May 2 After May 2
{ "providerID": "opencode", "modelID": "big-pickle" } { "id": "big-pickle", "providerID": "opencode", "variant": "..." }

The PR updated:

  • the Drizzle column annotation in session.sql.ts from $type<{ providerID, modelID }>() to $type<{ id, providerID, variant }>()
  • fromRow in session.ts to read row.model.id

But $type<>() is a compile-time-only TypeScript cast. It does not transform, validate, or migrate the underlying JSON bytes. No data migration was added, so existing on-disk rows still contain { providerID, modelID }.

When fromRow reads one of those legacy rows it computes ModelID.make(row.model.id)row.model.id is undefined, the Schema.brand("ModelID") decoder throws Expected string, got undefined, and the whole Session.list call rejects (no per-row containment in listByProject). One bad row hides every other valid session.

Why this slipped through review

  • The TypeScript type changed; the storage shape didn't — the PR's diff looked self-consistent at the type level. CI tests use freshly-created rows, which already use the new shape, so they all pass.
  • Read-only paths that fetch a session by ID (the common case during a normal chat session) don't necessarily read legacy rows.
  • The crash only fires for users who: (a) had at least one session created before 2026-05-02, AND (b) ran opencode session list or anything else that hits listByProject. Many users create a fresh session each chat and never trigger list.
  • The bug surfaced the moment someone with old rows ran the list command on Windows (session list crashes on legacy sessions with modelID key (expected id) #26435).

The fix

fromRow reads row.model.id ?? row.model.modelID so legacy rows decode using the old field name. Newly-written rows still use the new shape; this is read-side compatibility, not a write-side change. No data migration needed; no risk of corrupting existing data.

Reproducer

packages/opencode/test/session/session-legacy-model-shape.test.ts creates a session through the normal API, then directly mutates its model column to the legacy shape, then asserts Session.list returns it (and a sibling valid session) without crashing.

Verified red → green → red → green: pre-fix the test fails with the exact Expected string, got undefined error; post-fix it passes; reverting the fix makes it fail again.

Follow-up worth considering (not in this PR)

  • Per-row containment in listByProject so a corrupt row never tanks the whole list, regardless of shape.
  • A one-time migration that walks SessionTable and rewrites legacy model JSON to the new shape, eventually letting us drop this fallback.

Closes #26435.

Sessions created before #24512 (commit a3bc5d3, 2026-05-02) wrote
the `model` column as { providerID, modelID }. That PR renamed the
shape to { id, providerID, variant } and updated `fromRow` to read
row.model.id, but only updated the Drizzle \$type<>() annotation —
the on-disk JSON of existing rows was not migrated. fromRow now
crashes on those legacy rows with 'Expected string, got undefined',
killing the whole session list (no per-row containment).

Fall back to row.model.modelID when row.model.id is absent. Newly
written rows still use the new shape; this only affects reads of
legacy data.
@kitlangton
Copy link
Copy Markdown
Contributor Author

Closing for further investigation. Review agents flagged factual issues with the PR's root-cause story, missing coverage in v2/session.ts:132, and an unsafe ?? "" fallback. Need to first establish how a real user ends up with {providerID, modelID} shape in the new model column — the column was added (not renamed) on 2026-05-02, so BennD's data must have come from a migration/import path or a non-mainline build. Will reopen with corrected story and fuller fix once that's clear.

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.

session list crashes on legacy sessions with modelID key (expected id)

1 participant