fix(cli): keep models.dev refresh errors off the TUI - #13107
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (3 files)
lgtm Previous Review Summary (commit 8aa53f9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8aa53f9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
The core fix looks correct: capturing Reviewed by kimi-k3 · Input: 40.6K · Output: 2.9K · Cached: 153.6K Review guidance: REVIEW.md from base branch |
Capture CurrentLoggers at ModelsDev construction so a failed catalog refresh cannot dump Effect's default logger onto the TUI tty.
8aa53f9 to
e5c9ef1
Compare
Capture CurrentLoggers at ModelsDev construction so a failed catalog refresh cannot dump Effect's default logger onto the TUI tty.
Issue
No existing gh issue. A live TUI session when
models.devwas unreachable: Effect's default logger dumped[HH:MM:SS.mmm] ERROR (#N): Failed to fetch models.dev { cause: { _id: "Cause", ... } }onto the tty and overwrote the prompt footer (Claude Opus 5 Kilo Gateway · highbecame_id: "Cause",de Opus 5 Kilo Gateway · high).Context
The TUI listener builds a fresh ModelsDev graph and only
Layer.providesAppLayer. That does not install Observability into the ModelsDev construction fiber, so a failed catalog refresh uses Effect's default pretty logger and writes to the tty.AppLayer/createRoutesalreadyprovideMerge(Observability.layer)last and do not leak.Implementation
ModelsDev.nodenow depends onObservability.node. Construction capturesLogger.CurrentLoggersonce andrefresh()reuses that set. Failed fetches still log at Error to the file/OTLP loggers; they no longer rebuild Observability or dump to the TUI.Screenshots / Video
N/A — the leaked line is the Effect defaultLogger dump above. No layout or styling change.
How to Test
Manual/local verification
bun test ./test/kilocode/models-dev-logger.test.tsinpackages/core— 1 pass.Received: "... ERROR (#2):\nFailed to fetch models.dev\n[object Object]"; with the fix it passes.bun test ./test/models.test.ts ./test/plugin/models-dev.test.ts ./test/kilocode/models-dev-logger.test.tsinpackages/core— 12/12.bun test ./test/provider/provider-model-refresh.test.ts ./test/provider/provider.test.tsinpackages/opencode— 100/100 (from the independent review).bun run script/check-opencode-annotations.ts --worktreepassed.Reviewer test steps
Reproduce the leak on
main/ this branch's parent (8013e5f504):packages/core, force a failedModelsDev.refresh(true)against a 500https://models.dev/api.jsonresponse withoutLayer.provideMerge(Observability.layer)— the TUI listener shape isLayer.fresh(listener).pipe(Layer.provide(AppLayer)).[HH:MM:SS.mmm] ERROR (#N): Failed to fetch models.dev { cause: { _id: "Cause", failures: [[Object ...]] } }to the console.opencode.log).kilowith outboundmodels.devblocked (sandbox / offline). The prompt footer must stay intact instead of being overwritten by_id: "Cause",.Minimal unit check:
That test fails on HEAD and passes here.
Checklist